Re: [Qemu-devel] [PATCH v5 04/36] ppc/xive: introduce the XiveRouter model

[Cédric Le Goater]

On 11/22/18 5:11 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:56:57AM +0100, Cédric Le Goater wrote:
>> The XiveRouter models the second sub-engine of the overall XIVE
>> architecture : the Interrupt Virtualization Routing Engine (IVRE).
>>
>> The IVRE handles event notifications of the IVSE through MMIO stores
>> and performs the interrupt routing process. For this purpose, it uses
>> a set of table stored in system memory, the first of which being the
>> Event Assignment Structure (EAS) table.
>>
>> The EAT associates an interrupt source number with an Event Notification
>> Descriptor (END) which will be used in a second phase of the routing
>> process to identify a Notification Virtual Target.
>>
>> The XiveRouter is an abstract class which needs to be inherited from
>> to define a storage for the EAT, and other upcoming tables. The
>> 'chip-id' atttribute is not strictly necessary for the sPAPR and
>> PowerNV machines but it's a good way to test the routing algorithm.
>> Without this atttribute, the XiveRouter could be a simple QOM
>> interface.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  include/hw/ppc/xive.h      | 32 ++++++++++++++
>>  include/hw/ppc/xive_regs.h | 31 ++++++++++++++
>>  hw/intc/xive.c             | 86 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 149 insertions(+)
>>  create mode 100644 include/hw/ppc/xive_regs.h
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index be93fae6317b..5a0696366577 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -11,6 +11,7 @@
>>  #define PPC_XIVE_H
>>  
>>  #include "hw/sysbus.h"
> 
> Again, I don't think making this a SysBusDevice is quite right.
> Even more so for the router than the source, because at least for PAPR
> it might not have any MMIO presence at all.

The controller model inherits from the XiveRouter and manages the TIMA.

