On 16.07.2025 19:31, Andrew Cooper wrote:
> Architecturally, stepping is a 4-bit field, so a uint16_t suffices for a
> bitmap of steppings.
>
> In order to keep the size of struct x86_cpu_id the same, shrink the vendor and
> family fields, neither of which need to be uint16_t in Xen.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
>
> Linux supports all fields being optional. This has lead to using
> X86_MATCH_CPU(ANY, ANY, ANY, ANY, FEATURE_FOO, NULL) in place of
> boot_cpu_has(), and is not a construct I think we want to encorage.
+1
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -1003,13 +1003,15 @@ const struct x86_cpu_id *x86_match_cpu(const struct
> x86_cpu_id table[])
> const struct x86_cpu_id *m;
> const struct cpuinfo_x86 *c = &boot_cpu_data;
>
> - for (m = table; m->vendor | m->family | m->model | m->feature; m++) {
> + for (m = table; m->vendor | m->family | m->model | m->steppings |
> m->feature; m++) {
Nit: Line length. But - do we need the change at all? It looks entirely
implausible to me to use ->steppings with all of vendor, family, and
model being *_ANY (if, as per below, they would be 0 in the first place).
Tangential: The ->feature check is slightly odd here. With everything
else being a wildcard (assuming these are 0; I can't find any X86_*_ANY
in the code base; INTEL_FAM6_ANY expands to X86_MODEL_ANY, but is itself
also not used anywhere), one wouldn't be able to use FPU, as that's
feature index 0. I notice though that ...
> if (c->x86_vendor != m->vendor)
> continue;
> if (c->x86 != m->family)
> continue;
> if (c->x86_model != m->model)
> continue;
... X86_*_ANY also aren't catered for here. Hence it remains unclear
what value those constants would actually be meant to have.
Further tangential: The vendor check could in principle permit for
multiple vendors (e.g. AMD any Hygon at the same time), considering that
we use bit masks now. That would require the != there to change, though.
> --- a/xen/arch/x86/include/asm/match-cpu.h
> +++ b/xen/arch/x86/include/asm/match-cpu.h
> @@ -8,28 +8,32 @@
> #include <asm/intel-family.h>
> #include <asm/x86-vendors.h>
>
> +#define X86_STEPPINGS_ANY 0
Given the (deliberate aiui) plural, maybe better X86_STEPPINGS_ALL?
Also perhaps use 0xffff as the value, allowing to drop part of the
conditional in x86_match_cpu()?
> #define X86_FEATURE_ANY X86_FEATURE_LM
>
> struct x86_cpu_id {
> - uint16_t vendor;
> - uint16_t family;
> + uint8_t vendor;
Is shrinking this to 8 bits a good idea? We use 5 of them already. (Of
course we can re-enlarge later, if and when the need arises.)
> + uint8_t family;
The family formula allows the value to be up to 0x10e. The return type
of get_cpu_family() is therefore wrong too, strictly speaking. As is
struct cpuinfo_x86's x86 field.
> uint16_t model;
Whereas the model is strictly limited to 8 bits.
Jan
