[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




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?




+    /* Location of the address and data registers is variable and denoted
+     * by the least significant bit in the Input/Output register.
+     */
+    ioas = inb(0x3cc) & 0x1;
+    if (ioas) {
+        areg = 0x3d4;
+        dreg = 0x3d5;
+    } else {
+        areg = 0x3b4;
+        dreg = 0x3b5;
+    }

Is this changing during runtime or could be figure out the location during initialization and use it?

I honestly don't know. I wouldn't expect this to change after it is set once, but I don't know for sure. Supposedly, 0x3b is mostly there for compatibility with older monochrome adapters, so it probably should be 0x3d pretty much always. If we go by the "this is a driver for QEMU VGA only", we can probably scrap it altogether. Otherwise, it's *probably* fine to just check it once in the beginning.

I am not an expert either but depending on your explanation and what I've seen so far it looks like we just need to do it once. ;-) What do you think?



_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.