Subject: Re: [PATCH] QEMU fw_cfg DMA interface documentation
To: =?UTF-8?Q?Marc_Mar=c3=ad?= <markmb@redhat.com>,
        linux-kernel@vger.kernel.org
References: <1438858816-29385-1-git-send-email-markmb@redhat.com>
 <1438858987-29566-1-git-send-email-markmb@redhat.com>
Cc: Drew Jones <drjones@redhat.com>, Stefan Hajnoczi <stefanha@gmail.com>,
        "Kevin O'Connor" <kevin@koconnor.net>,
        Gerd Hoffmann <kraxel@redhat.com>,
        Peter Maydell <peter.maydell@linaro.org>
From: Laszlo Ersek <lersek@redhat.com>
Message-ID: <55C3CCC1.1000201@redhat.com>
Date: Thu, 6 Aug 2015 23:08:17 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.1.0
MIME-Version: 1.0
In-Reply-To: <1438858987-29566-1-git-send-email-markmb@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4841
Lines: 134

On 08/06/15 13:03, 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.

- Should be updated to say "bitmap" etc.

- I think these additions should start one paragraph earlier in the
  text file (ie. before the paragraph starting with "The guest kernel
  is not expected...")

> This can be used
> +through a write-only, 64-bit wide address register.

Should be updated to two, 32-bit wide registers, defining which half is
most significant and least significant, then the byte order within each
half, and also which is the one that fires off the DMA.

(I think Drew mentioned the same, but I can't find the message.)


> +
> +In this register, a pointer to a FWCfgDmaAccess structure can be written, in
> +big endian mode. This is the format of the FWCfgDmaAccess structure:

Ah okay, big endian is mentioned here.

> +
> +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,

in what byte order?

> a read operation will be performed.
> +"length" bytes

in what byte order?

> for the current selector and offset will be mapped into the
> +address specified by the "address" field.

In what byte order? :)

(To wit, I think that the fw_cfg_dma_transfer() function in "[PATCH 3/5]
Implement fw_cfg DMA interface" lacks translation from guest endianness
to host endianness, for the fields of FWCfgDmaAccess.)

> +
> +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:
> +   error bit set     ->  something went wrong.
> +   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.
> +
> +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 agree with Drew about padding being necessary here. I have proposed to
break up the address register into two 32-bit halves, but even for that,
0xa is not good enough. 0x0c..0x13, inclusive, seems better.

(Actually I wonder if we should instead add a separate region ("reg")
for this register, instead of padding. I don't know enough about device
trees to make a suggestion here. Let's go with this approach first, ie.
keep the single reg for now, just add the padding I guess.)

>    * Further registers may be appended to the region in case of future interface
>      revisions / feature bits.
>  
> 

The example DTB should also be refreshed. You can do it like this:
- apply your (updated) patch "[PATCH 4/5] Enable fw_cfg DMA interface
  for ARM" to QEMU

- run

  qemu-system-aarch64 -machine virt,dumpdtb=dumped.dtb ...
  dtc -I dtb -O dts dumped.dtb >dumped.dts

- locate "fw-cfg" in "dumped.dts"

The example should be similar to:

        fw-cfg@9020000 {
                compatible = "qemu,fw-cfg-mmio";
                reg = <0x0 0x9020000 0x0 0x14>;
        };


Thanks
Laszlo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/