Re: [PATCH v2 2/9] x86emul/test: rename "cp"

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/08/2024 9:51 am, Jan Beulich wrote:
> In preparation of introducing a const struct cpu_policy * local in
> x86_emulate(), rename that global variable to something more suitable:
> "cp" is our commonly used name for function parameters or local
> variables of type struct cpu_policy *, and the present name of the
> global could hence have interfered already.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: Re-base over dropping of Xeon Phi support.

Bah - still need to reinstate part of that patch.  The model numbers
need to stay.  /me adds it to the todo list.

For this patch, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>,
with one request for this patch a couple of thoughts for separate patches.

> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -123,7 +123,7 @@ static inline uint64_t xgetbv(uint32_t x
>  }
>  
>  /* Intentionally checking OSXSAVE here. */
> -#define cpu_has_xsave     (cp.basic.raw[1].c & (1u << 27))
> +#define cpu_has_xsave     (cpu_policy.basic.raw[1].c & (1u << 27))

Right now we deliberately don't emit names for APIC, OSXSAVE and OSPKE,
because the values won't be correct in a guest's policy.  But this is a
legitimate corner case.

What about this?

diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 601eec608983..1b56958f07e7 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -382,10 +382,11 @@ def crunch_numbers(state):
             if name and name[0] in "0123456789":
                 name = "_" + name
 
-            # Don't generate names for features fast-forwarded from other
-            # state
+            # For dynamic features (fast forwarded from other state), also
+            # prefix with an underscore to highlight that extra care
needs to
+            # be taken.
             if name in ("APIC", "OSXSAVE", "OSPKE"):
-                name = ""
+                name = "_" + name
 
             names.append(name.lower())
 
which would make the expression become cpu_policy.basic._osxsave and
avoid the opencoded number.

> @@ -133,63 +133,63 @@ static inline bool xcr0_mask(uint64_t ma
>  unsigned int rdpkru(void);
>  void wrpkru(unsigned int val);
>  
> -#define cache_line_size() (cp.basic.clflush_size * 8)
> -#define cpu_has_fpu        cp.basic.fpu
> -#define cpu_has_mmx        cp.basic.mmx
> -#define cpu_has_fxsr       cp.basic.fxsr
> -#define cpu_has_sse        cp.basic.sse
> -#define cpu_has_sse2       cp.basic.sse2
> -#define cpu_has_sse3       cp.basic.sse3
> -#define cpu_has_pclmulqdq  cp.basic.pclmulqdq
> -#define cpu_has_ssse3      cp.basic.ssse3
> -#define cpu_has_fma       (cp.basic.fma && xcr0_mask(6))
> -#define cpu_has_sse4_1     cp.basic.sse4_1
> -#define cpu_has_sse4_2     cp.basic.sse4_2
> -#define cpu_has_popcnt     cp.basic.popcnt
> -#define cpu_has_aesni      cp.basic.aesni
> -#define cpu_has_avx       (cp.basic.avx  && xcr0_mask(6))
> -#define cpu_has_f16c      (cp.basic.f16c && xcr0_mask(6))
> -
> -#define cpu_has_avx2      (cp.feat.avx2 && xcr0_mask(6))
> -#define cpu_has_bmi1       cp.feat.bmi1
> -#define cpu_has_bmi2       cp.feat.bmi2
> -#define cpu_has_avx512f   (cp.feat.avx512f  && xcr0_mask(0xe6))
> -#define cpu_has_avx512dq  (cp.feat.avx512dq && xcr0_mask(0xe6))
> -#define cpu_has_avx512_ifma (cp.feat.avx512_ifma && xcr0_mask(0xe6))
> -#define cpu_has_avx512cd  (cp.feat.avx512cd && xcr0_mask(0xe6))
> -#define cpu_has_sha        cp.feat.sha
> -#define cpu_has_avx512bw  (cp.feat.avx512bw && xcr0_mask(0xe6))
> -#define cpu_has_avx512vl  (cp.feat.avx512vl && xcr0_mask(0xe6))
> -#define cpu_has_avx512_vbmi (cp.feat.avx512_vbmi && xcr0_mask(0xe6))
> -#define cpu_has_avx512_vbmi2 (cp.feat.avx512_vbmi2 && xcr0_mask(0xe6))
> -#define cpu_has_gfni       cp.feat.gfni
> -#define cpu_has_vaes      (cp.feat.vaes && xcr0_mask(6))
> -#define cpu_has_vpclmulqdq (cp.feat.vpclmulqdq && xcr0_mask(6))
> -#define cpu_has_avx512_vnni (cp.feat.avx512_vnni && xcr0_mask(0xe6))
> -#define cpu_has_avx512_bitalg (cp.feat.avx512_bitalg && xcr0_mask(0xe6))
> -#define cpu_has_avx512_vpopcntdq (cp.feat.avx512_vpopcntdq && 
> xcr0_mask(0xe6))
> -#define cpu_has_movdiri    cp.feat.movdiri
> -#define cpu_has_movdir64b  cp.feat.movdir64b
> -#define cpu_has_avx512_vp2intersect (cp.feat.avx512_vp2intersect && 
> xcr0_mask(0xe6))
> -#define cpu_has_serialize  cp.feat.serialize
> -#define cpu_has_avx512_fp16 (cp.feat.avx512_fp16 && xcr0_mask(0xe6))
> -#define cpu_has_sha512     (cp.feat.sha512 && xcr0_mask(6))
> -#define cpu_has_sm3        (cp.feat.sm3 && xcr0_mask(6))
> -#define cpu_has_sm4        (cp.feat.sm4 && xcr0_mask(6))
> -#define cpu_has_avx_vnni   (cp.feat.avx_vnni && xcr0_mask(6))
> -#define cpu_has_avx512_bf16 (cp.feat.avx512_bf16 && xcr0_mask(0xe6))
> -#define cpu_has_avx_ifma   (cp.feat.avx_ifma && xcr0_mask(6))
> -#define cpu_has_avx_vnni_int8 (cp.feat.avx_vnni_int8 && xcr0_mask(6))
> -#define cpu_has_avx_ne_convert (cp.feat.avx_ne_convert && xcr0_mask(6))
> -#define cpu_has_avx_vnni_int16 (cp.feat.avx_vnni_int16 && xcr0_mask(6))

Can we take the opportunity to realign these vertically?

Also, we have X86_XCR0_* constants in a helpful place now.  Could we do
something like:

#define XCR0_AVX xcr0_mask(X86_XCR0_YMM | X86_XCR0_SSE)
#define XCR0_AVX512 ...

So that these expressions now read

#define cpu_has_$X (... && XCR0_AVX)

rather than forcing the reader to know %xcr0 by raw hex?

~Andrew