On Tue, Feb 13, 2018 at 03:14:03PM +0100, Marc-Andre Lureau wrote:
> 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;
> }
I think you need an rmb after the read of d->control.
> >
> >
> >> >
> >> >> +}
> >> >> +
> >> >> +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
