Re: [PATCH 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests

[Jan Beulich <jbeulich@suse.com>]

On 01.02.2022 17:46, Roger Pau Monne wrote:
> Allow HVM guests untrapped access to MSR_VIRT_SPEC_CTRL if the
> hardware has support for it. This requires adding logic in the
> vm{entry,exit} paths for SVM in order to context switch between the
> hypervisor value and the guest one. The added handlers for context
> switch will also be used for the legacy SSBD support.

So by "hardware" you mean virtual hardware here, when we run
virtualized ourselves? While the wording in AMD's whitepaper suggests
hardware could exist with both MSRs implemented, so far it was my
understanding that VIRT_SPEC_CTRL was rather left for hypervisors to
implement. Maybe I'm wrong with this, in which case some of the
further comments may also be wrong.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -687,6 +687,7 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
>   */
>  void amd_init_ssbd(const struct cpuinfo_x86 *c)
>  {
> +	struct cpu_info *info = get_cpu_info();
>  	int bit = -1;
>  
>  	if (cpu_has_ssb_no)
> @@ -699,7 +700,7 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
>  
>  	if (cpu_has_virt_ssbd) {
>  		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> -		return;
> +		goto out;
>  	}
>  
>  	switch (c->x86) {
> @@ -729,6 +730,10 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
>  
>  	if (bit < 0)
>  		printk_once(XENLOG_ERR "No SSBD controls available\n");
> +
> + out:
> +	info->last_spec_ctrl = info->xen_spec_ctrl = opt_ssbd ? SPEC_CTRL_SSBD
> +							      : 0;
>  }

Besides me being uncertain about the placement of these (preferably
the writes would be where the other similar writes are), this re-use
of the values suggests that you mean to prefer VIRT_SPEC_CTRL use
over that of SPEC_CTRL (see below).

Additionally - the value you store isn't necessarily the value you
wrote to the MSR. It only is if you cam here via the "goto out".

> --- a/xen/arch/x86/hvm/svm/entry.S
> +++ b/xen/arch/x86/hvm/svm/entry.S
> @@ -71,7 +71,9 @@ __UNLIKELY_END(nsvm_hap)
>              mov    %al, CPUINFO_last_spec_ctrl(%rsp)
>  1:          /* No Spectre v1 concerns.  Execution will hit VMRUN imminently. */
>          .endm
> -        ALTERNATIVE "", svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> +        ALTERNATIVE_2 "", STR(call vmentry_virt_spec_ctrl), \

I'm afraid this violates the "ret" part of the warning a few lines up,
while ...

> +                          X86_FEATURE_VIRT_SC_MSR_HVM, \
> +                      svm_vmentry_spec_ctrl, X86_FEATURE_SC_MSR_HVM
>  
>          pop  %r15
>          pop  %r14
> @@ -111,7 +113,9 @@ __UNLIKELY_END(nsvm_hap)
>              wrmsr
>              mov    %al, CPUINFO_last_spec_ctrl(%rsp)
>          .endm
> -        ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> +        ALTERNATIVE_2 "", STR(call vmexit_virt_spec_ctrl), \

... this violates ...

> +                          X86_FEATURE_VIRT_SC_MSR_HVM, \
> +                      svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */

... the "ret" part of this warning.

Furthermore, opposite to what the change to amd_init_ssbd() suggests,
the ordering of the alternatives here means you prefer SPEC_CTRL over
VIRT_SPEC_CTRL; see the comment near the top of _apply_alternatives().
Unless I've missed logic guaranteeing that both of the keyed to
features can't be active at the same time.

Jan