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

[Marc Marí]

On Thu, 6 Aug 2015 10:20:38 -0400
"Kevin O'Connor" <address@hidden> wrote:

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

(Problems with email, I got this delivered today)

I don't think there's much problem in using the original fw cfg select
field. Adding a new select field will add complexity to the guest (when
use one select field or the other), and the host (how the two select
fields interact with each other). I think this part is good enough as
it is now.

Thanks
Marc