qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 5/6] fw_cfg: add generic non-DMA read method


From: Gabriel L. Somlo
Subject: Re: [Qemu-devel] [PATCH v4 5/6] fw_cfg: add generic non-DMA read method
Date: Thu, 5 Nov 2015 08:54:00 -0500
User-agent: Mutt/1.5.24 (2015-08-30)

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 <address@hidden>
> >>> Cc: Gerd Hoffmann <address@hidden>
> >>> Cc: Marc MarĂ­ <address@hidden>
> >>> Signed-off-by: Gabriel Somlo <address@hidden>
> >>> ---
> >>>  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
> >>>
> >>
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]