[PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler

[PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler

[Sasha Levin]

INT10 handler is a basic implementation of BIOS video services.

The handler implements a VESA interface which is initialized at
the very beginning of loading the kernel.

Signed-off-by: John Floren <john@jfloren.net>
[ turning code into patches and cleanup ]
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/bios/bios-rom.S |   56 ++++++++--------
 tools/kvm/bios/int10.c    |  161 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+), 28 deletions(-)
 create mode 100644 tools/kvm/bios/int10.c

diff --git a/tools/kvm/bios/bios-rom.S b/tools/kvm/bios/bios-rom.S
index 8a53dcd..b636cb8 100644
--- a/tools/kvm/bios/bios-rom.S
+++ b/tools/kvm/bios/bios-rom.S
@@ -27,36 +27,36 @@ ENTRY_END(bios_intfake)
  * We ignore bx settings
  */
 ENTRY(bios_int10)
-	test $0x0e, %ah
-	jne 1f
+	pushw	%fs
+	pushl	%es
+	pushl	%edi
+	pushl	%esi
+	pushl	%ebp
+	pushl	%esp
+	pushl	%edx
+	pushl	%ecx
+	pushl	%ebx
+	pushl	%eax
+
+	movl		%esp, %eax
+	/* this is way easier than doing it in assembly */
+	/* just push all the regs and jump to a C handler */
+	call	int10handler
+
+	popl	%eax
+	popl	%ebx
+	popl	%ecx
+	popl	%edx
+	popl	%esp
+	popl	%ebp
+	popl	%esi
+	popl	%edi
+	popl	%es
+	popw	%fs
 
-/*
- * put char in AL at current cursor and
- * increment cursor position
- */
-putchar:
-	stack_swap
-
-	push %fs
-	push %bx
-
-	mov $VGA_RAM_SEG, %bx
-	mov %bx, %fs
-	mov %cs:(cursor), %bx
-	mov %al, %fs:(%bx)
-	inc %bx
-	test $VGA_PAGE_SIZE, %bx
-	jb putchar_new
-	xor %bx, %bx
-putchar_new:
-	mov %bx, %fs:(cursor)
-
-	pop %bx
-	pop %fs
-
-	stack_restore
-1:
 	IRET
+
+
 /*
  * private IRQ data
  */
diff --git a/tools/kvm/bios/int10.c b/tools/kvm/bios/int10.c
new file mode 100644
index 0000000..98205c3
--- /dev/null
+++ b/tools/kvm/bios/int10.c
@@ -0,0 +1,161 @@
+#include "kvm/segment.h"
+#include "kvm/bios.h"
+#include "kvm/util.h"
+#include "kvm/vesa.h"
+#include <stdint.h>
+
+#define VESA_MAGIC ('V' + ('E' << 8) + ('S' << 16) + ('A' << 24))
+
+struct int10args {
+	u32	eax;
+	u32	ebx;
+	u32	ecx;
+	u32	edx;
+	u32	esp;
+	u32	ebp;
+	u32	esi;
+	u32	edi;
+	u32	es;
+};
+
+/* VESA General Information table */
+struct vesa_general_info {
+	u32 signature;			/* 0 Magic number = "VESA" */
+	u16 version;			/* 4 */
+	void *vendor_string;		/* 6 */
+	u32 capabilities;		/* 10 */
+	void *video_mode_ptr;		/* 14 */
+	u16 total_memory;		/* 18 */
+
+	u8 reserved[236];		/* 20 */
+} __attribute__ ((packed));
+
+
+struct vminfo {
+	u16	mode_attr;		/* 0 */
+	u8	win_attr[2];		/* 2 */
+	u16	win_grain;		/* 4 */
+	u16	win_size;		/* 6 */
+	u16	win_seg[2];		/* 8 */
+	u32	win_scheme;		/* 12 */
+	u16	logical_scan;		/* 16 */
+
+	u16	h_res;			/* 18 */
+	u16	v_res;			/* 20 */
+	u8	char_width;		/* 22 */
+	u8	char_height;		/* 23 */
+	u8	memory_planes;		/* 24 */
+	u8	bpp;			/* 25 */
+	u8	banks;			/* 26 */
+	u8	memory_layout;		/* 27 */
+	u8	bank_size;		/* 28 */
+	u8	image_planes;		/* 29 */
+	u8	page_function;		/* 30 */
+
+	u8	rmask;			/* 31 */
+	u8	rpos;			/* 32 */
+	u8	gmask;			/* 33 */
+	u8	gpos;			/* 34 */
+	u8	bmask;			/* 35 */
+	u8	bpos;			/* 36 */
+	u8	resv_mask;		/* 37 */
+	u8	resv_pos;		/* 38 */
+	u8	dcm_info;		/* 39 */
+
+	u32	lfb_ptr;		/* 40 Linear frame buffer address */
+	u32	offscreen_ptr;		/* 44 Offscreen memory address */
+	u16	offscreen_size;		/* 48 */
+
+	u8	reserved[206];		/* 50 */
+};
+
+char oemstring[11] = "KVM VESA";
+u16 modes[2] = { 0x0112, 0xffff };
+
+static inline void outb(unsigned short port, unsigned char val)
+{
+	asm volatile("outb %0, %1" : : "a"(val), "Nd"(port));
+}
+
+/*
+ * It's probably much more useful to make this print to the serial
+ * line rather than print to a non-displayed VGA memory
+ */
+static inline void int10putchar(struct int10args *args)
+{
+	u8 al, ah;
+
+	al = args->eax & 0xFF;
+	ah = (args->eax & 0xFF00) >> 8;
+
+	outb(0x3f8, al);
+}
+
+static void int10vesa(struct int10args *args)
+{
+	u8 al, ah;
+	struct vesa_general_info *destination;
+	struct vminfo *vi;
+
+	al = args->eax;
+	ah = args->eax >> 8;
+
+	switch (al) {
+	case 0:
+		/* Set controller info */
+
+		destination = (struct vesa_general_info *)args->edi;
+		*destination = (struct vesa_general_info) {
+			.signature	= VESA_MAGIC,
+			.version	= 0x102,
+			.vendor_string	= oemstring,
+			.capabilities	= 0x10,
+			.video_mode_ptr	= modes,
+			.total_memory	= (4*VESA_WIDTH * VESA_HEIGHT) / 0x10000,
+		};
+
+		break;
+	case 1:
+		vi = (struct vminfo *)args->edi;
+		*vi = (struct vminfo) {
+			.mode_attr	= 0xd9, /* 11011011 */
+			.logical_scan	= VESA_WIDTH*4,
+			.h_res		= VESA_WIDTH,
+			.v_res		= VESA_HEIGHT,
+			.bpp		= VESA_BPP,
+			.memory_layout	= 6,
+			.memory_planes	= 1,
+			.lfb_ptr	= VESA_MEM_ADDR,
+			.rmask		= 8,
+			.gmask		= 8,
+			.bmask		= 8,
+			.resv_mask	= 8,
+			.resv_pos	= 24,
+			.bpos		= 16,
+			.gpos		= 8,
+		};
+
+		break;
+	}
+
+	args->eax			= 0x004f; /* return success every time */
+
+}
+
+bioscall void int10handler(struct int10args *args)
+{
+	u8 ah;
+
+	ah = (args->eax & 0xff00) >> 8;
+
+	switch (ah) {
+	case 0x0e:
+		int10putchar(args);
+		break;
+	case 0x4f:
+		int10vesa(args);
+		break;
+	}
+
+}
+
-- 
1.7.5.rc3

[PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization

[Sasha Levin]

Allow setting video mode in guest kernel.

For possible values see Documentation/fb/vesafb.txt

Signed-off-by: John Floren <john@jfloren.net>
[ turning code into patches and cleanup ]
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/kvm.h |    2 +-
 tools/kvm/kvm.c             |    7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
index 08c6fda..f951f2d 100644
--- a/tools/kvm/include/kvm/kvm.h
+++ b/tools/kvm/include/kvm/kvm.h
@@ -41,7 +41,7 @@ int kvm__max_cpus(struct kvm *kvm);
 void kvm__init_ram(struct kvm *kvm);
 void kvm__delete(struct kvm *kvm);
 bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
-			const char *initrd_filename, const char *kernel_cmdline);
+			const char *initrd_filename, const char *kernel_cmdline, u16 vidmode);
 void kvm__setup_bios(struct kvm *kvm);
 void kvm__start_timer(struct kvm *kvm);
 void kvm__stop_timer(struct kvm *kvm);
diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
index 4393a41..7284211 100644
--- a/tools/kvm/kvm.c
+++ b/tools/kvm/kvm.c
@@ -320,7 +320,7 @@ static int load_flat_binary(struct kvm *kvm, int fd)
 static const char *BZIMAGE_MAGIC	= "HdrS";
 
 static bool load_bzimage(struct kvm *kvm, int fd_kernel,
-			int fd_initrd, const char *kernel_cmdline)
+			int fd_initrd, const char *kernel_cmdline, u16 vidmode)
 {
 	struct boot_params *kern_boot;
 	unsigned long setup_sects;
@@ -383,6 +383,7 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel,
 	kern_boot->hdr.type_of_loader	= 0xff;
 	kern_boot->hdr.heap_end_ptr	= 0xfe00;
 	kern_boot->hdr.loadflags	|= CAN_USE_HEAP;
+	kern_boot->hdr.vid_mode		= vidmode;
 
 	/*
 	 * Read initrd image into guest memory
@@ -441,7 +442,7 @@ static bool initrd_check(int fd)
 }
 
 bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
-		const char *initrd_filename, const char *kernel_cmdline)
+		const char *initrd_filename, const char *kernel_cmdline, u16 vidmode)
 {
 	bool ret;
 	int fd_kernel = -1, fd_initrd = -1;
@@ -459,7 +460,7 @@ bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
 			die("%s is not an initrd", initrd_filename);
 	}
 
-	ret = load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline);
+	ret = load_bzimage(kvm, fd_kernel, fd_initrd, kernel_cmdline, vidmode);
 
 	if (initrd_filename)
 		close(fd_initrd);
-- 
1.7.5.rc3

[PATCH 3/5 V2] kvm tools: Add VESA device

[Sasha Levin]

Add a simple VESA device which simply moves a framebuffer
from guest kernel to a VNC server.

VESA device PCI code is very similar to virtio-* PCI code.

Signed-off-by: John Floren <john@jfloren.net>
[ turning code into patches and cleanup ]
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/hw/vesa.c                    |  108 ++++++++++++++++++++++++++++++++
 tools/kvm/include/kvm/ioport.h         |    2 +
 tools/kvm/include/kvm/vesa.h           |   27 ++++++++
 tools/kvm/include/kvm/virtio-pci-dev.h |    3 +
 4 files changed, 140 insertions(+), 0 deletions(-)
 create mode 100644 tools/kvm/hw/vesa.c
 create mode 100644 tools/kvm/include/kvm/vesa.h

diff --git a/tools/kvm/hw/vesa.c b/tools/kvm/hw/vesa.c
new file mode 100644
index 0000000..3003aa5
--- /dev/null
+++ b/tools/kvm/hw/vesa.c
@@ -0,0 +1,108 @@
+#include "kvm/vesa.h"
+#include "kvm/ioport.h"
+#include "kvm/util.h"
+#include "kvm/kvm.h"
+#include "kvm/pci.h"
+#include "kvm/kvm-cpu.h"
+#include "kvm/irq.h"
+#include "kvm/virtio-pci-dev.h"
+
+#include <rfb/rfb.h>
+
+#include <sys/types.h>
+#include <sys/ioctl.h>
+#include <inttypes.h>
+#include <unistd.h>
+
+#define VESA_QUEUE_SIZE		128
+#define VESA_IRQ		14
+
+/*
+ * This "6000" value is pretty much the result of experimentation
+ * It seems that around this value, things update pretty smoothly
+ */
+#define VESA_UPDATE_TIME	6000
+
+u8 videomem[VESA_MEM_SIZE];
+
+static bool vesa_pci_io_in(struct kvm *kvm, u16 port, void *data, int size, u32 count)
+{
+	printf("vesa in port=%u\n", port);
+	return true;
+}
+
+static bool vesa_pci_io_out(struct kvm *kvm, u16 port, void *data, int size, u32 count)
+{
+	printf("vesa out port=%u\n", port);
+	return true;
+}
+
+static struct ioport_operations vesa_io_ops = {
+	.io_in	= vesa_pci_io_in,
+	.io_out	= vesa_pci_io_out,
+};
+
+static struct pci_device_header vesa_pci_device = {
+	.vendor_id		= PCI_VENDOR_ID_REDHAT_QUMRANET,
+	.device_id		= PCI_DEVICE_ID_VESA,
+	.header_type		= PCI_HEADER_TYPE_NORMAL,
+	.revision_id		= 0,
+	.class			= 0x030000,
+	.subsys_vendor_id	= PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
+	.subsys_id		= PCI_SUBSYSTEM_ID_VESA,
+	.bar[0]			= IOPORT_VESA | PCI_BASE_ADDRESS_SPACE_IO,
+	.bar[1]			= VESA_MEM_ADDR | PCI_BASE_ADDRESS_SPACE_MEMORY,
+};
+
+
+void vesa_mmio_callback(u64 addr, u8 *data, u32 len, u8 is_write)
+{
+	if (is_write)
+		memcpy(&videomem[addr - VESA_MEM_ADDR], data, len);
+
+	return;
+}
+
+void vesa__init(struct kvm *kvm)
+{
+	u8 dev, line, pin;
+	pthread_t thread;
+
+	if (irq__register_device(PCI_DEVICE_ID_VESA, &dev, &pin, &line) < 0)
+		return;
+
+	vesa_pci_device.irq_pin = pin;
+	vesa_pci_device.irq_line = line;
+	pci__register(&vesa_pci_device, dev);
+	ioport__register(IOPORT_VESA, &vesa_io_ops, IOPORT_VESA_SIZE);
+
+	kvm__register_mmio(VESA_MEM_ADDR, VESA_MEM_SIZE, &vesa_mmio_callback);
+	pthread_create(&thread, NULL, vesa__dovnc, kvm);
+}
+
+/*
+ * This starts a VNC server to display the framebuffer.
+ * It's not altogether clear this belongs here rather than in kvm-run.c
+ */
+void *vesa__dovnc(void *v)
+{
+	/*
+	 * Make a fake argc and argv because the getscreen function
+	 * seems to want it.
+	 */
+	int ac = 1;
+	char av[1][1] = {{0} };
+	rfbScreenInfoPtr server;
+
+	server = rfbGetScreen(&ac, (char **)av, VESA_WIDTH, VESA_HEIGHT, 8, 3, 4);
+	server->frameBuffer = (char *)videomem;
+	server->alwaysShared = TRUE;
+	rfbInitServer(server);
+
+	while (rfbIsActive(server)) {
+		rfbMarkRectAsModified(server, 0, 0, VESA_WIDTH, VESA_HEIGHT);
+		rfbProcessEvents(server, server->deferUpdateTime * VESA_UPDATE_TIME);
+	}
+	return NULL;
+}
+
diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h
index 218530c..8253938 100644
--- a/tools/kvm/include/kvm/ioport.h
+++ b/tools/kvm/include/kvm/ioport.h
@@ -7,6 +7,8 @@
 
 /* some ports we reserve for own use */
 #define IOPORT_DBG			0xe0
+#define IOPORT_VESA			0xa200
+#define IOPORT_VESA_SIZE		256
 #define IOPORT_VIRTIO_P9		0xb200	/* Virtio 9P device */
 #define IOPORT_VIRTIO_P9_SIZE		256
 #define IOPORT_VIRTIO_BLK		0xc200	/* Virtio block device */
diff --git a/tools/kvm/include/kvm/vesa.h b/tools/kvm/include/kvm/vesa.h
new file mode 100644
index 0000000..3e58587
--- /dev/null
+++ b/tools/kvm/include/kvm/vesa.h
@@ -0,0 +1,27 @@
+#ifndef KVM__VESA_H
+#define KVM__VESA_H
+
+#include <linux/types.h>
+
+#define VESA_WIDTH	640
+#define VESA_HEIGHT	480
+
+#define VESA_MEM_ADDR	0xd0000000
+#define VESA_MEM_SIZE	(4*VESA_WIDTH*VESA_HEIGHT)
+#define VESA_BPP	32
+
+struct kvm;
+struct int10args;
+
+void vesa_mmio_callback(u64, u8*, u32, u8);
+void vesa__init(struct kvm *self);
+void *vesa__dovnc(void *);
+void int10handler(struct int10args *args);
+
+#ifndef CONFIG_HAS_VNCSERVER
+void vesa__init(struct kvm *self) { }
+#endif
+
+extern u8 videomem[VESA_MEM_SIZE];
+
+#endif
diff --git a/tools/kvm/include/kvm/virtio-pci-dev.h b/tools/kvm/include/kvm/virtio-pci-dev.h
index 1b7862e..ca373df 100644
--- a/tools/kvm/include/kvm/virtio-pci-dev.h
+++ b/tools/kvm/include/kvm/virtio-pci-dev.h
@@ -13,8 +13,11 @@
 #define PCI_DEVICE_ID_VIRTIO_CONSOLE		0x1003
 #define PCI_DEVICE_ID_VIRTIO_RNG		0x1004
 #define PCI_DEVICE_ID_VIRTIO_P9			0x1009
+#define PCI_DEVICE_ID_VESA			0x2000
 
 #define PCI_VENDOR_ID_REDHAT_QUMRANET		0x1af4
 #define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET	0x1af4
 
+#define PCI_SUBSYSTEM_ID_VESA			0x0004
+
 #endif /* VIRTIO_PCI_DEV_H_ */
-- 
1.7.5.rc3

[PATCH 4/5 V2] kvm tools: Update makefile and feature tests

[Sasha Levin]

Update feature tests to test for libvncserver.

VESA support doesn't get compiled in unless libvncserver
is installed.

Signed-off-by: John Floren <john@jfloren.net>
[ turning code into patches and cleanup ]
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/Makefile                 |   11 ++++++++++-
 tools/kvm/config/feature-tests.mak |   10 ++++++++++
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
index e6e8d4e..2ebc86c 100644
--- a/tools/kvm/Makefile
+++ b/tools/kvm/Makefile
@@ -58,6 +58,14 @@ ifeq ($(has_bfd),y)
 	LIBS	+= -lbfd
 endif
 
+FLAGS_VNCSERVER=$(CFLAGS) -lvncserver
+has_vncserver := $(call try-cc,$(SOURCE_VNCSERVER),$(FLAGS_VNCSERVER))
+ifeq ($(has_vncserver),y)
+	CFLAGS	+= -DCONFIG_HAS_VNCSERVER
+	OBJS	+= hw/vesa.o
+	LIBS	+= -lvncserver
+endif
+
 DEPS	:= $(patsubst %.o,%.d,$(OBJS))
 
 # Exclude BIOS object files from header dependencies.
@@ -153,9 +161,10 @@ bios/bios.o: bios/bios.S bios/bios-rom.bin
 bios/bios-rom.bin: bios/bios-rom.S bios/e820.c
 	$(E) "  CC      " $@
 	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c -s bios/e820.c -o bios/e820.o
+	$(Q) $(CC) -include code16gcc.h $(CFLAGS) $(BIOS_CFLAGS) -c -s bios/int10.c -o bios/int10.o
 	$(Q) $(CC) $(CFLAGS) $(BIOS_CFLAGS) -c -s bios/bios-rom.S -o bios/bios-rom.o
 	$(E) "  LD      " $@
-	$(Q) ld -T bios/rom.ld.S -o bios/bios-rom.bin.elf bios/bios-rom.o bios/e820.o
+	$(Q) ld -T bios/rom.ld.S -o bios/bios-rom.bin.elf bios/bios-rom.o bios/e820.o bios/int10.o
 	$(E) "  OBJCOPY " $@
 	$(Q) objcopy -O binary -j .text bios/bios-rom.bin.elf bios/bios-rom.bin
 	$(E) "  NM      " $@
diff --git a/tools/kvm/config/feature-tests.mak b/tools/kvm/config/feature-tests.mak
index 6170fd2..0801b54 100644
--- a/tools/kvm/config/feature-tests.mak
+++ b/tools/kvm/config/feature-tests.mak
@@ -126,3 +126,13 @@ int main(void)
 	return 0;
 }
 endef
