On Thu, 6 Aug 2015 10:47:21 -0400
"Kevin O'Connor" <ke...@koconnor.net> wrote:
> On Thu, Aug 06, 2015 at 01:01:16PM +0200, Marc Marí wrote:
> > Based on the specifications on docs/specs/fw_cfg.txt
> >
> > This interface is an addon. The old interface can still be used as
> > usual.
> >
> > For x86 arch, this addon will use one extra port (0x512). For ARM,
> > it will use 8 more bytes, following the actual implementation.
> >
> > Based on Gerd Hoffman's initial implementation.
> >
> > Signed-off-by: Marc Marí <mar...@redhat.com>
> > ---
> > 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(-)
> >
> > 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..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"
> >
> > -#define FW_CFG_SIZE 2
> > +#define FW_CFG_SIZE 3
> > #define FW_CFG_NAME "fw_cfg"
> > #define FW_CFG_PATH "/machine/" FW_CFG_NAME
> >
> > +#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)
> > +/* 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;
> > };
> >
> > struct FWCfgIoState {
> > @@ -75,7 +88,7 @@ struct FWCfgMemState {
> > FWCfgState parent_obj;
> > /*< public >*/
> >
> > - 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);
> > }
> >
> > +static void fw_cfg_dma_transfer(FWCfgState *s)
> > +{
> > + dma_addr_t len;
> > + uint8_t *ptr;
> > + FWCfgDmaAccess *dma;
> > + int arch = !!(s->cur_entry & FW_CFG_ARCH_LOCAL);
> > + FWCfgEntry *e = &s->entries[arch][s->cur_entry &
> > FW_CFG_ENTRY_MASK]; +
> > + len = sizeof(*dma);
> > + dma = 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) {
>
> 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.
>
> > + while (dma->length > 0) {
> > + if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> > + s->cur_offset >= e->len) {
> > + len = dma->length;
> > + if (dma->address) {
> > + ptr = dma_memory_map(s->dma_as, dma->address, &len,
> > + DMA_DIRECTION_FROM_DEVICE);
> > + if (!ptr || !len) {
> > + dma->control |= FW_CFG_DMA_CTL_ERROR;
>
> 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.
>
> [...]
> > @@ -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 = opaque;
> > + s->dma_addr = le64_to_cpu(value);
> > + fw_cfg_dma_transfer(s);
> > break;
>
> 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.)
>
> 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
