On Tue, 21 Jul 2015 21:44:49 +0200
Laszlo Ersek <ler...@redhat.com> wrote:
> On 07/21/15 18:03, Marc Marí wrote:
> > From: Gerd Hoffmann <kra...@redhat.com>
> >
> > First draft of a fw_cfg dma interface. Designed as add-on to the
> > extisting fw_cfg interface, i.e. there is no select register. There
> > are four 32bit registers: Target address (low and high bits),
> > transfer length, control register.
> >
> > See docs/specs/fw_cfg.txt update for details.
> >
> > Possible improvements (can be done incremental):
> >
> > * Better error reporting. Right now we throw errors on invalid
> > addresses only. We could also throw errors on invalid
> > reads/writes instead of using fw_cfg_read (return zeros on error)
> > behavior.
> > * Use memcpy instead of calling fw_cfg_read in a loop.
> >
> > Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> > ---
> > docs/specs/fw_cfg.txt | 35 ++++++++++
> > hw/arm/virt.c | 2 +-
> > hw/nvram/fw_cfg.c | 159
> > ++++++++++++++++++++++++++++++++++++++++++++--
> > include/hw/nvram/fw_cfg.h | 5 +- 4 files changed, 194
> > insertions(+), 7 deletions(-)
> >
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 5bc7b96..64d9192 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -132,6 +132,41 @@ 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).
> >
> > += Guest-side DMA Interface =
> > +
> > +== Registers ==
> > +
> > +This does not replace the existing fw_cfg interface, it is an
> > add-on. +Specifically there is no select register in the dma
> > interface, guests +must use the select register of the classic
> > fw_cfg interface instead. +
> > +There are four 32bit registers: Target address (low and high bits),
> > +transfer length, control register.
> > +
> > +== Protocol ==
> > +
> > +Probing for dma support being available: Write target address,
> > read it +back, verify you got back the value you wrote.
>
> In the kernel tree, in
> "Documentation/devicetree/bindings/arm/fw-cfg.txt", we said
>
> > The outermost protocol (involving the write / read sequences of the
> > control and data registers) is expected to be versioned, and/or
> > described by feature bits. The interface revision / feature bitmap
> > can be retrieved with key 0x0001. The 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.
>
> So, I think the DMA interface should be advertised as a new bit in the
> bitmap. (Bit #0 with value 1 should not be used, because that's
> already used -- for nothing, actually -- on x86 targets. But bit #1
> with value 2 should be okay for this.)
>
Ok. When the changes get accepted, I'll also update that document in
the kernel tree.
> > +
> > +Kick a transfer: Write select, target address and transfer length
> > +registers (order doesn't matter). Then flip read bit in the
> > control +register to '1'. Also make sure the error bit is cleared.
> > +
> > +Check result: Read control register:
> > + error bit set -> something went wrong.
> > + all bits cleared -> transfer finished successfully.
> > + otherwise -> transfer still in progress (doesn't happen
> > + today due to implementation not being
> > async,
> > + but may in the future).
> > +
> > +Target address goes up and transfer length goes down as the
> > transfer +happens, so after a successfull
>
> successful[]
>
> > transfer the length register is zero
> > +and the address register points right after the memory block
> > written. +
> > +If a partial transfer happened before an error occured the address
> > and +length registers indicate how much data has been transfered
> > +successfully.
> > +
> > = Host-side API =
> >
> > The following functions are available to the QEMU programmer for
> > adding diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 4846892..374660c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -612,7 +612,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);
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 88481b7..5bcd0e0 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -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"
> > @@ -42,6 +43,18 @@
> > #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 dma registers */
> > +#define FW_CFG_DMA_ADDR_LO 0
> > +#define FW_CFG_DMA_ADDR_HI 4
> > +#define FW_CFG_DMA_LENGTH 8
> > +#define FW_CFG_DMA_CONTROL 12
> > +#define FW_CFG_DMA_SIZE 16
> > +
> > +/* FW_CFG_DMA_CONTROL bits */
> > +#define FW_CFG_DMA_CTL_ERROR 0x01
> > +#define FW_CFG_DMA_CTL_READ 0x02
> > +#define FW_CFG_DMA_CTL_MASK 0x03
> > +
> > typedef struct FWCfgEntry {
> > uint32_t len;
> > uint8_t *data;
> > @@ -59,6 +72,12 @@ struct FWCfgState {
> > uint16_t cur_entry;
> > uint32_t cur_offset;
> > Notifier machine_ready;
> > +
> > + bool dma_enabled;
> > + AddressSpace *dma_as;
> > + dma_addr_t dma_addr;
> > + uint32_t dma_len;
> > + uint32_t dma_ctl;
> > };
> >
> > struct FWCfgIoState {
> > @@ -75,7 +94,10 @@ struct FWCfgMemState {
> > FWCfgState parent_obj;
> > /*< public >*/
> >
> > - MemoryRegion ctl_iomem, data_iomem;
> > + MemoryRegion ctl_iomem, data_iomem, dma_iomem;
> > +#if 0
> > + hwaddr ctl_addr, data_addr, dma_addr;
> > +#endif
>
> What's the goal of this?
I suppose the comment has no goal. It was just left undeleted, and I
haven't seen it.
>
> > uint32_t data_width;
> > MemoryRegionOps wide_data_ops;
> > };
> > @@ -294,6 +316,88 @@ 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;
> > + uint32_t i;
> > +
> > + if (s->dma_ctl & FW_CFG_DMA_CTL_ERROR) {
> > + return;
> > + }
> > + if (!(s->dma_ctl & FW_CFG_DMA_CTL_READ)) {
> > + return;
> > + }
> > +
> > + while (s->dma_len > 0) {
> > + len = s->dma_len;
> > + ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> > + DMA_DIRECTION_FROM_DEVICE);
> > + if (!ptr || !len) {
> > + s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
> > + return;
> > + }
> > +
> > + for (i = 0; i < len; i++) {
> > + ptr[i] = fw_cfg_read(s);
> > + }
> > +
> > + s->dma_addr += i;
> > + s->dma_len -= i;
> > + dma_memory_unmap(s->dma_as, ptr, len,
> > + DMA_DIRECTION_FROM_DEVICE, i);
> > + }
> > + s->dma_ctl = 0;
> > +}
>
> On Aarch64 KVM, is this going to show the same cache coherence
> problems as VGA memory access?
Somebody with more knowledge in this architecture should answer this
question.
Thanks
Marc
