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

[Laszlo Ersek <lersek@redhat.com>]

On 10/01/15 19:02, Kevin O'Connor wrote:
> 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 <kevin@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 <kevin@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.

Okay, I don't insist. :)

> 
> [...]
>>> @@ -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.

I'd be fine even with just a comment then. :)

> 
> [...]
>>> @@ -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.

That may be true, but -- for me at least -- that's too much to remember
(or to look up all the time). Plus, I find an explicit check more
robust, should the alignment restriction be lifted at some point.

In any case, I don't insist.

Thanks
Laszlo

> 
> -Kevin
>