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

[Peter Xu]

On Thu, Jan 12, 2017 at 09:54:24AM +0100, Paolo Bonzini wrote:
> 
> 
> On 12/01/2017 06:50, Peter Xu wrote:
> > 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.
> 
> Indeed.  My idea was that the RW permission is not well defined for
> non-RAM memory regions, and ROMD regions in MMIO mode shows as "--"
> while MMIO regions show as "RW".  But perhaps it's confusing.
> 
> What about writing one of "ram", "rom", "ramd", "romd", "i/o" (with I/O
> also covering rom_device && !romd_mode)?
> 
> If you disagree, the below patch looks good.

Yes, above suggestion makes sense to me, since after all the RW
permissions are derived from the type of memory regions, and the type
itself tells more things than the RW bits. So I totally agree we can
replace the "RW" chars with its type directly (if no one else
disagree, of course).

While for below patch, do you want me to include it as well as a
standalone patch, for the purpose of refactoring
memory_access_is_direct()? Since IMHO it's tiny clearer and more
readable than before.

Thanks!

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

-- peterx