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@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-a... https://channel9.msdn.com/Shows/Going+Deep/Cpp-and-Beyond-2012-Herb-Sutter-a...
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@seabios.org http://www.seabios.org/mailman/listinfo/seabios