[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/arm/smmuv3: Add GBPA register
|
From: |
Mostafa Saleh |
|
Subject: |
Re: [PATCH] hw/arm/smmuv3: Add GBPA register |
|
Date: |
Mon, 9 Jan 2023 13:53:39 +0000 |
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
>