Re: [Qemu-devel] [PATCH v5 11/17] ppc/xics: Add "native" XICS subclass

[David Gibson]

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 <address@hidden>
> >> ---
> >>
> >>  Changes since v4:
> >>
> >>  - replaced the pir_table 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.

> > 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.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature