Re: [PATCH v3 05/23] xen/arm: vsmmuv3: Add dummy support for virtual SMMUv3 for guests

[Julien Grall <julien@xxxxxxx>]

Hi Milan,

On 03/05/2026 19:38, Milan Djokic wrote:
> On 4/14/26 09:09, Julien Grall wrote:
> > > diff --git a/xen/arch/arm/include/asm/viommu.h b/xen/arch/arm/ 
> > > include/asm/viommu.h
> > > index 4598f543b8..2a6742de73 100644
> > > --- a/xen/arch/arm/include/asm/viommu.h
> > > +++ b/xen/arch/arm/include/asm/viommu.h
> > > @@ -5,9 +5,21 @@
> > >    #ifdef CONFIG_ARM_VIRTUAL_IOMMU
> > >    #include <xen/lib.h>
> > > +#include <xen/list.h>
> > >    #include <xen/types.h>
> > >    #include <public/xen.h>
> > > +extern struct list_head host_iommu_list;
> > > +
> > > +/* data structure for each hardware IOMMU */
> > > +struct host_iommu {
> > > +    struct list_head entry;
> > > +    const struct dt_device_node *dt_node;
> > > +    paddr_t addr;
> > > +    paddr_t size;
> > > +    uint32_t irq;
> > You don't seem to use ``irq`` in this patch. What is this meant to be
> > used for?
> This field will be used for vIOMMU event queue creation for the hardware 
> domain in a later patch (xen/arm: vsmmuv3: Add support for event queue 
> and global error). The emulated IRQ and MMIO region for the hardware 
> domain vIOMMU will match those of the host IOMMU.
I would prefer if this is introduced when you need it.

[..]

> > > diff --git a/xen/drivers/passthrough/arm/viommu.c b/xen/drivers/ 
> > > passthrough/arm/viommu.c
> > > index 7ab6061e34..53ae46349a 100644
> > > --- a/xen/drivers/passthrough/arm/viommu.c
> > > +++ b/xen/drivers/passthrough/arm/viommu.c
> > > @@ -2,12 +2,42 @@
> > >    #include <xen/errno.h>
> > >    #include <xen/init.h>
> > > +#include <xen/irq.h>
> > >    #include <xen/types.h>
> > >    #include <asm/viommu.h>
> > > +/* List of all host IOMMUs */
> > > +LIST_HEAD(host_iommu_list);
> > I don't quite follow why this is part of the common code. That said, why
> > do we need to register the host IOMMU? Wouldn't it be simpler to go
> > through the list of pIOMMU in the vSMMU v3 implementation?
> ``host_iommu_list`` is part of the generic code to allow reuse for other 
> IOMMU types in the future. For example, it can be reused for Renesas 
> IPMMU, rather than duplicating it.
> As for why we need to register IOMMUs, it seems more suitable to create 
> this list at initialization and add IOMMUs with the necessary properties 
> when the pIOMMU is probed.
> We can't reuse the same list from the SMMU driver because vIOMMU needs 
> raw DT properties (address, size), which I don't think we can extract 
> from the host driver list.
The IOMMU ABI is not fixed in Xen. It can be modified to fit the vIOMMU 
work. My main concern with the current approach is the list of 
information we may need will differ between IOMMUs and the information 
will need to be duplicated. So I am still not convinced this is the 
right way to have generic code.
It would be better if we have callback to prepare the firmware table 
(and maybe mapping/irq) for a given SMMU.
That said, it might be preferable to avoid any genericity until we 
actually know how this will be used by other virtual IOMMUs.
> > >    const struct viommu_desc __read_mostly *cur_viommu;
> > > +/* Common function for adding to host_iommu_list */
> > > +void add_to_host_iommu_list(paddr_t addr, paddr_t size,
> > > +                            const struct dt_device_node *node)
> > Is this supposed to only be called during __init? If so, this will help
> > to justify the ...
> >
> > > +{
> > > +    struct host_iommu *iommu_data;
> > > +
> > > +    iommu_data = xzalloc(struct host_iommu);
> > > +    if ( !iommu_data )
> > > +        panic("vIOMMU: Cannot allocate memory for host IOMMU data\n");
> > ... panic(). If not, then this function needs to return an error.
> Yes, this is called during init, on pIOMMU driver probe 
> (arm_smmu_device_probe())
> > > +
> > > +    iommu_data->addr = addr;
> > > +    iommu_data->size = size;
> > > +    iommu_data->dt_node = node;
> > > +    iommu_data->irq = platform_get_irq(node, 0);
> > > +    if ( iommu_data->irq < 0 )
> > > +    {
> > > +        gdprintk(XENLOG_ERR,
> > > +                 "vIOMMU: Cannot find a valid IOMMU irq\n");
> > Shouldn't you free the allocated memory? That said, why is it ok to
> > ignore the vIOMMU in this case?
> Yes, this is missing. We ignore the host IOMMU with an invalid IRQ 
> because event queue emulation won't work in this case.
What you are telling me is that the vIOMMU emulation doesn't support the 
event queue. However, if the OS is able to probe/use an SMMU without the 
IRQ then we should support it.
Cheers,

--
Julien Grall