On Mon, 31 Aug 2015 12:18:27 -0400 "Kevin O'Connor" <ke...@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í <mar...@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 _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios