[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 05/14] public / x86: introduce __HYPERCALL_iommu_op
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@xxxxxxxx] > Sent: 04 September 2018 12:50 > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx> > Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu > <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian > Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini > <sstabellini@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; > Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) > <tim@xxxxxxx> > Subject: Re: [PATCH v6 05/14] public / x86: introduce > __HYPERCALL_iommu_op > > >>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |