diff mbox series

[v5,06/36] ppc/xive: add support for the END Event State buffers

Message ID 20181116105729.23240-7-clg@kaod.org
State New
Headers show
Series ppc: support for the XIVE interrupt controller (POWER9) | expand

Commit Message

Cédric Le Goater Nov. 16, 2018, 10:56 a.m. UTC
The Event Notification Descriptor also contains two Event State
Buffers providing further coalescing of interrupts, one for the
notification event (ESn) and one for the escalation events (ESe). A
MMIO page is assigned for each to control the EOI through loads
only. Stores are not allowed.

The END ESBs are modeled through an object resembling the 'XiveSource'
It is stateless as the END state bits are backed into the XiveEND
structure under the XiveRouter and the MMIO accesses follow the same
rules as for the standard source ESBs.

END ESBs are not supported by the Linux drivers neither on OPAL nor on
sPAPR. Nevetherless, it provides a mean to study the question in the
future and validates a bit more the XIVE model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/xive.h |  20 ++++++
 hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 178 insertions(+), 2 deletions(-)

Comments

David Gibson Nov. 22, 2018, 5:13 a.m. UTC | #1
On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
> The Event Notification Descriptor also contains two Event State
> Buffers providing further coalescing of interrupts, one for the
> notification event (ESn) and one for the escalation events (ESe). A
> MMIO page is assigned for each to control the EOI through loads
> only. Stores are not allowed.
> 
> The END ESBs are modeled through an object resembling the 'XiveSource'
> It is stateless as the END state bits are backed into the XiveEND
> structure under the XiveRouter and the MMIO accesses follow the same
> rules as for the standard source ESBs.
> 
> END ESBs are not supported by the Linux drivers neither on OPAL nor on
> sPAPR. Nevetherless, it provides a mean to study the question in the
> future and validates a bit more the XIVE model.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/xive.h |  20 ++++++
>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 178 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index ce62aaf28343..24301bf2076d 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>                          XiveEND *end);
>  
> +/*
> + * XIVE END ESBs
> + */
> +
> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
> +#define XIVE_END_SOURCE(obj) \
> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)

Is there a particular reason to make this a full QOM object, rather
than just embedding it in the XiveRouter?

> +typedef struct XiveENDSource {
> +    SysBusDevice parent;
> +
> +    uint32_t        nr_ends;
> +
> +    /* ESB memory region */
> +    uint32_t        esb_shift;
> +    MemoryRegion    esb_mmio;
> +
> +    XiveRouter      *xrtr;
> +} XiveENDSource;
> +
>  /*
>   * For legacy compatibility, the exceptions define up to 256 different
>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index 9cb001e7b540..5a8882d47a98 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>       * even futher coalescing in the Router
>       */
>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
> -        return;
> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
> +        bool notify = xive_esb_trigger(&pq);
> +
> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
> +        }
> +
> +        /* ESn[Q]=1 : end of notification */
> +        if (!notify) {
> +            return;
> +        }
>      }
>  
>      /*
> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
>  }
>  
> +/*
> + * END ESB MMIO loads
> + */
> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
> +    XiveRouter *xrtr = xsrc->xrtr;
> +    uint32_t offset = addr & 0xFFF;
> +    uint8_t end_blk;
> +    uint32_t end_idx;
> +    XiveEND end;
> +    uint32_t end_esmask;
> +    uint8_t pq;
> +    uint64_t ret = -1;
> +
> +    end_blk = xrtr->chip_id;
> +    end_idx = addr >> (xsrc->esb_shift + 1);
> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
> +                      end_idx);
> +        return -1;
> +    }
> +
> +    if (!(end.w0 & END_W0_VALID)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: END %x/%x is invalid\n",
> +                      end_blk, end_idx);
> +        return -1;
> +    }
> +
> +    end_esmask = addr_is_even(addr, xsrc->esb_shift) ? END_W1_ESn : END_W1_ESe;
> +    pq = GETFIELD(end_esmask, end.w1);
> +
> +    switch (offset) {
> +    case XIVE_ESB_LOAD_EOI ... XIVE_ESB_LOAD_EOI + 0x7FF:
> +        ret = xive_esb_eoi(&pq);
> +
> +        /* Forward the source event notification for routing ?? */
> +        break;
> +
> +    case XIVE_ESB_GET ... XIVE_ESB_GET + 0x3FF:
> +        ret = pq;
> +        break;
> +
> +    case XIVE_ESB_SET_PQ_00 ... XIVE_ESB_SET_PQ_00 + 0x0FF:
> +    case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
> +    case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
> +    case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
> +        ret = xive_esb_set(&pq, (offset >> 8) & 0x3);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid END ESB load addr %d\n",
> +                      offset);
> +        return -1;
> +    }
> +
> +    if (pq != GETFIELD(end_esmask, end.w1)) {
> +        end.w1 = SETFIELD(end_esmask, end.w1, pq);
> +        xive_router_set_end(xrtr, end_blk, end_idx, &end);
> +    }

We can probably share some more code with XiveSource here, but that's
something that can be refined later.

> +
> +    return ret;
> +}
> +
> +/*
> + * END ESB MMIO stores are invalid
> + */
> +static void xive_end_source_write(void *opaque, hwaddr addr,
> +                                  uint64_t value, unsigned size)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr 0x%"
> +                  HWADDR_PRIx"\n", addr);
> +}
> +
> +static const MemoryRegionOps xive_end_source_ops = {
> +    .read = xive_end_source_read,
> +    .write = xive_end_source_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .valid = {
> +        .min_access_size = 8,
> +        .max_access_size = 8,
> +    },
> +    .impl = {
> +        .min_access_size = 8,
> +        .max_access_size = 8,
> +    },
> +};
> +
> +static void xive_end_source_realize(DeviceState *dev, Error **errp)
> +{
> +    XiveENDSource *xsrc = XIVE_END_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->xrtr = XIVE_ROUTER(obj);
> +
> +    if (!xsrc->nr_ends) {
> +        error_setg(errp, "Number of interrupt needs to be greater than 0");
> +        return;
> +    }
> +
> +    if (xsrc->esb_shift != XIVE_ESB_4K &&
> +        xsrc->esb_shift != XIVE_ESB_64K) {
> +        error_setg(errp, "Invalid ESB shift setting");
> +        return;
> +    }
> +
> +    /*
> +     * Each END is assigned an even/odd pair of MMIO pages, the even page
> +     * manages the ESn field while the odd page manages the ESe field.
> +     */
> +    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
> +                          &xive_end_source_ops, xsrc, "xive.end",
> +                          (1ull << (xsrc->esb_shift + 1)) * xsrc->nr_ends);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
> +}
> +
> +static Property xive_end_source_properties[] = {
> +    DEFINE_PROP_UINT32("nr-ends", XiveENDSource, nr_ends, 0),
> +    DEFINE_PROP_UINT32("shift", XiveENDSource, esb_shift, XIVE_ESB_64K),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void xive_end_source_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc    = "XIVE END Source";
> +    dc->props   = xive_end_source_properties;
> +    dc->realize = xive_end_source_realize;
> +}
> +
> +static const TypeInfo xive_end_source_info = {
> +    .name          = TYPE_XIVE_END_SOURCE,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(XiveENDSource),
> +    .class_init    = xive_end_source_class_init,
> +};
> +
>  /*
>   * XIVE Fabric
>   */
> @@ -720,6 +875,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_register_static(&xive_end_source_info);
>  }
>  
>  type_init(xive_register_types)
Cédric Le Goater Nov. 22, 2018, 9:58 p.m. UTC | #2
On 11/22/18 6:13 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
>> The Event Notification Descriptor also contains two Event State
>> Buffers providing further coalescing of interrupts, one for the
>> notification event (ESn) and one for the escalation events (ESe). A
>> MMIO page is assigned for each to control the EOI through loads
>> only. Stores are not allowed.
>>
>> The END ESBs are modeled through an object resembling the 'XiveSource'
>> It is stateless as the END state bits are backed into the XiveEND
>> structure under the XiveRouter and the MMIO accesses follow the same
>> rules as for the standard source ESBs.
>>
>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
>> sPAPR. Nevetherless, it provides a mean to study the question in the
>> future and validates a bit more the XIVE model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/xive.h |  20 ++++++
>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 178 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index ce62aaf28343..24301bf2076d 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>                          XiveEND *end);
>>  
>> +/*
>> + * XIVE END ESBs
>> + */
>> +
>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
>> +#define XIVE_END_SOURCE(obj) \
>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> 
> Is there a particular reason to make this a full QOM object, rather
> than just embedding it in the XiveRouter?

