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

[Julien Grall <julien@xxxxxxx>]

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@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");
> +        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