+
+define SOURCE_VNCSERVER
+#include <rfb/rfb.h>
+
+int main(void)
+{
+	rfbIsActive((void *)0);
+	return 0;
+}
+endef
-- 
1.7.5.rc3

[PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Sasha Levin]

Requirements - Kernel compiled with:
CONFIG_FB_BOOT_VESA_SUPPORT=y
CONFIG_FB_VESA=y
CONFIG_FRAMEBUFFER_CONSOLE=y

Start VNC server by starting kvm tools with "--vnc".
Connect to the VNC server by running: "vncviewer :0".

Since there is no support for input devices at this time,
it may be useful starting kvm tools with an additional
' -p "console=ttyS0" ' parameter so that it would be possible
to use a serial console alongside with a graphic one.

Signed-off-by: John Floren <john@jfloren.net>
[ turning code into patches and cleanup ]
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/kvm-run.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
index 288e1fb..adbb25b 100644
--- a/tools/kvm/kvm-run.c
+++ b/tools/kvm/kvm-run.c
@@ -28,6 +28,7 @@
 #include <kvm/barrier.h>
 #include <kvm/symbol.h>
 #include <kvm/virtio-9p.h>
+#include <kvm/vesa.h>
 
 /* header files for gitish interface  */
 #include <kvm/kvm-run.h>
@@ -66,6 +67,7 @@ static const char *virtio_9p_dir;
 static bool single_step;
 static bool readonly_image[MAX_DISK_IMAGES];
 static bool virtio_rng;
+static bool vnc;
 extern bool ioport_debug;
 extern int  active_console;
 
@@ -110,6 +112,7 @@ static const struct option options[] = {
 	OPT_STRING('\0', "kvm-dev", &kvm_dev, "kvm-dev", "KVM device file"),
 	OPT_STRING('\0', "virtio-9p", &virtio_9p_dir, "root dir",
 			"Enable 9p over virtio"),
+	OPT_BOOLEAN('\0', "vnc", &vnc, "Enable VNC framebuffer"),
 
 	OPT_GROUP("Kernel options:"),
 	OPT_STRING('k', "kernel", &kernel_filename, "kernel",
@@ -413,6 +416,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 	char *hi;
 	int i;
 	void *ret;
+	u16 vidmode = 0;
 
 	signal(SIGALRM, handle_sigalrm);
 	signal(SIGQUIT, handle_sigquit);
@@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 	kvm->nrcpus = nrcpus;
 
 	memset(real_cmdline, 0, sizeof(real_cmdline));
-	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial");
+	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1");
+	if (vnc) {
+		strcat(real_cmdline, " video=vesafb console=tty0");
+		vidmode = 0x312;
+	} else {
+		strcat(real_cmdline, " console=ttyS0 earlyprintk=serial");
+	}
 	strcat(real_cmdline, " ");
 	if (kernel_cmdline)
 		strlcat(real_cmdline, kernel_cmdline, sizeof(real_cmdline));
@@ -543,7 +553,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 	printf("  # kvm run -k %s -m %Lu -c %d\n", kernel_filename, ram_size / 1024 / 1024, nrcpus);
 
 	if (!kvm__load_kernel(kvm, kernel_filename, initrd_filename,
-				real_cmdline))
+				real_cmdline, vidmode))
 		die("unable to load kernel %s", kernel_filename);
 
 	kvm->vmlinux		= vmlinux_filename;
@@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 
 	kvm__init_ram(kvm);
 
