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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.18 at 11:34, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 07 September 2018 12:01
>> 
>> >>> 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.
> 
> That would still be phrased as a single range though, right?

Ah, yes, indeed.

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

In case of such complications I view doing so as perfectly legitimate. It's
just that in cases where copying a single field is obviously easy and
sufficient that I suggest/request that only that one field be copied.


Jan



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