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

[Marc Marí]

On Mon, 31 Aug 2015 12:18:27 -0400
"Kevin O'Connor" <kevin at koconnor.net> wrote:

> 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í <markmb at redhat.com>
> > ---
> >  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.

The reason was that the host could write the elements in different
order, or something like this. I now see it again and it doesn't make
sense. So I'll put it in a simpler way.

> 
> > +    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();

I was leaving the len bit because it was in the original
implementation. But it's true that taking it out simplifies everything.

Thanks
Marc

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