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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Aug 09, 2017 at 04:34:02PM -0400, Lan Tianyu wrote:
[...]
>  
> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op,
> +                  bool *need_copy)
> +{
> +    int rc = -EINVAL, ret;
> +
> +    if ( !viommu_enabled() )
> +        return rc;
> +
> +    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);

Please align these with "d" in previous line.

> +        if ( ret >= 0 ) {

Coding style is wrong.

> +            op->u.create_viommu.viommu_id = ret;
> +            *need_copy = true;
> +            rc = 0; /* return 0 if success */

No need to have that comment.

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

Why use a bit when the types are mutually exclusive? Using a number
should be fine?

> +
> +/* 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;
> +            /* IN - Capabilities with which we want to create */
> +            uint64_t capabilities;
> +            /* OUT - vIOMMU identity */
> +            uint32_t viommu_id;
> +        } create_viommu;

create should be fine.

> +
> +        struct {
> +            /* IN - vIOMMU identity */
> +            uint32_t viommu_id;
> +        } destroy_viommu;

destroy should be fine.

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