Merge remote-tracking branch 'remotes/kraxel/tags/pull-fw-cfg-20151217-1' into staging

fw_cfg: doc updates, various optimizations.

# gpg: Signature made Thu 17 Dec 2015 08:59:32 GMT using RSA key ID D3E87138
# gpg: Good signature from "Gerd Hoffmann (work) <kraxel@redhat.com>"
# gpg:                 aka "Gerd Hoffmann <gerd@kraxel.org>"
# gpg:                 aka "Gerd Hoffmann (private) <kraxel@gmail.com>"

* remotes/kraxel/tags/pull-fw-cfg-20151217-1:
  fw_cfg: replace ioport data read with generic method
  fw_cfg: add generic non-DMA read method
  fw_cfg: avoid calculating invalid current entry pointer
  fw_cfg: remove offset argument from callback prototype
  fw_cfg: amend callback behavior spec to once per select
  fw_cfg: move internal function call docs to header file

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

6 files changed, 165 insertions, 127 deletions

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index b8c794f546..2099ad9a4d 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -192,90 +192,7 @@ To check the result, read the "control" field:
                             today due to implementation not being async,
                             but may in the future).
 
-= Host-side API =
-
-The following functions are available to the QEMU programmer for adding
-data to a fw_cfg device during guest initialization (see fw_cfg.h for
-each function's complete prototype):
-
-== fw_cfg_add_bytes() ==
-
-Given a selector key value, starting pointer, and size, create an item
-as a raw "blob" of the given size, available by selecting the given key.
-The data referenced by the starting pointer is only linked, NOT copied,
-into the data structure of the fw_cfg device.
-
-== fw_cfg_add_string() ==
-
-Instead of a starting pointer and size, this function accepts a pointer
-to a NUL-terminated ascii string, and inserts a newly allocated copy of
-the string (including the NUL terminator) into the fw_cfg device data
-structure.
-
-== fw_cfg_add_iXX() ==
-
-Insert an XX-bit item, where XX may be 16, 32, or 64. These functions
-will convert a 16-, 32-, or 64-bit integer to little-endian, then add
-a dynamically allocated copy of the appropriately sized item to fw_cfg
-under the given selector key value.
-
-== fw_cfg_modify_iXX() ==
-
-Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
-Similarly to the corresponding fw_cfg_add_iXX() function set, convert
-a 16-, 32-, or 64-bit integer to little endian, create a dynamically
-allocated copy of the required size, and replace the existing item at
-the given selector key value with the newly allocated one. The previous
-item, assumed to have been allocated during an earlier call to
-fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
-before the function returns.
-
-== fw_cfg_add_file() ==
-
-Given a filename (i.e., fw_cfg item name), starting pointer, and size,
-create an item as a raw "blob" of the given size. Unlike fw_cfg_add_bytes()
-above, the next available selector key (above 0x0020, FW_CFG_FILE_FIRST)
-will be used, and a new entry will be added to the file directory structure
-(at key 0x0019), containing the item name, blob size, and automatically
-assigned selector key value. The data referenced by the starting pointer
-is only linked, NOT copied, into the fw_cfg data structure.
-
-== fw_cfg_add_file_callback() ==
-
-Like fw_cfg_add_file(), but additionally sets pointers to a callback
-function (and opaque argument), which will be executed host-side by
-QEMU each time a byte is read by the guest from this particular item.
-
-NOTE: The callback function is given the opaque argument set by
-fw_cfg_add_file_callback(), but also the current data offset,
-allowing it the option of only acting upon specific offset values
-(e.g., 0, before the first data byte of the selected item is
-returned to the guest).
-
-== fw_cfg_modify_file() ==
-
-Given a filename (i.e., fw_cfg item name), starting pointer, and size,
-completely replace the configuration item referenced by the given item
-name with the new given blob. If an existing blob is found, its
-callback information is removed, and a pointer to the old data is
-returned to allow the caller to free it, helping avoid memory leaks.
-If a configuration item does not already exist under the given item
-name, a new item will be created as with fw_cfg_add_file(), and NULL
-is returned to the caller. In any case, the data referenced by the
-starting pointer is only linked, NOT copied, into the fw_cfg data
-structure.
-
-== fw_cfg_add_callback() ==
-
-Like fw_cfg_add_bytes(), but additionally sets pointers to a callback
-function (and opaque argument), which will be executed host-side by
-QEMU each time a guest-side write operation to this particular item
-completes fully overwriting the item's data.
-
-NOTE: This function is deprecated, and will be completely removed
-starting with QEMU v2.4.
-
-== Externally Provided Items ==
+= Externally Provided Items =
 
 As of v2.4, "file" fw_cfg items (i.e., items with selector keys above
 FW_CFG_FILE_FIRST, and with a corresponding entry in the fw_cfg file
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3c2c5d6bfa..8fd0b1c57b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -631,7 +631,7 @@ static void acpi_ram_update(MemoryRegion *mr, GArray *data)
     memory_region_set_dirty(mr, 0, size);
 }
 
-static void virt_acpi_build_update(void *build_opaque, uint32_t offset)
+static void virt_acpi_build_update(void *build_opaque)
 {
     AcpiBuildState *build_state = build_opaque;
     AcpiBuildTables tables;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c657a7..29e30ce79a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1818,7 +1818,7 @@ static void acpi_ram_update(MemoryRegion *mr, GArray *data)
     memory_region_set_dirty(mr, 0, size);
 }
 
-static void acpi_build_update(void *build_opaque, uint32_t offset)
+static void acpi_build_update(void *build_opaque)
 {
     AcpiBuildState *build_state = build_opaque;
     AcpiBuildTables tables;
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 73b0a813a7..a1d650d5a9 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -252,7 +252,8 @@ static void fw_cfg_write(FWCfgState *s, uint8_t value)
 
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
-    int ret;
+    int arch, ret;
+    FWCfgEntry *e;
 
     s->cur_offset = 0;
     if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
@@ -261,41 +262,45 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key)
     } else {
         s->cur_entry = key;
         ret = 1;
-    }
-
-    trace_fw_cfg_select(s, key, ret);
-    return ret;
-}
-
-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 {
+        /* entry successfully selected, now run callback if present */
+        arch = !!(key & FW_CFG_ARCH_LOCAL);
+        e = &s->entries[arch][key & FW_CFG_ENTRY_MASK];
         if (e->read_callback) {
-            e->read_callback(e->callback_opaque, s->cur_offset);
+            e->read_callback(e->callback_opaque);
         }
-        ret = e->data[s->cur_offset++];
     }
 
