Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Rahul,

On 01/12/2022 17:02, Rahul Singh wrote:
> 
> 
> This patch adds basic framework for vIOMMU.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
>  xen/arch/arm/domain.c                | 17 +++++++
>  xen/arch/arm/domain_build.c          |  3 ++
>  xen/arch/arm/include/asm/viommu.h    | 70 ++++++++++++++++++++++++++++
>  xen/drivers/passthrough/Kconfig      |  6 +++
>  xen/drivers/passthrough/arm/Makefile |  1 +
>  xen/drivers/passthrough/arm/viommu.c | 48 +++++++++++++++++++
>  xen/include/public/arch-arm.h        |  4 ++
>  7 files changed, 149 insertions(+)
>  create mode 100644 xen/arch/arm/include/asm/viommu.h
>  create mode 100644 xen/drivers/passthrough/arm/viommu.c
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 38e22f12af..2a85209736 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -37,6 +37,7 @@
>  #include <asm/tee/tee.h>
>  #include <asm/vfp.h>
>  #include <asm/vgic.h>
> +#include <asm/viommu.h>
>  #include <asm/vtimer.h>
> 
>  #include "vpci.h"
> @@ -691,6 +692,13 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
> 
> +    if ( config->arch.viommu_type != XEN_DOMCTL_CONFIG_VIOMMU_NONE )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "vIOMMU type requested not supported by the platform or 
> Xen\n");
Maybe a simpler message like for TEE would be better: "Unsupported vIOMMU type"

> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
> 
> @@ -783,6 +791,9 @@ int arch_domain_create(struct domain *d,
>      if ( (rc = domain_vpci_init(d)) != 0 )
>          goto fail;
> 
> +    if ( (rc = domain_viommu_init(d, config->arch.viommu_type)) != 0 )
> +        goto fail;
> +
>      return 0;
> 
>  fail:
> @@ -998,6 +1009,7 @@ static int relinquish_memory(struct domain *d, struct 
> page_list_head *list)
>  enum {
>      PROG_pci = 1,
>      PROG_tee,
> +    PROG_viommu,
>      PROG_xen,
>      PROG_page,
>      PROG_mapping,
> @@ -1048,6 +1060,11 @@ int domain_relinquish_resources(struct domain *d)
>          if (ret )
>              return ret;
> 
> +    PROGRESS(viommu):
> +        ret = viommu_relinquish_resources(d);
> +        if (ret )
> +            return ret;
> +
>      PROGRESS(xen):
>          ret = relinquish_memory(d, &d->xenpage_list);
>          if ( ret )
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bd30d3798c..abbaf37a2e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -27,6 +27,7 @@
>  #include <asm/setup.h>
>  #include <asm/cpufeature.h>
>  #include <asm/domain_build.h>
> +#include <asm/viommu.h>
>  #include <xen/event.h>
> 
>  #include <xen/irq.h>
> @@ -3858,6 +3859,7 @@ void __init create_domUs(void)
>          struct domain *d;
>          struct xen_domctl_createdomain d_cfg = {
>              .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> +            .arch.viommu_type = viommu_get_type(),
>              .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>              /*
>               * The default of 1023 should be sufficient for guests because
> @@ -4052,6 +4054,7 @@ void __init create_dom0(void)
>          printk(XENLOG_WARNING "Maximum number of vGIC IRQs exceeded.\n");
>      dom0_cfg.arch.tee_type = tee_get_type();
>      dom0_cfg.max_vcpus = dom0_max_vcpus();
> +    dom0_cfg.arch.viommu_type = viommu_get_type();
> 
>      if ( iommu_enabled )
>          dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> diff --git a/xen/arch/arm/include/asm/viommu.h 
> b/xen/arch/arm/include/asm/viommu.h
> new file mode 100644
> index 0000000000..7cd3818a12
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/viommu.h
> @@ -0,0 +1,70 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
> +#ifndef __ARCH_ARM_VIOMMU_H__
> +#define __ARCH_ARM_VIOMMU_H__
> +
> +#ifdef CONFIG_VIRTUAL_IOMMU
> +
> +#include <xen/lib.h>
> +#include <xen/types.h>
> +#include <public/xen.h>
> +
> +struct viommu_ops {
> +    /*
> +     * Called during domain construction if toolstack requests to enable
> +     * vIOMMU support.
> +     */
> +    int (*domain_init)(struct domain *d);
> +
> +    /*
> +     * Called during domain destruction to free resources used by vIOMMU.
> +     */
> +    int (*relinquish_resources)(struct domain *d);
> +};
> +
> +struct viommu_desc {
> +    /* vIOMMU domains init/free operations described above. */
> +    const struct viommu_ops *ops;
> +
> +    /*
> +     * ID of vIOMMU. Corresponds to xen_arch_domainconfig.viommu_type.
> +     * Should be one of XEN_DOMCTL_CONFIG_VIOMMU_xxx
> +     */
> +    uint16_t viommu_type;
Here the viommu_type is uint16_t ...

> +};
> +
> +int domain_viommu_init(struct domain *d, uint16_t viommu_type);
> +int viommu_relinquish_resources(struct domain *d);
> +uint16_t viommu_get_type(void);
> +
> +#else
> +
> +static inline uint8_t viommu_get_type(void)
Here you are returning uint8_t ...

> +{
> +    return XEN_DOMCTL_CONFIG_VIOMMU_NONE;
> +}
> +
> +static inline int domain_viommu_init(struct domain *d, uint16_t viommu_type)
Here you are taking uint16_t. So it looks like that ...

> +{
> +    if ( likely(viommu_type == XEN_DOMCTL_CONFIG_VIOMMU_NONE) )
> +        return 0;
> +
> +    return -ENODEV;
> +}
> +
> +static inline int viommu_relinquish_resources(struct domain *d)
> +{
> +    return 0;
> +}
> +
> +#endif /* CONFIG_VIRTUAL_IOMMU */
> +
> +#endif /* __ARCH_ARM_VIOMMU_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
> index 479d7de57a..19924fa2de 100644
> --- a/xen/drivers/passthrough/Kconfig
> +++ b/xen/drivers/passthrough/Kconfig
> @@ -35,6 +35,12 @@ config IPMMU_VMSA
>           (H3 ES3.0, M3-W+, etc) or Gen4 SoCs which IPMMU hardware supports 
> stage 2
>           translation table format and is able to use CPU's P2M table as is.
> 
> +config VIRTUAL_IOMMU
> +       bool "Virtual IOMMU Support (UNSUPPORTED)" if UNSUPPORTED
> +       default n
> +       help
> +        Support virtual IOMMU infrastructure to implement vIOMMU.
> +
>  endif
> 
>  config IOMMU_FORCE_PT_SHARE
> diff --git a/xen/drivers/passthrough/arm/Makefile 
> b/xen/drivers/passthrough/arm/Makefile
> index c5fb3b58a5..4cc54f3f4d 100644
> --- a/xen/drivers/passthrough/arm/Makefile
> +++ b/xen/drivers/passthrough/arm/Makefile
> @@ -2,3 +2,4 @@ obj-y += iommu.o iommu_helpers.o iommu_fwspec.o
>  obj-$(CONFIG_ARM_SMMU) += smmu.o
>  obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
>  obj-$(CONFIG_ARM_SMMU_V3) += smmu-v3.o
> +obj-$(CONFIG_VIRTUAL_IOMMU) += viommu.o
> diff --git a/xen/drivers/passthrough/arm/viommu.c 
> b/xen/drivers/passthrough/arm/viommu.c
> new file mode 100644
> index 0000000000..7ab6061e34
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/viommu.c
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */
> +
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/types.h>
> +
> +#include <asm/viommu.h>
> +
> +const struct viommu_desc __read_mostly *cur_viommu;
> +
> +int domain_viommu_init(struct domain *d, uint16_t viommu_type)
> +{
> +    if ( viommu_type == XEN_DOMCTL_CONFIG_VIOMMU_NONE )
> +        return 0;
> +
> +    if ( !cur_viommu )
> +        return -ENODEV;
> +
> +    if ( cur_viommu->viommu_type != viommu_type )
> +        return -EINVAL;
> +
> +    return cur_viommu->ops->domain_init(d);
> +}
> +
> +int viommu_relinquish_resources(struct domain *d)
> +{
> +    if ( !cur_viommu )
> +        return 0;
> +
> +    return cur_viommu->ops->relinquish_resources(d);
> +}
> +
> +uint16_t viommu_get_type(void)
> +{
> +    if ( !cur_viommu )
> +        return XEN_DOMCTL_CONFIG_VIOMMU_NONE;
> +
> +    return cur_viommu->viommu_type;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 1528ced509..33d32835e7 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -297,10 +297,14 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>  #define XEN_DOMCTL_CONFIG_TEE_NONE      0
>  #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
> 
> +#define XEN_DOMCTL_CONFIG_VIOMMU_NONE   0
> +
>  struct xen_arch_domainconfig {
>      /* IN/OUT */
>      uint8_t gic_version;
>      /* IN */
> +    uint8_t viommu_type;
this should be uint16_t and not uint8_t 

> +    /* IN */
>      uint16_t tee_type;
>      /* IN */
>      uint32_t nr_spis;
> --
> 2.25.1
> 
> 

~Michal