On 11/03/15 01:35, Gabriel L. Somlo wrote:
> 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 <ler...@redhat.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Marc Marí <mar...@redhat.com>
> Signed-off-by: Gabriel Somlo <so...@cmu.edu>
> ---
> hw/nvram/fw_cfg.c | 19 ++++++++++---------
> include/hw/nvram/fw_cfg.h | 10 +++-------
> 2 files changed, 13 insertions(+), 16 deletions(-)
>
> 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
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 4b5e196..a1cfaa4 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -183,13 +183,9 @@ void fw_cfg_add_file(FWCfgState *s, const char
> *filename, void *data,
> * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
> * data size, and assigned selector key value.
> * Additionally, set a callback function (and argument) to be called each
> - * time a byte is read by the guest from this particular item, or, in the
> - * case of DMA, each time a read or skip request overlaps with the valid
> - * offset range of the item.
> - * NOTE: In addition to the opaque argument set here, the callback function
> - * takes the current data offset as an additional argument, allowing it the
> - * option of only acting upon specific offset values (e.g., 0, before the
> - * first data byte of the selected item is returned to the guest).
> + * time this item is selected (by having its selector key either written to
> + * the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.control
> + * with FW_CFG_DMA_CTL_SELECT).
> */
> void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
> FWCfgReadCallback callback, void
> *callback_opaque,
>
diffed this against the v2 counterpart, looks good
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
