Re: [Qemu-devel] [PATCH v3 5/5] fw_cfg: replace ioport data read with generic method

[Laszlo Ersek <lersek@redhat.com>]

On 11/03/15 01:35, Gabriel L. Somlo wrote:
> IOPort read access is limited to one byte at a time by
> fw_cfg_comb_valid(). As such, fw_cfg_comb_read() may safely
> ignore its size argument (which will always be 1), and simply
> call its fw_cfg_read() helper function once, returning 8 bits
> via the least significant byte of a 64-bit return value.
> 
> This patch replaces fw_cfg_comb_read() with the generic method
> fw_cfg_data_read(), and removes the unused fw_cfg_read() helper.
> 
> When called with size = 1, fw_cfg_data_read() acts exactly like
> fw_cfg_read(), performing the same set of sanity checks, and
> executing the while loop at most once (subject to the current
> read offset being within range).

If you choose (2a) for v3 4/5, then the commit message here can stay as-is.

If you choose (2b), then the last sentence should say

  When called with size = 1, fw_cfg_data_read() acts exactly like
  fw_cfg_read(), performing the same set of sanity checks, and
  executing the while loop exactly once (where the value composition
  honors the current read offset being within range).

With that update (as appropriate),

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc Marí <markmb@redhat.com>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 8aa980c..7d216f0 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -292,22 +292,6 @@ static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size)
>      return value;
>  }
>  
> -static uint8_t fw_cfg_read(FWCfgState *s)
> -{
> -    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> -    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> -    uint8_t ret;
> -
> -    if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len)
> -        ret = 0;
> -    else {
> -        ret = e->data[s->cur_offset++];
> -    }
> -
> -    trace_fw_cfg_read(s, ret);
> -    return ret;
> -}
> -
>  static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
>                                    uint64_t value, unsigned size)
>  {
> @@ -456,12 +440,6 @@ static bool fw_cfg_ctl_mem_valid(void *opaque, hwaddr addr,
>      return is_write && size == 2;
>  }
>  
> -static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
> -                                 unsigned size)
> -{
> -    return fw_cfg_read(opaque);
> -}
> -
>  static void fw_cfg_comb_write(void *opaque, hwaddr addr,
>                                uint64_t value, unsigned size)
>  {
> @@ -499,7 +477,7 @@ static const MemoryRegionOps fw_cfg_data_mem_ops = {
>  };
>  
>  static const MemoryRegionOps fw_cfg_comb_mem_ops = {
> -    .read = fw_cfg_comb_read,
> +    .read = fw_cfg_data_read,
>      .write = fw_cfg_comb_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
>      .valid.accepts = fw_cfg_comb_valid,
>