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

[Cédric Le Goater]

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.

> that would then (after possible masking) forward on to the overall> xive 
> fabric ? 

yes. May be XIVEFabric is a confusing name. What about XIVEForwarder ? 

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

> 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, 
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 */
>>>
>>
>