Re: [PATCH] x86: fix compat header generation

[Jan Beulich <jbeulich@xxxxxxxx>]

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