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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.