diff mbox series

[v3,12/20] cputlb: Move ROM handling from I/O path to TLB path

Message ID 20190922035458.14879-13-richard.henderson@linaro.org
State Superseded
Headers show
Series Move rom and notdirty handling to cputlb | expand

Commit Message

Richard Henderson Sept. 22, 2019, 3:54 a.m. UTC 
It does not require going through the whole I/O path
in order to discard a write.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 include/exec/cpu-all.h    |  5 ++++-
 include/exec/cpu-common.h |  1 -
 accel/tcg/cputlb.c        | 35 +++++++++++++++++++--------------
 exec.c                    | 41 +--------------------------------------
 4 files changed, 25 insertions(+), 57 deletions(-)

-- 
2.17.1

Comments

David Hildenbrand Sept. 23, 2019, 8:39 a.m. UTC | #1 
On 22.09.19 05:54, Richard Henderson wrote:
> It does not require going through the whole I/O path

> in order to discard a write.

> 

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  include/exec/cpu-all.h    |  5 ++++-

>  include/exec/cpu-common.h |  1 -

>  accel/tcg/cputlb.c        | 35 +++++++++++++++++++--------------

>  exec.c                    | 41 +--------------------------------------

>  4 files changed, 25 insertions(+), 57 deletions(-)

> 

> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h

> index 1ebd1b59ab..9f0b17802e 100644

> --- a/include/exec/cpu-all.h

> +++ b/include/exec/cpu-all.h

> @@ -348,12 +348,15 @@ CPUArchState *cpu_copy(CPUArchState *env);

>  #define TLB_WATCHPOINT      (1 << (TARGET_PAGE_BITS_MIN - 4))

>  /* Set if TLB entry requires byte swap.  */

>  #define TLB_BSWAP           (1 << (TARGET_PAGE_BITS_MIN - 5))

> +/* Set if TLB entry writes ignored.  */

> +#define TLB_ROM             (1 << (TARGET_PAGE_BITS_MIN - 6))


I was wondering if TLB_DISCARD_WRITE/TLB_IGNORE_WRITE/TLB_READONLY would
make it clearer what's actually happening here.

E.g., it isn't used for memory_region_is_romd(section->mr) but only for
memory_region_is_ram(section->mr) && section->readonly.

But anyhow, changes look fine to me

Reviewed-by: David Hildenbrand <david@redhat.com>


>  

>  /* Use this mask to check interception with an alignment mask

>   * in a TCG backend.

>   */

>  #define TLB_FLAGS_MASK \

> -    (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO | TLB_WATCHPOINT | TLB_BSWAP)

> +    (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO \

> +    | TLB_WATCHPOINT | TLB_BSWAP | TLB_ROM)

>  

>  /**

>   * tlb_hit_page: return true if page aligned @addr is a hit against the

> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h

> index f7dbe75fbc..1c0e03ddc2 100644

> --- a/include/exec/cpu-common.h

> +++ b/include/exec/cpu-common.h

> @@ -100,7 +100,6 @@ void qemu_flush_coalesced_mmio_buffer(void);

>  

>  void cpu_flush_icache_range(hwaddr start, hwaddr len);

>  

> -extern struct MemoryRegion io_mem_rom;

>  extern struct MemoryRegion io_mem_notdirty;

>  

>  typedef int (RAMBlockIterFunc)(RAMBlock *rb, void *opaque);

> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c

> index cb603917a2..7ab523d7ec 100644

> --- a/accel/tcg/cputlb.c

> +++ b/accel/tcg/cputlb.c

> @@ -577,7 +577,7 @@ static void tlb_reset_dirty_range_locked(CPUTLBEntry *tlb_entry,

>  {

>      uintptr_t addr = tlb_entry->addr_write;

>  

> -    if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_NOTDIRTY)) == 0) {

> +    if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_ROM | TLB_NOTDIRTY)) == 0) {

>          addr &= TARGET_PAGE_MASK;

>          addr += tlb_entry->addend;

>          if ((addr - start) < length) {

> @@ -745,7 +745,6 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,

>          address |= TLB_MMIO;

>          addend = 0;

>      } else {

> -        /* TLB_MMIO for rom/romd handled below */

>          addend = (uintptr_t)memory_region_get_ram_ptr(section->mr) + xlat;

>      }

>  

> @@ -822,16 +821,17 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,

>  

>      tn.addr_write = -1;

