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

[Marc-Andre Lureau]

On Tue, Feb 13, 2018 at 4:19 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> 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@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?

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