Re: [Qemu-devel] [PATCH 2/5] fw_cfg DMA interface documentation

[Kevin O'Connor]

On Thu, Aug 06, 2015 at 01:01:15PM +0200, Marc Marí wrote:
> Add fw_cfg DMA interface specification in the documentation.
> 
> Based on Gerd Hoffman's initial implementation.
> 
> Signed-off-by: Marc Marí <address@hidden>
> ---
>  docs/specs/fw_cfg.txt | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index 5bc7b96..dc8051e 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -76,6 +76,7 @@ increasing address order, similar to memcpy().
>  
>  Selector Register IOport: 0x510
>  Data Register IOport:     0x511
> +DMA Address IOport:       0x512
>  
>  == Firmware Configuration Items ==
>  
> @@ -89,8 +90,9 @@ present, the four bytes read will contain the characters 
> "QEMU".
>  === Revision (Key 0x0001, FW_CFG_ID) ===
>  
>  A 32-bit little-endian unsigned int, this item is used as an interface
> -revision number, and is currently set to 1 by QEMU when fw_cfg is
> -initialized.
> +revision number. If it is set to 1, the interface is the traditional
> +selector / data interface. If it is set to 2, the DMA extension is
> +also present.
>  
>  === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
>  
> @@ -132,6 +134,42 @@ Selector Reg.    Range Usage
>  In practice, the number of allowed firmware configuration items is given
>  by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>  
> += Guest-side DMA Interface =
> +
> +For revision value 2, the DMA interface is present. This does not replace
> +the existing fw_cfg interface, it is an add-on.
> +
> +When this interface is enabled the DMA Address register can be used to
> +write the address of a FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> +    uint64_t address;
> +    uint32_t length;
> +    uint32_t control;
> +} FWCfgDmaAccess;
> +
> +If "control" has the bit 1 set (FW_CFG_DMA_CTL_READ), a read operation is
> +performed on the selected entry. "length" bytes of data in that fw_cfg
> +entry are copied to the address specified by "address".
> +
> +If the field "address" has value 0, the read is considered a skip, and
> +the data is not copied anywhere, but the offset is still incremented.

Thanks!

I have a few suggestions on the interface:

- I think it would be better to place the 'control' field first (ie,
  control/length/address instead of address/length/control).  Placing
  the 'control' field first makes potential future extensions easier -
  that is, it would make it easier for future control bits to change
  the layout of the struct.

- It would be good to add a 'select' field to the struct.  I think
  this could be done by using a 'u16 control; u16 select' instead of
  the current 'u32 control'.  It's common for guests to select a
  fw_cfg entry and read its entire contents - with the 'select' field
  in the struct this could be done with a single guest/host fault.  A
  new control bit could be added (eg, FW_CFG_DMA_CTL_SELECT) to
  determine whether or not the select field is used - iff the bit is
  set then the given fw_cfg entry is selected and the read position is
  reset to the start prior to the DMA data copy.

- Instead of using address==0 to check for skip, I think a new control
  bit would be a little more friendly.  That is, one could add a new
  control bit FW_CFG_DMA_CTL_COPY, and iff it is set then do the DMA
  style copy to the 'address' field.  If it is not set, then 'address'
  is ignored and effectively the read position is just incremented.

-Kevin