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: 7101 bytes --]

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, that would then (after possible masking) forward
on to the overall xive fabric?

That seems like a property of the source object, 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.

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