Re: [PATCH v5 02/24] xen/x86: library-fy cpuid-releated functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2025 05:01, Penny Zheng wrote:
> There are some cpuid library functions only referenced in
> XEN_DOMCTL-case, and shall be wrapped with CONFIG_MGMT_HYPERCALLS later,
> otherwise they will become unreachable when MGMT_HYPERCALLS=n, and hence
> violate Misra 2.1

At this point of the series there's no MGMT_HYPERCALLS yet (see also my reply
to the cover letter).

> For file cpupolicy-clr.c to contain cpupolicy clearing library function:
> - x86_cpu_policy_clear_out_of_range_leaves
>   - zero_leaves
> For file cpuid-cp2buf.c to contain cpuid copy-to-buffer library function:
> - x86_cpuid_copy_to_buffer
>   - copy_leaf_to_buffer
> For file cpuid-cpfrbuf.c to contain cpuid copy-from-buffer library function:
> - x86_cpuid_copy_from_buffer
> Sunmmerize all needed cpuid-library object file under a new variable
> CPUID_OBJS in Makefile.
> 
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v4 -> v5:
> - library-fy cpuid-releated functions
> ---
>  tools/fuzz/cpu-policy/Makefile               |   4 +-
>  tools/fuzz/x86_instruction_emulator/Makefile |  11 +-
>  tools/libs/guest/Makefile.common             |   4 +-
>  tools/tests/cpu-policy/Makefile              |   3 +-
>  tools/tests/x86_emulator/Makefile            |   3 +-
>  xen/lib/Makefile                             |   5 +
>  xen/lib/x86/cpuid-cp2buf.c                   | 123 ++++++++
>  xen/lib/x86/cpuid-cpfrbuf.c                  | 129 +++++++++
>  xen/lib/x86/cpuid.c                          | 286 -------------------
>  xen/lib/x86/cpupolicy-clr.c                  |  73 +++++
>  10 files changed, 346 insertions(+), 295 deletions(-)
>  create mode 100644 xen/lib/x86/cpuid-cp2buf.c
>  create mode 100644 xen/lib/x86/cpuid-cpfrbuf.c
>  create mode 100644 xen/lib/x86/cpupolicy-clr.c

This looks to be doing at least three things in one go. If all of them would be
simple (including them being a reasonably small diff), that may be fine. But
the diffstat above says otherwise, so I may I ask that this be split in three,
maybe even four pieces (one per function moving to a new file, and maybe one
doing prep work in the Makefile-s touched)?

The filenames also aren't very descriptive. cp-from-buffer.c, cp-to-buffer.c,
and cp-clear.c maybe? Albeit the last one is where I'm the least convinced
that splitting out and making a library function is actually a good idea. Note
how I also didn't mention that function as a possible candidate for library-
fying. I'll try to not forget to bring this up with the x86 maintainers later
in the day.

> --- a/tools/libs/guest/Makefile.common
> +++ b/tools/libs/guest/Makefile.common
> @@ -35,7 +35,9 @@ OBJS-y += $(LIBELF_OBJS)
>  ifeq ($(CONFIG_X86),y) # Add libx86 to the build
>  vpath %.c ../../../xen/lib/x86
>  
> -OBJS-y                 += cpuid.o msr.o policy.o
> +CPUID_OBJS := cpuid.o cpuid-cp2buf.o cpuid-cpfrbuf.o cpupolicy-clr.o
> +OBJS-y                 += $(CPUID_OBJS)
> +OBJS-y                 += msr.o policy.o
>  endif

Why the mismatched padding on the := line?

> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -45,3 +45,8 @@ lib-$(CONFIG_X86) += xxhash64.o
>  lib32-y := divmod.o
>  lib32-$(CONFIG_64BIT) :=
>  lib-y += $(lib32-y)
> +
> +libx86-y := x86/cpuid-cp2buf.o
> +libx86-y += x86/cpuid-cpfrbuf.o
> +libx86-y += x86/cpupolicy-clr.o
> +lib-$(CONFIG_X86) += $(libx86-y)

Why the intermediate libx86-y? And why is this not being done in 
xen/lib/x86/Makefile
anyway?

Jan