From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41153) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztxgt-0003Pv-20 for qemu-devel@nongnu.org; Wed, 04 Nov 2015 08:01:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ztxgm-0006QV-C1 for qemu-devel@nongnu.org; Wed, 04 Nov 2015 08:01:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45727) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztxgm-0006QH-2t for qemu-devel@nongnu.org; Wed, 04 Nov 2015 08:01:20 -0500 References: <1446510945-18477-1-git-send-email-somlo@cmu.edu> <1446510945-18477-5-git-send-email-somlo@cmu.edu> <56389243.4040106@redhat.com> <20151103175515.GF10717@HEDWIG.INI.CMU.EDU> <563928A8.5030907@redhat.com> <20151103220337.GH10717@HEDWIG.INI.CMU.EDU> From: Laszlo Ersek Message-ID: <563A019C.80405@redhat.com> Date: Wed, 4 Nov 2015 14:01:16 +0100 MIME-Version: 1.0 In-Reply-To: <20151103220337.GH10717@HEDWIG.INI.CMU.EDU> Content-Type: text/plain; charset=windows-1252 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" Cc: peter.maydell@linaro.org, jordan.l.justen@intel.com, qemu-devel@nongnu.org, kraxel@redhat.com, pbonzini@redhat.com, markmb@redhat.com On 11/03/15 23:03, Gabriel L. Somlo wrote: > On Tue, Nov 03, 2015 at 10:35:36PM +0100, Laszlo Ersek wrote: >> On 11/03/15 18:55, Gabriel L. Somlo wrote: >>> On Tue, Nov 03, 2015 at 11:53:55AM +0100, Laszlo Ersek wrote: >>>> 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. >>>>> >>>>> 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. >>>>> >>>>> Cc: Laszlo Ersek >>>>> Cc: Gerd Hoffmann >>>>> Cc: Marc Mar=ED >>>>> Signed-off-by: Gabriel Somlo >>>>> --- >>>>> hw/nvram/fw_cfg.c | 33 +++++++++++++++++++-------------- >>>>> trace-events | 2 +- >>>>> 2 files changed, 20 insertions(+), 15 deletions(-) >>>>> >>>>> 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 key) >>>>> return ret; >>>>> } >>>>> =20 >>>>> +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsign= ed size) >>>>> +{ >>>>> + 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_ENTR= Y_MASK]; >>>> >>>> (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. Instea= d, >>>> *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-garbag= e) >>>> SIGSEGV. >>> >>> Done. >>> >>>> >>>> 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 zer= oed >>>> if the item ended too early. Whereas this loop *stops shifting* when= the >>>> blob ends. >>> >>> D'OH!!! That should teach me to pay more attention -- thanks for >>> catching it! >>> >>>> 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, w= hen >>>> 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 firmw= are >>>> 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; >>> >>> I went with 2a. Also added a comment to make things painfully obvious >>> to any potential future archaeologists: >>> >>> +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned= size) >>> +{ >>> + FWCfgState *s =3D opaque; >>> + int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); >>> + FWCfgEntry *e =3D (s->cur_entry =3D=3D FW_CFG_INVALID) ? NULL : >>> + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MA= SK]; >>> + uint64_t value =3D 0; >>> + >>> + assert(size <=3D sizeof(value)); >>> + if (s->cur_entry !=3D FW_CFG_INVALID && e->data) { >>> + /* The least significant 'size' bytes of the return value ar= e >>> + * expected to contain a string preserving portion of the it= em >>> + * data, padded with zeros to the right in case we run out e= arly. >> >> Please say "*on* the right" here, just like it reads below (emphasis >> added only for review purposes). >=20 > Done. >=20 >> Also, while the above seems correct, I prefer my own wording from comm= it >> 3c23402d4032: >> >> The solution is to compose the host-endian representation [...] of >> the big endian interpretation [...] of the fw_cfg string [...] >> >> I'm admittedly biased (I have deep scars that read "FW CFG" if I squin= t >> ;)) -- my preference could be harder to interpret for "future >> archeologist". So I'll leave it to you whether to keep yours, pick min= e, >> or run with a mixture / union. >=20 > You mean commit 36b62ae, I think :) Sigh, yes. Sorry. Must have been multi-tasking too heavily. > I'm going to go with a "union", > since the "string preserving" verbiage is also in use (by our mutual > agreement) in docs/specs/fw_cfg.txt :) >=20 > So that comment will read: >=20 > /* The least significant 'size' bytes of the return value are > * expected to contain a string preserving portion of the item > * data, padded with zeros on the right in case we run out earl= y. > * In technical terms, we're composing the host-endian represen= tation > * of the big endian interpretation of the fw_cfg string. > */ Sounds great, thanks! Laszlo >=20 > ... when I'll send out v5. >=20 > Thanks, > --Gabriel >=20 >>> + */ >>> + while (size && s->cur_offset < e->len) { >>> + value =3D (value << 8) | e->data[s->cur_offset++]; >>> + size--; >>> + } >>> + /* If size is still not zero, we *did* run out early, so fin= ish >>> + * left-shifting to add the appropriate number of padding ze= ros >>> + * on the right. >>> + */ >>> + value <<=3D 8 * size; >>> + } >>> + >>> + trace_fw_cfg_read(s, value); >>> + return value; >>> +} >>> >>> Version 4 should be out by the end of today. >>> >>> Thanks again, >>> --Gabriel >>> >>>> >>>> (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)= "Read 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 >>>>> >>>> >>