Re: [PATCH v1.1 1/2] x86emul: drop wrapper C file

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2026 14:42, Teddy Astie wrote:
> Le 14/07/2026 à 16:00, Jan Beulich a écrit :
>> Move the little bit that's still left in x86/x86_emulate.c into a __XEN__
>> conditional in x86/x86_emulate/x86_emulate.c. Move what are roughly the
>> test/fuzzing harness counterparts from there into the corresponding #else.
>>
>> Interestingly more recent gcc takes issue with three static const arrays
>> which are unused when HVM=n (noticeable in particule in the shim build).
>> Apparently such are diagnosed only when present in the top-level file, but
>> not when living in an #include-d one.
>>
>> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> v1.1: Address HVM=n build issues. See
>>        
>> https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/2674985992.
>>
>> --- a/tools/tests/x86_emulator/x86-emulate.c
>> +++ b/tools/tests/x86_emulator/x86-emulate.c
>> @@ -3,27 +3,6 @@
>>   #include <errno.h>
>>   #include <sys/mman.h>
>>   
>> -/* See gcc bug 100680, but here don't bother making this version dependent. 
>> */
>> -#define gcc11_wrap(x) ({                  \
>> -    unsigned long x_;                     \
>> -    __asm__ ( "" : "=g" (x_) : "0" (x) ); \
>> -    (typeof(x))x_;                        \
>> -})
>> -
>> -#define cpu_has_amd_erratum(nr) 0
>> -#define cpu_has_mpx false
>> -#define read_bndcfgu() 0
>> -#define xstate_set_init(what)
>> -
>> -/* For generic assembly code: use macros to define operation/operand sizes. 
>> */
>> -#ifdef __i386__
>> -# define __OS          "l"  /* Operation Suffix */
>> -# define __OP          "e"  /* Operand Prefix */
>> -#else
>> -# define __OS          "q"  /* Operation Suffix */
>> -# define __OP          "r"  /* Operand Prefix */
>> -#endif
>> -
>>   uint32_t mxcsr_mask = 0x0000ffbf;
>>   struct cpu_policy cpu_policy;
>>   
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -69,7 +69,6 @@ obj-y += traps-setup.o
>>   obj-y += traps.o
>>   obj-$(CONFIG_INTEL) += tsx.o
>>   obj-$(CONFIG_VM_EVENT) += vm_event.o
>> -obj-y += x86_emulate.o
>>   obj-y += xstate.o
>>   
>>   ifneq ($(CONFIG_PV_SHIM_EXCLUSIVE),y)
>> @@ -86,13 +85,6 @@ hostprogs-y += efi/mkreloc
>>   
>>   $(obj)/efi/mkreloc: HOSTCFLAGS += -I$(srctree)/include
>>   
>> -ifneq ($(CONFIG_HVM),y)
>> -$(obj)/x86_emulate.o: CFLAGS-y += -Wno-unused-label
>> -endif
>> -ifeq ($(CONFIG_CONDITION_COVERAGE)$(CONFIG_CC_IS_GCC),yy)
>> -$(obj)/x86_emulate.o: CFLAGS-y += -Wno-error=coverage-too-many-conditions
>> -endif
>> -
>>   efi-y := $(shell if [ ! -r $(objtree)/include/xen/compile.h -o \
>>                         -O $(objtree)/include/xen/compile.h ]; then \
>>                            echo '$(TARGET).efi'; fi) \
>> --- a/xen/arch/x86/x86_emulate.c
>> +++ /dev/null
>> @@ -1,37 +0,0 @@
>> -/******************************************************************************
>> - * x86_emulate.c
>> - *
>> - * Wrapper for generic x86 instruction decoder and emulator.
>> - *
>> - * Copyright (c) 2008, Citrix Systems, Inc.
>> - *
>> - * Authors:
>> - *    Keir Fraser <keir@xxxxxxx>
>> - */
>> -
>> -#include <xen/event.h>
>> -
>> -#include <asm/x86_emulate.h>
>> -#include <asm/processor.h> /* current_cpu_info */
>> -#include <asm/xstate.h>
>> -#include <asm/amd.h> /* cpu_has_amd_erratum() */
>> -
>> -/* Avoid namespace pollution. */
>> -#undef cmpxchg
>> -#undef cpuid
>> -#undef wbinvd
>> -
>> -#define cpu_has_amd_erratum(nr) \
>> -        cpu_has_amd_erratum(&current_cpu_data, AMD_ERRATUM_##nr)
>> -
>> -#include "x86_emulate/x86_emulate.c"
>> -
>> -/*
>> - * Local variables:
>> - * mode: C
>> - * c-file-style: "BSD"
>> - * c-basic-offset: 4
>> - * tab-width: 4
>> - * indent-tabs-mode: nil
>> - * End:
>> - */
>> --- a/xen/arch/x86/x86_emulate/Makefile
>> +++ b/xen/arch/x86/x86_emulate/Makefile
>> @@ -1,3 +1,14 @@
>> +# Put this ahead of the sorted list below, as it takes long to build and 
>> hence
>> +# we'd like parallel make to schedule its building early.
>> +obj-y += x86_emulate.o
>> +
>> +ifneq ($(CONFIG_HVM),y)
>> +$(obj)/x86_emulate.o: CFLAGS-y += -Wno-unused-label
>> +endif
>> +ifeq ($(CONFIG_CONDITION_COVERAGE)$(CONFIG_CC_IS_GCC),yy)
>> +$(obj)/x86_emulate.o: CFLAGS-y += -Wno-error=coverage-too-many-conditions
>> +endif
>> +
>>   obj-y += 0f01.o
>>   obj-y += 0fae.o
>>   obj-y += 0fc7.o
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -8,8 +8,52 @@
>>    * Copyright (c) 2005-2007 XenSource Inc.
>>    */
>>   
>> +#ifdef __XEN__
>> +
>> +# include <xen/event.h>
>> +
>> +# include <asm/amd.h> /* cpu_has_amd_erratum() */
>> +# include <asm/processor.h> /* current_cpu_info */
>> +# include <asm/x86_emulate.h>
>> +# include <asm/xstate.h>
>> +
>> +/* Avoid namespace pollution. */
>> +# undef cmpxchg
>> +# undef cpuid
>> +# undef wbinvd
>> +
>> +# define cpu_has_amd_erratum(nr) \
>> +         cpu_has_amd_erratum(&current_cpu_data, AMD_ERRATUM_##nr)
>> +
>> +#else /* !__XEN__ */
>> +
>> +/* See gcc bug 100680, but here don't bother making this version dependent. 
>> */
>> +# define gcc11_wrap(x) ({                  \
>> +     unsigned long x_;                     \
>> +     __asm__ ( "" : "=g" (x_) : "0" (x) ); \
>> +     (typeof(x))x_;                        \
>> +})
>> +
>> +# define cpu_has_amd_erratum(nr) 0
>> +# define cpu_has_mpx false
>> +# define read_bndcfgu() 0
>> +# define xstate_set_init(what)
>> +
>> +/* For generic assembly code: use macros to define operation/operand sizes. 
>> */
>> +# ifdef __i386__
>> +#  define __OS          "l"  /* Operation Suffix */
>> +#  define __OP          "e"  /* Operand Prefix */
>> +# else
>> +#  define __OS          "q"  /* Operation Suffix */
>> +#  define __OP          "r"  /* Operand Prefix */
>> +# endif
>> +
>> +#endif /* __XEN__ */
>> +
>>   #include "private.h"
>>   
>> +#ifndef X86EMUL_NO_MMX
>> +
>>   /*
>>    * The next two tables are indexed by high opcode extension byte (the one
>>    * that's encoded like an immediate) nibble, with each table element then
>> @@ -45,9 +89,15 @@ static const uint16_t _3dnow_ext_table[1
>>       [0xb] = (1 << 0xb) /* pswapd */,
>>   };
>>   
>> +#endif /* !X86EMUL_NO_MMX */
>> +
>> +#ifndef X86EMUL_NO_SIMD
>> +
>>   /* Shift values between src and dst sizes of pmov{s,z}x{b,w,d}{w,d,q}. */
>>   static const uint8_t pmov_convert_delta[] = { 1, 2, 3, 1, 2, 1 };
>>   
>> +#endif /* !X86EMUL_NO_SIMD */
>> +
>>   static const uint8_t sse_prefix[] = { 0x66, 0xf3, 0xf2 };
>>   
>>   #ifdef __x86_64__
>>
> 
> Reviewed-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

Thanks. May I ask, though, that you reduce reply context in such cases? Every
reader needs to scroll through the entire original mail just to find the single
line at the bottom. See how others do this, e.g.
https://lists.xen.org/archives/html/xen-devel/2026-07/msg00139.html
https://lists.xen.org/archives/html/xen-devel/2026-06/msg01595.html
https://lists.xen.org/archives/html/xen-devel/2026-06/msg01493.html

Thanks, Jan