From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35822) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtZE4-0002Pf-2B for qemu-devel@nongnu.org; Tue, 03 Nov 2015 05:54:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtZDz-0000Ko-28 for qemu-devel@nongnu.org; Tue, 03 Nov 2015 05:54:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40055) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtZDy-0000Ki-QO for qemu-devel@nongnu.org; Tue, 03 Nov 2015 05:53:58 -0500 References: <1446510945-18477-1-git-send-email-somlo@cmu.edu> <1446510945-18477-5-git-send-email-somlo@cmu.edu> From: Laszlo Ersek Message-ID: <56389243.4040106@redhat.com> Date: Tue, 3 Nov 2015 11:53:55 +0100 MIME-Version: 1.0 In-Reply-To: <1446510945-18477-5-git-send-email-somlo@cmu.edu> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 4/5] fw_cfg: add generic non-DMA read method 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 Thank you for splitting out this patch; it makes it easier to review. However, On 11/03/15 01:35, Gabriel L. Somlo wrote: > Introduce fw_cfg_data_read(), a generic read method which works > on all access widths (1 through 8 bytes, inclusive), and can be > used during both IOPort and MMIO read accesses. >=20 > To maintain legibility, only fw_cfg_data_mem_read() (the MMIO > data read method) is replaced by this patch. The new method > essentially unwinds the fw_cfg_data_mem_read() + fw_cfg_read() > combo, but without unnecessarily repeating all the validity > checks performed by the latter on each byte being read. this unwinding caused a bug to creep in. Namely, we have to identify the set of data that remains constant between *all* "size" calls that fw_cfg_data_mem_read() makes to fw_cfg_read(), and hoist / eliminate the checks on those *only*. Specifically, > This patch also modifies the trace_fw_cfg_read prototype to > accept a 64-bit value argument, allowing it to work properly > with the new read method, but also remain backward compatible > with existing call sites. >=20 > Cc: Laszlo Ersek > Cc: Gerd Hoffmann > Cc: Marc Mar=C3=AD > Signed-off-by: Gabriel Somlo > --- > hw/nvram/fw_cfg.c | 33 +++++++++++++++++++-------------- > trace-events | 2 +- > 2 files changed, 20 insertions(+), 15 deletions(-) >=20 > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index c2d3a0a..8aa980c 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -274,6 +274,24 @@ static int fw_cfg_select(FWCfgState *s, uint16_t k= ey) > return ret; > } > =20 > +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned s= ize) > +{ > + FWCfgState *s =3D opaque; This is good. > + int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); Okay too. > + FWCfgEntry *e =3D &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MA= SK]; (1) Side point: the conversion here is faithful to the original code in fw_cfg_read(), but even in the original code, the expression uses "s->cur_entry" as a (masked) subscript *before* comparing it against FW_CFG_INVALID. I don't think that's right. The same issue is present in fw_cfg_dma_transfer(). Care to write a patch (before the restructuring) that fixes both? Note, I am aware that the expression in both of the above mentioned functions only calculates the *address* of the nonexistent element belonging to (FW_CFG_INVALID & FW_CFG_ENTRY_MASK) =3D=3D 0x3FFF: e =3D &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; But it doesn't matter; it's undefined behavior just the same. Instead, *both* locations should say: e =3D (s->cur_entry =3D=3D FW_CFG_INVALID) ? NULL : &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; (I share the blame for not noticing this earlier -- I too reviewed fw_cfg_dma_transfer().) NULL is a valid pointer to *evaluate* (not to dereference), whereas the current address-of expression is not valid even for evaluation. Also, in practice, dereferencing NULL would give us a nice (as in, non-garbage) SIGSEGV. Anyway, back to the topic at hand: > + uint64_t value =3D 0; > + > + assert(size <=3D sizeof(value)); > + if (s->cur_entry !=3D FW_CFG_INVALID && e->data) { Right, good conversion. (Side note: this does protect against *dereferencing* "e", but it's already too late, as far as undefined behavior is concerned.) > + while (size-- && s->cur_offset < e->len) { > + value =3D (value << 8) | e->data[s->cur_offset++]; > + } (2) So, this is the bug. The pre-conversion code would keep shifting "value" to the left until "size" was reached, regardless of the underlying blob size, and just leave the least significant bytes zeroed if the item ended too early. Whereas this loop *stops shifting* when the blob ends. Since the wide data register (which is big-endian) implements a substring-preserving transfer (on top of QEMU's integer preserving device r/w infrastructure), this change breaks the case when the firmware reads, say, 8 bytes from the register in a single access, when only 3 are left in the blob, and then uses only the three *lowest address* bytes from the uint64_t value read. Although no known firmware does this at the moment, it would be valid, and the above hunk would break it. Hence please (2a) either append the missing "cumulative" shift after the loop: while (size && s->cur_offset < e->len) { --size; value =3D (value << 8) | e->data[s->cur_offset++]; } value <<=3D 8 * size; (2b) or move the offset check from the loop's controlling expression into the value composition: while (size--) { value =3D (value << 8) | (s->cur_offset < e->len ? e->data[s->cur_offset++] : 0); } The rest looks good. Thanks Laszlo > + } > + > + trace_fw_cfg_read(s, value); > + return value; > +} > + > static uint8_t fw_cfg_read(FWCfgState *s) > { > int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > @@ -290,19 +308,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) > return ret; > } > =20 > -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; > - > - for (i =3D 0; i < size; ++i) { > - value =3D (value << 8) | fw_cfg_read(s); > - } > - return value; > -} > - > static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, > uint64_t value, unsigned size) > { > @@ -483,7 +488,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 { > diff --git a/trace-events b/trace-events > index 72136b9..5073040 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 > 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