Re: [Qemu-devel] [PATCH v3 2/5] fw_cfg: amend callback behavior spec to once per select

[Laszlo Ersek]

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 <address@hidden>
> Cc: Gerd Hoffmann <address@hidden>
> Cc: Marc Marí <address@hidden>
> Signed-off-by: Gabriel Somlo <address@hidden>
> ---
>  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 <address@hidden>