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

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

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?

> --- 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 assume this is not added here in order to prevent exploding due to
the xen_mc issues.

Thanks, Roger.