Re: [PATCH v3] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Petre Ovidiu PIRCALABU <ppircalabu@bitdefender.com>]

On Lu, 2017-10-09 at 04:36 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote:
> > On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
> > > > +    switch ( a.cmd )
> > > > +    {
> > > > +        case HVMOP_altp2m_set_mem_access_multi:
> > > > +#define
> > > > XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_,
> > > > _s_); \
> > > > +            guest_from_compat_handle((_d_)->pfn_list, (_s_)-
> > > > > 
> > > > > pfn_list)
> > > > +#define
> > > > XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_,
> > > > _s_); \
> > > > +            guest_from_compat_handle((_d_)->access_list,
> > > > (_s_)-
> > > > > 
> > > > > access_list)
> > > > +            XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_o
> > > > p-
> > > > > 
> > > > > u.set_mem_access_multi,
> > > > +                    &a.u.set_mem_access_multi);
> > > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list
> > > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list
> > > > +            break;
> > > > +        default:
> > > > +            return do_altp2m_op(arg);
> > > > +    }
> > > > +
> > > > +    nat.altp2m_op->version  = a.version;
> > > > +    nat.altp2m_op->cmd      = a.cmd;
> > > > +    nat.altp2m_op->domain   = a.domain;
> > > > +    nat.altp2m_op->pad1     = a.pad1;
> > > > +    nat.altp2m_op->pad2     = a.pad2;
> > > Why do you do this by hand, rather than using XLAT_*() macros
> > > which at the same time check that the field sizes match?
> > Actually, there is a problem with the XLAT_hvm_altp2m_op macro
> > generation.
> > The current definition of struct xen_hvm_altp2m_op uses "structs"
> > as
> > member of the union. In this case the enum values for switch(u) are
> > not
> > generated.
> > [...]
> > If the "structs" are replaced with the corresponding typedefs in
> > the
> > definition of xen_hvm_altp2m_op
> > (e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct
> > xen_hvm_altp2m_domain_state), the enum values are generated
> > correctly
> > but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with
> > a
> > simple assignment, thus breaking the build (
> > compat_hvm_altp2m_set_mem_access_multi_t
> > to xen_hvm_altp2m_set_mem_access_multi_t assignment)
> > [...]
> > At this stage the easiest approach was to set the values by hand.
> Okay, but then please add a comment to say so (after all it should
> really be the script that gets adjusted in order to correctly deal
> with the cases) and add the missing size checks (and whatever
> else verification the macros may do).
Will fix in in the new patch iteration.
> 
> > 
> > > 
> > > > 
> > > > --- a/xen/tools/compat-build-header.py
> > > > +++ b/xen/tools/compat-build-header.py
> > > > @@ -16,6 +16,7 @@ pats = [
> > > >   [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ],
> > > >   [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)",
> > > > r"\1compat_\2_t\3"
> > > > ],
> > > >   [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ],
> > > > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ],
> > > What is this needed for? I can't find any instance of HVMMEM_*
> > > elsewhere in the patch. As you can see, so far there are only
> > > pretty generic tokens being replaced here.
> > > 
> > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums
> > will
> > define the same values (e.g.  HVMMEM_ram_rw,) resulting in a
> > compile
> > error. By adding this translation we will have a COMPAT_HVMMEM
> > value
> > for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g.
> > HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw)
> That's ugly. I realize that we shouldn't even attempt to translate
> enumerations (or to be fully precise, there shouldn't be any
> enumerations in the public interface in the first place), as the
> enumerator values ought to remain consistent between native
> and compat uses. Hence we could either convert the enum to a
> set of #define-s, or we would need a mechanism to exclude
> parts of a header from the compat conversion.
> 
> In the end the problem here is because of the enumerators,
> other than xenmem_access_t's, aren't properly prefixed with
> XEN or XEN_ (or else the script would already handle them fine
> afaict). So another variant of addressing this would be to
> deprecate (but not remove) the current names, introducing
> properly named ones for __XEN_INTERFACE_VERSION__ >=
> 0x00040a00.
> 
Unfortunately the enum is referenced also in other functions
(e.g. xendevicemodel_set_mem_type) so replacing it with #defines would
be more difficult.
When generating the compat headers,-DXEN_GENERATING_COMPAT_HEADERS is
defined (xen/include/Makefile). I can guard hvmmem_type_t with an
#ifndef XEN_GENERATING_COMPAT_HEADERS so the enum is not processed by
the compat-build-header.py script. (in my opinion this is the minimum-
impact approach)
Do you agree with this?

Many thanks,
Petre
> Jan
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel