* [PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization
2011-05-23 11:19 [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Sasha Levin
@ 2011-05-23 11:19 ` Sasha Levin
2011-05-23 11:30 ` Ingo Molnar
2011-05-23 11:19 ` [PATCH 3/5 V2] kvm tools: Add VESA device Sasha Levin
` (3 subsequent siblings)
4 siblings, 1 reply; 43+ messages in thread
From: Sasha Levin @ 2011-05-23 11:19 UTC (permalink / raw)
To: penberg
Cc: john, kvm, mingo, asias.hejun, gorcunov, prasadjoshi124, 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization
2011-05-23 11:19 ` [PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization Sasha Levin
@ 2011-05-23 11:30 ` Ingo Molnar
0 siblings, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-23 11:30 UTC (permalink / raw)
To: Sasha Levin; +Cc: penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 3/5 V2] kvm tools: Add VESA device
2011-05-23 11:19 [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Sasha Levin
2011-05-23 11:19 ` [PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization Sasha Levin
@ 2011-05-23 11:19 ` Sasha Levin
2011-05-23 11:32 ` Ingo Molnar
2011-05-23 11:19 ` [PATCH 4/5 V2] kvm tools: Update makefile and feature tests Sasha Levin
` (2 subsequent siblings)
4 siblings, 1 reply; 43+ messages in thread
From: Sasha Levin @ 2011-05-23 11:19 UTC (permalink / raw)
To: penberg
Cc: john, kvm, mingo, asias.hejun, gorcunov, prasadjoshi124, 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 4/5 V2] kvm tools: Update makefile and feature tests
2011-05-23 11:19 [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Sasha Levin
2011-05-23 11:19 ` [PATCH 2/5 V2] kvm tools: Add video mode to kernel initialization Sasha Levin
2011-05-23 11:19 ` [PATCH 3/5 V2] kvm tools: Add VESA device Sasha Levin
@ 2011-05-23 11:19 ` Sasha Levin
2011-05-23 11:19 ` [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC Sasha Levin
2011-05-23 11:29 ` [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Ingo Molnar
4 siblings, 0 replies; 43+ messages in thread
From: Sasha Levin @ 2011-05-23 11:19 UTC (permalink / raw)
To: penberg
Cc: john, kvm, mingo, asias.hejun, gorcunov, prasadjoshi124, 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-23 11:19 [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Sasha Levin
` (2 preceding siblings ...)
2011-05-23 11:19 ` [PATCH 4/5 V2] kvm tools: Update makefile and feature tests Sasha Levin
@ 2011-05-23 11:19 ` Sasha Levin
2011-05-23 11:38 ` Ingo Molnar
2011-05-23 14:10 ` Pekka Enberg
2011-05-23 11:29 ` [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Ingo Molnar
4 siblings, 2 replies; 43+ messages in thread
From: Sasha Levin @ 2011-05-23 11:19 UTC (permalink / raw)
To: penberg
Cc: john, kvm, mingo, asias.hejun, gorcunov, prasadjoshi124, 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
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-23 11:19 ` [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC Sasha Levin
@ 2011-05-23 11:38 ` Ingo Molnar
2011-05-23 11:45 ` Pekka Enberg
2011-05-24 8:37 ` Paolo Bonzini
2011-05-23 14:10 ` Pekka Enberg
1 sibling, 2 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-23 11:38 UTC (permalink / raw)
To: Sasha Levin; +Cc: penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-23 11:38 ` Ingo Molnar
@ 2011-05-23 11:45 ` Pekka Enberg
2011-05-24 8:37 ` Paolo Bonzini
1 sibling, 0 replies; 43+ messages in thread
From: Pekka Enberg @ 2011-05-23 11:45 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Sasha Levin, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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?
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-23 11:38 ` Ingo Molnar
2011-05-23 11:45 ` Pekka Enberg
@ 2011-05-24 8:37 ` Paolo Bonzini
2011-05-24 8:50 ` Ingo Molnar
2011-05-24 8:51 ` Cyrill Gorcunov
1 sibling, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-24 8:37 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 8:37 ` Paolo Bonzini
@ 2011-05-24 8:50 ` Ingo Molnar
2011-05-24 9:10 ` Paolo Bonzini
2011-05-24 9:18 ` Paolo Bonzini
2011-05-24 8:51 ` Cyrill Gorcunov
1 sibling, 2 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-24 8:50 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 8:50 ` Ingo Molnar
@ 2011-05-24 9:10 ` Paolo Bonzini
2011-05-24 9:55 ` Pekka Enberg
2011-05-24 19:16 ` Ingo Molnar
2011-05-24 9:18 ` Paolo Bonzini
1 sibling, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-24 9:10 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 9:10 ` Paolo Bonzini
@ 2011-05-24 9:55 ` Pekka Enberg
2011-05-24 11:22 ` Avi Kivity
2011-05-24 19:16 ` Ingo Molnar
1 sibling, 1 reply; 43+ messages in thread
From: Pekka Enberg @ 2011-05-24 9:55 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Ingo Molnar, Sasha Levin, john, kvm, asias.hejun, gorcunov,
prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 9:55 ` Pekka Enberg
@ 2011-05-24 11:22 ` Avi Kivity
2011-05-24 11:26 ` Pekka Enberg
0 siblings, 1 reply; 43+ messages in thread
From: Avi Kivity @ 2011-05-24 11:22 UTC (permalink / raw)
To: Pekka Enberg
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:22 ` Avi Kivity
@ 2011-05-24 11:26 ` Pekka Enberg
2011-05-24 11:30 ` Sasha Levin
2011-05-24 11:30 ` Avi Kivity
0 siblings, 2 replies; 43+ messages in thread
From: Pekka Enberg @ 2011-05-24 11:26 UTC (permalink / raw)
To: Avi Kivity
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:26 ` Pekka Enberg
@ 2011-05-24 11:30 ` Sasha Levin
2011-05-24 11:30 ` Avi Kivity
1 sibling, 0 replies; 43+ messages in thread
From: Sasha Levin @ 2011-05-24 11:30 UTC (permalink / raw)
To: Pekka Enberg
Cc: Avi Kivity, Paolo Bonzini, Ingo Molnar, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:26 ` Pekka Enberg
2011-05-24 11:30 ` Sasha Levin
@ 2011-05-24 11:30 ` Avi Kivity
2011-05-24 11:38 ` Pekka Enberg
1 sibling, 1 reply; 43+ messages in thread
From: Avi Kivity @ 2011-05-24 11:30 UTC (permalink / raw)
To: Pekka Enberg
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:30 ` Avi Kivity
@ 2011-05-24 11:38 ` Pekka Enberg
2011-05-24 11:41 ` Avi Kivity
2011-05-24 19:00 ` Ingo Molnar
0 siblings, 2 replies; 43+ messages in thread
From: Pekka Enberg @ 2011-05-24 11:38 UTC (permalink / raw)
To: Avi Kivity
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:38 ` Pekka Enberg
@ 2011-05-24 11:41 ` Avi Kivity
2011-05-24 11:56 ` Pekka Enberg
2011-05-24 14:37 ` Avi Kivity
2011-05-24 19:00 ` Ingo Molnar
1 sibling, 2 replies; 43+ messages in thread
From: Avi Kivity @ 2011-05-24 11:41 UTC (permalink / raw)
To: Pekka Enberg
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:41 ` Avi Kivity
@ 2011-05-24 11:56 ` Pekka Enberg
2011-05-24 12:27 ` Paolo Bonzini
2011-05-24 14:37 ` Avi Kivity
1 sibling, 1 reply; 43+ messages in thread
From: Pekka Enberg @ 2011-05-24 11:56 UTC (permalink / raw)
To: Avi Kivity
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:56 ` Pekka Enberg
@ 2011-05-24 12:27 ` Paolo Bonzini
2011-05-24 14:38 ` Avi Kivity
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-24 12:27 UTC (permalink / raw)
To: Pekka Enberg
Cc: Avi Kivity, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 12:27 ` Paolo Bonzini
@ 2011-05-24 14:38 ` Avi Kivity
0 siblings, 0 replies; 43+ messages in thread
From: Avi Kivity @ 2011-05-24 14:38 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Pekka Enberg, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:41 ` Avi Kivity
2011-05-24 11:56 ` Pekka Enberg
@ 2011-05-24 14:37 ` Avi Kivity
2011-05-24 14:54 ` Pekka Enberg
1 sibling, 1 reply; 43+ messages in thread
From: Avi Kivity @ 2011-05-24 14:37 UTC (permalink / raw)
To: Pekka Enberg
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 14:37 ` Avi Kivity
@ 2011-05-24 14:54 ` Pekka Enberg
2011-05-24 19:03 ` Ingo Molnar
0 siblings, 1 reply; 43+ messages in thread
From: Pekka Enberg @ 2011-05-24 14:54 UTC (permalink / raw)
To: Avi Kivity
Cc: Paolo Bonzini, Ingo Molnar, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 14:54 ` Pekka Enberg
@ 2011-05-24 19:03 ` Ingo Molnar
0 siblings, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-24 19:03 UTC (permalink / raw)
To: Pekka Enberg
Cc: Avi Kivity, Paolo Bonzini, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 11:38 ` Pekka Enberg
2011-05-24 11:41 ` Avi Kivity
@ 2011-05-24 19:00 ` Ingo Molnar
1 sibling, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-24 19:00 UTC (permalink / raw)
To: Pekka Enberg
Cc: Avi Kivity, Paolo Bonzini, Sasha Levin, john, kvm, asias.hejun,
gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 9:10 ` Paolo Bonzini
2011-05-24 9:55 ` Pekka Enberg
@ 2011-05-24 19:16 ` Ingo Molnar
1 sibling, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-24 19:16 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 8:50 ` Ingo Molnar
2011-05-24 9:10 ` Paolo Bonzini
@ 2011-05-24 9:18 ` Paolo Bonzini
2011-05-24 19:40 ` Ingo Molnar
1 sibling, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-24 9:18 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 9:18 ` Paolo Bonzini
@ 2011-05-24 19:40 ` Ingo Molnar
2011-05-25 8:21 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Ingo Molnar @ 2011-05-24 19:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 19:40 ` Ingo Molnar
@ 2011-05-25 8:21 ` Paolo Bonzini
2011-05-25 8:32 ` Ingo Molnar
2011-05-25 8:38 ` Ingo Molnar
0 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-25 8:21 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 8:21 ` Paolo Bonzini
@ 2011-05-25 8:32 ` Ingo Molnar
2011-05-25 9:15 ` Paolo Bonzini
2011-05-25 8:38 ` Ingo Molnar
1 sibling, 1 reply; 43+ messages in thread
From: Ingo Molnar @ 2011-05-25 8:32 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 8:32 ` Ingo Molnar
@ 2011-05-25 9:15 ` Paolo Bonzini
2011-05-25 9:36 ` Ingo Molnar
2011-05-25 9:49 ` Ingo Molnar
0 siblings, 2 replies; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-25 9:15 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 9:15 ` Paolo Bonzini
@ 2011-05-25 9:36 ` Ingo Molnar
2011-05-25 10:01 ` Paolo Bonzini
2011-05-25 9:49 ` Ingo Molnar
1 sibling, 1 reply; 43+ messages in thread
From: Ingo Molnar @ 2011-05-25 9:36 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 9:36 ` Ingo Molnar
@ 2011-05-25 10:01 ` Paolo Bonzini
2011-05-25 10:17 ` Ingo Molnar
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-25 10:01 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 10:01 ` Paolo Bonzini
@ 2011-05-25 10:17 ` Ingo Molnar
2011-05-25 10:44 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Ingo Molnar @ 2011-05-25 10:17 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 10:17 ` Ingo Molnar
@ 2011-05-25 10:44 ` Paolo Bonzini
2011-05-25 12:53 ` Ingo Molnar
0 siblings, 1 reply; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-25 10:44 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 10:44 ` Paolo Bonzini
@ 2011-05-25 12:53 ` Ingo Molnar
2011-05-25 15:37 ` Paolo Bonzini
0 siblings, 1 reply; 43+ messages in thread
From: Ingo Molnar @ 2011-05-25 12:53 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 12:53 ` Ingo Molnar
@ 2011-05-25 15:37 ` Paolo Bonzini
0 siblings, 0 replies; 43+ messages in thread
From: Paolo Bonzini @ 2011-05-25 15:37 UTC (permalink / raw)
To: Ingo Molnar
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 9:15 ` Paolo Bonzini
2011-05-25 9:36 ` Ingo Molnar
@ 2011-05-25 9:49 ` Ingo Molnar
1 sibling, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-25 9:49 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-25 8:21 ` Paolo Bonzini
2011-05-25 8:32 ` Ingo Molnar
@ 2011-05-25 8:38 ` Ingo Molnar
1 sibling, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-25 8:38 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sasha Levin, penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-24 8:37 ` Paolo Bonzini
2011-05-24 8:50 ` Ingo Molnar
@ 2011-05-24 8:51 ` Cyrill Gorcunov
1 sibling, 0 replies; 43+ messages in thread
From: Cyrill Gorcunov @ 2011-05-24 8:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Ingo Molnar, Sasha Levin, penberg, john, kvm, asias.hejun,
prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC
2011-05-23 11:19 ` [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC Sasha Levin
2011-05-23 11:38 ` Ingo Molnar
@ 2011-05-23 14:10 ` Pekka Enberg
1 sibling, 0 replies; 43+ messages in thread
From: Pekka Enberg @ 2011-05-23 14:10 UTC (permalink / raw)
To: Sasha Levin; +Cc: john, kvm, mingo, asias.hejun, gorcunov, prasadjoshi124
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
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler
2011-05-23 11:19 [PATCH 1/5 V2] kvm tools: Add BIOS INT10 handler Sasha Levin
` (3 preceding siblings ...)
2011-05-23 11:19 ` [PATCH 5/5 V2] kvm tools: Initialize and use VESA and VNC Sasha Levin
@ 2011-05-23 11:29 ` Ingo Molnar
4 siblings, 0 replies; 43+ messages in thread
From: Ingo Molnar @ 2011-05-23 11:29 UTC (permalink / raw)
To: Sasha Levin; +Cc: penberg, john, kvm, asias.hejun, gorcunov, prasadjoshi124
* 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
^ permalink raw reply [flat|nested] 43+ messages in thread