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(
next prev parent reply other threads:[~2020-10-06 17:41 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-05 16:51 [PATCH v2 0/6] spapr/xive: Activate StoreEOI in P10 compat guests Cédric Le Goater
2020-10-05 16:51 ` [PATCH v2 1/6] spapr/xive: Introduce a StoreEOI capability Cédric Le Goater
2020-10-06 16:42 ` Greg Kurz
2020-10-07 5:59 ` Cédric Le Goater
2020-10-07 7:24 ` Greg Kurz
2020-10-05 16:51 ` [PATCH v2 2/6] spapr/xive: Add a warning when StoreEOI is activated on POWER8 CPUs Cédric Le Goater
2020-10-06 16:52 ` Greg Kurz
2020-10-05 16:51 ` [PATCH v2 3/6] spapr/xive: Add a warning when StoreEOI is activated on POWER9 CPUs Cédric Le Goater
2020-10-06 16:58 ` Greg Kurz
2020-10-06 17:03 ` Cédric Le Goater
2020-10-07 8:56 ` Greg Kurz
2020-10-07 9:21 ` Cédric Le Goater
2020-10-05 16:51 ` [PATCH v2 4/6] spapr/xive: Enforce load-after-store ordering Cédric Le Goater
2020-10-06 17:02 ` Greg Kurz
2020-10-05 16:51 ` [PATCH v2 5/6] spapr/xive: Activate StoreEOI at the source level Cédric Le Goater
2020-10-06 17:06 ` Greg Kurz
2020-10-06 17:41 ` Cédric Le Goater
2020-10-07 7:26 ` Greg Kurz
2020-10-05 16:51 ` [PATCH v2 6/6] spapr/xive: Introduce a new CAS value for the StoreEOI capability Cédric Le Goater
2020-10-06 17:39 ` Greg Kurz [this message]
2020-10-06 17:56 ` Cédric Le Goater
2020-10-07 13:43 ` Greg Kurz
2020-10-07 14:28 ` Cédric Le Goater
2020-10-09 0:23 ` [PATCH v2 0/6] spapr/xive: Activate StoreEOI in P10 compat guests David Gibson
2020-10-09 5:57 ` Cédric Le Goater
2020-10-12 5:38 ` David Gibson
2020-11-02 13:22 ` Cédric Le Goater
2020-11-23 6:44 ` David Gibson
2020-11-23 11:16 ` Cédric Le Goater
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201006193909.617cc487@bahia.lan \
--to=groug@kaod.org \
--cc=clg@kaod.org \
--cc=david@gibson.dropbear.id.au \
--cc=gromero@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).