| Message ID | 20201005165147.526426-6-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr/xive: Activate StoreEOI in P10 compat guests | expand |
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);
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); >
On Tue, 6 Oct 2020 19:41:28 +0200 Cédric Le Goater <clg@kaod.org> wrote: > 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. > Ok, then: Reviewed-by: Greg Kurz <groug@kaod.org> > 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); > > >
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);
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. 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(+)