>      if (prot & PAGE_WRITE) {

> -        if ((memory_region_is_ram(section->mr) && section->readonly)

> -            || memory_region_is_romd(section->mr)) {

> -            /* Write access calls the I/O callback.  */

> -            tn.addr_write = address | TLB_MMIO;

> -        } else if (memory_region_is_ram(section->mr)

> -                   && cpu_physical_memory_is_clean(

> -                       memory_region_get_ram_addr(section->mr) + xlat)) {

> -            tn.addr_write = address | TLB_NOTDIRTY;

> -        } else {

> -            tn.addr_write = address;

> +        tn.addr_write = address;

> +        if (memory_region_is_romd(section->mr)) {

> +            /* Use the MMIO path so that the device can switch states. */

> +            tn.addr_write |= TLB_MMIO;

> +        } else if (memory_region_is_ram(section->mr)) {

> +            if (section->readonly) {

> +                tn.addr_write |= TLB_ROM;

> +            } else if (cpu_physical_memory_is_clean(

> +                        memory_region_get_ram_addr(section->mr) + xlat)) {

> +                tn.addr_write |= TLB_NOTDIRTY;

> +            }

>          }

>          if (prot & PAGE_WRITE_INV) {

>              tn.addr_write |= TLB_INVALID_MASK;

> @@ -904,7 +904,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,

>      mr = section->mr;

>      mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;

>      cpu->mem_io_pc = retaddr;

> -    if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {

> +    if (mr != &io_mem_notdirty && !cpu->can_do_io) {

>          cpu_io_recompile(cpu, retaddr);

>      }

>  

> @@ -945,7 +945,7 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,

>      section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);

>      mr = section->mr;

>      mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;

> -    if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {

> +    if (mr != &io_mem_notdirty && !cpu->can_do_io) {

>          cpu_io_recompile(cpu, retaddr);

>      }

>      cpu->mem_io_vaddr = addr;

> @@ -1125,7 +1125,7 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size,

>      }

>  

>      /* Reject I/O access, or other required slow-path.  */

> -    if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO | TLB_BSWAP)) {

> +    if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO | TLB_BSWAP | TLB_ROM)) {

>          return NULL;

>      }

>  

> @@ -1612,6 +1612,11 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,

>              return;

>          }

>  

> +        /* Ignore writes to ROM.  */

> +        if (unlikely(tlb_addr & TLB_ROM)) {

> +            return;

> +        }

> +

>          haddr = (void *)((uintptr_t)addr + entry->addend);

>  

