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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -459,8 +459,8 @@ void recalculate_cpuid_policy(struct domain *d)
>      uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
>      unsigned int i;
>  
> -    p->x86_vendor = get_cpu_vendor(p->basic.vendor_ebx, p->basic.vendor_ecx,
> -                                   p->basic.vendor_edx, gcv_guest);
> +    p->x86_vendor = x86_cpuid_lookup_vendor(
> +        p->basic.vendor_ebx, p->basic.vendor_ecx, p->basic.vendor_edx);

Personally I dislike this style of line wrapping, but I know
./CODING_STYLE doesn't say anything as to what style to use, so
your choice is going to be it here.

Jan



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