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

[Laszlo Ersek <lersek@redhat.com>]

On 11/05/15 15:32, 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).
> 
> 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>
> Reviewd-by: Laszlo Ersek <lersek@redhat.com>

Not sure who will pick up this series, but he please fix the typo in the
last tag above, at commit time.

Thanks!
Laszlo

> ---
>  hw/nvram/fw_cfg.c | 25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 8f566f6..a1d650d 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -304,23 +304,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->cur_entry == FW_CFG_INVALID) ? NULL :
> -                    &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)
>  {
> @@ -470,12 +453,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)
>  {
> @@ -513,7 +490,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,
>