Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Andrew Jones <drjones@redhat.com>]

On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Marí wrote:
> On Thu, 6 Aug 2015 16:08:49 +0200
> Andrew Jones <drjones@redhat.com> wrote:
> 
> > On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí wrote:
> > > Add fw_cfg DMA interface specfication in the fw_cfg documentation.
> > > 
> > > Signed-off-by: Marc Marí <markmb@redhat.com>
> > > ---
> > >  Documentation/devicetree/bindings/arm/fw-cfg.txt | 36
> > > ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> > > b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> > > 953fb64..c880eec 100644 ---
> > > a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> > > b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6 +49,41
> > > @@ The guest kernel is not expected to use these registers
> > > (although it is certainly allowed to); the device tree bindings are
> > > documented here because this is where device tree bindings reside
> > > in general. +Starting from revision 2, a DMA interface has also
> > > been added. This can be used +through a write-only, 64-bit wide
> > > address register. +
> > > +In this register, a pointer to a FWCfgDmaAccess structure can be
> > > written, in +big endian mode. This is the format of the
> > > FWCfgDmaAccess structure:
> > s/mode/format/
> > > +
> > > +typedef struct FWCfgDmaAccess {
> > > +    uint64_t address;
> > > +    uint32_t length;
> > > +    uint32_t control;
> > > +} FWCfgDmaAccess;
> > > +
> > > +Once the address to this structure has been written, an DMA
> > > operation is +started. If the "control" field has value 2, a read
> > > operation will be performed. +"length" bytes for the current
> > > selector and offset will be mapped into the +address specified by
> > > the "address" field. +
> > > +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.
> > 
> > So we can't DMA to address == 0? That might not generally be a good
> > idea, but why limit ourselves? Can't we add another control input for
> > skip instead? Actually, what inputs are accepted now? READ == 2,
> > WRITE? ??
> > 
> 
> Write was already disabled for PIO:
> 
> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> {
>     /* nothing, write support removed in QEMU v2.4+ */
> }
> 
> > > +
> > > +To check result, read the control register:
> > > +   error bit set     ->  something went wrong.
> > echo Stefan's which bit question, and also what types of errors? If
> > there are many, then how about an error bit, plus field of bits for
> > an error code?
> > 
> 
> Bit 0. At this moment the only error possible is DMA mapping failure.
> But its true that it might be useful to have some bits in the control
> field or another field to indicate the type of error, in case of future
> extensions.
> 
> > > +   all bits cleared  ->  transfer finished successfully.
> > > +   otherwise         ->  transfer still in progress (doesn't happen
> > > +                         today due to implementation not being
> > > async,
> > > +                         but may in the future).
> > I don't think we need to point out that this isn't implemented yet,
> > but may be in the future. If that's how it may work, then let's just
> > document it. And why not specify an in-progress bit?
> 
> Is this a feature we will want?

I don't know. Need firmware person like Laszlo to give an opinion. Maybe
he'd want OVMF/AAVMF to be able to output progress while transferring,
to keep users more patient?

> 
> > > +
> > > +Target address goes up and transfer length goes down as the
> > > transfer +happens, so after a successful transfer the length
> > > register is zero +and the address register points right after the
> > > memory block written. +
> > > +If a partial transfer happened before an error occured the address
> > > and +length registers indicate how much data has been transfered
> > > +successfully.
> > > +
> > >  Required properties:
> > >  
> > >  - compatible: "qemu,fw-cfg-mmio".
> > > @@ -56,6 +91,7 @@ Required properties:
> > >  - reg: the MMIO region used by the device.
> > >    * Bytes 0x0 to 0x7 cover the data register.
> > >    * Bytes 0x8 to 0x9 cover the selector register.
> > > +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address
> > > register.
> > 
> > I think we should naturally align the register, i.e we can reserve
> > bytes 0xa - 0xf, and then put the DMA register at 0x10.
> > 
> > >    * Further registers may be appended to the region in case of
> > > future interface revisions / feature bits.
> > >  
> > > -- 
> > > 2.4.3
> > > 
> > 
> 
> Thanks
> Marc