On Mon, 5 Oct 2020 18:51:47 +0200
Cédric Le Goater <c...@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 <c...@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(
