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

[David Gibson]

On Thu, Oct 27, 2016 at 07:43:10PM +0200, Cédric Le Goater wrote:
> 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 <address@hidden>
> >>>> ---
> >>>>
> >>>>  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. 

Ok.

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

Right, that makes sense.

> So if that comes well together, we will get rid of XICSNative and we 
> will use a XICSState for its ICP array.

So, I just had a thought about this that I think might work, though it
would require cleaning up the existing stuff in spapr before extending
for powernv:

Abolish the (overall) XICS as a fully realized object, and instead
make it a QOM interface, which is implemented by the machine.  ICPs
and ICSes remain real devices, which (as now) would have a link back
to the XICS interface object.

The XICS interface would provide server-number-to-ICP-pointer and
irq-number-to-ICS-pointer callbacks.  That puts the "fabric" logic -
how the IPCs and ICSes find each other back in the machine, which I
think is where it belongs.  Obviously we could still provide
xics_system_init() or similar helpers to make it easier for the
machines to implement the xics interface.

The overall XICS has no migrated state, which means that change
shouldn't fundamentally break things.  There might be issues with the
qom paths of ICS or ICP changing, we'd have to check that.


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