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

[Pu Wen <puwen@xxxxxxxx>]

On 2019/4/3 16:43, Jan Beulich wrote:
> 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.
The code is derived but not fully copied. I tested the conditionals and 
found that the other branches are not reached on Hygon platforms, so I 
removed them.
Our firmware will make sure that the bit AMD64_DE_CFG_LFENCE_SERIALISE 
will be set. So I just check here instead of setting. If you think 
retaining all the original conditionals is better, I'll do that. :)
> > +       /*
> > +        * 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).
I'm a little confused about which style to follow? In v3 series I 
followed the style of the derived code. But in other patch you told me 
to follow the Xen coding style, so in v4 series I changed the style to 
match the bracing section of CODING_STYLE.
Anyway I can still inherit the style from amd.c.

> > +               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?
Because Hygon has feature LFENCE_DISPATCH, so the feature MFENCE_RDTSC 
will not be set here.
But if you think the conditional should be retained here for some reason 
(even though the conditional may not be touched), I'll add it.
> > +       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?
The reason is somehow the same as the explanations above. Hygon CPU 
always has CPUID extended level, so I think there is no need to check it 
here.
Different from AMD, which has many old families without the CPUID 
extended level, Hygon CPU is derived from AMD family 17h and always has 
the extended features.
> Please don't forget that we may run virtualized ourselves, and
> that the respective leaves may have got hidden by the lower
> level hypervisor.
I think this is the most important reason. Previously I only considered 
to run Hygon Xen on bare hardware, which is the most important usage for 
a server processor. To match all the using cases I'll add the checking 
you mentioned above.
--
Regards,
Pu Wen

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