+	if (vnc)
+		vesa__init(kvm);
+
 	thread_pool__init(nr_online_cpus);
 
 	for (i = 0; i < nrcpus; i++) {
-- 
1.7.5.rc3

Re: [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> INT10 handler is a basic implementation of BIOS video services.
> 
> The handler implements a VESA interface which is initialized at
> the very beginning of loading the kernel.
> 
> Signed-off-by: John Floren <john@jfloren.net>
> [ turning code into patches and cleanup ]
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/bios/bios-rom.S |   56 ++++++++--------
>  tools/kvm/bios/int10.c    |  161 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+), 28 deletions(-)
>  create mode 100644 tools/kvm/bios/int10.c
> 
> diff --git a/tools/kvm/bios/bios-rom.S b/tools/kvm/bios/bios-rom.S
> index 8a53dcd..b636cb8 100644
> --- a/tools/kvm/bios/bios-rom.S
> +++ b/tools/kvm/bios/bios-rom.S
> @@ -27,36 +27,36 @@ ENTRY_END(bios_intfake)
>   * We ignore bx settings
>   */
>  ENTRY(bios_int10)
> -	test $0x0e, %ah
> -	jne 1f
> +	pushw	%fs
> +	pushl	%es
> +	pushl	%edi
> +	pushl	%esi
> +	pushl	%ebp
> +	pushl	%esp
> +	pushl	%edx
> +	pushl	%ecx
> +	pushl	%ebx
> +	pushl	%eax
> +
> +	movl		%esp, %eax
> +	/* this is way easier than doing it in assembly */
> +	/* just push all the regs and jump to a C handler */
> +	call	int10handler
> +
> +	popl	%eax
> +	popl	%ebx
> +	popl	%ecx
> +	popl	%edx
> +	popl	%esp
> +	popl	%ebp
> +	popl	%esi
> +	popl	%edi
> +	popl	%es
> +	popw	%fs
>  
> -/*
> - * put char in AL at current cursor and
> - * increment cursor position
> - */
> -putchar:
> -	stack_swap
> -
> -	push %fs
> -	push %bx
> -
> -	mov $VGA_RAM_SEG, %bx
> -	mov %bx, %fs
> -	mov %cs:(cursor), %bx
> -	mov %al, %fs:(%bx)
> -	inc %bx
> -	test $VGA_PAGE_SIZE, %bx
> -	jb putchar_new
> -	xor %bx, %bx
> -putchar_new:
> -	mov %bx, %fs:(cursor)
> -
> -	pop %bx
> -	pop %fs
> -
> -	stack_restore
> -1:
>  	IRET
> +
> +
>  /*
>   * private IRQ data
>   */
> diff --git a/tools/kvm/bios/int10.c b/tools/kvm/bios/int10.c
> new file mode 100644
> index 0000000..98205c3
> --- /dev/null
> +++ b/tools/kvm/bios/int10.c
> @@ -0,0 +1,161 @@
> +#include "kvm/segment.h"
> +#include "kvm/bios.h"
> +#include "kvm/util.h"
> +#include "kvm/vesa.h"
> +#include <stdint.h>
> +
> +#define VESA_MAGIC ('V' + ('E' << 8) + ('S' << 16) + ('A' << 24))
> +
> +struct int10args {
> +	u32	eax;
> +	u32	ebx;
> +	u32	ecx;
> +	u32	edx;
> +	u32	esp;
> +	u32	ebp;
> +	u32	esi;
> +	u32	edi;
> +	u32	es;
> +};
> +
> +/* VESA General Information table */
> +struct vesa_general_info {
> +	u32 signature;			/* 0 Magic number = "VESA" */
> +	u16 version;			/* 4 */
> +	void *vendor_string;		/* 6 */
> +	u32 capabilities;		/* 10 */
> +	void *video_mode_ptr;		/* 14 */
> +	u16 total_memory;		/* 18 */
> +
> +	u8 reserved[236];		/* 20 */
> +} __attribute__ ((packed));
> +
> +
> +struct vminfo {
> +	u16	mode_attr;		/* 0 */
> +	u8	win_attr[2];		/* 2 */
> +	u16	win_grain;		/* 4 */
> +	u16	win_size;		/* 6 */
> +	u16	win_seg[2];		/* 8 */
> +	u32	win_scheme;		/* 12 */
> +	u16	logical_scan;		/* 16 */
> +
> +	u16	h_res;			/* 18 */
> +	u16	v_res;			/* 20 */
> +	u8	char_width;		/* 22 */
> +	u8	char_height;		/* 23 */
> +	u8	memory_planes;		/* 24 */
> +	u8	bpp;			/* 25 */
> +	u8	banks;			/* 26 */
> +	u8	memory_layout;		/* 27 */
> +	u8	bank_size;		/* 28 */
> +	u8	image_planes;		/* 29 */
> +	u8	page_function;		/* 30 */
> +
> +	u8	rmask;			/* 31 */
> +	u8	rpos;			/* 32 */
> +	u8	gmask;			/* 33 */
> +	u8	gpos;			/* 34 */
> +	u8	bmask;			/* 35 */
> +	u8	bpos;			/* 36 */
> +	u8	resv_mask;		/* 37 */
> +	u8	resv_pos;		/* 38 */
> +	u8	dcm_info;		/* 39 */
> +
> +	u32	lfb_ptr;		/* 40 Linear frame buffer address */
> +	u32	offscreen_ptr;		/* 44 Offscreen memory address */
> +	u16	offscreen_size;		/* 48 */
> +
> +	u8	reserved[206];		/* 50 */
> +};
> +
> +char oemstring[11] = "KVM VESA";
> +u16 modes[2] = { 0x0112, 0xffff };
> +
> +static inline void outb(unsigned short port, unsigned char val)
> +{
> +	asm volatile("outb %0, %1" : : "a"(val), "Nd"(port));
> +}
> +
> +/*
> + * It's probably much more useful to make this print to the serial
> + * line rather than print to a non-displayed VGA memory
> + */
> +static inline void int10putchar(struct int10args *args)
> +{
> +	u8 al, ah;
> +
> +	al = args->eax & 0xFF;
> +	ah = (args->eax & 0xFF00) >> 8;
> +
> +	outb(0x3f8, al);
> +}
> +
> +static void int10vesa(struct int10args *args)
> +{
> +	u8 al, ah;
> +	struct vesa_general_info *destination;
> +	struct vminfo *vi;
> +
> +	al = args->eax;
> +	ah = args->eax >> 8;
> +
> +	switch (al) {
> +	case 0:
> +		/* Set controller info */
> +
> +		destination = (struct vesa_general_info *)args->edi;
> +		*destination = (struct vesa_general_info) {
> +			.signature	= VESA_MAGIC,
> +			.version	= 0x102,
> +			.vendor_string	= oemstring,
> +			.capabilities	= 0x10,
> +			.video_mode_ptr	= modes,
> +			.total_memory	= (4*VESA_WIDTH * VESA_HEIGHT) / 0x10000,
> +		};
> +
> +		break;
> +	case 1:
> +		vi = (struct vminfo *)args->edi;
> +		*vi = (struct vminfo) {
> +			.mode_attr	= 0xd9, /* 11011011 */
> +			.logical_scan	= VESA_WIDTH*4,
> +			.h_res		= VESA_WIDTH,
> +			.v_res		= VESA_HEIGHT,
> +			.bpp		= VESA_BPP,
> +			.memory_layout	= 6,
> +			.memory_planes	= 1,
> +			.lfb_ptr	= VESA_MEM_ADDR,
> +			.rmask		= 8,
> +			.gmask		= 8,
> +			.bmask		= 8,
> +			.resv_mask	= 8,
> +			.resv_pos	= 24,
> +			.bpos		= 16,
> +			.gpos		= 8,
> +		};
> +
> +		break;
> +	}
> +
> +	args->eax			= 0x004f; /* return success every time */
> +
> +}
> +
> +bioscall void int10handler(struct int10args *args)
> +{
> +	u8 ah;
> +
> +	ah = (args->eax & 0xff00) >> 8;
> +
> +	switch (ah) {
> +	case 0x0e:
> +		int10putchar(args);
> +		break;
> +	case 0x4f:
> +		int10vesa(args);
> +		break;
> +	}

Why are these functions prefixed in such a weird way? Why not int10_putchar(), 
int10_vesa(), etc. like all other bits in tools/kvm/?

Thanks,

	Ingo

Re: [PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

>  bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
> -			const char *initrd_filename, const char *kernel_cmdline);
> +			const char *initrd_filename, const char *kernel_cmdline, u16 vidmode);

Suggestion for future cleanup: we really want to gros a 'struct kernel_params' 
kind of thing which could be passed along here by address.

That would make it easier to extent it with whatever may come along in the 
future, and would make the code look cleaner as well.

Thanks,

	Ingo

Re: [PATCH 3/5 V2] kvm tools: Add VESA device

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> +struct int10args;

this should be int10_args.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> @@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  	kvm->nrcpus = nrcpus;
>  
>  	memset(real_cmdline, 0, sizeof(real_cmdline));
> -	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial");
> +	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1");
> +	if (vnc) {
> +		strcat(real_cmdline, " video=vesafb console=tty0");
> +		vidmode = 0x312;
> +	} else {
> +		strcat(real_cmdline, " console=ttyS0 earlyprintk=serial");
> +	}

Hm, i think all the kernel parameter handling code wants to move into driver 
specific routines as well. Something like:

	serial_init(kvm, real_cmdline);

where serial_init() would append to real_cmdline if needed.

This removes a bit of serial-driver specific knowledge from kvm-run.c.

Same goes for the VESA driver and the above video mode flag logic.

> @@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>  
>  	kvm__init_ram(kvm);
>  
> +	if (vnc)
> +		vesa__init(kvm);

Shouldnt vesa__init() itself know about whether it's active (i.e. the 'vnc' 
flag is set) and return early if it's not set?

That way this could become more encapsulated and self-sufficient:

	vesa__init(kvm);

With no VESA driver specific state exposed to the generic kvm_cmd_run() 
function.

Ideally kvm_cmd_run() hould just be a series of:

	serial_init(kvm, real_cmdline);
	vesa_init(kvm, real_cmdline);
	...

initialization routines. Later on even this could be removed: using section 
tricks we can put init functions into a section and drivers could register 
their init function like initcall(func) functions are registered within the 
kernel. kvm_cmd_run() could thus iterate over that (build time constructed) 
section like this:

extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[];

static void __init do_initcalls(void)
{
        initcall_t *fn;

        for (fn = __early_initcall_end; fn < __initcall_end; fn++)
                do_one_initcall(*fn);
}

and would not actually have *any* knowledge about what drivers were built in.

Currently it's fine to initialize everything explicitly - but this would be the 
long term model to work towards ...

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

On 5/23/11 2:38 PM, Ingo Molnar wrote:
> * Sasha Levin<levinsasha928@gmail.com>  wrote:
>
>> @@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>>   	kvm->nrcpus = nrcpus;
>>
>>   	memset(real_cmdline, 0, sizeof(real_cmdline));
>> -	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial");
>> +	strcpy(real_cmdline, "notsc noapic noacpi pci=conf1");
>> +	if (vnc) {
>> +		strcat(real_cmdline, " video=vesafb console=tty0");
>> +		vidmode = 0x312;
>> +	} else {
>> +		strcat(real_cmdline, " console=ttyS0 earlyprintk=serial");
>> +	}
> Hm, i think all the kernel parameter handling code wants to move into driver
> specific routines as well. Something like:
>
> 	serial_init(kvm, real_cmdline);
>
> where serial_init() would append to real_cmdline if needed.
>
> This removes a bit of serial-driver specific knowledge from kvm-run.c.
>
> Same goes for the VESA driver and the above video mode flag logic.
>
>> @@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>>
>>   	kvm__init_ram(kvm);
>>
>> +	if (vnc)
>> +		vesa__init(kvm);
> Shouldnt vesa__init() itself know about whether it's active (i.e. the 'vnc'
> flag is set) and return early if it's not set?
>
> That way this could become more encapsulated and self-sufficient:
>
> 	vesa__init(kvm);
>
> With no VESA driver specific state exposed to the generic kvm_cmd_run()
> function.
>
> Ideally kvm_cmd_run() hould just be a series of:
>
> 	serial_init(kvm, real_cmdline);
> 	vesa_init(kvm, real_cmdline);
> 	...
>
> initialization routines. Later on even this could be removed: using section
> tricks we can put init functions into a section and drivers could register
> their init function like initcall(func) functions are registered within the
> kernel. kvm_cmd_run() could thus iterate over that (build time constructed)
> section like this:
>
> extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[];
>
> static void __init do_initcalls(void)
> {
>          initcall_t *fn;
>
>          for (fn = __early_initcall_end; fn<  __initcall_end; fn++)
>                  do_one_initcall(*fn);
> }
>
> and would not actually have *any* knowledge about what drivers were built in.
>
> Currently it's fine to initialize everything explicitly - but this would be the
> long term model to work towards ...

Prasad, didn't you have patches to do exactly that?

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

On Mon, May 23, 2011 at 2:19 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Requirements - Kernel compiled with:
> CONFIG_FB_BOOT_VESA_SUPPORT=y
> CONFIG_FB_VESA=y
> CONFIG_FRAMEBUFFER_CONSOLE=y

