Re: [PATCH v3 04/23] xen/arm: vIOMMU: add generic vIOMMU framework

[Milan Djokic <milan_djokic@xxxxxxxx>]

Hi Julien,

On 4/14/26 08:15, Julien Grall wrote:
> Hi Milan,
>
> On 31/03/2026 10:52, Milan Djokic wrote:
> > From: Rahul Singh <rahul.singh@xxxxxxx>
> >
> > This patch adds basic framework for vIOMMU.
> >
> > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
> > ---
> >    xen/arch/arm/dom0less-build.c        |  2 +
> >    xen/arch/arm/domain.c                | 33 +++++++++++++
> >    xen/arch/arm/domain_build.c          |  2 +
> >    xen/arch/arm/include/asm/viommu.h    | 70 ++++++++++++++++++++++++++++
> >    xen/drivers/passthrough/Kconfig      |  5 ++
> >    xen/drivers/passthrough/arm/Makefile |  1 +
> >    xen/drivers/passthrough/arm/viommu.c | 48 +++++++++++++++++++
> >    xen/include/public/arch-arm.h        |  5 ++
> >    xen/include/public/domctl.h          |  4 +-
> >    9 files changed, 168 insertions(+), 2 deletions(-)
> >    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/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index 4181c10538..067835e5d0 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -23,6 +23,7 @@
> >    #include <asm/arm64/sve.h>
> >    #include <asm/domain_build.h>
> >    #include <asm/firmware/sci.h>
> > +#include <asm/viommu.h>
> >    #include <asm/grant_table.h>
> >    #include <asm/setup.h>
> >    
> > @@ -317,6 +318,7 @@ int __init arch_parse_dom0less_node(struct dt_device_node *node,
> >        uint32_t val;
> >    
> >        d_cfg->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> > +    d_cfg->arch.viommu_type = viommu_get_type();
> >        d_cfg->flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
> >    
> >        if ( domu_dt_sci_parse(node, d_cfg) )
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 94b9858ad2..241f87386b 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -28,6 +28,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"
> > @@ -550,6 +551,14 @@ int arch_sanitise_domain_config(struct 
> > xen_domctl_createdomain *config)
> >            return -EINVAL;
> >        }
> >    
> > +    /* Check config structure padding */
> > +    if ( config->arch.pad )
> > +    {
> > +        dprintk(XENLOG_INFO,
> > +            "Invalid input config, padding must be zero\n");
> > +        return -EINVAL;
> > +    }
> > +
> >        /* Check feature flags */
> >        if ( sve_vl_bits > 0 )
> >        {
> > @@ -626,6 +635,21 @@ int arch_sanitise_domain_config(struct 
> > xen_domctl_createdomain *config)
> >            return -EINVAL;
> >        }
> >    
> > +    if ( !(config->flags & XEN_DOMCTL_CDF_iommu) &&
> > +         config->arch.viommu_type != XEN_DOMCTL_CONFIG_VIOMMU_NONE )
> > +    {
> > +        dprintk(XENLOG_INFO,
> > +                "vIOMMU requested while iommu not enabled for domain\n");
> > +        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 sci_domain_sanitise_config(config);
> >    }
> >    
> > @@ -721,6 +745,9 @@ int arch_domain_create(struct domain *d,
> >        if ( (rc = sci_domain_init(d, config)) != 0 )
> >            goto fail;
> >    
> > +    if ( (rc = domain_viommu_init(d, config->arch.viommu_type)) != 0 )
> > +        goto fail;
> > +
> >        return 0;
> >    
> >    fail:
> > @@ -965,6 +992,7 @@ enum {
> >        PROG_pci = 1,
> >        PROG_sci,
> >        PROG_tee,
> > +    PROG_viommu,
> I am not entirely sure about the position. Is the intention to
> relinquish the viommu state *after* the devices are detached? If so, it
> would be better to move this call just after 'PROG_pci' and add a
> comment indicating the dependency.
Yes, viommu cleanup should be performed after the devices are released.
After the rebase, it ended up in the wrong place because new components 
were added in the meantime.
I will update this and add a note for dependecy.

> >        PROG_xen,
> >        PROG_page,
> >        PROG_mapping,
> > @@ -1021,6 +1049,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 e8795745dd..a51563ee3d 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -35,6 +35,7 @@
> >    #include <asm/arm64/sve.h>
> >    #include <asm/cpufeature.h>
> >    #include <asm/domain_build.h>
> > +#include <asm/viommu.h>
> >    #include <xen/event.h>
> >    
> >    #include <xen/irq.h>
> > @@ -1946,6 +1947,7 @@ void __init create_dom0(void)
> >        dom0_cfg.arch.nr_spis = vgic_def_nr_spis();
> >        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..4598f543b8
> > --- /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_ARM_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;
> Below, you define viommu_type as 'uint8_t'. So shouldn't this also be
> 'uint8_t'?
Should be uint8_t, will fix it.

> > +};
> > +
> > +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_ARM_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 b413c33a4c..3c174bc87b 100644
> > --- a/xen/drivers/passthrough/Kconfig
> > +++ b/xen/drivers/passthrough/Kconfig
> > @@ -35,6 +35,11 @@ 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 ARM_VIRTUAL_IOMMU
> > +       bool "Virtual IOMMU Support (UNSUPPORTED)" if UNSUPPORTED
> > +       help
> > +        Support virtual IOMMU infrastructure to implement vIOMMU.
> > +
> >    endif
> >    
> >    config AMD_IOMMU
> > diff --git a/xen/drivers/passthrough/arm/Makefile 
> > b/xen/drivers/passthrough/arm/Makefile
> > index c5fb3b58a5..c3783188e3 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_ARM_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;
> You don't seem to define 'cur_viommmu' in the header. So shouldn't this
> be 'static'? Also, AFAICT, 'cur_viommu' would be set only once at boot.
> So rather than using __read_mostly, you probably want to use
> '__ro_after_init'.
I will update the 'cur_viommu' properties and add a set function for it.


> > +
> > +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 cd563cf706..d4953d40fd 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -330,6 +330,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
> >    #define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
> >    #define XEN_DOMCTL_CONFIG_ARM_SCI_SCMI_SMC  1
> >    
> > +#define XEN_DOMCTL_CONFIG_VIOMMU_NONE   0
> > +
> >    struct xen_arch_domainconfig {
> >        /* IN/OUT */
> >        uint8_t gic_version;
> > @@ -355,6 +357,9 @@ struct xen_arch_domainconfig {
> >        uint32_t clock_frequency;
> >        /* IN */
> >        uint8_t arm_sci_type;
> > +    /* IN */
> > +    uint8_t viommu_type;
> > +    uint16_t pad;
> >    };
> >    #endif /* __XEN__ || __XEN_TOOLS__ */
> >    
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 8f6708c0a7..23124547f3 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -30,9 +30,9 @@
> >     * fields) don't require a change of the version.
> >     * Stable ops are NOT covered by XEN_DOMCTL_INTERFACE_VERSION!
> >     *
> > - * Last version bump: Xen 4.19
> > + * Last version bump: Xen 4.22
> >     */
> > -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000018
> >    
> >    /*
> >     * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> Cheers,
BR,
Milan