Re: [Xen-devel] [PATCH v4 01/15] x86/cpu: Create Hygon Dhyana architecture support file

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.03.19 at 11:42, <puwen@xxxxxxxx> wrote:
> +static void init_hygon(struct cpuinfo_x86 *c)
> +{
> +     unsigned long long value;
> +
> +     /* Attempt to set LFENCE to be Dispatch Serialising. */
> +     if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
> +             /* Unable to read.  Assume the safer default. */
> +             __clear_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability);
> +     if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
> +             /* Dispatch Serialising. */
> +             __set_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability);

This still isn't in line with the AMD code it was derived from. In
particular code and comment do not match up: You don't make any
attempt to actually _set_ the intended mode, you only record the
setting found in the feature flags.

> +     /*
> +      * 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))
> +     {

Since you've decided to inherit style from amd.c, the opening brace
belongs on the previous line (more instances further down).

> +             value |= 1ull << 10;
> +             wrmsr_safe(MSR_AMD64_LS_CFG, value);
> +     }
> +
> +     display_cacheinfo(c);

Above from here amd.c sets MFENCE_RDTSC as well. Why would
this not be needed for Hygon?

> +     if (cpu_has(c, X86_FEATURE_ITSC))
> +     {
> +             __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> +             __set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability);
> +             __set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability);
> +     }

There is a CPUID extended level check around this and ...

> +     c->x86_max_cores = (cpuid_ecx(0x80000008) & 0xff) + 1;

... also around this in the AMD original. Why did you drop this?
Please don't forget that we may run virtualized ourselves, and
that the respective leaves may have got hidden by the lower
level hypervisor.

Jan



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