David Hildenbrand <da...@redhat.com> writes:
> On 25.09.19 02:16, Alex Bennée wrote:
>>
>> Richard Henderson <richard.hender...@linaro.org> writes:
>>
>>> It does not require going through the whole I/O path
>>> in order to discard a write.
>>>
>>> Reviewed-by: David Hildenbrand <da...@redhat.com>
>>> Signed-off-by: Richard Henderson <richard.hender...@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
