hw/arm/smmuv3-internal.h | 4 ++++ hw/arm/smmuv3.c | 14 ++++++++++++++ include/hw/arm/smmuv3.h | 1 + 3 files changed, 19 insertions(+)
GBPA register can be used to globally abort all
transactions.
Only UPDATE and ABORT bits are considered in this patch.
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).
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3-internal.h | 4 ++++
hw/arm/smmuv3.c | 14 ++++++++++++++
include/hw/arm/smmuv3.h | 1 +
3 files changed, 19 insertions(+)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index bce161870f..71f70141e8 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -79,6 +79,10 @@ 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)
+
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..2843bc3da9 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 = 0;
}
static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
@@ -663,6 +664,11 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
goto epilogue;
}
+ if (FIELD_EX32(s->gbpa, GBPA, ABORT)) {
+ status = SMMU_TRANS_ABORT;
+ goto epilogue;
+ }
+
cfg = smmuv3_get_config(sdev, &event);
if (!cfg) {
status = SMMU_TRANS_ERROR;
@@ -1170,6 +1176,10 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
case A_GERROR_IRQ_CFG2:
s->gerror_irq_cfg2 = data;
return MEMTX_OK;
+ case A_GBPA:
+ /* 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 +1328,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;
@@ -1495,6 +1508,7 @@ static const VMStateDescription vmstate_smmuv3 = {
VMSTATE_UINT32_ARRAY(cr, SMMUv3State, 3),
VMSTATE_UINT32(cr0ack, SMMUv3State),
VMSTATE_UINT32(statusr, SMMUv3State),
+ VMSTATE_UINT32(gbpa, SMMUv3State),
VMSTATE_UINT32(irq_ctrl, SMMUv3State),
VMSTATE_UINT32(gerror, SMMUv3State),
VMSTATE_UINT32(gerrorn, SMMUv3State),
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.0.314.g84b9a713c41-googHi Mostafa,
On Mon, Dec 19, 2022 at 12:57:20PM +0000, Mostafa Saleh wrote:
> GBPA register can be used to globally abort all
> transactions.
>
> Only UPDATE and ABORT bits are considered in this patch.
That's fair, although it effectively implements all bits since
smmuv3_translate() ignores memory attributes anyway
>
> 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).
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmuv3-internal.h | 4 ++++
> hw/arm/smmuv3.c | 14 ++++++++++++++
> include/hw/arm/smmuv3.h | 1 +
> 3 files changed, 19 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index bce161870f..71f70141e8 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -79,6 +79,10 @@ 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)
> +
> 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..2843bc3da9 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 = 0;
> }
>
> static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
> @@ -663,6 +664,11 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> goto epilogue;
> }
>
> + if (FIELD_EX32(s->gbpa, GBPA, ABORT)) {
> + status = SMMU_TRANS_ABORT;
> + goto epilogue;
> + }
> +
GBPA is only taken into account when SMMU_CR0.SMMUEN is 0 (6.3.9.6 SMMUEN)
> cfg = smmuv3_get_config(sdev, &event);
> if (!cfg) {
> status = SMMU_TRANS_ERROR;
> @@ -1170,6 +1176,10 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> case A_GERROR_IRQ_CFG2:
> s->gerror_irq_cfg2 = data;
> return MEMTX_OK;
> + case A_GBPA:
> + /* Ignore update bit as write is synchronous. */
We could also ignore a write that has Update=0, since that's required for
SMMUv3.2+ implementations (6.3.14.1 Update procedure)
> + s->gbpa = data & ~R_GBPA_UPDATE_MASK;
Do we need to synchronize with concurrent transactions here?
I couldn't find if QEMU already serializes MMIO writes and IOMMU
translation.
"Transactions arriving at the SMMU after completion of a GPBA update are
guaranteed to take the new attributes written." The guest tests completion
by reading the Update bit:
vCPU (host CPU 0) Device thread (host CPU 1)
(a) read GBPA.abort = 1
(b) write GBPA.{update,abort} = {1,0}
(c) read GBPA.update = 0
(d) launch DMA (e) execute DMA
(f) translation must read GBPA.abort = 0
I guess memory barriers after (b) and before (f) would ensure that. But I
wonder if SMMUEN also needs additional synchronization, and in that case a
rwlock would probably be simpler.
Thanks,
Jean
> + return MEMTX_OK;
> case A_STRTAB_BASE: /* 64b */
> s->strtab_base = deposit64(s->strtab_base, 0, 32, data);
> return MEMTX_OK;
> @@ -1318,6 +1328,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;
> @@ -1495,6 +1508,7 @@ static const VMStateDescription vmstate_smmuv3 = {
> VMSTATE_UINT32_ARRAY(cr, SMMUv3State, 3),
> VMSTATE_UINT32(cr0ack, SMMUv3State),
> VMSTATE_UINT32(statusr, SMMUv3State),
> + VMSTATE_UINT32(gbpa, SMMUv3State),
> VMSTATE_UINT32(irq_ctrl, SMMUv3State),
> VMSTATE_UINT32(gerror, SMMUv3State),
> VMSTATE_UINT32(gerrorn, SMMUv3State),
> 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.0.314.g84b9a713c41-goog
>
>
Hi Mostafan jean,
On 1/4/23 13:29, Jean-Philippe Brucker wrote:
> Hi Mostafa,
>
> On Mon, Dec 19, 2022 at 12:57:20PM +0000, Mostafa Saleh wrote:
>> GBPA register can be used to globally abort all
>> transactions.
>>
>> Only UPDATE and ABORT bits are considered in this patch.
> That's fair, although it effectively implements all bits since
> smmuv3_translate() ignores memory attributes anyway
I see SHCFG 0b00 value means non shareable whereas other reset values
correspond to "Use Incoming". Isn't it a bit weird? Is it better to have
"non shareable" or "Use Incoming" as de fault value (which is
IMPLEMENTATION DEFINED)
>
>> 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).
>>
>> Signed-off-by: Mostafa Saleh <smostafa@google.com>
>> ---
>> hw/arm/smmuv3-internal.h | 4 ++++
>> hw/arm/smmuv3.c | 14 ++++++++++++++
>> include/hw/arm/smmuv3.h | 1 +
>> 3 files changed, 19 insertions(+)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index bce161870f..71f70141e8 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -79,6 +79,10 @@ 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)
>> +
>> 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..2843bc3da9 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 = 0;
>> }
>>
>> static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
>> @@ -663,6 +664,11 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>> goto epilogue;
>> }
>>
>> + if (FIELD_EX32(s->gbpa, GBPA, ABORT)) {
>> + status = SMMU_TRANS_ABORT;
>> + goto epilogue;
>> + }
>> +
> GBPA is only taken into account when SMMU_CR0.SMMUEN is 0 (6.3.9.6 SMMUEN)
indeed "This register controls the global bypass attributes used for
transactions from Non-secure StreamIDs (as determined
by SSD, if supported) when SMMU_CR0.SMMUEN == 0"
>
>> cfg = smmuv3_get_config(sdev, &event);
>> if (!cfg) {
>> status = SMMU_TRANS_ERROR;
>> @@ -1170,6 +1176,10 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
>> case A_GERROR_IRQ_CFG2:
>> s->gerror_irq_cfg2 = data;
>> return MEMTX_OK;
>> + case A_GBPA:
>> + /* Ignore update bit as write is synchronous. */
actually you immediataly reset the update bit and not really "ignore" it.
> We could also ignore a write that has Update=0, since that's required for
> SMMUv3.2+ implementations (6.3.14.1 Update procedure)
agreed:
"If this register is written without simultaneously setting Update to 1,
the effect is CONSTRAINED UNPREDICTABLE
and has one of the following behaviors:
• The write is IGNORED. This is the only permitted behavior in SMMUv3.2
and later."
>
>> + s->gbpa = data & ~R_GBPA_UPDATE_MASK;
> Do we need to synchronize with concurrent transactions here?
> I couldn't find if QEMU already serializes MMIO writes and IOMMU
> translation.
my understanding is qemu_global_mutex also is enough. BQL usage was
discussed in
https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02403.html
>
> "Transactions arriving at the SMMU after completion of a GPBA update are
> guaranteed to take the new attributes written." The guest tests completion
> by reading the Update bit:
>
> vCPU (host CPU 0) Device thread (host CPU 1)
>
> (a) read GBPA.abort = 1
> (b) write GBPA.{update,abort} = {1,0}
> (c) read GBPA.update = 0
> (d) launch DMA (e) execute DMA
> (f) translation must read GBPA.abort = 0
>
> I guess memory barriers after (b) and before (f) would ensure that. But I
> wonder if SMMUEN also needs additional synchronization, and in that case a
> rwlock would probably be simpler.
>
> Thanks,
> Jean
>
>> + return MEMTX_OK;
>> case A_STRTAB_BASE: /* 64b */
>> s->strtab_base = deposit64(s->strtab_base, 0, 32, data);
>> return MEMTX_OK;
>> @@ -1318,6 +1328,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;
>> @@ -1495,6 +1508,7 @@ static const VMStateDescription vmstate_smmuv3 = {
>> VMSTATE_UINT32_ARRAY(cr, SMMUv3State, 3),
>> VMSTATE_UINT32(cr0ack, SMMUv3State),
>> VMSTATE_UINT32(statusr, SMMUv3State),
>> + VMSTATE_UINT32(gbpa, SMMUv3State),
This will break migration (see
https://www.qemu.org/docs/master/devel/migration.html)
>> VMSTATE_UINT32(irq_ctrl, SMMUv3State),
>> VMSTATE_UINT32(gerror, SMMUv3State),
>> VMSTATE_UINT32(gerrorn, SMMUv3State),
>> 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.0.314.g84b9a713c41-goog
>>
>>
Thanks
Eric
Hi Eric,
On Fri, Jan 06, 2023 at 02:07:37PM +0100, Eric Auger wrote:
> Hi Mostafan jean,
>
> On 1/4/23 13:29, Jean-Philippe Brucker wrote:
> > Hi Mostafa,
> >
> > On Mon, Dec 19, 2022 at 12:57:20PM +0000, Mostafa Saleh wrote:
> >> GBPA register can be used to globally abort all
> >> transactions.
> >>
> >> Only UPDATE and ABORT bits are considered in this patch.
> > That's fair, although it effectively implements all bits since
> > smmuv3_translate() ignores memory attributes anyway
>
> I see SHCFG 0b00 value means non shareable whereas other reset values
> correspond to "Use Incoming". Isn't it a bit weird? Is it better to have
> "non shareable" or "Use Incoming" as de fault value (which is
> IMPLEMENTATION DEFINED)
I agree, "Use Incoming" would be more consistent with the others, I will
change that in V2.
> >
> >> 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).
> >>
> >> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> >> ---
> >> hw/arm/smmuv3-internal.h | 4 ++++
> >> hw/arm/smmuv3.c | 14 ++++++++++++++
> >> include/hw/arm/smmuv3.h | 1 +
> >> 3 files changed, 19 insertions(+)
> >>
> >> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> >> index bce161870f..71f70141e8 100644
> >> --- a/hw/arm/smmuv3-internal.h
> >> +++ b/hw/arm/smmuv3-internal.h
> >> @@ -79,6 +79,10 @@ 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)
> >> +
> >> 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..2843bc3da9 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 = 0;
> >> }
> >>
> >> static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
> >> @@ -663,6 +664,11 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> >> goto epilogue;
> >> }
> >>
> >> + if (FIELD_EX32(s->gbpa, GBPA, ABORT)) {
> >> + status = SMMU_TRANS_ABORT;
> >> + goto epilogue;
> >> + }
> >> +
> > GBPA is only taken into account when SMMU_CR0.SMMUEN is 0 (6.3.9.6 SMMUEN)
> indeed "This register controls the global bypass attributes used for
> transactions from Non-secure StreamIDs (as determined
> by SSD, if supported) when SMMU_CR0.SMMUEN == 0"
Will fix it in V2.
> >
> >> cfg = smmuv3_get_config(sdev, &event);
> >> if (!cfg) {
> >> status = SMMU_TRANS_ERROR;
> >> @@ -1170,6 +1176,10 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> >> case A_GERROR_IRQ_CFG2:
> >> s->gerror_irq_cfg2 = data;
> >> return MEMTX_OK;
> >> + case A_GBPA:
> >> + /* Ignore update bit as write is synchronous. */
> actually you immediataly reset the update bit and not really "ignore" it.
> > We could also ignore a write that has Update=0, since that's required for
> > SMMUv3.2+ implementations (6.3.14.1 Update procedure)
> agreed:
> "If this register is written without simultaneously setting Update to 1,
> the effect is CONSTRAINED UNPREDICTABLE
> and has one of the following behaviors:
> • The write is IGNORED. This is the only permitted behavior in SMMUv3.2
> and later."
I will update it to have v3.2 behaviour (Ignore if UPDATE not set).
> >
> >> + s->gbpa = data & ~R_GBPA_UPDATE_MASK;
> > Do we need to synchronize with concurrent transactions here?
> > I couldn't find if QEMU already serializes MMIO writes and IOMMU
> > translation.
>
> my understanding is qemu_global_mutex also is enough. BQL usage was
> discussed in
> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg02403.html
> >
> > "Transactions arriving at the SMMU after completion of a GPBA update are
> > guaranteed to take the new attributes written." The guest tests completion
> > by reading the Update bit:
> >
> > vCPU (host CPU 0) Device thread (host CPU 1)
> >
> > (a) read GBPA.abort = 1
> > (b) write GBPA.{update,abort} = {1,0}
> > (c) read GBPA.update = 0
> > (d) launch DMA (e) execute DMA
> > (f) translation must read GBPA.abort = 0
> >
> > I guess memory barriers after (b) and before (f) would ensure that. But I
> > wonder if SMMUEN also needs additional synchronization, and in that case a
> > rwlock would probably be simpler.
> >
> > Thanks,
> > Jean
> >
> >> + return MEMTX_OK;
> >> case A_STRTAB_BASE: /* 64b */
> >> s->strtab_base = deposit64(s->strtab_base, 0, 32, data);
> >> return MEMTX_OK;
> >> @@ -1318,6 +1328,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;
> >> @@ -1495,6 +1508,7 @@ static const VMStateDescription vmstate_smmuv3 = {
> >> VMSTATE_UINT32_ARRAY(cr, SMMUv3State, 3),
> >> VMSTATE_UINT32(cr0ack, SMMUv3State),
> >> VMSTATE_UINT32(statusr, SMMUv3State),
> >> + VMSTATE_UINT32(gbpa, SMMUv3State),
> This will break migration (see
> https://www.qemu.org/docs/master/devel/migration.html)
Thanks for pointing this out, I was not familiar with migration.
I will add a subsection for GBPA, so we can have forward compatibility.
But do we need to have backward compatibility also?
As(following the paradigm) if we added a property "migrate_gbpa", it can
lead to behaviors where transactions are aborted(with a new qemu version) and
then migrated to bypass(for an old qemu without gbpa).
Is this acceptable?
Maybe we can use this property, and unconditionally send GBPA if it was
set to 1(so migration fails if GBPA is set and the old machine doesn't
have it), otherwise we maintain backward compatibility.
> >> VMSTATE_UINT32(irq_ctrl, SMMUv3State),
> >> VMSTATE_UINT32(gerror, SMMUv3State),
> >> VMSTATE_UINT32(gerrorn, SMMUv3State),
> >> 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.0.314.g84b9a713c41-goog
> >>
> >>
> Thanks
>
> Eric
>
Hi Jean,
Thanks for taking the time to look into this.
On Wed, Jan 04, 2023 at 12:29:10PM +0000, Jean-Philippe Brucker wrote:
> Hi Mostafa,
>
> On Mon, Dec 19, 2022 at 12:57:20PM +0000, Mostafa Saleh wrote:
> > GBPA register can be used to globally abort all
> > transactions.
> >
> > Only UPDATE and ABORT bits are considered in this patch.
>
> That's fair, although it effectively implements all bits since
> smmuv3_translate() ignores memory attributes anyway
>
> >
> > 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).
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > hw/arm/smmuv3-internal.h | 4 ++++
> > hw/arm/smmuv3.c | 14 ++++++++++++++
> > include/hw/arm/smmuv3.h | 1 +
> > 3 files changed, 19 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> > index bce161870f..71f70141e8 100644
> > --- a/hw/arm/smmuv3-internal.h
> > +++ b/hw/arm/smmuv3-internal.h
> > @@ -79,6 +79,10 @@ 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)
> > +
> > 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..2843bc3da9 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 = 0;
> > }
> >
> > static int smmu_get_ste(SMMUv3State *s, dma_addr_t addr, STE *buf,
> > @@ -663,6 +664,11 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> > goto epilogue;
> > }
> >
> > + if (FIELD_EX32(s->gbpa, GBPA, ABORT)) {
> > + status = SMMU_TRANS_ABORT;
> > + goto epilogue;
> > + }
> > +
>
> GBPA is only taken into account when SMMU_CR0.SMMUEN is 0 (6.3.9.6 SMMUEN)
>
I missed that, will update it in V2.
> > cfg = smmuv3_get_config(sdev, &event);
> > if (!cfg) {
> > status = SMMU_TRANS_ERROR;
> > @@ -1170,6 +1176,10 @@ static MemTxResult smmu_writel(SMMUv3State *s, hwaddr offset,
> > case A_GERROR_IRQ_CFG2:
> > s->gerror_irq_cfg2 = data;
> > return MEMTX_OK;
> > + case A_GBPA:
> > + /* Ignore update bit as write is synchronous. */
>
> We could also ignore a write that has Update=0, since that's required for
> SMMUv3.2+ implementations (6.3.14.1 Update procedure)
I will add it in V2.
> > + s->gbpa = data & ~R_GBPA_UPDATE_MASK;
>
> Do we need to synchronize with concurrent transactions here?
> I couldn't find if QEMU already serializes MMIO writes and IOMMU
> translation.
>
> "Transactions arriving at the SMMU after completion of a GPBA update are
> guaranteed to take the new attributes written." The guest tests completion
> by reading the Update bit:
>
> vCPU (host CPU 0) Device thread (host CPU 1)
>
> (a) read GBPA.abort = 1
> (b) write GBPA.{update,abort} = {1,0}
> (c) read GBPA.update = 0
> (d) launch DMA (e) execute DMA
> (f) translation must read GBPA.abort = 0
>
> I guess memory barriers after (b) and before (f) would ensure that. But I
> wonder if SMMUEN also needs additional synchronization, and in that case a
> rwlock would probably be simpler.
>
From what I see, it does with qemu_global_mutex.
smmu_write_mmio: acquired from context of io_writex
smmuv3_translate: acquired from context of os_host_main_loop_wait
So I'd assume this should be fine. (I also checked with GDB)
However, if I missed something, and we need to synchronize, I think this
would also be a bug in SMMUEN.
As it is written from smmu_write_mmio and read at smmuv3_translate the
same way as GBPA.
And as described here (6.3.9.6 SMMUEN)
Completion of an Update of SMMUEN from 0 to 1 ensures that:
-Configuration written to SMMU_(S_)CR2 has taken effect.
-All new transactions will be treated with STE configuration relevant to
their stream, and will not undergo SMMU bypass.
So it will suffer from the same problem.
Thanks,
Mostafa
> Thanks,
> Jean
>
> > + return MEMTX_OK;
> > case A_STRTAB_BASE: /* 64b */
> > s->strtab_base = deposit64(s->strtab_base, 0, 32, data);
> > return MEMTX_OK;
> > @@ -1318,6 +1328,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;
> > @@ -1495,6 +1508,7 @@ static const VMStateDescription vmstate_smmuv3 = {
> > VMSTATE_UINT32_ARRAY(cr, SMMUv3State, 3),
> > VMSTATE_UINT32(cr0ack, SMMUv3State),
> > VMSTATE_UINT32(statusr, SMMUv3State),
> > + VMSTATE_UINT32(gbpa, SMMUv3State),
> > VMSTATE_UINT32(irq_ctrl, SMMUv3State),
> > VMSTATE_UINT32(gerror, SMMUv3State),
> > VMSTATE_UINT32(gerrorn, SMMUv3State),
> > 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.0.314.g84b9a713c41-goog
> >
> >
© 2016 - 2024 Red Hat, Inc.