Re: [PATCH v2 1/4] exec/memattrs: Add iopmp source id, start address, end address to MemTxAttrs

[Ethan Chen]

On Mon, Nov 06, 2023 at 10:34:41AM +0000, Peter Maydell wrote:
> On Mon, 6 Nov 2023 at 01:57, Ethan Chen <ethan84@andestech.com> wrote:
> >
> > On Fri, Nov 03, 2023 at 10:34:28AM +0000, Peter Maydell wrote:
> > > On Fri, 3 Nov 2023 at 03:29, Ethan Chen <ethan84@andestech.com> wrote:
> > > >
> > > > On Thu, Nov 02, 2023 at 01:53:05PM +0000, Peter Maydell wrote:
> > > > > On Thu, 2 Nov 2023 at 13:49, Peter Xu <peterx@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Nov 02, 2023 at 05:40:12PM +0800, Ethan Chen wrote:
> > > > > > > Signed-off-by: Ethan Chen <ethan84@andestech.com>
> > > > > > > ---
> > > > > > >  include/exec/memattrs.h | 6 ++++++
> > > > > > >  1 file changed, 6 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> > > > > > > index d04170aa27..fc15e5d7d3 100644
> > > > > > > --- a/include/exec/memattrs.h
> > > > > > > +++ b/include/exec/memattrs.h
> > > > > > > @@ -64,6 +64,12 @@ typedef struct MemTxAttrs {
> > > > > > >      unsigned int target_tlb_bit0 : 1;
> > > > > > >      unsigned int target_tlb_bit1 : 1;
> > > > > > >      unsigned int target_tlb_bit2 : 1;
> > > > > > > +
> > > > > > > +    /* IOPMP support up to 65535 sources */
> > > > > > > +    unsigned int iopmp_sid:16;
> > > > > >
> > > > > > There's MemTxAttrs.requester_id, SID for pci, same length.  Reuse 
> > > > > > it?
> > > > > >
> > > > > > > +    /* Transaction infomation for IOPMP */
> > > > > > > +    unsigned long long iopmp_start_addr;
> > > > > > > +    unsigned long long iopmp_end_addr;
> > > > > >
> > > > > > PS: encoding addresses into memattrs is.. strange, but since I know 
> > > > > > nothing
> > > > > > about iopmp, I'll leave that for other reviewers.
> > > > > >
> > > > > > Currently MemTxAttrs are passed as a whole int on the stack, if it 
> > > > > > keeps
> > > > > > growing we may start to consider a pointer, but need to check the 
> > > > > > side
> > > > > > effects of unexpected fields modified within a call.
> > > > >
> > > > > Yeah, this struct is intended to model the various attributes that
> > > > > get passed around on the bus alongside data in real hardware.
> > > > > I'm pretty sure no real hardware is passing around start and
> > > > > end transaction addresses on its bus with every read and
> > > > > write, which suggests that we should be doing this some other
> > > > > way than adding these fields to the MemTxAttrs struct.
> > > >
> > > > For AXI bus ADDR, LEN, SIZE are signals in read/write address channel.
> > > > IOPMP will check that start address = ADDR,
> > > > and end address = ADDR + LEN * SIZE.
> > >
> > > Yes, but you don't pass the start and end address on the AXI
> > > bus, so they don't go in QEMU's MemTxAttrs either.
> >
> > I will add those AXI bus signals to MemTxAttrs instead of using start and 
> > end
> > address in next revision.
> 
> What AXI bus signals? You already get address and size in the
> actual memory transaction, they don't need to go in the MemTxAttrs.
>

A burst contains multiple continuous read or write operations. In current 
transaction, I can only get the size and address of a single operation. IOPMP 
checks not only a single operation but also the burst information. I propose
to add those signals to MemTxAttrs.

Following AXI signals are needed for IOPMP.
(AW/AR)ADDR: Address of the first beat(operation) of the burst
(AW/AR)LEN: Number of beats inside the burst
(AW/AR)SIZE: Size of each beat
(AW/AR)BURST: Type of the burst

Thanks,
Ethan Chen