QEMU fw_cfg DMA interface

From: "Gabriel L. Somlo" <[email protected]>

Document the behavior of fw_cfg_modify_iXX() for leak-less updating of integer-type blobs.

Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions may be added later if necessary..

Signed-off-by Gabriel Somlo <[email protected]>

Signed-off-by Gerd Hoffmann <[email protected]>

Reviewed-by Laszlo Ersek <[email protected]>

---
 docs/specs/fw_cfg.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

docs/specs/fw_cfg.txt +4 -0

index 74351dd..5bc7b96 100644
@@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add
 a dynamically allocated copy of the appropriately sized item to fw_cfg
 under the given selector key value.
 
+== fw_cfg_modify_iXX() ==
+
+Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
+Similarly to the corresponding fw_cfg_add_iXX() function set, convert

Quoted 36 lines · Marc Marí

From: "Gabriel L. Somlo" <[email protected]>

Document the behavior of fw_cfg_modify_iXX() for leak-less updating of integer-type blobs.

Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions may be added later if necessary..

Signed-off-by Gabriel Somlo <[email protected]>

Signed-off-by Gerd Hoffmann <[email protected]>

Reviewed-by Laszlo Ersek <[email protected]>

---
 docs/specs/fw_cfg.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

docs/specs/fw_cfg.txt +11 -0

index 74351dd..5bc7b96 100644
@@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to little-endian, then add
 a dynamically allocated copy of the appropriately sized item to fw_cfg
 under the given selector key value.
 
+== fw_cfg_modify_iXX() ==
+
+Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
+Similarly to the corresponding fw_cfg_add_iXX() function set, convert
+a 16-, 32-, or 64-bit integer to little endian, create a dynamically
+allocated copy of the required size, and replace the existing item at
+the given selector key value with the newly allocated one. The previous
+item, assumed to have been allocated during an earlier call to
+fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
+before the function returns.
+
 == fw_cfg_add_file() ==
 

docs/specs/fw_cfg.txt will convert a 16-, 32-, or 64-bit integer to little-endian, then add · line 175

Quoted 1 line

docs/specs/fw_cfg.txt +0 -0

 Given a filename (i.e., fw_cfg item name), starting pointer, and size,

This doesn't cover fw_cfg_modify_file(); is that intentional?

As an aside, shouldn't this function-level documentation be done via doc-comments in the header file where the prototypes are declared? (You don't need to move the docs around in this series, but it might be nice to do at some point.)

Quoted 1 line · Peter Maydell

This doesn't cover fw_cfg_modify_file(); is that intentional?

There should already be a paragraph in there (further down from where this is supposed to go in) describing fw_cfg_modify_file(), which isn't affected by this.

Quoted 4 lines

As an aside, shouldn't this function-level documentation be done via doc-comments in the header file where the prototypes are declared? (You don't need to move the docs around in this series, but it might be nice to do at some point.)

You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side port/mmio api only, and have the host-side functions simply documented as comments in include/hw/nvram/fw_cfg.h ?

That should be relatively painless, if that's the agreed-upon convention...

Quoted 10 lines · Gabriel L. Somlo

Quoted 4 lines · Peter Maydell

As an aside, shouldn't this function-level documentation be done via doc-comments in the header file where the prototypes are declared? (You don't need to move the docs around in this series, but it might be nice to do at some point.)

You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side port/mmio api only, and have the host-side functions simply documented as comments in include/hw/nvram/fw_cfg.h ?

Quoted 2 lines

That should be relatively painless, if that's the agreed-upon convention...

Yes. I think at the point this file was written we probably hadn't started using doc-comments for our APIs. (My usual place to crib the formatting for doc-comments is the extract/deposit APIs in bitops.h.)

Also, since I'll be tinkering with fw_cfg again, and you mentioned using DT on arm and ACPI on x86 to auto-detect the presence (and location) of fw_cfg from the guest-side in a related thread:

Quoted 3 lines · Gabriel L. Somlo

Also, since I'll be tinkering with fw_cfg again, and you mentioned using DT on arm and ACPI on x86 to auto-detect the presence (and location) of fw_cfg from the guest-side in a related thread:

