Re: [PATCH V3 3/29] DOMCTL: Introduce new DOMCTL commands for vIOMMU support

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Sep 21, 2017 at 11:01:44PM -0400, Lan Tianyu wrote:
> This patch is to introduce create, destroy and query capabilities
> command for vIOMMU. vIOMMU layer will deal with requests and call
> arch vIOMMU ops.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  xen/common/domctl.c         |  6 ++++++
>  xen/common/viommu.c         | 30 ++++++++++++++++++++++++++++++
>  xen/include/public/domctl.h | 42 ++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/viommu.h    |  2 ++
>  4 files changed, 80 insertions(+)
> 
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 42658e5..7e28237 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1149,6 +1149,12 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>              copyback = 1;
>          break;
>  
> +#ifdef CONFIG_VIOMMU
> +    case XEN_DOMCTL_viommu_op:
> +        ret = viommu_domctl(d, &op->u.viommu_op, &copyback);

IMHO, I'm not really sure if it's worth to pass the copyback parameter
around. Can you just do the copy if !ret?

> +        break;
> +#endif

Instead of guarding every call to a viommu related function with
CONFIG_VIOMMU I would rather add dummy replacements for them in the
!CONFIG_VIOMMU case in the viommu.h header.

> +
>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
>          break;
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> index 64d91e6..55feb5d 100644
> --- a/xen/common/viommu.c
> +++ b/xen/common/viommu.c
> @@ -133,6 +133,36 @@ static int viommu_create(struct domain *d, uint64_t type,
>      return 0;
>  }
>  
> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op,
> +                  bool *need_copy)
> +{
> +    int rc = -EINVAL;

Why do you need to set rc to EINVAL? AFAICT there's no path that would
return rc without being initialized.

> +
> +    if ( !viommu_enabled() )
> +        return -ENODEV;
> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_create_viommu:
> +        rc = viommu_create(d, op->u.create.viommu_type,
> +                           op->u.create.base_address,
> +                           op->u.create.capabilities,
> +                           &op->u.create.viommu_id);
> +        if ( !rc )
> +            *need_copy = true;
> +        break;
> +
> +    case XEN_DOMCTL_destroy_viommu:
> +        rc = viommu_destroy_domain(d);
> +        break;
> +
> +    default:
> +        return -ENOSYS;
> +    }
> +
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 50ff58f..68854b6 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1163,6 +1163,46 @@ struct xen_domctl_psr_cat_op {
>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>  
> +/*  vIOMMU helper
> + *
> + *  vIOMMU interface can be used to create/destroy vIOMMU and
> + *  query vIOMMU capabilities.
> + */
> +
> +/* vIOMMU type - specify vendor vIOMMU device model */
> +#define VIOMMU_TYPE_INTEL_VTD           0
> +
> +/* vIOMMU capabilities */
> +#define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)

Please put those two defines next to the fields they belong to.

> +
> +struct xen_domctl_viommu_op {
> +    uint32_t cmd;
> +#define XEN_DOMCTL_create_viommu          0
> +#define XEN_DOMCTL_destroy_viommu         1

Would be nice if the values where right aligned.

> +    union {
> +        struct {
> +            /* IN - vIOMMU type */
> +            uint64_t viommu_type;
> +            /* 
> +             * IN - MMIO base address of vIOMMU. vIOMMU device models
> +             * are in charge of to check base_address.
> +             */
> +            uint64_t base_address;
> +            /* IN - Capabilities with which we want to create */
> +            uint64_t capabilities;
> +            /* OUT - vIOMMU identity */
> +            uint32_t viommu_id;
> +        } create;
> +
> +        struct {
> +            /* IN - vIOMMU identity */
> +            uint32_t viommu_id;
> +        } destroy;
> +    } u;
> +};

See my comments about the struct in patch 01/29.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel