On Thu, 6 Aug 2015 10:20:38 -0400 "Kevin O'Connor" <ke...@koconnor.net> 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í <mar...@redhat.com> > > --- > > 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