| Message ID | 969e851e-5674-9aa5-b00f-4fe4d4cf8e5e@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixed width type adjustments | expand |
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@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.cooper3@citrix.com>. ~Andrew
--- 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; int cpuid_level; /* Maximum supported CPUID level, -1=no CPUID */ - __u32 extended_cpuid_level; /* Maximum supported CPUID extended level */ + uint32_t extended_cpuid_level; /* Maximum supported CPUID extended level */ unsigned int x86_capability[NCAPINTS]; char x86_vendor_id[16]; char x86_model_id[64]; int x86_cache_size; /* in KB - valid for CPUS which support this call */ int x86_cache_alignment; /* In bytes */ - __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 */ unsigned short x86_clflush_size; } __cacheline_aligned; --- a/xen/arch/x86/oprofile/nmi_int.c +++ b/xen/arch/x86/oprofile/nmi_int.c @@ -311,11 +311,11 @@ void nmi_stop(void) static int __init p4_init(char ** cpu_type) { - __u8 cpu_model = current_cpu_data.x86_model; + unsigned int cpu_model = current_cpu_data.x86_model; if ((cpu_model > 6) || (cpu_model == 5)) { printk("xenoprof: Initialization failed. " - "Intel processor model %d for pentium 4 family is not " + "Intel processor model %u for pentium 4 family is not " "supported\n", cpu_model); return 0; } @@ -355,12 +355,10 @@ custom_param("cpu_type", force_cpu_type) static int __init ppro_init(char ** cpu_type) { - __u8 cpu_model = current_cpu_data.x86_model; - if (force_arch_perfmon && cpu_has_arch_perfmon) return 0; - switch (cpu_model) { + switch (current_cpu_data.x86_model) { case 14: *cpu_type = "i386/core"; break; @@ -390,9 +388,8 @@ static int __init arch_perfmon_init(char static int __init cf_check nmi_init(void) { - __u8 vendor = current_cpu_data.x86_vendor; - __u8 family = current_cpu_data.x86; - __u8 _model = current_cpu_data.x86_model; + unsigned int vendor = current_cpu_data.x86_vendor; + unsigned int family = current_cpu_data.x86; if (!cpu_has_apic) { printk("xenoprof: Initialization failed. No APIC\n"); @@ -406,7 +403,7 @@ static int __init cf_check nmi_init(void switch (family) { default: printk("xenoprof: Initialization failed. " - "AMD processor family %d is not " + "AMD processor family %u is not " "supported\n", family); return -ENODEV; case 0xf: @@ -458,15 +455,15 @@ static int __init cf_check nmi_init(void } if (!cpu_type && !arch_perfmon_init(&cpu_type)) { printk("xenoprof: Initialization failed. " - "Intel processor family %d model %d " - "is not supported\n", family, _model); + "Intel processor family %u model %d is not supported\n", + family, current_cpu_data.x86_model); return -ENODEV; } break; default: printk("xenoprof: Initialization failed. " - "Unsupported processor. Unknown vendor %d\n", + "Unsupported processor. Unknown vendor %u\n", vendor); return -ENODEV; }
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@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 ...