Re: [RFC v5 18/28] hw/arm/smmuv3: Add access checks for CMDQ and EVENTQ registers

[Eric Auger <eric.auger@redhat.com>]

On 8/13/26 6:25 PM, Tao Tang wrote:
> Add access control for command queue and event queue related registers
> to ensure they can only be modified under proper conditions.

please explain what is wrong first
>
> For command queue (CMDQ):
> - smmu_cmdq_disabled_stable(): checks CMDQ bit in CR0/CR0ACK
always assume CR0=CR0ACK?
> - smmu_cmdq_base_writable(): checks IDR1.QUEUES_PRESET==0 and CMDQ disabled
>
> For event queue (EVTQ):
> - smmu_eventq_disabled_stable(): checks EVTQ bit in CR0/CR0ACK
> - smmu_eventq_base_writable(): checks IDR1.QUEUES_PRESET==0 and EVTQ disabled
> - smmu_irq_cfg_present(): checks SMMU_IDR0.MSI for the Non-secure
>   EVENTQ_IRQ_CFG register set and SMMU_S_IDR0.MSI for the Secure set
can't you simply assume MSI is not supported?
> - smmu_eventq_irq_cfg_writable(): additionally checks EVENTQ_IRQEN
>
> Absent EVENTQ_IRQ_CFG registers read as RES0. Present registers retain
> their programmed values for readback. The helper remains keyed by
> SEC_SID so future Realm support can select SMMU_R_IDR0.MSI.
>
> Additionally, mask reserved bits on writes using SMMU_QUEUE_BASE_RESERVED
> for queue base registers and SMMU_EVENTQ_IRQ_CFG0_RESERVED for
> EVENTQ_IRQ_CFG0.
>
> Fixes: fae4be38b35d ("hw/arm/smmuv3: Implement MMIO write operations")
> Signed-off-by: Tao Tang <tangtao1634@phytium.com.cn>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  hw/arm/smmuv3.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++--
210 changes lines -> Can't you split into 2a ll the more so the handling
is different from CMDQ and EVENTQ
>  1 file changed, 204 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index de8ca7cda48..efa78869251 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1432,19 +1432,29 @@ smmu_cmdq_stage2_supported(SMMUv3State *s, SMMUSecSID sec_sid)
>  static bool smmu_irq_cfg_present(SMMUv3State *s, SMMUSecSID sec_sid,
>                                   SMMUIrq irq)
>  {
> -    SMMUv3RegBank *bank = smmuv3_bank(s, SMMU_SEC_SID_NS);
> +    SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> +    SMMUv3RegBank *ns_bank = smmuv3_bank(s, SMMU_SEC_SID_NS);
>  
>      switch (irq) {
>      case SMMU_IRQ_GERROR:
>          switch (sec_sid) {
>          case SMMU_SEC_SID_NS:
>          case SMMU_SEC_SID_S:
> -            return FIELD_EX32(bank->idr[0], IDR0, MSI);
> +            return FIELD_EX32(ns_bank->idr[0], IDR0, MSI);
>          case SMMU_SEC_SID_NUM:
>              g_assert_not_reached();
>          }
>          break;
>      case SMMU_IRQ_EVTQ:
> +        switch (sec_sid) {
> +        case SMMU_SEC_SID_NS:
> +            return FIELD_EX32(bank->idr[0], IDR0, MSI);
> +        case SMMU_SEC_SID_S:
> +            return FIELD_EX32(bank->idr[0], S_IDR0, MSI);
> +        case SMMU_SEC_SID_NUM:
> +            g_assert_not_reached();
> +        }
> +        break;
>      case SMMU_IRQ_PRIQ:
>      case SMMU_IRQ_CMD_SYNC:
>          g_assert_not_reached();
> @@ -1469,6 +1479,8 @@ static bool smmu_irq_cfg_writable(SMMUv3State *s, SMMUSecSID sec_sid,
>          irqen = FIELD_EX32(bank->irq_ctrl, IRQ_CTRL, GERROR_IRQEN);
>          break;
>      case SMMU_IRQ_EVTQ:
> +        irqen = FIELD_EX32(bank->irq_ctrl, IRQ_CTRL, EVENTQ_IRQEN);
> +        break;
>      case SMMU_IRQ_PRIQ:
>      case SMMU_IRQ_CMD_SYNC:
>          g_assert_not_reached();
> @@ -1514,6 +1526,69 @@ static bool smmu_strtab_base_writable(SMMUv3State *s, SMMUSecSID sec_sid)
>      return smmuv3_smmu_disabled_stable(s, sec_sid);
>  }
>  
> +static inline int smmuv3_get_cr0_cmdqen(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    return FIELD_EX32(s->bank[sec_sid].cr[0], CR0, CMDQEN);
> +}
> +
> +static inline int smmuv3_get_cr0ack_cmdqen(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    return FIELD_EX32(s->bank[sec_sid].cr0ack, CR0, CMDQEN);
> +}
> +
> +static inline int smmuv3_get_cr0_eventqen(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    return FIELD_EX32(s->bank[sec_sid].cr[0], CR0, EVENTQEN);
> +}
> +
> +static inline int smmuv3_get_cr0ack_eventqen(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    return FIELD_EX32(s->bank[sec_sid].cr0ack, CR0, EVENTQEN);
> +}
> +
> +/* Check if CMDQ is disabled in stable status */
> +static bool smmu_cmdq_disabled_stable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    int cr0_cmdqen = smmuv3_get_cr0_cmdqen(s, sec_sid);
> +    int cr0ack_cmdqen = smmuv3_get_cr0ack_cmdqen(s, sec_sid);
> +    return (cr0_cmdqen == 0 && cr0ack_cmdqen == 0);
> +}
> +
> +/* Check if CMDQ_BASE register is writable */
> +static bool smmu_cmdq_base_writable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    /* SMMU_IDR1.QUEUES_PRESET applies to both modeled interfaces. */
> +    if (FIELD_EX32(s->bank[SMMU_SEC_SID_NS].idr[1], IDR1, QUEUES_PRESET)) {
> +        return false;
> +    }
> +
> +    return smmu_cmdq_disabled_stable(s, sec_sid);
> +}
> +
> +/* Check if EVENTQ is disabled in stable status */
> +static bool smmu_eventq_disabled_stable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    int cr0_eventqen = smmuv3_get_cr0_eventqen(s, sec_sid);
> +    int cr0ack_eventqen = smmuv3_get_cr0ack_eventqen(s, sec_sid);
> +    return (cr0_eventqen == 0 && cr0ack_eventqen == 0);
> +}
> +
> +/* Check if EVENTQ_BASE register is writable */
> +static bool smmu_eventq_base_writable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    if (FIELD_EX32(s->bank[SMMU_SEC_SID_NS].idr[1], IDR1, QUEUES_PRESET)) {
> +        return false;
> +    }
> +
> +    return smmu_eventq_disabled_stable(s, sec_sid);
> +}
> +
> +/* Check if EVENTQ_IRQ_CFGx is writable */
> +static bool smmu_eventq_irq_cfg_writable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> +    return smmu_irq_cfg_writable(s, sec_sid, SMMU_IRQ_EVTQ);
> +}
> +
>  static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp, SMMUSecSID sec_sid)
>  {
>      SMMUState *bs = ARM_SMMU(s);
> @@ -1841,21 +1916,39 @@ static MemTxResult smmu_writell(SMMUv3State *s, hwaddr offset,
>          bank->strtab_base = data & SMMU_STRTAB_BASE_RESERVED;
>          return MEMTX_OK;
>      case A_CMDQ_BASE:
> -        bank->cmdq.base = data;
> +        if (!smmu_cmdq_base_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "CMDQ_BASE write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        bank->cmdq.base = data & SMMU_QUEUE_BASE_RESERVED;
>          bank->cmdq.log2size = extract64(bank->cmdq.base, 0, 5);
>          if (bank->cmdq.log2size > SMMU_CMDQS) {
>              bank->cmdq.log2size = SMMU_CMDQS;
>          }
>          return MEMTX_OK;
>      case A_EVENTQ_BASE:
> -        bank->eventq.base = data;
> +        if (!smmu_eventq_base_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_BASE write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        bank->eventq.base = data & SMMU_QUEUE_BASE_RESERVED;
>          bank->eventq.log2size = extract64(bank->eventq.base, 0, 5);
>          if (bank->eventq.log2size > SMMU_EVENTQS) {
>              bank->eventq.log2size = SMMU_EVENTQS;
>          }
>          return MEMTX_OK;
>      case A_EVENTQ_IRQ_CFG0:
> -        bank->eventq_irq_cfg0 = data;
> +        if (!smmu_eventq_irq_cfg_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_IRQ_CFG0 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        bank->eventq_irq_cfg0 = data & SMMU_EVENTQ_IRQ_CFG0_RESERVED;
>          return MEMTX_OK;
>      default:
>          qemu_log_mask(LOG_UNIMP,
> @@ -2001,6 +2094,13 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
>          }
>          break;
>      case A_CMDQ_BASE: /* 64b */
> +        if (!smmu_cmdq_base_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "CMDQ_BASE write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        data &= SMMU_QUEUE_BASE_RESERVED;
>          bank->cmdq.base = deposit64(bank->cmdq.base, 0, 32, data);
>          bank->cmdq.log2size = extract64(bank->cmdq.base, 0, 5);
>          if (bank->cmdq.log2size > SMMU_CMDQS) {
> @@ -2008,6 +2108,13 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
>          }
>          break;
>      case A_CMDQ_BASE + 4: /* 64b */
> +        if (!smmu_cmdq_base_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "CMDQ_BASE + 4 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        data &= SMMU_QUEUE_BASE_RESERVED;
>          bank->cmdq.base = deposit64(bank->cmdq.base, 32, 32, data);
>          break;
>      case A_CMDQ_PROD:
> @@ -2015,9 +2122,22 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
>          smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
>          break;
>      case A_CMDQ_CONS:
> +        if (!smmu_cmdq_disabled_stable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "CMDQ_CONS write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
>          bank->cmdq.cons = data;
>          break;
>      case A_EVENTQ_BASE: /* 64b */
> +        if (!smmu_eventq_base_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_BASE write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        data &= SMMU_QUEUE_BASE_RESERVED;
>          bank->eventq.base = deposit64(bank->eventq.base, 0, 32, data);
>          bank->eventq.log2size = extract64(bank->eventq.base, 0, 5);
>          if (bank->eventq.log2size > SMMU_EVENTQS) {
> @@ -2025,24 +2145,63 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
>          }
>          break;
>      case A_EVENTQ_BASE + 4:
> +        if (!smmu_eventq_base_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_BASE + 4 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        data &= SMMU_QUEUE_BASE_RESERVED;
>          bank->eventq.base = deposit64(bank->eventq.base, 32, 32, data);
>          break;
>      case A_EVENTQ_PROD:
> +        if (!smmu_eventq_disabled_stable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_PROD write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
>          bank->eventq.prod = data;
>          break;
>      case A_EVENTQ_CONS:
>          bank->eventq.cons = data;
>          break;
>      case A_EVENTQ_IRQ_CFG0: /* 64b */
> +        if (!smmu_eventq_irq_cfg_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_IRQ_CFG0 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        data &= SMMU_EVENTQ_IRQ_CFG0_RESERVED;
>          bank->eventq_irq_cfg0 = deposit64(bank->eventq_irq_cfg0, 0, 32, data);
>          break;
>      case A_EVENTQ_IRQ_CFG0 + 4:
> +        if (!smmu_eventq_irq_cfg_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_IRQ_CFG0+4 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
> +        data &= SMMU_EVENTQ_IRQ_CFG0_RESERVED >> 32;
>          bank->eventq_irq_cfg0 = deposit64(bank->eventq_irq_cfg0, 32, 32, data);
>          break;
>      case A_EVENTQ_IRQ_CFG1:
> +        if (!smmu_eventq_irq_cfg_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_IRQ_CFG1 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
>          bank->eventq_irq_cfg1 = data;
>          break;
>      case A_EVENTQ_IRQ_CFG2:
> +        if (!smmu_eventq_irq_cfg_writable(s, reg_sec_sid)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "EVENTQ_IRQ_CFG2 write ignored: register is RO\n");
> +            return MEMTX_OK;
> +        }
> +
>          bank->eventq_irq_cfg2 = data;
>          break;
>      default:
> @@ -2108,6 +2267,14 @@ static MemTxResult smmu_readll(SMMUv3State *s, hwaddr offset,
>      case A_EVENTQ_BASE:
>          *data = bank->eventq.base;
>          return MEMTX_OK;
> +    case A_EVENTQ_IRQ_CFG0:
> +        if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_EVTQ)) {
> +            *data = 0; /* RES0 */
> +            return MEMTX_OK;
> +        }
> +
> +        *data = bank->eventq_irq_cfg0;
> +        return MEMTX_OK;
>      default:
>          *data = 0;
>          qemu_log_mask(LOG_UNIMP,
> @@ -2229,6 +2396,38 @@ static MemTxResult smmu_readl(SMMUv3State *s, hwaddr offset,
>      case A_EVENTQ_CONS:
>          *data = bank->eventq.cons;
>          return MEMTX_OK;
> +    case A_EVENTQ_IRQ_CFG0: /* 64b */
> +        if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_EVTQ)) {
> +            *data = 0; /* RES0 */
> +            return MEMTX_OK;
> +        }
> +
> +        *data = extract64(bank->eventq_irq_cfg0, 0, 32);
> +        return MEMTX_OK;
> +    case A_EVENTQ_IRQ_CFG0 + 4:
> +        if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_EVTQ)) {
> +            *data = 0; /* RES0 */
> +            return MEMTX_OK;
> +        }
> +
> +        *data = extract64(bank->eventq_irq_cfg0, 32, 32);
> +        return MEMTX_OK;
> +    case A_EVENTQ_IRQ_CFG1:
> +        if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_EVTQ)) {
> +            *data = 0; /* RES0 */
> +            return MEMTX_OK;
> +        }
> +
> +        *data = bank->eventq_irq_cfg1;
> +        return MEMTX_OK;
> +    case A_EVENTQ_IRQ_CFG2:
> +        if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_EVTQ)) {
> +            *data = 0; /* RES0 */
> +            return MEMTX_OK;
> +        }
> +
> +        *data = bank->eventq_irq_cfg2;
> +        return MEMTX_OK;
>      default:
>          *data = 0;
>          qemu_log_mask(LOG_UNIMP,
> @@ -2610,4 +2809,3 @@ static void smmuv3_register_types(void)
>  }
>  
>  type_init(smmuv3_register_types)
> -
Thanks

Eric