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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.19 at 12:05, <puwen@xxxxxxxx> wrote:
> 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. :)

I'm not convinced you need to retain everything, but you surely
shouldn't limit code to work just on your specific variant of
firmware.

>>> +   /*
>>> +    * 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.

Well, taking just the brace placement part doesn't make this
the file Xen style. In my earlier response to that style
question I did suggest you switch to Xen style for the new
file. I'd still view this as the preferred option, but then all
aspects should be taken care of. But I won't insist, yet in that
case clean Linux style is the only other alternative.

>>> +           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.

See above - yes, I think it should be retained.

Jan



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