On Tue, Jan 24, 2012 at 12:47:39PM +0100, Rudolf Marek wrote:
This reminds me that the GET_ set stuff should treat the variables as volatile, because otherwise if it can be changed from outside compiler might not know and will happily perform a test on register version of the variable. Or even better, it should contain memory clobber asm volatile ("" ::: "memory") which will act as a barrier for compiler to reload all variables used and not rely on cached values in registers.
Just to make it more clear, this remark is just for the situation if you may have irq handler interrupting a function. Compiler can read from memory just once and use cached value in register.
SeaBIOS C code always runs with irqs disabled.
I've played with the macros in farptr.h quite a bit, and I think they're okay. If anything, the compiler seems to unnecessarily pessimize the resulting assembler (I've never seen it cache the result of a read even when it's valid to do that).
All in all everything depends what can/could be called while interrupts are enabled in seabios. If you want to have the GET/SET stuff at least atomic, and reloadable then use same thing as in Linux kernel "atomic" variables. On x86 mov is guaranteed to be atomic (at most 32bit only) and because it is internally implemented as structure with volatile property it works fine in all cases.
Since the code runs with irqs disabled, it can't be interrupted and is therefore atomic. The yield() call does invoke a barrier() so it can't be run out of order.
In my case I use here 64bit data then it means I need to use irq_save restore in any case. It is even doing a barrier for compiler. Therefore I think it is the best solution here.
Please don't do that. It's just going to confuse the code. There should be no reason for a barrier() or messing with the irqs.