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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9191 bytes --]

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 <clg@kaod.org>
> >>>> ---
> >>>>  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.

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

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

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

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]