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

[Paolo Bonzini <pbonzini@redhat.com>]

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