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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2022 15:18, Roger Pau Monne wrote:
> +void amd_init_ssbd(const struct cpuinfo_x86 *c)
> +{
> +     if (cpu_has_ssb_no)
> +             return;
> +
> +     if (cpu_has_amd_ssbd) {
> +             /* Handled by common MSR_SPEC_CTRL logic */
> +             return;
> +     }
> +
> +     if (cpu_has_virt_ssbd) {
> +             wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
> +             return;
> +     }
> +
> +     if (!set_legacy_ssbd(c, opt_ssbd))
> +     {

Nit: In this file the brace belongs on the earlier line and ...

>               printk_once(XENLOG_ERR "No SSBD controls available\n");
> +             if (amd_legacy_ssbd)
> +                     panic("CPU feature mismatch: no legacy SSBD\n");
> +     }
> +     else if ( c == &boot_cpu_data )

... you want to omit the blanks immediately inside the parentheses here.

> +             amd_legacy_ssbd = true;
> +}
> +
> +static struct ssbd_core {
> +    spinlock_t lock;
> +    unsigned int count;
> +} *ssbd_core;
> +static unsigned int __ro_after_init ssbd_max_cores;
> +
> +bool __init amd_setup_legacy_ssbd(void)
> +{
> +     unsigned int i;
> +
> +     if (boot_cpu_data.x86 != 0x17 || 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;
> +
> +     /* Max is two sockets for Fam17h hardware. */
> +     ssbd_core = xzalloc_array(struct ssbd_core, ssbd_max_cores * 2);

If I'm not mistaken this literal 2, ...

> +     if (!ssbd_core)
> +             return false;
> +
> +     for (i = 0; i < ssbd_max_cores * 2; i++) {

... this one, and ...

> +             spin_lock_init(&ssbd_core[i].lock);
> +             /* Record initial state, also applies to any hotplug CPU. */
> +             if (opt_ssbd)
> +                     ssbd_core[i].count = boot_cpu_data.x86_num_siblings;
> +     }
> +
> +     return true;
> +}
> +
> +void amd_set_legacy_ssbd(bool enable)
> +{
> +     const struct cpuinfo_x86 *c = &current_cpu_data;
> +     struct ssbd_core *core;
> +     unsigned long flags;
> +
> +     if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> +             BUG_ON(!set_legacy_ssbd(c, enable));
> +             return;
> +     }
> +
> +     BUG_ON(c->phys_proc_id >= 2);

.. this one are all referring to the same thing. Please use a #define to
make the connection obvious.

> @@ -677,14 +680,17 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>          if ( !cp->extd.virt_ssbd )
>              goto gp_fault;
>  
> -        /*
> -         * Only supports SSBD bit, the rest are ignored. Only modify the SSBD
> -         * bit in case other bits are set.
> -         */
> -        if ( val & SPEC_CTRL_SSBD )
> -            msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> +        /* Only supports SSBD bit, the rest are ignored. */
> +        if ( cpu_has_amd_ssbd )
> +        {
> +            /* Only modify the SSBD bit in case other bits are set. */

While more a comment on the earlier patch introducing this wording, it
occurred to me only here that this is ambiguous: It can also be read as
"Only modify the SSBD bit as long as other bits are set."

> +            if ( val & SPEC_CTRL_SSBD )
> +                msrs->spec_ctrl.raw |= SPEC_CTRL_SSBD;
> +            else
> +                msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> +        }
>          else
> -            msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
> +            msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;

I also think the comment applies equally to the "else" logic, so perhaps
the comment would best remain as is (and merely be re-worded in the
earlier patch)?

Jan