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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.10.17 at 22:42, <petre.pircalabu@xxxxxxxxx> wrote:
> @@ -4586,6 +4620,107 @@ static int do_altp2m_op(
>      return rc;
>  }
>  
> +DEFINE_XEN_GUEST_HANDLE(compat_hvm_altp2m_op_t);
> +
> +#ifndef CHECK_hvm_altp2m_op
> +#define CHECK_hvm_altp2m_op \
> +    CHECK_SIZE_(struct, hvm_altp2m_op); \
> +    CHECK_FIELD_(struct, hvm_altp2m_op, version); \
> +    CHECK_FIELD_(struct, hvm_altp2m_op, cmd); \
> +    CHECK_FIELD_(struct, hvm_altp2m_op, domain); \
> +    CHECK_FIELD_(struct, hvm_altp2m_op, pad1); \
> +    CHECK_FIELD_(struct, hvm_altp2m_op, pad2);
> +#endif /* CHECK_hvm_altp2m_op */
> +
> +#ifndef CHECK_hvm_altp2m_set_mem_access_multi
> +#define CHECK_hvm_altp2m_set_mem_access_multi \
> +    CHECK_SIZE_(struct, hvm_altp2m_set_mem_access_multi); \
> +    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, view); \
> +    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, pad); \
> +    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, nr); \
> +    CHECK_FIELD_(struct, hvm_altp2m_set_mem_access_multi, opaque);
> +#endif /* CHECK_hvm_altp2m_set_mem_access_multi */

Please omit the trailing semicolons in both cases, just like the
generated macros don't have them. They're ...

> +CHECK_hvm_altp2m_op;
> +CHECK_hvm_altp2m_set_mem_access_multi;

... redundant with the ones here.

> +static int compat_altp2m_op(
> +    XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    int rc = 0;
> +    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:
> +        BUILD_BUG_ON(sizeof(struct xen_hvm_altp2m_set_mem_access_multi) <
> +                     sizeof(struct compat_hvm_altp2m_set_mem_access_multi));

With the checking macros above I would hope this isn't needed
anymore.

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

I think a variant of this comment would now better be placed ahead
of the CHECK_hvm_altp2m_* macro definitions above, with the one
here becoming brief by simply referring to the larger one.

> +    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;
> +
> +    rc = do_altp2m_op(nat.hnd);
> +
> +    switch ( a.cmd )
> +    {
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        /*
> +         * The return code can be positive only if it is the return value
> +         * of hypercall_create_continuation. In this case, the opaque value
> +         * must be copied back to the guest.
> +         */
> +        if ( rc > 0 )
> +        {

ASSERT(rc == ...);

> +            a.u.set_mem_access_multi.opaque =
> +                nat.altp2m_op->u.set_mem_access_multi.opaque;
> +            if ( __copy_field_to_guest(guest_handle_cast(arg,
> +                                                         
> compat_hvm_altp2m_op_t),
> +                                       &a, u.set_mem_access_multi.opaque) )
> +                rc = -EFAULT;
> +        }
> +        break;

default:
    ASSERT_UNREACHABLE();

Jan

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