Re: [PATCH v2 5/7] x86: generalize padding field handling

[Jan Beulich <jbeulich@suse.com>]

On 14.07.2020 16:29, Roger Pau Monné wrote:
> On Wed, Jul 01, 2020 at 12:27:37PM +0200, Jan Beulich wrote:
>> The original intention was to ignore padding fields, but the pattern
>> matched only ones whose names started with an underscore. Also match
>> fields whose names are in line with the C spec by not having a leading
>> underscore. (Note that the leading ^ in the sed regexps was pointless
>> and hence get dropped.)
>>
>> This requires adjusting some vNUMA macros, to avoid triggering
>> "enumeration value ... not handled in switch" warnings, which - due to
>> -Werror - would cause the build to fail. (I have to admit that I find
>> these padding fields odd, when translation of the containing structure
>> is needed anyway.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> While for translation macros skipping padding fields pretty surely is a
>> reasonable thing to do, we may want to consider not ignoring them when
>> generating checking macros.

(note this remark, towards your question at the end)

>> --- a/xen/common/compat/memory.c
>> +++ b/xen/common/compat/memory.c
>> @@ -354,10 +354,13 @@ int compat_memory_op(unsigned int cmd, X
>>                  return -EFAULT;
>>  
>>  #define XLAT_vnuma_topology_info_HNDL_vdistance_h(_d_, _s_)		\
>> +            case XLAT_vnuma_topology_info_vdistance_pad:                \
>>              guest_from_compat_handle((_d_)->vdistance.h, (_s_)->vdistance.h)
>>  #define XLAT_vnuma_topology_info_HNDL_vcpu_to_vnode_h(_d_, _s_)		\
>> +            case XLAT_vnuma_topology_info_vcpu_to_vnode_pad:            \
>>              guest_from_compat_handle((_d_)->vcpu_to_vnode.h, (_s_)->vcpu_to_vnode.h)
>>  #define XLAT_vnuma_topology_info_HNDL_vmemrange_h(_d_, _s_)		\
>> +            case XLAT_vnuma_topology_info_vmemrange_pad:                \
>>              guest_from_compat_handle((_d_)->vmemrange.h, (_s_)->vmemrange.h)
> 
> I find this quite ugly, would it be better to just handle them with a
> default case in the XLAT_ macros?

Default cases explicitly do not get added to be able to spot missing
case labels, as most compilers will warn about such when the controlling
expression is of enum type.

> AFAICT it will also set (_d_)->vmemrange.h twice?

I'm not seeing it (and if it was, I'd then also wonder why not for the
other two handles above). This is the generated macro:

#define XLAT_vnuma_topology_info(_d_, _s_) do { \
    (_d_)->domid = (_s_)->domid; \
    (_d_)->nr_vnodes = (_s_)->nr_vnodes; \
    (_d_)->nr_vcpus = (_s_)->nr_vcpus; \
    (_d_)->nr_vmemranges = (_s_)->nr_vmemranges; \
    switch (vdistance) { \
    case XLAT_vnuma_topology_info_vdistance_h: \
        XLAT_vnuma_topology_info_HNDL_vdistance_h(_d_, _s_); \
        break; \
    } \
    switch (vcpu_to_vnode) { \
    case XLAT_vnuma_topology_info_vcpu_to_vnode_h: \
        XLAT_vnuma_topology_info_HNDL_vcpu_to_vnode_h(_d_, _s_); \
        break; \
    } \
    switch (vmemrange) { \
    case XLAT_vnuma_topology_info_vmemrange_h: \
        XLAT_vnuma_topology_info_HNDL_vmemrange_h(_d_, _s_); \
        break; \
    } \
} while (0)

Am I overlooking any further aspect?

>> --- a/xen/tools/get-fields.sh
>> +++ b/xen/tools/get-fields.sh
>> @@ -218,7 +218,7 @@ for line in sys.stdin.readlines():
>>  				fi
>>  				;;
>>  			[\,\;])
>> -				if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
>> +				if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ]
>>  				then
>>  					if [ $kind = union ]
>>  					then
>> @@ -347,7 +347,7 @@ build_body ()
>>  			fi
>>  			;;
>>  		[\,\;])
>> -			if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
>> +			if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ]
>>  			then
>>  				if [ -z "$array" -a -z "$array_type" ]
>>  				then
>> @@ -437,7 +437,7 @@ check_field ()
>>  				id=$token
>>  				;;
>>  			[\,\;])
>> -				if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
>> +				if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ]
>>  				then
>>  					check_field $1 $2 $3.$id "$fields"
>>  					test "$token" != ";" || fields= id=
>> @@ -491,7 +491,7 @@ build_check ()
>>  			test $level != 2 -o $arrlvl != 1 || id=$token
>>  			;;
>>  		[\,\;])
>> -			if [ $level = 2 -a -n "$(echo $id | $SED 's,^_pad[[:digit:]]*,,')" ]
>> +			if [ $level = 2 -a -n "$(echo $id | $SED 's,_\?pad[[:digit:]]*,,')" ]
>>  			then
>>  				check_field $kind $1 $id "$fields"
>>  				test "$token" != ";" || fields= id=
> 
> I have to admit I'm not overly happy with this level of repetition
> (not that you introduce it here), but I would prefer to have the
> regexp in a single place if possible, it's easy to miss instances
> IMO.

I too thought so while making the changes, but besides viewing this
as an orthogonal adjustment I'm also, as per the remark further up,
unconvinced the expressions actually want to be the same between
the checking macros and the xlat ones.

Jan