Re: [PATCH v2 1/7] x86: fix compat header generation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.07.2020 18:10, Roger Pau Monné wrote:
> On Wed, Jul 01, 2020 at 12:25:15PM +0200, Jan Beulich wrote:
>> As was pointed out by 0e2e54966af5 ("mm: fix public declaration of
>> struct xen_mem_acquire_resource"), we're not currently handling structs
>> correctly that have 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.
>> Additionally, for the cases where native structures get re-used,
>> enforce suitable alignment via typedef-s (which allow alignment to be
>> reduced).
>>
>> This use of typedef-s makes necessary changes to CHECK_*() macro
>> generation: Previously get-fields.sh relied on finding struct/union
>> keywords when other compound types were used. We now need to use the
>> typedef-s (guaranteeing suitable alignment) now, and hence the script
> 
> Extra now before the comma I think.
> 
>> has to recognize those cases, too. (Unfortunately there are a few
>> special cases to be dealt with, but this is really not much different
>> from e.g. the pre-existing compat_domain_handle_t special case.)
>>
>> This need to use typedef-s is certainly somewhat fragile going forward,
>> as in similar future cases it is imperative to also use typedef-s, or
>> else the CHECK_*() macros won't check what they're supposed to check. I
>> don't currently see any means to avoid this fragility, though.
>>
>> 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.
>>
>> There also results some curious inconsistency in struct xen_mc from
>> these changes - I intend to clean this up later on. Otherwise unrelated
>> code would also need adjustment right here.
> 
> Oh, so that's the reason fields in xen_mc are not all switched to use
> their typedef equivalent I guess?

Yes - see patches later in the series, which take care of the anomaly.

>> --- a/xen/tools/get-fields.sh
>> +++ b/xen/tools/get-fields.sh
>> @@ -418,6 +418,21 @@ check_field ()
>>                      "}")
>>                              level=$(expr $level - 1) id=
>>                              ;;
>> +                    compat_*_t)
>> +                            if [ $level = 2 ]
>> +                            then
>> +                                    fields=" "
>> +                                    token="${token%_t}"
>> +                                    token="${token#compat_}"
>> +                            fi
>> +                            ;;
>> +                    evtchn_*_compat_t)
>> +                            if [ $level = 2 -a $token != 
>> evtchn_port_compat_t ]
>> +                            then
>> +                                    fields=" "
>> +                                    token="${token%_compat_t}"
>> +                            fi
>> +                            ;;
> 
> Likely related to the above, but I assume we might want to add a check
> here to assert no struct fields are used?

I think we could, but have you found similar assertions
elsewhere? There being any fields would, aiui, indicate a syntax
violation (or else $level can't be 2), and I'd rather leave
catching these to the compiler.

> I assume this is not added here in order to prevent exploding due to
> the xen_mc issues.

I don't think it would, as it continues handling struct/union
just fine. (We may want to drop this support, to enforce no
use of only typedef-s, but I'm not sure _that_ wouldn't
explode.)

Jan