diff mbox series

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

Message ID 83274416-2812-53c9-f8cb-23ebdf73782e@suse.com (mailing list archive)
State Superseded
Headers show
Series x86: compat header generation and checking adjustments | expand

Commit Message

Jan Beulich July 1, 2020, 10:27 a.m. UTC
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>
---
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.

Comments

Roger Pau Monné July 14, 2020, 2:29 p.m. UTC | #1
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>

> ---
> 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.
> 
> --- 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?

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

>  
>              XLAT_vnuma_topology_info(nat.vnuma, &cmp.vnuma);
> --- 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.

Thanks.
Jan Beulich July 15, 2020, 6:36 a.m. UTC | #2
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
Roger Pau Monné July 15, 2020, 8:34 a.m. UTC | #3
On Wed, Jul 15, 2020 at 08:36:10AM +0200, Jan Beulich wrote:
> 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.

As you say on the comment above, ignoring those for translation
macros would be better, and would avoid the ugliness of having to add
the _pad cases here.

> > 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?

No, vdistance, vcpu_to_vnode and vmemrange are set by the caller, so
the enums will never have the _pad value, and hence the assignation
will be done only once, you are right.

Thanks, Roger.
Jan Beulich July 15, 2020, 8:47 a.m. UTC | #4
On 15.07.2020 10:34, Roger Pau Monné wrote:
> On Wed, Jul 15, 2020 at 08:36:10AM +0200, Jan Beulich wrote:
>> On 14.07.2020 16:29, Roger Pau Monné wrote:
>>> On Wed, Jul 01, 2020 at 12:27:37PM +0200, Jan Beulich wrote:
>>>> --- 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.
> 
> As you say on the comment above, ignoring those for translation
> macros would be better, and would avoid the ugliness of having to add
> the _pad cases here.

Ah, yes, if the supposed adjustment would also suppress the generation
of respective enumerators.

Jan
diff mbox series

Patch

--- 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)
 
             XLAT_vnuma_topology_info(nat.vnuma, &cmp.vnuma);
--- 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=