On 9 July 2018 at 18:23, Richard Henderson <richard.hender...@linaro.org> 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 <peter.mayd...@linaro.org> >> --- >> 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
