[PATCH v4] hw/arm/smmuv3: Add GBPA register

[PATCH v4] hw/arm/smmuv3: Add GBPA register

[Mostafa Saleh]

GBPA register can be used to globally abort all
transactions.

It is described in the SMMU manual in "6.3.14 SMMU_GBPA".
ABORT reset value is IMPLEMENTATION DEFINED, it is chosen to
be zero(Do not abort incoming transactions).

Other fields have default values of Use Incoming.

If UPDATE is not set, the write is ignored. This is the only permitted
behavior in SMMUv3.2 and later.(6.3.14.1 Update procedure)

As this patch adds a new state to the SMMU (GBPA), it is added
in a new subsection for forward migration compatibility.
GBPA is only migrated if its value is different from the reset value.
It does this to be backward migration compatible if SW didn't write
the register.

Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v4:
- Migrate if GBPA is different from reset value, not only ABORT bit.

Changes in v3:
- Remove migrate_gbpa property as it was unnecessary.

Changes in v2:
- GBPA is effective only when SMMU is not enabled.
- Ignore GBPA write when UPDATE is not set.
- Default value for SHCFG is "Use Incoming".
- Support migration.
---
 hw/arm/smmuv3-internal.h |  7 ++++++
 hw/arm/smmuv3.c          | 47 +++++++++++++++++++++++++++++++++++++++-
 include/hw/arm/smmuv3.h  |  1 +
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index bce161870f..e8f0ebf25e 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -79,6 +79,13 @@ REG32(CR0ACK,              0x24)
 REG32(CR1,                 0x28)
 REG32(CR2,                 0x2c)
 REG32(STATUSR,             0x40)
+REG32(GBPA,                0x44)
+    FIELD(GBPA, ABORT,        20, 1)
+    FIELD(GBPA, UPDATE,       31, 1)
+
+/* Use incoming. */
+#define SMMU_GBPA_RESET_VAL 0x1000
+
 REG32(IRQ_CTRL,            0x50)
     FIELD(IRQ_CTRL, GERROR_IRQEN,        0, 1)
     FIELD(IRQ_CTRL, PRI_IRQEN,           1, 1)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 955b89c8d5..ddd37f233b 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -285,6 +285,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
     s->gerror = 0;
     s->gerrorn = 0;
     s->statusr = 0;
+    s->gbpa = SMMU_GBPA_RESET_VAL;
 }
 
 static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
@@ -659,7 +660,11 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
     qemu_mutex_lock(&s->mutex);
 
     if (!smmu_enabled(s)) {
-        status = SMMU_TRANS_DISABLE;
+        if (FIELD_EX32(s->gbpa, GBPA, ABORT)) {
+            status = SMMU_TRANS_ABORT;
+        } else {
+            status = SMMU_TRANS_DISABLE;
+        }
         goto epilogue;
     }
 
@@ -1170,6 +1175,16 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
     case A_GERROR_IRQ_CFG2:
         s->gerror_irq_cfg2 = data;
         return MEMTX_OK;
+    case A_GBPA:
+        /*
+         * If UPDATE is not set, the write is ignored. This is the only
+         * permitted behavior in SMMUv3.2 and later.
+         */
+        if (data & R_GBPA_UPDATE_MASK) {
+            /* Ignore update bit as write is synchronous. */
+            s->gbpa = data & ~R_GBPA_UPDATE_MASK;
+        }
+        return MEMTX_OK;
     case A_STRTAB_BASE: /* 64b */
         s->strtab_base = deposit64(s->strtab_base, 0, 32, data);
         return MEMTX_OK;
@@ -1318,6 +1333,9 @@ static MemTxResult smmu_readl(SMMUv3State *s, hwaddr offset,
     case A_STATUSR:
         *data = s->statusr;
         return MEMTX_OK;
+    case A_GBPA:
+        *data = s->gbpa;
+        return MEMTX_OK;
     case A_IRQ_CTRL:
     case A_IRQ_CTRL_ACK:
         *data = s->irq_ctrl;
@@ -1482,6 +1500,29 @@ static const VMStateDescription vmstate_smmuv3_queue = {
     },
 };
 
