Re: [Qemu-devel] [PATCH v4 5/6] fw_cfg: add generic non-DMA read method

[Laszlo Ersek <lersek@redhat.com>]

On 11/03/15 22:40, Gabriel L. Somlo wrote:
> Introduce fw_cfg_data_read(), a generic read method which works
> on all access widths (1 through 8 bytes, inclusive), and can be
> used during both IOPort and MMIO read accesses.
> 
> To maintain legibility, only fw_cfg_data_mem_read() (the MMIO
> data read method) is replaced by this patch. The new method
> essentially unwinds the fw_cfg_data_mem_read() + fw_cfg_read()
> combo, but without unnecessarily repeating all the validity
> checks performed by the latter on each byte being read.
> 
> This patch also modifies the trace_fw_cfg_read prototype to
> accept a 64-bit value argument, allowing it to work properly
> with the new read method, but also remain backward compatible
> with existing call sites.
> 
> 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 | 44 ++++++++++++++++++++++++++++++--------------
>  trace-events      |  2 +-
>  2 files changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 046fa74..9e01b46 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -274,6 +274,35 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
>      return ret;
>  }
>  
> +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->cur_entry == FW_CFG_INVALID) ? NULL :
> +                    &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
> +    uint64_t value = 0;
> +
> +    assert(size <= sizeof(value));
> +    if (s->cur_entry != FW_CFG_INVALID && e->data) {
> +        /* The least significant 'size' bytes of the return value are
> +         * expected to contain a string preserving portion of the item
> +         * data, padded with zeros to the right in case we run out early.
> +         */
> +        while (size && s->cur_offset < e->len) {
> +            value = (value << 8) | e->data[s->cur_offset++];
> +            size--;
> +        }
> +        /* If size is still not zero, we *did* run out early, so continue
> +         * left-shifting, to add the appropriate number of padding zeros
> +         * on the right.
> +         */
> +        value <<= 8 * size;
> +    }
> +
> +    trace_fw_cfg_read(s, value);
> +    return value;
> +}

With the wording you proposed in
<http://thread.gmane.org/gmane.comp.emulators.qemu/373165/focus=373507>,
this looks okay.

... Except my (2a) proposal wasn't entirely correct, and now you get to
fix it up for v5. :( Apologies. (It is a different experience when you
see the code in full.)

Namely, consider the case when this code is entered with:

  (size == 8 && s->cur_offset == e->len)

(Which is possible if the guest makes a qword read access just after
reading the full blob.)

In this case, the loop won't be entered at all (which is okay), but then
you'll have:

  uint64_t << 64

which is undefined behavior. ("If the value of the right operand is
negative or is greater than or equal to the width of the promoted left
operand, the behavior is undefined.")

So please protect the final shift with "if (size < 8)".

*Alternatively*, you could restrict the *outer* condition, i.e.,

  s->cur_entry != FW_CFG_INVALID && e->data

with (s->cur_offset < e->len).

... And then you can even replace the "while" with a "do" loop. (Because
both (size > 0) and (s->cur_offset < e->len) will be true if the loop is
reached at all.)

Just the code, without comments:

    assert(size <= sizeof(value));
    assert(size > 0);
    if (s->cur_entry != FW_CFG_INVALID && e->data &&
        s->cur_offset < e->len) {
        /* ... */
        do {
            value = (value << 8) | e->data[s->cur_offset++];
            size--;
        } while (size && s->cur_offset < e->len);
        /* ... */
        value <<= 8 * size;
    }

This makes it clear that "size" is strictly smaller than sizeof(value)
when the shift is reached.

I'll let you choose between the two alternatives. :)

Thanks, and I'm sorry.
Laszlo

> +
>  static uint8_t fw_cfg_read(FWCfgState *s)
>  {
>      int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> @@ -291,19 +320,6 @@ static uint8_t fw_cfg_read(FWCfgState *s)
>      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);
> -    }
> -    return value;
> -}
> -
>  static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
>                                    uint64_t value, unsigned size)
>  {
> @@ -485,7 +501,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 = {
> diff --git a/trace-events b/trace-events
> index 72136b9..5073040 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
>  fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)"
>  
>  # hw/block/hd-geometry.c
>