Re: [Xen-devel] [PATCH 4/5] x86/cpu: Create Hygon Dhyana architecture support file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.04.19 at 22:26, <andrew.cooper3@xxxxxxxxxx> wrote:
> +static void init_hygon(struct cpuinfo_x86 *c)
> +{
> +     unsigned long long value;
> +
> +     /*
> +      * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
> +      * certainly isn't virtualised (and Xen at least will leak the real
> +      * value in but silently discard writes), as well as being per-core
> +      * rather than per-thread, so do a full safe read/write/readback cycle
> +      * in the worst case.
> +      */
> +     if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
> +             /* Unable to read.  Assume the safer default. */
> +             __clear_bit(X86_FEATURE_LFENCE_DISPATCH,
> +                         c->x86_capability);
> +     else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
> +             /* Already dispatch serialising. */
> +             __set_bit(X86_FEATURE_LFENCE_DISPATCH,
> +                       c->x86_capability);
> +     else if (wrmsr_safe(MSR_AMD64_DE_CFG,
> +                         value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
> +              rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
> +              !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
> +             /* Attempt to set failed.  Assume the safer default. */
> +             __clear_bit(X86_FEATURE_LFENCE_DISPATCH,
> +                         c->x86_capability);
> +     else
> +             /* Successfully enabled! */
> +             __set_bit(X86_FEATURE_LFENCE_DISPATCH,
> +                       c->x86_capability);

Down the road we may want to make this another helper function
shared by AMD any Hygon code.

> +     /*
> +      * If the user has explicitly chosen to disable Memory Disambiguation
> +      * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
> +      */
> +     if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> +             value |= 1ull << 10;
> +             wrmsr_safe(MSR_AMD64_LS_CFG, value);
> +     }

Like the above, this lacks a model check. Is it really expected for
all future Hygon models to supports both in exactly the same
fashion? Even if there's just a small chance of this not being the
case, rather than fiddling with MSRs which have an entirely
different meaning in future models, I'd prefer if model checks
were added in cases like these.

I do realize that in the former case there's effectively an "all
models except a few" logic already in the original AMD code,
which I would too question whether it's really desirable. After
all we've learned recently that MSRs indeed can go away
(albeit in that case it was a change to the MSR simply becoming
meaningless, rather than obtaining a new meaning). Based on
this I could accept just the SSBD logic to gain a model check.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel