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

[Stefan Hajnoczi]

On Thu, Aug 6, 2015 at 5:15 PM, Laszlo Ersek <lersek at redhat.com> wrote:
> On 08/06/15 14:35, Stefan Hajnoczi wrote:
>> On Thu, Aug 6, 2015 at 12:02 PM, Marc Marí <markmb at redhat.com> 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