Re: [PATCH v3 08/23] xen/arm: vsmmuv3: Add support for registers emulation

[Julien Grall <julien@xxxxxxx>]

Hi Milan,

On 18/05/2026 01:17, Milan Djokic wrote:
> Hi Julien,
>
> On 4/14/26 10:10, Julien Grall wrote:
> > Hi Milan,
> >
> > On 31/03/2026 10:52, Milan Djokic wrote:
> > > From: Rahul Singh <rahul.singh@xxxxxxx>
> > >
> > > Add initial support for various emulated registers for virtual SMMUv3
> > > for guests and also add support for virtual cmdq and eventq.
> > >
> > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > Signed-off-by: Milan Djokic <milan_djokic@xxxxxxxx>
> > > ---
> > >    xen/drivers/passthrough/arm/smmu-v3.h  |   6 +
> > >    xen/drivers/passthrough/arm/vsmmu-v3.c | 286 +++++++++++++++++++++ 
> > > ++++
> > >    2 files changed, 292 insertions(+)
> > >
> > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.h b/xen/drivers/ 
> > > passthrough/arm/smmu-v3.h
> > > index 3fb13b7e21..fab4fd5a26 100644
> > > --- a/xen/drivers/passthrough/arm/smmu-v3.h
> > > +++ b/xen/drivers/passthrough/arm/smmu-v3.h
> > > @@ -60,6 +60,12 @@
> > >    #define IDR5_VAX            GENMASK(11, 10)
> > >    #define IDR5_VAX_52_BIT            1
> > > +#define ARM_SMMU_IIDR            0x18
> > > +#define IIDR_PRODUCTID            GENMASK(31, 20)
> > > +#define IIDR_VARIANT            GENMASK(19, 16)
> > > +#define IIDR_REVISION            GENMASK(15, 12)
> > > +#define IIDR_IMPLEMENTER        GENMASK(11, 0)
> > > +
> > >    #define ARM_SMMU_CR0            0x20
> > >    #define CR0_ATSCHK            (1 << 4)
> > >    #define CR0_CMDQEN            (1 << 3)
> > > diff --git a/xen/drivers/passthrough/arm/vsmmu-v3.c b/xen/drivers/ 
> > > passthrough/arm/vsmmu-v3.c
> > > index e36f200ba5..3ae1e62a50 100644
> > > --- a/xen/drivers/passthrough/arm/vsmmu-v3.c
> > > +++ b/xen/drivers/passthrough/arm/vsmmu-v3.c
> > > @@ -3,25 +3,307 @@
> > >    #include <xen/param.h>
> > >    #include <xen/sched.h>
> > >    #include <asm/mmio.h>
> > > +#include <asm/vgic-emul.h>
> > vgic-emul.h is intended to only be used in the vGIC code. I am fine if
> > you want to use it in vsmmu-v3.c but it needs to be renamed. Maybe to
> > vdev-emul.h.
> Sure, I'll rename it
>
> > >    #include <asm/viommu.h>
> > > +#include <asm/vreg.h>
> > > +
> > > +#include "smmu-v3.h"
> > > +
> > > +/* Register Definition */
> > > +#define ARM_SMMU_IDR2       0x8
> > > +#define ARM_SMMU_IDR3       0xc
> > > +#define ARM_SMMU_IDR4       0x10
> > > +#define IDR0_TERM_MODEL     (1 << 26)
> > > +#define IDR3_RIL            (1 << 10)
> > > +#define CR0_RESERVED        0xFFFFFC20
> > AFAIU, this is covering all the bits defined by the SMMU spec. But some
> > of them are optional. Does this mean we will expose those optional 
> > features?
> Right now only mandatory features are supported (SMMU_EN, CMDQ, EVTQ). 
> Most of the optional features are not advertised in the IDR registers, 
> so guests are not expected to enable or use them via CR0.
Guests are not trusted by default. So what is the guest tries to set them?

> > > +#define SMMU_IDR1_SIDSIZE   16
> > > +#define SMMU_CMDQS          19
> > Can you add some details how you decided the size of the command and ...
> >
> > > +#define SMMU_EVTQS          19
> > ... even queues?
> The CMDQ/EVTQ sizes are currently set to the architectural maximum. 
> Since there is no direct dependency on the underlying hardware queue 
> sizes, using the maximum supported value seemed like the simplest option.
> > > +#define DWORDS_BYTES        8
> > > +#define ARM_SMMU_IIDR_VAL   0x12
> > I am not sure which implementer this is referring to. But how do you
> > plan to handle errata? Are we sure they can always be handled by Xen?
> This is currently a dummy value used to avoid triggering guest driver 
> errata/quirk paths. I will replace it with a more meaningful value. 
> Using the Arm implementer ID with the remaining fields cleared should be 
> sufficient.
I am not sure to understand why would that value be unused. Do you have 
more details?
> My expectation is that errata handling should remain in Xen rather than 
> the guest.
I am not fully convinced you will be able to apply all the errata in the 
hypervisor. At least with close to no cost.
[...]

