Re: [Qemu-devel] [PATCH v2 3/5] Implement fw_cfg DMA interface

[Peter Maydell]

On 31 August 2015 at 10:10, Marc Marí <address@hidden> 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.
>
> Based on Gerd Hoffman's initial implementation.
>
> Signed-off-by: Marc Marí <address@hidden>

> @@ -294,6 +309,142 @@ 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;
> +    void *addr;
> +    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);
> +    addr = dma_memory_map(s->dma_as, s->dma_addr, &len,
> +                                DMA_DIRECTION_FROM_DEVICE);
> +
> +    s->dma_addr = 0;
> +
> +    if (!addr || !len) {
> +        return;
> +    }
> +
> +    dma.address = be64_to_cpu(*(uint64_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, address)));
> +    dma.length = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, length)));
> +    dma.control = be32_to_cpu(*(uint32_t *)(addr +
> +                    offsetof(FWCfgDmaAccess, control)));

There are only three fields in this struct, so rather than
using dma_memory_map/unmap and manual byteswapping, how about:

 dma.control = ldl_be_dma(s->dma_as, s->dma_addr);
 dma.length = ldl_be_dma(s->dma_as, s->dma_addr + 4);
 dma.address = ldq_be_dma(s->dma_as, s->dma_addr + 8);

Kevin's suggestion to use dma_memory_read() would be OK as well,
if you really want to check for the return value from the memory
transaction. But you probably don't care because if the guest has
pointed the dma address register into nowhere then it deserves
whatever it gets as long as we don't fall over; and we need to
not fall over whatever the values of control/length/address are.
In any case, please don't use dma_memory_map/unmap.

There are st*_ versions for writing back updated values.

> +
> +    if (dma.control & FW_CFG_DMA_CTL_ERROR) {
> +        goto out;
> +    }
> +
> +    if (!(dma.control & (FW_CFG_DMA_CTL_READ | FW_CFG_DMA_CTL_SKIP))) {
> +        goto out;
> +    }
> +
> +    while (dma.length > 0) {
> +        if (s->cur_entry == FW_CFG_INVALID || !e->data ||
> +                                s->cur_offset >= e->len) {
> +            len = dma.length;
> +
> +            /* If the access is not a read access, it will be a skip access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memset(ptr, 0, len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

dma_memory_write() would be better than map-zero-unmap I think,
though you would need to have a handy buffer of zeroes to pass
to that function, so maybe not.

> +            }
> +
> +        } else {
> +            if (dma.length <= e->len) {
> +                len = dma.length;
> +            } else {
> +                len = e->len;
> +            }
> +
> +            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 access,
> +             * tested before.
> +             */
> +            if (dma.control & FW_CFG_DMA_CTL_READ) {
> +                ptr = dma_memory_map(s->dma_as, dma.address, &len,
> +                                     DMA_DIRECTION_FROM_DEVICE);
> +                if (!ptr || !len) {
> +                    dma.control |= FW_CFG_DMA_CTL_ERROR;
> +                    goto out;
> +                }
> +
> +                memcpy(ptr, &e->data[s->cur_offset], len);
> +
> +                dma_memory_unmap(s->dma_as, ptr, len,
> +                                 DMA_DIRECTION_FROM_DEVICE, len);

...and dma_memory_write() is definitely better than
map-memcpy-unmap.

> +            }
> +
> +            s->cur_offset += len;
> +
> +        }
> +
> +        dma.address += len;
> +        dma.length  -= len;
> +        dma.control = 0;
> +
> +        *(uint64_t *)(addr + offsetof(FWCfgDmaAccess, address)) =
> +                                                cpu_to_be64(dma.address);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, length)) =
> +                                                cpu_to_be32(dma.length);
> +        *(uint32_t *)(addr + offsetof(FWCfgDmaAccess, control)) =
> +                                                cpu_to_be32(dma.control);
> +    }
> +
> +    trace_fw_cfg_read(s, 0);
> +
> +out:
> +    dma_memory_unmap(s->dma_as, addr, sizeof(dma),
> +                                DMA_DIRECTION_FROM_DEVICE, sizeof(dma));
> +    return;
> +
> +}
> +
> +static void fw_cfg_dma_mem_write(void *opaque, hwaddr addr,
> +                                 uint64_t value, unsigned size)
> +{
> +    FWCfgState *s = opaque;
> +
> +    if (size == 4) {
> +        if (addr == 0) {
> +            /* FWCfgDmaAccess high address */
> +            s->dma_addr = (0xFFFFFFFF & value) << 32;

The mask here is unnecessary, since you're shifting by 32 bits.

> +        } else if (addr == 4) {
> +            /* FWCfgDmaAccess low address */
> +            s->dma_addr |= value;
> +            fw_cfg_dma_transfer(s);
> +        }
> +    } else if (size == 8 && addr == 0) {
> +        s->dma_addr = 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 == 4 && (addr == 0 || addr == 4)) ||
> +                        (size == 8 && addr == 0));
> +}
> +
>  static bool fw_cfg_data_mem_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> @@ -321,20 +472,35 @@ 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);
> +    case 4:
> +        /* FWCfgDmaAccess low address */
> +        s = opaque;
> +        s->dma_addr |= value;
> +        fw_cfg_dma_transfer(s);
>          break;
> +    case 8:
> +        /* FWCfgDmaAccess high address */
> +        s = opaque;
> +        s->dma_addr = (0xFFFFFFFF & value) << 32;
>      }
>  }
>
>  static bool fw_cfg_comb_valid(void *opaque, hwaddr addr,
>                                    unsigned size, bool is_write)
>  {
> -    return (size == 1) || (is_write && size == 2);
> +    FWCfgState *s = opaque;
> +
> +    return (size == 1) || (is_write && size == 2) ||
> +            (s->dma_enabled && is_write && addr >= 4);

This doesn't look right, since it will allow 1 and 2 byte
writes to the dma_address port, which doesn't seem like
a great idea.

Maybe it would be easier to make the dma port its own
memory region (and then init and sysbus_add_io() it in
fw_cfg_io_realize()) ? Then you wouldn't need to mess with
the already confusing logic for handling combined config/data
ports (which have to be handled together because of the weird
"1 byte accesses are data and 2 byte accesses are config"
semantics).

thanks
-- PMM