> -----Original Message-----
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 04 September 2018 12:50
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Wei Liu
> <wei.l...@citrix.com>; George Dunlap <george.dun...@citrix.com>; Ian
> Jackson <ian.jack...@citrix.com>; Stefano Stabellini
> <sstabell...@kernel.org>; xen-devel <xen-devel@lists.xenproject.org>;
> Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Tim (Xen.org)
> <t...@xen.org>
> Subject: Re: [PATCH v6 05/14] public / x86: introduce
> __HYPERCALL_iommu_op
>
> >>> On 23.08.18 at 11:47, <paul.durr...@citrix.com> wrote:
> > --- /dev/null
> > +++ b/xen/common/iommu_op.c
> > @@ -0,0 +1,184 @@
> >
> +/*********************************************************
> *********************
> > + * x86/iommu_op.c
>
> Oops?
>
Yep. Missed that in the move to common.
> > +int do_one_iommu_op(xen_iommu_op_buf_t *buf)
> > +{
> > + xen_iommu_op_t op;
> > + int rc;
> > +
> > + if ( buf->size < sizeof(op) )
> > + return -EFAULT;
> > +
> > + if ( copy_from_guest((void *)&op, buf->h, sizeof(op)) )
>
> This cast could be avoided if you made ...
>
> > + return -EFAULT;
> > +
> > + if ( op.pad )
> > + return -EINVAL;
> > +
> > + rc = xsm_iommu_op(XSM_PRIV, current->domain, op.op);
> > + if ( rc )
> > + return rc;
> > +
> > + iommu_op(&op);
> > +
> > + if ( __copy_field_to_guest(guest_handle_cast(buf->h,
> xen_iommu_op_t),
>
> ... this cast the initializer of a local variable of suitable handle
> type (same on the compat path then).
Ok. I'll look at that.
>
> > +int compat_one_iommu_op(compat_iommu_op_buf_t *buf)
> > +{
> > + compat_iommu_op_t cmp;
> > + xen_iommu_op_t nat;
> > + int rc;
> > +
> > + if ( buf->size < sizeof(cmp) )
> > + return -EFAULT;
> > +
> > + if ( copy_from_compat((void *)&cmp, buf->h, sizeof(cmp)) )
> > + return -EFAULT;
> > +
> > + if ( cmp.pad )
> > + return -EINVAL;
> > +
> > + rc = xsm_iommu_op(XSM_PRIV, current->domain, cmp.op);
> > + if ( rc )
> > + return rc;
> > +
> > + XLAT_iommu_op(&nat, &cmp);
> > +
> > + iommu_op(&nat);
> > +
> > + XLAT_iommu_op(&cmp, &nat);
> > +
> > + if ( __copy_field_to_compat(compat_handle_cast(buf->h,
> > + compat_iommu_op_t),
> > + &cmp, status) )
>
> Since you're only after the status field, perhaps better to avoid the
> full-blown reverse XLAT_iommu_op() and copy just that one field?
>
I kind of like the fact that the two calls mirror each other so I'd prefer to
keep it.
> > --- a/xen/include/Makefile
> > +++ b/xen/include/Makefile
> > @@ -11,6 +11,7 @@ headers-y := \
> > compat/features.h \
> > compat/grant_table.h \
> > compat/kexec.h \
> > + compat/iommu_op.h \
> > compat/memory.h \
> > compat/nmi.h \
> > compat/physdev.h \
>
> I guess this is just an off-by-one wrt sorting?
>
Yep. I'll move.
> > @@ -29,6 +30,7 @@ headers-$(CONFIG_X86) += compat/arch-x86/xen-
> $(compat-arch-y).h
> > headers-$(CONFIG_X86) += compat/hvm/dm_op.h
> > headers-$(CONFIG_X86) += compat/hvm/hvm_op.h
> > headers-$(CONFIG_X86) += compat/hvm/hvm_vcpu.h
> > +headers-$(CONFIG_X86) += compat/iommu_op.h
>
> Did you forget to remove this when adding the entry above?
>
Yes, it should have gone.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
