Message ID | 1446586842-21793-6-git-send-email-somlo@cmu.edu |
---|---|
State | New |
Headers | show |
On 11/03/15 22:40, 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 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 <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 | 44 ++++++++++++++++++++++++++++++-------------- > trace-events | 2 +- > 2 files changed, 31 insertions(+), 15 deletions(-) > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 046fa74..9e01b46 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -274,6 +274,35 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) > return ret; > } > > +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) > +{ > + FWCfgState *s = opaque; > + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : > + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > + uint64_t value = 0; > + > + assert(size <= sizeof(value)); > + if (s->cur_entry != FW_CFG_INVALID && e->data) { > + /* The least significant 'size' bytes of the return value are > + * expected to contain a string preserving portion of the item > + * data, padded with zeros to the right in case we run out early. > + */ > + while (size && s->cur_offset < e->len) { > + value = (value << 8) | e->data[s->cur_offset++]; > + size--; > + } > + /* If size is still not zero, we *did* run out early, so continue > + * left-shifting, to add the appropriate number of padding zeros > + * on the right. > + */ > + value <<= 8 * size; > + } > + > + trace_fw_cfg_read(s, value); > + return value; > +} With the wording you proposed in <http://thread.gmane.org/gmane.comp.emulators.qemu/373165/focus=373507>, this looks okay. ... Except my (2a) proposal wasn't entirely correct, and now you get to fix it up for v5. :( Apologies. (It is a different experience when you see the code in full.) Namely, consider the case when this code is entered with: (size == 8 && s->cur_offset == e->len) (Which is possible if the guest makes a qword read access just after reading the full blob.) In this case, the loop won't be entered at all (which is okay), but then you'll have: uint64_t << 64 which is undefined behavior. ("If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.") So please protect the final shift with "if (size < 8)". *Alternatively*, you could restrict the *outer* condition, i.e., s->cur_entry != FW_CFG_INVALID && e->data with (s->cur_offset < e->len). ... And then you can even replace the "while" with a "do" loop. (Because both (size > 0) and (s->cur_offset < e->len) will be true if the loop is reached at all.) Just the code, without comments: assert(size <= sizeof(value)); assert(size > 0); if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset < e->len) { /* ... */ do { value = (value << 8) | e->data[s->cur_offset++]; size--; } while (size && s->cur_offset < e->len); /* ... */ value <<= 8 * size; } This makes it clear that "size" is strictly smaller than sizeof(value) when the shift is reached. I'll let you choose between the two alternatives. :) Thanks, and I'm sorry. Laszlo > + > static uint8_t fw_cfg_read(FWCfgState *s) > { > int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > @@ -291,19 +320,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) > return ret; > } > > -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, > - unsigned size) > -{ > - FWCfgState *s = opaque; > - uint64_t value = 0; > - unsigned i; > - > - for (i = 0; i < size; ++i) { > - value = (value << 8) | fw_cfg_read(s); > - } > - return value; > -} > - > static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, > uint64_t value, unsigned size) > { > @@ -485,7 +501,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = { > }; > > static const MemoryRegionOps fw_cfg_data_mem_ops = { > - .read = fw_cfg_data_mem_read, > + .read = fw_cfg_data_read, > .write = fw_cfg_data_mem_write, > .endianness = DEVICE_BIG_ENDIAN, > .valid = { > 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"= %02x > > # hw/nvram/fw_cfg.c > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > -fw_cfg_read(void *s, uint8_t ret) "%p = %d" > +fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64 > fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" > > # hw/block/hd-geometry.c >
On Wed, Nov 04, 2015 at 04:04:09PM +0100, Laszlo Ersek wrote: > On 11/03/15 22:40, 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 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 <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 | 44 ++++++++++++++++++++++++++++++-------------- > > trace-events | 2 +- > > 2 files changed, 31 insertions(+), 15 deletions(-) > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > index 046fa74..9e01b46 100644 > > --- a/hw/nvram/fw_cfg.c > > +++ b/hw/nvram/fw_cfg.c > > @@ -274,6 +274,35 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) > > return ret; > > } > > > > +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) > > +{ > > + FWCfgState *s = opaque; > > + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > > + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : > > + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > > + uint64_t value = 0; > > + > > + assert(size <= sizeof(value)); > > + if (s->cur_entry != FW_CFG_INVALID && e->data) { > > + /* The least significant 'size' bytes of the return value are > > + * expected to contain a string preserving portion of the item > > + * data, padded with zeros to the right in case we run out early. > > + */ > > + while (size && s->cur_offset < e->len) { > > + value = (value << 8) | e->data[s->cur_offset++]; > > + size--; > > + } > > + /* If size is still not zero, we *did* run out early, so continue > > + * left-shifting, to add the appropriate number of padding zeros > > + * on the right. > > + */ > > + value <<= 8 * size; > > + } > > + > > + trace_fw_cfg_read(s, value); > > + return value; > > +} > > With the wording you proposed in > <http://thread.gmane.org/gmane.comp.emulators.qemu/373165/focus=373507>, > this looks okay. > > ... Except my (2a) proposal wasn't entirely correct, and now you get to > fix it up for v5. :( Apologies. (It is a different experience when you > see the code in full.) > > Namely, consider the case when this code is entered with: > > (size == 8 && s->cur_offset == e->len) > > (Which is possible if the guest makes a qword read access just after > reading the full blob.) > > In this case, the loop won't be entered at all (which is okay), but then > you'll have: > > uint64_t << 64 > > which is undefined behavior. ("If the value of the right operand is > negative or is greater than or equal to the width of the promoted left > operand, the behavior is undefined.") Yeah, we're hitting all the corner cases of the C standard, aren't we :) > So please protect the final shift with "if (size < 8)". > > *Alternatively*, you could restrict the *outer* condition, i.e., > > s->cur_entry != FW_CFG_INVALID && e->data > > with (s->cur_offset < e->len). > > ... And then you can even replace the "while" with a "do" loop. (Because > both (size > 0) and (s->cur_offset < e->len) will be true if the loop is > reached at all.) > > Just the code, without comments: > > assert(size <= sizeof(value)); > assert(size > 0); > if (s->cur_entry != FW_CFG_INVALID && e->data && > s->cur_offset < e->len) { > /* ... */ > do { > value = (value << 8) | e->data[s->cur_offset++]; > size--; > } while (size && s->cur_offset < e->len); > /* ... */ > value <<= 8 * size; > } > > This makes it clear that "size" is strictly smaller than sizeof(value) > when the shift is reached. > > I'll let you choose between the two alternatives. :) I like the do/while idea, so here's the new function: +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) +{ + FWCfgState *s = opaque; + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; + uint64_t value = 0; + + assert(size > 0 && size <= sizeof(value)); + if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset < e->len) { + /* 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 early. + * In technical terms, we're composing the host-endian representation + * of the big endian interpretation of the fw_cfg string. + */ + do { + value = (value << 8) | e->data[s->cur_offset++]; + } while (--size && s->cur_offset < e->len); + /* If size is still not zero, we *did* run out early, so continue + * left-shifting, to add the appropriate number of padding zeros + * on the right. + */ + value <<= 8 * size; + } + + trace_fw_cfg_read(s, value); + return value; +} > > Thanks, and I'm sorry. Thank you, and no worries -- after all, what's *my* excuse for not catching it ? :) Guess I'll put a low-pass filter on blasting out v5, given how this is a "target rich environment" for subtle C standard violations :) Cheers, --Gabriel > > > + > > static uint8_t fw_cfg_read(FWCfgState *s) > > { > > int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > > @@ -291,19 +320,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) > > return ret; > > } > > > > -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, > > - unsigned size) > > -{ > > - FWCfgState *s = opaque; > > - uint64_t value = 0; > > - unsigned i; > > - > > - for (i = 0; i < size; ++i) { > > - value = (value << 8) | fw_cfg_read(s); > > - } > > - return value; > > -} > > - > > static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, > > uint64_t value, unsigned size) > > { > > @@ -485,7 +501,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = { > > }; > > > > static const MemoryRegionOps fw_cfg_data_mem_ops = { > > - .read = fw_cfg_data_mem_read, > > + .read = fw_cfg_data_read, > > .write = fw_cfg_data_mem_write, > > .endianness = DEVICE_BIG_ENDIAN, > > .valid = { > > 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"= %02x > > > > # hw/nvram/fw_cfg.c > > fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > > -fw_cfg_read(void *s, uint8_t ret) "%p = %d" > > +fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64 > > fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" > > > > # hw/block/hd-geometry.c > > >
On 11/04/15 17:35, Gabriel L. Somlo wrote: > On Wed, Nov 04, 2015 at 04:04:09PM +0100, Laszlo Ersek wrote: >> On 11/03/15 22:40, 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 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 <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 | 44 ++++++++++++++++++++++++++++++-------------- >>> trace-events | 2 +- >>> 2 files changed, 31 insertions(+), 15 deletions(-) >>> >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >>> index 046fa74..9e01b46 100644 >>> --- a/hw/nvram/fw_cfg.c >>> +++ b/hw/nvram/fw_cfg.c >>> @@ -274,6 +274,35 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) >>> return ret; >>> } >>> >>> +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) >>> +{ >>> + FWCfgState *s = opaque; >>> + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); >>> + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : >>> + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; >>> + uint64_t value = 0; >>> + >>> + assert(size <= sizeof(value)); >>> + if (s->cur_entry != FW_CFG_INVALID && e->data) { >>> + /* The least significant 'size' bytes of the return value are >>> + * expected to contain a string preserving portion of the item >>> + * data, padded with zeros to the right in case we run out early. >>> + */ >>> + while (size && s->cur_offset < e->len) { >>> + value = (value << 8) | e->data[s->cur_offset++]; >>> + size--; >>> + } >>> + /* If size is still not zero, we *did* run out early, so continue >>> + * left-shifting, to add the appropriate number of padding zeros >>> + * on the right. >>> + */ >>> + value <<= 8 * size; >>> + } >>> + >>> + trace_fw_cfg_read(s, value); >>> + return value; >>> +} >> >> With the wording you proposed in >> <http://thread.gmane.org/gmane.comp.emulators.qemu/373165/focus=373507>, >> this looks okay. >> >> ... Except my (2a) proposal wasn't entirely correct, and now you get to >> fix it up for v5. :( Apologies. (It is a different experience when you >> see the code in full.) >> >> Namely, consider the case when this code is entered with: >> >> (size == 8 && s->cur_offset == e->len) >> >> (Which is possible if the guest makes a qword read access just after >> reading the full blob.) >> >> In this case, the loop won't be entered at all (which is okay), but then >> you'll have: >> >> uint64_t << 64 >> >> which is undefined behavior. ("If the value of the right operand is >> negative or is greater than or equal to the width of the promoted left >> operand, the behavior is undefined.") > > Yeah, we're hitting all the corner cases of the C standard, aren't we :) > >> So please protect the final shift with "if (size < 8)". >> >> *Alternatively*, you could restrict the *outer* condition, i.e., >> >> s->cur_entry != FW_CFG_INVALID && e->data >> >> with (s->cur_offset < e->len). >> >> ... And then you can even replace the "while" with a "do" loop. (Because >> both (size > 0) and (s->cur_offset < e->len) will be true if the loop is >> reached at all.) >> >> Just the code, without comments: >> >> assert(size <= sizeof(value)); >> assert(size > 0); >> if (s->cur_entry != FW_CFG_INVALID && e->data && >> s->cur_offset < e->len) { >> /* ... */ >> do { >> value = (value << 8) | e->data[s->cur_offset++]; >> size--; >> } while (size && s->cur_offset < e->len); >> /* ... */ >> value <<= 8 * size; >> } >> >> This makes it clear that "size" is strictly smaller than sizeof(value) >> when the shift is reached. >> >> I'll let you choose between the two alternatives. :) > > I like the do/while idea, so here's the new function: > > +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) > +{ > + FWCfgState *s = opaque; > + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : > + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > + uint64_t value = 0; > + > + assert(size > 0 && size <= sizeof(value)); It's a matter of taste, and I won't insist at all, just mention that I didn't write those two assert()s as separate statements :) Namely, with a conjunction (P1 && P2 && ... && Pn), you have the possibility to spell the assertion as: assert(P1); assert(P2); ... assert(Pn); And, if any one of those fails, you will know *which one*. Because the line number in the "assertion failed" message will tell you. > + if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset < e->len) { > + /* 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 early. > + * In technical terms, we're composing the host-endian representation > + * of the big endian interpretation of the fw_cfg string. > + */ > + do { > + value = (value << 8) | e->data[s->cur_offset++]; > + } while (--size && s->cur_offset < e->len); > + /* If size is still not zero, we *did* run out early, so continue > + * left-shifting, to add the appropriate number of padding zeros > + * on the right. > + */ > + value <<= 8 * size; > + } > + > + trace_fw_cfg_read(s, value); > + return value; > +} Looks good! > >> >> Thanks, and I'm sorry. > > Thank you, and no worries -- after all, what's *my* excuse for not > catching it ? :) "No interest in language lawyering", perhaps? :) > Guess I'll put a low-pass filter on blasting out v5, given how this is > a "target rich environment" for subtle C standard violations :) I think I'm ready to give my R-b for the final missing piece. (Not sure if others would like to comment as well, on v4.) Your call :) Cheers Laszlo > > Cheers, > --Gabriel > >> >>> + >>> static uint8_t fw_cfg_read(FWCfgState *s) >>> { >>> int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); >>> @@ -291,19 +320,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) >>> return ret; >>> } >>> >>> -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, >>> - unsigned size) >>> -{ >>> - FWCfgState *s = opaque; >>> - uint64_t value = 0; >>> - unsigned i; >>> - >>> - for (i = 0; i < size; ++i) { >>> - value = (value << 8) | fw_cfg_read(s); >>> - } >>> - return value; >>> -} >>> - >>> static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, >>> uint64_t value, unsigned size) >>> { >>> @@ -485,7 +501,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = { >>> }; >>> >>> static const MemoryRegionOps fw_cfg_data_mem_ops = { >>> - .read = fw_cfg_data_mem_read, >>> + .read = fw_cfg_data_read, >>> .write = fw_cfg_data_mem_write, >>> .endianness = DEVICE_BIG_ENDIAN, >>> .valid = { >>> 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"= %02x >>> >>> # hw/nvram/fw_cfg.c >>> fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" >>> -fw_cfg_read(void *s, uint8_t ret) "%p = %d" >>> +fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64 >>> fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" >>> >>> # hw/block/hd-geometry.c >>> >>
Laszlo Ersek <lersek@redhat.com> writes: > On 11/04/15 17:35, Gabriel L. Somlo wrote: [...] >> + assert(size > 0 && size <= sizeof(value)); > > It's a matter of taste, and I won't insist at all, just mention that I > didn't write those two assert()s as separate statements :) > > Namely, with a conjunction (P1 && P2 && ... && Pn), you have the > possibility to spell the assertion as: > > assert(P1); > assert(P2); > ... > assert(Pn); > > And, if any one of those fails, you will know *which one*. Because the > line number in the "assertion failed" message will tell you. Yes, matter of taste, but that can't stop me having opinions on matters of taste ;) You pay for the more detailed assertion failure reporting with extra source code clutter (as written, it's immediately obvious that it's a bounds check, less so if split), and extra object code when NDEBUG is off (which it should always be). Personally, I'm content to fish details out of a core dump. YMMV. [...]
On 11/05/15 13:57, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: > >> On 11/04/15 17:35, Gabriel L. Somlo wrote: > [...] >>> + assert(size > 0 && size <= sizeof(value)); >> >> It's a matter of taste, and I won't insist at all, just mention that I >> didn't write those two assert()s as separate statements :) >> >> Namely, with a conjunction (P1 && P2 && ... && Pn), you have the >> possibility to spell the assertion as: >> >> assert(P1); >> assert(P2); >> ... >> assert(Pn); >> >> And, if any one of those fails, you will know *which one*. Because the >> line number in the "assertion failed" message will tell you. > > Yes, matter of taste, but that can't stop me having opinions on matters > of taste ;) > > You pay for the more detailed assertion failure reporting with extra > source code clutter (as written, it's immediately obvious that it's a > bounds check, less so if split), and extra object code when NDEBUG is > off (which it should always be). > > Personally, I'm content to fish details out of a core dump. YMMV. The 12 GB core dump that the Launchpad user, reporting the bug from the other side of the Earth, failed to save? :) But, I do concede your point. (Line numbers aren't a panacea either, in situations like the above.) Gabriel, please pick whichever format you like more. Thanks Laszlo
On Thu, Nov 05, 2015 at 01:23:15PM +0100, Laszlo Ersek wrote: > On 11/04/15 17:35, Gabriel L. Somlo wrote: > > On Wed, Nov 04, 2015 at 04:04:09PM +0100, Laszlo Ersek wrote: > >> On 11/03/15 22:40, 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 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 <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 | 44 ++++++++++++++++++++++++++++++-------------- > >>> trace-events | 2 +- > >>> 2 files changed, 31 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > >>> index 046fa74..9e01b46 100644 > >>> --- a/hw/nvram/fw_cfg.c > >>> +++ b/hw/nvram/fw_cfg.c > >>> @@ -274,6 +274,35 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) > >>> return ret; > >>> } > >>> > >>> +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) > >>> +{ > >>> + FWCfgState *s = opaque; > >>> + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > >>> + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : > >>> + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > >>> + uint64_t value = 0; > >>> + > >>> + assert(size <= sizeof(value)); > >>> + if (s->cur_entry != FW_CFG_INVALID && e->data) { > >>> + /* The least significant 'size' bytes of the return value are > >>> + * expected to contain a string preserving portion of the item > >>> + * data, padded with zeros to the right in case we run out early. > >>> + */ > >>> + while (size && s->cur_offset < e->len) { > >>> + value = (value << 8) | e->data[s->cur_offset++]; > >>> + size--; > >>> + } > >>> + /* If size is still not zero, we *did* run out early, so continue > >>> + * left-shifting, to add the appropriate number of padding zeros > >>> + * on the right. > >>> + */ > >>> + value <<= 8 * size; > >>> + } > >>> + > >>> + trace_fw_cfg_read(s, value); > >>> + return value; > >>> +} > >> > >> With the wording you proposed in > >> <http://thread.gmane.org/gmane.comp.emulators.qemu/373165/focus=373507>, > >> this looks okay. > >> > >> ... Except my (2a) proposal wasn't entirely correct, and now you get to > >> fix it up for v5. :( Apologies. (It is a different experience when you > >> see the code in full.) > >> > >> Namely, consider the case when this code is entered with: > >> > >> (size == 8 && s->cur_offset == e->len) > >> > >> (Which is possible if the guest makes a qword read access just after > >> reading the full blob.) > >> > >> In this case, the loop won't be entered at all (which is okay), but then > >> you'll have: > >> > >> uint64_t << 64 > >> > >> which is undefined behavior. ("If the value of the right operand is > >> negative or is greater than or equal to the width of the promoted left > >> operand, the behavior is undefined.") > > > > Yeah, we're hitting all the corner cases of the C standard, aren't we :) > > > >> So please protect the final shift with "if (size < 8)". > >> > >> *Alternatively*, you could restrict the *outer* condition, i.e., > >> > >> s->cur_entry != FW_CFG_INVALID && e->data > >> > >> with (s->cur_offset < e->len). > >> > >> ... And then you can even replace the "while" with a "do" loop. (Because > >> both (size > 0) and (s->cur_offset < e->len) will be true if the loop is > >> reached at all.) > >> > >> Just the code, without comments: > >> > >> assert(size <= sizeof(value)); > >> assert(size > 0); > >> if (s->cur_entry != FW_CFG_INVALID && e->data && > >> s->cur_offset < e->len) { > >> /* ... */ > >> do { > >> value = (value << 8) | e->data[s->cur_offset++]; > >> size--; > >> } while (size && s->cur_offset < e->len); > >> /* ... */ > >> value <<= 8 * size; > >> } > >> > >> This makes it clear that "size" is strictly smaller than sizeof(value) > >> when the shift is reached. > >> > >> I'll let you choose between the two alternatives. :) > > > > I like the do/while idea, so here's the new function: > > > > +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) > > +{ > > + FWCfgState *s = opaque; > > + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > > + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : > > + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > > + uint64_t value = 0; > > + > > + assert(size > 0 && size <= sizeof(value)); > > It's a matter of taste, and I won't insist at all, just mention that I > didn't write those two assert()s as separate statements :) > > Namely, with a conjunction (P1 && P2 && ... && Pn), you have the > possibility to spell the assertion as: > > assert(P1); > assert(P2); > ... > assert(Pn); > > And, if any one of those fails, you will know *which one*. Because the > line number in the "assertion failed" message will tell you. Ah, good point! My original instinct was to blend in with the existing style as much as possible (many other places where several &&s are packed into a single assert). But now that you spelled it out, it's a good case for NOT doing that, so I'll add this to my list of "substance over style" exceptions to watch out for :) > > + if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset < e->len) { > > + /* 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 early. > > + * In technical terms, we're composing the host-endian representation > > + * of the big endian interpretation of the fw_cfg string. > > + */ > > + do { > > + value = (value << 8) | e->data[s->cur_offset++]; > > + } while (--size && s->cur_offset < e->len); > > + /* If size is still not zero, we *did* run out early, so continue > > + * left-shifting, to add the appropriate number of padding zeros > > + * on the right. > > + */ > > + value <<= 8 * size; > > + } > > + > > + trace_fw_cfg_read(s, value); > > + return value; > > +} > > Looks good! > > > > >> > >> Thanks, and I'm sorry. > > > > Thank you, and no worries -- after all, what's *my* excuse for not > > catching it ? :) > > "No interest in language lawyering", perhaps? :) Oh, but it's the fine print where most interesting things always happen :) > > Guess I'll put a low-pass filter on blasting out v5, given how this is > > a "target rich environment" for subtle C standard violations :) > > I think I'm ready to give my R-b for the final missing piece. (Not sure > if others would like to comment as well, on v4.) Your call :) OK, coming right up! Thanks again, --Gabriel > >>> static uint8_t fw_cfg_read(FWCfgState *s) > >>> { > >>> int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > >>> @@ -291,19 +320,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) > >>> return ret; > >>> } > >>> > >>> -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, > >>> - unsigned size) > >>> -{ > >>> - FWCfgState *s = opaque; > >>> - uint64_t value = 0; > >>> - unsigned i; > >>> - > >>> - for (i = 0; i < size; ++i) { > >>> - value = (value << 8) | fw_cfg_read(s); > >>> - } > >>> - return value; > >>> -} > >>> - > >>> static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, > >>> uint64_t value, unsigned size) > >>> { > >>> @@ -485,7 +501,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = { > >>> }; > >>> > >>> static const MemoryRegionOps fw_cfg_data_mem_ops = { > >>> - .read = fw_cfg_data_mem_read, > >>> + .read = fw_cfg_data_read, > >>> .write = fw_cfg_data_mem_write, > >>> .endianness = DEVICE_BIG_ENDIAN, > >>> .valid = { > >>> 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"= %02x > >>> > >>> # hw/nvram/fw_cfg.c > >>> fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" > >>> -fw_cfg_read(void *s, uint8_t ret) "%p = %d" > >>> +fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64 > >>> fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" > >>> > >>> # hw/block/hd-geometry.c > >>> > >> >
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 046fa74..9e01b46 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -274,6 +274,35 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key) return ret; } +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned size) +{ + FWCfgState *s = opaque; + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); + FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL : + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; + uint64_t value = 0; + + assert(size <= sizeof(value)); + if (s->cur_entry != FW_CFG_INVALID && e->data) { + /* The least significant 'size' bytes of the return value are + * expected to contain a string preserving portion of the item + * data, padded with zeros to the right in case we run out early. + */ + while (size && s->cur_offset < e->len) { + value = (value << 8) | e->data[s->cur_offset++]; + size--; + } + /* If size is still not zero, we *did* run out early, so continue + * left-shifting, to add the appropriate number of padding zeros + * on the right. + */ + value <<= 8 * size; + } + + trace_fw_cfg_read(s, value); + return value; +} + static uint8_t fw_cfg_read(FWCfgState *s) { int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL); @@ -291,19 +320,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) return ret; } -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, - unsigned size) -{ - FWCfgState *s = opaque; - uint64_t value = 0; - unsigned i; - - for (i = 0; i < size; ++i) { - value = (value << 8) | fw_cfg_read(s); - } - return value; -} - static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { @@ -485,7 +501,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops = { }; static const MemoryRegionOps fw_cfg_data_mem_ops = { - .read = fw_cfg_data_mem_read, + .read = fw_cfg_data_read, .write = fw_cfg_data_mem_write, .endianness = DEVICE_BIG_ENDIAN, .valid = { 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"= %02x # hw/nvram/fw_cfg.c fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d = %d" -fw_cfg_read(void *s, uint8_t ret) "%p = %d" +fw_cfg_read(void *s, uint64_t ret) "%p = %"PRIx64 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s (%zd bytes)" # hw/block/hd-geometry.c
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 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 <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 | 44 ++++++++++++++++++++++++++++++-------------- trace-events | 2 +- 2 files changed, 31 insertions(+), 15 deletions(-)