| Message ID | 0e617191-d61f-08e2-eaa9-b324cd6501f0@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: fix compat header generation | expand |
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.
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
--- 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" ], ];
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>