From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNRJT-0000OQ-MX for qemu-devel@nongnu.org; Thu, 06 Aug 2015 15:58:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNRJM-0007J1-Sh for qemu-devel@nongnu.org; Thu, 06 Aug 2015 15:58:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40813) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNRJM-0007Ip-LA for qemu-devel@nongnu.org; Thu, 06 Aug 2015 15:58:44 -0400 Date: Thu, 6 Aug 2015 16:59:15 +0200 From: Marc =?UTF-8?B?TWFyw60=?= Message-ID: <20150806165915.516cc97c@markmb_rh> In-Reply-To: <20150806144721.GB3606@morn.localdomain> References: <1438858816-29385-1-git-send-email-markmb@redhat.com> <1438858878-29450-1-git-send-email-markmb@redhat.com> <1438858878-29450-4-git-send-email-markmb@redhat.com> <20150806144721.GB3606@morn.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/5] Implement fw_cfg DMA interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: Stefan Hajnoczi , Drew , Laszlo , qemu-devel@nongnu.org, Gerd Hoffmann On Thu, 6 Aug 2015 10:47:21 -0400 "Kevin O'Connor" wrote: > On Thu, Aug 06, 2015 at 01:01:16PM +0200, Marc Mar=C3=AD wrote: > > Based on the specifications on docs/specs/fw_cfg.txt > >=20 > > This interface is an addon. The old interface can still be used as > > usual. > >=20 > > For x86 arch, this addon will use one extra port (0x512). For ARM, > > it will use 8 more bytes, following the actual implementation. > >=20 > > Based on Gerd Hoffman's initial implementation. > >=20 > > Signed-off-by: Marc Mar=C3=AD > > --- > > hw/arm/virt.c | 2 +- > > hw/nvram/fw_cfg.c | 212 > > +++++++++++++++++++++++++++++++++++++++++++--- > > include/hw/nvram/fw_cfg.h | 12 ++- 3 files changed, 211 > > insertions(+), 15 deletions(-) > >=20 > > 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..7399008 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" > > @@ -30,10 +31,13 @@ > > #include "qemu/error-report.h" > > #include "qemu/config-file.h" > > =20 > > -#define FW_CFG_SIZE 2 > > +#define FW_CFG_SIZE 3 > > #define FW_CFG_NAME "fw_cfg" > > #define FW_CFG_PATH "/machine/" FW_CFG_NAME > > =20 > > +#define FW_CFG_VERSION 1 > > +#define FW_CFG_VERSION_DMA 2 > > + > > #define TYPE_FW_CFG "fw_cfg" > > #define TYPE_FW_CFG_IO "fw_cfg_io" > > #define TYPE_FW_CFG_MEM "fw_cfg_mem" > > @@ -42,6 +46,11 @@ > > #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_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 +68,10 @@ struct FWCfgState { > > uint16_t cur_entry; > > uint32_t cur_offset; > > Notifier machine_ready; > > + > > + bool dma_enabled; > > + AddressSpace *dma_as; > > + dma_addr_t dma_addr; > > }; > > =20 > > struct FWCfgIoState { > > @@ -75,7 +88,7 @@ struct FWCfgMemState { > > FWCfgState parent_obj; > > /*< public >*/ > > =20 > > - MemoryRegion ctl_iomem, data_iomem; > > + MemoryRegion ctl_iomem, data_iomem, dma_iomem; > > uint32_t data_width; > > MemoryRegionOps wide_data_ops; > > }; > > @@ -294,6 +307,108 @@ 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; > > + FWCfgDmaAccess *dma; > > + int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > > + FWCfgEntry *e =3D &s->entries[arch][s->cur_entry & > > FW_CFG_ENTRY_MASK]; + > > + len =3D sizeof(*dma); > > + dma =3D dma_memory_map(s->dma_as, s->dma_addr, &len, > > + DMA_DIRECTION_FROM_DEVICE); > > + > > + if (!dma || !len) { > > + return; > > + } > > + > > + if (dma->control & FW_CFG_DMA_CTL_ERROR) { >=20 > I think the dma structure would need to be copied to a local instance > of the struct and the fields endianness updated. (Also, I think the > documentation should state the required endianness of the fields.) > I'm not an expert at this, but if this is not done, I think there > could be problems both with endian and with unaligned memory accesses > in the host. >=20 > > + while (dma->length > 0) { > > + if (s->cur_entry =3D=3D FW_CFG_INVALID || !e->data || > > + s->cur_offset >=3D e->len) { > > + len =3D dma->length; > > + if (dma->address) { > > + ptr =3D dma_memory_map(s->dma_as, dma->address, &len, > > + DMA_DIRECTION_FROM_DEVICE); > > + if (!ptr || !len) { > > + dma->control |=3D FW_CFG_DMA_CTL_ERROR; >=20 > Can you write to "dma->control" if the memory is mapped with > DMA_DIRECTION_FROM_DEVICE? Also, since the control field update > should be atomic, I think the dma struct should probably always be at > least 4 byte aligned and the documentation should be updated to > reflect that. >=20 > [...] > > @@ -321,12 +436,20 @@ static uint64_t fw_cfg_comb_read(void > > *opaque, hwaddr addr, static void fw_cfg_comb_write(void *opaque, > > hwaddr addr, uint64_t value, unsigned size) > > { > > - switch (size) { > > + FWCfgState *s; > > + > > + switch (addr) { > > + case 0: > > + fw_cfg_select(opaque, (uint16_t)value); > > + break; > > case 1: > > fw_cfg_write(opaque, (uint8_t)value); > > break; > > case 2: > > - fw_cfg_select(opaque, (uint16_t)value); > > + /* FWCfgDmaAccess address */ > > + s =3D opaque; > > + s->dma_addr =3D le64_to_cpu(value); > > + fw_cfg_dma_transfer(s); > > break; >=20 > There is no ability to perfrom 64bit IO writes on x86 (to the best of > my knowledge). So, I think this code needs to be able to handle a > 32bit write to a high bits address and then store those bits until the > 32bit write to the low bits address occurs. (I'd also recommend that > after every dma transfer the stored high bits are reset to zero so > that the common case of a 32bit address can be performed with a single > 32bit write to the low bits field.) >=20 > Also, it's very unusual to see 32bit writes to an unaligned IO address > - I think two pad bytes should be added so that the offset for the dma > address is at position 4 (instead of 2). This is a PIO port (out), not a MMIO access (write). Maybe I'm wrong, but I don't think it matters to have the port number aligned with the write size. Thanks Marc