| Message ID | 20201005165147.526426-7-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr/xive: Activate StoreEOI in P10 compat guests | expand |
On Mon, 5 Oct 2020 18:51:47 +0200 Cédric Le Goater <clg@kaod.org> wrote: > When the StoreEOI capability is set to "cas", let CAS decide when > StoreEOI should be advertised. StoreEOI is safe to use with a P10 > compat machine because the OS enforces load-after-store ordering but > not with P9 compat. > > The question now is : should we make "cas" the default at the machine > level ? > Hmm, spapr capabilities are knobs for the end user to provide a specific and consistent behavior to the guest... so the "let CAS decide depending on what the guest asked for" thing looks awkward... I mean that making "cas" the default at the machine level looks like you don't really want this to be a capability at all. If the user asked for StoreEOI but the guest chose a compat mode that can't support it, QEMU should simply exit IMHO. Or alternatively I guess you can just forget about the spapr capability and do all the checks (XIVE interrupt controller, KVM support, guest asked for P10) at CAS. > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/spapr.h | 1 + > include/hw/ppc/spapr_xive.h | 1 + > hw/intc/spapr_xive.c | 9 +++++++++ > hw/ppc/spapr_caps.c | 21 +++++++++++++++++---- > hw/ppc/spapr_hcall.c | 7 +++++++ > 5 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index b701c14b4e09..17e7d873e8dc 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -87,6 +87,7 @@ typedef enum { > #define SPAPR_CAP_ON 0x01 > > /* Custom Caps */ > +#define SPAPR_CAP_CAS 0x02 > > /* Generic */ > #define SPAPR_CAP_BROKEN 0x00 > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index 26c8d90d7196..8b8aa586e44f 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive); > > int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > uint32_t *out_server, uint8_t *out_prio); > +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable); > > /* > * KVM XIVE device helpers > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 41f2719ff93a..f57a2681dd91 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -1802,3 +1802,12 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr) > spapr_register_hypercall(H_INT_SYNC, h_int_sync); > spapr_register_hypercall(H_INT_RESET, h_int_reset); > } > + > +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable) > +{ > + if (enable) { > + xive->source.esb_flags |= XIVE_SRC_STORE_EOI; > + } else { > + xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI; > + } > +} > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 9251badbdc27..c55e1fccb9bc 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > } > } > > +SpaprCapPossible cap_storeeoi_possible = { > + .num = 3, > + .vals = { "off", "on", "cas" }, > + .help = "off - no StoreEOI, on - StoreEOI, " > + "cas - negotiated at CAS (POWER10 compat only)", > +}; > + > static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > Error **errp) > { > @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > return; > } > > + /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */ > + if (val == SPAPR_CAP_CAS) { > + return; > + } > + > /* > * load-after-store ordering is not enforced on POWER9 CPUs > * and StoreEOI can be racy. > @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > }, > [SPAPR_CAP_STOREEOI] = { > .name = "storeeoi", > - .description = "Implements XIVE StoreEOI feature", > + .description = "Implements XIVE StoreEOI feature (off, on, cas)", > .index = SPAPR_CAP_STOREEOI, > - .get = spapr_cap_get_bool, > - .set = spapr_cap_set_bool, > - .type = "bool", > + .get = spapr_cap_get_string, > + .set = spapr_cap_set_string, > + .type = "string", > + .possible = &cap_storeeoi_possible, > .apply = cap_storeeoi_apply, > }, > }; > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 607740150fa2..158b122b9192 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, > "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive or ic-mode=dual machine property"); > exit(EXIT_FAILURE); > } > + > + /* Advertise StoreEOI for a P10 compat guest. */ > + if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) { > + bool enable = ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, > + cpu->compat_pvr); > + spapr_xive_enable_store_eoi(spapr->xive, enable); > + } > } else { > if (!spapr->irq->xics) { > error_report(
On 10/6/20 7:39 PM, Greg Kurz wrote: > On Mon, 5 Oct 2020 18:51:47 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> When the StoreEOI capability is set to "cas", let CAS decide when >> StoreEOI should be advertised. StoreEOI is safe to use with a P10 >> compat machine because the OS enforces load-after-store ordering but >> not with P9 compat. >> >> The question now is : should we make "cas" the default at the machine >> level ? >> > > Hmm, spapr capabilities are knobs for the end user to provide a specific > and consistent behavior to the guest... so the "let CAS decide depending > on what the guest asked for" thing looks awkward... The guest doesn't ask for StoreEOI. The hypervisor will return StoreEOI if the kernel is P10 compat and will not if it's P9 to make sure that it can be migrated to a P9 host. > I mean that making > "cas" the default at the machine level looks like you don't really want > this to be a capability at all. It means that you are not forcing a behaviour "off" or "on". It depends on the guest support, P10 or P9, which is what we want as a default behavior to be in sync with pHyp. > If the user asked for StoreEOI but the guest chose a compat mode that > can't support it, QEMU should simply exit IMHO It's not like XICS. A P8 compat can not run on XIVE. A P9 compat can use StoreEOI. So it should be considered more like a mitigation. But anyhow, the P9 systems currenly shipped never activate StoreEOI. So you will get the error. > Or alternatively I guess you can just forget about the spapr capability > and do all the checks (XIVE interrupt controller, KVM support, guest > asked for P10) at CAS. That's how I did in the fisrt patches but then you loose the ability to switch it off on P10 and switch on on P9. Which is good to have for tests and performance and for possible issues if we ever have to handle a source which has different charateristics. In that case, we would switch it off. Thanks, C. >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> include/hw/ppc/spapr.h | 1 + >> include/hw/ppc/spapr_xive.h | 1 + >> hw/intc/spapr_xive.c | 9 +++++++++ >> hw/ppc/spapr_caps.c | 21 +++++++++++++++++---- >> hw/ppc/spapr_hcall.c | 7 +++++++ >> 5 files changed, 35 insertions(+), 4 deletions(-) >> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >> index b701c14b4e09..17e7d873e8dc 100644 >> --- a/include/hw/ppc/spapr.h >> +++ b/include/hw/ppc/spapr.h >> @@ -87,6 +87,7 @@ typedef enum { >> #define SPAPR_CAP_ON 0x01 >> >> /* Custom Caps */ >> +#define SPAPR_CAP_CAS 0x02 >> >> /* Generic */ >> #define SPAPR_CAP_BROKEN 0x00 >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h >> index 26c8d90d7196..8b8aa586e44f 100644 >> --- a/include/hw/ppc/spapr_xive.h >> +++ b/include/hw/ppc/spapr_xive.h >> @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive); >> >> int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, >> uint32_t *out_server, uint8_t *out_prio); >> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable); >> >> /* >> * KVM XIVE device helpers >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >> index 41f2719ff93a..f57a2681dd91 100644 >> --- a/hw/intc/spapr_xive.c >> +++ b/hw/intc/spapr_xive.c >> @@ -1802,3 +1802,12 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr) >> spapr_register_hypercall(H_INT_SYNC, h_int_sync); >> spapr_register_hypercall(H_INT_RESET, h_int_reset); >> } >> + >> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable) >> +{ >> + if (enable) { >> + xive->source.esb_flags |= XIVE_SRC_STORE_EOI; >> + } else { >> + xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI; >> + } >> +} >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c >> index 9251badbdc27..c55e1fccb9bc 100644 >> --- a/hw/ppc/spapr_caps.c >> +++ b/hw/ppc/spapr_caps.c >> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, >> } >> } >> >> +SpaprCapPossible cap_storeeoi_possible = { >> + .num = 3, >> + .vals = { "off", "on", "cas" }, >> + .help = "off - no StoreEOI, on - StoreEOI, " >> + "cas - negotiated at CAS (POWER10 compat only)", >> +}; >> + >> static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, >> Error **errp) >> { >> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, >> return; >> } >> >> + /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */ >> + if (val == SPAPR_CAP_CAS) { >> + return; >> + } >> + >> /* >> * load-after-store ordering is not enforced on POWER9 CPUs >> * and StoreEOI can be racy. >> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { >> }, >> [SPAPR_CAP_STOREEOI] = { >> .name = "storeeoi", >> - .description = "Implements XIVE StoreEOI feature", >> + .description = "Implements XIVE StoreEOI feature (off, on, cas)", >> .index = SPAPR_CAP_STOREEOI, >> - .get = spapr_cap_get_bool, >> - .set = spapr_cap_set_bool, >> - .type = "bool", >> + .get = spapr_cap_get_string, >> + .set = spapr_cap_set_string, >> + .type = "string", >> + .possible = &cap_storeeoi_possible, >> .apply = cap_storeeoi_apply, >> }, >> }; >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 607740150fa2..158b122b9192 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, >> "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive or ic-mode=dual machine property"); >> exit(EXIT_FAILURE); >> } >> + >> + /* Advertise StoreEOI for a P10 compat guest. */ >> + if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) { >> + bool enable = ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, >> + cpu->compat_pvr); >> + spapr_xive_enable_store_eoi(spapr->xive, enable); >> + } >> } else { >> if (!spapr->irq->xics) { >> error_report( >
On Tue, 6 Oct 2020 19:56:20 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 10/6/20 7:39 PM, Greg Kurz wrote: > > On Mon, 5 Oct 2020 18:51:47 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > >> When the StoreEOI capability is set to "cas", let CAS decide when > >> StoreEOI should be advertised. StoreEOI is safe to use with a P10 > >> compat machine because the OS enforces load-after-store ordering but > >> not with P9 compat. Hmm... OS enforcing load-after-store based on a PVR ? I can't find such a thing in the kernel sources. What I find is: if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI) offset |= XIVE_ESB_LD_ST_MO; where xd->flags comes from H_INT_GET_SOURCE_INFO, so this essentially depends on what the hypervisor advertises. For example a P9 compat guest could use it if QEMU has an emulated XIVE, right ? > >> > >> The question now is : should we make "cas" the default at the machine > >> level ? > >> > > > > Hmm, spapr capabilities are knobs for the end user to provide a specific > > and consistent behavior to the guest... so the "let CAS decide depending > > on what the guest asked for" thing looks awkward... > > The guest doesn't ask for StoreEOI. The hypervisor will return StoreEOI > if the kernel is P10 compat and will not if it's P9 to make sure that > it can be migrated to a P9 host. > Yeah I know that the guest doesn't ask for StoreEOI. What I mean is that the idea behind spapr capabilities is only about saying "this machine type offers this feature and the host supports it". The fact that the guest might use it or not is another matter. So I don't quite see how "cas" fits into this model... > > I mean that making > > "cas" the default at the machine level looks like you don't really want > > this to be a capability at all. > > It means that you are not forcing a behaviour "off" or "on". It depends > on the guest support, P10 or P9, which is what we want as a default > behavior to be in sync with pHyp. > I think there's confusion here. Setting the capability to "on"/"off" doesn't mean your force anything. As said above, this isn't related to what the guest supports but essentially to what the host supports. "on" means that we can offer the feature to the guest in a reliable manner because we have verified it can work, and "off" simply means we don't offer the feature (but of course the guest is still free to shoot itself in the foot). The reference to what pHyp does is thus irrelevant here because we won't ever migrate a guest between KVM and PowerVM. We could decide to do nothing and let the OS only do "LoadEOI" operations as it does today. If we want StoreEOI to be usable by default, then the default value should definitely be "on". This should be kept to "off" for older machine types to stay compatible with existing setups. Let's only consider the KVM case only because we don't explicitly support KVM<->TCG migration. The availability of StoreEOI is: - emulated XIVE on P9 system => ok - emulated XIVE on P10 system => ok - in-kernel XIVE on P10 system => ok - in-kernel XIVE on P9 system => not ok This means that on a P9 system we must fallback on emulated XIVE, like we already do with bostons today. This might look sub-optimal from a performance standpoint but this is the price to pay to guarantee migration doesn't fail with the default settings. > > If the user asked for StoreEOI but the guest chose a compat mode that > > can't support it, QEMU should simply exit IMHO > It's not like XICS. A P8 compat can not run on XIVE. A P9 compat can use > StoreEOI. So it should be considered more like a mitigation. But anyhow, > the P9 systems currenly shipped never activate StoreEOI. So you will get > the error. > > > Or alternatively I guess you can just forget about the spapr capability > > and do all the checks (XIVE interrupt controller, KVM support, guest > > asked for P10) at CAS. > > That's how I did in the fisrt patches but then you loose the ability to > switch it off on P10 and switch on on P9. Which is good to have for tests > and performance and for possible issues if we ever have to handle a source > which has different charateristics. In that case, we would switch it off. > I understand that having fine grain knobs is cool for tests and performance but keep in mind that we mostly target end-users who expect this to work out of the box with the default settings. This being said, if some end-users might hit a corner case where it would be valuable to switch it off, then it makes sense to keep the ability to do so of course. > Thanks, > > C. > > > > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> include/hw/ppc/spapr.h | 1 + > >> include/hw/ppc/spapr_xive.h | 1 + > >> hw/intc/spapr_xive.c | 9 +++++++++ > >> hw/ppc/spapr_caps.c | 21 +++++++++++++++++---- > >> hw/ppc/spapr_hcall.c | 7 +++++++ > >> 5 files changed, 35 insertions(+), 4 deletions(-) > >> > >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > >> index b701c14b4e09..17e7d873e8dc 100644 > >> --- a/include/hw/ppc/spapr.h > >> +++ b/include/hw/ppc/spapr.h > >> @@ -87,6 +87,7 @@ typedef enum { > >> #define SPAPR_CAP_ON 0x01 > >> > >> /* Custom Caps */ > >> +#define SPAPR_CAP_CAS 0x02 > >> > >> /* Generic */ > >> #define SPAPR_CAP_BROKEN 0x00 > >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > >> index 26c8d90d7196..8b8aa586e44f 100644 > >> --- a/include/hw/ppc/spapr_xive.h > >> +++ b/include/hw/ppc/spapr_xive.h > >> @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive); > >> > >> int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, > >> uint32_t *out_server, uint8_t *out_prio); > >> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable); > >> > >> /* > >> * KVM XIVE device helpers > >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > >> index 41f2719ff93a..f57a2681dd91 100644 > >> --- a/hw/intc/spapr_xive.c > >> +++ b/hw/intc/spapr_xive.c > >> @@ -1802,3 +1802,12 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr) > >> spapr_register_hypercall(H_INT_SYNC, h_int_sync); > >> spapr_register_hypercall(H_INT_RESET, h_int_reset); > >> } > >> + > >> +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable) > >> +{ > >> + if (enable) { > >> + xive->source.esb_flags |= XIVE_SRC_STORE_EOI; > >> + } else { > >> + xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI; > >> + } > >> +} > >> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > >> index 9251badbdc27..c55e1fccb9bc 100644 > >> --- a/hw/ppc/spapr_caps.c > >> +++ b/hw/ppc/spapr_caps.c > >> @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, > >> } > >> } > >> > >> +SpaprCapPossible cap_storeeoi_possible = { > >> + .num = 3, > >> + .vals = { "off", "on", "cas" }, > >> + .help = "off - no StoreEOI, on - StoreEOI, " > >> + "cas - negotiated at CAS (POWER10 compat only)", > >> +}; > >> + > >> static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > >> Error **errp) > >> { > >> @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, > >> return; > >> } > >> > >> + /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */ > >> + if (val == SPAPR_CAP_CAS) { > >> + return; > >> + } > >> + > >> /* > >> * load-after-store ordering is not enforced on POWER9 CPUs > >> * and StoreEOI can be racy. > >> @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { > >> }, > >> [SPAPR_CAP_STOREEOI] = { > >> .name = "storeeoi", > >> - .description = "Implements XIVE StoreEOI feature", > >> + .description = "Implements XIVE StoreEOI feature (off, on, cas)", > >> .index = SPAPR_CAP_STOREEOI, > >> - .get = spapr_cap_get_bool, > >> - .set = spapr_cap_set_bool, > >> - .type = "bool", > >> + .get = spapr_cap_get_string, > >> + .set = spapr_cap_set_string, > >> + .type = "string", > >> + .possible = &cap_storeeoi_possible, > >> .apply = cap_storeeoi_apply, > >> }, > >> }; > >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > >> index 607740150fa2..158b122b9192 100644 > >> --- a/hw/ppc/spapr_hcall.c > >> +++ b/hw/ppc/spapr_hcall.c > >> @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, > >> "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive or ic-mode=dual machine property"); > >> exit(EXIT_FAILURE); > >> } > >> + > >> + /* Advertise StoreEOI for a P10 compat guest. */ > >> + if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) { > >> + bool enable = ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, > >> + cpu->compat_pvr); > >> + spapr_xive_enable_store_eoi(spapr->xive, enable); > >> + } > >> } else { > >> if (!spapr->irq->xics) { > >> error_report( > > >
> I understand that having fine grain knobs is cool for tests and performance > but keep in mind that we mostly target end-users who expect this to work > out of the box with the default settings. This being said, if some end-users > might hit a corner case where it would be valuable to switch it off, then > it makes sense to keep the ability to do so of course. Yes. Since StoreEOI only applies to P10, we could simplify. We can address that when there is a need in mainline. Thanks for the inputs, C.
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index b701c14b4e09..17e7d873e8dc 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -87,6 +87,7 @@ typedef enum { #define SPAPR_CAP_ON 0x01 /* Custom Caps */ +#define SPAPR_CAP_CAS 0x02 /* Generic */ #define SPAPR_CAP_BROKEN 0x00 diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h index 26c8d90d7196..8b8aa586e44f 100644 --- a/include/hw/ppc/spapr_xive.h +++ b/include/hw/ppc/spapr_xive.h @@ -75,6 +75,7 @@ void spapr_xive_map_mmio(SpaprXive *xive); int spapr_xive_end_to_target(uint8_t end_blk, uint32_t end_idx, uint32_t *out_server, uint8_t *out_prio); +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable); /* * KVM XIVE device helpers diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index 41f2719ff93a..f57a2681dd91 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -1802,3 +1802,12 @@ void spapr_xive_hcall_init(SpaprMachineState *spapr) spapr_register_hypercall(H_INT_SYNC, h_int_sync); spapr_register_hypercall(H_INT_RESET, h_int_reset); } + +void spapr_xive_enable_store_eoi(SpaprXive *xive, bool enable) +{ + if (enable) { + xive->source.esb_flags |= XIVE_SRC_STORE_EOI; + } else { + xive->source.esb_flags &= ~XIVE_SRC_STORE_EOI; + } +} diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index 9251badbdc27..c55e1fccb9bc 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -524,6 +524,13 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, uint8_t val, } } +SpaprCapPossible cap_storeeoi_possible = { + .num = 3, + .vals = { "off", "on", "cas" }, + .help = "off - no StoreEOI, on - StoreEOI, " + "cas - negotiated at CAS (POWER10 compat only)", +}; + static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, Error **errp) { @@ -550,6 +557,11 @@ static void cap_storeeoi_apply(SpaprMachineState *spapr, uint8_t val, return; } + /* CAS will decide to advertise StoreEOI (P10 compat kernels only) */ + if (val == SPAPR_CAP_CAS) { + return; + } + /* * load-after-store ordering is not enforced on POWER9 CPUs * and StoreEOI can be racy. @@ -671,11 +683,12 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { }, [SPAPR_CAP_STOREEOI] = { .name = "storeeoi", - .description = "Implements XIVE StoreEOI feature", + .description = "Implements XIVE StoreEOI feature (off, on, cas)", .index = SPAPR_CAP_STOREEOI, - .get = spapr_cap_get_bool, - .set = spapr_cap_set_bool, - .type = "bool", + .get = spapr_cap_get_string, + .set = spapr_cap_set_string, + .type = "string", + .possible = &cap_storeeoi_possible, .apply = cap_storeeoi_apply, }, }; diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 607740150fa2..158b122b9192 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1804,6 +1804,13 @@ target_ulong do_client_architecture_support(PowerPCCPU *cpu, "Guest requested unavailable interrupt mode (XIVE), try the ic-mode=xive or ic-mode=dual machine property"); exit(EXIT_FAILURE); } + + /* Advertise StoreEOI for a P10 compat guest. */ + if (spapr_get_cap(spapr, SPAPR_CAP_STOREEOI) == SPAPR_CAP_CAS) { + bool enable = ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_10, 0, + cpu->compat_pvr); + spapr_xive_enable_store_eoi(spapr->xive, enable); + } } else { if (!spapr->irq->xics) { error_report(
When the StoreEOI capability is set to "cas", let CAS decide when StoreEOI should be advertised. StoreEOI is safe to use with a P10 compat machine because the OS enforces load-after-store ordering but not with P9 compat. The question now is : should we make "cas" the default at the machine level ? Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/spapr.h | 1 + include/hw/ppc/spapr_xive.h | 1 + hw/intc/spapr_xive.c | 9 +++++++++ hw/ppc/spapr_caps.c | 21 +++++++++++++++++---- hw/ppc/spapr_hcall.c | 7 +++++++ 5 files changed, 35 insertions(+), 4 deletions(-)