[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH 2/2] plat/kvm: Update VGA console cursor location
Hey Florian,
On 22.08.2018 09:53, Florian Schmidt wrote:
Hi Simon,
On 08/21/2018 03:38 PM, Simon Kuenzer wrote:
On 02.08.2018 07:39, Florian Schmidt wrote:
On 08/02/2018 07:03 AM, Florian Schmidt wrote:
/* Hardware text mode color constants. */
@@ -109,6 +111,36 @@ static void vga_scroll(void)
= vga_entry(' ', terminal_color);
}
+static void vga_update_cursor(void)
+{
+ unsigned long irq_flags;
+ uint8_t old;
+ uint8_t ioas; // VGA Input/Output select
+ uint16_t areg; // VGA address register
+ uint16_t dreg; // VGA data register
+
+ local_irq_save(irq_flags);
I wonder if it makes sense to protect the whole
"_libkvmplat_vga_putc()" function where this function is just
called from. This would truly enable printing from multiple threads
since it protects the global state in this VGA driver.
Hmm. I'm not a big fan of disabling interrupts for large amounts of
code, including several functions. Long strings can take time to
print out, and I'm not sure I feel comfortable disabling interrupts
for that long If at all, I would suggest doing that with other kinds
of locking mechanisms. I'm also wondering, though, whether it
shouldn't be the job of whatever's calling coutk. The typical
behavior on most other systems is that, if you print from several
threads without synchronization, your output will be interleaved,
right?
Nevermind, I see you were talking about the putc function (it really
is a bit too early in the morning). So this wouldn't disable
interrupts for massive amounts of time if the string is longer,
because it works char-by-char.
Nevertheless, I'm still not sure. Should we disable interrupts for
the whole function? Should we disable interrupts at all? My reasoning
was: I disable interrupts there because I interact with (vritualized)
hardware. If I get interupted after setting areg, but before writing
to dreg, this could end in me writing to some other place, which
sounds bad and could leave the hardware in a broken state. On the
other hand, your worry is about leaving terminal_{column,row} in a
broken state in _libkvmplat_vga_putc(), right? That's also a good
point, but I wouldn't disable interrupts for that.
What about adding a "vga mutex" to vga_console.c and using it in
_libkvmplat_vga_putc(), dependent on libuklock becing compiled in?
That would give us locking in those case. I can't think of a very
realistic scenario in which one would have several threads running in
one unikernel (and them all printing out stuff) without any locking
primitives, and thus libuklock not compiled in.
Hum... I rather prefer disabling interrupts over a mutex here because
(1) mutex would not work when print from the interrupt context (I
know, this is a bad thing but it works for now and printing is anyway
you should not do to much because it is slow)
(2) it enables printing from interrupt context (again: I know this is
something bad but it might be still useful to have)
(3) I do not like to many dependencies to general libraries implied by
the platform. libuklock require libuksched and then you cant compile a
plain kvm Unikernel without scheduling anymore.
Can't you just disable interrupts for small period of time in the
code, e.g., when interacting with the registers and adding a character
to the VGA buffer?
Starting with point (3), that wouldn't apply in my planned solution,
because I would make the mutex guarded by an #ifdef
CONFIG_LIBUKLOCK_MUTEX. Regarding the other two points... well, I do see
that printing from interrupt context can be useful, strictly for
debugging. However, it isn't really good practice, and for such a
somewhat weak reason, I wouldn't want to disable interrupts every time
we call _libkvm_vga_putc. Because it's not just "a small period of
time". It does quite a bit of stuff, most importantly scrolling the
screen bufer when necessary, which means memcpy'ing memory ranges. Of
course, a mutex has the problem that if we ever get into the function in
interrupt context while the mutex is held, we have an immediate deadlock.
So none of the solutions seems perfect: we either (1) disable interrupts
for quite a long time, or (2) (with a mutex) lose the possibility to
print from interrupt context (or risk going into a deadlock), or (3)
(without either), we risk race conditions on terminal_{column,row},
which worst case can lead to writing outside of the bounds of the
0xb8000 video buffer. That being said, the 0xb8000 video buffer
continues up to 0xc0000, way beyond the area we use, and is preceded by
the monochrome adaptor buffer at 0xb0000-0xb7fff, so writing out of
bounds is not an immediate disaster, you simply lose some screen output.
Though, I agree, definitely very not nice, either.
Hum... okay, I see.
One fourth solution could be to have a mutex guard in the function, but
"break the lock" (fun the function without trying to acquire the lock)
when we enter from interrupt context. That way, the possible race
condition and writing outside of the visible screen buffer could only
happen when you print from an interrupt, and well, in that case you know
you're doing naughty stuff anyway. Not that this is a very clean
approach either, but it would be another idea.
What's your opinion?
Actually, after studying our existing code, I think it is better if we
add any mutex logic to uk_printk() and uk_printd() and keep
ukplat_coutd() and ukplat_coutk() non-thread-safe.
The interrupt problem is actually a question if those functions are
reentrant-safe. And I was wrong: Except the emergency console on Xen,
all of our current console implementations are NOT reentrant-safe.
Something will definitely screw up but we should keep the mis-behavior
in bounds.
So, I think we can make this happen by atomically copying the global
state (which is row, column) by interrupts off to local stack variables.
Then we do all of our operations as normal (including scrolling in the
buffer) and save atomically the state again before exiting the function.
I think this is the best we can do for now, and - yes, this should be
fixed also on the other console implementations in a similar way.
Cheers,
Florian
Cheers,
Simon
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
