Re: [PATCH] x86: fix compat header generation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

I'm not familiar with compat header generation, sorry if the comments
below are obvious or plain wrong.

On Mon, Jun 29, 2020 at 05:50:59PM +0200, Jan Beulich wrote:
> As was pointed out by "mm: fix public declaration of struct
> xen_mem_acquire_resource", we're not currently handling structs
> correctly that has uint64_aligned_t fields. #pragma pack(4) suppresses
> the necessary alignment even if the type did properly survive (which
> it also didn't) in the process of generating the headers. Overall,
> with the above mentioned change applied, there's only a latent issue
> here afaict, i.e. no other of our interface structs is currently
> affected.
> 
> As a result it is clear that using #pragma pack(4) is not an option.
> Drop all uses from compat header generation. Make sure
> {,u}int64_aligned_t actually survives, such that explicitly aligned
> fields will remain aligned. Arrange for {,u}int64_t to be transformed
> into a type that's 64 bits wide and 4-byte aligned, by utilizing that
> in typedef-s the "aligned" attribute can be used to reduce alignment.
> 
> Note that this changes alignment (but not size) of certain compat
> structures, when one or more of their fields use a non-translated struct
> as their type(s). This isn't correct, and hence applying alignof() to
> such fields requires care, but the prior situation was even worse.

Just to clarify my understanding, this means that struct fields that
are also structs will need special alignment? (because we no longer have
the 4byte packaging).

I see from the generated headers that uint64_compat_t is already
aligned to 4 bytes, and I assume something similar will be needed for
all 8byte types?

> There's one change to generated code according to my observations: In
> arch_compat_vcpu_op() the runstate area "area" variable would previously
> have been put in a just 4-byte aligned stack slot (despite being 8 bytes
> in size), whereas now it gets put in an 8-byte aligned location.

Is there someway that we could spot such changes, maybe building a
version of the plain structures with -m32 and comparing against their
compat versions?

I know we have some compat checking infrastructure, so I wonder if we
could use it to avoid issues like the one we had with
xen_mem_acquire_resource, as it seems like something that could be
programmatically detected.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -34,15 +34,6 @@ headers-$(CONFIG_XSM_FLASK) += compat/xs
>  cppflags-y                := -include public/xen-compat.h 
> -DXEN_GENERATING_COMPAT_HEADERS
>  cppflags-$(CONFIG_X86)    += -m32
>  
> -# 8-byte types are 4-byte aligned on x86_32 ...
> -ifeq ($(CONFIG_CC_IS_CLANG),y)
> -prefix-$(CONFIG_X86)      := \#pragma pack(push, 4)
> -suffix-$(CONFIG_X86)      := \#pragma pack(pop)
> -else
> -prefix-$(CONFIG_X86)      := \#pragma pack(4)
> -suffix-$(CONFIG_X86)      := \#pragma pack()
> -endif
> -
>  endif
>  
>  public-$(CONFIG_X86) := $(wildcard public/arch-x86/*.h public/arch-x86/*/*.h)
> @@ -57,16 +48,16 @@ compat/%.h: compat/%.i Makefile $(BASEDI
>       echo "#define $$id" >>$@.new; \
>       echo "#include <xen/compat.h>" >>$@.new; \
>       $(if $(filter-out compat/arch-%.h,$@),echo "#include <$(patsubst 
> compat/%,public/%,$@)>" >>$@.new;) \
> -     $(if $(prefix-y),echo "$(prefix-y)" >>$@.new;) \
>       grep -v '^# [0-9]' $< | \
>       $(PYTHON) $(BASEDIR)/tools/compat-build-header.py | uniq >>$@.new; \
> -     $(if $(suffix-y),echo "$(suffix-y)" >>$@.new;) \
>       echo "#endif /* $$id */" >>$@.new
>       mv -f $@.new $@
>  
> +.PRECIOUS: compat/%.i
>  compat/%.i: compat/%.c Makefile
>       $(CPP) $(filter-out -Wa$(comma)% -include 
> %/include/xen/config.h,$(XEN_CFLAGS)) $(cppflags-y) -o $@ $<
>  
> +.PRECIOUS: compat/%.c

Not sure if it's worth mentioning that the .i and .c files are now
kept.

Roger.