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

[Laszlo Ersek]

On 08/06/15 20:59, Kevin O'Connor wrote:
> On Thu, Aug 06, 2015 at 06:15:57PM +0200, Laszlo Ersek 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'd prefer to avoid volatile for the reasons described at:
> 
>   https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt

Even that document concedes that "volatile" is appropriate for memory
mapped IO.

Personally, the way I distinguish "volatile" from atomics and SMP
barriers is this: in his presentations entitled "atomic<> Weapons":

https://channel9.msdn.com/Shows/Going+Deep/Cpp-and-Beyond-2012-Herb-Sutter-atomic-Weapons-1-of-2
https://channel9.msdn.com/Shows/Going+Deep/Cpp-and-Beyond-2012-Herb-Sutter-atomic-Weapons-2-of-2

Herb Sutter said (I'm paraphrasing from memory): "use volatile for
memory mapped IO, that is, for communication between a CPU and a device.
Use atomics, memory barriers, locking etc. for synchronization between
CPUs, eg. with multi-threaded code."

I have found that a very concise and helpful pair of sentences.

In this instance, we're definitely doing IO, so volatile is appropriate.

Clearly though, I defer to your judgement re SeaBIOS code :)

> I think barrier() and yield() are what we want - in particular, we
> really do want to yield() while busy so that interrupts and other code
> can continue to run.

If you prefer barrier() over volatile, then, I believe:

- barrier() is sufficient between the filling of the struct and the outl().

- the read loop should be probably re-coded as:

  do {
    barrier();
  } while (access.length != 0 &&
           !(access.control & QEMU_CFG_DMA_CTL_ERROR));

Re yield(), I guess that could be necessary in SeaBIOS, yes. (In edk2,
there is only one interrupt, the timer interrupt, and it is serviced (=
timer callbacks are dispatched) by default unless the "task priority
level" is raised enough.)

Thanks
Laszlo

> -Kevin
> 
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS at seabios.org
> http://www.seabios.org/mailman/listinfo/seabios
>