On Tue, Feb 13, 2018 at 04:16:08PM +0100, Marc-Andre Lureau wrote:
> Hi
>
> On Tue, Feb 13, 2018 at 3:27 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
> > 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.
> >
>
>
> There are two reads of d->control, one in fw_cfg_wait_for_control() to
> wait for completion, and the other one here to handle error. Do you
> mean that for clarity rmb() should be moved at the end of
> fw_cfg_wait_for_control() instead?
>
> thanks
IMHO that's a reasonable way to do it, yes.
OTOH is looking at DMA data the only way to detect DMA is complete?
Isn't there an IO register for that?