+static bool smmuv3_gbpa_needed(void *opaque)
+{
+    SMMUv3State *s = opaque;
+
+    /* Only migrate GBPA if it has different reset value. */
+    if (s->gbpa != SMMU_GBPA_RESET_VAL) {
+        return true;
+    }
+
+    return false;
+}
+
+static const VMStateDescription vmstate_gbpa = {
+    .name = "smmuv3/gbpa",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = smmuv3_gbpa_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(gbpa, SMMUv3State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_smmuv3 = {
     .name = "smmuv3",
     .version_id = 1,
@@ -1512,6 +1553,10 @@ static const VMStateDescription vmstate_smmuv3 = {
 
         VMSTATE_END_OF_LIST(),
     },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_gbpa,
+        NULL
+    }
 };
 
 static void smmuv3_instance_init(Object *obj)
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index f1921fdf9e..9899fa1860 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -46,6 +46,7 @@ struct SMMUv3State {
     uint32_t cr[3];
     uint32_t cr0ack;
     uint32_t statusr;
+    uint32_t gbpa;
     uint32_t irq_ctrl;
     uint32_t gerror;
     uint32_t gerrorn;
-- 
2.39.1.581.gbfd45094c4-goog

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Richard Henderson]

On 2/10/23 12:19, Mostafa Saleh wrote:
> +static bool smmuv3_gbpa_needed(void *opaque)
> +{
> +    SMMUv3State *s = opaque;
> +
> +    /* Only migrate GBPA if it has different reset value. */
> +    if (s->gbpa != SMMU_GBPA_RESET_VAL) {
> +        return true;
> +    }
> +
> +    return false;

Just "return s->gbpa != SMMU_GBPA_RESET_VAL;".

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Richard Henderson]

On 2/10/23 12:19, Mostafa Saleh wrote:
> @@ -1512,6 +1553,10 @@ static const VMStateDescription vmstate_smmuv3 = {
>   
>           VMSTATE_END_OF_LIST(),
>       },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_gbpa,
> +        NULL
> +    }

Actually, I suspect that you need a pre_load hook that resets gbpa, which will then be 
overwritten if and only if the saved value != reset value.


r~

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Mostafa Saleh]

On Sat, Feb 11, 2023 at 01:48:57PM -1000, Richard Henderson wrote:

> Just "return s->gbpa != SMMU_GBPA_RESET_VAL;".

I will update it.

> > @@ -1512,6 +1553,10 @@ static const VMStateDescription vmstate_smmuv3 = {
> >           VMSTATE_END_OF_LIST(),
> >       },
> > +    .subsections = (const VMStateDescription * []) {
> > +        &vmstate_gbpa,
> > +        NULL
> > +    }
> 
> Actually, I suspect that you need a pre_load hook that resets gbpa, which
> will then be overwritten if and only if the saved value != reset value.
> 

Please correct me if I am wrong. From what I see, the initial for value
GBPA will be set from smmu_reset_hold which is called from context of
qemu_system_reset from qemu_init context.
And migration will start after that in migration_incoming_process from
qemu_main_loop context.

I validated that also by printing the value of GBPA from vmstate_smmuv3
pre_load at migration without GPBA, and it is the same as
SMMU_GBPA_RESET_VAL.

Thanks,
Mostafa

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Richard Henderson]

On 2/12/23 03:23, Mostafa Saleh wrote:
> On Sat, Feb 11, 2023 at 01:48:57PM -1000, Richard Henderson wrote:
> 
>> Just "return s->gbpa != SMMU_GBPA_RESET_VAL;".
> 
> I will update it.
> 
>>> @@ -1512,6 +1553,10 @@ static const VMStateDescription vmstate_smmuv3 = {
>>>            VMSTATE_END_OF_LIST(),
>>>        },
>>> +    .subsections = (const VMStateDescription * []) {
>>> +        &vmstate_gbpa,
>>> +        NULL
>>> +    }
>>
>> Actually, I suspect that you need a pre_load hook that resets gbpa, which
>> will then be overwritten if and only if the saved value != reset value.
>>
> 
> Please correct me if I am wrong. From what I see, the initial for value
> GBPA will be set from smmu_reset_hold which is called from context of
> qemu_system_reset from qemu_init context.
> And migration will start after that in migration_incoming_process from
> qemu_main_loop context.
> 
> I validated that also by printing the value of GBPA from vmstate_smmuv3
> pre_load at migration without GPBA, and it is the same as
> SMMU_GBPA_RESET_VAL.