yes, it should probably be under the XiveRouter you are right because
there is a direct link with the ENDT which is in the XiverRouter. 

But if I remove the chip_id field from the XiveRouter, it becomes a QOM
interface. something to ponder.
 
>> +typedef struct XiveENDSource {
>> +    SysBusDevice parent;
>> +
>> +    uint32_t        nr_ends;
>> +
>> +    /* ESB memory region */
>> +    uint32_t        esb_shift;
>> +    MemoryRegion    esb_mmio;
>> +
>> +    XiveRouter      *xrtr;
>> +} XiveENDSource;
>> +
>>  /*
>>   * For legacy compatibility, the exceptions define up to 256 different
>>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 9cb001e7b540..5a8882d47a98 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>>       * even futher coalescing in the Router
>>       */
>>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
>> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
>> -        return;
>> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
>> +        bool notify = xive_esb_trigger(&pq);
>> +
>> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
>> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
>> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
>> +        }
>> +
>> +        /* ESn[Q]=1 : end of notification */
>> +        if (!notify) {
>> +            return;
>> +        }
>>      }
>>  
>>      /*
>> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
>>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
>>  }
>>  
>> +/*
>> + * END ESB MMIO loads
>> + */
>> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
>> +    XiveRouter *xrtr = xsrc->xrtr;
>> +    uint32_t offset = addr & 0xFFF;
>> +    uint8_t end_blk;
>> +    uint32_t end_idx;
>> +    XiveEND end;
>> +    uint32_t end_esmask;
>> +    uint8_t pq;
>> +    uint64_t ret = -1;
>> +
>> +    end_blk = xrtr->chip_id;
>> +    end_idx = addr >> (xsrc->esb_shift + 1);
>> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
>> +                      end_idx);
>> +        return -1;
>> +    }
>> +
>> +    if (!(end.w0 & END_W0_VALID)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: END %x/%x is invalid\n",
>> +                      end_blk, end_idx);
>> +        return -1;
>> +    }
>> +
>> +    end_esmask = addr_is_even(addr, xsrc->esb_shift) ? END_W1_ESn : END_W1_ESe;
>> +    pq = GETFIELD(end_esmask, end.w1);
>> +
>> +    switch (offset) {
>> +    case XIVE_ESB_LOAD_EOI ... XIVE_ESB_LOAD_EOI + 0x7FF:
>> +        ret = xive_esb_eoi(&pq);
>> +
>> +        /* Forward the source event notification for routing ?? */
>> +        break;
>> +
>> +    case XIVE_ESB_GET ... XIVE_ESB_GET + 0x3FF:
>> +        ret = pq;
>> +        break;
>> +
>> +    case XIVE_ESB_SET_PQ_00 ... XIVE_ESB_SET_PQ_00 + 0x0FF:
>> +    case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
>> +    case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
>> +    case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
>> +        ret = xive_esb_set(&pq, (offset >> 8) & 0x3);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid END ESB load addr %d\n",
>> +                      offset);
>> +        return -1;
>> +    }
>> +
>> +    if (pq != GETFIELD(end_esmask, end.w1)) {
>> +        end.w1 = SETFIELD(end_esmask, end.w1, pq);
>> +        xive_router_set_end(xrtr, end_blk, end_idx, &end);
>> +    }
> 
> We can probably share some more code with XiveSource here, but that's
> something that can be refined later.

yes clearly. The idea was to introduce a XiveESB model handling only the 
MMIO aspects and rely on an interface to query/modify the underlying PQ bits.
These state bits are related to a device and the ESB pages are the XIVE way 
to expose them. 

I left that for later. I didn't want to complexify more the XiveSource 
with a feature not used today. 

Thanks,

C.

> 
>> +
>> +    return ret;
>> +}
>> +
>> +/*
>> + * END ESB MMIO stores are invalid
>> + */
>> +static void xive_end_source_write(void *opaque, hwaddr addr,
>> +                                  uint64_t value, unsigned size)
>> +{
>> +    qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr 0x%"
>> +                  HWADDR_PRIx"\n", addr);
>> +}
>> +
>> +static const MemoryRegionOps xive_end_source_ops = {
>> +    .read = xive_end_source_read,
>> +    .write = xive_end_source_write,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 8,
>> +        .max_access_size = 8,
>> +    },
>> +    .impl = {
>> +        .min_access_size = 8,
>> +        .max_access_size = 8,
>> +    },
>> +};
>> +
>> +static void xive_end_source_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XiveENDSource *xsrc = XIVE_END_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->xrtr = XIVE_ROUTER(obj);
>> +
>> +    if (!xsrc->nr_ends) {
>> +        error_setg(errp, "Number of interrupt needs to be greater than 0");
>> +        return;
>> +    }
>> +
>> +    if (xsrc->esb_shift != XIVE_ESB_4K &&
>> +        xsrc->esb_shift != XIVE_ESB_64K) {
>> +        error_setg(errp, "Invalid ESB shift setting");
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Each END is assigned an even/odd pair of MMIO pages, the even page
>> +     * manages the ESn field while the odd page manages the ESe field.
>> +     */
>> +    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
>> +                          &xive_end_source_ops, xsrc, "xive.end",
>> +                          (1ull << (xsrc->esb_shift + 1)) * xsrc->nr_ends);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
>> +}
>> +
>> +static Property xive_end_source_properties[] = {
>> +    DEFINE_PROP_UINT32("nr-ends", XiveENDSource, nr_ends, 0),
>> +    DEFINE_PROP_UINT32("shift", XiveENDSource, esb_shift, XIVE_ESB_64K),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void xive_end_source_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->desc    = "XIVE END Source";
>> +    dc->props   = xive_end_source_properties;
>> +    dc->realize = xive_end_source_realize;
>> +}
>> +
>> +static const TypeInfo xive_end_source_info = {
>> +    .name          = TYPE_XIVE_END_SOURCE,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(XiveENDSource),
>> +    .class_init    = xive_end_source_class_init,
>> +};
>> +
>>  /*
>>   * XIVE Fabric
>>   */
>> @@ -720,6 +875,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_register_static(&xive_end_source_info);
>>  }
>>  
>>  type_init(xive_register_types)
>
David Gibson Nov. 23, 2018, 4:36 a.m. UTC | #3
On Thu, Nov 22, 2018 at 10:58:56PM +0100, Cédric Le Goater wrote:
> On 11/22/18 6:13 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
> >> The Event Notification Descriptor also contains two Event State
> >> Buffers providing further coalescing of interrupts, one for the
> >> notification event (ESn) and one for the escalation events (ESe). A
> >> MMIO page is assigned for each to control the EOI through loads
> >> only. Stores are not allowed.
> >>
> >> The END ESBs are modeled through an object resembling the 'XiveSource'
> >> It is stateless as the END state bits are backed into the XiveEND
> >> structure under the XiveRouter and the MMIO accesses follow the same
> >> rules as for the standard source ESBs.
> >>
> >> END ESBs are not supported by the Linux drivers neither on OPAL nor on
> >> sPAPR. Nevetherless, it provides a mean to study the question in the
> >> future and validates a bit more the XIVE model.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/xive.h |  20 ++++++
> >>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 178 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index ce62aaf28343..24301bf2076d 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>                          XiveEND *end);
> >>  
> >> +/*
> >> + * XIVE END ESBs
> >> + */
> >> +
> >> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
> >> +#define XIVE_END_SOURCE(obj) \
> >> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> > 
> > Is there a particular reason to make this a full QOM object, rather
> > than just embedding it in the XiveRouter?
> 
> yes, it should probably be under the XiveRouter you are right because
> there is a direct link with the ENDT which is in the XiverRouter. 
> 
> But if I remove the chip_id field from the XiveRouter, it becomes a QOM
> interface. something to ponder.

