* [PATCH] hw/arm/smmuv3: Add GBPA register @ 2022-12-19 12:57 Mostafa Saleh 2023-01-04 12:29 ` Jean-Philippe Brucker 0 siblings, 1 reply; 5+ messages in thread From: Mostafa Saleh @ 2022-12-19 12:57 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Maydell, Eric Auger, qemu-arm, Mostafa Saleh 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-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/arm/smmuv3: Add GBPA register 2022-12-19 12:57 [PATCH] hw/arm/smmuv3: Add GBPA register Mostafa Saleh @ 2023-01-04 12:29 ` Jean-Philippe Brucker 2023-01-05 17:26 ` Mostafa Saleh 2023-01-06 13:07 ` Eric Auger 0 siblings, 2 replies; 5+ messages in thread From: Jean-Philippe Brucker @ 2023-01-04 12:29 UTC (permalink / raw) To: Mostafa Saleh; +Cc: qemu-devel, Peter Maydell, Eric Auger, qemu-arm 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) > 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/arm/smmuv3: Add GBPA register 2023-01-04 12:29 ` Jean-Philippe Brucker @ 2023-01-05 17:26 ` Mostafa Saleh 2023-01-06 13:07 ` Eric Auger 1 sibling, 0 replies; 5+ messages in thread From: Mostafa Saleh @ 2023-01-05 17:26 UTC (permalink / raw) To: Jean-Philippe Brucker; +Cc: qemu-devel, Peter Maydell, Eric Auger, qemu-arm 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 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/arm/smmuv3: Add GBPA register 2023-01-04 12:29 ` Jean-Philippe Brucker 2023-01-05 17:26 ` Mostafa Saleh @ 2023-01-06 13:07 ` Eric Auger 2023-01-09 13:53 ` Mostafa Saleh 1 sibling, 1 reply; 5+ messages in thread From: Eric Auger @ 2023-01-06 13:07 UTC (permalink / raw) To: Jean-Philippe Brucker, Mostafa Saleh; +Cc: qemu-devel, Peter Maydell, qemu-arm 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/arm/smmuv3: Add GBPA register 2023-01-06 13:07 ` Eric Auger @ 2023-01-09 13:53 ` Mostafa Saleh 0 siblings, 0 replies; 5+ messages in thread From: Mostafa Saleh @ 2023-01-09 13:53 UTC (permalink / raw) To: Eric Auger; +Cc: Jean-Philippe Brucker, qemu-devel, Peter Maydell, qemu-arm 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-09 14:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-19 12:57 [PATCH] hw/arm/smmuv3: Add GBPA register Mostafa Saleh 2023-01-04 12:29 ` Jean-Philippe Brucker 2023-01-05 17:26 ` Mostafa Saleh 2023-01-06 13:07 ` Eric Auger 2023-01-09 13:53 ` Mostafa Saleh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).