>          if (unlikely(tlb_addr & TLB_BSWAP)) {

> diff --git a/exec.c b/exec.c

> index 7ce0515635..e21e068535 100644

> --- a/exec.c

> +++ b/exec.c

> @@ -88,7 +88,7 @@ static MemoryRegion *system_io;

>  AddressSpace address_space_io;

>  AddressSpace address_space_memory;

>  

> -MemoryRegion io_mem_rom, io_mem_notdirty;

> +MemoryRegion io_mem_notdirty;

>  static MemoryRegion io_mem_unassigned;

>  #endif

>  

> @@ -158,7 +158,6 @@ typedef struct subpage_t {

>  

>  #define PHYS_SECTION_UNASSIGNED 0

>  #define PHYS_SECTION_NOTDIRTY 1

> -#define PHYS_SECTION_ROM 2

>  

>  static void io_mem_init(void);

>  static void memory_map_init(void);

> @@ -1441,8 +1440,6 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,

>          iotlb = memory_region_get_ram_addr(section->mr) + xlat;

>          if (!section->readonly) {

>              iotlb |= PHYS_SECTION_NOTDIRTY;

> -        } else {

> -            iotlb |= PHYS_SECTION_ROM;

>          }

>      } else {

>          AddressSpaceDispatch *d;

> @@ -2968,38 +2965,6 @@ static uint16_t dummy_section(PhysPageMap *map, FlatView *fv, MemoryRegion *mr)

>      return phys_section_add(map, &section);

>  }

>  

> -static void readonly_mem_write(void *opaque, hwaddr addr,

> -                               uint64_t val, unsigned size)

> -{

> -    /* Ignore any write to ROM. */

> -}

> -

> -static bool readonly_mem_accepts(void *opaque, hwaddr addr,

> -                                 unsigned size, bool is_write,

> -                                 MemTxAttrs attrs)

> -{

> -    return is_write;

> -}

> -

> -/* This will only be used for writes, because reads are special cased

> - * to directly access the underlying host ram.

> - */

> -static const MemoryRegionOps readonly_mem_ops = {

> -    .write = readonly_mem_write,

> -    .valid.accepts = readonly_mem_accepts,

> -    .endianness = DEVICE_NATIVE_ENDIAN,

> -    .valid = {

> -        .min_access_size = 1,

> -        .max_access_size = 8,

> -        .unaligned = false,

> -    },

> -    .impl = {

> -        .min_access_size = 1,

> -        .max_access_size = 8,

> -        .unaligned = false,

> -    },

> -};

> -

>  MemoryRegionSection *iotlb_to_section(CPUState *cpu,

>                                        hwaddr index, MemTxAttrs attrs)

>  {

> @@ -3013,8 +2978,6 @@ MemoryRegionSection *iotlb_to_section(CPUState *cpu,

>  

>  static void io_mem_init(void)

>  {

> -    memory_region_init_io(&io_mem_rom, NULL, &readonly_mem_ops,

> -                          NULL, NULL, UINT64_MAX);

>      memory_region_init_io(&io_mem_unassigned, NULL, &unassigned_mem_ops, NULL,

>                            NULL, UINT64_MAX);

>  

> @@ -3035,8 +2998,6 @@ AddressSpaceDispatch *address_space_dispatch_new(FlatView *fv)

>      assert(n == PHYS_SECTION_UNASSIGNED);

>      n = dummy_section(&d->map, fv, &io_mem_notdirty);

>      assert(n == PHYS_SECTION_NOTDIRTY);

> -    n = dummy_section(&d->map, fv, &io_mem_rom);

> -    assert(n == PHYS_SECTION_ROM);

>  

>      d->phys_map  = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 };

>  

> 



-- 

Thanks,

David / dhildenb
diff mbox series

Patch

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 1ebd1b59ab..9f0b17802e 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -348,12 +348,15 @@  CPUArchState *cpu_copy(CPUArchState *env);
 #define TLB_WATCHPOINT      (1 << (TARGET_PAGE_BITS_MIN - 4))
 /* Set if TLB entry requires byte swap.  */
 #define TLB_BSWAP           (1 << (TARGET_PAGE_BITS_MIN - 5))
+/* Set if TLB entry writes ignored.  */
+#define TLB_ROM             (1 << (TARGET_PAGE_BITS_MIN - 6))
 
 /* Use this mask to check interception with an alignment mask
  * in a TCG backend.
  */
 #define TLB_FLAGS_MASK \
-    (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO | TLB_WATCHPOINT | TLB_BSWAP)
+    (TLB_INVALID_MASK | TLB_NOTDIRTY | TLB_MMIO \
+    | TLB_WATCHPOINT | TLB_BSWAP | TLB_ROM)
 
 /**
  * tlb_hit_page: return true if page aligned @addr is a hit against the
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index f7dbe75fbc..1c0e03ddc2 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -100,7 +100,6 @@  void qemu_flush_coalesced_mmio_buffer(void);
 
 void cpu_flush_icache_range(hwaddr start, hwaddr len);
 
-extern struct MemoryRegion io_mem_rom;
 extern struct MemoryRegion io_mem_notdirty;
 
 typedef int (RAMBlockIterFunc)(RAMBlock *rb, void *opaque);
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index cb603917a2..7ab523d7ec 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -577,7 +577,7 @@  static void tlb_reset_dirty_range_locked(CPUTLBEntry *tlb_entry,
 {
     uintptr_t addr = tlb_entry->addr_write;
 
-    if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_NOTDIRTY)) == 0) {
+    if ((addr & (TLB_INVALID_MASK | TLB_MMIO | TLB_ROM | TLB_NOTDIRTY)) == 0) {
         addr &= TARGET_PAGE_MASK;
         addr += tlb_entry->addend;
         if ((addr - start) < length) {
@@ -745,7 +745,6 @@  void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
         address |= TLB_MMIO;
         addend = 0;
     } else {
-        /* TLB_MMIO for rom/romd handled below */
         addend = (uintptr_t)memory_region_get_ram_ptr(section->mr) + xlat;
     }
 
@@ -822,16 +821,17 @@  void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
 
     tn.addr_write = -1;
     if (prot & PAGE_WRITE) {
-        if ((memory_region_is_ram(section->mr) && section->readonly)
-            || memory_region_is_romd(section->mr)) {
-            /* Write access calls the I/O callback.  */
-            tn.addr_write = address | TLB_MMIO;
-        } else if (memory_region_is_ram(section->mr)
-                   && cpu_physical_memory_is_clean(
-                       memory_region_get_ram_addr(section->mr) + xlat)) {
-            tn.addr_write = address | TLB_NOTDIRTY;
-        } else {
-            tn.addr_write = address;
+        tn.addr_write = address;
+        if (memory_region_is_romd(section->mr)) {
+            /* Use the MMIO path so that the device can switch states. */
+            tn.addr_write |= TLB_MMIO;
+        } else if (memory_region_is_ram(section->mr)) {
+            if (section->readonly) {
+                tn.addr_write |= TLB_ROM;
+            } else if (cpu_physical_memory_is_clean(
+                        memory_region_get_ram_addr(section->mr) + xlat)) {
+                tn.addr_write |= TLB_NOTDIRTY;
+            }
         }
         if (prot & PAGE_WRITE_INV) {
             tn.addr_write |= TLB_INVALID_MASK;
@@ -904,7 +904,7 @@  static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     mr = section->mr;
     mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
     cpu->mem_io_pc = retaddr;
-    if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
+    if (mr != &io_mem_notdirty && !cpu->can_do_io) {
         cpu_io_recompile(cpu, retaddr);
     }
 
@@ -945,7 +945,7 @@  static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
     mr = section->mr;
     mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
-    if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
+    if (mr != &io_mem_notdirty && !cpu->can_do_io) {
         cpu_io_recompile(cpu, retaddr);
     }
     cpu->mem_io_vaddr = addr;
@@ -1125,7 +1125,7 @@  void *probe_access(CPUArchState *env, target_ulong addr, int size,
     }
 
     /* Reject I/O access, or other required slow-path.  */
-    if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO | TLB_BSWAP)) {
+    if (tlb_addr & (TLB_NOTDIRTY | TLB_MMIO | TLB_BSWAP | TLB_ROM)) {
         return NULL;
     }
 
@@ -1612,6 +1612,11 @@  store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
             return;
         }
 
+        /* Ignore writes to ROM.  */
+        if (unlikely(tlb_addr & TLB_ROM)) {
+            return;
+        }
+
         haddr = (void *)((uintptr_t)addr + entry->addend);
 
         if (unlikely(tlb_addr & TLB_BSWAP)) {
diff --git a/exec.c b/exec.c
index 7ce0515635..e21e068535 100644
--- a/exec.c
+++ b/exec.c
@@ -88,7 +88,7 @@  static MemoryRegion *system_io;
 AddressSpace address_space_io;
 AddressSpace address_space_memory;
 
-MemoryRegion io_mem_rom, io_mem_notdirty;
+MemoryRegion io_mem_notdirty;
 static MemoryRegion io_mem_unassigned;
 #endif
 
@@ -158,7 +158,6 @@  typedef struct subpage_t {
 
 #define PHYS_SECTION_UNASSIGNED 0
 #define PHYS_SECTION_NOTDIRTY 1
-#define PHYS_SECTION_ROM 2
 
 static void io_mem_init(void);
 static void memory_map_init(void);
@@ -1441,8 +1440,6 @@  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
         iotlb = memory_region_get_ram_addr(section->mr) + xlat;
         if (!section->readonly) {
             iotlb |= PHYS_SECTION_NOTDIRTY;
-        } else {
-            iotlb |= PHYS_SECTION_ROM;
         }
     } else {
         AddressSpaceDispatch *d;
@@ -2968,38 +2965,6 @@  static uint16_t dummy_section(PhysPageMap *map, FlatView *fv, MemoryRegion *mr)
     return phys_section_add(map, &section);
 }
 
-static void readonly_mem_write(void *opaque, hwaddr addr,
-                               uint64_t val, unsigned size)
-{
-    /* Ignore any write to ROM. */
-}
-
-static bool readonly_mem_accepts(void *opaque, hwaddr addr,
-                                 unsigned size, bool is_write,
-                                 MemTxAttrs attrs)
-{
-    return is_write;
-}
-
-/* This will only be used for writes, because reads are special cased
- * to directly access the underlying host ram.
- */
-static const MemoryRegionOps readonly_mem_ops = {
-    .write = readonly_mem_write,
-    .valid.accepts = readonly_mem_accepts,
-    .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
-        .min_access_size = 1,
-        .max_access_size = 8,
-        .unaligned = false,
-    },
-    .impl = {
-        .min_access_size = 1,
-        .max_access_size = 8,
-        .unaligned = false,
-    },
-};
-
 MemoryRegionSection *iotlb_to_section(CPUState *cpu,
                                       hwaddr index, MemTxAttrs attrs)
 {
@@ -3013,8 +2978,6 @@  MemoryRegionSection *iotlb_to_section(CPUState *cpu,
 
 static void io_mem_init(void)
 {
-    memory_region_init_io(&io_mem_rom, NULL, &readonly_mem_ops,
-                          NULL, NULL, UINT64_MAX);
     memory_region_init_io(&io_mem_unassigned, NULL, &unassigned_mem_ops, NULL,
                           NULL, UINT64_MAX);
 
@@ -3035,8 +2998,6 @@  AddressSpaceDispatch *address_space_dispatch_new(FlatView *fv)
     assert(n == PHYS_SECTION_UNASSIGNED);
     n = dummy_section(&d->map, fv, &io_mem_notdirty);
     assert(n == PHYS_SECTION_NOTDIRTY);
-    n = dummy_section(&d->map, fv, &io_mem_rom);
-    assert(n == PHYS_SECTION_ROM);
 
     d->phys_map  = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 };