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

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

[Peter Maydell]

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.maydell@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. Somebody with a better
grasp of the various different kinds of memory regions might be
able to suggest better documentation and/or a way to avoid this
oddball TCG-only function?

 include/exec/exec-all.h | 9 ++++++++-
 accel/tcg/cputlb.c      | 2 +-
 exec.c                  | 6 +++---
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index da73e3bfed2..7de4e4646f6 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -502,7 +502,14 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
                                        hwaddr paddr, hwaddr xlat,
                                        int prot,
                                        target_ulong *address);
-bool memory_region_is_unassigned(MemoryRegion *mr);
+/**
+ * memory_region_is_ram_backed:
+ * @mr: Memory region
+ *
+ * Return true if this memory region is backed by host RAM that we
+ * can directly execute guest code from.
+ */
+bool memory_region_is_ram_backed(MemoryRegion *mr);
 
 #endif
 
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 20c147d6554..e5e3bf76298 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1001,7 +1001,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     iotlbentry = &env->iotlb[mmu_idx][index];
     section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
     mr = section->mr;
-    if (memory_region_is_unassigned(mr)) {
+    if (!memory_region_is_ram_backed(mr)) {
         qemu_mutex_lock_iothread();
         if (memory_region_request_mmio_ptr(mr, addr)) {
             qemu_mutex_unlock_iothread();
diff --git a/exec.c b/exec.c
index 4f5df07b6a2..6aea975c266 100644
--- a/exec.c
+++ b/exec.c
@@ -402,10 +402,10 @@ static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr addr)
     }
 }
 
-bool memory_region_is_unassigned(MemoryRegion *mr)
+bool memory_region_is_ram_backed(MemoryRegion *mr)
 {
-    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);
 }
 
 /* Called from RCU critical section */
-- 
2.17.1

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 <peter.maydell@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.

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

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 <richard.henderson@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.maydell@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

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

[Richard Henderson]

On 07/09/2018 10:33 AM, Peter Maydell wrote:
>> 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;

Huh.  I don't follow how this old expression excludes ram,
and so assumed that must be checked separately earlier.

There's clearly still something here I don't understand.


r~

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:50, Richard Henderson <rth@twiddle.net> wrote:
> On 07/09/2018 10:33 AM, Peter Maydell wrote:
>>> 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;
>
> Huh.  I don't follow how this old expression excludes ram,
> and so assumed that must be checked separately earlier.
>
> There's clearly still something here I don't understand.

I dug a bit deeper, and the reason why this works is that the MR
it's operating on is not an arbitrary MR for this chunk of the
address space. It's the section->mr for the section returned by
iotlb_to_section() for the iotlbentry value. That value is set
up by memory_region_section_get_iotlb(), which picks either
PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM for any MR that is
RAM (by the memory_region_is_ram() definition). So then the
iotlb_to_section() call will return one of the dummy sections
and its section->mr will be either io_mem_rom or io_mem_notdirty,
not the original MR.

The other thing I've noticed is that after my in-progress
patches to handle execution from MMIO regions, the code that
calls memory_region_is_unassigned() is just

    iotlbentry = &env->iotlb[mmu_idx][index];
    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
    mr = section->mr;
    if (memory_region_is_unassigned(mr)) {
       ...

and there is no other use of 'mr', so perhaps it would be
better to have a function that takes a MemoryRegionSection
rather than a MemoryRegion that must be the one we got from
section->mr...

thanks
-- PMM