From 3bef7e8aab8af2f86c5785761c37e068428c689d Mon Sep 17 00:00:00 2001 From: "Gabriel L. Somlo" Date: Thu, 5 Nov 2015 09:32:48 -0500 Subject: fw_cfg: amend callback behavior spec to once per select MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, the fw_cfg internal API specifies that if an item was set up with a read callback, the callback must be run each time a byte is read from the item. This behavior is both wasteful (most items do not have a read callback set), and impractical for bulk transfers (e.g., DMA read). At the time of this writing, the only items configured with a callback are "/etc/table-loader", "/etc/acpi/tables", and "/etc/acpi/rsdp". They all share the same callback functions: virt_acpi_build_update() on ARM (in hw/arm/virt-acpi-build.c), and acpi_build_update() on i386 (in hw/i386/acpi.c). Both of these callbacks are one-shot (i.e. they return without doing anything at all after the first time they are invoked with a given build_state; since build_state is also shared across all three items mentioned above, the callback only ever runs *once*, the first time either of the listed items is read). This patch amends the specification for fw_cfg_add_file_callback() to state that any available read callback will only be invoked once each time the item is selected. This change has no practical effect on the current behavior of QEMU, and it enables us to significantly optimize the behavior of fw_cfg reads during guest firmware setup, eliminating a large amount of redundant callback checks and invocations. Cc: Laszlo Ersek Cc: Gerd Hoffmann Cc: Marc Marí Signed-off-by: Gabriel Somlo Reviewed-by: Laszlo Ersek Message-id: 1446733972-1602-3-git-send-email-somlo@cmu.edu Signed-off-by: Gerd Hoffmann --- hw/nvram/fw_cfg.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'hw') diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 73b0a81..6e6414b 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,6 +262,12 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) } else { s->cur_entry = key; ret = 1; + /* 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); + } } trace_fw_cfg_select(s, key, ret); @@ -276,9 +283,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len) ret = 0; else { - if (e->read_callback) { - e->read_callback(e->callback_opaque, s->cur_offset); - } ret = e->data[s->cur_offset++]; } @@ -371,10 +375,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. */ @@ -513,7 +513,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 -- cgit v1.1 From 3f8752b4e5a3871f0d2963ab889be937d9a4226a Mon Sep 17 00:00:00 2001 From: "Gabriel L. Somlo" Date: Thu, 5 Nov 2015 09:32:49 -0500 Subject: fw_cfg: remove offset argument from callback prototype MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read callbacks are now only invoked at item selection, before any data is read. As such, the value of the offset argument passed to the callback will always be 0. Also, the two callback instances currently in use both leave their offset argument unused. This patch removes the offset argument from the fw_cfg read callback prototype, and from the currently available instances. The unused (write) callback prototype is also removed (write support was removed earlier, in commit 023e3148). Cc: Laszlo Ersek Cc: Gerd Hoffmann Cc: Marc Marí Signed-off-by: Gabriel Somlo Reviewed-by: Laszlo Ersek Message-id: 1446733972-1602-4-git-send-email-somlo@cmu.edu Signed-off-by: Gerd Hoffmann --- hw/arm/virt-acpi-build.c | 2 +- hw/i386/acpi-build.c | 2 +- hw/nvram/fw_cfg.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) (limited to 'hw') diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 3c2c5d6..8fd0b1c 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 95e0c65..29e30ce 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 6e6414b..c2d3a0a 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -266,7 +266,7 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) 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); } } -- cgit v1.1 From 66f8fd9dda312191b78d2a2ba2848bcee76127a2 Mon Sep 17 00:00:00 2001 From: "Gabriel L. Somlo" Date: Thu, 5 Nov 2015 09:32:50 -0500 Subject: fw_cfg: avoid calculating invalid current entry pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When calculating a pointer to the currently selected fw_cfg item, the following is used: FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; When s->cur_entry is FW_CFG_INVALID, we are calculating the address of a non-existent element in s->entries[arch][...], which is undefined. This patch ensures the resulting entry pointer is set to NULL whenever s->cur_entry is FW_CFG_INVALID. Reported-by: Laszlo Ersek Reviewed-by: Laszlo Ersek Signed-off-by: Gabriel Somlo Message-id: 1446733972-1602-5-git-send-email-somlo@cmu.edu Cc: Marc Marí Signed-off-by: Gabriel Somlo Reviewed-by: Laszlo Ersek Signed-off-by: Gerd Hoffmann --- hw/nvram/fw_cfg.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'hw') diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index c2d3a0a..046fa74 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -277,7 +277,8 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) 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]; + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : + &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) @@ -342,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; -- cgit v1.1 From 38bf20931afe761fccda6e1eb91d64c7498ed9c9 Mon Sep 17 00:00:00 2001 From: "Gabriel L. Somlo" Date: Thu, 5 Nov 2015 09:32:51 -0500 Subject: fw_cfg: add generic non-DMA read method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce fw_cfg_data_read(), a generic read method which works on all access widths (1 through 8 bytes, inclusive), and can be used during both IOPort and MMIO read accesses. To maintain legibility, only fw_cfg_data_mem_read() (the MMIO data read method) is replaced by this patch. The new method essentially unwinds the fw_cfg_data_mem_read() + fw_cfg_read() combo, but without unnecessarily repeating all the validity checks performed by the latter on each byte being read. This patch also modifies the trace_fw_cfg_read prototype to accept a 64-bit value argument, allowing it to work properly with the new read method, but also remain backward compatible with existing call sites. Cc: Laszlo Ersek Cc: Gerd Hoffmann Cc: Marc Marí Signed-off-by: Gabriel Somlo Reviewed-by: Laszlo Ersek Message-id: 1446733972-1602-6-git-send-email-somlo@cmu.edu Signed-off-by: Gerd Hoffmann --- hw/nvram/fw_cfg.c | 45 +++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) (limited to 'hw') diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 046fa74..8f566f6 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -274,6 +274,36 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) return ret; } +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; + + 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; +} + static uint8_t fw_cfg_read(FWCfgState *s) { int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); @@ -291,19 +321,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) return ret; } -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, - unsigned size) -{ - FWCfgState *s = opaque; - uint64_t value = 0; - unsigned i; - - for (i = 0; i < size; ++i) { - value = (value << 8) | fw_cfg_read(s); - } - return value; -} - static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { @@ -485,7 +502,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 = { -- cgit v1.1 From 6c8d56a2e95712a6206a2671d2b04b2e59cabc0b Mon Sep 17 00:00:00 2001 From: "Gabriel L. Somlo" Date: Thu, 5 Nov 2015 09:32:52 -0500 Subject: fw_cfg: replace ioport data read with generic method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IOPort read access is limited to one byte at a time by fw_cfg_comb_valid(). As such, fw_cfg_comb_read() may safely ignore its size argument (which will always be 1), and simply call its fw_cfg_read() helper function once, returning 8 bits via the least significant byte of a 64-bit return value. This patch replaces fw_cfg_comb_read() with the generic method fw_cfg_data_read(), and removes the unused fw_cfg_read() helper. When called with size = 1, fw_cfg_data_read() acts exactly like fw_cfg_read(), performing the same set of sanity checks, and executing the while loop at most once (subject to the current read offset being within range). Cc: Laszlo Ersek Cc: Gerd Hoffmann Cc: Marc Marí Signed-off-by: Gabriel Somlo Message-id: 1446733972-1602-7-git-send-email-somlo@cmu.edu Reviewed-by: Laszlo Ersek Signed-off-by: Gerd Hoffmann --- hw/nvram/fw_cfg.c | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) (limited to 'hw') diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 8f566f6..a1d650d 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -304,23 +304,6 @@ static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) return value; } -static uint8_t fw_cfg_read(FWCfgState *s) -{ - 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]; - uint8_t ret; - - if (s->cur_entry == FW_CFG_INVALID || !e->data || s->cur_offset >= e->len) - ret = 0; - else { - ret = e->data[s->cur_offset++]; - } - - trace_fw_cfg_read(s, ret); - return ret; -} - static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { @@ -470,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) { @@ -513,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, -- cgit v1.1