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

[Mostafa Saleh]

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
> > 
> >