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

[David Hildenbrand]

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 <address@hidden>
> ---
>  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 <address@hidden>

>  
>  /* 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