[v2,1/2] memory: provide common macros for mtree_print_mr()

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

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.

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

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

On 12/01/2017 10:46, Peter Xu wrote:
> 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.

It is more readable, but my plan was to turn these fields into a single
field (with bits) to speed up memory_access_is_direct.  For that we'd
need to undo your change.  So I'm undecided.

Paolo

On Thu, Jan 12, 2017 at 12:19:21PM +0100, Paolo Bonzini wrote:
> 
> 
> On 12/01/2017 10:46, Peter Xu wrote:
> > 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.
> 
> It is more readable, but my plan was to turn these fields into a single
> field (with bits) to speed up memory_access_is_direct.  For that we'd
> need to undo your change.  So I'm undecided.

No problem. Then I'll just ignore it and repost with above. Thanks!

-- peterx