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

[Pu Wen <puwen@xxxxxxxx>]

On 2019/3/26 23:49, Jan Beulich wrote:
> On 25.03.19 at 14:29, <puwen@xxxxxxxx> wrote:
>> -static unsigned int __initdata opt_cpuid_mask_l7s0_eax = ~0u;
>> -integer_param("cpuid_mask_l7s0_eax", opt_cpuid_mask_l7s0_eax);
>> -static unsigned int __initdata opt_cpuid_mask_l7s0_ebx = ~0u;
>> -integer_param("cpuid_mask_l7s0_ebx", opt_cpuid_mask_l7s0_ebx);
> 
> This is no longer needed - all references are now local to amd.c.

Okay, will put them back to amd.c.

>> @@ -116,6 +121,9 @@ bool __init probe_cpuid_faulting(void)
>>      uint64_t val;
>>      int rc;
>>   
>> +    if(boot_cpu_data.x86_vendor == X86_VENDOR_HYGON)
>> +            return false;
>> +
>>      if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0)
> 
> Isn't this similarly true for AMD, in which case adding both at the

There is no MSR_INTEL_PLATFORM_INFO for AMD family 17h and Hygon family
18h. Reading this MSR will stall on Hygon system. I don't know if it
would successfully returned when reading it on AMD system.

> same time in a separate patch would seem better? Yet then again -

In a separate patch is fine.

> did you look at the description of the commit moving the function
> here (6e2fdc0f89 ["x86: Common cpuid faulting support"])? Hence
> if anything this would need qualifying by !cpu_has_hypervisor.

Then it would be like this:
        if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON &&
            !cpu_has_hypervisor)
                return false;

> Also the contextual if() tells you that there's a blank missing in the
> one you add. However, there's a wider style question to raise:
> This file is not a Linux clone, so generally I'd expect it to be
> written in plain Xen style.

I'm sorry for the missing blank. Okay, will use the right style.

>> +static void early_init_hygon(struct cpuinfo_x86 *c)
>> +{
>> +    early_init_amd(c);
>> +}
> 
> Why the wrapper function? It can be introduced once you actually

To keep the functions uniform Hygon ones in hygon_cpu_dev.

> need to do more than just the call.

Okay, will remove the wrapper and directly call early_init_amd().

>> +static void init_hygon(struct cpuinfo_x86 *c)
>> +{
>> +    u32 l, h;
> 
> As mentioned before, please prefer uint32_t et al over u32 and
> friends. While that's applicable to the entire series (and then
> also to use basic types in preference to the fixed width ones,

Okay.

> where possible), in this particular case it would be even better if
> you dropped the variables altogether, using ...
>> +    unsigned long long value;
...
>> +    if (cpu_has(c, X86_FEATURE_EFRO)) {
>> +            rdmsr(MSR_K7_HWCR, l, h);
>> +            l |= (1 << 27); /* Enable read-only APERF/MPERF bit */
>> +            wrmsr(MSR_K7_HWCR, l, h);
>> +    }
> 
> ... "value" and rdmsrl() / wrmsrl() here instead.

Will use rdmsrl()/wrmsrl() instead.

-- 
Regards,
Pu Wen

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