Hi
On Mon, Feb 12, 2018 at 10:00 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
> On Mon, Feb 12, 2018 at 11:04:49AM +0100, Marc-Andre Lureau wrote:
>> >> +}
>> >> +
>> >> +/* qemu fw_cfg device is sync today, but spec says it may become async */
>> >> +static void fw_cfg_wait_for_control(struct fw_cfg_dma *d)
>> >> +{
>> >> + do {
>> >> + u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
>> >> +
>> >> + if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
>> >> + return;
>> >> +
>> >> + usleep_range(50, 100);
>> >> + } while (true);
>> >
>> > And you need an smp rmb here.
>
> I'd just do rmb() in fact.
>
>> Could you explain? thanks
>
> See Documentation/memory-barriers.txt
> You know that control is valid, but following read of
> the structure could be reordered. So you need that barrier there.
> Same for write: wmb.
Is this ok?
@@ -103,10 +104,14 @@ static ssize_t fw_cfg_dma_transfer(void
*address, u32 length, u32 control)
dma = virt_to_phys(d);
iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
+ /* force memory to sync before notifying device via MMIO */
+ wmb();
iowrite32be(dma, fw_cfg_reg_dma + 4);
fw_cfg_wait_for_control(d);
+ /* do not reorder the read to d->control */
+ rmb();
if (be32_to_cpu(READ_ONCE(d->control)) & FW_CFG_DMA_CTL_ERROR) {
ret = -EIO;
}
>
>
>> >
>> >> +}
>> >> +
>> >> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32
>> >> control)
>> >> +{
>> >> + phys_addr_t dma;
>> >> + struct fw_cfg_dma *d = NULL;
>> >> + ssize_t ret = length;
>> >> +
>> >> + d = kmalloc(sizeof(*d), GFP_KERNEL);
>> >> + if (!d) {
>> >> + ret = -ENOMEM;
>> >> + goto end;
>> >> + }
>> >> +
>> >> + *d = (struct fw_cfg_dma) {
>> >> + .address = address ? cpu_to_be64(virt_to_phys(address)) : 0,
>> >> + .length = cpu_to_be32(length),
>> >> + .control = cpu_to_be32(control)
>> >> + };
>> >> +
>> >> + dma = virt_to_phys(d);
>> >
>> > Pls add docs on why this DMA bypasses the DMA API.
>>
>> Peter said in his patch: "fw_cfg device does not need IOMMU
>> protection, so use physical addresses
>> always. That's how QEMU implements fw_cfg. Otherwise we'll see call
>> traces during boot."
>>
>> Is that enough justification?
>
> what are the reasons for the traces exactly though?
> some kind of explanation should go into comments, and
> I think it should be a bit more detailed than just "it doesn't
> work otherwise".
>
I can use Peter help here. My understanding is because the qemu fw-cfg
device doesn't go through iommu when doing DMA op. Whether it should
or could, I can't answer.
thanks
