Re: [PATCH 5/6] x86: use standard C types in struct cpuinfo_x86

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/02/2023 10:42 am, Jan Beulich wrote:
> Consolidate this to use exclusively standard types, and change oprofile
> code (apparently trying to mirror those types) at the same time. Where
> sensible actually drop local variables.
>
> No functional change intended.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Much like x86_capability[] already doesn't use a fixed width type, most
> if not all of the other fields touched here probably also shouldn't. I
> wasn't sure though whether switching might be controversial for some of
> them ...

x86_capability isn't an inherently 32bit structure.  It's a bitmap, and
all of Xen's bitmap operations take unsigned int *.

Most other things in this structure don't need to have specific widths
IMO, but there is huge quantity of junk here.

> --- a/xen/arch/x86/include/asm/processor.h
> +++ b/xen/arch/x86/include/asm/processor.h
> @@ -119,24 +119,24 @@ struct x86_cpu_id {
>  };
>  
>  struct cpuinfo_x86 {
> -    __u8 x86;            /* CPU family */
> -    __u8 x86_vendor;     /* CPU vendor */
> -    __u8 x86_model;
> -    __u8 x86_mask;
> +    uint8_t x86;            /* CPU family */
> +    uint8_t x86_vendor;     /* CPU vendor */
> +    uint8_t x86_model;
> +    uint8_t x86_mask;

These specific names always irritated me.  They should be vendor,
family, model, stepping and probably in that order.  The x86 prefix is
entirely redundant.

>      int  cpuid_level;    /* Maximum supported CPUID level, -1=no CPUID */

There's no such thing a "no CPUID" cpu for Xen any more.

> -    __u32 extended_cpuid_level; /* Maximum supported CPUID extended level */
> +    uint32_t extended_cpuid_level; /* Maximum supported CPUID extended level 
> */

This does need to be this wide, but I really regret the name being this
wide...

>      unsigned int x86_capability[NCAPINTS];
>      char x86_vendor_id[16];
>      char x86_model_id[64];

These arrays should be 12 and 48 respectively, but the vendor id is
redundant with the vendor field.

Furthermore, we do some non-trivial string rearranging of the string,
and (seeing as you rejected my patch to print it on boot) only ever gets
used to hand to dom0 in a machine check.

>      int  x86_cache_size; /* in KB - valid for CPUS which support this call  
> */
>      int  x86_cache_alignment;    /* In bytes */

The only interesting thing I can see about this field is that the
Netburst quirk is wrong.  double-pumped IO was a firmware setting
because it was a tradeoff and different workloads favoured different
settings.

> -    __u32 x86_max_cores; /* cpuid returned max cores value */
> -    __u32 booted_cores;  /* number of cores as seen by OS */
> -    __u32 x86_num_siblings; /* cpuid logical cpus per chip value */
> -    __u32 apicid;
> -    __u32 phys_proc_id;    /* package ID of each logical CPU */
> -    __u32 cpu_core_id;     /* core ID of each logical CPU*/
> -    __u32 compute_unit_id; /* AMD compute unit ID of each logical CPU */
> +    uint32_t x86_max_cores;   /* cpuid returned max cores value */
> +    uint32_t booted_cores;    /* number of cores as seen by OS */
> +    uint32_t x86_num_siblings; /* cpuid logical cpus per chip value */
> +    uint32_t apicid;
> +    uint32_t phys_proc_id;    /* package ID of each logical CPU */
> +    uint32_t cpu_core_id;     /* core ID of each logical CPU */
> +    uint32_t compute_unit_id; /* AMD compute unit ID of each logical CPU */

There's lots of redundancy here, and half of these fields can be derived
directly from the 32bit APIC ID.

For the purpose of the type cleanup, Acked-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>.

~Andrew