Re: [Xen-devel] [PATCH v6 07/14] x86: add iommu_op to query reserved ranges

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> This patch adds an iommu_op to allow the domain IOMMU reserved ranges to be
> queried by the guest.
> 
> NOTE: The number of reserved ranges is determined by system firmware, in
>       conjunction with Xen command line options, and is expected to be
>       small. Thus, to avoid over-complicating the code, there is no
>       pre-emption check within the operation.

Hmm, RMRRs reportedly can cover a fair part of (the entire?) frame
buffer of a graphics device.

> @@ -100,16 +176,27 @@ long do_iommu_op(unsigned int nr_bufs,
>      return rc;
>  }
>  
> +CHECK_iommu_reserved_range;
> +
>  int compat_one_iommu_op(compat_iommu_op_buf_t *buf)
>  {
> -    compat_iommu_op_t cmp;
> +    compat_iommu_op_t cmp = {};
> +    size_t offset;
> +    static const size_t op_size[] = {
> +        [XEN_IOMMUOP_query_reserved] = sizeof(struct 
> compat_iommu_op_query_reserved),
> +    };
> +    size_t size;
>      xen_iommu_op_t nat;
> +    unsigned int u;
> +    int32_t status;
>      int rc;
>  
> -    if ( buf->size < sizeof(cmp) )
> +    offset = offsetof(struct compat_iommu_op, u);
> +
> +    if ( buf->size < offset )
>          return -EFAULT;

For some reason I notice this only now and here - -EFAULT isn't
really appropriately describing the error condition here. -ENODATA
or -ENOBUFS perhaps?

> @@ -119,17 +206,82 @@ int compat_one_iommu_op(compat_iommu_op_buf_t *buf)
>      if ( rc )
>          return rc;
>  
> +    if ( cmp.op >= ARRAY_SIZE(op_size) )
> +        return -EOPNOTSUPP;
> +
> +    size = op_size[array_index_nospec(cmp.op, ARRAY_SIZE(op_size))];
> +    if ( buf->size < offset + size )
> +        return -EFAULT;
> +
> +    if ( copy_from_compat_offset((void *)&cmp.u, buf->h, offset, size) )
> +        return -EFAULT;
> +
> +    /*
> +     * The xlat magic doesn't quite know how to handle the union so
> +     * we need to fix things up here.
> +     */
> +#define XLAT_iommu_op_u_query_reserved XEN_IOMMUOP_query_reserved
> +    u = cmp.op;
> +
> +#define XLAT_iommu_op_query_reserved_HNDL_ranges(_d_, _s_)            \
> +    do                                                                \
> +    {                                                                 \
> +        if ( !compat_handle_is_null((_s_)->ranges) )                  \
> +        {                                                             \
> +            unsigned int *nr_entries = COMPAT_ARG_XLAT_VIRT_BASE;     \

uint32_t (see below) or perhaps even better typeof().

> +            xen_iommu_reserved_range_t *ranges =                      \
> +                (void *)(nr_entries + 1);                             \
> +                                                                      \
> +            if ( sizeof(*nr_entries) +                                \
> +                 (sizeof(*ranges) * (_s_)->nr_entries) >              \
> +                 COMPAT_ARG_XLAT_SIZE )                               \
> +                return -E2BIG;                                        \
> +                                                                      \
> +            *nr_entries = (_s_)->nr_entries;                          \
> +            set_xen_guest_handle((_d_)->ranges, ranges);              \
> +        }                                                             \
> +        else                                                          \
> +            set_xen_guest_handle((_d_)->ranges, NULL);                \
> +    } while (false)
> +
>      XLAT_iommu_op(&nat, &cmp);
>  
> +#undef XLAT_iommu_op_query_reserved_HNDL_ranges
> +
>      iommu_op(&nat);
>  
> +    status = nat.status;
> +
> +#define XLAT_iommu_op_query_reserved_HNDL_ranges(_d_, _s_)               \
> +    do                                                                   \
> +    {                                                                    \
> +        if ( !compat_handle_is_null((_d_)->ranges) )                     \
> +        {                                                                \
> +            unsigned int *nr_entries = COMPAT_ARG_XLAT_VIRT_BASE;        \
> +            compat_iommu_reserved_range_t *ranges =                      \
> +                (void *)(nr_entries + 1);                                \
> +            unsigned int nr =                                            \
> +                min_t(unsigned int, (_d_)->nr_entries, *nr_entries);     \
> +                                                                         \
> +            if ( __copy_to_compat_offset((_d_)->ranges, 0, ranges, nr) ) \
> +                status = -EFAULT;                                        \
> +        }                                                                \
> +    } while (false)
> +
>      XLAT_iommu_op(&cmp, &nat);
>  
> +    /* status will have been modified if __copy_to_compat_offset() failed */
> +    cmp.status = status;
> +
> +#undef XLAT_iommu_op_query_reserved_HNDL_ranges
> +
>      if ( __copy_field_to_compat(compat_handle_cast(buf->h,
>                                                     compat_iommu_op_t),
>                                  &cmp, status) )
>          return -EFAULT;
>  
> +#undef XLAT_iommu_op_u_query_reserved
> +
>      return 0;
>  }

It's somehow more evident here, but I think even on the native path
the nr_entries field doesn't get copied back despite it being an OUT.

> --- a/xen/include/public/iommu_op.h
> +++ b/xen/include/public/iommu_op.h
> @@ -25,11 +25,50 @@
>  
>  #include "xen.h"
>  
> +typedef uint64_t xen_bfn_t;
> +
> +/* Structure describing a single range reserved in the IOMMU */
> +struct xen_iommu_reserved_range {
> +    xen_bfn_t start_bfn;
> +    unsigned int nr_frames;
> +    unsigned int pad;

uint32_t

> +};
> +typedef struct xen_iommu_reserved_range xen_iommu_reserved_range_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_iommu_reserved_range_t);
> +
> +/*
> + * XEN_IOMMUOP_query_reserved: Query ranges reserved in the IOMMU.
> + */

Single line comment please.

> +#define XEN_IOMMUOP_query_reserved 1
> +
> +struct xen_iommu_op_query_reserved {
> +    /*
> +     * IN/OUT - On entry this is the number of entries available
> +     *          in the ranges array below.
> +     *          On exit this is the actual number of reserved ranges.
> +     */
> +    unsigned int nr_entries;
> +    unsigned int pad;

uint32_t

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel