Re: [SeaBIOS] [PATCH v2 1/2] Add QEMU fw_cfg DMA interface

[Kevin O'Connor]

On Mon, Aug 31, 2015 at 11:12:01AM +0200, Marc Marí wrote:
> Add support for the new fw_cfg DMA interface. The protocol is explained in
> QEMU documentation.

Thanks for working on this.
> Signed-off-by: Marc Marí
>
> src/fw/paravirt.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  src/fw/paravirt.h | 25 +++++++++++++++++----
>  2 files changed, 83 insertions(+), 7 deletions(-)
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index db22ae8..26af499 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -23,6 +23,7 @@
>  #include "util.h" // pci_setup
>  #include "x86.h" // cpuid
>  #include "xen.h" // xen_biostable_setup
> +#include "stacks.h" // yield
>
> // Amount of continuous ram under 4Gig
>  u32 RamSize;
> @@ -30,6 +31,13 @@ u32 RamSize;
>  u64 RamSizeOver4G;
>  // Type of emulator platform.
>  int PlatformRunningOn VARFSEG;
> +// cfg_dma enabled
> +int cfg_dma_enabled = 0;
>
> +inline int qemu_cfg_dma_enabled(void)
> +{
> return cfg_dma_enabled;
>
> +}
>
> /* This CPUID returns the signature 'KVMKVMKVM' in ebx, ecx, and edx.  It
> should be used to determine that a VM is running under KVM.
>
> @@ -199,16 +207,57 @@ qemu_cfg_select(u16 f)
>  }
>
> static void
> +qemu_cfg_dma_transfer(u64 address, u32 length, u32 control)
> +{
> QemuCfgDmaAccess access;
> void *p = &access;
> *(u64 *)(p + offsetof(QemuCfgDmaAccess, address)) =
>                                    cpu_to_be64(address);
> *(u32 *)(p + offsetof(QemuCfgDmaAccess, length)) =
>                                    cpu_to_be32(length);
> *(u32 *)(p + offsetof(QemuCfgDmaAccess, control)) =
>                                    cpu_to_be32(control);
> barrier();
> outl((u32)p, PORT_QEMU_CFG_DMA_ADDR_LOW);

Unless I'm missing something, the above is the same as:

QemuCfgDmaAccess access;
    access.address = cpu_to_be64(address);
    access.length = cpu_to_be64(length);
    access.control = cpu_to_be64(control);

barrier();
    outl((u32)access, PORT_QEMU_CFG_DMA_ADDR_LOW);

I'm not sure what the reason for the casts and offsetof() was, but I
find them confusing.
> u32 len;
> u32 ctl;
> do {
>    yield();
>    len = be32_to_cpu(*(u32 *)(p +
>            offsetof(QemuCfgDmaAccess, length)));
>    ctl = be32_to_cpu(*(u32 *)(p +
>            offsetof(QemuCfgDmaAccess, control)));
> } while(len != 0 && !(ctl & QEMU_CFG_DMA_CTL_ERROR));

As mentioned in another email, we can't spin on "length or control"
because it can lead to race conditions (we need to spin only on the
field that QEMU will last update).  Also, it's best not to yield()
unless we really do need to wait.  I think the following would be
better:

while (be32_to_cpu(access.control) & ~QEMU_CFG_DMA_CTL_ERROR)
        yield();
> +static void
>  qemu_cfg_read(void *buf, int len)
>  {
> insb(PORT_QEMU_CFG_DATA, buf, len);
> if (qemu_cfg_dma_enabled()) {
>    qemu_cfg_dma_transfer((u64)(u32)buf, len, QEMU_CFG_DMA_CTL_READ);

I would pass a "void *" to qemu_cfg_dma_transfer() and let
qemu_cfg_dma_transfer() do whatever casts are necessary to make the
address work.

-Kevin