-    trace_fw_cfg_read(s, ret);
+    trace_fw_cfg_select(s, key, ret);
     return ret;
 }
 
-static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr,
-                                     unsigned size)
+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;
-    unsigned i;
 
-    for (i = 0; i < size; ++i) {
-        value = (value << 8) | fw_cfg_read(s);
+    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;
 }
 
@@ -338,7 +343,8 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
     }
 
     arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+    e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
+        &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
 
     if (dma.control & FW_CFG_DMA_CTL_READ) {
         read = 1;
@@ -371,10 +377,6 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
                 len = (e->len - s->cur_offset);
             }
 
-            if (e->read_callback) {
-                e->read_callback(e->callback_opaque, s->cur_offset);
-            }
-
             /* If the access is not a read access, it will be a skip access,
              * tested before.
              */
@@ -451,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)
 {
@@ -483,7 +479,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 +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,
@@ -513,7 +509,8 @@ static void fw_cfg_reset(DeviceState *d)
 {
     FWCfgState *s = FW_CFG(d);
 
-    fw_cfg_select(s, 0);
+    /* we never register a read callback for FW_CFG_SIGNATURE */
+    fw_cfg_select(s, FW_CFG_SIGNATURE);
 }
 
 /* Save restore 32 bit int as uint16_t
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index ee0cd8adb3..664eaf6452 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -70,22 +70,146 @@ typedef struct FWCfgDmaAccess {
     uint64_t address;
 } QEMU_PACKED FWCfgDmaAccess;
 
-typedef void (*FWCfgCallback)(void *opaque, uint8_t *data);
-typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset);
+typedef void (*FWCfgReadCallback)(void *opaque);
 
+/**
+ * fw_cfg_add_bytes:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @data: pointer to start of item data
+ * @len: size of item data
+ *
+ * Add a new fw_cfg item, available by selecting the given key, as a raw
+ * "blob" of the given size. The data referenced by the starting pointer
+ * is only linked, NOT copied, into the data structure of the fw_cfg device.
+ */
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
+
+/**
+ * fw_cfg_add_string:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @value: NUL-terminated ascii string
+ *
+ * Add a new fw_cfg item, available by selecting the given key. The item
+ * data will consist of a dynamically allocated copy of the provided string,
+ * including its NUL terminator.
+ */
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value);
+
+/**
+ * fw_cfg_add_i16:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @value: 16-bit integer
+ *
+ * Add a new fw_cfg item, available by selecting the given key. The item
+ * data will consist of a dynamically allocated copy of the given 16-bit
+ * value, converted to little-endian representation.
+ */
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
+
+/**
+ * fw_cfg_modify_i16:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @value: 16-bit integer
+ *
+ * Replace the fw_cfg item available by selecting the given key. The new
+ * data will consist of a dynamically allocated copy of the given 16-bit
+ * value, converted to little-endian representation. The data being replaced,
+ * assumed to have been dynamically allocated during an earlier call to
+ * either fw_cfg_add_i16() or fw_cfg_modify_i16(), is freed before returning.
+ */
 void fw_cfg_modify_i16(FWCfgState *s, uint16_t key, uint16_t value);
