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

[Richard Henderson]

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.

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


r~