Re: [PATCH v4 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2022 12:47, Roger Pau Monne wrote:> Changes since v3:>  - Align ssbd 
per-core struct to a cache line.>  - Open code a simple spinlock to avoid 
playing tricks with the lock>    detector.>  - s/ssbd_core/ssbd_ls_cfg/.>  - 
Fix log message wording.>  - Fix define name and remove comment.>  - Also 
handle Hygon processors (Fam18h).>  - Add changelog entry.
What is this last line about?

> +bool __init amd_setup_legacy_ssbd(void)
> +{
> +     unsigned int i;
> +
> +     if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
> +         boot_cpu_data.x86_num_siblings <= 1)
> +             return true;
> +
> +     /*
> +      * One could be forgiven for thinking that c->x86_max_cores is the
> +      * correct value to use here.
> +      *
> +      * However, that value is derived from the current configuration, and
> +      * c->cpu_core_id is sparse on all but the top end CPUs.  Derive
> +      * max_cpus from ApicIdCoreIdSize which will cover any sparseness.
> +      */
> +     if (boot_cpu_data.extended_cpuid_level >= 0x80000008) {
> +             ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 0xf000);
> +             ssbd_max_cores /= boot_cpu_data.x86_num_siblings;
> +     }
> +     if (!ssbd_max_cores)
> +             return false;
> +
> +     ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
> +                                 ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
> +     if (!ssbd_ls_cfg)
> +             return false;
> +
> +     for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
> +             /* Record initial state, also applies to any hotplug CPU. */
> +             if (opt_ssbd)
> +                     ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;

Perhaps flip if() and for()?

> +void amd_set_legacy_ssbd(bool enable)
> +{
> +     const struct cpuinfo_x86 *c = &current_cpu_data;
> +     struct ssbd_ls_cfg *status;
> +
> +     if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> +             BUG_ON(!set_legacy_ssbd(c, enable));
> +             return;
> +     }
> +
> +     BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
> +     BUG_ON(c->cpu_core_id >= ssbd_max_cores);
> +     status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> +                           c->cpu_core_id];
> +
> +     /*
> +      * Open code a very simple spinlock: this function is used with GIF==0
> +      * and different IF values, so would trigger the checklock detector.
> +      * Instead of trying to workaround the detector, use a very simple lock
> +      * implementation: it's better to reduce the amount of code executed
> +      * with GIF==0.
> +      */
> +     while ( test_and_set_bool(status->locked) )
> +         cpu_relax();
> +     status->count += enable ? 1 : -1;
> +     ASSERT(status->count <= c->x86_num_siblings);
> +     if (enable ? status->count == 1 : !status->count)
> +             BUG_ON(!set_legacy_ssbd(c, enable));

What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
region?

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -544,6 +544,16 @@ static void __init calculate_hvm_max_policy(void)
>      if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
>          /* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
>          __clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> +    else
> +        /*
> +         * Expose VIRT_SSBD if VIRT_SPEC_CTRL is supported, as that implies 
> the
> +         * underlying hardware is capable of setting SSBD using
> +         * non-architectural way or VIRT_SSBD is available.
> +         *
> +         * Note that if the hardware supports VIRT_SSBD natively this setting
> +         * will just override an already set bit.
> +         */
> +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);

With the 's' annotation gone from the public header, is this last
sentence of the comment actually true? Aiui code near the top of
the function would have zapped the bit from hvm_featureset[].

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -3126,6 +3126,8 @@ void vmexit_virt_spec_ctrl(void)
>  
>      if ( cpu_has_virt_ssbd )
>          wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
> +    else
> +         amd_set_legacy_ssbd(opt_ssbd);

Nit: Indentation is off by one here. Of course this alone could
easily be adjusted while committing.

> @@ -3138,6 +3140,9 @@ void vmentry_virt_spec_ctrl(void)
>  
>      if ( cpu_has_virt_ssbd )
>          wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
> +    else
> +        amd_set_legacy_ssbd(current->arch.msrs->virt_spec_ctrl.raw &
> +                            SPEC_CTRL_SSBD);

Would seem cheaper to use !val here (and then val for symmetry in
the other function).

Jan