[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Minios-devel] [UNIKRAFT PATCH] plat/kvm, lib/ukdebug: Add VGA option for printing in kvm
Hi Dafna,
thanks for this patch! This is a nice feature which would reduce
Please see my comments inline.
On 28.06.2018 18:52, Dafna Hirschfeld wrote:
Add support in the kvm platform for vga textmode console.
Add an option in the ukdebug configuration to use the vga
textmode for prints.
Signed-off-by: Dafna Hirschfeld <dafna3@xxxxxxxxx>
---
lib/ukdebug/Config.uk | 29 ++++++
plat/kvm/Makefile.uk | 2 +
plat/kvm/include/kvm-x86/serial_console.h | 30 ++++++
plat/kvm/include/kvm-x86/vga_console.h | 30 ++++++
plat/kvm/x86/console.c | 88 ++++++----------
plat/kvm/x86/serial_console.c | 85 +++++++++++++++
plat/kvm/x86/vga_console.c | 120 ++++++++++++++++++++++
7 files changed, 329 insertions(+), 55 deletions(-)
create mode 100644 plat/kvm/include/kvm-x86/serial_console.h
create mode 100644 plat/kvm/include/kvm-x86/vga_console.h
create mode 100644 plat/kvm/x86/serial_console.c
create mode 100644 plat/kvm/x86/vga_console.c
diff --git a/lib/ukdebug/Config.uk b/lib/ukdebug/Config.uk
index dcaeb3a..470f8b9 100644
--- a/lib/ukdebug/Config.uk
+++ b/lib/ukdebug/Config.uk
@@ -10,12 +10,41 @@ config LIBUKDEBUG_PRINTK
help
Build with debugging symbols enabled.
+config KERNEL_SERIAL_CONSOLE
+ bool "Serial console for the kernel prints"
+ default y
+ depends on (LIBUKDEBUG_PRINTK && PLAT_KVM && ARCH_X86_64)
+ help
+ Choose serial console for the kernel printing
+
+
+config KERNEL_VGA_CONSOLE
+ bool "Vga console for the kernel prints (kvm platform)"
+ default y
+ depends on (LIBUKDEBUG_PRINTK && PLAT_KVM && ARCH_X86_64)
+ help
+ Choose vga console for the kernel printing
+
I think it makes more sense to have this option as part of the KVM
platform (plat/kvm/Config.uk). If you want you can add an submenu for
the console output options. libukdebug should not care how the platform
implements the kernel and debug output. It calls just the API functions
(ukplat_coutd(), ukplat_coutk()). This means, that the redirection or
cloning should happen within these API calls of the platform only (you
even implemented it in this way already).
libukdebug should stay untouched by this patch. ;-)
config LIBUKDEBUG_PRINTD
bool "Enable debug messages (uk_printd)"
default y
help
Build with debugging symbols enabled.
+config DEBUG_SERIAL_CONSOLE
+ bool "Serial console for the debug prints"
+ default y
+ depends on (LIBUKDEBUG_PRINTD && PLAT_KVM && ARCH_X86_64)
+ help
+ Choose serial console for the debug printing
+
+config DEBUG_VGA_CONSOLE
+ bool "Vga console for the debug prints (kvm platform)"
+ default y
+ depends on (LIBUKDEBUG_PRINTD && PLAT_KVM && ARCH_X86_64)
+ help
+ Choose vga console for the debug printing
+
choice
prompt "Debug message level"
default LIBUKDEBUG_PRINTD_ERR
diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
index 2705fd1..759d338 100644
--- a/plat/kvm/Makefile.uk
+++ b/plat/kvm/Makefile.uk
@@ -32,6 +32,8 @@ LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
$(LIBKVMPLAT_BASE)/x86/traps.c
LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
$(LIBKVMPLAT_BASE)/x86/cpu_vectors_x86_64.S
LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) += $(LIBKVMPLAT_BASE)/x86/setup.c
LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) += $(LIBKVMPLAT_BASE)/x86/console.c
+LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) += $(LIBKVMPLAT_BASE)/x86/vga_console.c
+LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
$(LIBKVMPLAT_BASE)/x86/serial_console.c
LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) += $(LIBKVMPLAT_BASE)/x86/lcpu.c
LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) += $(LIBKVMPLAT_BASE)/x86/intctrl.c
LIBKVMPLAT_SRCS-y += $(LIBKVMPLAT_BASE)/shutdown.c
diff --git a/plat/kvm/include/kvm-x86/serial_console.h
b/plat/kvm/include/kvm-x86/serial_console.h
new file mode 100644
index 0000000..7fc43a6
--- /dev/null
+++ b/plat/kvm/include/kvm-x86/serial_console.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Authors: Dan Williams
+ * Martin Lucina
+ * Felipe Huici <felipe.huici@xxxxxxxxx>
+ * Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ * Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *
+ * Copyright (c) 2015-2017 IBM
+ * Copyright (c) 2016-2017 Docker, Inc.
+ * Copyright (c) 2017 NEC Europe Ltd., NEC Corporation
+ *
+ * Permission to use, copy, modify, and/or distribute this software
+ * for any purpose with or without fee is hereby granted, provided
+ * that the above copyright notice and this permission notice appear
+ * in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
+ * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
+ * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
Please add import guards:
#ifndef __KVM_SERIAL_CONS__
#define __KVM_SERIAL_CONS__
and at the end of this file:
#endif /* __KVM_SERIAL_CONS__ */
+
+void _libkvmplat_init_serial_console(void);
+
+void serial_putc(char a);
Hum... You name spaced _libkvmplat_init_serial_console() but you did not
with serial_putc and vga_putc. How about _libkvmplat_serial_putc()?
Maybe you also want to add _libkvmplat_serial_getc() here, since we have
this function already.
diff --git a/plat/kvm/include/kvm-x86/vga_console.h
b/plat/kvm/include/kvm-x86/vga_console.h
new file mode 100644
index 0000000..df001d1
--- /dev/null
+++ b/plat/kvm/include/kvm-x86/vga_console.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Authors: Dan Williams
+ * Martin Lucina
+ * Felipe Huici <felipe.huici@xxxxxxxxx>
+ * Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ * Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *
+ * Copyright (c) 2015-2017 IBM
+ * Copyright (c) 2016-2017 Docker, Inc.
+ * Copyright (c) 2017 NEC Europe Ltd., NEC Corporation
This file is from you, right? You can replace all copyrights with yours
and set a license you prefer. Everything that is BSD or BSD-compatible
(like this ISC license) is fine.
+ *
+ * Permission to use, copy, modify, and/or distribute this software
+ * for any purpose with or without fee is hereby granted, provided
+ * that the above copyright notice and this permission notice appear
+ * in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
+ * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
+ * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
Do not forget the guards here, too. ;-)
+
+void _libkvmplat_init_vga_console(void);
+
+void vga_putc(char a);
Please name space this function too.
diff --git a/plat/kvm/x86/console.c b/plat/kvm/x86/console.c
index 5ec03b0..a5dc615 100644
--- a/plat/kvm/x86/console.c
+++ b/plat/kvm/x86/console.c
@@ -25,76 +25,52 @@
* CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
-#include <x86/cpu.h>
-#include <kvm/console.h>
#include <uk/plat/console.h>
-#include <uk/essentials.h>
-#include <uk/print.h>
+#include <uk/config.h>
-#define COM1 0x3f8
+#if (CONFIG_DEBUG_VGA_CONSOLE || CONFIG_KERNEL_VGA_CONSOLE)
+#include <kvm-x86/vga_console.h>
+#endif
-#define COM1_DATA (COM1 + 0)
-#define COM1_INTR (COM1 + 1)
-#define COM1_CTRL (COM1 + 3)
-#define COM1_STATUS (COM1 + 5)
-
-/* only when DLAB is set */
-#define COM1_DIV_LO (COM1 + 0)
-#define COM1_DIV_HI (COM1 + 1)
-
-#define DLAB 0x80
-#define PROT 0x03 /* 8N1 (8 bits, no parity, one stop bit) */
+#if (CONFIG_DEBUG_SERIAL_CONSOLE || CONFIG_KERNEL_SERIAL_CONSOLE)
+#include <kvm-x86/serial_console.h>
+#endif
void _libkvmplat_init_console(void)
{
- outb(COM1_INTR, 0x00); /* Disable all interrupts */
- outb(COM1_CTRL, DLAB); /* Enable DLAB (set baudrate divisor) */
- outb(COM1_DIV_LO, 0x01);/* Set div to 1 (lo byte) 115200 baud */
- outb(COM1_DIV_HI, 0x00);/* (hi byte) */
- outb(COM1_CTRL, PROT); /* Set 8N1, clear DLAB */
-}
-
-int ukplat_coutd(const char *str, unsigned int len)
-{
- return ukplat_coutk(str, len);
-}
-
-static int serial_tx_empty(void)
-{
- return inb(COM1_STATUS) & 0x20;
-}
-
-static void serial_write(char a)
-{
- while (!serial_tx_empty())
- ;
+#if (CONFIG_DEBUG_VGA_CONSOLE || CONFIG_KERNEL_VGA_CONSOLE)
+ _libkvmplat_init_vga_console();
+#endif
+#if (CONFIG_DEBUG_SERIAL_CONSOLE || CONFIG_KERNEL_SERIAL_CONSOLE)
+ _libkvmplat_init_serial_console();
+#endif
- outb(COM1_DATA, a);
}
-static void serial_putc(char a)
+int ukplat_coutd(const char *buf, unsigned int len)
{
- if (a == '\n')
- serial_write('\r');
- serial_write(a);
+ for (unsigned int i = 0; i < len; i++) {
+#if CONFIG_DEBUG_SERIAL_CONSOLE
+ serial_putc(buf[i]);
+#endif
+#if CONFIG_DEBUG_VGA_CONSOLE
+ vga_putc(buf[i]);
+#endif
+ }
+ return len;
}
-static int serial_rx_ready(void)
-{
- return inb(COM1_STATUS) & 0x01;
-}
-
-static int serial_getc(void)
-{
- if (!serial_rx_ready())
- return -1;
- return (int) inb(COM1_DATA);
-}
int ukplat_coutk(const char *buf, unsigned int len)
{
- for (unsigned int i = 0; i < len; i++)
+ for (unsigned int i = 0; i < len; i++) {
+#if CONFIG_KERNEL_SERIAL_CONSOLE
serial_putc(buf[i]);
+#endif
+#if CONFIG_KERNEL_VGA_CONSOLE
+ vga_putc(buf[i]);
+#endif
+ }
return len;
}
@@ -103,11 +79,13 @@ int ukplat_cink(char *buf, unsigned int maxlen)
int ret;
unsigned int num = 0;
+#if CONFIG_KERNEL_SERIAL_CONSOLE
+
while (num < maxlen
&& (ret = serial_getc()) >= 0) {
*(buf++) = (char) ret;
num++;
}
-
+#endif
return (int) num;
}
diff --git a/plat/kvm/x86/serial_console.c b/plat/kvm/x86/serial_console.c
new file mode 100644
index 0000000..eccb126
--- /dev/null
+++ b/plat/kvm/x86/serial_console.c
@@ -0,0 +1,85 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Authors: Dan Williams
+ * Martin Lucina
+ * Felipe Huici <felipe.huici@xxxxxxxxx>
+ * Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ * Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *
+ * Copyright (c) 2015-2017 IBM
+ * Copyright (c) 2016-2017 Docker, Inc.
+ * Copyright (c) 2017 NEC Europe Ltd., NEC Corporation
+ *
+ * Permission to use, copy, modify, and/or distribute this software
+ * for any purpose with or without fee is hereby granted, provided
+ * that the above copyright notice and this permission notice appear
+ * in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
+ * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
+ * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <kvm-x86/serial_console.h>
+
+#include <x86/cpu.h>
+
+#define COM1 0x3f8
+
+#define COM1_DATA (COM1 + 0)
+#define COM1_INTR (COM1 + 1)
+#define COM1_CTRL (COM1 + 3)
+#define COM1_STATUS (COM1 + 5)
+
+/* only when DLAB is set */
+#define COM1_DIV_LO (COM1 + 0)
+#define COM1_DIV_HI (COM1 + 1)
+
+#define DLAB 0x80
+#define PROT 0x03 /* 8N1 (8 bits, no parity, one stop bit) */
+
+void _libkvmplat_init_serial_console(void)
+{
+ outb(COM1_INTR, 0x00); /* Disable all interrupts */
+ outb(COM1_CTRL, DLAB); /* Enable DLAB (set baudrate divisor) */
+ outb(COM1_DIV_LO, 0x01);/* Set div to 1 (lo byte) 115200 baud */
+ outb(COM1_DIV_HI, 0x00);/* (hi byte) */
+ outb(COM1_CTRL, PROT); /* Set 8N1, clear DLAB */
+}
+
+static int serial_tx_empty(void)
+{
+ return inb(COM1_STATUS) & 0x20;
+}
+
+static void serial_write(char a)
+{
+ while (!serial_tx_empty())
+ ;
+
+ outb(COM1_DATA, a);
+}
+
+void serial_putc(char a)
+{
+ if (a == '\n')
+ serial_write('\r');
+ serial_write(a);
+}
+
+static int serial_rx_ready(void)
+{
+ return inb(COM1_STATUS) & 0x01;
+}
+
+int serial_getc(void)
+{
+ if (!serial_rx_ready())
+ return -1;
+ return (int) inb(COM1_DATA);
+}
diff --git a/plat/kvm/x86/vga_console.c b/plat/kvm/x86/vga_console.c
new file mode 100644
index 0000000..6d7342e
--- /dev/null
+++ b/plat/kvm/x86/vga_console.c
@@ -0,0 +1,120 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Authors: Dan Williams
+ * Martin Lucina
+ * Felipe Huici <felipe.huici@xxxxxxxxx>
+ * Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ * Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ *
+ * Copyright (c) 2015-2017 IBM
+ * Copyright (c) 2016-2017 Docker, Inc.
+ * Copyright (c) 2017 NEC Europe Ltd., NEC Corporation
+ *
+ * Permission to use, copy, modify, and/or distribute this software
+ * for any purpose with or without fee is hereby granted, provided
+ * that the above copyright notice and this permission notice appear
+ * in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL
+ * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE
+ * AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, INDIRECT, OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <stdint.h>
+#include <kvm-x86/vga_console.h>
+
+/* Hardware text mode color constants. */
+enum vga_color {
+ VGA_COLOR_BLACK = 0,
+ VGA_COLOR_BLUE = 1,
+ VGA_COLOR_GREEN = 2,
+ VGA_COLOR_CYAN = 3,
+ VGA_COLOR_RED = 4,
+ VGA_COLOR_MAGENTA = 5,
+ VGA_COLOR_BROWN = 6,
+ VGA_COLOR_LIGHT_GREY = 7,
+ VGA_COLOR_DARK_GREY = 8,
+ VGA_COLOR_LIGHT_BLUE = 9,
+ VGA_COLOR_LIGHT_GREEN = 10,
+ VGA_COLOR_LIGHT_CYAN = 11,
+ VGA_COLOR_LIGHT_RED = 12,
+ VGA_COLOR_LIGHT_MAGENTA = 13,
+ VGA_COLOR_LIGHT_BROWN = 14,
+ VGA_COLOR_WHITE = 15,
+};
+
+static inline uint8_t vga_entry_color(enum vga_color fg, enum vga_color bg)
+{
+ return fg | bg << 4;
+}
+
+static inline uint16_t vga_entry(unsigned char uc, uint8_t color)
+{
+ return (uint16_t) uc | (uint16_t) color << 8;
+}
+
+
+static const size_t VGA_WIDTH = 80;
+static const size_t VGA_HEIGHT = 25;
+
I think these variables should be declared as static, right?:
+size_t terminal_row;
+size_t terminal_column;
+uint8_t terminal_color;
+uint16_t *terminal_buffer;
+
+static void clear_terminal(void)
+{
+ for (size_t y = 0; y < VGA_HEIGHT; y++) {
+ for (size_t x = 0; x < VGA_WIDTH; x++) {
+ const size_t index = y * VGA_WIDTH + x;
+
+ terminal_buffer[index] = vga_entry(' ', terminal_color);
+ }
+ }
+}
+
+void _libkvmplat_init_vga_console(void)
+{
+ terminal_row = 0;
+ terminal_column = 0;
+ terminal_color = vga_entry_color(VGA_COLOR_LIGHT_GREY, VGA_COLOR_BLACK);
+ terminal_buffer = (uint16_t *) 0xB8000;
+ clear_terminal();
+}
+
+static void terminal_setcolor(uint8_t color)
+{
+ terminal_color = color;
+}
+
+static void terminal_putentryat(char c, uint8_t color, size_t x, size_t y)
+{
+ const size_t index = y * VGA_WIDTH + x;
+
+ terminal_buffer[index] = vga_entry(c, color);
+}
+
+void vga_putc(char c)
+{
Do you know if we would need to mask 'c' in order to remove some
unsupported characters and control codes? I am not sure if all 256
character codes are supported by VGA.
+ if (terminal_column == 0 && terminal_row == 0)
+ clear_terminal();
+
+ if (c == 10 || c == 13) {
I am prefering using '\n' and '\r' instead of 10 and 13. It makes it
quicker to read if you do not have the ASCII control codes in memory.
Btw, '\r' has a different meaning than '\n'. '\n' sets the screen
pointer to the next line (terminal_row++), '\r' puts it just to the
beginning of the current line and the line is not erased
(terminal_column=0). In Unikraft we actually interpret '\n' as '\r\n':
Set the write pointer to the beginning of the line and then move it to
the next line.
So, you should implement '\r' differently. The current behavior for '\n'
is fine for Unikraft.
'\b' should also be implemented since we will need it for supporting
shells: just terminal_column--, no character deletion.
+ terminal_column = 0;
+ if (++terminal_row == VGA_HEIGHT)
+ terminal_row = 0;
+ } else {
+ terminal_putentryat(c, terminal_color,
+ terminal_column, terminal_row);
+ if (++terminal_column == VGA_WIDTH) {
+ terminal_column = 0;
+ if (++terminal_row == VGA_HEIGHT)
+ terminal_row = 0;
+ }
+ }
+}
Right now, when your screen is full, you clear the screen and start with
the next line at the top of the screen. I think this is totally fine for
the first version of VGA support and I would take it. But I also think
that we should add scrolling as an enhancement with a follow-up patch
because it makes reading of messages easier. Do you know if the VGA
standard has even a call that we could use and that handles the screen
buffer for us? I am asking because we would not need to implement this
behavior by our own (although it is fine if we would need to do it).
What do you think?
I am looking forward for the next version of this patch. I like having
this feature in Unikraft ;-)
Thanks,
Simon
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
