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

[Gabriel L. Somlo]

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 <address@hidden>
Cc: Gerd Hoffmann <address@hidden>
Cc: Marc Marí <address@hidden>
Signed-off-by: Gabriel Somlo <address@hidden>
---

I think in this case I'll stick with a single assert for the
whole bounds check. In the long run, "easy to read" might be
just a bit more helpful than "in the VERY unlikely event of
actually triggering this, it will also tell us which side of
the interval we fell out on"...

Thanks again for all the advice and feedback!
  --Gabriel

 hw/nvram/fw_cfg.c | 45 +++++++++++++++++++++++++++++++--------------
 trace-events      |  2 +-
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 046fa74..8f566f6 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -274,6 +274,36 @@ 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 > 0 && size <= sizeof(value));
+    if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset < e->len) {
+        /* The least significant 'size' bytes of the return value are
+         * expected to contain a string preserving portion of the item
+         * data, padded with zeros on the right in case we run out early.
+         * In technical terms, we're composing the host-endian representation
+         * of the big endian interpretation of the fw_cfg string.
+         */
+        do {
+            value = (value << 8) | e->data[s->cur_offset++];
+        } while (--size && s->cur_offset < e->len);
+        /* 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;
+}
+
 static uint8_t fw_cfg_read(FWCfgState *s)
 {
     int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
@@ -291,19 +321,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 +502,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 17fbddf..f2f2ce4 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
-- 
2.4.3