lkml.org 
[lkml]   [2018]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v13 3/4] fw_cfg: write vmcoreinfo details
    On Tue, Feb 13, 2018 at 4:19 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
    > 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?

    That's the only thing that indicate completion it seems:

    https://git.qemu.org/?p=qemu.git;a=blob;f=hw/nvram/fw_cfg.c;hb=HEAD#l389

    The spec doesn't describe other ways either:

    https://git.qemu.org/?p=qemu.git;a=blob;f=docs/specs/fw_cfg.txt

    \
     
     \ /
      Last update: 2018-02-13 16:36    [W:2.982 / U:0.004 seconds]
    ©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site