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 <smost...@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
