Re: [SeaBIOS] [PATCH 1/2] Add QEMU fw_cfg DMA interface

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 5:15 PM, Laszlo Ersek  wrote:
> On 08/06/15 14:35, Stefan Hajnoczi wrote:
> > On Thu, Aug 6, 2015 at 12:02 PM, Marc Marí  wrote:
> > > if (qemu_cfg_dma_enabled()) {
> > >    QemuCfgDmaAccess access;
> > >    access.address = (u64)(u32)buf;
> > >    access.length = len;
> > >    access.control = QEMU_CFG_DMA_CTL_READ;
> > >    /*
> > >     * The out is done before the write of the variables on memory. This
> > >     * causes misread on the QEMU side.
> > >     */
> > >    barrier();
> > >    outl((u32)&access, PORT_QEMU_CFG_DMA_ADDR);
> >
> > I thought PORT_QEMU_CFG_DMA_ADDR is a 64-bit register according to the
> > spec you posted?
> > >    while(access.length != 0 && !(access.control & QEMU_CFG_DMA_CTL_ERROR));
> >
> > Either the field accesses need to be marked volatile, or a barrier is
> > needed to force the compiler to reload these register from memory each
> > iteration of the loop.
>
> Better yet, I would recommend declaring (and here, also defining)
> "access" as "volatile".
>
> volatile QemuCfgDmaAccess access;
> > I think the problem is that there are no "sequence points" (according
> > to the C language specification) in this loop, so the compiler may
> > assume that access.length and access.control will not change.
> > https://en.wikipedia.org/wiki/Sequence_point
>
> The loop does contain sequence points; three in total.
>
> Appendix C of the ISO C spec gives an informative summary on the
> sequence points (with references to the individual normative passages).
> Quoting from appendix C:
>
> 1 The following are the sequence points described in 5.1.2.3:
> [...]
>
> - The end of the first operand of the following operators: logical
>   AND && (6.5.13); [...]
>
> [...]
>
> - The end of a full expression: [...] the expression in an
>   expression statement (6.8.3); [...] the controlling expression of
>   a while or do statement (6.8.5); [...]
>
> Here we have the controlling expression of a while statement.
> In that expression, we also have a logical AND operator.
> And the null statement in the loop body is an expression statement.
>
> The lack of sequence points is not the problem (we do have sequence
> points). The problem is that without "volatile", the compiler might
> optimize out the accesses, because it deduces that the expressions
> produce no side effects.
>
> 5.1.2.3 Program execution
>
> [...]
>
> 2 Accessing a volatile object, [...] are all side effects, which are
>     changes in the state of the execution environment. Evaluation of an
>     expression may produce side effects. At certain specified points in
>     the execution sequence called sequence points, all side effects of
>     previous evaluations shall be complete and no side effects of
>     subsequent evaluations shall have taken place. [...]
>
> 3 In the abstract machine, all expressions are evaluated as specified
>     by the semantics. An actual implementation need not evaluate part
>     of an expression if it can deduce that its value is not used and
>     that no needed side effects are produced (including any caused by
>     calling a function or accessing a volatile object).

You are right.  I misremembered what sequence points mean.  Thanks for
the quotes from the standard.

Stefan