Re: [Xen-devel] [PATCH V2 1/25] DOMCTL: Introduce new DOMCTL commands for vIOMMU support

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 04:34:02PM -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@xxxxxxxxx>
> ---
>  xen/common/domctl.c         |  3 +++
>  xen/common/viommu.c         | 43 +++++++++++++++++++++++++++++++++++++

I'm confused, I don't see this file in the repo, and the cover letter
doesn't mention this being based on top of any other series, where
does this viommu.c file come from?

>  xen/include/public/domctl.h | 52 
> +++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/viommu.h    |  6 ++++++
>  4 files changed, 104 insertions(+)
> 
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index d80488b..01c3024 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1144,6 +1144,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          if ( !ret )
>              copyback = 1;
>          break;
> +    case XEN_DOMCTL_viommu_op:
> +        ret = viommu_domctl(d, &op->u.viommu_op, &copyback);
> +        break;

Hm, shouldn't this be protected with #ifdef CONFIG_VIOMMU?

>      default:
>          ret = arch_do_domctl(op, d, u_domctl);
> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
> index 6874d9f..a4d004d 100644
> --- a/xen/common/viommu.c
> +++ b/xen/common/viommu.c
> @@ -148,6 +148,49 @@ static u64 viommu_query_caps(struct domain *d, u64 type)
>      return viommu_type->ops->query_caps(d);
>  }
>  
> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op,
> +                  bool *need_copy)
> +{
> +    int rc = -EINVAL, ret;

Do you really need both ret and rc?

> +    if ( !viommu_enabled() )
> +        return rc;

EINVAL? Maybe ENODEV?

> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_DOMCTL_create_viommu:
> +        ret = viommu_create(d, op->u.create_viommu.viommu_type,
> +            op->u.create_viommu.base_address,
> +            op->u.create_viommu.length,
> +            op->u.create_viommu.capabilities);

I would rather prefer for viommu_create to simply return an error or
0, and store the viommu_id by passing a pointer parameter to viommu_create, ie:

rc = viommu_create(d, op->u.create_viommu.viommu_type,
                   op->u.create_viommu.base_address,
                   op->u.create_viommu.length,
                   op->u.create_viommu.capabilities,
                   &op->u.create_viommu.viommu_id);

> +        if ( ret >= 0 ) {
                           ^ coding style
> +            op->u.create_viommu.viommu_id = ret;
> +            *need_copy = true;
> +            rc = 0; /* return 0 if success */
> +        }
> +        break;
> +
> +    case XEN_DOMCTL_destroy_viommu:
> +        rc = viommu_destroy(d, op->u.destroy_viommu.viommu_id);
> +        break;
> +
> +    case XEN_DOMCTL_query_viommu_caps:
> +        ret = viommu_query_caps(d, op->u.query_caps.viommu_type);

Same here, I would rather pass another parameter and use the return
for error only.

> +        if ( ret >= 0 )
> +        {
> +            op->u.query_caps.capabilities = ret;
> +            rc = 0;
> +        }
> +        *need_copy = true;
> +        break;
> +
> +    default:
> +        break;

Here you should return ENOSYS.

> +    }
> +
> +    return rc;
> +}
> +
>  int __init viommu_setup(void)
>  {
>      INIT_LIST_HEAD(&type_list);
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index ff39762..4b10f26 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1149,6 +1149,56 @@ 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     (1u << 0)

If this going to be used to specify the vendor only, it doesn't need
to be a bitfield, because it doesn't make sense to specify for
example VIOMMU_TYPE_INTEL_VTD | VIOMMU_TYPE_AMD, it's either Intel or
AMD. Do you have plans to expand this with other uses? In which case
the comment should be fixed.

> +
> +/* vIOMMU capabilities */
> +#define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)
> +
> +struct xen_domctl_viommu_op {
> +    uint32_t cmd;
> +#define XEN_DOMCTL_create_viommu          0
> +#define XEN_DOMCTL_destroy_viommu         1
> +#define XEN_DOMCTL_query_viommu_caps      2
> +    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 and length.
> +             */
> +            uint64_t base_address;
> +            /* IN - Length of MMIO region */
> +            uint64_t length;

It seems weird that you can specify the length, is that something
that a user would like to set? Isn't the length of the IOMMU MMIO
region fixed by the hardware spec?

> +            /* IN - Capabilities with which we want to create */
> +            uint64_t capabilities;
> +            /* OUT - vIOMMU identity */
> +            uint32_t viommu_id;
> +        } create_viommu;
> +
> +        struct {
> +            /* IN - vIOMMU identity */
> +            uint32_t viommu_id;
> +        } destroy_viommu;
> +
> +        struct {
> +            /* IN - vIOMMU type */
> +            uint64_t viommu_type;
> +            /* OUT - vIOMMU Capabilities */
> +            uint64_t capabilities;
> +        } query_caps;

This also seems weird, shouldn't you query the capabilities of an
already created vIOMMU, rather than a vIOMMU type? Shouldn't the first
field be viommu_id?

Roger.

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