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

[Gabriel L. Somlo]

On Mon, Nov 02, 2015 at 03:16:47PM +0100, Laszlo Ersek wrote:
> 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".

OK.

> 
> > (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

Ha, you're right -- build-state is shared across all blobs, so the
callback only really fires once for the whole guest VM.

Thanks for catching that !

> > 
> > 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         | 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).

OK.

> 
> (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.)

Done.

Thanks,
--Gabriel

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