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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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.

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

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

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

> +            } \
> +            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).

> +                \
> +                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())?

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

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

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

> +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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel