Re: [Qemu-devel] [PATCH v2 4/4] fw_cfg: streamline (non-DMA) read operations

[Laszlo Ersek]

Two comments for now:

On 10/28/15 18:20, Gabriel L. Somlo wrote:
> Replace fw_cfg_comb_read(), fw_cfg_data_mem_read(), and fw_cfg_read()
> with a single method, fw_cfg_data_read(), which works on all possible
> read sizes (single- or multi-byte). The new method also eliminates
> redundant validity checks caused by multi-byte reads repeatedly invoking
> the old single-byte fw_cfg_read() method.
> 
> Also update trace_fw_cfg_read() prototype to accept a 64-bit value
> argument.
> 
> Cc: Laszlo Ersek <address@hidden>
> Cc: Gerd Hoffmann <address@hidden>
> Cc: Marc Marí <address@hidden>
> Signed-off-by: Gabriel Somlo <address@hidden>
> ---
>  hw/nvram/fw_cfg.c | 37 ++++++++++---------------------------
>  trace-events      |  2 +-
>  2 files changed, 11 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 5de6dbc..cd477f9 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -274,32 +274,20 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
>      return ret;
>  }
>  
> -static uint8_t fw_cfg_read(FWCfgState *s)
> +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size)
>  {
> +    FWCfgState *s = opaque;
>      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 uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
> -                                     unsigned size)
> -{
> -    FWCfgState *s = opaque;
>      uint64_t value = 0;
> -    unsigned i;
>  
> -    for (i = 0; i < size; ++i) {
> -        value = (value << 8) | fw_cfg_read(s);
> +    if (s->cur_entry != FW_CFG_INVALID && e->data) {
> +        while (size-- && s->cur_offset < e->len) {
> +            value = (value << 8) | e->data[s->cur_offset++];
> +        }
>      }
> +
> +    trace_fw_cfg_read(s, value);
>      return value;
>  }
>  
> @@ -451,11 +439,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)
> @@ -483,7 +466,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
>  };
>  
>  static const MemoryRegionOps fw_cfg_data_mem_ops = {
> -    .read = fw_cfg_data_mem_read,
> +    .read = fw_cfg_data_read,
>      .write = fw_cfg_data_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid = {
> @@ -494,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,

(1) Can you please split this patch in two? Maybe I'm just particularly
slow today, but I feel that it would help me review this patch if I
could look at each .read conversion in separation. I'd like to see that
each conversion, individually, is unobservable from the guest.

The first patch would introduce the new function and convert one of the
callbacks. The second patch would convert the other callback and remove
the old function. (Un-called static functions would break the compile,
so the removal cannot be left for a third patch.)

Alternatively, you can keep this patch as-is, but then please prove in
the commit message, in detail, why the new shared callback is
*identical* (that is, neither stricter nor laxer) to the previous
specific callback, in *both* cases. Please spare me the burden of
thinking; I should be able to read the proof from you (rather than
derive it myself), and just nod. :)

> diff --git a/trace-events b/trace-events
> index bdfe79f..a0eb1d8 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -196,7 +196,7 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read 
> diagnostic %"PRId64"= %02x
>  
>  # hw/nvram/fw_cfg.c
>  fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d"
> -fw_cfg_read(void *s, uint8_t ret) "%p = %d"
> +fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64

(2) This changes the trace format even for single byte reads (from
decimal to hex), but I think that should be fine; plus we never
guaranteed any kind of stability here. So this looks good to me.

>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd 
> bytes)"
>  
>  # hw/block/hd-geometry.c
> 

Thanks
Laszlo