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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 12:10, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
>> > > > On 05.10.17 at 17:42, <ppircalabu@xxxxxxxxxxxxxxx> 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_op-
>> > >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).

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

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel