From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtGFb-0001pm-SL for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:38:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtGFW-0002Zp-Rq for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:38:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47956) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtGFW-0002Zk-K1 for qemu-devel@nongnu.org; Mon, 02 Nov 2015 09:38:18 -0500 References: <1446052836-31737-1-git-send-email-somlo@cmu.edu> <1446052836-31737-5-git-send-email-somlo@cmu.edu> From: Laszlo Ersek Message-ID: <56377556.1020901@redhat.com> Date: Mon, 2 Nov 2015 15:38:14 +0100 MIME-Version: 1.0 In-Reply-To: <1446052836-31737-5-git-send-email-somlo@cmu.edu> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 4/4] fw_cfg: streamline (non-DMA) read operations 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, markmb@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, jordan.l.justen@intel.com Two comments for now: On 10/28/15 18:20, Gabriel L. Somlo wrote: > Replace fw_cfg_comb_read(), fw_cfg_data_mem_read(), and fw_cfg_read() > with a single method, fw_cfg_data_read(), which works on all possible > read sizes (single- or multi-byte). The new method also eliminates > redundant validity checks caused by multi-byte reads repeatedly invokin= g > the old single-byte fw_cfg_read() method. >=20 > Also update trace_fw_cfg_read() prototype to accept a 64-bit value > argument. >=20 > Cc: Laszlo Ersek > Cc: Gerd Hoffmann > Cc: Marc Mar=C3=AD > Signed-off-by: Gabriel Somlo > --- > hw/nvram/fw_cfg.c | 37 ++++++++++--------------------------- > trace-events | 2 +- > 2 files changed, 11 insertions(+), 28 deletions(-) >=20 > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 5de6dbc..cd477f9 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -274,32 +274,20 @@ static int fw_cfg_select(FWCfgState *s, uint16_t = key) > return ret; > } > =20 > -static uint8_t fw_cfg_read(FWCfgState *s) > +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned s= ize) > { > + FWCfgState *s =3D opaque; > int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > FWCfgEntry *e =3D &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MA= SK]; > - uint8_t ret; > - > - if (s->cur_entry =3D=3D FW_CFG_INVALID || !e->data || s->cur_offse= t >=3D e->len) > - ret =3D 0; > - else { > - ret =3D e->data[s->cur_offset++]; > - } > - > - trace_fw_cfg_read(s, ret); > - return ret; > -} > - > -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, > - unsigned size) > -{ > - FWCfgState *s =3D opaque; > uint64_t value =3D 0; > - unsigned i; > =20 > - for (i =3D 0; i < size; ++i) { > - value =3D (value << 8) | fw_cfg_read(s); > + if (s->cur_entry !=3D FW_CFG_INVALID && e->data) { > + while (size-- && s->cur_offset < e->len) { > + value =3D (value << 8) | e->data[s->cur_offset++]; > + } > } > + > + trace_fw_cfg_read(s, value); > return value; > } > =20 > @@ -451,11 +439,6 @@ static bool fw_cfg_ctl_mem_valid(void *opaque, hwa= ddr addr, > return is_write && size =3D=3D 2; > } > =20 > -static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr, > - unsigned size) > -{ > - return fw_cfg_read(opaque); > -} > =20 > static void fw_cfg_comb_write(void *opaque, hwaddr addr, > uint64_t value, unsigned size) > @@ -483,7 +466,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops =3D= { > }; > =20 > static const MemoryRegionOps fw_cfg_data_mem_ops =3D { > - .read =3D fw_cfg_data_mem_read, > + .read =3D fw_cfg_data_read, > .write =3D fw_cfg_data_mem_write, > .endianness =3D DEVICE_BIG_ENDIAN, > .valid =3D { > @@ -494,7 +477,7 @@ static const MemoryRegionOps fw_cfg_data_mem_ops =3D= { > }; > =20 > static const MemoryRegionOps fw_cfg_comb_mem_ops =3D { > - .read =3D fw_cfg_comb_read, > + .read =3D fw_cfg_data_read, > .write =3D fw_cfg_comb_write, > .endianness =3D DEVICE_LITTLE_ENDIAN, > .valid.accepts =3D fw_cfg_comb_valid, (1) Can you please split this patch in two? Maybe I'm just particularly slow today, but I feel that it would help me review this patch if I could look at each .read conversion in separation. I'd like to see that each conversion, individually, is unobservable from the guest. The first patch would introduce the new function and convert one of the callbacks. The second patch would convert the other callback and remove the old function. (Un-called static functions would break the compile, so the removal cannot be left for a third patch.) Alternatively, you can keep this patch as-is, but then please prove in the commit message, in detail, why the new shared callback is *identical* (that is, neither stricter nor laxer) to the previous specific callback, in *both* cases. Please spare me the burden of thinking; I should be able to read the proof from you (rather than derive it myself), and just nod. :) > diff --git a/trace-events b/trace-events > index bdfe79f..a0eb1d8 100644 > --- a/trace-events > +++ b/trace-events > @@ -196,7 +196,7 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Re= ad diagnostic %"PRId64"=3D %02x > =20 > # hw/nvram/fw_cfg.c > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d =3D %d" > -fw_cfg_read(void *s, uint8_t ret) "%p =3D %d" > +fw_cfg_read(void *s, uint64_t ret) "%p =3D %"PRIx64 (2) This changes the trace format even for single byte reads (from decimal to hex), but I think that should be fine; plus we never guaranteed any kind of stability here. So this looks good to me. > fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %= s (%zd bytes)" > =20 > # hw/block/hd-geometry.c >=20 Thanks Laszlo