From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55309) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHpFT-0002On-2L for qemu-devel@nongnu.org; Wed, 22 Jul 2015 04:19:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHpFP-0006OB-6r for qemu-devel@nongnu.org; Wed, 22 Jul 2015 04:19:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45991) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHpFO-0006Nx-VQ for qemu-devel@nongnu.org; Wed, 22 Jul 2015 04:19:27 -0400 Date: Wed, 22 Jul 2015 10:19:19 +0200 From: Marc =?UTF-8?B?TWFyw60=?= Message-ID: <20150722101919.3c5ca13b@markmb_rh> In-Reply-To: <55AEA131.3010104@redhat.com> References: <1437494626-3773-1-git-send-email-markmb@redhat.com> <1437494626-3773-3-git-send-email-markmb@redhat.com> <55AEA131.3010104@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: Peter Maydell , Drew Jones , Stefan Hajnoczi , qemu-devel@nongnu.org, 'Kevin O'Connor' , Gerd Hoffmann , Paolo Bonzini On Tue, 21 Jul 2015 21:44:49 +0200 Laszlo Ersek wrote: > On 07/21/15 18:03, Marc Mar=C3=AD wrote: > > From: Gerd Hoffmann > >=20 > > 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. > >=20 > > See docs/specs/fw_cfg.txt update for details. > >=20 > > Possible improvements (can be done incremental): > >=20 > > * 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. > >=20 > > Signed-off-by: Gerd Hoffmann > > --- > > 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(-) > >=20 > > 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). > > =20 > > +=3D Guest-side DMA Interface =3D > > + > > +=3D=3D Registers =3D=3D > > + > > +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. > > + > > +=3D=3D Protocol =3D=3D > > + > > +Probing for dma support being available: Write target address, > > read it +back, verify you got back the value you wrote. >=20 > In the kernel tree, in > "Documentation/devicetree/bindings/arm/fw-cfg.txt", we said >=20 > > 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. >=20 > 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.) >=20 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 >=20 > successful[] >=20 > > 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. > > + > > =3D Host-side API =3D > > =20 > > 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 =3D vbi->memmap[VIRT_FW_CFG].size; > > char *nodename; > > =20 > > - fw_cfg_init_mem_wide(base + 8, base, 8); > > + fw_cfg_init_mem_wide(base + 8, base, 8, 0, NULL); > > =20 > > nodename =3D 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)=20 > > +/* 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; > > }; > > =20 > > struct FWCfgIoState { > > @@ -75,7 +94,10 @@ struct FWCfgMemState { > > FWCfgState parent_obj; > > /*< public >*/ > > =20 > > - MemoryRegion ctl_iomem, data_iomem; > > + MemoryRegion ctl_iomem, data_iomem, dma_iomem; > > +#if 0 > > + hwaddr ctl_addr, data_addr, dma_addr; > > +#endif >=20 > What's the goal of this? I suppose the comment has no goal. It was just left undeleted, and I haven't seen it. >=20 > > 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); > > } > > =20 > > +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 =3D s->dma_len; > > + ptr =3D dma_memory_map(s->dma_as, s->dma_addr, &len, > > + DMA_DIRECTION_FROM_DEVICE); > > + if (!ptr || !len) { > > + s->dma_ctl |=3D FW_CFG_DMA_CTL_ERROR; > > + return; > > + } > > + > > + for (i =3D 0; i < len; i++) { > > + ptr[i] =3D fw_cfg_read(s); > > + } > > + > > + s->dma_addr +=3D i; > > + s->dma_len -=3D i; > > + dma_memory_unmap(s->dma_as, ptr, len, > > + DMA_DIRECTION_FROM_DEVICE, i); > > + } > > + s->dma_ctl =3D 0; > > +} >=20 > 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