Re: [RFC PATCH 02/16] accel/tcg: memory access from CPU will pass access_type to IOMMU

2024-06-13 Thread Jim Shu 
Hi Ethan,

Yes, this mechanism could handle such situations.

The important part is that the translate() function of
IOMMUMemoryRegion only returns the correct permission of the section
related to CPU access type.
For example, if wgChecker only permits RO permission, it will return
"downstream_as" with RO perm or "blocked_io_as" with WO perm.
(depending on CPU access type).

When the CPU access type is different from the previous one, it will
get TLB miss due to mismatched permissions.
Then, it will try to translate again, find the section of another
access type, and fill into iotlb. (also kick out the previous iotlb
entry).

This mechanism has poor performance if alternatively doing read/write
access from CPU to IOMMUMemoryRegion with RO/WO perm due to TLB miss.
I think it is the limitation that CPUTLBEntry doesn't support having
different sections of each permission. At least this mechanism has the
correct behavior.

Thanks,
Jim



On Thu, Jun 13, 2024 at 1:34 PM Ethan Chen  wrote:
>
> On Wed, Jun 12, 2024 at 04:14:02PM +0800, Jim Shu wrote:
> > [EXTERNAL MAIL]
> >
> > It is the preparation patch for upcoming RISC-V wgChecker device.
> >
> > Since RISC-V wgChecker could permit access in RO/WO permission, the
> > IOMMUMemoryRegion could return different section for read & write
> > access. The memory access from CPU should also pass the access_type to
> > IOMMU translate function so that IOMMU could return the correct section
> > of specified access_type.
> >
>
> Hi Jim,
>
> Does this method take into account the situation where the CPU access type is
> different from the access type when creating iotlb? I think the section
> might be wrong in this situation.
>
> Thanks,
> Ethan
> >
> >

Re: [RFC PATCH 02/16] accel/tcg: memory access from CPU will pass access_type to IOMMU

2024-06-12 Thread Ethan Chen via 
On Wed, Jun 12, 2024 at 04:14:02PM +0800, Jim Shu wrote:
> [EXTERNAL MAIL]
> 
> It is the preparation patch for upcoming RISC-V wgChecker device.
> 
> Since RISC-V wgChecker could permit access in RO/WO permission, the
> IOMMUMemoryRegion could return different section for read & write
> access. The memory access from CPU should also pass the access_type to
> IOMMU translate function so that IOMMU could return the correct section
> of specified access_type.
> 

Hi Jim,

Does this method take into account the situation where the CPU access type is
different from the access type when creating iotlb? I think the section
might be wrong in this situation.

Thanks,
Ethan
> 
>

[RFC PATCH 02/16] accel/tcg: memory access from CPU will pass access_type to IOMMU

2024-06-12 Thread Jim Shu 
It is the preparation patch for upcoming RISC-V wgChecker device.

Since RISC-V wgChecker could permit access in RO/WO permission, the
IOMMUMemoryRegion could return different section for read & write
access. The memory access from CPU should also pass the access_type to
IOMMU translate function so that IOMMU could return the correct section
of specified access_type.

