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 <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.
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
>
