| Message ID | 20190923230004.9231-9-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Move rom and notdirty handling to cputlb | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > It does not require going through the whole I/O path > in order to discard a write. > > Reviewed-by: David Hildenbrand <david@redhat.com> > 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 d148bded35..26547cd6dd 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h <snip> > @@ -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; > + } This reads a bit weird because we are saying romd isn't a ROM but something that identifies as RAM can be ROM rather than just a memory protected piece of RAM. > } > if (prot & PAGE_WRITE_INV) { > tn.addr_write |= TLB_INVALID_MASK; So at the moment I don't see what the TLB_ROM flag gives us that setting TLB_INVALID doesn't - either way we won't make the write to our ram-not-ram-rom. > @@ -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; > } > > @@ -1613,6 +1613,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(need_swap)) { > diff --git a/exec.c b/exec.c > index 5f2587b621..ea8c0b18ac 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 > > @@ -192,7 +192,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); > @@ -1475,8 +1474,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; > @@ -3002,38 +2999,6 @@ static uint16_t dummy_section(PhysPageMap *map, FlatView *fv, MemoryRegion *mr) > return phys_section_add(map, §ion); > } > > -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) > { > @@ -3047,8 +3012,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); > > @@ -3069,8 +3032,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 }; -- Alex Bennée
On 25.09.19 02:16, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> It does not require going through the whole I/O path >> in order to discard a write. >> >> Reviewed-by: David Hildenbrand <david@redhat.com> >> 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 d148bded35..26547cd6dd 100644 >> --- a/include/exec/cpu-all.h >> +++ b/include/exec/cpu-all.h > <snip> >> @@ -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; >> + } > > This reads a bit weird because we are saying romd isn't a ROM but > something that identifies as RAM can be ROM rather than just a memory > protected piece of RAM. > I proposed a bunch of alternatives as reply to v3 (e.g., TLB_DISCARD_WRITES), either Richard missed them or I missed his reply :) >> } >> if (prot & PAGE_WRITE_INV) { >> tn.addr_write |= TLB_INVALID_MASK; > > So at the moment I don't see what the TLB_ROM flag gives us that setting > TLB_INVALID doesn't - either way we won't make the write to our > ram-not-ram-rom. TLB_INVALID will trigger a new MMU translation on every access to fill the TLB. TLB_ROM states that we have a valid entry, but that writes are to be discarded. -- Thanks, David / dhildenb
David Hildenbrand <david@redhat.com> writes: > On 25.09.19 02:16, Alex Bennée wrote: >> >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> It does not require going through the whole I/O path >>> in order to discard a write. >>> >>> Reviewed-by: David Hildenbrand <david@redhat.com> >>> 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 d148bded35..26547cd6dd 100644 >>> --- a/include/exec/cpu-all.h >>> +++ b/include/exec/cpu-all.h >> <snip> >>> @@ -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; >>> + } >> >> This reads a bit weird because we are saying romd isn't a ROM but >> something that identifies as RAM can be ROM rather than just a memory >> protected piece of RAM. >> > > I proposed a bunch of alternatives as reply to v3 (e.g., > TLB_DISCARD_WRITES), either Richard missed them or I missed his reply > :) That certainly passes the "does what it says on the tin" test. > >>> } >>> if (prot & PAGE_WRITE_INV) { >>> tn.addr_write |= TLB_INVALID_MASK; >> >> So at the moment I don't see what the TLB_ROM flag gives us that setting >> TLB_INVALID doesn't - either way we won't make the write to our >> ram-not-ram-rom. > > TLB_INVALID will trigger a new MMU translation on every access to fill > the TLB. TLB_ROM states that we have a valid entry, but that writes are > to be discarded. Ahh yes, I didn't notice it because it's hidden in he tlb_hit check. -- Alex Bennée
On 9/24/19 11:59 PM, David Hildenbrand wrote: >>> + 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; >>> + } >> >> This reads a bit weird because we are saying romd isn't a ROM but >> something that identifies as RAM can be ROM rather than just a memory >> protected piece of RAM. >> > > I proposed a bunch of alternatives as reply to v3 (e.g., > TLB_DISCARD_WRITES), either Richard missed them or I missed his reply :) Missed it, sorry. r~
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index d148bded35..26547cd6dd 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -337,12 +337,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 f634edb4f4..af9a44a847 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; } @@ -1613,6 +1613,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(need_swap)) { diff --git a/exec.c b/exec.c index 5f2587b621..ea8c0b18ac 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 @@ -192,7 +192,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); @@ -1475,8 +1474,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; @@ -3002,38 +2999,6 @@ static uint16_t dummy_section(PhysPageMap *map, FlatView *fv, MemoryRegion *mr) return phys_section_add(map, §ion); } -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) { @@ -3047,8 +3012,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); @@ -3069,8 +3032,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 };