>> +#include "hw/ppc/xive_regs.h"
>>  
>>  /*
>>   * XIVE Fabric (Interface between Source and Router)
>> @@ -168,4 +169,35 @@ static inline void xive_source_irq_set(XiveSource 
>> *xsrc, uint32_t srcno,
>>      }
>>  }
>>  
>> +/*
>> + * XIVE Router
>> + */
>> +
>> +typedef struct XiveRouter {
>> +    SysBusDevice    parent;
>> +
>> +    uint32_t        chip_id;
> 
> I don't think this belongs in the base class.  The PowerNV specific
> variants will need it, but it doesn't make sense for the PAPR version.

yeah. I am using it as a END and NVT block identifier but it's not 
required for sPAPR, it could just be zero. 

It was good to test the routing algo which should not assume that the 
block id is zero. 
 
> 
>> +} XiveRouter;
>> +
>> +#define TYPE_XIVE_ROUTER "xive-router"
>> +#define XIVE_ROUTER(obj)                                \
>> +    OBJECT_CHECK(XiveRouter, (obj), TYPE_XIVE_ROUTER)
>> +#define XIVE_ROUTER_CLASS(klass)                                        \
>> +    OBJECT_CLASS_CHECK(XiveRouterClass, (klass), TYPE_XIVE_ROUTER)
>> +#define XIVE_ROUTER_GET_CLASS(obj)                              \
>> +    OBJECT_GET_CLASS(XiveRouterClass, (obj), TYPE_XIVE_ROUTER)
>> +
>> +typedef struct XiveRouterClass {
>> +    SysBusDeviceClass parent;
>> +
>> +    /* XIVE table accessors */
>> +    int (*get_eas)(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas);
>> +    int (*set_eas)(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas);
>> +} XiveRouterClass;
>> +
>> +void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
>> +
>> +int xive_router_get_eas(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas);
>> +int xive_router_set_eas(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas);
>> +
>>  #endif /* PPC_XIVE_H */
>> diff --git a/include/hw/ppc/xive_regs.h b/include/hw/ppc/xive_regs.h
>> new file mode 100644
>> index 000000000000..12499b33614c
>> --- /dev/null
>> +++ b/include/hw/ppc/xive_regs.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * QEMU PowerPC XIVE interrupt controller model
>> + *
>> + * Copyright (c) 2016-2018, IBM Corporation.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef PPC_XIVE_REGS_H
>> +#define PPC_XIVE_REGS_H
>> +
>> +/* EAS (Event Assignment Structure)
>> + *
>> + * One per interrupt source. Targets an interrupt to a given Event
>> + * Notification Descriptor (END) and provides the corresponding
>> + * logical interrupt number (END data)
>> + */
>> +typedef struct XiveEAS {
>> +        /* Use a single 64-bit definition to make it easier to
>> +         * perform atomic updates
>> +         */
>> +        uint64_t        w;
>> +#define EAS_VALID       PPC_BIT(0)
>> +#define EAS_END_BLOCK   PPC_BITMASK(4, 7)        /* Destination END block# 
>> */
>> +#define EAS_END_INDEX   PPC_BITMASK(8, 31)       /* Destination END index */
>> +#define EAS_MASKED      PPC_BIT(32)              /* Masked */
>> +#define EAS_END_DATA    PPC_BITMASK(33, 63)      /* Data written to the END 
>> */
>> +} XiveEAS;
>> +
>> +#endif /* PPC_XIVE_REGS_H */
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 014a2e41f71f..c4c90a25758e 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -442,6 +442,91 @@ static const TypeInfo xive_source_info = {
>>      .class_init    = xive_source_class_init,
>>  };
>>  
>> +/*
>> + * XIVE Router (aka. Virtualization Controller or IVRE)
>> + */
>> +
>> +int xive_router_get_eas(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas)
>> +{
>> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
>> +
>> +    return xrc->get_eas(xrtr, lisn, eas);
>> +}
>> +
>> +int xive_router_set_eas(XiveRouter *xrtr, uint32_t lisn, XiveEAS *eas)
>> +{
>> +    XiveRouterClass *xrc = XIVE_ROUTER_GET_CLASS(xrtr);
>> +
>> +    return xrc->set_eas(xrtr, lisn, eas);
>> +}
>> +
>> +static void xive_router_notify(XiveFabric *xf, uint32_t lisn)
>> +{
>> +    XiveRouter *xrtr = XIVE_ROUTER(xf);
>> +    XiveEAS eas;
>> +
>> +    /* EAS cache lookup */
>> +    if (xive_router_get_eas(xrtr, lisn, &eas)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: Unknown LISN %x\n", lisn);
>> +        return;
>> +    }
> 
> AFAICT a bad LISN here means a qemu error (in the source, probably),
> not a user or guest error, so an assert() would be more appropriate.

hmm, I would say no because in the case of PowerNV, the firmware could
have badly configured the ISN offset of a source which would notify the 
router with a bad notification event data.  


>> +
>> +    /* The IVRE has a State Bit Cache for its internal sources which
>> +     * is also involed at this point. We skip the SBC lookup because
>> +     * the state bits of the sources are modeled internally in QEMU.
>> +     */
>> +
>> +    if (!(eas.w & EAS_VALID)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %x\n", lisn);
>> +        return;
>> +    }
>> +
>> +    if (eas.w & EAS_MASKED) {
>> +        /* Notification completed */
>> +        return;
>> +    }
>> +}
>> +
>> +static Property xive_router_properties[] = {
>> +    DEFINE_PROP_UINT32("chip-id", XiveRouter, chip_id, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void xive_router_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    XiveFabricClass *xfc = XIVE_FABRIC_CLASS(klass);
>> +
>> +    dc->desc    = "XIVE Router Engine";
>> +    dc->props   = xive_router_properties;
>> +    xfc->notify = xive_router_notify;
>> +}
>> +
>> +static const TypeInfo xive_router_info = {
>> +    .name          = TYPE_XIVE_ROUTER,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .abstract      = true,
>> +    .class_size    = sizeof(XiveRouterClass),
>> +    .class_init    = xive_router_class_init,
>> +    .interfaces    = (InterfaceInfo[]) {
>> +        { TYPE_XIVE_FABRIC },
> 
> So as far as I can see so far, the XiveFabric interface will
> essentially have to be implemented on the router object, so I'm not
> seeing much point to having the interface rather than just a direct
> call on the router object.  But I haven't read the whole series yet,
> so maybe I'm missing something.

The PSIHB and PHB4 models are using it but there are not in the series.

I can send the PSIHB patch in the next version if you like, it's the 
patch right after PnvXive. It's attached below for the moment. Look at 
pnv_psi_notify().

Thanks,

C.

>> +        { }
>> +    }
>> +};
>> +
>> +void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
>> +{
>> +    if (!(eas->w & EAS_VALID)) {
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "  %08x %s end:%02x/%04x data:%08x\n",
>> +                   lisn, eas->w & EAS_MASKED ? "M" : " ",
>> +                   (uint8_t)  GETFIELD(EAS_END_BLOCK, eas->w),
>> +                   (uint32_t) GETFIELD(EAS_END_INDEX, eas->w),
>> +                   (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
>> +}
>> +
>>  /*
>>   * XIVE Fabric
>>   */
>> @@ -455,6 +540,7 @@ static void xive_register_types(void)
>>  {
>>      type_register_static(&xive_source_info);
>>      type_register_static(&xive_fabric_info);
>> +    type_register_static(&xive_router_info);
>>  }
>>  
>>  type_init(xive_register_types)
>

0001-ppc-pnv-add-a-PSI-bridge-model-for-POWER9-processor.patch
Description: Text Data