[v4,7/7] fw_cfg: Define a static signature to be returned on DMA port reads

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(-)

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.

> +
>  === Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
>  
>  A 32-bit little-endian unsigned int, this item is used to check for enabled
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 59933b3..c6dcce4 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -53,6 +53,8 @@
>  #define FW_CFG_DMA_CTL_SKIP    0x04
>  #define FW_CFG_DMA_CTL_SELECT  0x08
>  
> +#define FW_CFG_DMA_SIGNATURE 0x51454d5520434647 /* "QEMU CFG" */
> +
>  typedef struct FWCfgEntry {
>      uint32_t len;
>      uint8_t *data;
> @@ -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.

>  static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
>                                   uint64_t value, unsigned size)
>  {
> @@ -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.)

Thus, for reading, please enforce:

  (addr + size <= 8)

(In an uncontrolled environment, I'd protect the addition against
overflow too, but here the individual addends are safe enough.)

>  
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
> @@ -488,6 +496,7 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops = {
>  };
>  
>  static const MemoryRegionOps fw_cfg_dma_mem_ops = {
> +    .read = fw_cfg_dma_mem_read,
>      .write = fw_cfg_dma_mem_write,
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid.accepts = fw_cfg_dma_mem_valid,
> 

Thanks
Laszlo

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.

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

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
>