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

From: Greg Kurz <groug@kaod.org>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: Gustavo Romero <gromero@linux.ibm.com>,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level
Date: Tue, 6 Oct 2020 19:06:45 +0200	[thread overview]
Message-ID: <20201006190645.33a05694@bahia.lan> (raw)
In-Reply-To: <20201005165147.526426-6-clg@kaod.org>

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 ?

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