[Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes

[Gabriel L. Somlo]

The fw_cfg device allowed guest-side data writes to overwrite the
selected entry in place, without allowing modification to the size
of the entry, and with the ability to invoke a callback each time
the entry was overwritten completely.

To date, we are not aware of any use case which relies on the guest's
ability to (over)write any given fw_cfg entry, and QEMU does not
register any fw_cfg write callbacks.

This patch removes the unused code path for registering fw_cfg write
callbacks, and also removes support for guest-side data writes to the
fw_cfg device.

Signed-off-by: Gabriel Somlo <address@hidden>
---
 docs/specs/fw_cfg.txt     | 21 ++++------------
 hw/nvram/fw_cfg.c         | 61 +++--------------------------------------------
 include/hw/nvram/fw_cfg.h |  4 +---
 3 files changed, 8 insertions(+), 78 deletions(-)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 7d156b7..01955dd 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -22,17 +22,9 @@ generic configuration items are accessed when the index is 
between
 0x0000-0x7fff, and architecture specific configuration items are
 accessed when the index is between 0x8000-0xffff.)
 
-Bit14 of the index value indicates if the configuration setting is
-being written.  If bit14 of the index is 0, then the item is only
-being read, and all write access to the data port will be completely
-ignored.  If bit14 of the index is 1, then the item's data can be
-written to by writing to the data port.  (In other words,
-configuration write mode is enabled when the index is between
-0x4000-0x7fff or 0xc000-0xffff.)
-
 == Data Port ==
 * One byte in width
-* Read + Write
+* Read only
 * Can be an I/O port and/or Memory-Mapped I/O
 * Location is platform dependent
 
@@ -41,24 +33,19 @@ configuration data item.  This item is selected by a write 
to the
 index port.
 
 Initially following a write to the index port, the data offset will
-be set to zero.  Each successful read or write to the data port will
+be set to zero.  Each successful read to the data port will
 cause the data offset to increment by one byte.  There is only one
 data offset value, and it will be incremented by accesses to any of
-the I/O or MMIO data ports.  A write access will not increment the
-data offset if the selected index did not have bit14 set.
+the I/O or MMIO data ports.
 
 Each firmware configuration item has a maximum length of data
 associated with the item.  After the data offset has passed the
 end of this maximum data length, then any reads will return a data
-value of 0x00, and all writes will be ignored.
+value of 0x00.
 
 A read of the data port will return the current byte of the firmware
 configuration item.
 
-A write of the data port will set the current byte of the firmware
-configuration item.  A write access will not impact the firmware
-configuration data if the selected index did not have bit14 set.
-
 == x86, x86-64 Ports ==
 
 I/O  Index Port: 0x510
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 78a37be..86090f3 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -46,7 +46,6 @@ typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;
     void *callback_opaque;
-    FWCfgCallback callback;
     FWCfgReadCallback read_callback;
 } FWCfgEntry;
 
@@ -230,23 +229,6 @@ static void fw_cfg_reboot(FWCfgState *s)
     fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4), 4);
 }
 
-static void fw_cfg_write(FWCfgState *s, uint8_t value)
-{
-    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
-    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
-
-    trace_fw_cfg_write(s, value);
-
-    if (s->cur_entry & FW_CFG_WRITE_CHANNEL && e->callback &&
-        s->cur_offset < e->len) {
-        e->data[s->cur_offset++] = value;
-        if (s->cur_offset == e->len) {
-            e->callback(e->callback_opaque, e->data);
-            s->cur_offset = 0;
-        }
-    }
-}
-
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
     int ret;
@@ -296,21 +278,10 @@ static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr 
addr,
     return value;
 }
 
-static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
-                                  uint64_t value, unsigned size)
-{
-    FWCfgState *s = opaque;
-    unsigned i = size;
-
-    do {
-        fw_cfg_write(s, value >> (8 * --i));
-    } while (i);
-}
-
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return addr == 0;
+    return addr == 0 && !is_write;
 }
 
 static void fw_cfg_ctl_mem_write(void *opaque, hwaddr addr,
@@ -334,20 +305,13 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr 
addr,
 static void fw_cfg_comb_write(void *opaque, hwaddr addr,
                               uint64_t value, unsigned size)
 {
-    switch (size) {
-    case 1:
-        fw_cfg_write(opaque, (uint8_t)value);
-        break;
-    case 2:
-        fw_cfg_select(opaque, (uint16_t)value);
-        break;
-    }
+    fw_cfg_select(opaque, (uint16_t)value);
 }
 
 static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {
-    return (size == 1) || (is_write && size == 2);
+    return (!is_write && size == 1) || (is_write && size == 2);
 }
 
 static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
@@ -358,7 +322,6 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = {
 
 static const MemoryRegionOps fw_cfg_data_mem_ops = {
     .read = fw_cfg_data_mem_read,
-    .write = fw_cfg_data_mem_write,
     .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
         .min_access_size = 1,
@@ -458,7 +421,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, 
uint16_t key,
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
     s->entries[arch][key].callback_opaque = NULL;
-    s->entries[arch][key].callback = NULL;
 
     return ptr;
 }
@@ -502,23 +464,6 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t 
value)
     fw_cfg_add_bytes(s, key, copy, sizeof(value));
 }
 
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len)
-{
-    int arch = !!(key & FW_CFG_ARCH_LOCAL);
-
-    assert(key & FW_CFG_WRITE_CHANNEL);
-
-    key &= FW_CFG_ENTRY_MASK;
-
-    assert(key < FW_CFG_MAX_ENTRY && len <= UINT32_MAX);
-
-    s->entries[arch][key].data = data;
-    s->entries[arch][key].len = (uint32_t)len;
-    s->entries[arch][key].callback_opaque = callback_opaque;
-    s->entries[arch][key].callback = callback;
-}
-
 void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgReadCallback callback, void 
*callback_opaque,
                               void *data, size_t len)
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 6d8a8ac..f11d7d5 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -40,7 +40,7 @@
 #define FW_CFG_FILE_SLOTS       0x10
 #define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST+FW_CFG_FILE_SLOTS)
 
-#define FW_CFG_WRITE_CHANNEL    0x4000
+#define FW_CFG_WRITE_CHANNEL    0x4000 /* reserved (not implemented) */
 #define FW_CFG_ARCH_LOCAL       0x8000
 #define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
 
@@ -69,8 +69,6 @@ void fw_cfg_add_string(FWCfgState *s, uint16_t key, const 
char *value);
 void fw_cfg_add_i16(FWCfgState *s, uint16_t key, uint16_t value);
 void fw_cfg_add_i32(FWCfgState *s, uint16_t key, uint32_t value);
 void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
-void fw_cfg_add_callback(FWCfgState *s, uint16_t key, FWCfgCallback callback,
-                         void *callback_opaque, void *data, size_t len);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
-- 
2.1.0