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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 07 September 2018 12:01
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian
> Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: Re: [PATCH v6 07/14] x86: add iommu_op to query reserved ranges
> 
> >>> On 23.08.18 at 11:47, <paul.durrant@citrix.com> 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.

That would still be phrased as a single range though, right?

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

Ok. ENODATA seems best.

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

Ok.

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

Indeed. It would be so much simpler just to copy back the entire buf to avoid the need to special-case this for each op.

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

Ok.

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

Ok.

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

Ok.

  Paul

> Jan


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