From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45138) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtYFW-0005XR-E1 for qemu-devel@nongnu.org; Tue, 03 Nov 2015 04:51:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtYFR-0001WV-CA for qemu-devel@nongnu.org; Tue, 03 Nov 2015 04:51:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52731) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtYFR-0001WC-4v for qemu-devel@nongnu.org; Tue, 03 Nov 2015 04:51:25 -0500 References: <1446510945-18477-1-git-send-email-somlo@cmu.edu> <1446510945-18477-3-git-send-email-somlo@cmu.edu> From: Laszlo Ersek Message-ID: <56388398.3040700@redhat.com> Date: Tue, 3 Nov 2015 10:51:20 +0100 MIME-Version: 1.0 In-Reply-To: <1446510945-18477-3-git-send-email-somlo@cmu.edu> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 2/5] fw_cfg: amend callback behavior spec to once per select List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, jordan.l.justen@intel.com, markmb@redhat.com, kraxel@redhat.com, pbonzini@redhat.com 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)= . >=20 > 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 wit= h > 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). >=20 > 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. >=20 > Cc: Laszlo Ersek > Cc: Gerd Hoffmann > Cc: Marc Mar=C3=AD > Signed-off-by: Gabriel Somlo > --- > hw/nvram/fw_cfg.c | 19 ++++++++++--------- > include/hw/nvram/fw_cfg.h | 10 +++------- > 2 files changed, 13 insertions(+), 16 deletions(-) >=20 > 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 val= ue) > =20 > static int fw_cfg_select(FWCfgState *s, uint16_t key) > { > - int ret; > + int arch, ret; > + FWCfgEntry *e; > =20 > s->cur_offset =3D 0; > if ((key & FW_CFG_ENTRY_MASK) >=3D FW_CFG_MAX_ENTRY) { > @@ -261,6 +262,12 @@ static int fw_cfg_select(FWCfgState *s, uint16_t k= ey) > } else { > s->cur_entry =3D key; > ret =3D 1; > + /* entry successfully selected, now run callback if present */ > + arch =3D !!(key & FW_CFG_ARCH_LOCAL); > + e =3D &s->entries[arch][key & FW_CFG_ENTRY_MASK]; > + if (e->read_callback) { > + e->read_callback(e->callback_opaque, s->cur_offset); > + } > } > =20 > trace_fw_cfg_select(s, key, ret); > @@ -276,9 +283,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) > if (s->cur_entry =3D=3D FW_CFG_INVALID || !e->data || s->cur_offse= t >=3D e->len) > ret =3D 0; > else { > - if (e->read_callback) { > - e->read_callback(e->callback_opaque, s->cur_offset); > - } > ret =3D e->data[s->cur_offset++]; > } > =20 > @@ -371,10 +375,6 @@ static void fw_cfg_dma_transfer(FWCfgState *s) > len =3D (e->len - s->cur_offset); > } > =20 > - 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 a= ccess, > * tested before. > */ > @@ -513,7 +513,8 @@ static void fw_cfg_reset(DeviceState *d) > { > FWCfgState *s =3D FW_CFG(d); > =20 > - fw_cfg_select(s, 0); > + /* we never register a read callback for FW_CFG_SIGNATURE */ > + fw_cfg_select(s, FW_CFG_SIGNATURE); > } > =20 > /* 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 *fi= lename, void *data, > * structure residing at key value FW_CFG_FILE_DIR, containing the ite= m name, > * data size, and assigned selector key value. > * Additionally, set a callback function (and argument) to be called e= ach > - * 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 val= id > - * offset range of the item. > - * NOTE: In addition to the opaque argument set here, the callback fun= ction > - * takes the current data offset as an additional argument, allowing i= t 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 writt= en to > + * the fw_cfg control register, or passed to QEMU in FWCfgDmaAccess.co= ntrol > + * with FW_CFG_DMA_CTL_SELECT). > */ > void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > FWCfgReadCallback callback, void *callba= ck_opaque, >=20 diffed this against the v2 counterpart, looks good Reviewed-by: Laszlo Ersek