+
+/**
+ * fw_cfg_add_i32:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @value: 32-bit integer
+ *
+ * Add a new fw_cfg item, available by selecting the given key. The item
+ * data will consist of a dynamically allocated copy of the given 32-bit
+ * value, converted to little-endian representation.
+ */
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
+
+/**
+ * fw_cfg_add_i64:
+ * @s: fw_cfg device being modified
+ * @key: selector key value for new fw_cfg item
+ * @value: 64-bit integer
+ *
+ * Add a new fw_cfg item, available by selecting the given key. The item
+ * data will consist of a dynamically allocated copy of the given 64-bit
+ * value, converted to little-endian representation.
+ */
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
+
+/**
+ * fw_cfg_add_file:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @data: pointer to start of item data
+ * @len: size of item data
+ *
+ * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
+ * referenced by the starting pointer is only linked, NOT copied, into the
+ * data structure of the fw_cfg device.
+ * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
+ * will be used; also, a new entry will be added to the file directory
+ * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
+ * data size, and assigned selector key value.
+ */
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
+
+/**
+ * fw_cfg_add_file_callback:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @callback: callback function
+ * @callback_opaque: argument to be passed into callback function
+ * @data: pointer to start of item data
+ * @len: size of item data
+ *
+ * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
+ * referenced by the starting pointer is only linked, NOT copied, into the
+ * data structure of the fw_cfg device.
+ * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
+ * will be used; also, a new entry will be added to the file directory
+ * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
+ * data size, and assigned selector key value.
+ * Additionally, set a callback function (and argument) to be called each
+ * time this item is selected (by having its selector key either written to
+ * the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.control
+ * with FW_CFG_DMA_CTL_SELECT).
+ */
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               FWCfgReadCallback callback, void *callback_opaque,
                               void *data, size_t len);
+
+/**
+ * fw_cfg_modify_file:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @data: pointer to start of item data
+ * @len: size of item data
+ *
+ * Replace a NAMED fw_cfg item. If an existing item is found, its callback
+ * information will be cleared, and a pointer to its data will be returned
+ * to the caller, so that it may be freed if necessary. If an existing item
+ * is not found, this call defaults to fw_cfg_add_file(), and NULL is
+ * returned to the caller.
+ * In either case, the new item data is only linked, NOT copied, into the
+ * data structure of the fw_cfg device.
+ *
+ * Returns: pointer to old item's data, or NULL if old item does not exist.
+ */
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
                          size_t len);
+
 FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
                                 AddressSpace *dma_as);
 FWCfgState *fw_cfg_init_io(uint32_t iobase);
diff --git a/trace-events b/trace-events
index 3686e95d98..55f5b9ed2c 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