Re: [Qemu-devel] [RFC 2/7] fw_cfg dma interface

[Laszlo Ersek]

On 07/22/15 10:19, Marc Marí wrote:
> On Tue, 21 Jul 2015 21:44:49 +0200
> Laszlo Ersek <address@hidden> wrote:
> 
>> On 07/21/15 18:03, Marc Marí wrote:
>>> From: Gerd Hoffmann <address@hidden>
>>>
>>> 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.
>>>
>>> See docs/specs/fw_cfg.txt update for details.
>>>
>>> Possible improvements (can be done incremental):
>>>
>>>  * 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.
>>>
>>> Signed-off-by: Gerd Hoffmann <address@hidden>
>>> ---
>>>  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(-)
>>>
>>> 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
>>> given by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>>>  
>>> += Guest-side DMA Interface =
>>> +
>>> +== Registers ==
>>> +
>>> +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.
>>> +
>>> +== Protocol ==
>>> +
>>> +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.)
>>
> 
> Ok. When the changes get accepted, I'll also update that document in
> the kernel tree.

The kernel documentation is more or less a specification for this
device. The documentation there dictates the implementation here.
Getting the kernel devs with jurisdiction to accept changes for the
binding docs is actually harder than getting the QEMU patches in. I
suggest to start there (or to do it in parallel).

Obviously, I'm not trying to "enforce" such a workflow, I'm just
explaining the method we used last time, when I worked on this device
(and wrote "Documentation/devicetree/bindings/arm/fw-cfg.txt", as Peter
asked).

Thanks
Laszlo

> 
>>> +
>>> +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.
>>> +
>>>  = Host-side API =
>>>  
>>>  The following functions are available to the QEMU programmer for
>>> adding 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("/address@hidden" 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) 
>>> +/* 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;
>>>  };
>>>  
>>>  struct FWCfgIoState {
>>> @@ -75,7 +94,10 @@ struct FWCfgMemState {
>>>      FWCfgState parent_obj;
>>>      /*< public >*/
>>>  
>>> -    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?
> 
> I suppose the comment has no goal. It was just left undeleted, and I
> haven't seen it.
> 
>>
>>>      uint32_t data_width;
>>>      MemoryRegionOps wide_data_ops;
>>>  };
>>> @@ -294,6 +316,88 @@ 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;
>>> +    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 = s->dma_len;
>>> +        ptr = dma_memory_map(s->dma_as, s->dma_addr, &len,
>>> +                             DMA_DIRECTION_FROM_DEVICE);
>>> +        if (!ptr || !len) {
>>> +            s->dma_ctl |= FW_CFG_DMA_CTL_ERROR;
>>> +            return;
>>> +        }
>>> +
>>> +        for (i = 0; i < len; i++) {
>>> +            ptr[i] = fw_cfg_read(s);
>>> +        }
>>> +
>>> +        s->dma_addr += i;
>>> +        s->dma_len  -= i;
>>> +        dma_memory_unmap(s->dma_as, ptr, len,
>>> +                         DMA_DIRECTION_FROM_DEVICE, i);
>>> +    }
>>> +    s->dma_ctl = 0;
>>> +}
>>
>> On Aarch64 KVM, is this going to show the same cache coherence
>> problems as VGA memory access?
> 
> Somebody with more knowledge in this architecture should answer this
> question.
> 
> Thanks
> Marc
>