Huh?  I really don't understand what you're saying here.  What does
chip_id have to do with anything?

>  
> >> +typedef struct XiveENDSource {
> >> +    SysBusDevice parent;
> >> +
> >> +    uint32_t        nr_ends;
> >> +
> >> +    /* ESB memory region */
> >> +    uint32_t        esb_shift;
> >> +    MemoryRegion    esb_mmio;
> >> +
> >> +    XiveRouter      *xrtr;
> >> +} XiveENDSource;
> >> +
> >>  /*
> >>   * For legacy compatibility, the exceptions define up to 256 different
> >>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >> index 9cb001e7b540..5a8882d47a98 100644
> >> --- a/hw/intc/xive.c
> >> +++ b/hw/intc/xive.c
> >> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
> >>       * even futher coalescing in the Router
> >>       */
> >>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
> >> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
> >> -        return;
> >> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
> >> +        bool notify = xive_esb_trigger(&pq);
> >> +
> >> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
> >> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
> >> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
> >> +        }
> >> +
> >> +        /* ESn[Q]=1 : end of notification */
> >> +        if (!notify) {
> >> +            return;
> >> +        }
> >>      }
> >>  
> >>      /*
> >> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
> >>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
> >>  }
> >>  
> >> +/*
> >> + * END ESB MMIO loads
> >> + */
> >> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
> >> +{
> >> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
> >> +    XiveRouter *xrtr = xsrc->xrtr;
> >> +    uint32_t offset = addr & 0xFFF;
> >> +    uint8_t end_blk;
> >> +    uint32_t end_idx;
> >> +    XiveEND end;
> >> +    uint32_t end_esmask;
> >> +    uint8_t pq;
> >> +    uint64_t ret = -1;
> >> +
> >> +    end_blk = xrtr->chip_id;
> >> +    end_idx = addr >> (xsrc->esb_shift + 1);
> >> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
> >> +                      end_idx);
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (!(end.w0 & END_W0_VALID)) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: END %x/%x is invalid\n",
> >> +                      end_blk, end_idx);
> >> +        return -1;
> >> +    }
> >> +
> >> +    end_esmask = addr_is_even(addr, xsrc->esb_shift) ? END_W1_ESn : END_W1_ESe;
> >> +    pq = GETFIELD(end_esmask, end.w1);
> >> +
> >> +    switch (offset) {
> >> +    case XIVE_ESB_LOAD_EOI ... XIVE_ESB_LOAD_EOI + 0x7FF:
> >> +        ret = xive_esb_eoi(&pq);
> >> +
> >> +        /* Forward the source event notification for routing ?? */
> >> +        break;
> >> +
> >> +    case XIVE_ESB_GET ... XIVE_ESB_GET + 0x3FF:
> >> +        ret = pq;
> >> +        break;
> >> +
> >> +    case XIVE_ESB_SET_PQ_00 ... XIVE_ESB_SET_PQ_00 + 0x0FF:
> >> +    case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
> >> +    case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
> >> +    case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
> >> +        ret = xive_esb_set(&pq, (offset >> 8) & 0x3);
> >> +        break;
> >> +    default:
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid END ESB load addr %d\n",
> >> +                      offset);
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (pq != GETFIELD(end_esmask, end.w1)) {
> >> +        end.w1 = SETFIELD(end_esmask, end.w1, pq);
> >> +        xive_router_set_end(xrtr, end_blk, end_idx, &end);
> >> +    }
> > 
> > We can probably share some more code with XiveSource here, but that's
> > something that can be refined later.
> 
> yes clearly. The idea was to introduce a XiveESB model handling only the 
> MMIO aspects and rely on an interface to query/modify the underlying PQ bits.
> These state bits are related to a device and the ESB pages are the XIVE way 
> to expose them. 
> 
> I left that for later. I didn't want to complexify more the XiveSource 
> with a feature not used today. 
> 
> Thanks,
> 
> C.
> 
> > 
> >> +
> >> +    return ret;
> >> +}
> >> +
> >> +/*
> >> + * END ESB MMIO stores are invalid
> >> + */
> >> +static void xive_end_source_write(void *opaque, hwaddr addr,
> >> +                                  uint64_t value, unsigned size)
> >> +{
> >> +    qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr 0x%"
> >> +                  HWADDR_PRIx"\n", addr);
> >> +}
> >> +
> >> +static const MemoryRegionOps xive_end_source_ops = {
> >> +    .read = xive_end_source_read,
> >> +    .write = xive_end_source_write,
> >> +    .endianness = DEVICE_BIG_ENDIAN,
> >> +    .valid = {
> >> +        .min_access_size = 8,
> >> +        .max_access_size = 8,
> >> +    },
> >> +    .impl = {
> >> +        .min_access_size = 8,
> >> +        .max_access_size = 8,
> >> +    },
> >> +};
> >> +
> >> +static void xive_end_source_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    XiveENDSource *xsrc = XIVE_END_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->xrtr = XIVE_ROUTER(obj);
> >> +
> >> +    if (!xsrc->nr_ends) {
> >> +        error_setg(errp, "Number of interrupt needs to be greater than 0");
> >> +        return;
> >> +    }
> >> +
> >> +    if (xsrc->esb_shift != XIVE_ESB_4K &&
> >> +        xsrc->esb_shift != XIVE_ESB_64K) {
> >> +        error_setg(errp, "Invalid ESB shift setting");
> >> +        return;
> >> +    }
> >> +
> >> +    /*
> >> +     * Each END is assigned an even/odd pair of MMIO pages, the even page
> >> +     * manages the ESn field while the odd page manages the ESe field.
> >> +     */
> >> +    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
> >> +                          &xive_end_source_ops, xsrc, "xive.end",
> >> +                          (1ull << (xsrc->esb_shift + 1)) * xsrc->nr_ends);
> >> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
> >> +}
> >> +
> >> +static Property xive_end_source_properties[] = {
> >> +    DEFINE_PROP_UINT32("nr-ends", XiveENDSource, nr_ends, 0),
> >> +    DEFINE_PROP_UINT32("shift", XiveENDSource, esb_shift, XIVE_ESB_64K),
> >> +    DEFINE_PROP_END_OF_LIST(),
> >> +};
> >> +
> >> +static void xive_end_source_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >> +
> >> +    dc->desc    = "XIVE END Source";
> >> +    dc->props   = xive_end_source_properties;
> >> +    dc->realize = xive_end_source_realize;
> >> +}
> >> +
> >> +static const TypeInfo xive_end_source_info = {
> >> +    .name          = TYPE_XIVE_END_SOURCE,
> >> +    .parent        = TYPE_SYS_BUS_DEVICE,
> >> +    .instance_size = sizeof(XiveENDSource),
> >> +    .class_init    = xive_end_source_class_init,
> >> +};
> >> +
> >>  /*
> >>   * XIVE Fabric
> >>   */
> >> @@ -720,6 +875,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_register_static(&xive_end_source_info);
> >>  }
> >>  
> >>  type_init(xive_register_types)
> > 
>
Cédric Le Goater Nov. 23, 2018, 7:28 a.m. UTC | #4
On 11/23/18 5:36 AM, David Gibson wrote:
> On Thu, Nov 22, 2018 at 10:58:56PM +0100, Cédric Le Goater wrote:
>> On 11/22/18 6:13 AM, David Gibson wrote:
>>> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
>>>> The Event Notification Descriptor also contains two Event State
>>>> Buffers providing further coalescing of interrupts, one for the
>>>> notification event (ESn) and one for the escalation events (ESe). A
>>>> MMIO page is assigned for each to control the EOI through loads
>>>> only. Stores are not allowed.
>>>>
>>>> The END ESBs are modeled through an object resembling the 'XiveSource'
>>>> It is stateless as the END state bits are backed into the XiveEND
>>>> structure under the XiveRouter and the MMIO accesses follow the same
>>>> rules as for the standard source ESBs.
>>>>
>>>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
>>>> sPAPR. Nevetherless, it provides a mean to study the question in the
>>>> future and validates a bit more the XIVE model.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  include/hw/ppc/xive.h |  20 ++++++
>>>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 178 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>>>> index ce62aaf28343..24301bf2076d 100644
>>>> --- a/include/hw/ppc/xive.h
>>>> +++ b/include/hw/ppc/xive.h
>>>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>>>                          XiveEND *end);
>>>>  
>>>> +/*
>>>> + * XIVE END ESBs
>>>> + */
>>>> +
>>>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
>>>> +#define XIVE_END_SOURCE(obj) \
>>>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
>>>
>>> Is there a particular reason to make this a full QOM object, rather
>>> than just embedding it in the XiveRouter?
>>
>> yes, it should probably be under the XiveRouter you are right because
>> there is a direct link with the ENDT which is in the XiverRouter. 
>>
>> But if I remove the chip_id field from the XiveRouter, it becomes a QOM
>> interface. something to ponder.
> 
> Huh?  I really don't understand what you're saying here.  What does
> chip_id have to do with anything?