I meant DT or ACPI on ARM, actually. I don't know what the x86 approach is for fw_cfg but I think it's just "known address". (For ARM we don't I think currently expose fw_cfg in the ACPI tables, but we probably ought to; only needed for the edge case where you actually wanted to read fw_cfg in the guest kernel rather than just the guest firmware, though.)

Right, that's what I'm trying to do -- expose fw_cfg in /sys/firmware/... on the guest -- which involves figuring out how to determine if it is present (DT on ARM, blindly poking port 0x510 on x86 for now, hence my interest in having it listed in ACPI).

Now, if we did expose fw_cfg in ACPI on *both* ARM and x86, I wonder I could get away with an ACPI-only detection scheme across both architectures...

Quoted 3 lines · Gabriel L. Somlo

Also, since I'll be tinkering with fw_cfg again, and you mentioned using DT on arm and ACPI on x86 to auto-detect the presence (and location) of fw_cfg from the guest-side in a related thread:

I meant DT or ACPI on ARM, actually. I don't know what the x86 approach is for fw_cfg but I think it's just "known address".

Quoted 7 lines · Peter Maydell

Quoted 3 lines · Gabriel L. Somlo

Also, since I'll be tinkering with fw_cfg again, and you mentioned using DT on arm and ACPI on x86 to auto-detect the presence (and location) of fw_cfg from the guest-side in a related thread:

I meant DT or ACPI on ARM, actually. I don't know what the x86 approach is for fw_cfg but I think it's just "known address".

Yes, "known address". And given that the firmware actually loads the acpi tables via fw_cfg that is very unlikely to change. Adding it to the acpi tables might be useful nevertheless so the guest os knows the ioports are in use.

I think on arm the acpi situation is the same, but I think firmware detecting the location via DT should work without chicken&egg problems.

Add fw_cfg DMA interface specification in the documentation.

Based on Gerd Hoffman's initial implementation.

+= Guest-side DMA Interface =

docs/specs/fw_cfg.txt +40 -0

+
+If bit 1 of the feature bitmap is set, the DMA interface is present. This does
+not replace the existing fw_cfg interface, it is an add-on. This interface
+can be used through the 64-bit wide address register.
+
+The address register, as the selector register, is in little-endian format
+when using IOports, and in big-endian format when using MMIO. 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.
+
+In this register, a physical RAM address to a FWCfgDmaAccess structure 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
+
+When an operation is triggered, 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 address specified by the "address" field.
+
+If the control field has only bit 2 set, a skip operation will be perfomed.
+The offset for the current selector will be advanced "length" bytes.
+
+To check result, read the "control" field:
+   error bit set        ->  something went wrong.
+   all bits cleared     ->  transfer finished successfully.

Add fw_cfg DMA interface specification in the documentation.

Based on Gerd Hoffman's initial implementation.

Signed-off-by Marc Marí <[email protected]>

---
 docs/specs/fw_cfg.txt | 68 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 4 deletions(-)

docs/specs/fw_cfg.txt +0 -0

index 5bc7b96..06302f6 100644

docs/specs/fw_cfg.txt +5 -4

 == Firmware Configuration Items ==
 

@@ -86,11 +93,12 @@ by selecting the "signature" item using key 0x0000 (FW_CFG_SIGNATURE),
 and reading four bytes from the data register. If the fw_cfg device is
 present, the four bytes read will contain the characters "QEMU".
 
-=== Revision (Key 0x0001, FW_CFG_ID) ===
+=== Revision / feature bitmap (Key 0x0001, FW_CFG_ID) ===
 
-A 32-bit little-endian unsigned int, this item is used as an interface
-revision number, and is currently set to 1 by QEMU when fw_cfg is
-initialized.
+A 32-bit little-endian unsigned int, this item is used to check for enabled
+features.
+ - Bit 0: traditional interface. Always set.
+ - Bit 1: DMA interface.
 
 === File Directory (Key 0x0019, FW_CFG_FILE_DIR) ===
 

@@ -132,6 +140,58 @@ Selector Reg.    Range Usage
 In practice, the number of allowed firmware configuration items is given
 by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
 

docs/specs/fw_cfg.txt +12 -0

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

docs/specs/fw_cfg.txt increasing address order, similar to memcpy(). · line 85

Quoted 5 lines · Marc Marí

+=== ARM Register Locations ===

docs/specs/fw_cfg.txt +4 -0

+
+Selector Register address: 0x09020000
+Data Register address:     0x09020008
+DMA Address address:       0x0902000c

These addresses shouldn't be documented -- the correct API is that the guest needs to find the base address of the fw_cfg device via device tree or ACPI table. You can document the layout of the registers within the device, obviously (ie +0, +4, +8).

Based on the specifications on docs/specs/fw_cfg.txt

This interface is an addon. The old interface can still be used as usual.

Based on Gerd Hoffman's initial implementation.

Signed-off-by Marc Marí <[email protected]>

---
 hw/arm/virt.c             |   2 +-
 hw/nvram/fw_cfg.c         | 261 +++++++++++++++++++++++++++++++++++++++++++---
 include/hw/nvram/fw_cfg.h |  15 ++-
 3 files changed, 260 insertions(+), 18 deletions(-)

hw/arm/virt.c +1 -1

index d5a8417..b88c104 100644
@@ -620,7 +620,7 @@ static void create_fw_cfg(const VirtBoardInfo *vbi)
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8);
+    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);

