[PATCH] x86: fix compat header generation

[PATCH] x86: fix compat header generation

[Jan Beulich]

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.

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- 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
 compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py
 	mkdir -p $(@D)
 	grep -v 'DEFINE_XEN_GUEST_HANDLE(long)' $< | \
--- a/xen/tools/compat-build-header.py
+++ b/xen/tools/compat-build-header.py
@@ -3,7 +3,7 @@
 import re,sys
 
 pats = [
- [ r"__InClUdE__(.*)", r"#include\1\n#pragma pack(4)" ],
+ [ r"__InClUdE__(.*)", r"#include\1" ],
  [ r"__IfDeF__ (XEN_HAVE.*)", r"#ifdef \1" ],
  [ r"__ElSe__", r"#else" ],
  [ r"__EnDif__", r"#endif" ],
@@ -13,7 +13,8 @@ pats = [
  [ r"(struct|union|enum)\s+(xen_?)?(\w)", r"\1 compat_\3" ],
  [ r"@KeeP@", r"" ],
  [ r"_t([^\w]|$)", r"_compat_t\1" ],
- [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
+ [ r"(8|16|32|64_aligned)_compat_t([^\w]|$)", r"\1_t\2" ],
+ [ r"(\su?int64(_compat)?)_T([^\w]|$)", r"\1_t\3" ],
  [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ],
  [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
  [ r"(^|[^\w])Xen_?", r"\1Compat_" ],
--- a/xen/tools/compat-build-source.py
+++ b/xen/tools/compat-build-source.py
@@ -9,6 +9,7 @@ pats = [
  [ r"^\s*#\s*endif /\* (XEN_HAVE.*) \*/\s+", r"__EnDif__" ],
  [ r"^\s*#\s*define\s+([A-Z_]*_GUEST_HANDLE)", r"#define HIDE_\1" ],
  [ r"^\s*#\s*define\s+([a-z_]*_guest_handle)", r"#define hide_\1" ],
+ [ r"^\s*#\s*define\s+(u?int64)_aligned_t\s.*", r"typedef \1_T __attribute__((aligned(4))) \1_compat_T;" ],
  [ r"XEN_GUEST_HANDLE(_[0-9A-Fa-f]+)?", r"COMPAT_HANDLE" ],
 ];
 

Re: [PATCH] x86: fix compat header generation

[Roger Pau Monné]

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@suse.com>
> 
> --- 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.

Re: [PATCH] x86: fix compat header generation

[Jan Beulich]

On 30.06.2020 11:52, Roger Pau Monné wrote:
> 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).

They may need in principle, but right now there's no instance of
such as per my comparing of the generated binaries.

> 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?

If there are native types that get re-used (rather than re-created
as compat version in the compat headers, which would then necessarily
derive from {u,}int64_t directly or indirectly, as there's no other
non-derived 8-byte type that's legitimate to use in public headers -
e.g. "unsigned long long" is not legitimate to be used, and all
"unsigned long" instances [if there are any left] get converted to
"unsigned int"), yes.

>> 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?

Depends on what "comparing" here means. Yes, something could
presumably be invented. But it may also be that we'd be better of
doing away with the re-use of native structs. But of course doing
so will have significant fallout, which right now I have no good
idea how to deal with.

> 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.

Yes, having this properly checked would definitely be nice. It's
just the "how" that's unclear to me here.

>> @@ -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.

Ouch - these weren't supposed to be left in. They were just for my
debugging. Thanks for noticing.

Jan