Dunno if it's possible but it would be nice to have a more readable
error message if you don't have that compiled in:

penberg@tiger:~/linux/tools/kvm$ ./kvm run --vnc -d
~/images/debian_squeeze_amd64_standard.img
  # kvm run -k ../../arch/x86/boot/bzImage -m 320 -c 2
  Warning: Config tap device error. Are you root?
23/05/2011 17:08:19 Listening for VNC connections on TCP port 5900
Undefined video mode number: 312
Press <ENTER> to see video modes available, <SPACE> to continue, or wait 30 sec
Killed

This obviously isn't an issue for merging this patch.

                       Pekka

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/23/2011 01:38 PM, Ingo Molnar wrote:
> Later on even this could be removed: using section
> tricks we can put init functions into a section

This is not kernel space, the C library provides a way to do that with 
__attribute__((constructor))...

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/23/2011 01:38 PM, Ingo Molnar wrote:

> > Later on even this could be removed: using section tricks we can put init 
> > functions into a section
> 
> This is not kernel space, the C library provides a way to do that
> with __attribute__((constructor))...

Yeah, that would certainly work for simple things but there's several reasons 
why explicit control over initcalls is preferred in a tool like tools/kvm/ over 
__attribute__((constructor)):

 - C constructors run before main() so any generic environment that tools/kvm/
   might want to set up is not available yet - including but not limited to the
   command line arguments.

 - C constructors have random limitations like apparently not being executed if
   the constructor is linked into a .a static library.

 - It has advantages to have explicit control over initcalls - that way
   debugging and other instrumentation can be added freely. For example the
   kernel has a debug_initcall boot parameter to print the initcalls as they
   are executed. They can also be traced.

 - The kernel has several classes of initcalls with different call priority,
   i'm not aware of an equivalent __attribute__((constructor)) capability.
   We could also do more sophisticated layers of initcalls or an explicit
   initcall dependency tree, should the need arise.

Using .section tricks directly isnt particularly complex and the result is 
rather flexible, well-defined and visible.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Cyrill Gorcunov]

On 05/24/2011 12:37 PM, Paolo Bonzini wrote:
> On 05/23/2011 01:38 PM, Ingo Molnar wrote:
>> Later on even this could be removed: using section
>> tricks we can put init functions into a section
> 
> This is not kernel space, the C library provides a way to do that with __attribute__((constructor))...
> 
> Paolo

Ingo meant to use the same trick as we do in kernel space for late initcalls.

-- 
    Cyrill

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/24/2011 10:50 AM, Ingo Molnar wrote:
> Yeah, that would certainly work for simple things but there's several reasons
> why explicit control over initcalls is preferred in a tool like tools/kvm/ over
> __attribute__((constructor)):

Some advantages you mention are real indeed, but they are general; 
there's no reason why they apply only to tools/kvm.  You can achieve the 
same by doing only minimal work (registering your 
subsystems/devices/whatever in a linked list) in the constructors.  Then 
you iterate on the list and call function pointers.

I know portability is not relevant to tools/kvm/, but using unportable 
tricks for the sake of using them is a direct way to NIH.  But oh well 
all of tools/kvm/ is NIH after all. :)

>   - The kernel has several classes of initcalls with different call priority,
>     i'm not aware of an equivalent __attribute__((constructor)) capability.
>     We could also do more sophisticated layers of initcalls or an explicit
>     initcall dependency tree, should the need arise.

Constructors and destructors can have a priority (low runs first for 
constructors, low runs last for destructors).

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/24/2011 10:50 AM, Ingo Molnar wrote:
>   - C constructors have random limitations like apparently not being executed if
>     the constructor is linked into a .a static library.

Ah, forgot about this.  Given _why_ this happens (for static libraries, 
the linker omits object modules that are not required to fulfill 
undefined references in previous objects), I'd be surprised if explicit 
section tricks do not have the same limitation.  After all the 
constructor attribute is only syntactic sugar for section tricks.

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

On Tue, May 24, 2011 at 12:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> I know portability is not relevant to tools/kvm/, but using unportable
> tricks for the sake of using them is a direct way to NIH.  But oh well all
> of tools/kvm/ is NIH after all. :)

Hmm?

The point is to follow Linux kernel conventions and idioms (and share
code) as much as possible so it's familiar to devs who are already
working on the kernel. That's why section tricks seem more appropriate
than using constructor to me. Or is there some technical advantage to
using constructors?

                       Pekka

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Avi Kivity]

On 05/24/2011 12:55 PM, Pekka Enberg wrote:
> On Tue, May 24, 2011 at 12:10 PM, Paolo Bonzini<pbonzini@redhat.com>  wrote:
> >  I know portability is not relevant to tools/kvm/, but using unportable
> >  tricks for the sake of using them is a direct way to NIH.  But oh well all
> >  of tools/kvm/ is NIH after all. :)
>
> Hmm?
>
> The point is to follow Linux kernel conventions and idioms (and share
> code) as much as possible so it's familiar to devs who are already
> working on the kernel. That's why section tricks seem more appropriate
> than using constructor to me. Or is there some technical advantage to
> using constructors?

You get to reuse infrastructure that's already there.

Things like using sections and s/uint64_t/u64/ look anti-reuse to me.  
Userspace isn't the kernel, for better or for worse.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

Hi Avi,

On Tue, May 24, 2011 at 2:22 PM, Avi Kivity <avi@redhat.com> wrote:
>> The point is to follow Linux kernel conventions and idioms (and share
>> code) as much as possible so it's familiar to devs who are already
>> working on the kernel. That's why section tricks seem more appropriate
>> than using constructor to me. Or is there some technical advantage to
>> using constructors?
>
> You get to reuse infrastructure that's already there.
>
> Things like using sections and s/uint64_t/u64/ look anti-reuse to me.
>  Userspace isn't the kernel, for better or for worse.

Not really. The type thing is pretty much required once you start
using kernel code (as we learned the hard way).

Btw, constructor attribute doesn't really seem like a good fit for
"late_initcall" type of thing:

  The constructor attribute causes the function to be called
automatically before execution enters main ()

What am I missing here?

                        Pekka

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Sasha Levin]

On Tue, 2011-05-24 at 14:26 +0300, Pekka Enberg wrote:
> Hi Avi,
> 
> On Tue, May 24, 2011 at 2:22 PM, Avi Kivity <avi@redhat.com> wrote:
> >> The point is to follow Linux kernel conventions and idioms (and share
> >> code) as much as possible so it's familiar to devs who are already
> >> working on the kernel. That's why section tricks seem more appropriate
> >> than using constructor to me. Or is there some technical advantage to
> >> using constructors?
> >
> > You get to reuse infrastructure that's already there.
> >
> > Things like using sections and s/uint64_t/u64/ look anti-reuse to me.
> >  Userspace isn't the kernel, for better or for worse.
> 
> Not really. The type thing is pretty much required once you start
> using kernel code (as we learned the hard way).
> 
> Btw, constructor attribute doesn't really seem like a good fit for
> "late_initcall" type of thing:
> 
>   The constructor attribute causes the function to be called
> automatically before execution enters main ()

You could add a small constructor function that'll add a pointer to the
real initialization function to a list which will get called after we
get everything initialized and ready.

-- 

Sasha.

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Avi Kivity]

On 05/24/2011 02:26 PM, Pekka Enberg wrote:
> Hi Avi,
>
> On Tue, May 24, 2011 at 2:22 PM, Avi Kivity<avi@redhat.com>  wrote:
> >>  The point is to follow Linux kernel conventions and idioms (and share
> >>  code) as much as possible so it's familiar to devs who are already
> >>  working on the kernel. That's why section tricks seem more appropriate
> >>  than using constructor to me. Or is there some technical advantage to
> >>  using constructors?
> >
> >  You get to reuse infrastructure that's already there.
> >
> >  Things like using sections and s/uint64_t/u64/ look anti-reuse to me.
> >    Userspace isn't the kernel, for better or for worse.
>
> Not really. The type thing is pretty much required once you start
> using kernel code (as we learned the hard way).

What happens when you start using userspace libraries?

Eventually you'll have a lot more of that than kernel code.

> Btw, constructor attribute doesn't really seem like a good fit for
> "late_initcall" type of thing:
>
>    The constructor attribute causes the function to be called
> automatically before execution enters main ()
>
> What am I missing here?

Like Paolo said, you can have the constructor register a function or 
structure to be called any time you like.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

Hi Avi,

On Tue, May 24, 2011 at 2:30 PM, Avi Kivity <avi@redhat.com> wrote:
> What happens when you start using userspace libraries?
>
> Eventually you'll have a lot more of that than kernel code.

I don't quite understand what problems you think we might have. We're
already using userspace libraries (libvnc) and most of us code is
non-kernel code.

We switched to u64 and friends for two reasons: (1) using uint*_t
turned out to be painful when using kernel headers (e.g., mptables,
e820, etc.) and (2) we want to be as close as possible to the coding
style of tools/perf to be able to reuse their code in the future.

On Tue, May 24, 2011 at 2:30 PM, Avi Kivity <avi@redhat.com> wrote:
> Like Paolo said, you can have the constructor register a function or
> structure to be called any time you like.

