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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

Jan



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