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

[Cédric Le Goater]

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

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

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

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

* 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.
 
  ICP reset would be handled at the Core level

  xics_common_pic_print_info() would be a machine handler.

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

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