Re: [Qemu-devel] [PATCH v3 03/35] ppc/xive: introduce the XiveFabric interface

[Cédric Le Goater]

On 04/26/2018 05:54 AM, David Gibson wrote:
> On Tue, Apr 24, 2018 at 11:33:11AM +0200, Cédric Le Goater wrote:
>> On 04/24/2018 08:46 AM, David Gibson wrote:
>>> On Mon, Apr 23, 2018 at 09:58:43AM +0200, Cédric Le Goater wrote:
>>>> On 04/23/2018 08:46 AM, David Gibson wrote:
>>>>> On Thu, Apr 19, 2018 at 02:42:59PM +0200, Cédric Le Goater wrote:
>>>>>> The XiveFabric offers a simple interface, between the XiveSourve
>>>>>> object and the device model owning the interrupt sources, to forward
>>>>>> an event notification to the XIVE interrupt controller of the machine
>>>>>> and if the owner is the controller, to call directly the routing
>>>>>> sub-engine.
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>>>>> ---
>>>>>>  hw/intc/xive.c        | 37 ++++++++++++++++++++++++++++++++++++-
>>>>>>  include/hw/ppc/xive.h | 25 +++++++++++++++++++++++++
>>>>>>  2 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>>>>> index 060976077dd7..b4c3d06c1219 100644
>>>>>> --- a/hw/intc/xive.c
>>>>>> +++ b/hw/intc/xive.c
>>>>>> @@ -17,6 +17,21 @@
>>>>>>  #include "hw/ppc/xive.h"
>>>>>>  
>>>>>>  /*
>>>>>> + * XIVE Fabric
>>>>>> + */
>>>>>> +
>>>>>> +static void xive_fabric_route(XiveFabric *xf, int lisn)
>>>>>> +{
>>>>>> +
>>>>>> +}
>>>>>> +
>>>>>> +static const TypeInfo xive_fabric_info = {
>>>>>> +    .name = TYPE_XIVE_FABRIC,
>>>>>> +    .parent = TYPE_INTERFACE,
>>>>>> +    .class_size = sizeof(XiveFabricClass),
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>>   * XIVE Interrupt Source
>>>>>>   */
>>>>>>  
>>>>>> @@ -97,11 +112,19 @@ static bool xive_source_pq_trigger(XiveSource 
>>>>>> *xsrc, uint32_t srcno)
>>>>>>  
>>>>>>  /*
>>>>>>   * Forward the source event notification to the associated XiveFabric,
>>>>>> - * the device owning the sources.
>>>>>> + * the device owning the sources, or perform the routing if the device
>>>>>> + * is the interrupt controller.
>>>>>>   */
>>>>>>  static void xive_source_notify(XiveSource *xsrc, int srcno)
>>>>>>  {
>>>>>>  
>>>>>> +    XiveFabricClass *xfc = XIVE_FABRIC_GET_CLASS(xsrc->xive);
>>>>>> +
>>>>>> +    if (xfc->notify) {
>>>>>> +        xfc->notify(xsrc->xive, srcno + xsrc->offset);
>>>>>> +    } else {
>>>>>> +        xive_fabric_route(xsrc->xive, srcno + xsrc->offset);
>>>>>> +    }
>>>>>
>>>>> Why 2 cases?  Can't the XiveFabric object just make its notify equal
>>>>> to xive_fabric_route if that's what it wants?
>>>> Under sPAPR, all the sources, IPIs and virtual device interrupts, 
>>>> generate events which are directly routed by xive_fabric_route(). 
>>>> There is no need of an extra hop. Indeed. 
>>>
>>> Ok.
>>>
>>>> Under PowerNV, some sources forward the notification to the routing 
>>>> engine using a specific MMIO load on a notify address which is stored 
>>>> in one of the controller registers. So we need a hop to reach the 
>>>> device model, owning the sources, and do that load :
>>>
>>> Hm.  So you're saying that in pnv some sources send their notification
>>> to some other unit, 
>>
>> Not to any unit/device, to the device owning the sources.
>>
>> For the XiveSource object under PSI, the XIVEFabric interface is the 
>> PSI device object it self, which knows how to forward the notification 
>> on the XIVE Power "bus". To be more precise, the PSI HB device has 
>> 14 interrupt sources, which notifications are forwarded using a MMIO 
>> load to some address. The load address is configured (by skiboot) in 
>> one of the PSI device registers, and points to a MMIO region of the 
>> main XIVE interrupt controller. 
>>
>> The PHB4 sources should be the same.
>>
>> For the XiveSource object (all interrupts) under sPAPRXive, the 
>> XIVEFabric is the main interrupt controller sPAPRXive.
>>
>> For the XiveSource object (IPIs) under PnvXive, the XIVEFabric is 
>> also the main interrupt controller PnvXive.
> 
> Hrm.  Apparently I'm missing something, I'm really not getting what
> you're trying to explain here.

I see that. Let's try again.

>>> that would then (after possible masking) forward on to the overall> xive 
>>> fabric ? 
>>
>> yes. May be XIVEFabric is a confusing name. What about XIVEForwarder ? 
> 
> Maybe..?
> 
>>> That seems like a property of the source object, 
>>
>> The source object is generic. It's a bunch of PQ bits that can be 
>> controlled by MMIOs. Nothing more.
> 
> Hmm.  Isn't the source object also responsible for forwarding the
> interrupt to something up the chain (whatever that is)?

Yes but it can not forward directly. The XiveSource is generic and 
can only call a handler :

        xfc->notify(xsrc->xive, srcno + xsrc->offset);

The device model owner, the parent of the XiveSource object, would 
do the real forward. 

It's very similar to what we have today with XICS :

        - The sPAPR model has an ICSState  
        - The PnvPSI model has an ICSState 
        - The PnvPHB3 model has two ICSStates

and the 'xics' pointer in ICSState points to the 'interrupt unit' of 
the machine to do resends and to grab ICPs. So it used for routing 
essentially.

in Xive 

        - sPAPRXive model has a XiveSource
        - PnvXive model has a XiveSource
        - PnvPSI model has a XiveSource
        - PnvPHB4 model should have also.

and the 'xive' pointer in XiveSource points to the parent object,
which will handle the event notification forwarding or routing.
 
C.

>>> rather than a
>>> property of the fabric.  Indeed varying this by source object would
>>> require the objects have a different xive pointer, when I thought the
>>> idea was that the XiveFabric was global.
>>
>> When a notification is forwarded, the sources needs to call an 
>> interface which generally is implemented by the source owner,
> 
> I'm not quite sure what you mean by "source owner".

The parent object.
 
>> which is not necessarily the main IC. 
>>
>>>>    static void pnv_psi_notify(XiveFabric *xf, uint32_t lisn)
>>>>    {
>>>>        PnvPsi *psi = PNV_PSI(xf);
>>>>        uint64_t notif_port =
>>>>            psi->regs[PSIHB_REG(PSIHB9_ESB_NOTIF_ADDR)];
>>>>        bool valid = notif_port & PSIHB9_ESB_NOTIF_VALID;
>>>>        uint64_t notify_addr = notif_port & ~PSIHB9_ESB_NOTIF_VALID;
>>>>        uint32_t data = cpu_to_be32(lisn);
>>>>    
>>>>        if (valid) {
>>>>            cpu_physical_memory_write(notify_addr, &data, sizeof(data));
>>>>        }
>>>>    }
>>>>
>>>> The PnvXive model handles the load and forwards to the fabric again.  
>>>>
>>>> The IPIs under PowerNV do not need an extra hop so they reach the 
>>>> routing routine directly without the extra notify() hop. 
>>>>
>>>> However, PowerNV at the end should be using xive_fabric_route() 
>>>> but there are some differences on how the NVT registers are 
>>>> updated (HV vs. OS mode) and it's not handled yet so it uses a 
>>>> notify() handler. But is should disappear and call directly 
>>>> xive_fabric_route() in a near future.
>>>>
>>>>
>>>> May be, XiveFabricNotifier would be a better name for this feature ?
>>>> I am adding a few ops later which are more related to routing.
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>
>>>>>
>>>>>>  }
>>>>>>  
>>>>>>  /*
>>>>>> @@ -302,6 +325,17 @@ static void xive_source_reset(DeviceState *dev)
>>>>>>  static void xive_source_realize(DeviceState *dev, Error **errp)
>>>>>>  {
>>>>>>      XiveSource *xsrc = XIVE_SOURCE(dev);
>>>>>> +    Object *obj;
>>>>>> +    Error *local_err = NULL;
>>>>>> +
>>>>>> +    obj = object_property_get_link(OBJECT(dev), "xive", &local_err);
>>>>>> +    if (!obj) {
>>>>>> +        error_propagate(errp, local_err);
>>>>>> +        error_prepend(errp, "required link 'xive' not found: ");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    xsrc->xive = XIVE_FABRIC(obj);
>>>>>>  
>>>>>>      if (!xsrc->nr_irqs) {
>>>>>>          error_setg(errp, "Number of interrupt needs to be greater than 
>>>>>> 0");
>>>>>> @@ -376,6 +410,7 @@ static const TypeInfo xive_source_info = {
>>>>>>  static void xive_register_types(void)
>>>>>>  {
>>>>>>      type_register_static(&xive_source_info);
>>>>>> +    type_register_static(&xive_fabric_info);
>>>>>>  }
>>>>>>  
>>>>>>  type_init(xive_register_types)
>>>>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>>>>>> index 0b76dd278d9b..4fcae2c763e6 100644
>>>>>> --- a/include/hw/ppc/xive.h
>>>>>> +++ b/include/hw/ppc/xive.h
>>>>>> @@ -12,6 +12,8 @@
>>>>>>  
>>>>>>  #include "hw/sysbus.h"
>>>>>>  
>>>>>> +typedef struct XiveFabric XiveFabric;
>>>>>> +
>>>>>>  /*
>>>>>>   * XIVE Interrupt Source
>>>>>>   */
>>>>>> @@ -46,6 +48,8 @@ typedef struct XiveSource {
>>>>>>      hwaddr       esb_base;
>>>>>>      uint32_t     esb_shift;
>>>>>>      MemoryRegion esb_mmio;
>>>>>> +
>>>>>> +    XiveFabric   *xive;
>>>>>>  } XiveSource;
>>>>>>  
>>>>>>  /*
>>>>>> @@ -143,4 +147,25 @@ static inline void xive_source_irq_set(XiveSource 
>>>>>> *xsrc, uint32_t srcno,
>>>>>>      xsrc->status[srcno] |= lsi ? XIVE_STATUS_LSI : 0;
>>>>>>  }
>>>>>>  
>>>>>> +/*
>>>>>> + * XIVE Fabric
>>>>>> + */
>>>>>> +
>>>>>> +typedef struct XiveFabric {
>>>>>> +    Object parent;
>>>>>> +} XiveFabric;
>>>>>> +
>>>>>> +#define TYPE_XIVE_FABRIC "xive-fabric"
>>>>>> +#define XIVE_FABRIC(obj)                                     \
>>>>>> +    OBJECT_CHECK(XiveFabric, (obj), TYPE_XIVE_FABRIC)
>>>>>> +#define XIVE_FABRIC_CLASS(klass)                                     \
>>>>>> +    OBJECT_CLASS_CHECK(XiveFabricClass, (klass), TYPE_XIVE_FABRIC)
>>>>>> +#define XIVE_FABRIC_GET_CLASS(obj)                                   \
>>>>>> +    OBJECT_GET_CLASS(XiveFabricClass, (obj), TYPE_XIVE_FABRIC)
>>>>>> +
>>>>>> +typedef struct XiveFabricClass {
>>>>>> +    InterfaceClass parent;
>>>>>> +    void (*notify)(XiveFabric *xf, uint32_t lisn);
>>>>>> +} XiveFabricClass;
>>>>>> +
>>>>>>  #endif /* PPC_XIVE_H */
>>>>>
>>>>
>>>
>>
>