On Jan 20, 2017, at 8:58 AM, Kevin O'Connor <kevin@koconnor.net> wrote:

On Fri, Jan 20, 2017 at 05:39:58PM +0100, Laszlo Ersek wrote:

On 01/20/17 17:08, Kevin O'Connor wrote:

On Thu, Jan 19, 2017 at 10:20:50PM -0800, ben@skyportsystems.com wrote:

From: Ben Warren <ben@skyportsystems.com>

This adds a new command to the DMA protocol.  The command allows the
memory allocation of a fw_cfg file by BIOS and subsequent return of
the allocated address to QEMU.  The initial use case is for Windows VM
Generation ID, where QEMU needs to change the contents of fw_cfg
data at runtime, while still having BIOS allocate and manage the memory.

Thanks - see my comments below.

[...]

--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -253,6 +253,20 @@ qemu_cfg_read(void *buf, int len)
}

static void
+qemu_cfg_write(void *buf, int len)
+{
+    if (len == 0) {
+        return;
+    }
+
+    if (qemu_cfg_dma_enabled()) {
+        qemu_cfg_dma_transfer(buf, len, QEMU_CFG_DMA_CTL_WRITE);
+    } else {
+        outsb(PORT_QEMU_CFG_DATA, buf, len);
+    }
+}
+
+static void
qemu_cfg_skip(int len)
{
    if (len == 0) {
@@ -280,6 +294,19 @@ qemu_cfg_read_entry(void *buf, int e, int len)
    }
}

+static void
+qemu_cfg_write_entry(void *buf, int e, int len)
+{
+    if (qemu_cfg_dma_enabled()) {
+        u32 control = (e << 16) | QEMU_CFG_DMA_CTL_SELECT
+                        | QEMU_CFG_DMA_CTL_WRITE;
+        qemu_cfg_dma_transfer(buf, len, control);
+    } else {
+        qemu_cfg_select(e);
+        qemu_cfg_write(buf, len);
+    }
+}
+
struct qemu_romfile_s {
    struct romfile_s file;
    int select, skip;
@@ -303,6 +330,24 @@ qemu_cfg_read_file(struct romfile_s *file, void *dst, u32 maxlen)
    return file->size;
}

+static int
+qemu_cfg_write_file(void *src, struct romfile_s *file, u32 maxlen)
+{
+    if (file->size < maxlen)
+        return -1;
+    struct qemu_romfile_s *qfile;
+    qfile = container_of(file, struct qemu_romfile_s, file);
+    if (qfile->skip == 0) {
+        /* Do it in one transfer */
+        qemu_cfg_write_entry(src, qfile->select, file->size);
+    } else {
+        qemu_cfg_select(qfile->select);
+        qemu_cfg_skip(qfile->skip);
+        qemu_cfg_write(src, file->size);
+    }
+    return file->size;
+}

I'd prefer if we could break this patch into two parts - one part to
add the write interface to fw_cfg and one part to add
ALLOCATE_RET_ADDR.  I'm fine with both interfaces, but the QEMU parts
still need to be committed prior to committing to SeaBIOS.  Having the
patch in separate parts will make it both easier to review and commit
at their appropriate times.

The fw_cfg write support is now upstream (QEMU commit
baf2d5bfbac015b27f4db74feab235e167df0c84, "fw-cfg: support writeable
blobs").

Thanks.  Is it correct that the QEMU write support is only possible
with the DMA interface?  If so, I'd request that Ben's patch also be
updated to not issue an outsb().  Preferably it should just fail at
the start of qemu_cfg_write_file() if the DMA interface isn't
available.

Hopefully I’ve addressed all of your concerns.  I’m curious now, how is this kind of thing typically phased with QEMU changes?  I’m working on getting the QEMU side of things submitted, but for it to work standalone, the embedded binary SeaBIOS image needs to be updated.  Who typically builds and inserts the new binary in to the QEMU code tree?

-Kevin