Re: [Xen-devel] [PATCH 2/4] x86/cpuid: Drop get_cpu_vendor() completely

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/03/2019 12:08, Jan Beulich wrote:
>>>> On 21.03.19 at 13:21, <andrew.cooper3@xxxxxxxxxx> wrote:
>> get_cpu_vendor() tries to do a number of things, and ends up doing none of
>> them well.
>>
>> For calculating the vendor itself, use x86_cpuid_lookup_vendor() which is
>> implemented in a far more efficient manner than looping over cpu_devs[].
> Well, yes, the new library function is more efficient. The downside is
> that the vendor specific information no longer lives in a central place
> (struct cpu_dev).

This is intentional.  The set of CPUs which Xen is compiled to support
is logically unrelated to what libx86 needs for vendor-specific safety.

I'm expecting almost all of cpu_dev to disappear in the long term.

>
>> @@ -313,7 +283,13 @@ static void __init early_cpu_detect(void)
>>      *(u32 *)&c->x86_vendor_id[8] = ecx;
>>      *(u32 *)&c->x86_vendor_id[4] = edx;
>>  
>> -    c->x86_vendor = get_cpu_vendor(ebx, ecx, edx, gcv_host);
>> +    c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
>> +    if (c->x86_vendor < ARRAY_SIZE(cpu_devs) && cpu_devs[c->x86_vendor])
>> +            this_cpu = cpu_devs[c->x86_vendor];
>> +    else
>> +            printk(XENLOG_ERR
>> +                   "Unrecognised or unsupported CPU vendor '%s'\n",
>> +                   c->x86_vendor_id);
> %s happens to work because x86_vendor_id is an array of 16
> characters. I'd prefer if the code here wasn't dependent on
> that property plus the fact that the last 4 characters start out
> as zeros and never get written to, by using %.*s instead.

Yeah - I spotted all of that.  I chose not to fix everything wrong with
Xen's cpu handling in this patch.

I'll switch to %.12s here, but...

>
> Preferably with this taken care of
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> Then again I wonder if we wouldn't better drop x86_vendor_id[]
> altogether. It's close to unused, yet takes up NR_CPUS*16 bytes
> of storage. The few uses could be replaced by a reverse function
> to x86_cpuid_lookup_vendor(). I don't think we care much to be
> able to "correctly" reverse X86_VENDOR_UNKNOWN.

... this wants deferring to a later change.

~Andrew

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