Re: [PATCH 6/7] x86: add iommu_op to query reserved ranges

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 19 March 2018 14:10
> 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@lists.xenproject.org; Konrad Rzeszutek
> Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH 6/7] x86: add iommu_op to query reserved ranges
> 
> >>> On 12.02.18 at 11:47, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/iommu_op.c
> > +++ b/xen/arch/x86/iommu_op.c
> > @@ -22,6 +22,58 @@
> >  #include <xen/event.h>
> >  #include <xen/guest_access.h>
> >  #include <xen/hypercall.h>
> > +#include <xen/iommu.h>
> > +
> > +struct get_rdm_ctxt {
> > +    unsigned int max_entries;
> > +    unsigned int nr_entries;
> > +    XEN_GUEST_HANDLE(xen_iommu_reserved_region_t) regions;
> > +};
> > +
> > +static int get_rdm(xen_pfn_t start, xen_ulong_t nr, u32 id, void *arg)
> 
> uint32_t please in new code.

Ok.

> 
> > +static int iommuop_query_reserved(struct
> xen_iommu_op_query_reserved *op)
> > +{
> > +    struct get_rdm_ctxt ctxt = {
> > +        .max_entries = op->nr_entries,
> > +        .regions = op->regions,
> > +    };
> > +    int rc;
> > +
> > +    if (op->pad != 0)
> 
> Missing blanks. Perhaps also drop the " != 0".
> 

Indeed.

> > +        return -EINVAL;
> > +
> > +    rc = iommu_get_reserved_device_memory(get_rdm, &ctxt);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    /* Pass back the actual number of reserved regions */
> > +    op->nr_entries = ctxt.nr_entries;
> > +
> > +    if ( ctxt.nr_entries > ctxt.max_entries )
> > +        return -ENOBUFS;
> 
> Perhaps unless the handle is null?
> 

Hmm. I'll re-work my Linux code and try that.

> > @@ -132,12 +190,75 @@ int
> compat_iommu_op(XEN_GUEST_HANDLE_PARAM(compat_iommu_op_t)
> uops,
> >              break;
> >          }
> >
> > +        /*
> > +         * The xlat magic doesn't quite know how to handle the union so
> > +         * we need to fix things up here.
> > +         */
> 
> That's quite sad, as this is the second instance in a relatively short
> period of time. We really should see whether the translation code
> can't be adjusted suitably.
> 
> > +#define XLAT_iommu_op_u_query_reserved
> XEN_IOMMUOP_query_reserved
> > +        u = cmp.op;
> > +
> > +#define XLAT_iommu_op_query_reserved_HNDL_regions(_d_, _s_) \
> > +        do \
> > +        { \
> > +            if ( !compat_handle_is_null((_s_)->regions) ) \
> 
> In the context of the earlier missing null handle check I find this
> a little surprising (but correct).
> 
> > +            { \
> > +                unsigned int *nr_entries = COMPAT_ARG_XLAT_VIRT_BASE; \
> > +                xen_iommu_reserved_region_t *regions = \
> > +                    (void *)(nr_entries + 1); \
> > +                \
> > +                if ( sizeof(*nr_entries) + \
> > +                     (sizeof(*regions) * (_s_)->nr_entries) > \
> > +                     COMPAT_ARG_XLAT_SIZE ) \
> > +                    return -E2BIG; \
> > +                \
> > +                *nr_entries = (_s_)->nr_entries; \
> > +                set_xen_guest_handle((_d_)->regions, regions); \
> 
> I don't understand why nr_entries has to be a pointer into the
> translation area. Can't this be a simple local variable?
> 

Probably. On the face of it it looks a stack variable should be fine. I'll check.

> > +            } \
> > +            else \
> > +                set_xen_guest_handle((_d_)->regions, NULL); \
> > +        } while (false)
> > +
> >          XLAT_iommu_op(&nat, &cmp);
> >
> > +#undef XLAT_iommu_op_query_reserved_HNDL_regions
> > +
> >          iommu_op(&nat);
> >
> > +        status = nat.status;
> > +
> > +#define XLAT_iommu_op_query_reserved_HNDL_regions(_d_, _s_) \
> > +        do \
> > +        { \
> > +            if ( !compat_handle_is_null((_d_)->regions) ) \
> > +            { \
> > +                unsigned int *nr_entries = COMPAT_ARG_XLAT_VIRT_BASE; \
> > +                xen_iommu_reserved_region_t *regions = \
> > +                    (void *)(nr_entries + 1); \
> > +                unsigned int j; \
> 
> Without any i in an outer scope, using j is a little unusual (but of
> course okay).
> 

Oh, that may have been a hangover from a previous incarnation of the code. I'll change it if there's no clash.

> > +                \
> > +                for ( j = 0; \
> > +                      j < min_t(unsigned int, (_d_)->nr_entries, \
> > +                                *nr_entries); \
> 
> Do you really need min_t() here (rather than the more safe min())?
> 

I've been asked to preferentially use min_t() before (although I don't think it was by you) so I'm not sure what the expectation is. I'm happy to use min().

> > +                      j++ ) \
> > +                { \
> > +                    compat_iommu_reserved_region_t region; \
> > +                    \
> > +                    XLAT_iommu_reserved_region(&region, &regions[j]); \
> > +                    \
> > +                    if ( __copy_to_compat_offset((_d_)->regions, j, \
> > +                                                 &region, 1) ) \
> 
> If you use the __-prefixed variant here, where's the address
> validity check?
> 

I thought it was validated on the way in but maybe I missed that.

> > --- a/xen/include/public/iommu_op.h
> > +++ b/xen/include/public/iommu_op.h
> > @@ -25,11 +25,46 @@
> >
> >  #include "xen.h"
> >
> > +typedef unsigned long xen_bfn_t;
> 
> Is this suitable for e.g. ARM, who don't use unsigned long for e.g.
> xen_pfn_t? Is there in fact any reason not to re-use the generic
> xen_pfn_t here (also see your get_rdm() above)? Otoh this is an
> opportunity to not widen the problem of limited addressability in
> 32-bit guests - the type could be 64-bit wide across the board.
> 

A fixed 64-bit type should mean I can lose the compat code so I'd be happy with that.

> > +struct xen_iommu_reserved_region {
> > +    xen_bfn_t start_bfn;
> > +    unsigned int nr_frames;
> > +    unsigned int pad;
> 
> Fixed width types (i.e. uint32_t) in the public interface please.
> Also, this not being the main MMU, page granularity needs to be
> specified somehow (also for the conversion between xen_bfn_t
> and a bus address).
> 

Do you think it would be better to have a separate query call to get the IOMMU page size back, or are you anticipating heterogeneous ranges (in which case I'm going to need to adjust the map and unmap functions to allow for that)?

  Paul

> > +struct xen_iommu_op_query_reserved {
> > +    /*
> > +     * IN/OUT - On entries this is the number of entries available
> > +     *          in the regions array below.
> > +     *          On exit this is the actual number of reserved regions.
> > +     */
> > +    unsigned int nr_entries;
> > +    unsigned int pad;
> 
> Same here.
> 
> Jan

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