| Message ID | 20201005165147.526426-1-clg@kaod.org |
|---|---|
| Headers | show |
| Series | spapr/xive: Activate StoreEOI in P10 compat guests | expand |
On Mon, 5 Oct 2020 18:51:43 +0200 Cédric Le Goater <clg@kaod.org> wrote: > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/spapr_caps.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 57c62c22e4cc..b0a9d0227db2 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -535,6 +535,14 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > return; /* Disabled by default */ > } > > + /* For POWER8 CPUs, setting StoreEOI is useless as XIVE is not used */ > + if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_00, 0, > + spapr->max_compat_pvr)) { It seems that this check is already done during machine init before we get here: spapr_machine_init() spapr_irq_init() spapr_irq_check() So you could maybe just check !spapr->irq->xive I think. And s/on POWER8 CPUs/with XICS/ in the title. > + warn_report("StoreEOI is for the XIVE interrupt mode " > + "(POWER9 and above)"); > + return; > + } > + > /* Check host support when the KVM device is in use */ > if (kvm_irqchip_in_kernel()) { > if (!kvm_storeeoi) {
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 10/6/20 6:42 PM, Greg Kurz wrote: > On Mon, 5 Oct 2020 18:51:42 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> When an interrupt has been handled, the OS notifies the interrupt >> controller with an EOI sequence. On the XIVE interrupt controller >> (POWER9 and POWER10), this can be done with a load or a store >> operation on the ESB interrupt management page of the interrupt. The >> StoreEOI operation has less latency and improves interrupt handling >> performance but it was deactivated during the POWER9 DD2.0 time-frame >> because of ordering issues. POWER9 systems use the LoadEOI instead. >> POWER10 has fixed the issue with a special load command which enforces >> Load-after-Store ordering and StoreEOI can be safely used. >> >> The new StoreEOI capability adds StoreEOI support to the flags >> returned by the hcall H_INT_GET_SOURCE_INFO. When the machine is using >> an emulated interrupt controller, TCG or without kernel IRQ chip, >> there are no limitations and activating StoreEOI is not an issue. >> However, when running with a kernel IRQ chip, some verification needs >> to be done on the host. This is done through the DT, which tells us >> that firmware has configured the HW for StoreEOI, but a new KVM >> capability would be cleaner. >> > > Cleaner and even required... a user could possibly run an older > KVM that doesn't know about StoreEOI on a POWER10 host and QEMU > would wrongly assume the feature is supported. Well, no, because the ESB pages of the interrupts being passthrough in the guest, it should be safe to use StoreEOI in a guest even if the host kernel is not aware of it. As long as HW is correctly configured by FW of course, which is what the DT property says. I agree it's a bit of shortcut but it works. > Also, I guess this > should rather be an attribute of the XIVE KVM device rather than a > plain KVM property. It should even be a source attribute in theory. Since QEMU does not, an attribute of the XIVE KVM device is fine but we would just be looking at the device tree from KVM. So may be, it's easier to make it global to VM without relying on the XIVE device. >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ppc/spapr.h | 4 +++- >> target/ppc/kvm_ppc.h | 6 ++++++ >> hw/ppc/spapr.c | 1 + >> hw/ppc/spapr_caps.c | 30 ++++++++++++++++++++++++++++++ >> target/ppc/kvm.c | 18 ++++++++++++++++++ >> 5 files changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index bba8736269f4..b701c14b4e09 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -74,8 +74,10 @@ typedef enum { >> #define SPAPR_CAP_CCF_ASSIST 0x09 >> /* Implements PAPR FWNMI option */ >> #define SPAPR_CAP_FWNMI 0x0A >> +/* Implements XIVE StoreEOI feature */ >> +#define SPAPR_CAP_STOREEOI 0x0B > > The name should mention XIVE, ie. SPAPR_CAP_XIVE_STOREEOI ok. > >> /* Num Caps */ >> -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1) >> +#define SPAPR_CAP_NUM (SPAPR_CAP_STOREEOI + 1) >> >> /* >> * Capability Values >> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h >> index 72e05f1cd2fc..c5a487dbba13 100644 >> --- a/target/ppc/kvm_ppc.h >> +++ b/target/ppc/kvm_ppc.h >> @@ -64,6 +64,7 @@ bool kvmppc_has_cap_htm(void); >> bool kvmppc_has_cap_mmu_radix(void); >> bool kvmppc_has_cap_mmu_hash_v3(void); >> bool kvmppc_has_cap_xive(void); >> +bool kvmppc_has_cap_xive_storeeoi(void); >> int kvmppc_get_cap_safe_cache(void); >> int kvmppc_get_cap_safe_bounds_check(void); >> int kvmppc_get_cap_safe_indirect_branch(void); >> @@ -346,6 +347,11 @@ static inline bool kvmppc_has_cap_xive(void) >> return false; >> } >> >> +static inline bool kvmppc_has_cap_xive_storeeoi(void) >> +{ >> + return false; >> +} >> + >> static inline int kvmppc_get_cap_safe_cache(void) >> { >> return 0; >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 4256794f3bed..e83de0580142 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -4447,6 +4447,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) >> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON; >> smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON; >> smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON; >> + smc->default_caps.caps[SPAPR_CAP_STOREEOI] = SPAPR_CAP_OFF; >> spapr_caps_add_properties(smc); >> smc->irq = &spapr_irq_dual; >> smc->dr_phb_enabled = true; >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >> index 9341e9782a3f..57c62c22e4cc 100644 >> --- a/hw/ppc/spapr_caps.c >> +++ b/hw/ppc/spapr_caps.c >> @@ -524,6 +524,26 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, >> } >> } >> >> +static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, >> + Error **errp) >> +{ >> + ERRP_GUARD(); > > From "qapi/error.h": > > * = Why, when and how to use ERRP_GUARD() = > * > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > * - It must not be dereferenced, because it may be null. > * - It should not be passed to error_prepend() or > * error_append_hint(), because that doesn't work with &error_fatal. > * ERRP_GUARD() lifts these restrictions. > * > * To use ERRP_GUARD(), add it right at the beginning of the function. > * @errp can then be used without worrying about the argument being > * NULL or &error_fatal. > * > * Using it when it's not needed is safe, but please avoid cluttering > * the source with useless code. > * > > So for this ERRP_GUARD() to be justified, you should come up with > a hint, otherwise you should drop it. OK. >> + MachineState *machine = MACHINE(spapr); >> + bool kvm_storeeoi = kvmppc_has_cap_xive_storeeoi(); >> + >> + if (!val) { >> + return; /* Disabled by default */ >> + } >> + >> + /* Check host support when the KVM device is in use */ >> + if (kvm_irqchip_in_kernel()) { > > Hmm... checking kvm_irqchip_in_kernel() is imprecise because > it returns true if either the XIVE or XICS KVM device has > been open, regardless of the flavor we're going to use. This > really depends on the ic-mode setting: True. > 1) xics: we really don't care whether StoreEOI is available or not. > This is very similar to the case of POWER8 in patch 2. Spit a > warning and return. yes > 2) xive: at this point the XIVE KVM device is open and we can check > the availability of StoreEOI with kvm_device_check_attr(). yes and similar to kvmppc_has_cap_xive_storeeoi() in a sense. > 3) dual: this one is problematic because at this point the XICS KVM > device is open but XIVE KVM won't be open until CAS. > > So I think we can only do something sensible for cases 1) and 2), > eg: > > if (!spapr->irq->xive) { > warn_report(...); > return; > } > > if (spapr_xive_in_kernel(spapr->xive)) { > !kvm_device_check_attr(spapr->xive->fd, ...) { > error_setg(errp, "StoreEOI not supported by XIVE KVM"); > return; > } > > Case 3) requires a similar check in CAS if the guest asked for XIVE > and cap-xive-storeeoi=on. Yes. dual is more complex because we could be using the XIVE emulated device which does not have limitations on P9. I need to take a closer look. >> + if (!kvm_storeeoi) { >> + error_setg(errp, "StoreEOI not supported by KVM"); >> + return; >> + } >> + } >> +} >> + >> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { >> [SPAPR_CAP_HTM] = { >> .name = "htm", >> @@ -632,6 +652,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { >> .type = "bool", >> .apply = cap_fwnmi_apply, >> }, >> + [SPAPR_CAP_STOREEOI] = { >> + .name = "storeeoi", >> + .description = "Implements XIVE StoreEOI feature", >> + .index = SPAPR_CAP_STOREEOI, >> + .get = spapr_cap_get_bool, >> + .set = spapr_cap_set_bool, >> + .type = "bool", >> + .apply = cap_storeeoi_apply, >> + }, >> }; >> >> static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr, >> @@ -772,6 +801,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); >> SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); >> SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); >> SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI); >> +SPAPR_CAP_MIG_STATE(storeeoi, SPAPR_CAP_STOREEOI); >> >> void spapr_caps_init(SpaprMachineState *spapr) >> { >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c >> index d85ba8ffe00b..9ad637151070 100644 >> --- a/target/ppc/kvm.c >> +++ b/target/ppc/kvm.c >> @@ -2448,6 +2448,24 @@ bool kvmppc_has_cap_xive(void) >> return cap_xive; >> } >> >> +/* >> + * TODO: Introduce a new KVM capability >> + */ > > Is there anything that prevents to add such a capability > or KVM device attribute before modifying QEMU ? no. I was just lazy as the device tree check is good enough. Thanks for the review, C. >> +bool kvmppc_has_cap_xive_storeeoi(void) >> +{ >> + static const char *compat = "ibm,opal-xive-pe"; >> + void *host_fdt; >> + int xive_node; >> + >> + host_fdt = load_device_tree_from_sysfs(); >> + xive_node = fdt_node_offset_by_compatible(host_fdt, -1, compat); >> + if (xive_node < 0) { >> + return false; >> + } >> + >> + return !!fdt_getprop(host_fdt, xive_node, "store-eoi-support", NULL); >> +} >> + >> static void kvmppc_get_cpu_characteristics(KVMState *s) >> { >> struct kvm_ppc_cpu_char c; >
On Wed, 7 Oct 2020 07:59:26 +0200 Cédric Le Goater <clg@kaod.org> wrote: > > > On 10/6/20 6:42 PM, Greg Kurz wrote: > > On Mon, 5 Oct 2020 18:51:42 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> When an interrupt has been handled, the OS notifies the interrupt > >> controller with an EOI sequence. On the XIVE interrupt controller > >> (POWER9 and POWER10), this can be done with a load or a store > >> operation on the ESB interrupt management page of the interrupt. The > >> StoreEOI operation has less latency and improves interrupt handling > >> performance but it was deactivated during the POWER9 DD2.0 time-frame > >> because of ordering issues. POWER9 systems use the LoadEOI instead. > >> POWER10 has fixed the issue with a special load command which enforces > >> Load-after-Store ordering and StoreEOI can be safely used. > >> > >> The new StoreEOI capability adds StoreEOI support to the flags > >> returned by the hcall H_INT_GET_SOURCE_INFO. When the machine is using > >> an emulated interrupt controller, TCG or without kernel IRQ chip, > >> there are no limitations and activating StoreEOI is not an issue. > >> However, when running with a kernel IRQ chip, some verification needs > >> to be done on the host. This is done through the DT, which tells us > >> that firmware has configured the HW for StoreEOI, but a new KVM > >> capability would be cleaner. > >> > > > > Cleaner and even required... a user could possibly run an older > > KVM that doesn't know about StoreEOI on a POWER10 host and QEMU > > would wrongly assume the feature is supported. > > Well, no, because the ESB pages of the interrupts being passthrough > in the guest, it should be safe to use StoreEOI in a guest even if > the host kernel is not aware of it. As long as HW is correctly > configured by FW of course, which is what the DT property says. > > I agree it's a bit of shortcut but it works. > Ok this makes sense. I suggest you write this down in a comment in kvmppc_has_cap_xive_storeeoi() then because it is only _obvious_ to the very few people that know about XIVE internals. > > Also, I guess this > > should rather be an attribute of the XIVE KVM device rather than a > > plain KVM property. > > It should even be a source attribute in theory. Since QEMU does not, > an attribute of the XIVE KVM device is fine but we would just be > looking at the device tree from KVM. So may be, it's easier to make > it global to VM without relying on the XIVE device. > Well if KVM doesn't explicitly exploit StoreEOI, not sure this would bring much to expose this through a KVM interface at all in the end... > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> include/hw/ppc/spapr.h | 4 +++- > >> target/ppc/kvm_ppc.h | 6 ++++++ > >> hw/ppc/spapr.c | 1 + > >> hw/ppc/spapr_caps.c | 30 ++++++++++++++++++++++++++++++ > >> target/ppc/kvm.c | 18 ++++++++++++++++++ > >> 5 files changed, 58 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > >> index bba8736269f4..b701c14b4e09 100644 > >> --- a/include/hw/ppc/spapr.h > >> +++ b/include/hw/ppc/spapr.h > >> @@ -74,8 +74,10 @@ typedef enum { > >> #define SPAPR_CAP_CCF_ASSIST 0x09 > >> /* Implements PAPR FWNMI option */ > >> #define SPAPR_CAP_FWNMI 0x0A > >> +/* Implements XIVE StoreEOI feature */ > >> +#define SPAPR_CAP_STOREEOI 0x0B > > > > The name should mention XIVE, ie. SPAPR_CAP_XIVE_STOREEOI > > ok. > > > > >> /* Num Caps */ > >> -#define SPAPR_CAP_NUM (SPAPR_CAP_FWNMI + 1) > >> +#define SPAPR_CAP_NUM (SPAPR_CAP_STOREEOI + 1) > >> > >> /* > >> * Capability Values > >> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h > >> index 72e05f1cd2fc..c5a487dbba13 100644 > >> --- a/target/ppc/kvm_ppc.h > >> +++ b/target/ppc/kvm_ppc.h > >> @@ -64,6 +64,7 @@ bool kvmppc_has_cap_htm(void); > >> bool kvmppc_has_cap_mmu_radix(void); > >> bool kvmppc_has_cap_mmu_hash_v3(void); > >> bool kvmppc_has_cap_xive(void); > >> +bool kvmppc_has_cap_xive_storeeoi(void); > >> int kvmppc_get_cap_safe_cache(void); > >> int kvmppc_get_cap_safe_bounds_check(void); > >> int kvmppc_get_cap_safe_indirect_branch(void); > >> @@ -346,6 +347,11 @@ static inline bool kvmppc_has_cap_xive(void) > >> return false; > >> } > >> > >> +static inline bool kvmppc_has_cap_xive_storeeoi(void) > >> +{ > >> + return false; > >> +} > >> + > >> static inline int kvmppc_get_cap_safe_cache(void) > >> { > >> return 0; > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index 4256794f3bed..e83de0580142 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -4447,6 +4447,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > >> smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON; > >> smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON; > >> smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON; > >> + smc->default_caps.caps[SPAPR_CAP_STOREEOI] = SPAPR_CAP_OFF; > >> spapr_caps_add_properties(smc); > >> smc->irq = &spapr_irq_dual; > >> smc->dr_phb_enabled = true; > >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > >> index 9341e9782a3f..57c62c22e4cc 100644 > >> --- a/hw/ppc/spapr_caps.c > >> +++ b/hw/ppc/spapr_caps.c > >> @@ -524,6 +524,26 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > >> } > >> } > >> > >> +static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > >> + Error **errp) > >> +{ > >> + ERRP_GUARD(); > > > > From "qapi/error.h": > > > > * = Why, when and how to use ERRP_GUARD() = > > * > > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > > * - It must not be dereferenced, because it may be null. > > * - It should not be passed to error_prepend() or > > * error_append_hint(), because that doesn't work with &error_fatal. > > * ERRP_GUARD() lifts these restrictions. > > * > > * To use ERRP_GUARD(), add it right at the beginning of the function. > > * @errp can then be used without worrying about the argument being > > * NULL or &error_fatal. > > * > > * Using it when it's not needed is safe, but please avoid cluttering > > * the source with useless code. > > * > > > > So for this ERRP_GUARD() to be justified, you should come up with > > a hint, otherwise you should drop it. > > OK. > > >> + MachineState *machine = MACHINE(spapr); > >> + bool kvm_storeeoi = kvmppc_has_cap_xive_storeeoi(); > >> + > >> + if (!val) { > >> + return; /* Disabled by default */ > >> + } > >> + > >> + /* Check host support when the KVM device is in use */ > >> + if (kvm_irqchip_in_kernel()) { > > > > Hmm... checking kvm_irqchip_in_kernel() is imprecise because > > it returns true if either the XIVE or XICS KVM device has > > been open, regardless of the flavor we're going to use. This > > really depends on the ic-mode setting: > True. > > 1) xics: we really don't care whether StoreEOI is available or not. > > This is very similar to the case of POWER8 in patch 2. Spit a > > warning and return. > > yes > > > 2) xive: at this point the XIVE KVM device is open and we can check > > the availability of StoreEOI with kvm_device_check_attr(). > > yes and similar to kvmppc_has_cap_xive_storeeoi() in a sense. > > > 3) dual: this one is problematic because at this point the XICS KVM > > device is open but XIVE KVM won't be open until CAS. > > > > So I think we can only do something sensible for cases 1) and 2), > > eg: > > > > if (!spapr->irq->xive) { > > warn_report(...); > > return; > > } > > > > if (spapr_xive_in_kernel(spapr->xive)) { > > !kvm_device_check_attr(spapr->xive->fd, ...) { > > error_setg(errp, "StoreEOI not supported by XIVE KVM"); > > return; > > } > > > > Case 3) requires a similar check in CAS if the guest asked for XIVE > > and cap-xive-storeeoi=on. > > Yes. dual is more complex because we could be using the XIVE emulated > device which does not have limitations on P9. I need to take a closer > look. > Yeah, we should take kernel-irqchip into account as well... > > >> + if (!kvm_storeeoi) { > >> + error_setg(errp, "StoreEOI not supported by KVM"); > >> + return; > >> + } > >> + } > >> +} > >> + > >> SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > >> [SPAPR_CAP_HTM] = { > >> .name = "htm", > >> @@ -632,6 +652,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > >> .type = "bool", > >> .apply = cap_fwnmi_apply, > >> }, > >> + [SPAPR_CAP_STOREEOI] = { > >> + .name = "storeeoi", > >> + .description = "Implements XIVE StoreEOI feature", > >> + .index = SPAPR_CAP_STOREEOI, > >> + .get = spapr_cap_get_bool, > >> + .set = spapr_cap_set_bool, > >> + .type = "bool", > >> + .apply = cap_storeeoi_apply, > >> + }, > >> }; > >> > >> static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr, > >> @@ -772,6 +801,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); > >> SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); > >> SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); > >> SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI); > >> +SPAPR_CAP_MIG_STATE(storeeoi, SPAPR_CAP_STOREEOI); > >> > >> void spapr_caps_init(SpaprMachineState *spapr) > >> { > >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > >> index d85ba8ffe00b..9ad637151070 100644 > >> --- a/target/ppc/kvm.c > >> +++ b/target/ppc/kvm.c > >> @@ -2448,6 +2448,24 @@ bool kvmppc_has_cap_xive(void) > >> return cap_xive; > >> } > >> > >> +/* > >> + * TODO: Introduce a new KVM capability > >> + */ > > > > Is there anything that prevents to add such a capability > > or KVM device attribute before modifying QEMU ? > > no. I was just lazy as the device tree check is good enough. > > Thanks for the review, > > C. > > > >> +bool kvmppc_has_cap_xive_storeeoi(void) > >> +{ > >> + static const char *compat = "ibm,opal-xive-pe"; > >> + void *host_fdt; > >> + int xive_node; > >> + > >> + host_fdt = load_device_tree_from_sysfs(); > >> + xive_node = fdt_node_offset_by_compatible(host_fdt, -1, compat); > >> + if (xive_node < 0) { > >> + return false; > >> + } > >> + > >> + return !!fdt_getprop(host_fdt, xive_node, "store-eoi-support", NULL); > >> +} > >> + > >> static void kvmppc_get_cpu_characteristics(KVMState *s) > >> { > >> struct kvm_ppc_cpu_char c; > >
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); > > >
On Tue, 6 Oct 2020 19:03:28 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 10/6/20 6:58 PM, Greg Kurz wrote: > > On Mon, 5 Oct 2020 18:51:44 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> StoreEOI on POWER9 CPUs is racy because load-after-store ordering is > >> not enforced. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/ppc/spapr_caps.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > >> index b0a9d0227db2..9251badbdc27 100644 > >> --- a/hw/ppc/spapr_caps.c > >> +++ b/hw/ppc/spapr_caps.c > >> @@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > >> error_setg(errp, "StoreEOI not supported by KVM"); > >> return; > >> } > >> + > >> + /* > >> + * load-after-store ordering is not enforced on POWER9 CPUs > >> + * and StoreEOI can be racy. > >> + */ > >> + if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10, > >> + 0, spapr->max_compat_pvr)) { > >> + warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM."); > > The error message should mention XIVE KVM device actually since this only depends on kernel-irqchip. > > It all boils down to what "unsafe" really means here... if the outcome is > > "very likely hang the guest" as soon as it starts doing I/O, shouldn't > > we error out instead ? What is the motivation to use StoreEOI if the > > processor doesn't really support it ? > > We use it in the lab on P9. We have never seen it failed even under stress. > But there is a possible race in the logic. > > C. Thinking again. P9 boston systems on the field only use emulated XIVE and we certainly want to be able to migrate to P9 systems that use in-kernel XIVE and vice-versa. So, even if StoreEOI is technically supported by the emulated XIVE, maybe we should disallow it anyway ?
On 10/7/20 10:56 AM, Greg Kurz wrote: > On Tue, 6 Oct 2020 19:03:28 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> On 10/6/20 6:58 PM, Greg Kurz wrote: >>> On Mon, 5 Oct 2020 18:51:44 +0200 >>> Cédric Le Goater <clg@kaod.org> wrote: >>> >>>> StoreEOI on POWER9 CPUs is racy because load-after-store ordering is >>>> not enforced. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> hw/ppc/spapr_caps.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >>>> index b0a9d0227db2..9251badbdc27 100644 >>>> --- a/hw/ppc/spapr_caps.c >>>> +++ b/hw/ppc/spapr_caps.c >>>> @@ -549,6 +549,15 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, >>>> error_setg(errp, "StoreEOI not supported by KVM"); >>>> return; >>>> } >>>> + >>>> + /* >>>> + * load-after-store ordering is not enforced on POWER9 CPUs >>>> + * and StoreEOI can be racy. >>>> + */ >>>> + if (!ppc_type_check_compat(machine->cpu_type, CPU_POWERPC_LOGICAL_3_10, >>>> + 0, spapr->max_compat_pvr)) { >>>> + warn_report("StoreEOI on a POWER9 CPU is unsafe on KVM."); >>> > > The error message should mention XIVE KVM device actually since this only > depends on kernel-irqchip. > >>> It all boils down to what "unsafe" really means here... if the outcome is >>> "very likely hang the guest" as soon as it starts doing I/O, shouldn't >>> we error out instead ? What is the motivation to use StoreEOI if the >>> processor doesn't really support it ? >> >> We use it in the lab on P9. We have never seen it failed even under stress. >> But there is a possible race in the logic. >> >> C. > > Thinking again. P9 boston systems on the field only use emulated XIVE and > we certainly want to be able to migrate to P9 systems that use in-kernel > XIVE and vice-versa. So, even if StoreEOI is technically supported by > the emulated XIVE, maybe we should disallow it anyway ? yes. But to activate it, one would need to set the capability to "on" because the default is "off". Even with the "cas" capability value introduced at the end of the patchset, StoreEOI would not be advertised to guests running under P9 compat. C.
On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote: > Hello, > > When an interrupt has been handled, the OS notifies the interrupt > controller with an EOI sequence. On the XIVE interrupt controller > (POWER9 and POWER10), this can be done with a load or a store > operation on the ESB interrupt management page of the interrupt. The > StoreEOI operation has less latency and improves interrupt handling > performance but it was deactivated during the POWER9 DD2.0 time-frame > because of ordering issues. POWER9 systems use the LoadEOI instead. > POWER10 has fixed the issue with a special load command which enforces > Load-after-Store ordering and StoreEOI can be safely used. Do you mean that ordering is *always* enforced on P10? Or it's a special form of load that has the ordering? Also, weirdly, despite the series being addressed to me, only some of the patches ended up in my inbox, rather than the list folder :/. > These changes add a new StoreEOI capability which activate StoreEOI > support in the flags returned by the hcall H_INT_GET_SOURCE_INFO. When > the machine is using an emulated interrupt controller, TCG or without > kernel IRQ chip, there are no limitations and activating StoreEOI is > not an issue. However, when running with a kernel IRQ chip, some > verification needs to be done on the host. This is done through the > DT, which tells us that firmware has configured the HW for StoreEOI, > but a new KVM capability would be cleaner. > > The last patch introduces a new 'cas' value to the capability which > lets the hypervisor decide at CAS time if StoreEOI should be > advertised to the guest OS. P10 compat kernel are considered safe > because the OS enforces load-after-store ordering but not with P9. > > The StoreEOI capability is a global setting and does not take into > account the characteristics of a single source. It 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 (like > in v1) or deactivate StoreEOI on the machine. > > We are using these patches today on P10 and P9 (with a custom FW > activating StoreEOI) systems to benchmark interrupt performance on > large guests but there's no hurry to take them. Let's discuss this new > approach. > > Thanks, > > C. > > Changes in v2: > > - completely approach using a capability > > Cédric Le Goater (6): > spapr/xive: Introduce a StoreEOI capability > spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs > spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs > spapr/xive: Enforce load-after-store ordering > spapr/xive: Activate StoreEOI at the source level > spapr/xive: Introduce a new CAS value for the StoreEOI capability > > include/hw/ppc/spapr.h | 5 +++- > include/hw/ppc/spapr_xive.h | 1 + > include/hw/ppc/xive.h | 8 +++++ > target/ppc/kvm_ppc.h | 6 ++++ > hw/intc/spapr_xive.c | 10 +++++++ > hw/intc/spapr_xive_kvm.c | 12 ++++++++ > hw/intc/xive.c | 6 ++++ > hw/ppc/spapr.c | 1 + > hw/ppc/spapr_caps.c | 60 +++++++++++++++++++++++++++++++++++++ > hw/ppc/spapr_hcall.c | 7 +++++ > hw/ppc/spapr_irq.c | 6 ++++ > target/ppc/kvm.c | 18 +++++++++++ > 12 files changed, 139 insertions(+), 1 deletion(-) > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 10/9/20 2:23 AM, David Gibson wrote: > On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote: >> Hello, >> >> When an interrupt has been handled, the OS notifies the interrupt >> controller with an EOI sequence. On the XIVE interrupt controller >> (POWER9 and POWER10), this can be done with a load or a store >> operation on the ESB interrupt management page of the interrupt. The >> StoreEOI operation has less latency and improves interrupt handling >> performance but it was deactivated during the POWER9 DD2.0 time-frame >> because of ordering issues. POWER9 systems use the LoadEOI instead. >> POWER10 has fixed the issue with a special load command which enforces >> Load-after-Store ordering and StoreEOI can be safely used. > > Do you mean that ordering is *always* enforced on P10? Or it's a > special form of load that has the ordering? It's a special form of load that has the ordering, only on available on P10. It's a no-op on P9. Linux commit b1f9be9392f0 ("powerpc/xive: Enforce load-after-store ordering when StoreEOI is active") introduced the Load-after-Store ordering offset and P10 support was added in the same 5.8 release. This is why StoreEOI should be advertised on P10 compat kernels only. I would have preferred to introduce some extra CAS bits. that would have been cleaner than mix the two. The basic requirement is to advertise StoreEOI when the CPU compat allows it. I have used the capabilities to toggle the feature on/off. It seemed a clean way to cover all the extra needs : - switch it off on P10 if needed - switch it on on P9 for tests > Also, weirdly, despite the series being addressed to me, only some of > the patches ended up in my inbox, rather than the list folder :/. Yes. I have received a few ot these : The original message was received at Mon, 5 Oct 2020 12:51:56 -0400 from m0098419.ppops.net [127.0.0.1] ----- The following addresses had permanent fatal errors ----- <david@gibson.dropbear.id.au> ----- Transcript of session follows ----- <david@gibson.dropbear.id.au>... Deferred: Name server: gibson.dropbear.id.au.: host name lookup failure Message could not be delivered for 1 day Message will be deleted from queue Reporting-MTA: dns; mx0b-001b2d01.pphosted.com Arrival-Date: Mon, 5 Oct 2020 12:51:56 -0400 Final-Recipient: RFC822; david@gibson.dropbear.id.au Action: failed Status: 4.4.7 Remote-MTA: DNS; gibson.dropbear.id.au Last-Attempt-Date: Tue, 6 Oct 2020 13:06:28 -0400 >> These changes add a new StoreEOI capability which activate StoreEOI >> support in the flags returned by the hcall H_INT_GET_SOURCE_INFO. When >> the machine is using an emulated interrupt controller, TCG or without >> kernel IRQ chip, there are no limitations and activating StoreEOI is >> not an issue. However, when running with a kernel IRQ chip, some >> verification needs to be done on the host. This is done through the >> DT, which tells us that firmware has configured the HW for StoreEOI, >> but a new KVM capability would be cleaner. >> >> The last patch introduces a new 'cas' value to the capability which >> lets the hypervisor decide at CAS time if StoreEOI should be >> advertised to the guest OS. P10 compat kernel are considered safe >> because the OS enforces load-after-store ordering but not with P9. >> >> The StoreEOI capability is a global setting and does not take into >> account the characteristics of a single source. It 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 (like >> in v1) or deactivate StoreEOI on the machine. >> >> We are using these patches today on P10 and P9 (with a custom FW >> activating StoreEOI) systems to benchmark interrupt performance on >> large guests but there's no hurry to take them. Let's discuss this new >> approach. >> >> Thanks, >> >> C. >> >> Changes in v2: >> >> - completely approach using a capability >> >> Cédric Le Goater (6): >> spapr/xive: Introduce a StoreEOI capability >> spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs >> spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs >> spapr/xive: Enforce load-after-store ordering >> spapr/xive: Activate StoreEOI at the source level >> spapr/xive: Introduce a new CAS value for the StoreEOI capability >> >> include/hw/ppc/spapr.h | 5 +++- >> include/hw/ppc/spapr_xive.h | 1 + >> include/hw/ppc/xive.h | 8 +++++ >> target/ppc/kvm_ppc.h | 6 ++++ >> hw/intc/spapr_xive.c | 10 +++++++ >> hw/intc/spapr_xive_kvm.c | 12 ++++++++ >> hw/intc/xive.c | 6 ++++ >> hw/ppc/spapr.c | 1 + >> hw/ppc/spapr_caps.c | 60 +++++++++++++++++++++++++++++++++++++ >> hw/ppc/spapr_hcall.c | 7 +++++ >> hw/ppc/spapr_irq.c | 6 ++++ >> target/ppc/kvm.c | 18 +++++++++++ >> 12 files changed, 139 insertions(+), 1 deletion(-) >> >
On Fri, Oct 09, 2020 at 07:57:32AM +0200, Cédric Le Goater wrote: > On 10/9/20 2:23 AM, David Gibson wrote: > > On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote: > >> Hello, > >> > >> When an interrupt has been handled, the OS notifies the interrupt > >> controller with an EOI sequence. On the XIVE interrupt controller > >> (POWER9 and POWER10), this can be done with a load or a store > >> operation on the ESB interrupt management page of the interrupt. The > >> StoreEOI operation has less latency and improves interrupt handling > >> performance but it was deactivated during the POWER9 DD2.0 time-frame > >> because of ordering issues. POWER9 systems use the LoadEOI instead. > >> POWER10 has fixed the issue with a special load command which enforces > >> Load-after-Store ordering and StoreEOI can be safely used. > > > > Do you mean that ordering is *always* enforced on P10? Or it's a > > special form of load that has the ordering? > > It's a special form of load that has the ordering, only on available > on P10. It's a no-op on P9. no-op as in the load will have regular semantics, or as in the whole load won't do anything? I assume this meanse XIVE code needs to be updated to use that special load for all accesses to XIVE registers... > Linux commit b1f9be9392f0 ("powerpc/xive: Enforce load-after-store > ordering when StoreEOI is active") introduced the Load-after-Store > ordering offset and P10 support was added in the same 5.8 release. .. which I guess this does? > This is why StoreEOI should be advertised on P10 compat kernels only. > I would have preferred to introduce some extra CAS bits. that would > have been cleaner than mix the two. Ok. > The basic requirement is to advertise StoreEOI when the CPU compat > allows it. I have used the capabilities to toggle the feature on/off. > It seemed a clean way to cover all the extra needs : > > - switch it off on P10 if needed > - switch it on on P9 for tests Ok, seems reasonable > > Also, weirdly, despite the series being addressed to me, only some of > > the patches ended up in my inbox, rather than the list folder :/. > > > Yes. I have received a few ot these : > > The original message was received at Mon, 5 Oct 2020 12:51:56 -0400 > from m0098419.ppops.net [127.0.0.1] > > ----- The following addresses had permanent fatal errors ----- > <david@gibson.dropbear.id.au> Drat, I guess ozlabs.org fell off the net for a while.
Sorry for the late answer I was out for a couple of weeks. On 10/9/20 2:23 AM, David Gibson wrote: > On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote: >> Hello, >> >> When an interrupt has been handled, the OS notifies the interrupt >> controller with an EOI sequence. On the XIVE interrupt controller >> (POWER9 and POWER10), this can be done with a load or a store >> operation on the ESB interrupt management page of the interrupt. The >> StoreEOI operation has less latency and improves interrupt handling >> performance but it was deactivated during the POWER9 DD2.0 time-frame >> because of ordering issues. POWER9 systems use the LoadEOI instead. >> POWER10 has fixed the issue with a special load command which enforces >> Load-after-Store ordering and StoreEOI can be safely used. > > Do you mean that ordering is *always* enforced on P10? Or it's a > special form of load that has the ordering? It's a special load offset that has the ordering. Oring 0x40 to the load address : #define XIVE_ESB_LOAD_EOI 0x000 /* Load */ #define XIVE_ESB_GET 0x800 /* Load */ #define XIVE_ESB_SET_PQ_00 0xc00 /* Load */ #define XIVE_ESB_SET_PQ_01 0xd00 /* Load */ #define XIVE_ESB_SET_PQ_10 0xe00 /* Load */ #define XIVE_ESB_SET_PQ_11 0xf00 /* Load */ will enforce load-after-store ordering. We only need it for XIVE_ESB_SET_PQ_10. See commit b1f9be9392f0 ("powerpc/xive: Enforce load-after-store ordering when StoreEOI is active") in Linux. C. > > Also, weirdly, despite the series being addressed to me, only some of > the patches ended up in my inbox, rather than the list folder :/. > >> These changes add a new StoreEOI capability which activate StoreEOI >> support in the flags returned by the hcall H_INT_GET_SOURCE_INFO. When >> the machine is using an emulated interrupt controller, TCG or without >> kernel IRQ chip, there are no limitations and activating StoreEOI is >> not an issue. However, when running with a kernel IRQ chip, some >> verification needs to be done on the host. This is done through the >> DT, which tells us that firmware has configured the HW for StoreEOI, >> but a new KVM capability would be cleaner. >> >> The last patch introduces a new 'cas' value to the capability which >> lets the hypervisor decide at CAS time if StoreEOI should be >> advertised to the guest OS. P10 compat kernel are considered safe >> because the OS enforces load-after-store ordering but not with P9. >> >> The StoreEOI capability is a global setting and does not take into >> account the characteristics of a single source. It 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 (like >> in v1) or deactivate StoreEOI on the machine. >> >> We are using these patches today on P10 and P9 (with a custom FW >> activating StoreEOI) systems to benchmark interrupt performance on >> large guests but there's no hurry to take them. Let's discuss this new >> approach. >> >> Thanks, >> >> C. >> >> Changes in v2: >> >> - completely approach using a capability >> >> Cédric Le Goater (6): >> spapr/xive: Introduce a StoreEOI capability >> spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs >> spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs >> spapr/xive: Enforce load-after-store ordering >> spapr/xive: Activate StoreEOI at the source level >> spapr/xive: Introduce a new CAS value for the StoreEOI capability >> >> include/hw/ppc/spapr.h | 5 +++- >> include/hw/ppc/spapr_xive.h | 1 + >> include/hw/ppc/xive.h | 8 +++++ >> target/ppc/kvm_ppc.h | 6 ++++ >> hw/intc/spapr_xive.c | 10 +++++++ >> hw/intc/spapr_xive_kvm.c | 12 ++++++++ >> hw/intc/xive.c | 6 ++++ >> hw/ppc/spapr.c | 1 + >> hw/ppc/spapr_caps.c | 60 +++++++++++++++++++++++++++++++++++++ >> hw/ppc/spapr_hcall.c | 7 +++++ >> hw/ppc/spapr_irq.c | 6 ++++ >> target/ppc/kvm.c | 18 +++++++++++ >> 12 files changed, 139 insertions(+), 1 deletion(-) >> >
On Mon, Nov 02, 2020 at 02:22:35PM +0100, Cédric Le Goater wrote: > Sorry for the late answer I was out for a couple of weeks. > > On 10/9/20 2:23 AM, David Gibson wrote: > > On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote: > >> Hello, > >> > >> When an interrupt has been handled, the OS notifies the interrupt > >> controller with an EOI sequence. On the XIVE interrupt controller > >> (POWER9 and POWER10), this can be done with a load or a store > >> operation on the ESB interrupt management page of the interrupt. The > >> StoreEOI operation has less latency and improves interrupt handling > >> performance but it was deactivated during the POWER9 DD2.0 time-frame > >> because of ordering issues. POWER9 systems use the LoadEOI instead. > >> POWER10 has fixed the issue with a special load command which enforces > >> Load-after-Store ordering and StoreEOI can be safely used. > > > > Do you mean that ordering is *always* enforced on P10? Or it's a > > special form of load that has the ordering? > > It's a special load offset that has the ordering. Oring 0x40 to the load > address : > > #define XIVE_ESB_LOAD_EOI 0x000 /* Load */ > #define XIVE_ESB_GET 0x800 /* Load */ > #define XIVE_ESB_SET_PQ_00 0xc00 /* Load */ > #define XIVE_ESB_SET_PQ_01 0xd00 /* Load */ > #define XIVE_ESB_SET_PQ_10 0xe00 /* Load */ > #define XIVE_ESB_SET_PQ_11 0xf00 /* Load */ > > will enforce load-after-store ordering. Oh... I had assumed the problem was to do with the load/store ordering within the CPU core itself (or maybe the L1, I guess). But if the address used can change it, the problem must be within the XIVE, yes? Or at least somwhere on the Powerbus. So, wasn't this just a plain XIVE hardware bug? In which case why is there software involvement as well? > We only need it for XIVE_ESB_SET_PQ_10. See commit b1f9be9392f0 > ("powerpc/xive: Enforce load-after-store ordering when StoreEOI is active") > in Linux. > > C. > > > > > > Also, weirdly, despite the series being addressed to me, only some of > > the patches ended up in my inbox, rather than the list folder :/. > > > >> These changes add a new StoreEOI capability which activate StoreEOI > >> support in the flags returned by the hcall H_INT_GET_SOURCE_INFO. When > >> the machine is using an emulated interrupt controller, TCG or without > >> kernel IRQ chip, there are no limitations and activating StoreEOI is > >> not an issue. However, when running with a kernel IRQ chip, some > >> verification needs to be done on the host. This is done through the > >> DT, which tells us that firmware has configured the HW for StoreEOI, > >> but a new KVM capability would be cleaner. > >> > >> The last patch introduces a new 'cas' value to the capability which > >> lets the hypervisor decide at CAS time if StoreEOI should be > >> advertised to the guest OS. P10 compat kernel are considered safe > >> because the OS enforces load-after-store ordering but not with P9. > >> > >> The StoreEOI capability is a global setting and does not take into > >> account the characteristics of a single source. It 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 (like > >> in v1) or deactivate StoreEOI on the machine. > >> > >> We are using these patches today on P10 and P9 (with a custom FW > >> activating StoreEOI) systems to benchmark interrupt performance on > >> large guests but there's no hurry to take them. Let's discuss this new > >> approach. > >> > >> Thanks, > >> > >> C. > >> > >> Changes in v2: > >> > >> - completely approach using a capability > >> > >> Cédric Le Goater (6): > >> spapr/xive: Introduce a StoreEOI capability > >> spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs > >> spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs > >> spapr/xive: Enforce load-after-store ordering > >> spapr/xive: Activate StoreEOI at the source level > >> spapr/xive: Introduce a new CAS value for the StoreEOI capability > >> > >> include/hw/ppc/spapr.h | 5 +++- > >> include/hw/ppc/spapr_xive.h | 1 + > >> include/hw/ppc/xive.h | 8 +++++ > >> target/ppc/kvm_ppc.h | 6 ++++ > >> hw/intc/spapr_xive.c | 10 +++++++ > >> hw/intc/spapr_xive_kvm.c | 12 ++++++++ > >> hw/intc/xive.c | 6 ++++ > >> hw/ppc/spapr.c | 1 + > >> hw/ppc/spapr_caps.c | 60 +++++++++++++++++++++++++++++++++++++ > >> hw/ppc/spapr_hcall.c | 7 +++++ > >> hw/ppc/spapr_irq.c | 6 ++++ > >> target/ppc/kvm.c | 18 +++++++++++ > >> 12 files changed, 139 insertions(+), 1 deletion(-) > >> > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 11/23/20 7:44 AM, David Gibson wrote: > On Mon, Nov 02, 2020 at 02:22:35PM +0100, Cédric Le Goater wrote: >> Sorry for the late answer I was out for a couple of weeks. >> >> On 10/9/20 2:23 AM, David Gibson wrote: >>> On Mon, Oct 05, 2020 at 06:51:41PM +0200, Cédric Le Goater wrote: >>>> Hello, >>>> >>>> When an interrupt has been handled, the OS notifies the interrupt >>>> controller with an EOI sequence. On the XIVE interrupt controller >>>> (POWER9 and POWER10), this can be done with a load or a store >>>> operation on the ESB interrupt management page of the interrupt. The >>>> StoreEOI operation has less latency and improves interrupt handling >>>> performance but it was deactivated during the POWER9 DD2.0 time-frame >>>> because of ordering issues. POWER9 systems use the LoadEOI instead. >>>> POWER10 has fixed the issue with a special load command which enforces >>>> Load-after-Store ordering and StoreEOI can be safely used. >>> >>> Do you mean that ordering is *always* enforced on P10? Or it's a >>> special form of load that has the ordering? >> >> It's a special load offset that has the ordering. Oring 0x40 to the load >> address : >> >> #define XIVE_ESB_LOAD_EOI 0x000 /* Load */ >> #define XIVE_ESB_GET 0x800 /* Load */ >> #define XIVE_ESB_SET_PQ_00 0xc00 /* Load */ >> #define XIVE_ESB_SET_PQ_01 0xd00 /* Load */ >> #define XIVE_ESB_SET_PQ_10 0xe00 /* Load */ >> #define XIVE_ESB_SET_PQ_11 0xf00 /* Load */ >> >> will enforce load-after-store ordering. > > Oh... I had assumed the problem was to do with the load/store ordering > within the CPU core itself (or maybe the L1, I guess). But if the > address used can change it, the problem must be within the XIVE, yes? Yes. It's in the XIVE logic handling the load/store operations on the PQ bits. > Or at least somwhere on the Powerbus. So, wasn't this just a plain > XIVE hardware bug? It's a theoretical bug in HW. StoreEOI is activated on the P9 systems we use for performance testing and it never showed up. > In which case why is there software involvement as well? Software is involved as an optimization, because only PQ_10 loads need the ordering enforcement. commit b1f9be9392f0 in Linux says more : There is usually no need to enforce ordering between ESB load and store operations as they should lead to the same result. E.g. a store trigger and a load EOI can be executed in any order. Assuming the interrupt state is PQ=10, a store trigger followed by a load EOI will return a Q bit. In the reverse order, it will create a new interrupt trigger from HW. In both cases, the handler processing interrupts is notified. In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to disable temporarily the interrupt source (mask/unmask). When the source is reenabled, the OS can detect if interrupts were received while the source was disabled and reinject them. This process needs special care when StoreEOI is activated. The ESB load and store operations should be correctly ordered because a XIVE_ESB_STORE_EOI operation could leave the source enabled if it has not completed before the loads. For those cases, we enforce Load-after-Store ordering with a special load operation offset. To avoid performance impact, this ordering is only enforced when really needed, that is when interrupt sources are temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not be needed for other loads. This ordering is a requirement for StoreEOI. C.