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

[Laszlo Ersek <lersek@redhat.com>]

Comments below:

On 10/28/15 18:20, 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

(1) I suggest "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 called on
> each distinct item).

(2) Shouldn't this be:

    ... after the first time they are called, regardless of the
    associated item being 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 <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc Marí <markmb@redhat.com>
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c         | 16 ++++++++--------
>  include/hw/nvram/fw_cfg.h |  8 ++------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 73b0a81..31fa5c8 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];

Seems to match the logic in fw_cfg_read().

> +        if (e->read_callback) {
> +            e->read_callback(e->callback_opaque, s->cur_offset);
> +        }

The offset is constant 0 here, but that's fine.

>      }
>  
>      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.
>               */
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 422e2e9..47ff118 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -183,12 +183,8 @@ 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 once per
> - * each DMA guest read operation.
> - * 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 written to the
> + * fw_cfg control register).

(3) This should be more precise. Selection doesn't only occur via an
explicit write to the control register. Kevin suggested the
FW_CFG_DMA_CTL_SELECT bit in FWCfgDmaAccess.control, for enabling
"single trap" transfers. For the last sentence, I recommend:

  Additionally, set a callback function (and argument) to be called each
  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).

(4) Please add the following comment to the body of fw_cfg_reset():

    /* we never register a read callback for FW_CFG_SIGNATURE */

(You might want to replace the open-coded 0 argument in that
fw_cfg_select() call with FW_CFG_SIGNATURE as well.)

>   */
>  void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
>                                FWCfgReadCallback callback, void *callback_opaque,
> 

Thanks!
Laszlo