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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2022 17:21, Roger Pau Monné wrote:
> On Thu, Apr 21, 2022 at 11:50:16AM +0200, Jan Beulich wrote:
>> On 31.03.2022 11:27, Roger Pau Monne wrote:
>>> Expose VIRT_SSBD to guests if the hardware supports setting SSBD in
>>> the LS_CFG MSR (a.k.a. non-architectural way). Different AMD CPU
>>> families use different bits in LS_CFG, so exposing VIRT_SPEC_CTRL.SSBD
>>> allows for an unified way of exposing SSBD support to guests on AMD
>>> hardware that's compatible migration wise, regardless of what
>>> underlying mechanism is used to set SSBD.
>>>
>>> Note that on AMD Family 17h (Zen 1) the value of SSBD in LS_CFG is
>>> shared between threads on the same core, so there's extra logic in
>>> order to synchronize the value and have SSBD set as long as one of the
>>> threads in the core requires it to be set. Such logic also requires
>>> extra storage for each thread state, which is allocated at
>>> initialization time.
>>
>> So where exactly is the boundary? If I'm not mistaken Zen2 is also
>> Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
>> take Zen1 == Fam17.
> 
> Right, but Zen2 already has AMD_SSBD (ie: SPEC_CTRL), so it's not
> using this logic.
> 
> The AMD whitepaper is more clear about this: any Fam17h processor that
> is using the non-architectural MSRs to set SSBD and has more than 1
> logical processor for each logical core must synchronize the setting
> of SSBD.
> 
> I think just dropping the mention of Zen 1 in the commit message
> should remove your concerns?

Or keep it, but qualify it by saying that Zen2 isn't expected to take
this path because of having SSBD. But iirc SSBD was introduced to
Zen2 only by a ucode update, so such a description should not be wrong
wrt such not-up-to-date systems.

>> Just one further nit on the code:
>>
>>> +static struct ssbd_core {
>>> +    spinlock_t lock;
>>> +    unsigned int count;
>>> +} *ssbd_core;
>>> +static unsigned int __ro_after_init ssbd_max_cores;
>>> +#define AMD_ZEN1_MAX_SOCKETS 2
>>
>> This #define looks to render ...
>>
>>> +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 * AMD_ZEN1_MAX_SOCKETS);
>>
>> ... largely redundant the comment here; if anywhere I think the comment
>> would want to go next to the #define.
> 
> I guess I should also rename the define to FAM17H instead of ZEN1, as
> all Fam17h is limited to two sockets, then the comment can be removed
> as I think the define is self-explanatory.

Yes, this rename would help both eliminate my confusion as well as the
need for the comment.

Jan