Messages in this thread | | | Date | Tue, 13 Feb 2018 17:19:28 +0200 | From | "Michael S. Tsirkin" <> | Subject | Re: [PATCH v13 3/4] fw_cfg: write vmcoreinfo details |
| |
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@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 <mst@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?
| |