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

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

On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote:
> > @@ -4451,6 +4453,7 @@ static int do_altp2m_op(
> >      case HVMOP_altp2m_destroy_p2m:
> >      case HVMOP_altp2m_switch_p2m:
> >      case HVMOP_altp2m_set_mem_access:
> > +    case HVMOP_altp2m_set_mem_access_multi:
> Was it agreed that this, just like others (many wrongly, I think) is
> supposed to be invokable by the affected domain itself? Its non-
> altp2m counterpart is a DM_PRIV operation. If the one here is
> meant to be different, I think the commit message should say why.
Will be corrected in the new patch iteration.
> 
> > 
> > @@ -4568,6 +4571,30 @@ static int do_altp2m_op(
> >                                      a.u.set_mem_access.view);
> >          break;
> >  
> > +    case HVMOP_altp2m_set_mem_access_multi:
> > +        if ( a.u.set_mem_access_multi.pad ||
> > +             a.u.set_mem_access_multi.opaque >=
> > a.u.set_mem_access_multi.nr )
> > +        {
> > +            rc = -EINVAL;
> > +            break;
> > +        }
> > +        rc = p2m_set_mem_access_multi(d,
> > a.u.set_mem_access_multi.pfn_list,
> > +                                      a.u.set_mem_access_multi.acc
> > ess_list,
> > +                                      a.u.set_mem_access_multi.nr,
> > +                                      a.u.set_mem_access_multi.opa
> > que,
> > +                                      MEMOP_CMD_MASK,
> > +                                      a.u.set_mem_access_multi.vie
> > w);
> > +        if ( rc > 0 )
> > +        {
> > +            a.u.set_mem_access_multi.opaque = rc;
> > +            if ( __copy_to_guest(arg, &a, 1) )
> __copy_field_to_guest() would suffice here afaict.

Will be corrected in the new patch iteration.
> 
> > 
> > @@ -4586,6 +4613,53 @@ static int do_altp2m_op(
> >      return rc;
> >  }
> >  
> > +static int compat_altp2m_op(
> > +    XEN_GUEST_HANDLE_PARAM(void) arg)
> > +{
> > +    struct compat_hvm_altp2m_op a;
> > +    union
> > +    {
> > +        XEN_GUEST_HANDLE_PARAM(void) hnd;
> > +        struct xen_hvm_altp2m_op *altp2m_op;
> > +    } nat;
> > +
> > +    if ( !hvm_altp2m_supported() )
> > +        return -EOPNOTSUPP;
> > +
> > +    if ( copy_from_guest(&a, arg, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( a.pad1 || a.pad2 ||
> > +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> > +        return -EINVAL;
> Why doesn't suffice what do_altp2m_op() does?
The sanity checks are the same as for do_altp2m_op but it wanted to
check as early as possible for invalid arguments.
> 
> > 
> > +    set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE);
> > +
> > +    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.

#define XLAT_hvm_altp2m_op(_d_, _s_) do { \
    (_d_)->version = (_s_)->version; \
    (_d_)->cmd = (_s_)->cmd; \
    (_d_)->domain = (_s_)->dWill be corrected in the new patch
iteration.omain; \
    (_d_)->pad1 = (_s_)->pad1; \
    (_d_)->pad2 = (_s_)->pad2; \
    switch (u) { \
    case XLAT_hvm_altp2m_op_u_domain_state: \
        XLAT_hvm_altp2m_domain_state(&(_d_)->u.domain_state, &(_s_)-
>u.domain_state); \
        break; \
    case
XLAT_hvm_alxen_hvm_altp2m_set_mem_access_multi_ttp2m_op_u_enable_notify
: \
        XLAT_hvm_altp2m_vcpu_enable_notify(&(_d_)->u.enable_notify,
&(_s_)->u.enable_notifyxen_hvm_altp2m_set_mem_access_multi_t); \
        break; \
    case XLAT_hvm_altp2m_op_u_view: \
        XLAT_hvm_altp2m_view(&(_d_)->u.view, &(_s_)->u.view); \
        break; \Will be corrected in the new patch iteration.
    case XLAT_hvm_altp2m_op_u_set_mem_access: \
        XLAT_hvm_altp2m_set_mem_access(&(_d_)->u.set_mem_access,
&(_s_)->u.set_mem_access); \
        break; \
    case XLAT_hvm_altp2m_op_u_change_gfn: \
        XLAT_hvm_altp2m_change_gfn(&(_d_)->u.change_gfn, &(_s_)-
>u.change_gfn); \
        break; \
    case XLAT_hvm_altp2m_op_u_set_mem_access_multi: \
        XLAT_hvm_altp2m_set_mem_access_multi(&(_d_)-
>u.set_mem_access_multi, &(_s_)->u.set_mem_access_multi); \
        break; \
    case XLAT_hvm_altp2m_op_u_pad: \
        (_d_)->u.pad = (_s_)->u.pad; \
        break; \
    } \
} while (0)

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)

enum XLAT_hvm_altp2m_op_u {
    XLAT_hvm_altp2m_op_u_domain_state,
    XLAT_hvm_altp2m_op_u_enable_notify,
    XLAT_hvm_altp2m_op_u_view,
    XLAT_hvm_altp2m_op_u_set_mem_access,
    XLAT_hvm_altp2m_op_u_change_gfn,
    XLAT_hvm_altp2m_op_u_set_mem_access_multi,
    XLAT_hvm_altp2m_op_u_pad,
};

#define XLAT_hvm_altp2m_op(_d_, _s_) do { \
    (_d_)->version = (_s_)->version; \
    (_d_)->cmd = (_s_)->cmd; \
    (_d_)->domain = (_s_)->domain; \
    (_d_)->pad1 = (_s_)->pad1; \
    (_d_)->pad2 = (_s_)->pad2; \
    switch (u) { \
    case XLAT_hvm_altp2m_op_u_domain_state: \
        (_d_)->u.domain_state = (_s_)->u.domain_state; \
        break; \
    case XLAT_hvm_altp2m_op_u_enable_notify: \
        (_d_)->u.enable_notify = (_s_)->u.enable_notify; \
        break; \
    case XLAT_hvm_altp2m_op_u_view: \
        (_d_)->u.view = (_s_)->u.view; \
        break; \
    case XLAT_hvm_altp2m_op_u_set_mem_access: \
        (_d_)->u.set_mem_access = (_s_)->u.set_mem_access; \
        break; \
    case XLAT_hvm_altp2m_op_u_change_gfn: \
        (_d_)->u.change_gfn = (_s_)->u.change_gfn; \
        break; \
    case XLAT_hvm_altp2m_op_u_set_mem_access_multi: \
        (_d_)->u.set_mem_access_multi = (_s_)->u.set_mem_access_multi;
\
        break; \
    case XLAT_hvm_altp2m_op_u_pad: \
        (_d_)->u.pad = (_s_)->u.pad; \
        break; \
    } \
} while (0)

At this stage the easiest approach was to set the values by hand.
> 
> > 
> > @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >          break;
> >  
> >      case HVMOP_altp2m:
> > -        rc = do_altp2m_op(arg);
> > +        rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) :
> > do_altp2m_op(arg);
> Pointless parentheses and spaces. Plus, to be honest, I'm not really
> happy about this ad hoc compat handling, but at the same time I
> can't suggest a truly better alternative.
> 
Removed the spaces. The alternative (e.g. changing hvm_hypercall_table
to use COMPAT_CALL(hvm_op) and defining a compat_hvm_op function in ch
only altp2m is handled differently) is uglier in my opinion because
only HVMOP_altp2m_set_mem_access_multi requires COMPAT argument
translation.
 
> > 
> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -73,6 +73,7 @@
> >  ?	vcpu_hvm_context		hvm/hvm_vcpu.h
> >  ?	vcpu_hvm_x86_32			hvm/hvm_vcpu.h
> >  ?	vcpu_hvm_x86_64			hvm/hvm_vcpu.h
> > +!	hvm_altp2m_set_mem_access_multi	hvm/hvm_op.h
> Please insert alphabetically, sorted by filename (and then structure
> name).
Will be corrected 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)

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