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

[Milan Djokic <milan_djokic@xxxxxxxx>]

Hi Julien,

On 5/24/26 13:49, Julien Grall wrote:
> 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?
This could cause issues on the guest side, as it may lead the guest to 
believe that additional features have been successfully enabled (for 
example, if the guest does not verify the advertised capabilities in the 
IDR registers).
I will also mask the bits corresponding to unsupported features in 
CR0_ACK to cover this case.
> > > > +#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?
I think that the IIDR is always used by the guest driver during 
initialization to identify the implementer/product revision and enable 
any required workarounds.
If that is the usage you are referring to, then using a generic IIDR 
value would prevent the guest driver from activating any 
implementer-specific workaround paths.
> > 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.
Yes, this is potentially problematic. However, at the moment I am not 
sure what the alternative would be, as I think that guest-side errata 
handling could be applied incorrectly due to the emulation layer.
> [...]
>
> > > >     /* 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.
>
> [...]
I see your point. I will extend the locking to all registers with 
potential concurrent access.
> > > 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?
Sorry for the confusion. The EVTQ registers on Page 1 (SMMU_EVTQ_PROD 
and SMMU_EVTQ_CONS) are intended to be emulated in this patch.
However, Page0 offset is applied when decoding register accesses, so 
these Page 1 registers are not handled correctly. I will fix the 
register decoding accordingly.
> > > > +    }
> > > > +
> > > >         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.
The reason I assumed only the AArch64 format should be advertised is 
that the Xen SMMU driver currently appears to require AArch64 table 
format support during device probe.
In arm_smmu_device_hw_probe() (xen/drivers/passthrough/arm/smmu-v3.c), 
the handling is:
/* We only support the AArch64 table format at present */
switch (FIELD_GET(IDR0_TTF, reg)) {
case IDR0_TTF_AARCH32_64:
    smmu->ias = 40;
    fallthrough;
case IDR0_TTF_AARCH64:
    break;
default:
    dev_err(smmu->dev, "AArch64 table format not supported!\n");
    return -ENXIO;
}

Based on this, I assumed that advertising AArch32 table format support 
in the emulation would not be correct. However, I may be missing 
something regarding how the guest page tables are shared between the CPU 
and the SMMU in this setup.
> Cheers,
Best regards,
Milan