Oh, okay. We should probably start out with that and switch to section
tricks if we have to.

                     Pekka

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Avi Kivity]

On 05/24/2011 02:38 PM, Pekka Enberg wrote:
> Hi Avi,
>
> On Tue, May 24, 2011 at 2:30 PM, Avi Kivity<avi@redhat.com>  wrote:
> >  What happens when you start using userspace libraries?
> >
> >  Eventually you'll have a lot more of that than kernel code.
>
> I don't quite understand what problems you think we might have. We're
> already using userspace libraries (libvnc) and most of us code is
> non-kernel code.

If uint64_t is defined differently than u64, you won't be able to pass 
it by reference if an external library expects it.

> We switched to u64 and friends for two reasons: (1) using uint*_t
> turned out to be painful when using kernel headers (e.g., mptables,
> e820, etc.) and (2) we want to be as close as possible to the coding
> style of tools/perf to be able to reuse their code in the future.
>

Regarding this reuse, I see it's done by copy/paste.  Won't it be better 
to have tools/lib and have tools/perf and tools/kvm use that?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

Hi Avi,

On Tue, May 24, 2011 at 2:41 PM, Avi Kivity <avi@redhat.com> wrote:
>> I don't quite understand what problems you think we might have. We're
>> already using userspace libraries (libvnc) and most of us code is
>> non-kernel code.
>
> If uint64_t is defined differently than u64, you won't be able to pass it by
> reference if an external library expects it.

Oh, the sizes must be the same for obvious reasons so that shouldn't
be a problem and it's better to do the casting in the external API
calls than in core code.

On Tue, May 24, 2011 at 2:41 PM, Avi Kivity <avi@redhat.com> wrote:
> Regarding this reuse, I see it's done by copy/paste.  Won't it be better to
> have tools/lib and have tools/perf and tools/kvm use that?

Yes, absolutely. We're keeping things well-contained under tools/kvm
with copy-paste until we hit Linus' tree in the future, though, for
merge reasons.

                       Pekka

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/24/2011 01:56 PM, Pekka Enberg wrote:
>> >  If uint64_t is defined differently than u64, you won't be able to pass it by
>> >  reference if an external library expects it.
>
> Oh, the sizes must be the same for obvious reasons so that shouldn't
> be a problem and it's better to do the casting in the external API
> calls than in core code.

Be careful about unsigned long vs. unsigned long long.  If you want to 
use the u64 spelling that's fine, but I would use a custom header that 
defines it from uint64_t.

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Avi Kivity]

On 05/24/2011 02:41 PM, Avi Kivity wrote:
>
>> We switched to u64 and friends for two reasons: (1) using uint*_t
>> turned out to be painful when using kernel headers (e.g., mptables,
>> e820, etc.) and (2) we want to be as close as possible to the coding
>> style of tools/perf to be able to reuse their code in the future.
>>
>
> Regarding this reuse, I see it's done by copy/paste.  Won't it be 
> better to have tools/lib and have tools/perf and tools/kvm use that?
>

Another thing, I believe reuse of actual kernel code has limited 
utility.  The kernel has vast infrastructure and porting all of it to 
userspace would be a huge undertaking.  It's pretty common for code to 
use printk(), kmalloc(), slab caches, rcu, various locks, per-cpu 
variables.  Are you going to port all of it?  and maintain it when it 
changes?

It's a lot easier to use the native userspace stacks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Avi Kivity]

On 05/24/2011 03:27 PM, Paolo Bonzini wrote:
> On 05/24/2011 01:56 PM, Pekka Enberg wrote:
>>> >  If uint64_t is defined differently than u64, you won't be able to 
>>> pass it by
>>> >  reference if an external library expects it.
>>
>> Oh, the sizes must be the same for obvious reasons so that shouldn't
>> be a problem and it's better to do the casting in the external API
>> calls than in core code.
>
> Be careful about unsigned long vs. unsigned long long. 

Yes, that's the issue I had in mind.

> If you want to use the u64 spelling that's fine, but I would use a 
> custom header that defines it from uint64_t.

And now you'll have printf/printk format mismatches if they happen to be 
different.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Pekka Enberg]

Hi Avi,

On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote:
> Another thing, I believe reuse of actual kernel code has limited utility.

OK, I don't agree with that.

On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote:
>  The kernel has vast infrastructure and porting all of it to userspace would
> be a huge undertaking.  It's pretty common for code to use printk(),
> kmalloc(), slab caches, rcu, various locks, per-cpu variables.  Are you
> going to port all of it?  and maintain it when it changes?

We will port whatever is useful and makes sense. Nothing new here -
we're following perf's lead, that's all.

As for the specific issues you mention, only RCU and locking
mechanisms seem like something that are not trivial to port (although
userspace RCU already exists).

[ Porting lockdep to userspace would be one cool feature, though. ]

On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote:
> It's a lot easier to use the native userspace stacks.

We will be using userspace libs where appropriate (like with libvnc).

                        Pekka

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Pekka Enberg <penberg@kernel.org> wrote:

> We switched to u64 and friends for two reasons: (1) using uint*_t turned out 
> to be painful when using kernel headers (e.g., mptables, e820, etc.) and (2) 
> we want to be as close as possible to the coding style of tools/perf to be 
> able to reuse their code in the future.

3)

uint*_t caused frequent type mismatches and breakages with format strings if 
accidentally an uint*_t was stuck into a regular printf format - it would 
trigger a warning only on 64-bit or only on 32-bit systems, causing frequent 
flux. This is not a very good data type model.

4)

uint*_t also causes absolute brain-damaged printf format hacks like:

                fprintf(stderr, "%s: %" PRIu64 " %" PRIu64 " %" PRIu64 "\n",
                        event_name(counter), count[0], count[1], count[2]);

instead of the much cleaner:

                fprintf(stderr, "%s: %Lu %Lu %Lu\n",
                        event_name(counter), count[0], count[1], count[2]);

So we can use uint*_t where absolutely necessary due to external ABI 
constraints, but otherwise it's discouraged for tools/kvm/ internal code.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Pekka Enberg <penberg@kernel.org> wrote:

> As for the specific issues you mention, only RCU and locking mechanisms seem 
> like something that are not trivial to port (although userspace RCU already 
> exists).

Indeed.

> [ Porting lockdep to userspace would be one cool feature, though. ]

Agreed - we already ran into a few locking bugs that were IMO needlessly 
difficult to debug - our expectations got spoiled by lockdep i guess! :-)

Porting RCU would also have its distinct advantages in terms of scalability. 
While tools/perf/ probably wont need it (all its scalability-relevant code runs 
in the kernel), tools/kvm/ definitely looks like a candidate for good 
user-space RCU primitives.

> On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote:
> > It's a lot easier to use the native userspace stacks.
> 
> We will be using userspace libs where appropriate (like with libvnc).

We are using a fair amount of userspace libraries in perf too: -ldw, -lelf, 
-lnewt, -lslang, we used -laudit too for some time.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/24/2011 10:50 AM, Ingo Molnar wrote:
> >Yeah, that would certainly work for simple things but there's several reasons
> >why explicit control over initcalls is preferred in a tool like tools/kvm/ over
> >__attribute__((constructor)):
> 
> Some advantages you mention are real indeed, but they are general;
> there's no reason why they apply only to tools/kvm.  You can achieve
> the same by doing only minimal work (registering your
> subsystems/devices/whatever in a linked list) in the constructors.
> Then you iterate on the list and call function pointers.

Well, the plain fact that __attribute__((constructor)) gets called on binary 
and shared library startup before main() is called is a show-stopper for us as 
i described it in my previous mail, so why are we even arguing about it?

((constructor)) has showstopper properties:

 - We don't have access to the program arguments

 - stdio is probably not set up yet (this is undefined AFAICS)

 - Also, over the years i have grown to be suspicious of GCC defined 
   extensions. More often than not the GCC project is fixing regressions not by 
   fixing the compiler but by changing the documentation ;-) We got bitten by 
   regressions in asm() behavior in the kernel rather often.

   In that sense ((section)) is way more robust: there's not really that many 
   ways to screw that up. Fiddling with the ((constructor)) environment on the 
   other hand ...

Note that in terms of explicit iterations we do that with 
__attribute__((section)) as well: except that *we* define where and how the 
functions get called.

> I know portability is not relevant to tools/kvm/, but using unportable tricks 
> for the sake of using them is a direct way to NIH. But oh well all of 
> tools/kvm/ is NIH after all. :)

__attribute__((constructor)) is not particularly portable to begin with: does 
the MSVC compiler support it for example?

So __attribute__ ((section)), which is used by the initcall() machinery is 
similarly portable: GCC and LLVM/Clang support it.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/24/2011 10:50 AM, Ingo Molnar wrote:
> >  - C constructors have random limitations like apparently not being executed if
> >    the constructor is linked into a .a static library.
> 
> Ah, forgot about this.  Given _why_ this happens (for static libraries, the 
> linker omits object modules that are not required to fulfill undefined 
> references in previous objects), I'd be surprised if explicit section tricks 
> do not have the same limitation.  [...]

Since we create an actual array (data) and iterate it the worst i can imagine 
is the linker dropping those sections - in which case the build will fail and 
we are alerted to the problem.

With ((constructor)) the program will misbehave silently - not good.

> [...] After all the constructor attribute is only syntactic sugar for section 
> tricks.

