| Message ID | 1446052836-31737-3-git-send-email-somlo@cmu.edu |
|---|---|
| State | New |
| Headers | show |
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
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 <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). 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
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]; + 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. */ 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). */ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, FWCfgReadCallback callback, void *callback_opaque,
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 called on each distinct item). 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(-)