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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 19:30, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> @@ -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.access_list,
> +                                      a.u.set_mem_access_multi.nr,
> +                                      a.u.set_mem_access_multi.opaque,
> +                                      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.

> +                                      a.u.set_mem_access_multi.view);
> +        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().

> @@ -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_guest(&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.

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

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

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

Jan

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