hw/nvram/fw_cfg.c +32 -2

index 88481b7..7e5ba96 100644
@@ -23,6 +23,7 @@
  */
 #include "hw/hw.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/dma.h"
 #include "hw/isa/isa.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/sysbus.h"

@@ -30,7 +31,8 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_SIZE 2
+#define FW_CFG_IO_SIZE 12 /* Address aligned to 4 bytes */
+#define FW_CFG_CTL_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 

@@ -42,6 +44,15 @@
 #define FW_CFG_IO(obj)  OBJECT_CHECK(FWCfgIoState,  (obj), TYPE_FW_CFG_IO)
 #define FW_CFG_MEM(obj) OBJECT_CHECK(FWCfgMemState, (obj), TYPE_FW_CFG_MEM)
 
+/* FW_CFG_VERSION bits */
+#define FW_CFG_VERSION      0x01
+#define FW_CFG_VERSION_DMA  0x02
+
+/* FW_CFG_DMA_CONTROL bits */
+#define FW_CFG_DMA_CTL_ERROR   0x01
+#define FW_CFG_DMA_CTL_READ    0x02
+#define FW_CFG_DMA_CTL_SKIP    0x04
+
 typedef struct FWCfgEntry {
     uint32_t len;
     uint8_t *data;

@@ -59,6 +70,10 @@ struct FWCfgState {
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
+
+    bool dma_enabled;
+    AddressSpace *dma_as;
+    dma_addr_t dma_addr;
 };
 
 struct FWCfgIoState {

@@ -75,7 +90,7 @@ struct FWCfgMemState {
     FWCfgState parent_obj;
     /*< public >*/
 
-    MemoryRegion ctl_iomem, data_iomem;
+    MemoryRegion ctl_iomem, data_iomem, dma_iomem;
     uint32_t data_width;
     MemoryRegionOps wide_data_ops;
 };

@@ -294,6 +309,142 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
     } while (i);
 }
 
+static void fw_cfg_dma_transfer(FWCfgState *s)
+{
+    dma_addr_t len;
+    uint8_t *ptr;
+    void *addr;
+    FWCfgDmaAccess dma;
+    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+
+    len = sizeof(dma);
+    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
+                                DMA_DIRECTION_FROM_DEVICE);
+
+    s->dma_addr = 0;
+

hw/nvram/fw_cfg.c +63 -0

+    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
+        goto out;
+    }
+
+    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
+        goto out;
+    }
+
+    while (dma.length > 0) {
+        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
+                                s->cur_offset >= e->len) {
+            len = dma.length;
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,
+                                     DMA_DIRECTION_FROM_DEVICE);
+                if (!ptr || !len) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                    goto out;
+                }
+
+                memset(ptr, 0, len);
+
+                dma_memory_unmap(s->dma_as, ptr, len,
+                                 DMA_DIRECTION_FROM_DEVICE, len);
+            }
+
+        } else {
+            if (dma.length <= e->len) {
+                len = dma.length;
+            } else {
+                len = e->len;
+            }
+
+            if (e->read_callback) {
+                e->read_callback(e->callback_opaque, s->cur_offset);
+            }
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,
+                                     DMA_DIRECTION_FROM_DEVICE);
+                if (!ptr || !len) {
+                    dma.control |= FW_CFG_DMA_CTL_ERROR;
+                    goto out;
+                }
+
+                memcpy(ptr, &e->data[s->cur_offset], len);
+
+                dma_memory_unmap(s->dma_as, ptr, len,
+                                 DMA_DIRECTION_FROM_DEVICE, len);
+            }
+
+            s->cur_offset += len;
+
+        }
+
+        dma.address += len;

Based on the specifications on docs/specs/fw_cfg.txt

This interface is an addon. The old interface can still be used as usual.

Based on Gerd Hoffman's initial implementation.

Signed-off-by Marc Marí <[email protected]>

hw/nvram/fw_cfg.c +15 -0

