From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/2] x86/spec-ctrl: Reduce HVM RSB overhead where possible
Date: Wed, 10 Aug 2022 10:00:48 +0200 [thread overview]
Message-ID: <b45778e0-9ad3-d32d-e226-7171cfb59394@suse.com> (raw)
In-Reply-To: <20220809170016.25148-3-andrew.cooper3@citrix.com>
On 09.08.2022 19:00, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -44,6 +44,7 @@ ENTRY(vmx_asm_vmexit_handler)
> .endm
> ALTERNATIVE "", restore_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> + /* On PBRSB-vulenrable hardware, `ret` not safe before the start of vmx_vmexit_handler() */
Besides the spelling issue mentioned by Jason I think this line also
wants wrapping. Maybe the two comments also want combining to just
one, such that "WARNING!" clearly applies to both parts.
> @@ -515,7 +515,8 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
> boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) ||
> opt_eager_fpu || opt_md_clear_hvm) ? "" : " None",
> boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ? " MSR_SPEC_CTRL" : "",
> - boot_cpu_has(X86_FEATURE_SC_RSB_HVM) ? " RSB" : "",
> + boot_cpu_has(X86_FEATURE_SC_RSB_HVM) ? " RSB" :
> + boot_cpu_has(X86_BUG_PBRSB) ? " PBRSB" : "",
> opt_eager_fpu ? " EAGER_FPU" : "",
> opt_md_clear_hvm ? " MD_CLEAR" : "",
> boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) ? " IBPB-entry" : "");
Along the lines of half of what fdbf8bdfebc2 ("x86/spec-ctrl:
correct per-guest-type reporting of MD_CLEAR") did, I think you also want
to extend the other (earlier) conditional in this function invocation.
I also wonder whether it wouldn't be helpful to parenthesize the new
construct, such that it'll be more obvious that this is a double
conditional operator determining a single function argument.
> @@ -718,6 +719,77 @@ static bool __init rsb_is_full_width(void)
> return true;
> }
>
> +/*
> + * HVM guests can create arbitrary RSB entries, including ones which point at
> + * Xen supervisor mappings.
> + *
> + * Traditionally, the RSB is not isolated on vmexit, so Xen needs to take
> + * safety precautions to prevent RSB speculation from consuming guest values.
> + *
> + * Intel eIBRS specifies that the RSB is flushed:
> + * 1) on VMExit when IBRS=1, or
> + * 2) shortly thereafter when Xen restores the host IBRS=1 setting.
> + * However, a subset of eIBRS-capable parts also suffer PBRSB and need
> + * software assistance to maintain RSB safety.
> + */
> +static __init enum hvm_rsb {
> + hvm_rsb_none,
> + hvm_rsb_pbrsb,
> + hvm_rsb_stuff32,
> +} hvm_rsb_calculations(uint64_t caps)
> +{
> + if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
> + boot_cpu_data.x86 != 6 )
> + return hvm_rsb_stuff32;
> +
> + if ( !(caps & ARCH_CAPS_IBRS_ALL) )
> + return hvm_rsb_stuff32;
> +
> + if ( caps & ARCH_CAPS_PBRSB_NO )
> + return hvm_rsb_none;
> +
> + /*
> + * We're choosing between the eIBRS-capable models which don't enumerate
> + * PBRSB_NO. Earlier steppings of some models don't enumerate eIBRS and
> + * are excluded above.
> + */
> + switch ( boot_cpu_data.x86_model )
> + {
> + /*
> + * Core (inc Hybrid) CPUs to date (August 2022) are vulenrable.
> + */
> + case 0x55: /* Skylake X */
> + case 0x6a: /* Ice Lake SP */
> + case 0x6c: /* Ice Lake D */
> + case 0x7e: /* Ice Lake client */
> + case 0x8a: /* Lakefield (SNC/TMT) */
> + case 0x8c: /* Tiger Lake U */
> + case 0x8d: /* Tiger Lake H */
> + case 0x8e: /* Skylake-L */
Hmm, is SDM Vol 4's initial table wrong then in stating Kaby Lake /
Coffee Lake for this and ...
> + case 0x97: /* Alder Lake S */
> + case 0x9a: /* Alder Lake H/P/U */
> + case 0x9e: /* Skylake */
... this? Otoh I notice that intel-family.h also says Skylake in
respective comments, despite the constants themselves being named
differently. Yet again ...
> + case 0xa5: /* Comet Lake */
> + case 0xa6: /* Comet Lake U62 */
... you call these Comet Lake when intel-family.h says Skylake also for
these (and names the latter's variable COMETLAKE_L).
What is in the comments here is of course not of primary concern for
getting this patch in, but the named anomalies will stand out when all
of this is switched over to use intel-family.h's constants.
> @@ -1327,9 +1400,33 @@ void __init init_speculation_mitigations(void)
> * HVM guests can always poison the RSB to point at Xen supervisor
> * mappings.
> */
> + hvm_rsb = hvm_rsb_calculations(caps);
> + if ( opt_rsb_hvm == -1 )
> + opt_rsb_hvm = hvm_rsb != hvm_rsb_none;
> +
> if ( opt_rsb_hvm )
> {
> - setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
> + switch ( hvm_rsb )
> + {
> + case hvm_rsb_pbrsb:
> + setup_force_cpu_cap(X86_BUG_PBRSB);
> + break;
> +
> + case hvm_rsb_none:
> + /*
> + * Somewhat arbitrary. If something is wrong and the user has
> + * forced HVM RSB protections on a system where we think nothing
> + * is necessary, they they possibly know something we dont.
> + *
> + * Use stuff32 in this case, which is the most protection we can
> + * muster.
> + */
> + fallthrough;
> +
> + case hvm_rsb_stuff32:
> + setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
> + break;
> + }
>
> /*
> * For SVM, Xen's RSB safety actions are performed before STGI, so
For people using e.g. "spec-ctrl=no-ibrs" but leaving RSB stuffing enabled
(or force-enabling it) we'd need to have an LFENCE somewhere as well. Since
putting one in the RSB stuffing macro would require a runtime conditional
(for its use for alternatives patching), can't we leverage the one
controlled by this logic? That'll be a slight abuse of the name of
X86_BUG_PBRSB, but probably acceptable with a suitable comment. In the end
it'll simply be another fall-through, I guess:
switch ( hvm_rsb )
{
case hvm_rsb_none:
/* ... */
fallthrough;
case hvm_rsb_stuff32:
setup_force_cpu_cap(X86_FEATURE_SC_RSB_HVM);
/* ... */
if ( boot_cpu_has(X86_FEATURE_SC_MSR_HVM) )
break;
fallthrough;
case hvm_rsb_pbrsb:
setup_force_cpu_cap(X86_BUG_PBRSB);
break;
}
That way, aiui, it also wouldn't get in the way of print_details(), which
checks X86_FEATURE_SC_RSB_HVM first.
Otoh I can see reasons why for the stuffing the LFENCE would really need
to live inside the macro, and in particular ahead of the RET there. Since
we don't want to have a runtime conditional there, I guess we should then,
as a fallback, extend the text in the command line doc to warn about the
inter-dependency. After all people "knowing what they are doing" doesn't
imply them knowing implementation details of Xen. But then I'd still be
a little concerned of the "they possibly know something we don't" case.
Jan
next prev parent reply other threads:[~2022-08-10 8:01 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-09 17:00 [PATCH 0/2] x86/spec-ctrl: Reduce HVM RSB overhead Andrew Cooper
2022-08-09 17:00 ` [PATCH 1/2] x86/spec-ctrl: Enumeration for PBRSB_NO Andrew Cooper
2022-08-10 7:10 ` Jan Beulich
2022-08-09 17:00 ` [PATCH 2/2] x86/spec-ctrl: Reduce HVM RSB overhead where possible Andrew Cooper
2022-08-09 20:20 ` Jason Andryuk
2022-08-11 17:05 ` Andrew Cooper
2022-08-10 8:00 ` Jan Beulich [this message]
2022-08-11 18:06 ` Andrew Cooper
2022-08-11 10:55 ` [PATCH 0/2] x86/spec-ctrl: Reduce HVM RSB overhead Andrew Cooper
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=b45778e0-9ad3-d32d-e226-7171cfb59394@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.