On Sun, Jan 22, 2012 at 11:12:00PM +0100, Rudolf Marek wrote:
Can't the existing timer_counter in the BDA be used instead?
I got some earlier iteration which used the *_timer value but it did not work for some reason (most likely I forgot all the magic for real mode)
I think using extra variables will remove the complexity with day rollover and custom changes to the timer variable either from option ROM or from BIOS API.
I'm not sure there would be an issue with third party code (optionroms / old code changing the pit). The seabios timer users are fairly straight forward - get the current value, and then wait for the expected value in a loop with yield(). So, there'd only be a problem if some third-party irq handler messed with the pit - if they did that from a hardware irq handler, I'd guess it would break lots of things. (Things are less clear with CONFIG_THREAD_OPTIONROMS, but I'd say it would be safe to leave that off for your use case.)
BTW, what's the impact of writing to the PORT_PIT_MODE? Will it corrupt the old value? (Not an issue for the patch, as obviously something is better than nothing for your board.)
I think none, it is a readback command of the 8254 chip which allows to read back the latch and configuration. The mode of timer or timer itself should not be affected. I think this may need to be protected if some interrupt jump in. I had in previous patch the IRQ_save/restore stuff but you told me that irqs are enabled only during yields. Maybe it would be safer to have the irq_save / restore here too?
All SeaBIOS C code runs with irqs disabled. There's no reason to have irq_save/restore in the C code.
However, are there any seabios timer users reachable from irq handlers? Should userspace start programming the pit without irq prevention, could it get interrupted mid-way through and then corrupted by a seabios timer user in an irq handler?
- // Emulate the CPUID if 386/486
- detect_cpuid();
This is too early (can't assign global variables here on QEmu). I suggest you put in maininit() after the malloc_fixup() call.
It is too late there. There is some CPUID instruction executed from xen_probe(). Yes I xen is enabled for coreboot for some reason... The whole point of the cpuid emulation is to have it globally available and not making it dependent on particular configuration. Not sure how to solve it here.
Hrmm - the xen thing is a special case - I think we could just make CONFIG_XEN dependent on !CONFIG_COREBOOT in Kconfig and ignore it's early cpuid usage.
-Kevin