[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes |
Date: |
Mon, 16 Mar 2015 18:02:45 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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 <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,
>
- 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
- [Qemu-devel] [PATCH 0/6] fw-cfg: documentation, cleanup, and proposed feature, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 1/6] fw_cfg: add documentation file (docs/specs/fw_cfg.txt), Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 4/6] fw_cfg: exit with error when dupe fw_cfg file name inserted, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 3/6] fw_cfg: assertion to detect memory leak when adding new data blob, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes, Gabriel L. Somlo, 2015/03/16
- Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes,
Laszlo Ersek <=
- Re: [Qemu-devel] [PATCH 0/6] fw-cfg: documentation, cleanup, and proposed feature, Patchew Tool, 2015/03/16