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

[Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > 
> > > 
> > > > 
> > > > 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,
> This not being a memop, re-using this value looks arbitrary.
> Unless it absolutely has to be this value, please either add a brief
> comment saying this just happens to fit your need or use a literal
> constant.
I will add a comment to clarify the reason for using MEMOP_CMD_MASK:
e.g.
/* MEMOP_CMD_MASK is used here to mirror the way
p2m_set_mem_access_multi() is called for the
XENMEM_access_op_set_access_multi case. */
> 
> > 
> > +                                      a.u.set_mem_access_multi.vie
> > w);
> > +        if ( rc > 0 )
> > +        {
> > +            a.u.set_mem_access_multi.opaque = rc;
> > +            if ( __copy_field_to_guest(guest_handle_cast(arg,
> > xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) )
> Long line.
> 
> Also please consider adding a prereq patch changing the function's
> parameter to a properly typed handle, which will then make
> unnecessary using the not obviously correct cast here. Other
> copying back of individual fields in this function could then also be
> switched to __copy_field_to_guest().

Actually, changing the function's parameter to a typed handle could
complicate things a little for compat_altp2m_op. It should be modified
also to use a typed handle (compat_hvm_altp2m_op_t), which in case of
commands other than HVMOP_altp2m_set_mem_access_multi should be casted
to xen_hvm_altp2m_op_t before calling do_altp2m_op.
If the problem was only related to the code clarity, I think the new
implementation will be a little more difficult to follow.
> 
> > 
> > @@ -4586,6 +4613,57 @@ 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_gueWill change in the next patch iteration.
> 
st(&a, arg, 1) )
> > +        return -EFAULT;
> > +
> > +    if ( a.pad1 || a.pad2 ||
> > +         (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) )
> > +        return -EINVAL;
> > +
> > +    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);
> > +    }
> > +
> > +    /* Manually fill the common part of the xen_hvm_altp2m_op
> > structure because
> Comment style.
Will change in the next patch iteration.
> 
> > 
> > +     * the generated XLAT_hvm_altp2m_op macro doesn't correctly
> > handle the
> > +     * translation of all fields from compat_hvm_altp2m_op to
> > xen_hvm_altp2m_op.
> > +     */
> > +    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;
> I specifically asked (and even more than once, I think) that you add
> the size (and whatever else) checks we would have here if this wasn't
> open coding an XLAT_* macro.
I ran into a build error while trying to use generated CHECK_ macros. I
will try to add a manual check here.
> 
> > 
> > +    return do_altp2m_op(nat.hnd);
> You appear to miss copying back opaque here, in case it was set
> in do_altp2m_op().
> 
> > 
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -83,6 +83,13 @@
> > DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t);
> >  /* Flushes all VCPU TLBs: @arg must be NULL. */
> >  #define HVMOP_flush_tlbs          5
> >  
> > +/*
> > + * hvmmem_type_t should not be defined when generating the
> > corresponding
> > + * compat header. This will ensure that the HVMMEM_(*) values are
> > defined
> > + * only once.
> To help readers fully understand the situation, please consider
> making this "This will ensure that the improperly named
> HVMMEM_(*) values ..."; otherwise it gives the appearance of
> there being a bug in the translation scripts.
Will change in the next patch iteration.
> 
Will change in the next patch iteration.
> 
> 
> > 
> > @@ -237,6 +246,23 @@ struct xen_hvm_altp2m_set_mem_access {
> >  typedef struct xen_hvm_altp2m_set_mem_access
> > xen_hvm_altp2m_set_mem_access_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> >  
> > +struct xen_hvm_altp2m_set_mem_access_multi {
> > +    /* view */
> > +    uint16_t view;
> > +    uint16_t pad;
> > +    /* Number of pages */
> > +    uint32_t nr;
> > +    /* Used for continuation purposes */
> > +    uint64_t opaque;
> > +    /* List of pfns to set access for */
> > +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
> > +    /* Corresponding list of access settings for pfn_list */
> > +    XEN_GUEST_HANDLE(const_uint8) access_list;
> > +};
> > +typedef struct xen_hvm_altp2m_set_mem_access_multi
> > +    xen_hvm_altp2m_set_mem_access_multi_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
> Are typedef and handle actually needed anywhere? Otherwise
> please don't add them. Just like recently done for domctl and
> sysctl we should even consider cleaning up the others here.
Will change in the next patch iteration.
> 
> 
> Jan
> 
> 

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