Re: [Qemu-devel] [PATCH 04/40] memory: Rename readable flag to romd_mode

[Peter Maydell]

On 7 May 2013 15:16, Paolo Bonzini <address@hidden> wrote:
> From: Jan Kiszka <address@hidden>
>
> "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