Re: [PATCH] QEMU fw_cfg DMA interface documentation

[Laszlo Ersek <lersek@redhat.com>]

On 08/06/15 14:12, Stefan Hajnoczi wrote:
> On Thu, Aug 6, 2015 at 12:03 PM, Marc Marí <markmb@redhat.com> 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
> 
> Please update the "=== Revision (Key 0x0001, FW_CFG_ID) ===" section
> to say that currently the revision is 2.

Sorry I haven't started reading the series yet, but this caught my eye
-- can we agree that this field should be a bitmap instead, and not a
counter-like value? I don't insist of course, because for the current
use case both approaches will work. But, for "future proofing", I think
it is useful to express features independently of each other. (See eg.
virtio feature flags.)

Just an idea.

Thanks
Laszlo

> 
>>  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
> 
> s/pointer/physical RAM address/ is clearer
> 
>> +big endian mode. This is the format of the FWCfgDmaAccess structure:
> 
> Please be explicit about the *order* of 32-bit writes to the 64-bit
> DMA register.
> 
> Big-endian only defines the layout of bits but it doesn't say in which
> order the two 32-bit sub-registers need to be written.
> 
>> +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.
> 
> "mapped" might be confusing.  "Copied" or "DMAed" is clearer.
> 
>> +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.
>> +
>> +To check result, read the control register:
> 
> FWCfgDmaAccess.control is not a register, it's a field.
> 
> s/register/field/
> 
>> +   error bit set     ->  something went wrong.
> 
> Which bit number is the error bit?
> 
>> +   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).
>> +
>> +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.
> 
> s/register/field/
>