Xen project Mailing List

Hi,

See my comments inline

On Thu, Jul 19, 2018 at 10:58 AM, Florian Schmidt <Florian.Schmidt@xxxxxxxxx> wrote:

Hi Dafna,

thank you for your patch!

This is not a formal code review (I didn't follow your patch from v1), but I have two remarks inline.

Cheers,
Florian

On 07/17/2018 09:11 PM, Dafna Hirschfeld wrote:

diff --git a/include/uk/essentials.h b/include/uk/essentials.h
index f9a7fd1..2a6ff93 100644
--- a/include/uk/essentials.h
+++ b/include/uk/essentials.h
@@ -58,6 +58,9 @@ extern "C" {
#ifndef __unused
#define __unused __attribute__((unused))
#endif
+#ifndef __maybe_unused
+#define __maybe_unused __attribute__((unused))
+#endif
#ifndef __section
#define __section(s) __attribute__((section(s)))
#endif
diff --git a/plat/kvm/Config.uk b/plat/kvm/Config.uk
index 118954d..1042a04 100644

I feel this is unrelated enough to the rest of your changes that it warrants an extra patch. You could make this a patch series with this part as 1/2 as a preparatory patch, and with the rest as 2/2.

Sure , Ill prepare a set of patches.

diff --git a/plat/kvm/x86/vga_console.c b/plat/kvm/x86/vga_console.c
new file mode 100644
index 0000000..37d584f
--- /dev/null
+++ b/plat/kvm/x86/vga_console.c
@@ -0,0 +1,144 @@
+/* SPDX-License-Identifier: ISC */
+/*
+ * Authors: Dan Williams
+ * Martin Lucina
+ * Felipe Huici <felipe.huici@xxxxxxxxx>
+ * Florian Schmidt <florian.schmidt@xxxxxxxxx>
+ * Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
+ * Dafna Hirschfeld <dafna3@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.
+ */
+

This didn't build for me in my testing environment with "unknown type name ‘size_t’" errors. You need to #include <stddef.h> here. This might have been masked by other build options in your build that implicitly included it somewhere down the line.

I compile the code on top of the staging branch through the helloworld repository, https://xenbits.xen.org/git-http/unikraft/apps/helloworld.git

I also see that stdint.h includes stddef.h . I wonder why I am able to compile and you aren't.

+#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;
+}
+
+#define TAB_ALIGNMENT 8
+#define VGA_WIDTH 80
+#define VGA_HEIGHT 25
+
+static size_t terminal_row;
+static size_t terminal_column;
+static uint8_t terminal_color;
+static 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_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 _libkvmplat_vga_putc(char c)
+{
+ if (terminal_column == 0 && terminal_row == 0)
+ clear_terminal();
+
+ switch (c) {
+ case '\a':
+ break; //ascii bel (0x07) - ignore
+ case '\b':
+ if (terminal_column > 0) {
+ terminal_column--;
+ } else if (terminal_row > 0) {
+ terminal_column = VGA_WIDTH - 1;
+ terminal_row--;
+ }
+ break;
+ case '\n':
+ _libkvmplat_vga_putc('\r');
+ if (++terminal_row == VGA_HEIGHT)
+ terminal_row = 0;
+ break;
+ case '\r':
+ terminal_column = 0;
+ break;
+ case '\t':
+ do {
+ terminal_column++;
+ } while (terminal_column % TAB_ALIGNMENT != 0
+ && terminal_column != VGA_WIDTH);
+
+ if (terminal_column == VGA_WIDTH) {
+ terminal_column = 0;
+ if (++terminal_row == VGA_HEIGHT)
+ terminal_row = 0;
+ }
+ break;
+ default:
+ 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;
+ }
+ break;
+ }
+}

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel. +49 (0)6221 4342-265
Fax: +49 (0)6221 4342-155
e-mail: florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

Re: [Minios-devel] [UNIKRAFT PATCH v2] plat/kvm: Add VGA textmode console support.