Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/04/2021 14:24, Jan Beulich wrote:
> On 14.04.2021 15:05, Andrew Cooper wrote:
>> On 14/04/2021 13:57, Jan Beulich wrote:
>>> On 14.04.2021 13:04, Roger Pau Monne wrote:
>>>> @@ -264,6 +265,38 @@ struct cpuid_policy
>>>>              };
>>>>              uint32_t nc:8, :4, apic_id_size:4, :16;
>>>>              uint32_t /* d */:32;
>>>> +
>>>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. 
>>>> */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. 
>>>> */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related 
>>>> info. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature 
>>>> information. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node 
>>>> IDs. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure 
>>>> Encryption. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
>>>> +
>>>> +            /* Leaf 0x80000021 - Extended Feature 2 */
>>>> +            union {
>>>> +                uint32_t e21a;
>>>> +                struct { DECL_BITFIELD(e21a); };
>>>> +            };
>>>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>>>>          };
>>>>      } extd;
>>> Due to the effect of this on what guests get to see, I think this
>>> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
>>> to actual leaf contents" as a prereq, which in turn may better
>>> remain on top of "x86/CPUID: adjust extended leaves out of range
>>> clearing" (both are neighbors in that over 4 months old series,
>>> fair parts of which could imo go in irrespective of the unsettled
>>> dispute on xvmalloc() - unfortunately I had made that patch 2 of
>>> the series, not expecting it to be blocked for so long, and then
>>> presumably signaling to others that the rest of the series is also
>>> blocked).
>> There is no shrinking to be done in this case.  The bit is set across
>> the board on AMD/Hygon hardware, even on older parts.
>>
>> What does need changing however is the logic to trim max_extd_leaf down
>> to what hardware supports, so the bit is visible on Rome/older
>> hardware.  I.e. after this change, all VMs should get 0x80000021 by
>> default on AMD hardware.
> As to this last sentence - not really. Only if we managed to set the
> flag (if it needs setting).

... or we find it already set.  Remember that we don't even offer an
option to let the user avoid this behaviour.

The only case where we'll boot with lfence not being dispatch
serialising is when we're virutalised under a pre-2018 hypervisor with
has no Spectre knowledge.  There are more important things in life, than
to worry about this case.

> It's a two-way thing really: If the flag ends up set, I agree there's
> no need to shrink max_extd_leaf. But if the flag ends up clear, the
> issue we have today (hence my patch) becomes worse: Trailing all zero
> leaves would result because we don't properly reduce the maximum from
> what hardware reports to what is actually populated. In any event - I
> think I'd much rather see issues with my patch pointed out, if any.

I don't have a problem with the shrinking change in principle, but it is
definitely not a prereq to this change.

Trailing zeroes aren't going to cause guests to malfunction, even if
we'd ideally be neater about the result.

~Andrew