Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754263AbbHFWgJ (ORCPT ); Thu, 6 Aug 2015 18:36:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50927 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751493AbbHFWgG (ORCPT ); Thu, 6 Aug 2015 18:36:06 -0400 Subject: Re: [PATCH] QEMU fw_cfg DMA interface documentation To: =?UTF-8?Q?Marc_Mar=c3=ad?= , 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 , Stefan Hajnoczi , "Kevin O'Connor" , Gerd Hoffmann , Peter Maydell From: Laszlo Ersek 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 List-ID: X-Mailing-List: linux-kernel@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Ă­ > --- > 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/