Re: [PATCH v13 3/4] fw_cfg: write vmcoreinfo details

[Michael S. Tsirkin]

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 <mst@xxxxxxxxxx> 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 <mst@xxxxxxxxxx> 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?