From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53452) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHdTK-00038b-1r for qemu-devel@nongnu.org; Tue, 21 Jul 2015 15:45:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHdTG-00068M-QZ for qemu-devel@nongnu.org; Tue, 21 Jul 2015 15:45:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41816) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHdTG-00068H-GE for qemu-devel@nongnu.org; Tue, 21 Jul 2015 15:44:58 -0400 References: <1437494626-3773-1-git-send-email-markmb@redhat.com> <1437494626-3773-3-git-send-email-markmb@redhat.com> From: Laszlo Ersek Message-ID: <55AEA131.3010104@redhat.com> Date: Tue, 21 Jul 2015 21:44:49 +0200 MIME-Version: 1.0 In-Reply-To: <1437494626-3773-3-git-send-email-markmb@redhat.com> Content-Type: text/plain; charset=windows-1252 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: =?UTF-8?Q?Marc_Mar=c3=ad?= , qemu-devel@nongnu.org Cc: Peter Maydell , Drew Jones , Stefan Hajnoczi , 'Kevin O'Connor' , Gerd Hoffmann , Paolo Bonzini On 07/21/15 18:03, Marc Mar=ED 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 giv= en > 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. 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.) > + > +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. > + > =3D Host-side API =3D > =20 > The following functions are available to the QEMU programmer for addin= g > 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 What's the goal of this? > uint32_t data_width; > MemoryRegionOps wide_data_ops; > }; > @@ -294,6 +316,88 @@ static void fw_cfg_data_mem_write(void *opaque, hw= addr 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; > +} On Aarch64 KVM, is this going to show the same cache coherence problems as VGA memory access? ... I definitely don't claim that I understand the rest of the patch, but for now I don't have more questions. :) Thanks! Laszlo > + > +static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr, > + unsigned size) > +{ > + FWCfgState *s =3D opaque; > + uint64_t ret =3D 0; > + > + switch (addr) { > + case FW_CFG_DMA_ADDR_LO: > + ret =3D s->dma_addr & 0xffffffff; > + break; > + case FW_CFG_DMA_ADDR_HI: > + ret =3D (s->dma_addr >> 32) & 0xffffffff; > + break; > + case FW_CFG_DMA_LENGTH: > + ret =3D s->dma_len; > + break; > + case FW_CFG_DMA_CONTROL: > + ret =3D s->dma_ctl; > + break; > + } > + return ret; > +} > + > +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > + FWCfgState *s =3D opaque; > + > + switch (addr) { > + case FW_CFG_DMA_ADDR_LO: > + s->dma_addr &=3D ~((dma_addr_t)0xffffffff); > + s->dma_addr |=3D value; > + break; > + case FW_CFG_DMA_ADDR_HI: > + s->dma_addr &=3D ~(((dma_addr_t)0xffffffff) << 32); > + s->dma_addr |=3D ((dma_addr_t)value) << 32; > + break; > + case FW_CFG_DMA_LENGTH: > + s->dma_len =3D value; > + break; > + case FW_CFG_DMA_CONTROL: > + value &=3D FW_CFG_DMA_CTL_MASK; > + s->dma_ctl =3D value; > + fw_cfg_dma_transfer(s); > + break; > + } > +} > + > static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr, > unsigned size, bool is_write) > { > @@ -361,6 +465,16 @@ static const MemoryRegionOps fw_cfg_comb_mem_ops =3D= { > .valid.accepts =3D fw_cfg_comb_valid, > }; > =20 > +static const MemoryRegionOps fw_cfg_dma_mem_ops =3D { > + .read =3D fw_cfg_dma_mem_read, > + .write =3D fw_cfg_dma_mem_write, > + .endianness =3D DEVICE_BIG_ENDIAN, > + .valid =3D { > + .min_access_size =3D 4, > + .max_access_size =3D 4, > + }, > +}; > + > static void fw_cfg_reset(DeviceState *d) > { > FWCfgState *s =3D FW_CFG(d); > @@ -401,6 +515,23 @@ static bool is_version_1(void *opaque, int version= _id) > return version_id =3D=3D 1; > } > =20 > +static VMStateDescription vmstate_fw_cfg_dma =3D { > + .name =3D "fw_cfg/dma", > + .fields =3D (VMStateField[]) { > + VMSTATE_UINT64(dma_addr, FWCfgState), > + VMSTATE_UINT32(dma_len, FWCfgState), > + VMSTATE_UINT32(dma_ctl, FWCfgState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static bool fw_cfg_dma_enabled(void *opaque) > +{ > + FWCfgState *s =3D opaque; > + > + return s->dma_enabled; > +} > + > static const VMStateDescription vmstate_fw_cfg =3D { > .name =3D "fw_cfg", > .version_id =3D 2, > @@ -410,6 +541,14 @@ static const VMStateDescription vmstate_fw_cfg =3D= { > VMSTATE_UINT16_HACK(cur_offset, FWCfgState, is_version_1), > VMSTATE_UINT32_V(cur_offset, FWCfgState, 2), > VMSTATE_END_OF_LIST() > + }, > + .subsections =3D (VMStateSubsection[]) { > + { > + .vmsd =3D &vmstate_fw_cfg_dma, > + .needed =3D fw_cfg_dma_enabled, > + }, { > + /* end of list */ > + } > } > }; > =20 > @@ -618,8 +757,9 @@ FWCfgState *fw_cfg_init_io(uint32_t iobase) > return FW_CFG(dev); > } > =20 > -FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr, > - uint32_t data_width) > +FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, > + hwaddr data_addr, uint32_t data_width= , > + hwaddr dma_addr, AddressSpace *dma_as= ) > { > DeviceState *dev; > SysBusDevice *sbd; > @@ -633,13 +773,20 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,= hwaddr data_addr, > sysbus_mmio_map(sbd, 0, ctl_addr); > sysbus_mmio_map(sbd, 1, data_addr); > =20 > + if (dma_addr && dma_as) { > + FW_CFG(dev)->dma_as =3D dma_as; > + FW_CFG(dev)->dma_enabled =3D true; > + sysbus_mmio_map(sbd, 1, dma_addr); > + } > + > return FW_CFG(dev); > } > =20 > FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr) > { > return fw_cfg_init_mem_wide(ctl_addr, data_addr, > - fw_cfg_data_mem_ops.valid.max_access_s= ize); > + fw_cfg_data_mem_ops.valid.max_access_s= ize, > + 0, NULL); > } > =20 > =20 > @@ -725,6 +872,10 @@ static void fw_cfg_mem_realize(DeviceState *dev, E= rror **errp) > memory_region_init_io(&s->data_iomem, OBJECT(s), data_ops, FW_CFG(= s), > "fwcfg.data", data_ops->valid.max_access_siz= e); > sysbus_init_mmio(sbd, &s->data_iomem); > + > + memory_region_init_io(&s->dma_iomem, OBJECT(s), &fw_cfg_dma_mem_op= s, > + FW_CFG(s), "fwcfg.dma", FW_CFG_DMA_SIZE); > + sysbus_init_mmio(sbd, &s->dma_iomem); > } > =20 > static void fw_cfg_mem_class_init(ObjectClass *klass, void *data) > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index e60d3ca..2fe31ea 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -79,8 +79,9 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *f= ilename, void *data, > size_t len); > FWCfgState *fw_cfg_init_io(uint32_t iobase); > FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr data_addr); > -FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, hwaddr data_addr, > - uint32_t data_width); > +FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, > + hwaddr data_addr, uint32_t data_width= , > + hwaddr dma_addr, AddressSpace *dma_as= ); > =20 > FWCfgState *fw_cfg_find(void); > =20 >=20