Re: [Qemu-devel] [PATCH v4 7/7] fw_cfg: Define a static signature to be returned on DMA port reads

[Kevin O'Connor]

On Thu, Oct 01, 2015 at 06:07:06PM +0200, Laszlo Ersek wrote:
> On 10/01/15 14:16, Marc Marí wrote:
> > From: Kevin O'Connor <address@hidden>
> > 
> > Return a static signature ("QEMU CFG") if the guest does a read to the
> > DMA address io register.
> > 
> > Signed-off-by: Kevin O'Connor <address@hidden>
> > ---
> >  docs/specs/fw_cfg.txt |  4 ++++
> >  hw/nvram/fw_cfg.c     | 13 +++++++++++--
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 2d6b2da..249b99e 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000 
> > (FW_CFG_SIGNATURE),
> >  and reading four bytes from the data register. If the fw_cfg device is
> >  present, the four bytes read will contain the characters "QEMU".
> >  
> > +Additionaly, if the DMA interface is available then a read to the DMA
> > +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian
> > +format).
> 
> I suggest:
> 
>   Additionaly, if the DMA interface is available, then a read into the
>   DMA Address register will return the matching substring of the
>   "QEMU CFG" string. Characters that are at lower offsets of the
>   substring being read will be stored at lower addresses in the guest.

The interface only supports 32bit and 64bit writes, so I would suggest
the document not encourage 8bit reads.  The implementation happens to
works with 8bit reads, but that was just a side-effect.

[...]
> > @@ -393,6 +395,12 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> >      trace_fw_cfg_read(s, 0);
> >  }
> >  
> > +static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr,
> > +                                    unsigned size)
> > +{
> > +    return FW_CFG_DMA_SIGNATURE >> ((8 - addr - size) * 8);
> > +}
> > +
> 
> After re-reading the message of QEMU commit 36b62ae6a5, I think this is
> mostly correct.
> 
> (I think *technically* it is fully correct, due to the truncation that
> is implicit in the argument passing to bswapXX(), in the
> adjust_endianness() function [memory.c].)
>
> However, for the human reader's sake, I'd like to see the
> over-significant bits masked off explicitly here, after the shift.

The value has to be truncated, as a guest inl() or readl() can't
possibly return more than 32 bits.

I think the code's harder to read with the mask in place.  Are there
any helper macros or functions in QEMU to help with register
read/writes of smaller/larger sizes than expected?  Maybe the
.impl.min_access_size should just be set to 4.

[...]
> > @@ -416,8 +424,8 @@ static void fw_cfg_dma_mem_write(void *opaque, hwaddr 
> > addr,
> >  static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
> >                                    unsigned size, bool is_write)
> >  {
> > -    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
> > -                        (size == 8 && addr == 0));
> > +    return !is_write || ((size == 4 && (addr == 0 || addr == 4)) ||
> > +                         (size == 8 && addr == 0));
> >  }
> 
> This is not good enough I think; it's possible for the guest to do an
> "inl" at offset 7 into the register, which will cause undefined behavior
> in fw_cfg_dma_mem_read(). (The shift count being negative.)

The function never gets called on unaligned accesses (.impl.unaligned
!= true), so that can't happen.

-Kevin