Re: [PATCH v6 05/14] public / x86: introduce __HYPERCALL_iommu_op

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 04 September 2018 12:50
> 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 05/14] public / x86: introduce
> __HYPERCALL_iommu_op
> 
> >>> On 23.08.18 at 11:47, <paul.durrant@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