@@ -294,6 +309,142 @@ static void fw_cfg_data_mem_write(void *opaque, hwaddr addr,
     } while (i);
 }
 
+static void fw_cfg_dma_transfer(FWCfgState *s)
+{
+    dma_addr_t len;
+    uint8_t *ptr;
+    void *addr;
+    FWCfgDmaAccess dma;
+    int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
+    FWCfgEntry *e = &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
+
+    len = sizeof(dma);
+    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
+                                DMA_DIRECTION_FROM_DEVICE);
+
+    s->dma_addr = 0;
+

hw/nvram/fw_cfg.c +19 -0

+
+    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
+        goto out;
+    }
+
+    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
+        goto out;
+    }
+
+    while (dma.length > 0) {
+        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
+                                s->cur_offset >= e->len) {
+            len = dma.length;
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,

hw/nvram/fw_cfg.c +18 -0

+            }
+
+        } else {
+            if (dma.length <= e->len) {
+                len = dma.length;
+            } else {
+                len = e->len;
+            }
+
+            if (e->read_callback) {
+                e->read_callback(e->callback_opaque, s->cur_offset);
+            }
+
+            /* If the access is not a read access, it will be a skip access,
+             * tested before.
+             */
+            if (dma.control & FW_CFG_DMA_CTL_READ) {
+                ptr = dma_memory_map(s->dma_as, dma.address, &len,

hw/nvram/fw_cfg.c +26 -0

+            }
+
+            s->cur_offset += len;
+
+        }
+
+        dma.address += len;
+        dma.length  -= len;
+        dma.control = 0;
+
+        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
+                                                cpu_to_be64(dma.address);
+        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
+                                                cpu_to_be32(dma.length);
+        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
+                                                cpu_to_be32(dma.control);
+    }
+
+    trace_fw_cfg_read(s, 0);
+
+out:
+    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
+                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
+    return;
+
+}

hw/nvram/fw_cfg.c +33 -3