> > >    /* Struct to hold the vIOMMU ops and vIOMMU type */
> > >    extern const struct viommu_desc __read_mostly *cur_viommu;
> > > +/* virtual smmu queue */
> > > +struct arm_vsmmu_queue {
> > > +    uint64_t    q_base; /* base register */
> > > +    uint32_t    prod;
> > > +    uint32_t    cons;
> > > +    uint8_t     ent_size;
> > > +    uint8_t     max_n_shift;
> > > +};
> > > +
> > >    struct virt_smmu {
> > >        struct      domain *d;
> > >        struct      list_head viommu_list;
> > > +    uint8_t     sid_split;
> > > +    uint32_t    features;
> > > +    uint32_t    cr[3];
> > > +    uint32_t    cr0ack;
> > > +    uint32_t    gerror;
> > > +    uint32_t    gerrorn;
> > > +    uint32_t    strtab_base_cfg;
> > > +    uint64_t    strtab_base;
> > > +    uint32_t    irq_ctrl;
> > > +    uint64_t    gerror_irq_cfg0;
> > > +    uint64_t    evtq_irq_cfg0;
> > > +    struct      arm_vsmmu_queue evtq, cmdq;
> > >    };
> > >    static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
> > >                                  register_t r, void *priv)
> > >    {
> > > +    struct virt_smmu *smmu = priv;
> > > +    uint64_t reg;
> > > +    uint32_t reg32;
> > Looking at this helper and the read one, I am bit surprised there is no
> > lock taken nor we check the access size.  Can you explain why?
> >
> > For instance, we should not allow 64-bit access on 32-bit register. The
> > rest of the size (8-bit and 16-bit) is IMP DEFINED so it may be easier
> > just not allow them.
> Most of the configuration registers are expected to be accessed in a 
> serialized manner by the guest driver, during driver initialization.
I am afraid we can't trust the guest to do the right thing... So we need 
to make sure this could not lead to an invalid state in the emulation.
Furthermore, on baremetal, when a two pCPUs are trying to write to the 
same address, you will be able to see value A or value B but not a mix. 
Without a lock, I don't believe this is upheld in your implementation.
[...]

> > NIT: The vIOMMU is per-domain so it is sufficient to print "%pd".
> >
> > > +               v, info->dabt.reg, (unsigned long)info->gpa & 0xffff);
> > > +        return IO_ABORT;
> > Per section 6 of the SMMU:
> >
> > "
> > For all pages except Page 1, undefined register locations are RES0. For
> > Page 1, access to undefined/Reserved
> > register locations is CONSTRAINED UNPREDICTABLE and an implementation
> > has one of the following behaviors:
> > [...]
> > "
> >
> > Here you seem to implement page0 so the default case should be write
> > ignore and therefore IO_HANDLED should be returned. BTW, you don't seem
> > to handle page1. Is this going to be handled later on?
>    From page1, right now only EVTQ registers are emulated. PRI is not 
> supported, but might be needed in the future for the PCI support (PRI 
> queue registers also belong to page1, but not emulated atm)
> So I think that page1 will be handled when PCI support is completed.
I am a bit confused with this answer. Are you saying you will handle 
page1 for the event queue register in another patch in this series?
> > > +    }
> > > +
> > >        return IO_HANDLED;
> > >    }
> > >    static int vsmmuv3_mmio_read(struct vcpu *v, mmio_info_t *info,
> > >                                 register_t *r, void *priv)
> > >    {
> > > +    struct virt_smmu *smmu = priv;
> > > +    uint64_t reg;
> > > +
> > > +    switch ( info->gpa & 0xffff )
> > > +    {
> > > +    case VREG32(ARM_SMMU_IDR0):
> > > +        reg  = FIELD_PREP(IDR0_S1P, 1) | FIELD_PREP(IDR0_TTF, 2) |
> > As the page-table will be used by the HW, shouldn't TTF reflect what the
> > HW supports? This would allow the vIOMMU to work for 32-bit domains.
> If my understanding is correct, Xen SMMU driver only supports AArch64 
> table format, so I think that we can't advertise 32-bit table format in 
> the emulation layer even if the hardware supports it.
Do you mind pointing me to the code? The page-tables are shared between 
the SMMU and the CPU. So we ought to support both.
Cheers,

--
Julien Grall