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

[Ethan Chen via <qemu-riscv@nongnu.org>]

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.

Thanks,
Ethan Chen