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

[Laszlo Ersek <lersek@redhat.com>]

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 <somlo@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