Mailing List Archive

[RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework
This patch adds basic framework for vIOMMU.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
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");
+ 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;
+};
+
+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)
+{
+ return XEN_DOMCTL_CONFIG_VIOMMU_NONE;
+}
+
+static inline int domain_viommu_init(struct domain *d, uint16_t viommu_type)
+{
+ 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;
+ /* IN */
uint16_t tee_type;
/* IN */
uint32_t nr_spis;
--
2.25.1
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
On 01.12.2022 17:02, Rahul Singh wrote:
> --- 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.

I simply "virtual" specific enough in the name? Seeing that there are
multiple IOMMU flavors for Arm, and judging from the titles of subsequent
patches, you're implementing a virtualized form of only one variant.

Also, nit: Please omit "default n" here - it leads to a needless
line in the resulting .config, which in addition prevents the prompt
from appearing for user selection when someone later enables
UNSUPPORTED in their config and then runs e.g. "make oldconfig". But
perhaps you anyway really mean

config VIRTUAL_IOMMU
bool "Virtual IOMMU Support (UNSUPPORTED)"
depends on UNSUPPORTED
help
Support virtual IOMMU infrastructure to implement vIOMMU.

?

Note (nit again) the slightly altered indentation I'm also using in
the alternative suggestion.

Jan
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
Hi Rahul,

On 01/12/2022 16:02, Rahul Singh wrote:
> This patch adds basic framework for vIOMMU.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> 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");
> + 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;

I would have expected us to relinquish the vIOMMU resources *before* we
detach the devices. So can you explain the ordering?

> +
> 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(),

I don't think the vIOMMU should be enabled to dom0less domU by default.

> .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();

Same here.

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

Did you mean ID rather than type?

> + * Should be one of XEN_DOMCTL_CONFIG_VIOMMU_xxx
> + */
> + uint16_t viommu_type;

The domctl is uint8_t.

Cheers,

--
Julien Grall
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
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@arm.com>
> ---
> 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
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
Hi Jan,

> On 2 Dec 2022, at 8:39 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.12.2022 17:02, Rahul Singh wrote:
>> --- 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.
>
> I simply "virtual" specific enough in the name? Seeing that there are
> multiple IOMMU flavors for Arm, and judging from the titles of subsequent
> patches, you're implementing a virtualized form of only one variant.

I agree with you I will remove the virtual in next version.
>
> Also, nit: Please omit "default n" here - it leads to a needless
> line in the resulting .config, which in addition prevents the prompt
> from appearing for user selection when someone later enables
> UNSUPPORTED in their config and then runs e.g. "make oldconfig". But
> perhaps you anyway really mean
>
> config VIRTUAL_IOMMU
> bool "Virtual IOMMU Support (UNSUPPORTED)"
> depends on UNSUPPORTED
> help
> Support virtual IOMMU infrastructure to implement vIOMMU.
>
> ?
>
> Note (nit again) the slightly altered indentation I'm also using in
> the alternative suggestion.
>

I will modify as below:

config VIRTUAL_IOMMU
bool "Virtual IOMMU Support (UNSUPPORTED)”
depends on UNSUPPORTED
help
Support IOMMU infrastructure to implement different variants of virtual
IOMMUs.

Regards,
Rahul
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
Hi Julien,

> On 3 Dec 2022, at 9:54 pm, Julien Grall <julien@xen.org> wrote:
>
> Hi Rahul,
>
> On 01/12/2022 16:02, Rahul Singh wrote:
>> This patch adds basic framework for vIOMMU.
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> 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");
>> + 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;
>
> I would have expected us to relinquish the vIOMMU resources *before* we detach the devices. So can you explain the ordering?

I think first we need to detach the device that will set the S1 and S2 stage translation to bypass/abort then we
need to remove the vIOMMU. Do you have anything that you feel if we relinquish the vIOMMU after detach is not right.

>
>> +
>> 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(),
>
> I don't think the vIOMMU should be enabled to dom0less domU by default.

I am not enabling the vIOMMU by default. If vIOMMU is disabled via the command line or config option
then viommu_get_type() will return XEN_DOMCTL_CONFIG_VIOMMU_NONE and in that case
domain_viommu_init() will return 0 without doing anything.

>
>> .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();
>
> Same here.
>
>> 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.
>
> Did you mean ID rather than type?

I mean here type of physical IOMMU on the host available SMMUv3, SMMUv1 or IPMMU.
If you think ID is the right name here I will change it.

>
>> + * Should be one of XEN_DOMCTL_CONFIG_VIOMMU_xxx
>> + */
>> + uint16_t viommu_type;
>
> The domctl is uint8_t.

Ack. I will fix that.

Regards,
Rahul
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
Hi Michal,

> On 5 Dec 2022, at 8:26 am, Michal Orzel <michal.orzel@amd.com> wrote:
>
> 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@arm.com>
>> ---
>> 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"

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

I will modify the in viommu_type to uint8_t in "arch/arm/include/asm/viommu.h" and will
also fix everywhere the viommu_type to uint8_t.

Regards,
Rahul
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
On 05/12/2022 13:48, Rahul Singh wrote:
> Hi Julien,
>
>> On 3 Dec 2022, at 9:54 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 01/12/2022 16:02, Rahul Singh wrote:
>>> This patch adds basic framework for vIOMMU.
>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>>> ---
>>> 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");
>>> + 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;
>>
>> I would have expected us to relinquish the vIOMMU resources *before* we detach the devices. So can you explain the ordering?
>
> I think first we need to detach the device that will set the S1 and S2 stage translation to bypass/abort then we
> need to remove the vIOMMU. Do you have anything that you feel if we relinquish the vIOMMU after detach is not right.

iommu_release_dt_devices() will effectively remove the device from the
domain. One could decide to store the S1 information per device and
therefore it feels wrong that we are removing the S1 information later on.

In addition to that, a device can be removed while the domain is running.

Therefore I think it would make more sense to remove the vIOMMU every
time we deassign the device.

>>> 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(),
>>
>> I don't think the vIOMMU should be enabled to dom0less domU by default.
>
> I am not enabling the vIOMMU by default. If vIOMMU is disabled via the command line or config option
> then viommu_get_type() will return XEN_DOMCTL_CONFIG_VIOMMU_NONE and in that case
> domain_viommu_init() will return 0 without doing anything.

What I mean by default is if the command line is set then you will
enable the vIOMMU for both dom0 and dom0less domUs.

vIOMMU should be selected per-domain.

>
>>
>>> .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();
>>
>> Same here.
>>
>>> 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.
>>
>> Did you mean ID rather than type?
>
> I mean here type of physical IOMMU on the host available SMMUv3, SMMUv1 or IPMMU.
> If you think ID is the right name here I will change it.

Hmmm... I inverted the two names. I was actually asking whether ID
should be Type instead.

Cheers,

--
Julien Grall
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
Hi Rahul,

On 05/12/2022 14:53, Rahul Singh wrote:
>
>
> Hi Michal,
>
>> On 5 Dec 2022, at 8:26 am, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> 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@arm.com>
>>> ---
>>> 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"
>
> Ack.
>>
>>> + 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
>
> I will modify the in viommu_type to uint8_t in "arch/arm/include/asm/viommu.h" and will
> also fix everywhere the viommu_type to uint8_t.
Also I think that you need to bump XEN_DOMCTL_INTERFACE_VERSION due to the change
in struct xen_arch_domainconfig.

>
> Regards,
> Rahul
>
>
~Michal
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
On 05/12/2022 14:25, Michal Orzel wrote:
>>>> 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
>>
>> I will modify the in viommu_type to uint8_t in "arch/arm/include/asm/viommu.h" and will
>> also fix everywhere the viommu_type to uint8_t.
> Also I think that you need to bump XEN_DOMCTL_INTERFACE_VERSION due to the change
> in struct xen_arch_domainconfig.

We only need to bump the domctl version once per release. So if this is
the first modification of domctl.h in 4.18 then yes.

That said, I am not sure whether this is necessary here as you are using
a padding.

@Rahul, BTW, I think you may need to regenerate the bindings for OCaml
and Go.

Cheers,

--
Julien Grall
Re: [RFC PATCH 04/21] xen/arm: vIOMMU: add generic vIOMMU framework [ In reply to ]
Hi Julien,

> On 5 Dec 2022, at 3:20 pm, Julien Grall <julien@xen.org> wrote:
>
> On 05/12/2022 14:25, Michal Orzel wrote:
>>>>> 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
>>>
>>> I will modify the in viommu_type to uint8_t in "arch/arm/include/asm/viommu.h" and will
>>> also fix everywhere the viommu_type to uint8_t.
>> Also I think that you need to bump XEN_DOMCTL_INTERFACE_VERSION due to the change
>> in struct xen_arch_domainconfig.
>
> We only need to bump the domctl version once per release. So if this is the first modification of domctl.h in 4.18 then yes.
>
> That said, I am not sure whether this is necessary here as you are using a padding.
>
> @Rahul, BTW, I think you may need to regenerate the bindings for OCaml and Go.

Ack. I will check this before sending the v2.

Regards,
Rahul