Yeah but not just syntactic sugar but also code built into glibc to execute 
them at some point before main().

And this last bit really matters as well.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/24/2011 09:40 PM, Ingo Molnar wrote:
> >  Ah, forgot about this.  Given_why_  this happens (for static libraries, the
> >  linker omits object modules that are not required to fulfill undefined
> >  references in previous objects), I'd be surprised if explicit section tricks
> >  do not have the same limitation.  [...]
>
> Since we create an actual array (data)

Are you?  I figured it was like this example from Linux:

         .x86_cpu_dev.init : AT(ADDR(.x86_cpu_dev.init) - LOAD_OFFSET) {
                 __x86_cpu_dev_start = .;
                 *(.x86_cpu_dev.init)
                 __x86_cpu_dev_end = .;
         }


   extern const struct cpu_dev *const __x86_cpu_dev_start[],
                               *const __x86_cpu_dev_end[];

   for (cdev = __x86_cpu_dev_start; cdev < __x86_cpu_dev_end; cdev++) {

This is pretty much the same as the linker script for ELF:

   .init_array     : {
      __init_array_start = .;
      *(.init_array)
      *(SORT(.init_array$*))
      __init_array_end = .;
   }

   extern void (*__init_array_start []) (int, char **, char **)
     attribute_hidden;
   extern void (*__init_array_end []) (int, char **, char **)
     attribute_hidden;

   const size_t size = __init_array_end - __init_array_start;
   for (size_t i = 0; i < size; i++)
       (*__init_array_start [i]) (argc, argv, envp);


> ((constructor)) has showstopper properties:
>
>  - We don't have access to the program arguments
>
>  - stdio is probably not set up yet (this is undefined AFAICS)

As I said, you can do this even better by doing only minimal work in the 
constructor.  Create a struct with several callbacks (pre_init, 
late_init, destroy, reset, whatever) and possibly other information (a 
human-readable device name and command-line argument to access it, for 
example).  In the constructor you just build a linked list of said 
structs, and then you can walk it whenever you see fit: do comparisons, 
call a function, whatever.  This is similar to the cpudev example from 
the kernel above.

A simple example from QEMU:

   static void virtio_pci_register_devices(void)
   {
       pci_qdev_register_many(virtio_info);
   }

   /* This macro wraps ((constructor)).  */
   device_init(virtio_pci_register_devices)

>    In that sense ((section)) is way more robust: there's not really that many
>    ways to screw that up. Fiddling with the ((constructor)) environment on the
>    other hand ...

Sorry, this is textbook FUD.

> __attribute__((constructor)) is not particularly portable to begin with: does
> the MSVC compiler support it for example?

No, but GCC supports it on non-ELF platforms, where you would need a 
similar but different linker script.  (Also, the differences between 
MSVC and GCC can easily be abstracted with a macro).

More practically, is your linker script supported by gold?

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> >   In that sense ((section)) is way more robust: there's not really that many
> >   ways to screw that up. Fiddling with the ((constructor)) environment on the
> >   other hand ...
> 
> Sorry, this is textbook FUD.

I specifically cited the problem of static libraries. They *do not work* with 
((constructor)).

That's not FUD but justified scepticism backed by hard evidence.

> > __attribute__((constructor)) is not particularly portable to begin with: 
> > does the MSVC compiler support it for example?
> 
> No, but GCC supports it on non-ELF platforms, [...]

So you claim that ((section)) is 'not portable' but concede that it does in 
fact not port to one of the most widely used compilers.

Instead you clarify that under 'not portable' you really meant to say that 
((section)) does not work on non-ELF binary platforms.

That is a pretty inconsistent position and has nothing to do with 'portability' 
in itself but with being portable to what *you* consider important.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > ((constructor)) has showstopper properties:
> >
> > - We don't have access to the program arguments
> >
> > - stdio is probably not set up yet (this is undefined AFAICS)
> 
> As I said, you can do this even better by doing only minimal work in
> the constructor. [...]

The costs of doing that:

  - extra per init entry data structures

  - extra code being run during the constructor phase

  - the risk of not getting the ((constructor)) init run at all,
    such as on static libraries

The only benefit is:

  - ((constructor)) is a non-portable GCC extension so the only portability
    win is to possibly support non-ELF targets. Which ones do we care about?

At this point it is a rather legitimate technical decision for us to say that 
we do not want to bear those cost for that limited benefit, wouldn't you agree?

If someone adds non-ELF support then it can all be switched over to the bit 
more bloated ((constructor)) approach rather easily - so this all is a 
non-issue really.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/25/2011 10:32 AM, Ingo Molnar wrote:
>
> * Paolo Bonzini<pbonzini@redhat.com>  wrote:
>
>>>    In that sense ((section)) is way more robust: there's not really that many
>>>    ways to screw that up. Fiddling with the ((constructor)) environment on the
>>>    other hand ...
>>
>> Sorry, this is textbook FUD.
>
> I specifically cited the problem of static libraries. They *do not work* with
> ((constructor)).

((constructor)) has easily explained semantics: it's the same semantics 
as C++ global constructors.  If you don't like those, that's another story.

But anyway, I specifically cited the reason why ((section)) is not any 
better, AFAIU how you would use it.  Now I didn't trust myself and tried it.

f.ld:
	SECTIONS {
		.paolo : {
		        PROVIDE(__paolo_start = .);
		        *(.paolo)
		        PROVIDE(__paolo_end = .);
		}
	}

f.c:
	int x __attribute__((section(".paolo"))) = 0x12345678;

	int main()
	{
		extern int __paolo_start[];
		extern int __paolo_end[];
	
		int *p;
		for (p = __paolo_start; p < __paolo_end; p++) {
			printf ("%x\n", *p);
		}
	}

f1.c:
	int y __attribute__((section(".paolo"))) = 0x76543210;

compilation with objects:
	$ gcc f.c -o f.o -c
	$ gcc f1.c -o f1.o -c
	$ ld -r f.ld f.o f1.o -o f2.o
	$ gcc f2.o -o a.out
	$ ./a.out
	12345678
	76543210

compilation with static libraries:
	$ gcc f.c -o f.o -c
	$ gcc f1.c -o f1.o -c
	$ ar cr f1.a f1.o
	$ ld -r f.ld f.o f1.a -o f2.o
	$ gcc f2.o -o a.out
	$ ./a.out
	12345678

Since this is userspace, you do not have complete control on the build 
and you still need to use gcc to drive the final link.

So you get exactly the same semantics as ((constructor)), plus one extra 
build step that spits out a warning ("ld: warning: f.ld contains output 
sections; did you forget -T?").

>>> __attribute__((constructor)) is not particularly portable to begin with:
>>> does the MSVC compiler support it for example?
>>
>> No, but GCC supports it on non-ELF platforms, [...]
>
> So you claim that ((section)) is 'not portable' but concede that it does in
> fact not port to one of the most widely used compilers.

Can you please avoid trimming messages?  I said "the differences between 
MSVC and GCC can easily be abstracted with a macro".  This is not unlike 
other differences between the two compilers (e.g. __forceinline vs. the 
((always_inline)) attribute), and it is not true of linker scripts.

> Instead you clarify that under 'not portable' you really meant to say that
> ((section)) does not work on non-ELF binary platforms.
>
> That is a pretty inconsistent position and has nothing to do with 'portability'
> in itself but with being portable to what *you* consider important.

My definition of "(reasonably) portable" includes having to write a 
5-line macro that works across _all_ GCC targets and _all_ MSVC targets.

Having to change the build process for every compiler target (see the 
above "ld -r" step) is a bit more unportable, though not much. 
Potentially having to modify the custom linker script for every targeted 
architecture (the above doesn't work for mingw32, it needs an extra _ in 
front of the start/end symbols) is pretty much my definition of 
"unportable".

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > So you claim that ((section)) is 'not portable' but concede that 
> > it does in fact not port to one of the most widely used 
> > compilers.
> 
> Can you please avoid trimming messages?  I said "the differences 
> between MSVC and GCC can easily be abstracted with a macro".  This 
> is not unlike other differences between the two compilers (e.g. 
> __forceinline vs. the ((always_inline)) attribute), and it is not 
> true of linker scripts.

So ((constructor)) is unportable but it can be wrapped. So can other 
portability problems be solved, such as linker scripts can be written 
for non-ELF targets (should that ever become necessary), so what's 
your point?

We want to use ELF sections in the future *anyway* to make tools/kvm/ 
scale even better, for example to use alternative instruction fixups 
(for which no GCC extension exists) so a linker script will be there 
anyway. ( We might also want to use sections to speed up exception 
handling in tools/kvm/. )

Fact is that neither ((section)) *NOR* ((constructor)) is portable: 
*both* are GCC extensions - while you falsely claimed that 
((constructor)) was somehow portable and that ((section)) was an 
'unportable trick'. Let me quote your initial claim:

>>>>> I know portability is not relevant to tools/kvm/, but using 
>>>>> unportable tricks for the sake of using them is a direct way to 
>>>>> NIH. But oh well all of tools/kvm/ is NIH after all. :)

Btw., that NIH claim was rather unfair and uncalled for as well.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/25/2011 10:32 AM, Ingo Molnar wrote:
> >
> >* Paolo Bonzini<pbonzini@redhat.com>  wrote:
> >
> >>>   In that sense ((section)) is way more robust: there's not really that many
> >>>   ways to screw that up. Fiddling with the ((constructor)) environment on the
> >>>   other hand ...
> >>
> >> Sorry, this is textbook FUD.
> >
> > I specifically cited the problem of static libraries. They *do 
> > not work* with ((constructor)).
> 
> ((constructor)) has easily explained semantics: it's the same 
> semantics as C++ global constructors.  If you don't like those, 
> that's another story.

It is a plain *bug* if device initialization is not being executed if 
kvm is linked into a static library using the regular way of how 
libraries are created ...

No amount of arguing about 'semantics' will change that simple fact: 
it breaks code so it's a bug. We don't want to rely on a facility 
that handles boundary conditions in such a poor way.

See my prior point:

 - Also, over the years i have grown to be suspicious of GCC defined
   extensions. More often than not the GCC project is fixing regressions not by
   fixing the compiler but by changing the documentation ;-) We got bitten by
   regressions in asm() behavior in the kernel rather often.

   In that sense ((section)) is way more robust: there's not really that many
   ways to screw that up. Fiddling with the ((constructor)) environment on the
   other hand ...

