On 29.10.2022 15:12, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -973,6 +973,16 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
>
> /* Resume use of ISTs now that the host TR is reinstated. */
> enable_each_ist(idt_tables[cpu]);
> +
> + /*
> + * Clear previous guest selection of SSBD if set. Note that
> SPEC_CTRL.SSBD
> + * is already cleared by svm_vmexit_spec_ctrl.
> + */
> + if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> + {
> + ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
> + amd_set_ssbd(false);
> + }
> }
Aren't you potentially turning off SSBD here just to ...
> @@ -1000,6 +1010,13 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
>
> if ( cpu_has_msr_tsc_aux )
> wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
> +
> + /* Load SSBD if set by the guest. */
> + if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
> + {
> + ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
> + amd_set_ssbd(true);
> + }
> }
... turn it on here again? IOW wouldn't switching better be isolated to
just svm_ctxt_switch_to(), doing nothing if already in the intended mode?
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -697,7 +697,15 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t
> val)
> msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> }
> else
> + {
> msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
> + if ( v == curr )
> + /*
> + * Propagate the value to hardware, as it won't be context
> + * switched on vmentry.
> + */
I have to admit that I find "on vmentry" in the comment misleading: Reading
it I first thought you're still alluding to the old model. Plus I also find
the combination of "context switched" and "on vmentry" problematic, as we
generally mean something else when we say "context switch".
> + goto set_reg;
It's not clear why you want to use hvm_set_reg() in the first place - the
comment says "propagate to hardware", which would mean wrmsrl() in the
usual case. Here it would mean a direct call to amd_set_ssbd() imo. That
would then also be in line with all other "v == curr" conditionals, none
of which apply to any "goto set_reg". ..._set_reg(), aiui, is meant only
for use in cases where vCPU state needs updating such that proper state
would be loaded later (e.g. during VM entry).
Jan