+        } else if (addr == 4) {
+            /* FWCfgDmaAccess low address */
+            s->dma_addr |= value;
+            fw_cfg_dma_transfer(s);
+        }
+    } else if (size == 8 && addr == 0) {
+        s->dma_addr = value;
+        fw_cfg_dma_transfer(s);
+    }
+}
+
+static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr,
+                                  unsigned size, bool is_write)
+{
+    return is_write && ((size == 4 && (addr == 0 || addr == 4)) ||
+                        (size == 8 && addr == 0));
+}
+
 static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
                                   unsigned size, bool is_write)
 {

@@ -321,20 +472,35 @@ static uint64_t fw_cfg_comb_read(void *opaque, hwaddr addr,
 static void fw_cfg_comb_write(void *opaque, hwaddr addr,
                               uint64_t value, unsigned size)
 {
-    switch (size) {
+    FWCfgState *s;
+
+    switch (addr) {
+    case 0:
+        fw_cfg_select(opaque, (uint16_t)value);
+        break;
     case 1:
         fw_cfg_write(opaque, (uint8_t)value);
         break;
-    case 2:
-        fw_cfg_select(opaque, (uint16_t)value);
+    case 4:
+        /* FWCfgDmaAccess low address */
+        s = opaque;
+        s->dma_addr |= value;
+        fw_cfg_dma_transfer(s);
         break;
+    case 8:
+        /* FWCfgDmaAccess high address */
+        s = opaque;
+        s->dma_addr = (0xFFFFFFFF & value) << 32;
     }

Enable the fw_cfg DMA interface for the ARM virt machine.

Based on Gerd Hoffman's initial implementation.

Signed-off-by Marc Marí <[email protected]>

---
 hw/arm/virt.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

hw/arm/virt.c +1 -1

index b88c104..54d5f54 100644
@@ -111,7 +111,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
-    [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },
+    [VIRT_FW_CFG] =             { 0x09020000, 0x00000014 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },

hw/arm/virt.c +2 -1

 {
     hwaddr base = vbi->memmap[VIRT_FW_CFG].base;
     hwaddr size = vbi->memmap[VIRT_FW_CFG].size;
     char *nodename;
 
-    fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL);
+    fw_cfg_init_mem_wide(base + 8, base, 8, base + 16, as);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);

@@ -808,6 +808,7 @@ static void machvirt_init(MachineState *machine)
     VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     VirtGuestInfo *guest_info = &guest_info_state->info;
     char **cpustr;
+    AddressSpace *as = NULL;
 
     if (!cpu_model) {
         cpu_model = "cortex-a15";

hw/arm/virt.c +2 -2

+
         /* Handle any CPU options specified by the user */
         cc->parse_features(CPU(cpuobj), cpuopts, &err);
         g_free(cpuopts);

@@ -897,7 +902,7 @@ static void machvirt_init(MachineState *machine)
      */
     create_virtio_devices(vbi, pic);
 
-    create_fw_cfg(vbi);
+    create_fw_cfg(as, vbi);
     rom_set_fw(fw_cfg_find());
 
     guest_info->smp_cpus = smp_cpus;
--

2.4.3

Add fw_cfg DMA interface specfication in the fw_cfg documentation.

Signed-off-by Marc Marí <[email protected]>

---
 Documentation/devicetree/bindings/arm/fw-cfg.txt | 51 +++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/fw-cfg.txt +7 -0

index 953fb64..766ddbe 100644
@@ -45,6 +45,53 @@ 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

Quoted 30 lines · Marc Marí

Add fw_cfg DMA interface specfication in the fw_cfg documentation.

Signed-off-by Marc Marí <[email protected]>

---
 Documentation/devicetree/bindings/arm/fw-cfg.txt | 51

+++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/arm/fw-cfg.txt index 953fb64..766ddbe 100644 --- a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++ b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -45,6 +45,53 @@ 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.

(quoted diff) +2 -0

+
+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. + +In this register, a physical RAM address to a FWCfgDmaAccess structure should +be written. This is the format of the FWCfgDmaAccess structure: +
+typedef struct FWCfgDmaAccess {

Documentation/devicetree/bindings/arm/fw-cfg.txt blob to be read from the data register has size 4, and it is to be interpreted · line 64

Quoted 4 lines

Documentation/devicetree/bindings/arm/fw-cfg.txt +4 -0

+    uint32_t control;
+    uint32_t length;
+    uint64_t address;
+} FWCfgDmaAccess;

I think including the selector field would be nice to avoid extra register accesses, but I'm not that familiar with fw_cfg so maybe there's a reason not to include that field.

Quoted 9 lines

+The fields of the structure are in big endian mode, and the field at the lowest +address is the "control" field.

Documentation/devicetree/bindings/arm/fw-cfg.txt +7 -0

+
+The "control" field has the following bits:
+ - Bit 0: Error
+ - Bit 1: Read
+ - Bit 2: Skip
+
+When an operation is triggered, if the "control" field has bit 1

Quoted 3 lines

set, a read +operation will be performed. "length" bytes for the current selector and +offset will be copied into the address specified by the "address" field.

Minor clarification:
s/address/physical RAM address/

Reviewed-by Stefan Hajnoczi <[email protected]>

It's just simplicity. If you want to read a few times from the same field (like in ACPI tables, read the data size and then the data), you need a way to enable and disable the selector and manage the current offset for that entry. This is already provided with the "old" interface.

Hi,

Quoted 5 lines

It's just simplicity. If you want to read a few times from the same field (like in ACPI tables, read the data size and then the data), you need a way to enable and disable the selector and manage the current offset for that entry. This is already provided with the "old" interface.

Could be handled with a 'select' control bit. Only when set select entry and reset offset to zero.

Also: would it make sense to allow an *array* of FWCfgDmaAccess structs? With a 'more' bit in control we could indicate that there are more entries. I'm not sure firmware would actually use that though ...

cheers,
Gerd

Quoted 5 lines

It's just simplicity. If you want to read a few times from the same field (like in ACPI tables, read the data size and then the data), you need a way to enable and disable the selector and manage the current offset for that entry. This is already provided with the "old" interface.

Could be handled with a 'select' control bit. Only when set select entry and reset offset to zero.

Quoted 8 lines · Gerd Hoffmann

Quoted 5 lines

It's just simplicity. If you want to read a few times from the same field (like in ACPI tables, read the data size and then the data), you need a way to enable and disable the selector and manage the current offset for that entry. This is already provided with the "old" interface.

Could be handled with a 'select' control bit. Only when set select entry and reset offset to zero.

I think two features would help "round off" the new fw_cfg DMA
proposal: add a select bit as you describe (that uses the 16 most significant bits of the "control" field for the "select entry" when the bit is set), and define a static signature (eg, "QEMU CFG") when reading the 64bit MMIO dma register.

Both are optional features that don't change the fundamental interface; I was thinking of sending them as two patches on top of Marc's next version of his patch series (if no one else gets to it first).