Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Peter Xu]

On Wed, Jan 11, 2017 at 06:21:46PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/12/2016 08:58, Peter Xu wrote:
> > -                   mr->romd_mode ? 'R' : '-',
> > -                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 
> > 'W'
> > -                                                                       : 
> > '-',
> > +                   MR_CHAR_RD(mr),
> > +                   MR_CHAR_WR(mr),
> 
> An alternative definition could be
> 
>       memory_access_is_direct(mr, false) ? 'R' : '-'
>       memory_access_is_direct(mr, true) ? 'W' : '-'
> 
> for MR_CHAR_RD and MR_CHAR_WR.  With this change, I think the small code
> duplication in the "? :" operator is tolerable and the code is clearer.

memory_access_is_direct() will check against whether mr is RAM, is
that what we want here? In that case we'll get most of the regions as
"--" as long as they are not RAM, while in fact IMHO we should want to
know the rw permission for all cases.

How about I add one more patch at the beginning to provide some more
memory_region_is_*() helpers (meanwhile refactor
memory_access_is_direct() a bit), like:

--------8<--------
diff --git a/include/exec/memory.h b/include/exec/memory.h
index bec9756..50974c8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, 
hwaddr addr,
                                     MemTxAttrs attrs, uint8_t *buf, int len);
 void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);

+static inline bool memory_region_is_readable(MemoryRegion *mr)
+{
+    return mr->rom_device ? mr->romd_mode : true;
+}
+
+static inline bool memory_region_is_writable(MemoryRegion *mr)
+{
+    return !mr->rom_device && !mr->readonly;
+}
+
+static inline bool memory_region_is_direct(MemoryRegion *mr)
+{
+    return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr);
+}
+
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
     if (is_write) {
-        return memory_region_is_ram(mr) &&
-               !mr->readonly && !memory_region_is_ram_device(mr);
+        return memory_region_is_direct(mr) && memory_region_is_writable(mr);
     } else {
-        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) 
||
-               memory_region_is_romd(mr);
+        return memory_region_is_direct(mr) && memory_region_is_readable(mr);
     }
 }
-------->8--------

Then, I can throw away MR_CHAR_* macros and use:

    memory_access_is_readable(mr, false) ? 'R' : '-'
    memory_access_is_writable(mr, true) ? 'W' : '-'

Do you like this approach?

-- peterx