From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44415) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkKLx-0003Hm-0M for qemu-devel@nongnu.org; Thu, 08 Oct 2015 19:12:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkKLs-0002sE-Rp for qemu-devel@nongnu.org; Thu, 08 Oct 2015 19:12:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46128) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkKLs-0002rB-JC for qemu-devel@nongnu.org; Thu, 08 Oct 2015 19:11:56 -0400 References: <1444316523-21711-1-git-send-email-markmb@redhat.com> <1444316578-21775-1-git-send-email-markmb@redhat.com> <1444316578-21775-4-git-send-email-markmb@redhat.com> From: Laszlo Ersek Message-ID: <5616F838.3040806@redhat.com> Date: Fri, 9 Oct 2015 01:11:52 +0200 MIME-Version: 1.0 In-Reply-To: <1444316578-21775-4-git-send-email-markmb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 3/6] Implement 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: Stefan Hajnoczi , Drew , Kevin O'Connor , "Gabriel L. Somlo" , Gerd Hoffmann On 10/08/15 17:02, 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 usua= l. >=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 | 240 ++++++++++++++++++++++++++++++++++++++= +++++--- > include/hw/nvram/fw_cfg.h | 16 +++- > 3 files changed, 244 insertions(+), 14 deletions(-) I diffed this against the corresponding patch in v4, and then compared the result with my v4 comments (and those of Stefan). Reviewed-by: Laszlo Ersek Thanks! Laszlo >=20 > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index d25d6cf..7ae984f 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -683,7 +683,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 658f8c4..d2d1bca 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,7 +31,7 @@ > #include "qemu/error-report.h" > #include "qemu/config-file.h" > =20 > -#define FW_CFG_SIZE 2 > +#define FW_CFG_CTL_SIZE 2 > #define FW_CFG_NAME "fw_cfg" > #define FW_CFG_PATH "/machine/" FW_CFG_NAME > =20 > @@ -42,6 +43,16 @@ > #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_VERSION bits */ > +#define FW_CFG_VERSION 0x01 > +#define FW_CFG_VERSION_DMA 0x02 > + > +/* FW_CFG_DMA_CONTROL bits */ > +#define FW_CFG_DMA_CTL_ERROR 0x01 > +#define FW_CFG_DMA_CTL_READ 0x02 > +#define FW_CFG_DMA_CTL_SKIP 0x04 > +#define FW_CFG_DMA_CTL_SELECT 0x08 > + > typedef struct FWCfgEntry { > uint32_t len; > uint8_t *data; > @@ -59,6 +70,11 @@ struct FWCfgState { > uint16_t cur_entry; > uint32_t cur_offset; > Notifier machine_ready; > + > + bool dma_enabled; > + dma_addr_t dma_addr; > + AddressSpace *dma_as; > + MemoryRegion dma_iomem; > }; > =20 > struct FWCfgIoState { > @@ -67,7 +83,7 @@ struct FWCfgIoState { > /*< public >*/ > =20 > MemoryRegion comb_iomem; > - uint32_t iobase; > + uint32_t iobase, dma_iobase; > }; > =20 > struct FWCfgMemState { > @@ -292,6 +308,122 @@ static void fw_cfg_data_mem_write(void *opaque, h= waddr addr, > } while (i); > } > =20 > +static void fw_cfg_dma_transfer(FWCfgState *s) > +{ > + dma_addr_t len; > + FWCfgDmaAccess dma; > + int arch; > + FWCfgEntry *e; > + int read; > + dma_addr_t dma_addr; > + > + /* Reset the address before the next access */ > + dma_addr =3D s->dma_addr; > + s->dma_addr =3D 0; > + > + if (dma_memory_read(s->dma_as, dma_addr, &dma, sizeof(dma))) { > + stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, cont= rol), > + FW_CFG_DMA_CTL_ERROR); > + return; > + } > + > + dma.address =3D be64_to_cpu(dma.address); > + dma.length =3D be32_to_cpu(dma.length); > + dma.control =3D be32_to_cpu(dma.control); > + > + if (dma.control & FW_CFG_DMA_CTL_SELECT) { > + fw_cfg_select(s, dma.control >> 16); > + } > + > + arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); > + e =3D &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; > + > + if (dma.control & FW_CFG_DMA_CTL_READ) { > + read =3D 1; > + } else if (dma.control & FW_CFG_DMA_CTL_SKIP) { > + read =3D 0; > + } else { > + dma.length =3D 0; > + } > + > + dma.control =3D 0; > + > + while (dma.length > 0 && !(dma.control & FW_CFG_DMA_CTL_ERROR)) { > + if (s->cur_entry =3D=3D FW_CFG_INVALID || !e->data || > + s->cur_offset >=3D e->len) { > + len =3D dma.length; > + > + /* If the access is not a read access, it will be a skip a= ccess, > + * tested before. > + */ > + if (read) { > + if (dma_memory_set(s->dma_as, dma.address, 0, len)) { > + dma.control |=3D FW_CFG_DMA_CTL_ERROR; > + } > + } > + > + } else { > + if (dma.length <=3D (e->len - s->cur_offset)) { > + len =3D dma.length; > + } else { > + len =3D (e->len - s->cur_offset); > + } > + > + if (e->read_callback) { > + e->read_callback(e->callback_opaque, s->cur_offset); > + } > + > + /* If the access is not a read access, it will be a skip a= ccess, > + * tested before. > + */ > + if (read) { > + if (dma_memory_write(s->dma_as, dma.address, > + &e->data[s->cur_offset], len)) { > + dma.control |=3D FW_CFG_DMA_CTL_ERROR; > + } > + } > + > + s->cur_offset +=3D len; > + } > + > + dma.address +=3D len; > + dma.length -=3D len; > + > + } > + > + stl_be_dma(s->dma_as, dma_addr + offsetof(FWCfgDmaAccess, control)= , > + dma.control); > + > + trace_fw_cfg_read(s, 0); > +} > + > +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > + FWCfgState *s =3D opaque; > + > + if (size =3D=3D 4) { > + if (addr =3D=3D 0) { > + /* FWCfgDmaAccess high address */ > + s->dma_addr =3D value << 32; > + } else if (addr =3D=3D 4) { > + /* FWCfgDmaAccess low address */ > + s->dma_addr |=3D value; > + fw_cfg_dma_transfer(s); > + } > + } else if (size =3D=3D 8 && addr =3D=3D 0) { > + s->dma_addr =3D value; > + fw_cfg_dma_transfer(s); > + } > +} > + > +static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr, > + unsigned size, bool is_write) > +{ > + return is_write && ((size =3D=3D 4 && (addr =3D=3D 0 || addr =3D=3D= 4)) || > + (size =3D=3D 8 && addr =3D=3D 0)); > +} > + > static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr, > unsigned size, bool is_write) > { > @@ -359,6 +491,14 @@ 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 { > + .write =3D fw_cfg_dma_mem_write, > + .endianness =3D DEVICE_BIG_ENDIAN, > + .valid.accepts =3D fw_cfg_dma_mem_valid, > + .valid.max_access_size =3D 8, > + .impl.max_access_size =3D 8, > +}; > + > static void fw_cfg_reset(DeviceState *d) > { > FWCfgState *s =3D FW_CFG(d); > @@ -399,6 +539,22 @@ static bool is_version_1(void *opaque, int version= _id) > return version_id =3D=3D 1; > } > =20 > +static bool fw_cfg_dma_enabled(void *opaque) > +{ > + FWCfgState *s =3D opaque; > + > + return s->dma_enabled; > +} > + > +static VMStateDescription vmstate_fw_cfg_dma =3D { > + .name =3D "fw_cfg/dma", > + .needed =3D fw_cfg_dma_enabled, > + .fields =3D (VMStateField[]) { > + VMSTATE_UINT64(dma_addr, FWCfgState), > + VMSTATE_END_OF_LIST() > + }, > +}; > + > static const VMStateDescription vmstate_fw_cfg =3D { > .name =3D "fw_cfg", > .version_id =3D 2, > @@ -408,6 +564,10 @@ 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 (const VMStateDescription*[]) { > + &vmstate_fw_cfg_dma, > + NULL, > } > }; > =20 > @@ -593,7 +753,6 @@ static void fw_cfg_init1(DeviceState *dev) > qdev_init_nofail(dev); > =20 > fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); > - fw_cfg_add_i32(s, FW_CFG_ID, 1); > fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16); > fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type =3D=3D= DT_NOGRAPHIC)); > fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus); > @@ -605,25 +764,53 @@ static void fw_cfg_init1(DeviceState *dev) > qemu_add_machine_init_done_notifier(&s->machine_ready); > } > =20 > -FWCfgState *fw_cfg_init_io(uint32_t iobase) > +FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase, > + AddressSpace *dma_as) > { > DeviceState *dev; > + FWCfgState *s; > + uint32_t version =3D FW_CFG_VERSION; > + bool dma_enabled =3D dma_iobase && dma_as; > =20 > dev =3D qdev_create(NULL, TYPE_FW_CFG_IO); > qdev_prop_set_uint32(dev, "iobase", iobase); > + qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase); > + qdev_prop_set_bit(dev, "dma_enabled", dma_enabled); > + > fw_cfg_init1(dev); > + s =3D FW_CFG(dev); > + > + if (dma_enabled) { > + /* 64 bits for the address field */ > + s->dma_as =3D dma_as; > + s->dma_addr =3D 0; > + > + version |=3D FW_CFG_VERSION_DMA; > + } > + > + fw_cfg_add_i32(s, FW_CFG_ID, version); > =20 > - return FW_CFG(dev); > + return s; > +} > + > +FWCfgState *fw_cfg_init_io(uint32_t iobase) > +{ > + return fw_cfg_init_io_dma(iobase, 0, NULL); > } > =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; > + FWCfgState *s; > + uint32_t version =3D FW_CFG_VERSION; > + bool dma_enabled =3D dma_addr && dma_as; > =20 > dev =3D qdev_create(NULL, TYPE_FW_CFG_MEM); > qdev_prop_set_uint32(dev, "data_width", data_width); > + qdev_prop_set_bit(dev, "dma_enabled", dma_enabled); > =20 > fw_cfg_init1(dev); > =20 > @@ -631,13 +818,25 @@ 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 > - return FW_CFG(dev); > + s =3D FW_CFG(dev); > + > + if (dma_enabled) { > + s->dma_as =3D dma_as; > + s->dma_addr =3D 0; > + sysbus_mmio_map(sbd, 2, dma_addr); > + version |=3D FW_CFG_VERSION_DMA; > + } > + > + fw_cfg_add_i32(s, FW_CFG_ID, version); > + > + return s; > } > =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 > @@ -664,6 +863,9 @@ static const TypeInfo fw_cfg_info =3D { > =20 > static Property fw_cfg_io_properties[] =3D { > DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1), > + DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1), > + DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabl= ed, > + false), > DEFINE_PROP_END_OF_LIST(), > }; > =20 > @@ -673,8 +875,15 @@ static void fw_cfg_io_realize(DeviceState *dev, Er= ror **errp) > SysBusDevice *sbd =3D SYS_BUS_DEVICE(dev); > =20 > memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_= ops, > - FW_CFG(s), "fwcfg", FW_CFG_SIZE); > + FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE); > sysbus_add_io(sbd, s->iobase, &s->comb_iomem); > + > + if (FW_CFG(s)->dma_enabled) { > + memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s), > + &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.d= ma", > + sizeof(dma_addr_t)); > + sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem); > + } > } > =20 > static void fw_cfg_io_class_init(ObjectClass *klass, void *data) > @@ -695,6 +904,8 @@ static const TypeInfo fw_cfg_io_info =3D { > =20 > static Property fw_cfg_mem_properties[] =3D { > DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1), > + DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enab= led, > + false), > DEFINE_PROP_END_OF_LIST(), > }; > =20 > @@ -705,7 +916,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Er= ror **errp) > const MemoryRegionOps *data_ops =3D &fw_cfg_data_mem_ops; > =20 > memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_op= s, > - FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE); > + FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE); > sysbus_init_mmio(sbd, &s->ctl_iomem); > =20 > if (s->data_width > data_ops->valid.max_access_size) { > @@ -723,6 +934,13 @@ 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); > + > + if (FW_CFG(s)->dma_enabled) { > + memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s), > + &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.d= ma", > + sizeof(dma_addr_t)); > + sysbus_init_mmio(sbd, &FW_CFG(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..ee0cd8a 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -61,6 +61,15 @@ typedef struct FWCfgFiles { > FWCfgFile f[]; > } FWCfgFiles; > =20 > +/* Control as first field allows for different structures selected by = this > + * field, which might be useful in the future > + */ > +typedef struct FWCfgDmaAccess { > + uint32_t control; > + uint32_t length; > + uint64_t address; > +} QEMU_PACKED FWCfgDmaAccess; > + > typedef void (*FWCfgCallback)(void *opaque, uint8_t *data); > typedef void (*FWCfgReadCallback)(void *opaque, uint32_t offset); > =20 > @@ -77,10 +86,13 @@ void fw_cfg_add_file_callback(FWCfgState *s, const = char *filename, > void *data, size_t len); > void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *da= ta, > size_t len); > +FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase, > + AddressSpace *dma_as); > 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