On 10/27/2016 05:09 AM, David Gibson wrote:
> On Wed, Oct 26, 2016 at 09:13:18AM +0200, Cédric Le Goater wrote:
>> On 10/25/2016 07:08 AM, David Gibson wrote:
>>> On Sat, Oct 22, 2016 at 11:46:44AM +0200, Cédric Le Goater wrote:
>>>> This provides access to the MMIO based Interrupt Presentation
>>>> Controllers (ICP) as found on a POWER8 system.
>>>>
>>>> A new XICSNative class is introduced to hold the MMIO region of the
>>>> ICPs. Each thread of the system has a subregion, indexed by its PIR
>>>> number, holding a XIVE (External Interrupt Vector Entry). This
>>>> provides a mean to make the link with the ICPState of the CPU.
>>>>
>>>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>>>> ---
>>>>
>>>> Changes since v4:
>>>>
>>>> - replaced the pir_able by memory subregions using an ICP.
>>>> - removed the find_icp() and cpu_setup() handlers which became
>>>> useless with the memory regions.
>>>> - removed the superfluous inits done in xics_native_initfn. This is
>>>> covered in the parent class init.
>>>> - took ownership of the patch.
>>>>
>>>> default-configs/ppc64-softmmu.mak | 3 +-
>>>> hw/intc/Makefile.objs | 1 +
>>>> hw/intc/xics_native.c | 304
>>>> ++++++++++++++++++++++++++++++++++++++
>>>> include/hw/ppc/pnv.h | 19 +++
>>>> include/hw/ppc/xics.h | 24 +++
>>>> 5 files changed, 350 insertions(+), 1 deletion(-)
>>>> create mode 100644 hw/intc/xics_native.c
>>>>
>>>> diff --git a/default-configs/ppc64-softmmu.mak
>>>> b/default-configs/ppc64-softmmu.mak
>>>> index 67a9bcaa67fa..a22c93a48686 100644
>>>> --- a/default-configs/ppc64-softmmu.mak
>>>> +++ b/default-configs/ppc64-softmmu.mak
>>>> @@ -48,8 +48,9 @@ CONFIG_PLATFORM_BUS=y
>>>> CONFIG_ETSEC=y
>>>> CONFIG_LIBDECNUMBER=y
>>>> # For pSeries
>>>> -CONFIG_XICS=$(CONFIG_PSERIES)
>>>> +CONFIG_XICS=$(or $(CONFIG_PSERIES),$(CONFIG_POWERNV))
>>>> CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
>>>> +CONFIG_XICS_NATIVE=$(CONFIG_POWERNV)
>>>> CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>>>> # For PReP
>>>> CONFIG_MC146818RTC=y
>>>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>>>> index 2f44a2da26e9..e44a29d75b32 100644
>>>> --- a/hw/intc/Makefile.objs
>>>> +++ b/hw/intc/Makefile.objs
>>>> @@ -34,6 +34,7 @@ obj-$(CONFIG_RASPI) += bcm2835_ic.o bcm2836_control.o
>>>> obj-$(CONFIG_SH4) += sh_intc.o
>>>> obj-$(CONFIG_XICS) += xics.o
>>>> obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
>>>> +obj-$(CONFIG_XICS_NATIVE) += xics_native.o
>>>> obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>>>> obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
>>>> obj-$(CONFIG_S390_FLIC) += s390_flic.o
>>>> diff --git a/hw/intc/xics_native.c b/hw/intc/xics_native.c
>>>> new file mode 100644
>>>> index 000000000000..bbdd786aeb50
>>>> --- /dev/null
>>>> +++ b/hw/intc/xics_native.c
>>>> @@ -0,0 +1,304 @@
>>>> +/*
>>>> + * QEMU PowerPC PowerNV machine model
>>>> + *
>>>> + * Native version of ICS/ICP
>>>> + *
>>>> + * Copyright (c) 2016, IBM Corporation.
>>>> + *
>>>> + * This library is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU Lesser General Public
>>>> + * License as published by the Free Software Foundation; either
>>>> + * version 2 of the License, or (at your option) any later version.
>>>> + *
>>>> + * This library is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>>> + * Lesser General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU Lesser General Public
>>>> + * License along with this library; if not, see
>>>> <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qapi/error.h"
>>>> +#include "qemu-common.h"
>>>> +#include "cpu.h"
>>>> +#include "hw/hw.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qapi/error.h"
>>>> +
>>>> +#include "hw/ppc/fdt.h"
>>>> +#include "hw/ppc/xics.h"
>>>> +#include "hw/ppc/pnv.h"
>>>> +
>>>> +#include <libfdt.h>
>>>> +
>>>> +static void xics_native_reset(void *opaque)
>>>> +{
>>>> + device_reset(DEVICE(opaque));
>>>> +}
>>>> +
>>>> +static void xics_native_initfn(Object *obj)
>>>> +{
>>>> + qemu_register_reset(xics_native_reset, obj);
>>>> +}
>>>
>>> I think we need to investigate why the xics native is not showing up
>>> on the SysBus. As a "raw" MMIO device, it really should.
>>
>> Well, it has sysbus mmio region, but it is not created with qdev_create(...)
>> so it is not under sysbus and the reset does not get called. That is my
>> understanding of the problem.
>>
>> May be we shouldn't be using a sysbus mmio region ?
>
> Yeah, maybe not. We don't really fit the sysbus model well.
>
> I do kind of wonder if the xics object should be an mmio device at
> all, or if just the individual ICPs should be. But that might make
> for more trouble.
A cleanup brings another :) It is true that xics native does not
have any controls. It is just a container to hold the array of ICPs
and so each of these could well be a child object of PnvCore. Well,
of a PnvThread but we don't have that today.
I am going to move the container region to PnvChip to start with and
if I can the ICP regions to PnvCore. When we realize the PnvCore, we
have the xics, but I need to find a way to grab the ICPState from it.
I might need to use the cpu_index for that or could I change
xics_cpu_setup() to return an 'ICPState *' ?
I would prefer the ICP to be a PnvCore/Thread child object but that
is too early I think.
So if that comes well together, we will get rid of XICSNative and we
will use a XICSState for its ICP array.
>>> If it was, device_reset should be called without these shenannigans.
>>
>> yes.
>>
>>
>>>> +
>>>> +static uint64_t xics_native_read(void *opaque, hwaddr addr, unsigned
>>>> width)
>>>> +{
>>>> + ICPState *icp = opaque;
>>>> + bool byte0 = (width == 1 && (addr & 0x3) == 0);
>>>> + uint64_t val = 0xffffffff;
>>>> +
>>>> + switch (addr & 0xffc) {
>>>> + case 0: /* poll */
>>>> + val = icp_ipoll(icp, NULL);
>>>> + if (byte0) {
>>>> + val >>= 24;
>>>> + } else if (width != 4) {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 4: /* xirr */
>>>> + if (byte0) {
>>>> + val = icp_ipoll(icp, NULL) >> 24;
>>>> + } else if (width == 4) {
>>>> + val = icp_accept(icp);
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 12:
>>>> + if (byte0) {
>>>> + val = icp->mfrr;
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 16:
>>>> + if (width == 4) {
>>>> + val = icp->links[0];
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 20:
>>>> + if (width == 4) {
>>>> + val = icp->links[1];
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 24:
>>>> + if (width == 4) {
>>>> + val = icp->links[2];
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + default:
>>>> +bad_access:
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%"
>>>> + HWADDR_PRIx"/%d\n", addr, width);
>>>> + }
>>>> +
>>>> + return val;
>>>> +}
>>>> +
>>>> +static void xics_native_write(void *opaque, hwaddr addr, uint64_t val,
>>>> + unsigned width)
>>>> +{
>>>> + ICPState *icp = opaque;
>>>> + bool byte0 = (width == 1 && (addr & 0x3) == 0);
>>>> +
>>>> + switch (addr & 0xffc) {
>>>> + case 4: /* xirr */
>>>> + if (byte0) {
>>>> + icp_set_cppr(icp, val);
>>>> + } else if (width == 4) {
>>>> + icp_eoi(icp, val);
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 12:
>>>> + if (byte0) {
>>>> + icp_set_mfrr(icp, val);
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 16:
>>>> + if (width == 4) {
>>>> + icp->links[0] = val;
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 20:
>>>> + if (width == 4) {
>>>> + icp->links[1] = val;
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + case 24:
>>>> + if (width == 4) {
>>>> + icp->links[2] = val;
>>>> + } else {
>>>> + goto bad_access;
>>>> + }
>>>> + break;
>>>> + default:
>>>> +bad_access:
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%"
>>>> + HWADDR_PRIx"/%d\n", addr, width);
>>>> + }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps xics_native_ops = {
>>>> + .read = xics_native_read,
>>>> + .write = xics_native_write,
>>>> + .valid.min_access_size = 1,
>>>> + .valid.max_access_size = 4,
>>>> + .impl.min_access_size = 1,
>>>> + .impl.max_access_size = 4,
>>>> + .endianness = DEVICE_BIG_ENDIAN,
>>>> +};
>>>> +
>>>> +static uint64_t xics_native_default_read(void *opaque, hwaddr addr,
>>>> + unsigned width)
>>>> +{
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%"
>>>> + HWADDR_PRIx"/%d\n", addr, width);
>>>> + return 0xffffffffffffffffull;
>>>> +}
>>>> +
>>>> +static void xics_native_default_write(void *opaque, hwaddr addr, uint64_t
>>>> val,
>>>> + unsigned width)
>>>> +{
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%"
>>>> + HWADDR_PRIx"/%d\n", addr, width);
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps xics_native_default_ops = {
>>>> + .read = xics_native_default_read,
>>>> + .write = xics_native_default_write,
>>>> + .valid.min_access_size = 1,
>>>> + .valid.max_access_size = 4,
>>>> + .impl.min_access_size = 1,
>>>> + .impl.max_access_size = 4,
>>>> + .endianness = DEVICE_BIG_ENDIAN,
>>>> +};
>>>> +
>>>> +static void xics_native_set_nr_servers(XICSState *xics, uint32_t
>>>> nr_servers,
>>>> + Error **errp)
>>>> +{
>>>> + xics_set_nr_servers(xics, nr_servers, TYPE_ICP, errp);
>>>> +}
>>>> +
>>>> +static void xics_native_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> + XICSState *xics = XICS_COMMON(dev);
>>>> + XICSNative *xicsn = XICS_NATIVE(dev);
>>>> + Error *error = NULL;
>>>> + int i;
>>>> +
>>>> + if (!xics->nr_servers) {
>>>> + error_setg(errp, "Number of servers needs to be greater than 0");
>>>> + return;
>>>> + }
>>>> +
>>>> + for (i = 0; i < xics->nr_servers; i++) {
>>>> + object_property_set_bool(OBJECT(&xics->ss[i]), true, "realized",
>>>> + &error);
>>>> + if (error) {
>>>> + error_propagate(errp, error);
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + /*
>>>> + * Initialize the container region for the ICPs and the subregion
>>>> + * for each cpu. The mmapping will be done at the board level
>>>> + * depending on the pir of the core.
>>>> + *
>>>> + * TODO: build a name with the chip id
>>>> + */
>>>> + memory_region_init_io(&xicsn->icp_mmio, OBJECT(dev),
>>>> + &xics_native_default_ops, xicsn, "icp-0",
>>>> + PNV_XICS_SIZE);
>>>
>>> I don't think you should need these native ops. I believe you can
>>> construct a memory region as a "pure" container, then just put the
>>> populated regions inside it.
>>
>> It is a way to track bogus read/writes the guest shouldn't be doing
>> but it is not that important. I can remove it.
>
> Right. I don't recall exactly what will happen if you don't populate
> this at all, but I think you should eventually arrive at the global
> fallback handler which should give you a similar effect.
>