I am quoting a comment of yours :

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


If we remove 'chip_id' from XiveRouter, it can become a QOM interface 
without state, like the XiveFabric is.

C.
David Gibson Nov. 26, 2018, 5:54 a.m. UTC | #5
On Fri, Nov 23, 2018 at 08:28:42AM +0100, Cédric Le Goater wrote:
> On 11/23/18 5:36 AM, David Gibson wrote:
> > On Thu, Nov 22, 2018 at 10:58:56PM +0100, Cédric Le Goater wrote:
> >> On 11/22/18 6:13 AM, David Gibson wrote:
> >>> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
> >>>> The Event Notification Descriptor also contains two Event State
> >>>> Buffers providing further coalescing of interrupts, one for the
> >>>> notification event (ESn) and one for the escalation events (ESe). A
> >>>> MMIO page is assigned for each to control the EOI through loads
> >>>> only. Stores are not allowed.
> >>>>
> >>>> The END ESBs are modeled through an object resembling the 'XiveSource'
> >>>> It is stateless as the END state bits are backed into the XiveEND
> >>>> structure under the XiveRouter and the MMIO accesses follow the same
> >>>> rules as for the standard source ESBs.
> >>>>
> >>>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
> >>>> sPAPR. Nevetherless, it provides a mean to study the question in the
> >>>> future and validates a bit more the XIVE model.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>> ---
> >>>>  include/hw/ppc/xive.h |  20 ++++++
> >>>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
> >>>>  2 files changed, 178 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >>>> index ce62aaf28343..24301bf2076d 100644
> >>>> --- a/include/hw/ppc/xive.h
> >>>> +++ b/include/hw/ppc/xive.h
> >>>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>>>                          XiveEND *end);
> >>>>  
> >>>> +/*
> >>>> + * XIVE END ESBs
> >>>> + */
> >>>> +
> >>>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
> >>>> +#define XIVE_END_SOURCE(obj) \
> >>>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> >>>
> >>> Is there a particular reason to make this a full QOM object, rather
> >>> than just embedding it in the XiveRouter?
> >>
> >> yes, it should probably be under the XiveRouter you are right because
> >> there is a direct link with the ENDT which is in the XiverRouter. 
> >>
> >> But if I remove the chip_id field from the XiveRouter, it becomes a QOM
> >> interface. something to ponder.
> > 
> > Huh?  I really don't understand what you're saying here.  What does
> > chip_id have to do with anything?
> 
> I am quoting a comment of yours :
> 
> 	> +/*
> 	> + * 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.
> 
> 
> If we remove 'chip_id' from XiveRouter, it can become a QOM interface 
> without state, like the XiveFabric is.

Hm, not really.  At this stage the object does't have any state, but
at least the pnv variants will have state for the registers which
point it to the EAST and ENDT, so a "real" object still makes more
sense than an interface.  If it were an interface it's not clear what
real object it would be attached to.
Cédric Le Goater Nov. 29, 2018, 10:06 p.m. UTC | #6
On 11/22/18 6:13 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
>> The Event Notification Descriptor also contains two Event State
>> Buffers providing further coalescing of interrupts, one for the
>> notification event (ESn) and one for the escalation events (ESe). A
>> MMIO page is assigned for each to control the EOI through loads
>> only. Stores are not allowed.
>>
>> The END ESBs are modeled through an object resembling the 'XiveSource'
>> It is stateless as the END state bits are backed into the XiveEND
>> structure under the XiveRouter and the MMIO accesses follow the same
>> rules as for the standard source ESBs.
>>
>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
>> sPAPR. Nevetherless, it provides a mean to study the question in the
>> future and validates a bit more the XIVE model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/xive.h |  20 ++++++
>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 178 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index ce62aaf28343..24301bf2076d 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>                          XiveEND *end);
>>  
>> +/*
>> + * XIVE END ESBs
>> + */
>> +
>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
>> +#define XIVE_END_SOURCE(obj) \
>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> 
> Is there a particular reason to make this a full QOM object, rather
> than just embedding it in the XiveRouter?

Coming back on this question because removing the chip_id from the
router is a problem for the END triggering. At least with the current
design. See below for the comment.

