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