lkml.org 
[lkml]   [2015]   [Oct]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5] QEMU fw_cfg DMA interface documentation
From
Date
On 10/08/15 17: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 | 52 +++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> index 953fb64..0633aad 100644
> --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt
> @@ -38,6 +38,9 @@ The presence of the registers can be verified by selecting the "signature" blob
> with key 0x0000, and reading four bytes from the data register. The returned
> signature is "QEMU".
>
> +If the DMA interface is available, then reading the DMA Address Register
> +returns 0x51454d5520434647 ("QEMU CFG" in big-endian format).
> +

marking this for a later argument: (*)

> The outermost protocol (involving the write / read sequences of the control and
> data registers) is expected to be versioned, and/or described by feature bits.
> The interface revision / feature bitmap can be retrieved with key 0x0001. The
> @@ -45,6 +48,51 @@ blob to be read from the data register has size 4, and it is to be interpreted
> as a uint32_t value in little endian byte order. The current value
> (corresponding to the above outer protocol) is zero.
>
> +If bit 1 of the feature bitmap is set, the DMA interface is present. This
> +can be used through the 64-bit wide address register.
> +
> +The address register is in big-endian format. The value for the register is 0
> +at startup and after an operation. A write to the lower half triggers an
> +operation. This means, that operations with 32-bit addresses can be triggered
> +with just one write, whereas operations with 64-bit addresses can be triggered
> +with one 64-bit write or two 32-bit writes, starting with the higher part.

Please sync this language with the "least significant half" / "most
significant half" wording that I requested after your QEMU

[PATCH v4 2/7] fw_cfg DMA interface documentation

and that you implemented in your present QEMU

[PATCH v5 2/6] fw_cfg DMA interface documentation

> +
> +In this register, the physical address of a FWCfgDmaAccess structure in RAM
> +should be written. This is the format of the FWCfgDmaAccess structure:
> +
> +typedef struct FWCfgDmaAccess {
> + uint32_t control;
> + uint32_t length;
> + uint64_t address;
> +} FWCfgDmaAccess;
> +
> +The fields of the structure are in big endian mode, and the field at the lowest
> +address is the "control" field.
> +
> +The "control" field has the following bits:
> + - Bit 0: Error
> + - Bit 1: Read
> + - Bit 2: Skip
> + - Bit 3: Select. The upper 16 bits are the selected index.
> +
> +When an operation is triggered, if the "control" field has bit 3 set, the
> +upper 16 bits are interpreted as an index of a firmware configuration item.
> +This has the same effect as writing the selector register.
> +
> +If the "control" field has bit 1 set, a read operation will be performed.
> +"length" bytes for the current selector and offset will be copied into the
> +physical RAM address specified by the "address" field.
> +
> +If the "control" field has bit 2 set (and not bit 1), a skip operation will be
> +performed. The offset for the current selector will be advanced "length" bytes.
> +
> +To check the result, read the "control" field:
> + 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).
> +
> 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.

I guess this is all coming verbatim from the QEMU spec. I think that's
okay, it doesn't speak about selector values etc, only about transport.
I think it's okay (and consistent with the current text) to have all the
details here.

> @@ -56,6 +104,8 @@ 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.
> + * With DMA interface enabled: Bytes 0x10 to 0x17 cover the DMA address
> + register.
> * Further registers may be appended to the region in case of future interface
> revisions / feature bits.
>
> @@ -66,7 +116,7 @@ Example:
> #address-cells = <0x2>;
>
> fw-cfg@9020000 {
> + reg = <0x0 0x9020000 0x0 0x18>;
> compatible = "qemu,fw-cfg-mmio";
> - reg = <0x0 0x9020000 0x0 0xa>;
> };
> };
>

Please make this a bit more precise. As already mentioned by Peter (in
another part of the discussion), you can't read the DMA address register
until you know that it exists. Therefore the part I marked with (*)
above is only usable in the ioport-mapped, x86 case for feature
detection. On ARM, what we key off of primarily is the *size* of the
register block, and only if the DMA address register fits in there, can
we read the feature bitmap (for double-checking), and/or read a
signature from the DMA address register.

Therefore, I think the example device tree snippet is okay, but the hunk
above it should sound like:

* Bytes 0x10 to 0x17, if they exist in the region -- and the
feature bitmap confirms the presence of the DMA interface --
cover the DMA address register.

The AAVMF client code does just this. (I.e., checks the size of the
region first, then reads the feature bitmap *without* DMA, then performs
further reads with DMA.)

Thanks
Laszlo


\
 
 \ /
  Last update: 2015-10-09 01:41    [W:2.124 / U:1.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site