Is that from -loadvm on the command-line, or the loadvm command from the monitor?  It's 
the latter that I suspect requires the pre_load.


r~

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Mostafa Saleh]

On Sun, Feb 12, 2023 at 05:18:33AM -1000, Richard Henderson wrote:
> > Please correct me if I am wrong. From what I see, the initial for value
> > GBPA will be set from smmu_reset_hold which is called from context of
> > qemu_system_reset from qemu_init context.
> > And migration will start after that in migration_incoming_process from
> > qemu_main_loop context.
> > 
> > I validated that also by printing the value of GBPA from vmstate_smmuv3
> > pre_load at migration without GPBA, and it is the same as
> > SMMU_GBPA_RESET_VAL.
> 
> Is that from -loadvm on the command-line, or the loadvm command from the
> monitor?  It's the latter that I suspect requires the pre_load.

I was testing before with migration
-From qemu monitor with migrate command
-Load it from cmdline with -incoming

I tested now with savevm/loadvm from qemu monitor
On vmload command, load_snapshot is called which in the following
order calls:
- qemu_system_reset which calls smmu_reset_hold initializing the SMMU
registers.
- qemu_loadvm_state which loads the saved state.

So From what I see, pre_load won’t be needed in this case also.

Thanks,
Mostafa

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Peter Maydell]

On Sat, 11 Feb 2023 at 23:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/10/23 12:19, Mostafa Saleh wrote:
> > @@ -1512,6 +1553,10 @@ static const VMStateDescription vmstate_smmuv3 = {
> >
> >           VMSTATE_END_OF_LIST(),
> >       },
> > +    .subsections = (const VMStateDescription * []) {
> > +        &vmstate_gbpa,
> > +        NULL
> > +    }
>
> Actually, I suspect that you need a pre_load hook that resets gbpa, which will then be
> overwritten if and only if the saved value != reset value.

VM loads are guaranteed to start from a VM state which is freshly
reset, so the device's reset method is sufficient for this.

thanks
-- PMM

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Eric Auger]

Hi Mostafa,

On 2/12/23 00:34, Richard Henderson wrote:
> On 2/10/23 12:19, Mostafa Saleh wrote:
>> +static bool smmuv3_gbpa_needed(void *opaque)
>> +{
>> +    SMMUv3State *s = opaque;
>> +
>> +    /* Only migrate GBPA if it has different reset value. */
>> +    if (s->gbpa != SMMU_GBPA_RESET_VAL) {
>> +        return true;
>> +    }
>> +
>> +    return false;
>
> Just "return s->gbpa != SMMU_GBPA_RESET_VAL;".
>
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

With this suggestion, it looks good to me.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


>
>
> r~
>

Re: [PATCH v4] hw/arm/smmuv3: Add GBPA register

[Richard Henderson]

On 2/13/23 00:57, Peter Maydell wrote:
> On Sat, 11 Feb 2023 at 23:49, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 2/10/23 12:19, Mostafa Saleh wrote:
>>> @@ -1512,6 +1553,10 @@ static const VMStateDescription vmstate_smmuv3 = {
>>>
>>>            VMSTATE_END_OF_LIST(),
>>>        },
>>> +    .subsections = (const VMStateDescription * []) {
>>> +        &vmstate_gbpa,
>>> +        NULL
>>> +    }
>>
>> Actually, I suspect that you need a pre_load hook that resets gbpa, which will then be
>> overwritten if and only if the saved value != reset value.
> 
> VM loads are guaranteed to start from a VM state which is freshly
> reset, so the device's reset method is sufficient for this.

Thanks for the confirmation.

r~