You are demonstrating this phenomenon rather well. You argue against 
plain old bugs with 'but these are well-defined semantics'.

That's not how we deal with bugs in tools/kvm/ really.

And then you argue that the bug can be worked around by writing a 
linker script:

 > compilation with static libraries:
 >       $ gcc f.c -o f.o -c
 >       $ gcc f1.c -o f1.o -c
 >       $ ar cr f1.a f1.o
 >       $ ld -r f.ld f.o f1.a -o f2.o
 >       $ gcc f2.o -o a.out
 >       $ ./a.out
 >       12345678

I will rather use linker scripts and less erratic facilities straight 
away.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/25/2011 11:36 AM, Ingo Molnar wrote:
> So can other portability problems be solved, such as linker scripts
> can be written for non-ELF targets (should that ever become
> necessary)

I know that's not going to be the case, and I mentioned that in my first 
message---portability is not relevant to tools/kvm/ just like it is not 
relevant say to systemd.

> so what's your point?

Using this kind of trick makes it harder to share code with other 
libraries that may require a higher standard of portability (not 
"better" or "worse", just "higher").  QEMU _does_ have this problem BTW, 
we have our own event loop and AIO, our own JSON parser, our own thread 
abstractions, and so on...

> We want to use ELF sections in the future *anyway* to make
> tools/kvm/ scale even better, for example to use alternative
> instruction fixups (for which no GCC extension exists) so a linker
> script will be there anyway.

Since you're not writing the hypervisor, but only the device model, I
doubt that this is needed (and you'd likely run into troubles with
SELinux).  QEMU scales to dozens of VCPUs with a big lock around all 
userspace device model code.  But it's premature to say that, and I'd be 
happy if I were proven wrong.

> Fact is that neither ((section)) *NOR* ((constructor)) is portable:
> *both* are GCC extensions - while you falsely claimed that
> ((constructor)) was somehow portable and that ((section)) was an
> 'unportable trick'.

Fair enough.  I agree that ((constructor)) has limited scope and is 
compiler-unportable.  ((section)) is more powerful and has the cost of 
using also target-unportable linker scripts, and requiring changes to 
the build system.  Both have the same problem with static libraries 
(please reread my message).

>>>>>> I know portability is not relevant to tools/kvm/, but
>>>>>> using unportable tricks for the sake of using them is a
>>>>>> direct way to NIH. But oh well all of tools/kvm/ is NIH
>>>>>> after all. :)
>
> Btw., that NIH claim was rather unfair and uncalled for as well.

Hey hey I put a smiley for a reason!

Anyway I think we both agree that this debate is pointless.  I learnt 
something (I wasn't aware of interaction between ((constructor)) and 
static libraries), you learnt something (it's the same with ((section)), 
and it's intrinsic in how static libraries work).

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > so what's your point?
> 
> Using this kind of trick makes it harder to share code with other 
> libraries that may require a higher standard of portability (not 
> "better" or "worse", just "higher"). [...]

That's an complication but should be fixable, should it ever happen.

As things stand today we:

 - Are *already* using an ELF linker script, see tools/kvm/bios/rom.ld.S

 - Have multiple valid reasons not to use ((constructor))

 - Want to use sections to implement other useful features as well

If the *only* linker script use would be the init facility then you'd 
probably have a valid point - although the possible code flow 
fragility with ((constructor)) is still a problem: we still would 
want to know when no constructors were executed.

Also it's not clear why ((constructor)) was written in the way it 
was: why apparently no access is given to the array of init functions 
and why it's not possible to turn the auto-execution off but still 
have the array generated, for legitimate cases that want to use data 
driven constructor execution.

> >>>>>> I know portability is not relevant to tools/kvm/, but using 
> >>>>>> unportable tricks for the sake of using them is a direct way 
> >>>>>> to NIH. But oh well all of tools/kvm/ is NIH after all. :)
> >
> > Btw., that NIH claim was rather unfair and uncalled for as well.
> 
> Hey hey I put a smiley for a reason!

Well after two insults in a single paragraph you need to put in at 
least two smileys! Or not write the insults in a technical discssion 
to begin with, especially if you are criticising a patch rather 
forcefully. It will be easily misunderstood as a real insult, despite 
the smiley ;-)

> Anyway I think we both agree that this debate is pointless.  I 
> learnt something (I wasn't aware of interaction between 
> ((constructor)) and static libraries), you learnt something (it's 
> the same with ((section)), and it's intrinsic in how static 
> libraries work).

While i did not know whether static libraries would work with a 
linker script (never tried it - and your experiment suggests that 
they wont), the ((section)) approach we could create a clear runtime 
BUG_ON() assert for a zero-sized array of init function pointers, 
while ((constructor)) will silently not execute initialization 
functions.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/25/2011 12:17 PM, Ingo Molnar wrote:
> Also it's not clear why ((constructor)) was written in the way it
> was: why apparently no access is given to the array of init functions

It is accessible---glibc uses it.

> and why it's not possible to turn the auto-execution off but still
> have the array generated, for legitimate cases that want to use data
> driven constructor execution.

The compiler doesn't care about what is done with the data.  It simply 
provides the table for the runtime library to use it.  ((constructor)) 
is a veneer over the same infrastructure used for C++ global 
constructors; that explains its design pretty well.

> the ((section)) approach we could create a clear runtime
> BUG_ON() assert for a zero-sized array of init function pointers,

Not really.  The problem is that f1.o is not pulled from the static 
library _because the linker thinks it is not necessary_.  If f1.o 
defines another symbol, and f.o uses it, then the constructor is pulled 
in together with the rest of f.o.  As soon as _one_ constructor is 
pulled in (even from outside the static library), your BUG() would not 
detect silently missing imports anymore.

It turns out a way to get the semantics you want for ((section)) is to 
use --whole-archive (and possibly --no-whole-archive) on the "ld -r" 
command line.  You can do the same on the link line for ((constructor)) 
or C++ constructors too.  However, since you're effectively using a C++ 
feature, static libraries are probably a bad idea just like C++ static 
libraries are.

Paolo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/25/2011 12:17 PM, Ingo Molnar wrote:
>
> > Also it's not clear why ((constructor)) was written in the way it 
> > was: why apparently no access is given to the array of init 
> > functions
> 
> It is accessible---glibc uses it.
> 
> > and why it's not possible to turn the auto-execution off but 
> > still have the array generated, for legitimate cases that want to 
> > use data driven constructor execution.
> 
> The compiler doesn't care about what is done with the data.  It 
> simply provides the table for the runtime library to use it. 
> ((constructor)) is a veneer over the same infrastructure used for 
> C++ global constructors; that explains its design pretty well.

Obviously the compiler did not provide this feature into a vacuum, it 
expected the C library to execute constructors, right?

So the above dodges my question of why there is no method (in glibc) 
to turn auto-execution off and let the app do it.

If all that was possible then ((constructor)) could certainly be used 
as a generalized shortcut for certain ((section)) uses.

> > the ((section)) approach we could create a clear runtime BUG_ON() 
> > assert for a zero-sized array of init function pointers,
> 
> Not really.  The problem is that f1.o is not pulled from the static 
> library _because the linker thinks it is not necessary_.  If f1.o 
> defines another symbol, and f.o uses it, then the constructor is 
> pulled in together with the rest of f.o.  As soon as _one_ 
> constructor is pulled in (even from outside the static library), 
> your BUG() would not detect silently missing imports anymore.

Yeah, good point.

Thanks,

	Ingo

Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC

[Paolo Bonzini]

On 05/25/2011 02:53 PM, Ingo Molnar wrote:
> Obviously the compiler did not provide this feature into a vacuum, it
> expected the C library to execute constructors, right?
>
> So the above dodges my question of why there is no method (in glibc)
> to turn auto-execution off and let the app do it.

Well, I wasn't there. :)

I suppose they simply chose those semantics for C++ constructors which 
were easiest to implement (i.e. execute before main).  ((constructor)) 
then followed suit.

Any further extension in the direction you suggest would have to be 
designed very carefully in order to coexist with C++ constructors, or it 
would have to be something completely separate... in the latter case, 
((constructor)) already provides the ingredients to do this, even if at 
a small cost in efficiency.  So there is no big incentive to add this 
further extension to the compiler and/or the C library.

Paolo