Re: [PATCH v4 12/22] xen/arm: vsmmuv3: Add support for event queue and global error

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

On 10/7/26 03:07, Milan Djokic wrote:

Hello Milan


> From: Rahul Singh <rahul.singh@arm.com>
> 
> Event queue is used to send the events to guest when there is an events/
> faults. Add support for event queue to send events to guest.
> 
> Global error in SMMUv3 hw will be updated in smmu_gerror and
> smmu_gerrorn register. Add support for global error registers to send
> global error to guest.
> 
> Add architectural error handling to the emulated command queue,
> updating the global error register and stopping command consumption
> on error.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Signed-off-by: Milan Djokic <milan_djokic@epam.com>
> ---
>   xen/drivers/passthrough/arm/smmu-v3.h  |  20 +++
>   xen/drivers/passthrough/arm/vsmmu-v3.c | 233 +++++++++++++++++++++++--
>   xen/include/public/arch-arm.h          |   2 +
>   3 files changed, 244 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.h b/xen/drivers/passthrough/arm/smmu-v3.h
> index 4a912be740..44ecb5d89c 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.h
> +++ b/xen/drivers/passthrough/arm/smmu-v3.h
> @@ -358,6 +358,26 @@
>   
>   #define EVTQ_0_ID			GENMASK_ULL(7, 0)
>   
> +#define EVT_ID_BAD_STREAMID		0x02
> +#define EVT_ID_BAD_STE		    	0x04
> +#define EVT_ID_TRANSLATION_FAULT	0x10
> +#define EVT_ID_ADDR_SIZE_FAULT		0x11
> +#define EVT_ID_ACCESS_FAULT		0x12
> +#define EVT_ID_PERMISSION_FAULT		0x13
> +
> +#define EVTQ_0_SSV			(1UL << 11)
> +#define EVTQ_0_SSID			GENMASK_ULL(31, 12)
> +#define EVTQ_0_SID			GENMASK_ULL(63, 32)
> +#define EVTQ_1_STAG			GENMASK_ULL(15, 0)
> +#define EVTQ_1_STALL			(1UL << 31)
> +#define EVTQ_1_PnU			(1UL << 33)
> +#define EVTQ_1_InD			(1UL << 34)
> +#define EVTQ_1_RnW			(1UL << 35)
> +#define EVTQ_1_S2			(1UL << 39)
> +#define EVTQ_1_CLASS			GENMASK_ULL(41, 40)
> +#define EVTQ_1_TT_READ			(1UL << 44)
> +#define EVTQ_2_ADDR			GENMASK_ULL(63, 0)
> +#define EVTQ_3_IPA			GENMASK_ULL(51, 12)
>   /* PRI queue */
>   #define PRIQ_ENT_SZ_SHIFT		4
>   #define PRIQ_ENT_DWORDS			((1 << PRIQ_ENT_SZ_SHIFT) >> 3)
> diff --git a/xen/drivers/passthrough/arm/vsmmu-v3.c b/xen/drivers/passthrough/arm/vsmmu-v3.c
> index 3f557d1c4d..87342fdf69 100644
> --- a/xen/drivers/passthrough/arm/vsmmu-v3.c
> +++ b/xen/drivers/passthrough/arm/vsmmu-v3.c
> @@ -85,6 +85,35 @@
>   #define smmu_get_ste_s1ctxptr(x)    FIELD_PREP(STRTAB_STE_0_S1CTXPTR_MASK, \
>                                       FIELD_GET(STRTAB_STE_0_S1CTXPTR_MASK, x))
>   
> +/* event queue entry */
> +struct arm_smmu_evtq_ent {
> +    /* Common fields */
> +    uint8_t     opcode;
> +    uint32_t    sid;
> +
> +    /* Event-specific fields */
> +    union {
> +        struct {
> +            uint32_t ssid;
> +            bool ssv;
> +        } c_bad_ste_streamid;
> +
> +        struct {
> +            bool stall;
> +            uint16_t stag;
> +            uint32_t ssid;
> +            bool ssv;
> +            bool s2;
> +            uint64_t addr;
> +            bool rnw;
> +            bool pnu;
> +            bool ind;
> +            uint8_t class;
> +            uint64_t addr2;
> +        } f_translation;
> +    };
> +};
> +
>   /* stage-1 translation configuration */
>   struct arm_vsmmu_s1_trans_cfg {
>       paddr_t s1ctxptr;
> @@ -115,6 +144,7 @@ struct virt_smmu {
>       uint32_t    strtab_base_cfg;
>       uint64_t    strtab_base;
>       uint32_t    irq_ctrl;
> +    uint32_t    virq;
>       uint64_t    gerror_irq_cfg0;
>       uint64_t    evtq_irq_cfg0;
>       struct      arm_vsmmu_queue evtq, cmdq;
> @@ -160,9 +190,15 @@ static inline bool smmu_get_evtq_enabled(struct virt_smmu *smmu)
>       spin_unlock(&smmu->cr0_lock);
>   
>       return enabled;
> -};
> +}
>   
>   /* Queue manipulation functions */
> +static bool queue_full(struct arm_vsmmu_queue *q)
> +{
> +    return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
> +           Q_WRP(q, q->prod) != Q_WRP(q, q->cons);
> +}
> +
>   static bool queue_empty(struct arm_vsmmu_queue *q)
>   {
>       return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
> @@ -175,12 +211,133 @@ static void queue_inc_cons(struct arm_vsmmu_queue *q)
>       q->cons = Q_OVF(q->cons) | Q_WRP(q, cons) | Q_IDX(q, cons);
>   }
>   
> +static void queue_inc_prod(struct arm_vsmmu_queue *q)
> +{
> +    u32 prod = (Q_WRP(q, q->prod) | Q_IDX(q, q->prod)) + 1;
> +    q->prod = Q_OVF(q->prod) | Q_WRP(q, prod) | Q_IDX(q, prod);
> +}
> +
>   static void dump_smmu_command(uint64_t *command)
>   {
>       gprintk(XENLOG_ERR, "cmd 0x%02llx: %016"PRIx64" %016"PRIx64"\n",
>                smmu_cmd_get_command(command[0]), command[0], command[1]);
>   }
>   
> +static void arm_vsmmu_update_irq(struct virt_smmu *smmu)
> +{
> +    bool evt_pending = false;
> +    bool gerr_pending = false;
> +    uint32_t irq_ctrl;
> +
> +    spin_lock(&smmu->irq_cfg_lock);
> +    irq_ctrl = smmu->irq_ctrl;
> +    spin_unlock(&smmu->irq_cfg_lock);
> +
> +    spin_lock(&smmu->evt_queue_lock);
> +    evt_pending = !queue_empty(&smmu->evtq);
> +    spin_unlock(&smmu->evt_queue_lock);
> +
> +    spin_lock(&smmu->gerror_lock);
> +    gerr_pending = (smmu->gerror ^ smmu->gerrorn) != 0;
> +    spin_unlock(&smmu->gerror_lock);
> +
> +    vgic_inject_irq(smmu->d, NULL, smmu->virq,
> +                    ((irq_ctrl & IRQ_CTRL_EVTQ_IRQEN) && evt_pending) ||
> +                    ((irq_ctrl & IRQ_CTRL_GERROR_IRQEN) && gerr_pending));
> +}
> +
> +static void arm_vsmmu_set_gerror(struct virt_smmu *smmu, uint32_t gerror_err)
> +{
> +    uint32_t pending, new_gerrors;
> +
> +    spin_lock(&smmu->gerror_lock);
> +
> +    pending = smmu->gerror ^ smmu->gerrorn;
> +    new_gerrors = ~pending & gerror_err;
> +
> +    if ( new_gerrors )
> +        smmu->gerror ^= new_gerrors;
> +
> +    spin_unlock(&smmu->gerror_lock);
> +
> +    arm_vsmmu_update_irq(smmu);
> +}
> +
> +static int arm_vsmmu_write_evtq(struct virt_smmu *smmu, uint64_t *evt)
> +{
> +    struct arm_vsmmu_queue *q = &smmu->evtq;
> +    struct domain *d = smmu->d;
> +    paddr_t addr;
> +    int ret;
> +
> +    if ( !smmu_get_evtq_enabled(smmu) )
> +        return -EINVAL;

If the queue is disabled, the event is simply discarded and nothing is 
wrong. Why do we then raise GERROR.EVTQ_ABT_ERR via arm_vsmmu_set_gerror()?


> +
> +    spin_lock(&smmu->evt_queue_lock);
> +
> +    if ( queue_full(q) )
> +    {
> +        spin_unlock(&smmu->evt_queue_lock);
> +        return -EINVAL;

This does not look like a global error either. From what I got from the 
SMMU spec (ARM IHI 0070/H.a 7.4 Event queue overflow), when the queue is 
full the event should be discarded and the overflow flag should be set 
in EVTQ_PROD.

"The Event queue overflow condition is present when:
SMMU_EVENTQ_PROD.OVFLG != SMMU_EVENTQ_CONS.OVACKFLG
When an overflow occurs, the SMMU toggles the SMMU_EVENTQ_PROD.OVFLG 
flag if the overflow condition is not already present."

Otherwise the guest might show a misleading message (an event queue 
memory-write error instead of an overflow).

Or I really missed something?

> +    }
> +
> +    addr = Q_PROD_ENT(q);
> +
> +    ret = access_guest_memory_by_gpa(d, addr, evt,
> +                                     sizeof(*evt) * EVTQ_ENT_DWORDS, true);
> +    if ( ret )
> +    {
> +        spin_unlock(&smmu->evt_queue_lock);
> +        return ret;
> +    }
> +
> +    queue_inc_prod(q);
> +
> +    spin_unlock(&smmu->evt_queue_lock);
> +
> +    arm_vsmmu_update_irq(smmu);
> +
> +    return 0;
> +}
> +
> +static void arm_vsmmu_send_event(struct virt_smmu *smmu,
> +                                 struct arm_smmu_evtq_ent *ent)
> +{
> +    uint64_t evt[EVTQ_ENT_DWORDS];
> +    int ret;
> +
> +    memset(evt, 0, 1 << EVTQ_ENT_SZ_SHIFT);
> +
> +    if ( !smmu_get_evtq_enabled(smmu) )
> +        return;
> +
> +    evt[0] |= FIELD_PREP(EVTQ_0_ID, ent->opcode);
> +    evt[0] |= FIELD_PREP(EVTQ_0_SID, ent->sid);
> +
> +    switch ( ent->opcode )
> +    {
> +    case EVT_ID_BAD_STREAMID:
> +    case EVT_ID_BAD_STE:
> +        evt[0] |= FIELD_PREP(EVTQ_0_SSID, ent->c_bad_ste_streamid.ssid);
> +        evt[0] |= FIELD_PREP(EVTQ_0_SSV, ent->c_bad_ste_streamid.ssv);
> +        break;
> +    case EVT_ID_TRANSLATION_FAULT:
> +    case EVT_ID_ADDR_SIZE_FAULT:
> +    case EVT_ID_ACCESS_FAULT:
> +    case EVT_ID_PERMISSION_FAULT:
> +        break;
> +    default:
> +        gdprintk(XENLOG_WARNING, "vSMMUv3: event opcode is bad\n");
> +        break;
> +    }
> +
> +    ret = arm_vsmmu_write_evtq(smmu, evt);
> +    if ( ret )
> +        arm_vsmmu_set_gerror(smmu, GERROR_EVTQ_ABT_ERR);
> +
> +    return;
> +}
> +
>   static int arm_vsmmu_find_ste(struct virt_smmu *smmu, uint32_t sid,
>                                 uint64_t *ste)
>   {
> @@ -189,6 +346,14 @@ static int arm_vsmmu_find_ste(struct virt_smmu *smmu, uint32_t sid,
>       uint32_t log2size;
>       int strtab_size_shift;
>       int ret;
> +    struct arm_smmu_evtq_ent ent = {
> +        .opcode = EVT_ID_BAD_STREAMID,
> +        .sid = sid,
> +        .c_bad_ste_streamid = {
> +            .ssid = 0,
> +            .ssv = false,
> +        },
> +    };
>   
>       spin_lock(&smmu->strtab_cfg_lock);
>       log2size = MIN(FIELD_GET(STRTAB_BASE_CFG_LOG2SIZE, smmu->strtab_base_cfg),
> @@ -196,7 +361,10 @@ static int arm_vsmmu_find_ste(struct virt_smmu *smmu, uint32_t sid,
>       spin_unlock(&smmu->strtab_cfg_lock);
>   
>       if ( sid >= (1U << log2size) )
> +    {
> +        arm_vsmmu_send_event(smmu, &ent);
>           return -EINVAL;
> +    }
>   
>       spin_lock(&smmu->strtab_cfg_lock);
>       if ( FIELD_GET(STRTAB_BASE_CFG_FMT, smmu->strtab_base_cfg) ==
> @@ -249,6 +417,7 @@ static int arm_vsmmu_find_ste(struct virt_smmu *smmu, uint32_t sid,
>           {
>               gdprintk(XENLOG_ERR, "idx=%d > max_l2_ste=%d\n",
>                        idx, max_l2_ste);
> +            arm_vsmmu_send_event(smmu, &ent);
>               return -EINVAL;
>           }
>           addr = l2ptr + idx * sizeof(*ste) * STRTAB_STE_DWORDS;
> @@ -277,6 +446,14 @@ static int arm_vsmmu_decode_ste(struct virt_smmu *smmu, uint32_t sid,
>                                   uint64_t *ste)
>   {
>       uint64_t val = ste[0];
> +    struct arm_smmu_evtq_ent ent = {
> +        .opcode = EVT_ID_BAD_STE,
> +        .sid = sid,
> +        .c_bad_ste_streamid = {
> +            .ssid = 0,
> +            .ssv = false,
> +        },
> +    };
>   
>       if ( !(val & STRTAB_STE_0_V) )
>           return -EAGAIN;
> @@ -312,6 +489,7 @@ static int arm_vsmmu_decode_ste(struct virt_smmu *smmu, uint32_t sid,
>       return 0;
>   
>    bad_ste:
> +    arm_vsmmu_send_event(smmu, &ent);
>       return -EINVAL;
>   }
>   
> @@ -362,16 +540,19 @@ static int arm_vsmmu_handle_cmds(struct virt_smmu *smmu)
>       if ( !smmu_get_cmdq_enabled(smmu) )
>           return 0;
>   
> -    while ( !queue_empty(q) )
> +    while ( !queue_empty(q) &&
> +          ((smmu->gerror ^ smmu->gerrorn) & (GERROR_CMDQ_ERR)) == 0 )
>       {
> -        int ret;
> -
>           addr = Q_CONS_ENT(q);
>           ret = access_guest_memory_by_gpa(d, addr, command,
>                                            sizeof(command), false);
> -        if ( ret ) {
> -            queue_inc_cons(q);
> -            return ret;
> +        if ( ret )
> +        {
> +            smmu->cmdq.cons &= ~CMDQ_CONS_ERR;
> +            smmu->cmdq.cons |= FIELD_PREP(CMDQ_CONS_ERR,
> +                                        CMDQ_ERR_CERROR_ABT_IDX);
> +            arm_vsmmu_set_gerror(smmu, GERROR_CMDQ_ERR);
> +            break;
>           }
>   
>           switch ( smmu_cmd_get_command(command[0]) )
> @@ -395,6 +576,10 @@ static int arm_vsmmu_handle_cmds(struct virt_smmu *smmu)
>           default:
>               gdprintk(XENLOG_ERR, "vSMMUv3: unhandled command\n");
>               dump_smmu_command(command);
> +            smmu->cmdq.cons &= ~CMDQ_CONS_ERR;
> +            smmu->cmdq.cons |= FIELD_PREP(CMDQ_CONS_ERR,
> +                                          CMDQ_ERR_CERROR_ILL_IDX);
> +            arm_vsmmu_set_gerror(smmu, GERROR_CMDQ_ERR);
>               break;
>           }
>   
> @@ -405,6 +590,15 @@ static int arm_vsmmu_handle_cmds(struct virt_smmu *smmu)
>                        ret);
>               dump_smmu_command(command);
>           }
> +
> +        /*
> +        * Stop consuming the command queue if a command error occurred.
> +        * CMDQ_CONS continues to point at the offending command until the
> +        * error is acknowledged.
> +        */
> +        if ( (smmu->gerror ^ smmu->gerrorn) & GERROR_CMDQ_ERR )
> +            break;
> +
>           queue_inc_cons(q);
>       }
>   
> @@ -443,6 +637,7 @@ static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
>               vreg_reg32_update(&reg32, r, info);
>               smmu->evtq.cons = reg32;
>               spin_unlock(&smmu->evt_queue_lock);
> +            arm_vsmmu_update_irq(smmu);
>               break;
>           default:
>               printk(XENLOG_G_ERR
> @@ -573,6 +768,7 @@ static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
>               vreg_reg32_update(&reg32, r, info);
>               smmu->irq_ctrl = reg32;
>               spin_unlock(&smmu->irq_cfg_lock);
> +            arm_vsmmu_update_irq(smmu);
>               break;
>   
>           case VREG64(ARM_SMMU_GERROR_IRQ_CFG0):
> @@ -602,6 +798,10 @@ static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
>               vreg_reg32_update(&reg32, r, info);
>               smmu->gerrorn = reg32;
>               spin_unlock(&smmu->gerror_lock);
> +            arm_vsmmu_update_irq(smmu);
> +            spin_lock(&smmu->cmd_queue_lock);
> +            arm_vsmmu_handle_cmds(smmu);
> +            spin_unlock(&smmu->cmd_queue_lock);
>               break;
>   
>           default:
> @@ -905,9 +1105,10 @@ static const struct mmio_handler_ops vsmmuv3_mmio_handler = {
>       .write = vsmmuv3_mmio_write,
>   };
>   
> -static int vsmmuv3_init_single(struct domain *d, paddr_t addr, paddr_t size,
> -                               uint32_t features)
> +static int vsmmuv3_init_single(struct domain *d, paddr_t addr,
> +                               paddr_t size, uint32_t virq, uint32_t features)
>   {
> +    int ret;
>       struct virt_smmu *smmu;
>   
>       smmu = xzalloc(struct virt_smmu);
> @@ -916,6 +1117,7 @@ static int vsmmuv3_init_single(struct domain *d, paddr_t addr, paddr_t size,
>   
>       smmu->d = d;
>       smmu->features = features;
> +    smmu->virq = virq;
>       smmu->cmdq.ent_size = CMDQ_ENT_DWORDS * DWORDS_BYTES;
>       smmu->evtq.ent_size = EVTQ_ENT_DWORDS * DWORDS_BYTES;
>   
> @@ -928,6 +1130,15 @@ static int vsmmuv3_init_single(struct domain *d, paddr_t addr, paddr_t size,
>       spin_lock_init(&smmu->strtab_cfg_lock);
>       spin_lock_init(&smmu->irq_cfg_lock);
>   
> +    ret = vgic_reserve_virq(d, virq);
> +    if ( !ret )
> +    {
> +        printk(XENLOG_G_ERR
> +                 "vSMMUv3: Failed to reserve domain vIRQ %u\n", virq);
> +        xfree(smmu);
> +        return -EINVAL;
> +    }
> +
>       register_mmio_handler(d, &vsmmuv3_mmio_handler, addr, size, smmu);
>   
>       /* Register the vIOMMU to be able to clean it up later. */
> @@ -953,7 +1164,7 @@ static int domain_vsmmuv3_init(struct domain *d)
>           {
>               smmu = (const struct arm_smmu_device *) hw_iommu->iommu;
>               ret = vsmmuv3_init_single(d, hw_iommu->addr, hw_iommu->size,
> -                                      smmu->features);
> +                                      hw_iommu->irq, smmu->features);
>               if ( ret )
>                   return ret;
>           }
> @@ -983,7 +1194,7 @@ static int domain_vsmmuv3_init(struct domain *d)
>           }
>   
>           ret = vsmmuv3_init_single(d, GUEST_VSMMUV3_BASE, GUEST_VSMMUV3_SIZE,
> -                                  features);
> +                                  GUEST_VSMMU_SPI, features);
>           if ( ret )
>               return ret;
>       }
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 8863e2e457..c43b8b7b71 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -538,6 +538,8 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_VIRTIO_MMIO_SPI_FIRST   33
>   #define GUEST_VIRTIO_MMIO_SPI_LAST    43
>   
> +#define GUEST_VSMMU_SPI         44

So this is the same emulated SPI as the one used for vPL011. You likely 
need to update d_cfg->arch.nr_spis if vSMMU is enabled for the domain, 
to ensure that we allocate enough SPIs for it (refer to 
arch_parse_dom0less_node(), when "nr_spis" is not explicitly specified
we update nr_spis if vPL011 is requested). Moreover, you already do this 
in libxl.


> +
>   /*
>    * SGI is the preferred delivery mechanism of FF-A pending notifications or
>    * schedule recveive interrupt. SGIs 8-15 are normally not used by a guest