On 03/16/15 15:15, Gabriel L. Somlo wrote: > 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 <so...@cmu.edu> > --- > 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, >
- I mostly ignored the documentation hunks, due to my comments for patch 1/6 (ie. it should be reworked first). - The code hunks seem okay to me. But, you also remove a trace call (trace_fw_cfg_write), so the corresponding trace point should be dropped from the file "trace-events". - I can't tell of the top of my head what happens if the guest attempts an fw_cfg data write nonetheless. I vaguely recall some unassigned-io stuff from MemoryRegion land, which ultimately renders the guest access effect-less. Can anyone please test / confirm / explain that? Hm, the new validity checks should catch those: memory_region_dispatch_write() memory_region_access_valid() mr->ops->valid.accepts() unassigned_mem_write() cpu_unassigned_access() cc->do_unassigned_access() Seems to land in the CPUClass.do_unassigned_access() member, if there is one. Hm. The intersection between "has non-NULL do_unassigned_access()" and "uses fw_cfg" seems to SPARC. See: - sparc_cpu_unassigned_access() in "target-sparc/ldst_helper.c", - fw_cfg_init_mem() in "hw/sparc/sun4m.c", - fw_cfg_init_io() in "hw/sparc64/sun4u.c". I don't have the slightest clue if we should care, but *theoretically*, this change could turn guest code (ie. fw_cfg data writes) that used to do "nothing" into traps. Someone else will have to chime in on that. Oh why is nothing simple. :( Thanks Laszlo