Re: [Qemu-ppc] [PATCH v1 10/11] ppc/xics: Add "native" XICS subclass

[David Gibson]

On Mon, Jun 27, 2016 at 03:23:13PM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> writes:
> 
> > [ Unknown signature status ]
> > On Thu, Jun 23, 2016 at 11:17:29PM +0530, Nikunj A Dadhania wrote:
> >> From: Benjamin Herrenschmidt <address@hidden>
> >> 
> >> This provides MMIO based ICP access as found on POWER8
> >> 
> >> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
> >> ---
> >>  default-configs/ppc64-softmmu.mak |   3 +-
> >>  hw/intc/Makefile.objs             |   1 +
> >>  hw/intc/xics_native.c             | 295 
> >> ++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/xics.h             |  14 ++
> >>  4 files changed, 312 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 c4be59f..315e30b 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))
> >
> > I don't think you've introduced CONFIG_POWERNV in your patches so far,
> > making this a bit weird.
> 
> I had kept this patch for completeness and review. We can push this once
> the POWERNV bits trickles in.
> 
> >
> >>  # For PReP
> >>  CONFIG_MC146818RTC=y
> >> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> >> index 530df2e..f8bbeda 100644
> >> --- a/hw/intc/Makefile.objs
> >> +++ b/hw/intc/Makefile.objs
> >> @@ -31,6 +31,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 0000000..26e45cc
> >> --- /dev/null
> >> +++ b/hw/intc/xics_native.c
> >> @@ -0,0 +1,295 @@
> >> +/*
> >> + * QEMU PowerPC hardware System Emulator
> >> + *
> >> + * Native version of ICS/ICP
> >> + *
> >> + * Copyright (c) 2010,2011 David Gibson, IBM Corporation.
> >
> > Surely this should have yours or Ben's copyright in addition to mine.
> 
> Sure, will add Ben's copyright.
> 
> >> + * Permission is hereby granted, free of charge, to any person obtaining 
> >> a copy
> >> + * of this software and associated documentation files (the "Software"), 
> >> to deal
> >> + * in the Software without restriction, including without limitation the 
> >> rights
> >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> >> sell
> >> + * copies of the Software, and to permit persons to whom the Software is
> >> + * furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice shall be 
> >> included in
> >> + * all copies or substantial portions of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
> >> EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
> >> MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> >> OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
> >> ARISING FROM,
> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
> >> IN
> >> + * THE SOFTWARE.
> >> + *
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "hw/hw.h"
> >> +#include "trace.h"
> >> +#include "qemu/timer.h"
> >> +#include "hw/ppc/xics.h"
> >> +#include "qapi/visitor.h"
> >> +#include "qapi/error.h"
> >> +
> >> +#include <libfdt.h>
> >> +
> >> +/* #define DEBUG_MM(fmt...)      printf(fmt) */
> >> +#define DEBUG_MM(fmt...)        do { } while (0)
> >> +
> >> +static void xics_native_initfn(Object *obj)
> >> +{
> >> +    XICSState *xics = XICS_NATIVE(obj);
> >> +
> >> +    QLIST_INIT(&xics->ics);
> >> +}
> >> +
> >> +static uint64_t icp_mm_read(void *opaque, hwaddr addr, unsigned width)
> >> +{
> >> +    XICSState *s = opaque;
> >> +    int32_t cpu_id, server;
> >> +    uint32_t val;
> >> +    ICPState *ss;
> >> +    bool byte0 = (width == 1 && (addr & 0x3) == 0);
> >> +
> >> +    cpu_id = (addr & (ICP_MM_SIZE - 1)) >> 12;
> >> +    server = get_cpu_index_by_dt_id(cpu_id);
> >
> > Shouldn't each ICP instance register its own MMIO window?
> 
> That is how the hardware works. Moreover, there is an SCOM that controls
> ICP BAR. We might support moving the ICP BAR at some stage so I'd rather
> keep it consistent.

Ok.

> 
> [...]
> 
> >> +
> >> +#define _FDT(exp) \
> >> +    do { \
> >> +        int ret = (exp);                                           \
> >> +        if (ret < 0) {                                             \
> >> +            fprintf(stderr, "qemu: error creating device tree: %s: %s\n", 
> >> \
> >> +                    #exp, fdt_strerror(ret));                      \
> >> +            exit(1);                                               \
> >> +        }                                                          \
> >> +    } while (0)
> >
> > Ugh, I have got to find time to push by qdt cleanup stuff again.
> 
> Yes, those are nice set of patches.
> 
> > Not within scope for this patch obviously, though.
> >
> >>  
> >>  #define XICS_IPI        0x2
> >>  #define XICS_BUID       0x1
> >> @@ -84,6 +89,7 @@ struct XICSState {
> >>      uint32_t nr_irqs;
> >>      ICPState *ss;
> >>      QLIST_HEAD(, ICSState) ics;
> >> +    MemoryRegion icp_mmio;
> >
> > This is only used by XICSNative, so it should be in a structure just
> > for the subclass, not in the common struct.
> 
> Sure.
> 
> >>  };
> >>  
> >>  #define TYPE_ICP "icp"
> >> @@ -115,8 +121,13 @@ struct ICPState {
> >>      uint8_t mfrr;
> >>      qemu_irq output;
> >>      bool cap_irq_xics_enabled;
> >> +    uint32_t links[3];
> >
> > Likewise here.
> 
> Ok.
> 
> Regards
> Nikunj
> 

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