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 <jbeul...@suse.com>
> ---
> 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.coop...@citrix.com>.
~Andrew