>> +typedef struct XiveENDSource {
>> +    SysBusDevice parent;
>> +
>> +    uint32_t        nr_ends;
>> +
>> +    /* ESB memory region */
>> +    uint32_t        esb_shift;
>> +    MemoryRegion    esb_mmio;
>> +
>> +    XiveRouter      *xrtr;
>> +} XiveENDSource;
>> +
>>  /*
>>   * For legacy compatibility, the exceptions define up to 256 different
>>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index 9cb001e7b540..5a8882d47a98 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>>       * even futher coalescing in the Router
>>       */
>>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
>> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
>> -        return;
>> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
>> +        bool notify = xive_esb_trigger(&pq);
>> +
>> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
>> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
>> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
>> +        }
>> +
>> +        /* ESn[Q]=1 : end of notification */
>> +        if (!notify) {
>> +            return;
>> +        }
>>      }
>>  
>>      /*
>> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
>>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
>>  }
>>  
>> +/*
>> + * END ESB MMIO loads
>> + */
>> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
>> +    XiveRouter *xrtr = xsrc->xrtr;
>> +    uint32_t offset = addr & 0xFFF;
>> +    uint8_t end_blk;
>> +    uint32_t end_idx;
>> +    XiveEND end;
>> +    uint32_t end_esmask;
>> +    uint8_t pq;
>> +    uint64_t ret = -1;
>> +
>> +    end_blk = xrtr->chip_id;
>> +    end_idx = addr >> (xsrc->esb_shift + 1);
>> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {

The current END accessors require a block identifier, hence xrtr->chip_id, 
but in this case, we don't really need it because we are using the ENDT 
local to the router/chip. 

I don't know how to handle simply this case without keeping chip_id :/

C.

>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
>> +                      end_idx);
>> +        return -1;
>> +    }
>> +
>> +    if (!(end.w0 & END_W0_VALID)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: END %x/%x is invalid\n",
>> +                      end_blk, end_idx);
>> +        return -1;
>> +    }
>> +
>> +    end_esmask = addr_is_even(addr, xsrc->esb_shift) ? END_W1_ESn : END_W1_ESe;
>> +    pq = GETFIELD(end_esmask, end.w1);
>> +
>> +    switch (offset) {
>> +    case XIVE_ESB_LOAD_EOI ... XIVE_ESB_LOAD_EOI + 0x7FF:
>> +        ret = xive_esb_eoi(&pq);
>> +
>> +        /* Forward the source event notification for routing ?? */
>> +        break;
>> +
>> +    case XIVE_ESB_GET ... XIVE_ESB_GET + 0x3FF:
>> +        ret = pq;
>> +        break;
>> +
>> +    case XIVE_ESB_SET_PQ_00 ... XIVE_ESB_SET_PQ_00 + 0x0FF:
>> +    case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
>> +    case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
>> +    case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
>> +        ret = xive_esb_set(&pq, (offset >> 8) & 0x3);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid END ESB load addr %d\n",
>> +                      offset);
>> +        return -1;
>> +    }
>> +
>> +    if (pq != GETFIELD(end_esmask, end.w1)) {
>> +        end.w1 = SETFIELD(end_esmask, end.w1, pq);
>> +        xive_router_set_end(xrtr, end_blk, end_idx, &end);
>> +    }
> 
> We can probably share some more code with XiveSource here, but that's
> something that can be refined later.
> 
>> +
>> +    return ret;
>> +}
>> +
>> +/*
>> + * END ESB MMIO stores are invalid
>> + */
>> +static void xive_end_source_write(void *opaque, hwaddr addr,
>> +                                  uint64_t value, unsigned size)
>> +{
>> +    qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr 0x%"
>> +                  HWADDR_PRIx"\n", addr);
>> +}
>> +
>> +static const MemoryRegionOps xive_end_source_ops = {
>> +    .read = xive_end_source_read,
>> +    .write = xive_end_source_write,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 8,
>> +        .max_access_size = 8,
>> +    },
>> +    .impl = {
>> +        .min_access_size = 8,
>> +        .max_access_size = 8,
>> +    },
>> +};
>> +
>> +static void xive_end_source_realize(DeviceState *dev, Error **errp)
>> +{
>> +    XiveENDSource *xsrc = XIVE_END_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->xrtr = XIVE_ROUTER(obj);
>> +
>> +    if (!xsrc->nr_ends) {
>> +        error_setg(errp, "Number of interrupt needs to be greater than 0");
>> +        return;
>> +    }
>> +
>> +    if (xsrc->esb_shift != XIVE_ESB_4K &&
>> +        xsrc->esb_shift != XIVE_ESB_64K) {
>> +        error_setg(errp, "Invalid ESB shift setting");
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Each END is assigned an even/odd pair of MMIO pages, the even page
>> +     * manages the ESn field while the odd page manages the ESe field.
>> +     */
>> +    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
>> +                          &xive_end_source_ops, xsrc, "xive.end",
>> +                          (1ull << (xsrc->esb_shift + 1)) * xsrc->nr_ends);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
>> +}
>> +
>> +static Property xive_end_source_properties[] = {
>> +    DEFINE_PROP_UINT32("nr-ends", XiveENDSource, nr_ends, 0),
>> +    DEFINE_PROP_UINT32("shift", XiveENDSource, esb_shift, XIVE_ESB_64K),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void xive_end_source_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->desc    = "XIVE END Source";
>> +    dc->props   = xive_end_source_properties;
>> +    dc->realize = xive_end_source_realize;
>> +}
>> +
>> +static const TypeInfo xive_end_source_info = {
>> +    .name          = TYPE_XIVE_END_SOURCE,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(XiveENDSource),
>> +    .class_init    = xive_end_source_class_init,
>> +};
>> +
>>  /*
>>   * XIVE Fabric
>>   */
>> @@ -720,6 +875,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_register_static(&xive_end_source_info);
>>  }
>>  
>>  type_init(xive_register_types)
>
David Gibson Nov. 30, 2018, 1:04 a.m. UTC | #7
On Thu, Nov 29, 2018 at 11:06:13PM +0100, Cédric Le Goater wrote:
> On 11/22/18 6:13 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
> >> The Event Notification Descriptor also contains two Event State
> >> Buffers providing further coalescing of interrupts, one for the
> >> notification event (ESn) and one for the escalation events (ESe). A
> >> MMIO page is assigned for each to control the EOI through loads
> >> only. Stores are not allowed.
> >>
> >> The END ESBs are modeled through an object resembling the 'XiveSource'
> >> It is stateless as the END state bits are backed into the XiveEND
> >> structure under the XiveRouter and the MMIO accesses follow the same
> >> rules as for the standard source ESBs.
> >>
> >> END ESBs are not supported by the Linux drivers neither on OPAL nor on
> >> sPAPR. Nevetherless, it provides a mean to study the question in the
> >> future and validates a bit more the XIVE model.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/xive.h |  20 ++++++
> >>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 178 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index ce62aaf28343..24301bf2076d 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>                          XiveEND *end);
> >>  
> >> +/*
> >> + * XIVE END ESBs
> >> + */
> >> +
> >> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
> >> +#define XIVE_END_SOURCE(obj) \
> >> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> > 
> > Is there a particular reason to make this a full QOM object, rather
> > than just embedding it in the XiveRouter?
> 
> Coming back on this question because removing the chip_id from the
> router is a problem for the END triggering. At least with the current
> design. See below for the comment.
> 
> >> +typedef struct XiveENDSource {
> >> +    SysBusDevice parent;
> >> +
> >> +    uint32_t        nr_ends;
> >> +
> >> +    /* ESB memory region */
> >> +    uint32_t        esb_shift;
> >> +    MemoryRegion    esb_mmio;
> >> +
> >> +    XiveRouter      *xrtr;
> >> +} XiveENDSource;
> >> +
> >>  /*
> >>   * For legacy compatibility, the exceptions define up to 256 different
> >>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >> index 9cb001e7b540..5a8882d47a98 100644
> >> --- a/hw/intc/xive.c
> >> +++ b/hw/intc/xive.c
> >> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
> >>       * even futher coalescing in the Router
> >>       */
> >>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
> >> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
> >> -        return;
> >> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
> >> +        bool notify = xive_esb_trigger(&pq);
> >> +
> >> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
> >> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
> >> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
> >> +        }
> >> +
> >> +        /* ESn[Q]=1 : end of notification */
> >> +        if (!notify) {
> >> +            return;
> >> +        }
> >>      }
> >>  
> >>      /*
> >> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
> >>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
> >>  }
> >>  
> >> +/*
> >> + * END ESB MMIO loads
> >> + */
> >> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
> >> +{
> >> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
> >> +    XiveRouter *xrtr = xsrc->xrtr;
> >> +    uint32_t offset = addr & 0xFFF;
> >> +    uint8_t end_blk;
> >> +    uint32_t end_idx;
> >> +    XiveEND end;
> >> +    uint32_t end_esmask;
> >> +    uint8_t pq;
> >> +    uint64_t ret = -1;
> >> +
> >> +    end_blk = xrtr->chip_id;
> >> +    end_idx = addr >> (xsrc->esb_shift + 1);
> >> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
> 
> The current END accessors require a block identifier, hence xrtr->chip_id, 
> but in this case, we don't really need it because we are using the ENDT 
> local to the router/chip. 

> I don't know how to handle simply this case without keeping chip_id :/

I don't really follow how chip_id is relevant here.  AFAICT the END
accessors take a block id and the back end is responsible for
interpreting them.  The ponwernv one will map it to chip id, but the
PAPR one can just ignore it or only use block 0.
Cédric Le Goater Nov. 30, 2018, 6:41 a.m. UTC | #8
On 11/30/18 2:04 AM, David Gibson wrote:
> On Thu, Nov 29, 2018 at 11:06:13PM +0100, Cédric Le Goater wrote:
>> On 11/22/18 6:13 AM, David Gibson wrote:
>>> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
>>>> The Event Notification Descriptor also contains two Event State
>>>> Buffers providing further coalescing of interrupts, one for the
>>>> notification event (ESn) and one for the escalation events (ESe). A
>>>> MMIO page is assigned for each to control the EOI through loads
>>>> only. Stores are not allowed.
>>>>
>>>> The END ESBs are modeled through an object resembling the 'XiveSource'
>>>> It is stateless as the END state bits are backed into the XiveEND
>>>> structure under the XiveRouter and the MMIO accesses follow the same
>>>> rules as for the standard source ESBs.
>>>>
>>>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
>>>> sPAPR. Nevetherless, it provides a mean to study the question in the
>>>> future and validates a bit more the XIVE model.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  include/hw/ppc/xive.h |  20 ++++++
>>>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 178 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>>>> index ce62aaf28343..24301bf2076d 100644
>>>> --- a/include/hw/ppc/xive.h
>>>> +++ b/include/hw/ppc/xive.h
>>>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>>>                          XiveEND *end);
>>>>  
>>>> +/*
>>>> + * XIVE END ESBs
>>>> + */
>>>> +
>>>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
>>>> +#define XIVE_END_SOURCE(obj) \
>>>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
>>>
>>> Is there a particular reason to make this a full QOM object, rather
>>> than just embedding it in the XiveRouter?
>>
>> Coming back on this question because removing the chip_id from the
>> router is a problem for the END triggering. At least with the current
>> design. See below for the comment.
>>
>>>> +typedef struct XiveENDSource {
>>>> +    SysBusDevice parent;
>>>> +
>>>> +    uint32_t        nr_ends;
>>>> +
>>>> +    /* ESB memory region */
>>>> +    uint32_t        esb_shift;
>>>> +    MemoryRegion    esb_mmio;
>>>> +
>>>> +    XiveRouter      *xrtr;
>>>> +} XiveENDSource;
>>>> +
>>>>  /*
>>>>   * For legacy compatibility, the exceptions define up to 256 different
>>>>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
>>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>>> index 9cb001e7b540..5a8882d47a98 100644
>>>> --- a/hw/intc/xive.c
>>>> +++ b/hw/intc/xive.c
>>>> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>>>>       * even futher coalescing in the Router
>>>>       */
>>>>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
>>>> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
>>>> -        return;
>>>> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
>>>> +        bool notify = xive_esb_trigger(&pq);
>>>> +
>>>> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
>>>> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
>>>> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
>>>> +        }
>>>> +
>>>> +        /* ESn[Q]=1 : end of notification */
>>>> +        if (!notify) {
>>>> +            return;
>>>> +        }
>>>>      }
>>>>  
>>>>      /*
>>>> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
>>>>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
>>>>  }
>>>>  
>>>> +/*
>>>> + * END ESB MMIO loads
>>>> + */
>>>> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
>>>> +{
>>>> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
>>>> +    XiveRouter *xrtr = xsrc->xrtr;
>>>> +    uint32_t offset = addr & 0xFFF;
>>>> +    uint8_t end_blk;
>>>> +    uint32_t end_idx;
>>>> +    XiveEND end;
>>>> +    uint32_t end_esmask;
>>>> +    uint8_t pq;
>>>> +    uint64_t ret = -1;
>>>> +
>>>> +    end_blk = xrtr->chip_id;
>>>> +    end_idx = addr >> (xsrc->esb_shift + 1);
>>>> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
>>
>> The current END accessors require a block identifier, hence xrtr->chip_id, 
>> but in this case, we don't really need it because we are using the ENDT 
>> local to the router/chip. 
> 
>> I don't know how to handle simply this case without keeping chip_id :/
> 
> I don't really follow how chip_id is relevant here.  AFAICT the END
> accessors take a block id and the back end is responsible for
> interpreting them.  The ponwernv one will map it to chip id, but the
> PAPR one can just ignore it or only use block 0.

Yes. But the block value comes from the xrtr->chip_id today, on PAPR and
PowerNV, even if it's block 0. 

What I could do is add a "chip-id" property to XiveENDSource possibly.

C.
David Gibson Dec. 3, 2018, 1:14 a.m. UTC | #9
On Fri, Nov 30, 2018 at 07:41:33AM +0100, Cédric Le Goater wrote:
> On 11/30/18 2:04 AM, David Gibson wrote:
> > On Thu, Nov 29, 2018 at 11:06:13PM +0100, Cédric Le Goater wrote:
> >> On 11/22/18 6:13 AM, David Gibson wrote:
> >>> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
> >>>> The Event Notification Descriptor also contains two Event State
> >>>> Buffers providing further coalescing of interrupts, one for the
> >>>> notification event (ESn) and one for the escalation events (ESe). A
> >>>> MMIO page is assigned for each to control the EOI through loads
> >>>> only. Stores are not allowed.
> >>>>
> >>>> The END ESBs are modeled through an object resembling the 'XiveSource'
> >>>> It is stateless as the END state bits are backed into the XiveEND
> >>>> structure under the XiveRouter and the MMIO accesses follow the same
> >>>> rules as for the standard source ESBs.
> >>>>
> >>>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
> >>>> sPAPR. Nevetherless, it provides a mean to study the question in the
> >>>> future and validates a bit more the XIVE model.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>> ---
> >>>>  include/hw/ppc/xive.h |  20 ++++++
> >>>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
> >>>>  2 files changed, 178 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >>>> index ce62aaf28343..24301bf2076d 100644
> >>>> --- a/include/hw/ppc/xive.h
> >>>> +++ b/include/hw/ppc/xive.h
> >>>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >>>>                          XiveEND *end);
> >>>>  
> >>>> +/*
> >>>> + * XIVE END ESBs
> >>>> + */
> >>>> +
> >>>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
> >>>> +#define XIVE_END_SOURCE(obj) \
> >>>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
> >>>
> >>> Is there a particular reason to make this a full QOM object, rather
> >>> than just embedding it in the XiveRouter?
> >>
> >> Coming back on this question because removing the chip_id from the
> >> router is a problem for the END triggering. At least with the current
> >> design. See below for the comment.
> >>
> >>>> +typedef struct XiveENDSource {
> >>>> +    SysBusDevice parent;
> >>>> +
> >>>> +    uint32_t        nr_ends;
> >>>> +
> >>>> +    /* ESB memory region */
> >>>> +    uint32_t        esb_shift;
> >>>> +    MemoryRegion    esb_mmio;
> >>>> +
> >>>> +    XiveRouter      *xrtr;
> >>>> +} XiveENDSource;
> >>>> +
> >>>>  /*
> >>>>   * For legacy compatibility, the exceptions define up to 256 different
> >>>>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
> >>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >>>> index 9cb001e7b540..5a8882d47a98 100644
> >>>> --- a/hw/intc/xive.c
> >>>> +++ b/hw/intc/xive.c
> >>>> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
> >>>>       * even futher coalescing in the Router
> >>>>       */
> >>>>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
> >>>> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
> >>>> -        return;
> >>>> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
> >>>> +        bool notify = xive_esb_trigger(&pq);
> >>>> +
> >>>> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
> >>>> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
> >>>> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
> >>>> +        }
> >>>> +
> >>>> +        /* ESn[Q]=1 : end of notification */
> >>>> +        if (!notify) {
> >>>> +            return;
> >>>> +        }
> >>>>      }
> >>>>  
> >>>>      /*
> >>>> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
> >>>>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
> >>>>  }
> >>>>  
> >>>> +/*
> >>>> + * END ESB MMIO loads
> >>>> + */
> >>>> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
> >>>> +{
> >>>> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
> >>>> +    XiveRouter *xrtr = xsrc->xrtr;
> >>>> +    uint32_t offset = addr & 0xFFF;
> >>>> +    uint8_t end_blk;
> >>>> +    uint32_t end_idx;
> >>>> +    XiveEND end;
> >>>> +    uint32_t end_esmask;
> >>>> +    uint8_t pq;
> >>>> +    uint64_t ret = -1;
> >>>> +
> >>>> +    end_blk = xrtr->chip_id;
> >>>> +    end_idx = addr >> (xsrc->esb_shift + 1);
> >>>> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
> >>
> >> The current END accessors require a block identifier, hence xrtr->chip_id, 
> >> but in this case, we don't really need it because we are using the ENDT 
> >> local to the router/chip. 
> > 
> >> I don't know how to handle simply this case without keeping chip_id :/
> > 
> > I don't really follow how chip_id is relevant here.  AFAICT the END
> > accessors take a block id and the back end is responsible for
> > interpreting them.  The ponwernv one will map it to chip id, but the
> > PAPR one can just ignore it or only use block 0.
> 
> Yes. But the block value comes from the xrtr->chip_id today, on PAPR and
> PowerNV, even if it's block 0. 
> 
> What I could do is add a "chip-id" property to XiveENDSource possibly.

This still seems wrong for the PAPR model.  Why can't you configure
the end_block value directly in the Xive components, then just set it
equal to the chip_id when you build the powernv machine?
Cédric Le Goater Dec. 3, 2018, 4:19 p.m. UTC | #10
On 12/3/18 2:14 AM, David Gibson wrote:
> On Fri, Nov 30, 2018 at 07:41:33AM +0100, Cédric Le Goater wrote:
>> On 11/30/18 2:04 AM, David Gibson wrote:
>>> On Thu, Nov 29, 2018 at 11:06:13PM +0100, Cédric Le Goater wrote:
>>>> On 11/22/18 6:13 AM, David Gibson wrote:
>>>>> On Fri, Nov 16, 2018 at 11:56:59AM +0100, Cédric Le Goater wrote:
>>>>>> The Event Notification Descriptor also contains two Event State
>>>>>> Buffers providing further coalescing of interrupts, one for the
>>>>>> notification event (ESn) and one for the escalation events (ESe). A
>>>>>> MMIO page is assigned for each to control the EOI through loads
>>>>>> only. Stores are not allowed.
>>>>>>
>>>>>> The END ESBs are modeled through an object resembling the 'XiveSource'
>>>>>> It is stateless as the END state bits are backed into the XiveEND
>>>>>> structure under the XiveRouter and the MMIO accesses follow the same
>>>>>> rules as for the standard source ESBs.
>>>>>>
>>>>>> END ESBs are not supported by the Linux drivers neither on OPAL nor on
>>>>>> sPAPR. Nevetherless, it provides a mean to study the question in the
>>>>>> future and validates a bit more the XIVE model.
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>>>> ---
>>>>>>  include/hw/ppc/xive.h |  20 ++++++
>>>>>>  hw/intc/xive.c        | 160 +++++++++++++++++++++++++++++++++++++++++-
>>>>>>  2 files changed, 178 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>>>>>> index ce62aaf28343..24301bf2076d 100644
>>>>>> --- a/include/hw/ppc/xive.h
>>>>>> +++ b/include/hw/ppc/xive.h
>>>>>> @@ -208,6 +208,26 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>>>>>  int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
>>>>>>                          XiveEND *end);
>>>>>>  
>>>>>> +/*
>>>>>> + * XIVE END ESBs
>>>>>> + */
>>>>>> +
>>>>>> +#define TYPE_XIVE_END_SOURCE "xive-end-source"
>>>>>> +#define XIVE_END_SOURCE(obj) \
>>>>>> +    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
>>>>>
>>>>> Is there a particular reason to make this a full QOM object, rather
>>>>> than just embedding it in the XiveRouter?
>>>>
>>>> Coming back on this question because removing the chip_id from the
>>>> router is a problem for the END triggering. At least with the current
>>>> design. See below for the comment.
>>>>
>>>>>> +typedef struct XiveENDSource {
>>>>>> +    SysBusDevice parent;
>>>>>> +
>>>>>> +    uint32_t        nr_ends;
>>>>>> +
>>>>>> +    /* ESB memory region */
>>>>>> +    uint32_t        esb_shift;
>>>>>> +    MemoryRegion    esb_mmio;
>>>>>> +
>>>>>> +    XiveRouter      *xrtr;
>>>>>> +} XiveENDSource;
>>>>>> +
>>>>>>  /*
>>>>>>   * For legacy compatibility, the exceptions define up to 256 different
>>>>>>   * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
>>>>>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>>>>>> index 9cb001e7b540..5a8882d47a98 100644
>>>>>> --- a/hw/intc/xive.c
>>>>>> +++ b/hw/intc/xive.c
>>>>>> @@ -622,8 +622,18 @@ static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
>>>>>>       * even futher coalescing in the Router
>>>>>>       */
>>>>>>      if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
>>>>>> -        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
>>>>>> -        return;
>>>>>> +        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
>>>>>> +        bool notify = xive_esb_trigger(&pq);
>>>>>> +
>>>>>> +        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
>>>>>> +            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
>>>>>> +            xive_router_set_end(xrtr, end_blk, end_idx, &end);
>>>>>> +        }
>>>>>> +
>>>>>> +        /* ESn[Q]=1 : end of notification */
>>>>>> +        if (!notify) {
>>>>>> +            return;
>>>>>> +        }
>>>>>>      }
>>>>>>  
>>>>>>      /*
>>>>>> @@ -706,6 +716,151 @@ void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
>>>>>>                     (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
>>>>>>  }
>>>>>>  
>>>>>> +/*
>>>>>> + * END ESB MMIO loads
>>>>>> + */
>>>>>> +static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
>>>>>> +{
>>>>>> +    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
>>>>>> +    XiveRouter *xrtr = xsrc->xrtr;
>>>>>> +    uint32_t offset = addr & 0xFFF;
>>>>>> +    uint8_t end_blk;
>>>>>> +    uint32_t end_idx;
>>>>>> +    XiveEND end;
>>>>>> +    uint32_t end_esmask;
>>>>>> +    uint8_t pq;
>>>>>> +    uint64_t ret = -1;
>>>>>> +
>>>>>> +    end_blk = xrtr->chip_id;
>>>>>> +    end_idx = addr >> (xsrc->esb_shift + 1);
>>>>>> +    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
>>>>
>>>> The current END accessors require a block identifier, hence xrtr->chip_id, 
>>>> but in this case, we don't really need it because we are using the ENDT 
>>>> local to the router/chip. 
>>>
>>>> I don't know how to handle simply this case without keeping chip_id :/
>>>
>>> I don't really follow how chip_id is relevant here.  AFAICT the END
>>> accessors take a block id and the back end is responsible for
>>> interpreting them.  The ponwernv one will map it to chip id, but the
>>> PAPR one can just ignore it or only use block 0.
>>
>> Yes. But the block value comes from the xrtr->chip_id today, on PAPR and
>> PowerNV, even if it's block 0. 
>>
>> What I could do is add a "chip-id" property to XiveENDSource possibly.
> 
> This still seems wrong for the PAPR model. 

We don't really care for PAPR. We can use 0 but the model is common
with PowerNV.

> Why can't you configure the end_block value directly in the Xive 
> components, 

That is what I am proposing to add a "chip-id" property to XiveENDSource 

> then just set it equal to the chip_id when you build the powernv 
> machine?

yes. that's how it's more or less built. It's a little more complex
on PowerNV because there are low level settings on how the chip_id is
used in the PC and VC, but the modeling is minimal.

C.
diff mbox series

Patch

diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index ce62aaf28343..24301bf2076d 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -208,6 +208,26 @@  int xive_router_get_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
 int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
                         XiveEND *end);
 
