Re: [PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level

[Cédric Le Goater]

On 10/6/20 7:06 PM, Greg Kurz wrote:
> On Mon, 5 Oct 2020 18:51:46 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> When the StoreEOI capability is "on", the H_INT_GET_SOURCE_INFO will
>> set the StoreEOI flag for all sources. This could be an issue if
>> StoreEOI is not supported on a specific source, of a passthrough
>> device for instance. In that case, we could either introduce a new KVM
>> ioctl to query the characteristics of the source at the HW level or
>> deactivate StoreEOI on the machine.
>>
>> This is theoretically unsafe on a POWER9 host but it still runs.
>>
> 
> Patch looks good but as said before, what is the likeliness of something
> really painful to happen on a POWER9 host ?

Nothing will happen because POWER9 systems deactivate StoreEOI. You need
a custom skiboot to add it back.

C.


>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/intc/spapr_xive.c | 1 +
>>  hw/ppc/spapr_irq.c   | 6 ++++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 1fa09f287ac0..41f2719ff93a 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -280,6 +280,7 @@ static void spapr_xive_instance_init(Object *obj)
>>      SpaprXive *xive = SPAPR_XIVE(obj);
>>  
>>      object_initialize_child(obj, "source", &xive->source, TYPE_XIVE_SOURCE);
>> +    object_property_add_alias(obj, "flags", OBJECT(&xive->source), "flags");
>>  
>>      object_initialize_child(obj, "end_source", &xive->end_source,
>>                              TYPE_XIVE_END_SOURCE);
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index f59960339ec3..cdf9f9df4173 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -325,9 +325,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
>> **errp)
>>  
>>      if (spapr->irq->xive) {
>>          uint32_t nr_servers = spapr_max_server_number(spapr);
>> +        uint64_t flags = 0;
>>          DeviceState *dev;
>>          int i;
>>  
>> +        if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_ON) {
>> +            flags |= XIVE_SRC_STORE_EOI;
>> +        }
>> +
>>          dev = qdev_new(TYPE_SPAPR_XIVE);
>>          qdev_prop_set_uint32(dev, "nr-irqs", smc->nr_xirqs + 
>> SPAPR_XIRQ_BASE);
>>          /*
>> @@ -337,6 +342,7 @@ void spapr_irq_init(SpaprMachineState *spapr, Error 
>> **errp)
>>          qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3);
>>          object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr),
>>                                   &error_abort);
>> +        object_property_set_int(OBJECT(dev), "flags", flags, &error_abort);
>>          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>  
>>          spapr->xive = SPAPR_XIVE(dev);
>