Re: [PATCH v2 5/7] x86: generalize padding field handling
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
Re: [PATCH v2 5/7] x86: generalize padding field handling
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
> >
> > Reviewed-by: Roger Pau Monné
>
> 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.
Re: [PATCH v2 5/7] x86: generalize padding field handling
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
>
> Reviewed-by: Roger Pau Monné
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
>>
Re: [PATCH v2 5/7] x86: generalize padding field handling
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 Reviewed-by: Roger Pau Monné > --- > 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, ); > --- 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.
[PATCH v2 5/7] x86: generalize padding field handling
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 --- 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) XLAT_vnuma_topology_info(nat.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=
