[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 

_______________________________________________
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®.