On 7 May 2013 15:16, Paolo Bonzini <pbonz...@redhat.com> wrote:
> From: Jan Kiszka <jan.kis...@siemens.com>
>
> "Readable" is a very unfortunate name for this flag because even a
> rom_device region will always be readable from the guest POV. What
> differs is the mapping, just like the comments had to explain already.
> Also, readable could currently be understood as being a generic region
> flag, but it only applies to rom_device regions.
>
> So name the flag and the function to modify it after the original term
"rename"
> "ROMD" which could also be interpreted as "ROM direct", i.e. ROM mode
> with direct access. In any case, the scope if the flag is clearer now.
"scope of".
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -111,7 +111,7 @@ static void pflash_setup_mappings(pflash_t *pfl)
>
> static void pflash_register_memory(pflash_t *pfl, int rom_mode)
> {
> - memory_region_rom_device_set_readable(&pfl->orig_mem, rom_mode);
> + memory_region_rom_device_set_romd(&pfl->orig_mem, rom_mode);
Obvious opportunity to change the name of the pflash_register_memory
parameter to match (separate trivial patch would be fine).
> pfl->rom_mode = rom_mode;
> }
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index efe210b..f65acfd 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -126,7 +126,7 @@ struct MemoryRegion {
> ram_addr_t ram_addr;
> bool subpage;
> bool terminates;
> - bool readable;
> + bool romd_mode;
> bool ram;
> bool readonly; /* For RAM regions */
> bool enabled;
> @@ -355,16 +355,16 @@ uint64_t memory_region_size(MemoryRegion *mr);
> bool memory_region_is_ram(MemoryRegion *mr);
>
> /**
> - * memory_region_is_romd: check whether a memory region is ROMD
> + * memory_region_is_romd: check whether a memory region is in ROMD mode
> *
> - * Returns %true is a memory region is ROMD and currently set to allow
> + * Returns %true is a memory region is a ROM device and currently set to
> allow
"Returns %true if"
> * direct reads.
> *
> * @mr: the memory region being queried
> */
> static inline bool memory_region_is_romd(MemoryRegion *mr)
> {
> - return mr->rom_device && mr->readable;
> + return mr->rom_device && mr->romd_mode;
> }
>
> /**
> @@ -502,18 +502,18 @@ void memory_region_reset_dirty(MemoryRegion *mr, hwaddr
> addr,
> void memory_region_set_readonly(MemoryRegion *mr, bool readonly);
>
> /**
> - * memory_region_rom_device_set_readable: enable/disable ROM readability
> + * memory_region_rom_device_set_romd: enable/disable ROMD mode
> *
> * Allows a ROM device (initialized with memory_region_init_rom_device() to
> - * to be marked as readable (default) or not readable. When it is readable,
> - * the device is mapped to guest memory. When not readable, reads are
> - * forwarded to the #MemoryRegion.read function.
> + * set to ROMD mode (default) or MMIO mode. When it is in ROMD mode, the
> + * device is mapped to guest memory and satisfies read access directly.
> + * When in MMIO mode, reads are forwarded to the #MemoryRegion.read function.
> + * Writes are always handled by the #MemoryRegion.write function.
> *
> * @mr: the memory region to be updated
> - * @readable: whether reads are satisified directly (%true) or via callbacks
> - * (%false)
> + * @romd_mode: whether the region in in ROMD mode or not
"is in", but that's not what the parameter means anyway.
"@romd_mode: %true to put the region into ROMD mode"
thanks
-- PMM
