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

[David Hildenbrand]

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 :)

>>          }
>>          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