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

[Michael S. Tsirkin]

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.


> >
> >
> >> >
> >> >> +}
> >> >> +
> >> >> +static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
> >> >> +{
> >> >> +     phys_addr_t dma;
> >> >> +     struct fw_cfg_dma *d = NULL;
> >> >> +     ssize_t ret = length;
> >> >> +
> >> >> +     d = kmalloc(sizeof(*d), GFP_KERNEL);
> >> >> +     if (!d) {
> >> >> +             ret = -ENOMEM;
> >> >> +             goto end;
> >> >> +     }
> >> >> +
> >> >> +     *d = (struct fw_cfg_dma) {
> >> >> +             .address = address ? cpu_to_be64(virt_to_phys(address)) : 0,
> >> >> +             .length = cpu_to_be32(length),
> >> >> +             .control = cpu_to_be32(control)
> >> >> +     };
> >> >> +
> >> >> +     dma = virt_to_phys(d);
> >> >
> >> > Pls add docs on why this DMA bypasses the DMA API.
> >>
> >> Peter said in his patch: "fw_cfg device does not need IOMMU
> >> protection, so use physical addresses
> >> always.  That's how QEMU implements fw_cfg.  Otherwise we'll see call
> >> traces during boot."
> >>
> >> Is that enough justification?
> >
> > what are the reasons for the traces exactly though?
> > some kind of explanation should go into comments, and
> > I think it should be a bit more detailed than just "it doesn't
> > work otherwise".
> >
> 
> I can use Peter help here. My understanding is because the qemu fw-cfg
> device doesn't go through iommu when doing DMA op. Whether it should
> or could, I can't answer.
> 
> thanks