Hi Tao,
On 8/13/26 6:25 PM, Tao Tang wrote:
Please directly explain what is the problem we want to fix before what
is actually done in the patch.
Spec says: "SMMU_GERROR_IRQ_CFG0 is Guarded by
SMMU_IRQ_CTRL.GERROR_IRQEN and must only be modified
when SMMU_IRQ_CTRL.GERROR_IRQEN == 0." This check is currently not
performed
In SMMUv3.2 and later, a write while SMMU_IRQ_CTRL.GERROR_IRQEN == 1 is
IGNORED.
Accesses to this register use the following encodings:
Accessible at offset 0x0068 from SMMUv3_PAGE_0
• When SMMU_IRQ_CTRL.GERROR_IRQEN == 0 and SMMU_IRQ_CTRLACK.GERROR_IRQEN
== 0,
accesses to this register are RW.
• Otherwise, accesses to this register are RO.
"
By the way you are not checking SMMU_IRQ_CTRLACK.GERROR_IRQEN
Nevertheless our emulation does not support MSI, right? So why don't we
hardcode this behavior on SMMU_GERROR_IRQ_CFG0 access, ie.
"This register is present only when SMMU_IDR0.MSI == 1.
SMMU_GERROR_IRQ_CFG0 are RES 0."
That would be simpler.
> Add helpers that separate IRQ_CFG register presence from the runtime
> IRQ enable guard. The Non-secure and Secure GERROR_IRQ_CFG register sets
> both use SMMU_IDR0.MSI for presence, while writability additionally
> requires GERROR_IRQEN to be clear. IRQ_CTRL and IRQ_CTRLACK share one
> synchronous backing field in the current model.
>
> Reads return RES0 when the register set is absent. Writes are ignored
> when it is absent or GERROR_IRQEN is set. Apply the same checks to all
> three GERROR_IRQ_CFG registers and mask reserved bits in CFG0 and CFG2.
>
> Fixes: fae4be38b35d ("hw/arm/smmuv3: Implement MMIO write operations")
above is enought I think
> Fixes: 10a83cb9887e ("hw/arm/smmuv3: Skeleton")
> Signed-off-by: Tao Tang <[email protected]>
> Reviewed-by: Pierrick Bouvier <[email protected]>
> ---
> hw/arm/smmuv3.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 116 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 47f0d575817..628911d3f21 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1428,6 +1428,62 @@ smmu_cmdq_stage2_supported(SMMUv3State *s, SMMUSecSID
> sec_sid)
> return true;
> }
>
> +/* Check whether the selected IRQ_CFG register set is present. */
Remove the final "."
Besides worth to add, check if MSI are supported. Even I would rather
rename the helper to reflect what is pratcically does, even if your
eventual interest is to know if SMMU_PRIQ_IRQ_CFGn and
SMMU_GERROR_IRQ_CFGn are not RES 0. By the way IRQ_CFG is always present
;-)
> +static bool smmu_irq_cfg_present(SMMUv3State *s, SMMUSecSID sec_sid,
> + SMMUIrq irq)
> +{
> + SMMUv3RegBank *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);
> + case SMMU_SEC_SID_NUM:
> + g_assert_not_reached();
> + }
> + break;
> + case SMMU_IRQ_EVTQ:
why do you pass irq if everything besides
SMMU_IRQ_GERROR is unexpected?
> + case SMMU_IRQ_PRIQ:
> + case SMMU_IRQ_CMD_SYNC:
> + g_assert_not_reached();
> + }
> +
> + g_assert_not_reached();
> +}
> +
> +/* Check whether the selected IRQ_CFG register set is writable. */
> +static bool smmu_irq_cfg_writable(SMMUv3State *s, SMMUSecSID sec_sid,
> + SMMUIrq irq)
> +{
> + SMMUv3RegBank *bank = smmuv3_bank(s, sec_sid);
> + uint32_t irqen;
> +
> + if (!smmu_irq_cfg_present(s, sec_sid, irq)) {
> + return false;
> + }
> +
> + switch (irq) {
> + case SMMU_IRQ_GERROR:
> + irqen = FIELD_EX32(bank->irq_ctrl, IRQ_CTRL, GERROR_IRQEN);
again the name of the helper does not reflect what it does.
GERROR_IRQENMeaning
0b0 interrupts are disabled.
0b1 Interrupts are enabled.
> + break;
> + case SMMU_IRQ_EVTQ:
> + case SMMU_IRQ_PRIQ:
> + case SMMU_IRQ_CMD_SYNC:
> + g_assert_not_reached();
> + }
> +
> + /* IRQ_CTRL and IRQ_CTRLACK share one synchronous backing field. */
> + return irqen == 0;
> +}
> +
> +static bool
> +smmu_gerror_irq_cfg_writable(SMMUv3State *s, SMMUSecSID sec_sid)
> +{
> + return smmu_irq_cfg_writable(s, sec_sid, SMMU_IRQ_GERROR);
> +}
> +
> static int smmuv3_cmdq_consume(SMMUv3State *s, Error **errp, SMMUSecSID
> sec_sid)
> {
> SMMUState *bs = ARM_SMMU(s);
> @@ -1735,7 +1791,14 @@ static MemTxResult smmu_writell(SMMUv3State *s, hwaddr
> offset,
>
> switch (offset) {
> case A_GERROR_IRQ_CFG0:
> - bank->gerror_irq_cfg0 = data;
> + if (!smmu_gerror_irq_cfg_writable(s, reg_sec_sid)) {
> + /* SMMU_(*_)_IRQ_CTRL.GERROR_IRQEN == 1: IGNORED this write */
> + qemu_log_mask(LOG_GUEST_ERROR, "GERROR_IRQ_CFG0 write ignored: "
> + "register is RO when IRQ enabled\n");
> + return MEMTX_OK;
> + }
> +
> + bank->gerror_irq_cfg0 = data & SMMU_GERROR_IRQ_CFG0_RESERVED;
> return MEMTX_OK;
> case A_STRTAB_BASE:
> bank->strtab_base = data;
> @@ -1803,16 +1866,42 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr
> offset,
> smmuv3_cmdq_consume(s, &local_err, reg_sec_sid);
> break;
> case A_GERROR_IRQ_CFG0: /* 64b */
> + if (!smmu_gerror_irq_cfg_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "GERROR_IRQ_CFG0 write ignored: "
> + "register is RO when IRQ enabled\n");
> + return MEMTX_OK;
> + }
> +
> + data &= SMMU_GERROR_IRQ_CFG0_RESERVED;
> bank->gerror_irq_cfg0 = deposit64(bank->gerror_irq_cfg0, 0, 32,
> data);
> break;
> case A_GERROR_IRQ_CFG0 + 4:
> + if (!smmu_gerror_irq_cfg_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "GERROR_IRQ_CFG0 + 4 write
> ignored: "
> + "register is RO when IRQ enabled\n");
> + return MEMTX_OK;
> + }
> +
> + data &= SMMU_GERROR_IRQ_CFG0_RESERVED >> 32;
> bank->gerror_irq_cfg0 = deposit64(bank->gerror_irq_cfg0, 32, 32,
> data);
> break;
> case A_GERROR_IRQ_CFG1:
> + if (!smmu_gerror_irq_cfg_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "GERROR_IRQ_CFG1 write ignored: "
> + "register is RO when IRQ enabled\n");
> + return MEMTX_OK;
> + }
> +
> bank->gerror_irq_cfg1 = data;
> break;
> case A_GERROR_IRQ_CFG2:
> - bank->gerror_irq_cfg2 = data;
> + if (!smmu_gerror_irq_cfg_writable(s, reg_sec_sid)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "GERROR_IRQ_CFG2 write ignored: "
> + "register is RO when IRQ enabled\n");
> + return MEMTX_OK;
> + }
> +
> + bank->gerror_irq_cfg2 = data & SMMU_GERROR_IRQ_CFG2_RESERVED;
> break;
> case A_GBPA:
> /*
> @@ -1938,6 +2027,11 @@ static MemTxResult smmu_readll(SMMUv3State *s, hwaddr
> offset,
>
> switch (offset) {
> case A_GERROR_IRQ_CFG0:
> + if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_GERROR)) {
> + *data = 0; /* RES0 */
> + return MEMTX_OK;
> + }
> +
> *data = bank->gerror_irq_cfg0;
> return MEMTX_OK;
> case A_STRTAB_BASE:
> @@ -2006,15 +2100,35 @@ static MemTxResult smmu_readl(SMMUv3State *s, hwaddr
> offset,
> *data = bank->gerrorn;
> return MEMTX_OK;
> case A_GERROR_IRQ_CFG0: /* 64b */
> + if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_GERROR)) {
> + *data = 0; /* RES0 */
> + return MEMTX_OK;
> + }
> +
> *data = extract64(bank->gerror_irq_cfg0, 0, 32);
> return MEMTX_OK;
> case A_GERROR_IRQ_CFG0 + 4:
> + if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_GERROR)) {
> + *data = 0; /* RES0 */
> + return MEMTX_OK;
> + }
> +
> *data = extract64(bank->gerror_irq_cfg0, 32, 32);
> return MEMTX_OK;
> case A_GERROR_IRQ_CFG1:
> + if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_GERROR)) {
> + *data = 0; /* RES0 */
> + return MEMTX_OK;
> + }
> +
> *data = bank->gerror_irq_cfg1;
> return MEMTX_OK;
> case A_GERROR_IRQ_CFG2:
> + if (!smmu_irq_cfg_present(s, reg_sec_sid, SMMU_IRQ_GERROR)) {
> + *data = 0; /* RES0 */
> + return MEMTX_OK;
> + }
> +
> *data = bank->gerror_irq_cfg2;
> return MEMTX_OK;
> case A_STRTAB_BASE: /* 64b */
Thanks
Eric
