Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability

From: Greg Kurz <groug@kaod.org>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: Gustavo Romero <gromero@linux.ibm.com>,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability
Date: Tue, 6 Oct 2020 19:39:09 +0200	[thread overview]
Message-ID: <20201006193909.617cc487@bahia.lan> (raw)
In-Reply-To: <20201005165147.526426-7-clg@kaod.org>

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(