On Thu, Aug 06, 2015 at 09:34:00PM +0200, Laszlo Ersek wrote:
On 08/06/15 20:59, Kevin O'Connor wrote:
On Thu, Aug 06, 2015 at 06:15:57PM +0200, Laszlo Ersek wrote:
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...
Thanks - I'll add the above to my queue.
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.
In this code, the QemuCfgDmaAccess is not memory mapped IO. It's a regular struct that lives in regular memory. The "device" in this case is reading the struct from memory, performing some action, and then writing back to the struct in memory. So, to me this is more similar to multi-threaded code (ie, the "device" could be seen as similar to another CPU).
Of course, the action isn't asynchronous in the current proposed QEMU implementation, but I understand that making the interface future proof in that regard was the goal.
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));
I'd probably do something like:
barrier(); outl((u32)&access, PORT_QEMU_CFG_DMA_ADDR); while (access.control & QEMU_CFG_DMA_CTL_READ) yield();
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.)
Yeah, SeaBIOS is a little more complicated wrt interrupts - there is a brief blurb describing threads and interrupts at: http://www.seabios.org/Execution_and_code_flow
Cheers, -Kevin