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 <ke...@koconnor.net>
> >
> > Return a static signature ("QEMU CFG") if the guest does a read to the
> > DMA address io register.
> >
> > Signed-off-by: Kevin O'Connor <ke...@koconnor.net>
> > ---
> > 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
