qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Gibson
Subject: Re: [Qemu-devel] [PATCH v5 11/17] ppc/xics: Add "native" XICS subclass
Date: Tue, 8 Nov 2016 12:44:05 +1100
User-agent: Mutt/1.7.1 (2016-10-04)

On Wed, Nov 02, 2016 at 11:48:51AM +0100, Cédric Le Goater wrote:
> On 10/28/2016 03:00 AM, David Gibson wrote:
> > 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.
> 
> OK. I will take a look at that for 2.9. 
> 
> Here is the current status for pnv. XICSNative is gone. The ICP container 
> region is under the chip and the ICP-per-thread subregions under PnvCore.
> The machine uses a XICS_COMMON object to hold the array of ICPs and the 
> list of ICS. The result is much better but I had to modify a few things 
> in xics to be able to instantiate a XICS_COMMON object : 
> 
>  * add a new ops to XICSStateClass : 
> 
>      void (*realize)(DeviceState *dev, Error **errp)
> 
>    to clean up xics_kvm_realize() and xics_spapr_realize() which are
>    doing the same thing. That's a good cleanup going in the direction 
>    you are talking about. So maybe I could send for 2.8

Sounds likely.  Please send it and I'll give it a look and see if I
think it's suitable as a late addition to 2.8.

>  * add a xics_common_set_nr_servers() routine to create the ICPs when 
>    a XICS_COMMON object is instantiated. That is more a work around. 
>    That business around the "nr_irqs" and "nr_servers" properties is 
>    confusing, we should clean it up.

Yeah, it's a bit clunky.  The nr_irqs really only makes sense for the
spapr case where there's a single ICS.

> So if you think this is worth 2.8, I can send a couple of patches. Else
> I can start by some xics cleanups and aim 2.9

I think it's at least worth a look to see how invasive it is.

> > 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.
> 
> Rough comments and questions :
> 
> * ICS :
> 
>   They don't belong to XICS but we do need to maintain a list as we 
>   have a few loops on the ICSs. Should we maintain the list under the 
>   machine ?

So, the XICS interface type would need some method to traverse all the
ICSes.  It would be up to the XICS interface implementor (the machine
in this, and probably all, cases) to decide how it does that.  spapr
would just go to the single known ICS instance.  powernv could
maintain a list, or alternatively it could use its existing list of
chips to find all the possible ICS instances.

> * ICP :
> 
>   The server-number-to-ICP-pointer handler should cover most of the 
>   needs. If we can use the Core object to hold the ICP, that would 
>   be even better. It whould greatly simplify cpu_setup() which is 
>   here just for KVM_CAP_IRQ_XICS and the list of cpus would provide
>   the array.

Sure.  I don't think whether the ICP is in the core or the chip has
much bearing on the XICS interface, only what the machine would need
to do exactly to locate the right ICP instance.  The machine knows how
the PIRs are allocated, so it should be able to parse the server
number and work out both chip and core.

>   ICP reset would be handled at the Core level
> 
>   xics_common_pic_print_info() would be a machine handler.

Actually, I think you have a couple of options there.  IIRC the
interrupt stats provider interface works so that the monitor commands
will iterate through all objects supporting the interface.

So, you could make the machine respond and give a global view of the
XICS, presumably using a helper function from the XICS code.
Alternatively, you could make each ICS support the interface and
provide a view of just its piece of the interrupt state (I think it's
the ICSes not the ICPs that are relevant for this information, but I
could be wrong).

>   I am not sure what to do with ics_simple_post_load().

So, ics_simple_post_load() is actually already a bit broken.  It
relies on the fact that the ICP state is restored before the ICS
state, without really enforcing that.  There was a bug somewhat
related to this recently with interrupts being lost across migration
sometimes - the interrupt controller variables were restored, but we
didn't correctly recalculate the state of the output line to interrupt
the CPU.

This callback is essentially about restoring some cached state across
the whole XICS subsystem from the state within the individual ICS and
ICP devices - in particular the state of the (logical) output lines.
So really, this would belong better in the machine - obviously we'd
want a helper in the xics code that any machine implementing the XICS
interface could use.

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

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]