[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr() |
Date: |
Thu, 12 Jan 2017 09:54:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
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
>
>