Re: [Qemu-devel] [PATCH] exec.c: Replace memory_region_is_unassigned() with memory_region_is_ram_backed()

[Peter Maydell]

On 9 July 2018 at 18:23, Richard Henderson <address@hidden> wrote:
> On 07/09/2018 08:58 AM, Peter Maydell wrote:
>> The function memory_region_is_unassigned() is rather misnamed, because
>> what it is actually testing is whether the memory region is backed
>> by host RAM, and so whether get_page_addr_code() can find a ram_addr_t
>> corresponding to the guest address.
>>
>> Replace it with memory_region_is_ram_backed(), which has a name
>> better matching its actual semantics.  (We invert the sense of the
>> return value to avoid having a _not_ in the function name.)
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>> The difference between this and memory_region_is_ram() is pretty
>> subtle, to the extent I'm not completely sure exactly what it is;
>> io_mem_notdirty and io_mem_watch at least won't be considered as
>> "ram" by memory_region_is_ram(), though.
>
> Yeah, I'm not quite sure why io_mem_rom and io_mem_notdirty at least do not
> have the rom_device bit set.
>
> I suppose io_mem_watch is even more special in that it also wants to trap read
> operations.  And do we really want to trap execute operations in that?  Surely
> that's what actual breakpoints are for.  Of course, that would probably mean
> special casing the special case.
>
>> -bool memory_region_is_unassigned(MemoryRegion *mr)
>> +bool memory_region_is_ram_backed(MemoryRegion *mr)
>
> Well... ok.  We should document that this, surprisingly, does not include
> actual ram.  Just things that are ram with caveats.

I think it must include actual RAM, or we would not be able to
execute from actual RAM ? The only way to get to get_page_addr_code()'s
"here's the ram_addr_t for this" exit path is if memory_region_is_unassigned()
returns false.

>> -    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
>> -        && mr != &io_mem_watch;
>> +    return !(mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
>> +             && mr != &io_mem_watch);
>
> This is annoyingly convoluted.  I would prefer to push
> the not through the expression:
>
>   return (mr->rom_device || mr == &io_mem_rom
>           || mr == &io_mem_notdirty || mr == &io_mem_watch);

Yeah, I was optimizing for "easy to review as not changing behaviour
except for flipping the sense of the return value", rather than
"resulting code is most simple".

thanks
-- PMM