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

[Laszlo Ersek]

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.

> +
>  === 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