+/*
+ * XIVE END ESBs
+ */
+
+#define TYPE_XIVE_END_SOURCE "xive-end-source"
+#define XIVE_END_SOURCE(obj) \
+    OBJECT_CHECK(XiveENDSource, (obj), TYPE_XIVE_END_SOURCE)
+
+typedef struct XiveENDSource {
+    SysBusDevice parent;
+
+    uint32_t        nr_ends;
+
+    /* ESB memory region */
+    uint32_t        esb_shift;
+    MemoryRegion    esb_mmio;
+
+    XiveRouter      *xrtr;
+} XiveENDSource;
+
 /*
  * For legacy compatibility, the exceptions define up to 256 different
  * priorities. P9 implements only 9 levels : 8 active levels [0 - 7]
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index 9cb001e7b540..5a8882d47a98 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -622,8 +622,18 @@  static void xive_router_end_notify(XiveRouter *xrtr, uint8_t end_blk,
      * even futher coalescing in the Router
      */
     if (!(end.w0 & END_W0_UCOND_NOTIFY)) {
-        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
-        return;
+        uint8_t pq = GETFIELD(END_W1_ESn, end.w1);
+        bool notify = xive_esb_trigger(&pq);
+
+        if (pq != GETFIELD(END_W1_ESn, end.w1)) {
+            end.w1 = SETFIELD(END_W1_ESn, end.w1, pq);
+            xive_router_set_end(xrtr, end_blk, end_idx, &end);
+        }
+
+        /* ESn[Q]=1 : end of notification */
+        if (!notify) {
+            return;
+        }
     }
 
     /*
@@ -706,6 +716,151 @@  void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon)
                    (uint32_t) GETFIELD(EAS_END_DATA, eas->w));
 }
 
+/*
+ * END ESB MMIO loads
+ */
+static uint64_t xive_end_source_read(void *opaque, hwaddr addr, unsigned size)
+{
+    XiveENDSource *xsrc = XIVE_END_SOURCE(opaque);
+    XiveRouter *xrtr = xsrc->xrtr;
+    uint32_t offset = addr & 0xFFF;
+    uint8_t end_blk;
+    uint32_t end_idx;
+    XiveEND end;
+    uint32_t end_esmask;
+    uint8_t pq;
+    uint64_t ret = -1;
+
+    end_blk = xrtr->chip_id;
+    end_idx = addr >> (xsrc->esb_shift + 1);
+    if (xive_router_get_end(xrtr, end_blk, end_idx, &end)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No END %x/%x\n", end_blk,
+                      end_idx);
+        return -1;
+    }
+
+    if (!(end.w0 & END_W0_VALID)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: END %x/%x is invalid\n",
+                      end_blk, end_idx);
+        return -1;
+    }
+
+    end_esmask = addr_is_even(addr, xsrc->esb_shift) ? END_W1_ESn : END_W1_ESe;
+    pq = GETFIELD(end_esmask, end.w1);
+
+    switch (offset) {
+    case XIVE_ESB_LOAD_EOI ... XIVE_ESB_LOAD_EOI + 0x7FF:
+        ret = xive_esb_eoi(&pq);
+
+        /* Forward the source event notification for routing ?? */
+        break;
+
+    case XIVE_ESB_GET ... XIVE_ESB_GET + 0x3FF:
+        ret = pq;
+        break;
+
+    case XIVE_ESB_SET_PQ_00 ... XIVE_ESB_SET_PQ_00 + 0x0FF:
+    case XIVE_ESB_SET_PQ_01 ... XIVE_ESB_SET_PQ_01 + 0x0FF:
+    case XIVE_ESB_SET_PQ_10 ... XIVE_ESB_SET_PQ_10 + 0x0FF:
+    case XIVE_ESB_SET_PQ_11 ... XIVE_ESB_SET_PQ_11 + 0x0FF:
+        ret = xive_esb_set(&pq, (offset >> 8) & 0x3);
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid END ESB load addr %d\n",
+                      offset);
+        return -1;
+    }
+
+    if (pq != GETFIELD(end_esmask, end.w1)) {
+        end.w1 = SETFIELD(end_esmask, end.w1, pq);
+        xive_router_set_end(xrtr, end_blk, end_idx, &end);
+    }
+
+    return ret;
+}
+
+/*
+ * END ESB MMIO stores are invalid
+ */
+static void xive_end_source_write(void *opaque, hwaddr addr,
+                                  uint64_t value, unsigned size)
+{
+    qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid ESB write addr 0x%"
+                  HWADDR_PRIx"\n", addr);
+}
+
+static const MemoryRegionOps xive_end_source_ops = {
+    .read = xive_end_source_read,
+    .write = xive_end_source_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .valid = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+    .impl = {
+        .min_access_size = 8,
+        .max_access_size = 8,
+    },
+};
+
+static void xive_end_source_realize(DeviceState *dev, Error **errp)
+{
+    XiveENDSource *xsrc = XIVE_END_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->xrtr = XIVE_ROUTER(obj);
+
+    if (!xsrc->nr_ends) {
+        error_setg(errp, "Number of interrupt needs to be greater than 0");
+        return;
+    }
+
+    if (xsrc->esb_shift != XIVE_ESB_4K &&
+        xsrc->esb_shift != XIVE_ESB_64K) {
+        error_setg(errp, "Invalid ESB shift setting");
+        return;
+    }
+
+    /*
+     * Each END is assigned an even/odd pair of MMIO pages, the even page
+     * manages the ESn field while the odd page manages the ESe field.
+     */
+    memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc),
+                          &xive_end_source_ops, xsrc, "xive.end",
+                          (1ull << (xsrc->esb_shift + 1)) * xsrc->nr_ends);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &xsrc->esb_mmio);
+}
+
+static Property xive_end_source_properties[] = {
+    DEFINE_PROP_UINT32("nr-ends", XiveENDSource, nr_ends, 0),
+    DEFINE_PROP_UINT32("shift", XiveENDSource, esb_shift, XIVE_ESB_64K),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void xive_end_source_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc    = "XIVE END Source";
+    dc->props   = xive_end_source_properties;
+    dc->realize = xive_end_source_realize;
+}
+
+static const TypeInfo xive_end_source_info = {
+    .name          = TYPE_XIVE_END_SOURCE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XiveENDSource),
+    .class_init    = xive_end_source_class_init,
+};
+
 /*
  * XIVE Fabric
  */
@@ -720,6 +875,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_register_static(&xive_end_source_info);
 }
 
 type_init(xive_register_types)