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
@@ -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 };
--
2.17.1
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~
© 2016 - 2026 Red Hat, Inc.