Signed-off-by: Jim Shu 
---
 accel/tcg/cputlb.c   | 15 +--
 include/exec/exec-all.h  | 11 +++
 system/physmem.c | 16 +++-
 target/alpha/helper.c|  2 +-
 target/arm/tcg/tlb_helper.c  |  2 +-
 target/avr/helper.c  |  2 +-
 target/cris/helper.c |  2 +-
 target/hppa/mem_helper.c |  2 +-
 target/i386/tcg/sysemu/excp_helper.c |  3 ++-
 target/loongarch/tcg/tlb_helper.c|  2 +-
 target/m68k/helper.c | 10 +++---
 target/microblaze/helper.c   |  8 
 target/mips/tcg/sysemu/tlb_helper.c  |  4 ++--
 target/openrisc/mmu.c|  2 +-
 target/ppc/mmu_helper.c  |  2 +-
 target/riscv/cpu_helper.c|  2 +-
 target/rx/cpu.c  |  3 ++-
 target/s390x/tcg/excp_helper.c   |  2 +-
 target/sh4/helper.c  |  2 +-
 target/sparc/mmu_helper.c|  6 +++---
 target/tricore/helper.c  |  2 +-
 target/xtensa/helper.c   |  3 ++-
 22 files changed, 61 insertions(+), 42 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 8cf124b760..f1b07f6926 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1036,7 +1036,8 @@ static inline void tlb_set_compare(CPUTLBEntryFull *full, 
CPUTLBEntry *ent,
  * critical section.
  */
 void tlb_set_page_full(CPUState *cpu, int mmu_idx,
-   vaddr addr, CPUTLBEntryFull *full)
+   vaddr addr, MMUAccessType access_type,
+   CPUTLBEntryFull *full)
 {
 CPUTLB *tlb = >neg.tlb;
 CPUTLBDesc *desc = >d[mmu_idx];
@@ -1063,7 +1064,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
 prot = full->prot;
 asidx = cpu_asidx_from_attrs(cpu, full->attrs);
 section = address_space_translate_for_iotlb(cpu, asidx, paddr_page,
-, , full->attrs, 
);
+, , full->attrs, ,
+access_type);
 assert(sz >= TARGET_PAGE_SIZE);
 
 tlb_debug("vaddr=%016" VADDR_PRIx " paddr=0x" HWADDR_FMT_plx
@@ -1200,7 +1202,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
 
 void tlb_set_page_with_attrs(CPUState *cpu, vaddr addr,
  hwaddr paddr, MemTxAttrs attrs, int prot,
- int mmu_idx, uint64_t size)
+ MMUAccessType access_type, int mmu_idx,
+ uint64_t size)
 {
 CPUTLBEntryFull full = {
 .phys_addr = paddr,
@@ -1210,15 +1213,15 @@ void tlb_set_page_with_attrs(CPUState *cpu, vaddr addr,
 };
 
 assert(is_power_of_2(size));
-tlb_set_page_full(cpu, mmu_idx, addr, );
+tlb_set_page_full(cpu, mmu_idx, addr, access_type, );
 }
 
 void tlb_set_page(CPUState *cpu, vaddr addr,
-  hwaddr paddr, int prot,
+  hwaddr paddr, int prot, MMUAccessType access_type,
   int mmu_idx, uint64_t size)
 {
 tlb_set_page_with_attrs(cpu, addr, paddr, MEMTXATTRS_UNSPECIFIED,
-prot, mmu_idx, size);
+prot, access_type, mmu_idx, size);
 }
 
 /*
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index b6b46ad13c..0d5363ac02 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -205,7 +205,7 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState 
*cpu,
  * used by tlb_flush_page.
  */
 void tlb_set_page_full(CPUState *cpu, int mmu_idx, vaddr addr,
-   CPUTLBEntryFull *full);
+   MMUAccessType access_type, CPUTLBEntryFull *full);
 
 /**
  * tlb_set_page_with_attrs:
@@ -231,7 +231,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx, vaddr 
addr,
  */
 void tlb_set_page_with_attrs(CPUState *cpu, vaddr addr,
  hwaddr paddr, MemTxAttrs attrs,
- int prot, int mmu_idx, vaddr size);
+ int prot, MMUAccessType access_type, int mmu_idx,
+ vaddr size);
 /* tlb_set_page:
  *
  * This function is equivalent to calling tlb_set_page_with_attrs()
@@ -240,7 +241,8 @@ void tlb_set_page_with_attrs(CPUState *cpu, vaddr addr,
  */
 void tlb_set_page(CPUState *cpu, vaddr addr,
   hwaddr paddr, int prot,
-  int mmu_idx, vaddr size);
+  MMUAccessType access_type, int mmu_idx,
+  vaddr size);
 #else
 static inline