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;
+    /* Transaction infomation for IOPMP */
+    unsigned long long iopmp_start_addr;
+    unsigned long long iopmp_end_addr;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
-- 
2.34.1

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.

Thanks,

-- 
Peter Xu

On Thu, Nov 02, 2023 at 09:49:17AM -0400, Peter Xu 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?

OK, I will 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.

Current IOMMU translate function only have address need to be translate. 
In IOPMP, It needs the transfer start and end address to check this transfer
is valid or not.

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

It's good for me to use a pointer.

Thanks,
Ethan Chen

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.

thanks
-- PMM

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.

Thanks,
Ethan Chen

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.

thanks
-- PMM

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

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.

thanks
-- PMM

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

On Tue, 7 Nov 2023 at 03:02, Ethan Chen <ethan84@andestech.com> wrote:
>
> On Mon, Nov 06, 2023 at 10:34:41AM +0000, Peter Maydell wrote:
> > 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.

QEMU doesn't emulate bus transactions at that level -- we have
no concept of burst transactions. You should have the IOMMU
do whatever it would do for a series of simple transactions.

thanks
-- PMM

On Tue, Nov 07, 2023 at 10:53:40AM +0000, Peter Maydell wrote:
> On Tue, 7 Nov 2023 at 03:02, Ethan Chen <ethan84@andestech.com> wrote:
> >
> > On Mon, Nov 06, 2023 at 10:34:41AM +0000, Peter Maydell wrote:
> > > 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.
> 
> QEMU doesn't emulate bus transactions at that level -- we have
> no concept of burst transactions. You should have the IOMMU
> do whatever it would do for a series of simple transactions.
>

I propose to use another method like StreamSink in hw/dma/xilinx_axidma.c to
let DMA send the signals to IOPMP instead of modifying IOMMU.

Thanks,
Ethan Chen