Re: [PATCH v4 08/16] cputlb: Move ROM handling from I/O path to TLB path

[Alex Bennée]

David Hildenbrand <address@hidden> writes:

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