[PATCH] kvm/ia64: Add printk support for kvm-intel modules.

[PATCH] kvm/ia64: Add printk support for kvm-intel modules.

[Zhang, Xiantao]

[-- Attachment #1: Type: text/plain, Size: 6267 bytes --]

Resend. 
Avi, 
	Please help to apply it, Thanks! 
Xiantao

>From 8df3865407420142f1e367616ca41ebf0a4e6e29 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Tue, 9 Sep 2008 22:47:55 +0800
Subject: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

Since this module will be reloated to an isolated address
space from host side, so kvm-intel can't call printk of host
kernel. This patch implements the printk function for kvm-intel
module, so it doesn't need to suffer no-printk pains.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 arch/ia64/include/asm/kvm_host.h |   17 +++++++++++++++--
 arch/ia64/kvm/Makefile           |    2 +-
 arch/ia64/kvm/kvm-ia64.c         |   25 ++++++++++++++++++++++++-
 arch/ia64/kvm/kvm_lib.c          |   13 +++++++++++++
 arch/ia64/kvm/vmm.c              |   21 +++++++++++++++++++++
 5 files changed, 74 insertions(+), 4 deletions(-)
 create mode 100644 arch/ia64/kvm/kvm_lib.c

diff --git a/arch/ia64/include/asm/kvm_host.h
b/arch/ia64/include/asm/kvm_host.h
index 1efe513..78f3712 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -55,13 +55,17 @@
 #define KVM_VMM_SIZE (16UL<<20)
 #define KVM_VMM_SHIFT 24
 #define KVM_VMM_BASE 0xD000000000000000UL
-#define VMM_SIZE (8UL<<20)
+#define VMM_SIZE (4UL<<20)
+
+/*Define vmm log buffer*/
+#define KVM_VMM_LOG_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define VMM_LOG_SIZE  (4UL<<20)
 
 /*
  * Define vm_buffer, used by PAL Services, base address.
  * Note: vmbuffer is in the VMM-BLOCK, the size must be < 8M
  */
-#define KVM_VM_BUFFER_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define KVM_VM_BUFFER_BASE (KVM_VMM_LOG_BASE + VMM_LOG_SIZE)
 #define KVM_VM_BUFFER_SIZE (8UL<<20)
 
 /*Define Virtual machine data layout.*/
@@ -524,4 +528,13 @@ void kvm_sal_emul(struct kvm_vcpu *vcpu);
 
 static inline void kvm_inject_nmi(struct kvm_vcpu *vcpu) {}
 
+#define BYTES_PER_LOG 1024
+
+struct kvm_vmm_log {
+	spinlock_t log_lock;
+	unsigned long w_pointer;
+	unsigned long r_pointer;
+	char log_slot[VMM_LOG_SIZE/BYTES_PER_LOG][BYTES_PER_LOG - 1];
+};
+
 #endif
diff --git a/arch/ia64/kvm/Makefile b/arch/ia64/kvm/Makefile
index bf22fb9..0d17d84 100644
--- a/arch/ia64/kvm/Makefile
+++ b/arch/ia64/kvm/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_KVM) += kvm.o
 FORCE : $(obj)/$(offsets-file)
 EXTRA_CFLAGS_vcpu.o += -mfixed-range=f2-f5,f12-f127
 kvm-intel-objs = vmm.o vmm_ivt.o trampoline.o vcpu.o optvfault.o mmio.o
\
-	vtlb.o process.o
+	vtlb.o process.o kvm_lib.o
 #Add link memcpy and memset to avoid possible structure assignment
error
 kvm-intel-objs += memcpy.o memset.o
 obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8a3b5fc..17f2cd7 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -50,6 +50,7 @@ static unsigned long kvm_vsa_base;
 static unsigned long kvm_vm_buffer;
 static unsigned long kvm_vm_buffer_size;
 unsigned long kvm_vmm_gp;
+struct kvm_vmm_log *vmm_log;
 
 static long vp_env_info;
 
@@ -496,6 +497,21 @@ static uint32_t kvm_get_exit_reason(struct kvm_vcpu
*vcpu)
 	return p_exit_data->exit_reason;
 }
 
+static void vcpu_print_vmm_log(void)
+{
+	unsigned int slot;
+
+	spin_lock(&vmm_log->log_lock);
+
+	while (vmm_log->r_pointer < vmm_log->w_pointer) {
+		slot = vmm_log->r_pointer %
(VMM_LOG_SIZE/BYTES_PER_LOG);
+		printk("%s", vmm_log->log_slot[slot]);
+		vmm_log->r_pointer++;
+	}
+
+	spin_unlock(&vmm_log->log_lock);
+}
+
 /*
  * The guest has exited.  See if we can fix it or if we need userspace
  * assistance.
@@ -505,6 +521,9 @@ static int kvm_handle_exit(struct kvm_run *kvm_run,
struct kvm_vcpu *vcpu)
 	u32 exit_reason = kvm_get_exit_reason(vcpu);
 	vcpu->arch.last_exit = exit_reason;
 
+	/*vcpu on behalf of VMM to print vmm's kernel log */
+	vcpu_print_vmm_log();
+
 	if (exit_reason < kvm_vti_max_exit_handlers
 			&& kvm_vti_exit_handlers[exit_reason])
 		return kvm_vti_exit_handlers[exit_reason](vcpu,
kvm_run);
@@ -1011,6 +1030,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu
*vcpu,
 
 static int kvm_alloc_vmm_area(void)
 {
+
 	if (!kvm_vmm_base && (kvm_vm_buffer_size < KVM_VM_BUFFER_SIZE))
{
 		kvm_vmm_base = __get_free_pages(GFP_KERNEL,
 				get_order(KVM_VMM_SIZE));
@@ -1018,7 +1038,10 @@ static int kvm_alloc_vmm_area(void)
 			return -ENOMEM;
 
 		memset((void *)kvm_vmm_base, 0, KVM_VMM_SIZE);
-		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE;
+		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE + VMM_LOG_SIZE;
+		vmm_log = (struct kvm_vmm_log *)(kvm_vmm_base +
+					(KVM_VMM_LOG_BASE -
KVM_VMM_BASE));
+		spin_lock_init(&vmm_log->log_lock);
 
 		printk(KERN_DEBUG"kvm:VMM's Base Addr:0x%lx,
vm_buffer:0x%lx\n",
 				kvm_vmm_base, kvm_vm_buffer);
diff --git a/arch/ia64/kvm/kvm_lib.c b/arch/ia64/kvm/kvm_lib.c
new file mode 100644
index 0000000..4c43efe
--- /dev/null
+++ b/arch/ia64/kvm/kvm_lib.c
@@ -0,0 +1,13 @@
+
+/*
+ * vsprintf.c: Let kvm-intel module has the ability to use print
functions.
+ *	Just include kernel's library, and disable symbols export.
+ * 	Copyright (C) 2008, Intel Corporation.
+ *  	Xiantao Zhang  (xiantao.zhang@intel.com)
+ *
+ */
+
+#undef CONFIG_MODULES
+
+#include "../../../lib/vsprintf.c"
+#include "../../../lib/ctype.c"
diff --git a/arch/ia64/kvm/vmm.c b/arch/ia64/kvm/vmm.c
index 2275bf4..e4c3597 100644
--- a/arch/ia64/kvm/vmm.c
+++ b/arch/ia64/kvm/vmm.c
@@ -62,5 +62,26 @@ void vmm_spin_unlock(spinlock_t *lock)
 {
 	_vmm_raw_spin_unlock(lock);
 }
+
+asmlinkage int printk(const char *fmt, ...)
+{
+	struct kvm_vmm_log *vmm_log = (struct kvm_vmm_log
*)KVM_VMM_LOG_BASE;
+	unsigned int slot;
+	va_list args;
+	int r;
+
+	vmm_spin_lock(&vmm_log->log_lock);
+	slot = vmm_log->w_pointer % (VMM_LOG_SIZE/BYTES_PER_LOG);
+	memset(vmm_log->log_slot[slot], 0, BYTES_PER_LOG - 1);
+	va_start(args, fmt);
+	r = vsnprintf(vmm_log->log_slot[slot], BYTES_PER_LOG - 1, fmt,
args);
+	va_end(args);
+	vmm_log->w_pointer++;
+	wmb();
+	vmm_spin_unlock(&vmm_log->log_lock);
+
+	return r;
+}
+
 module_init(kvm_vmm_init)
 module_exit(kvm_vmm_exit)
-- 
1.5.1


[-- Attachment #2: 0001-kvm-ia64-Add-printk-support-for-kvm-intel-modules.patch --]
[-- Type: application/octet-stream, Size: 6000 bytes --]

From 8df3865407420142f1e367616ca41ebf0a4e6e29 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Tue, 9 Sep 2008 22:47:55 +0800
Subject: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

Since this module will be reloated to an isolated address
space from host side, so kvm-intel can't call printk of host
kernel. This patch implements the printk function for kvm-intel
module, so it doesn't need to suffer no-printk pains.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 arch/ia64/include/asm/kvm_host.h |   17 +++++++++++++++--
 arch/ia64/kvm/Makefile           |    2 +-
 arch/ia64/kvm/kvm-ia64.c         |   25 ++++++++++++++++++++++++-
 arch/ia64/kvm/kvm_lib.c          |   13 +++++++++++++
 arch/ia64/kvm/vmm.c              |   21 +++++++++++++++++++++
 5 files changed, 74 insertions(+), 4 deletions(-)
 create mode 100644 arch/ia64/kvm/kvm_lib.c

diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index 1efe513..78f3712 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -55,13 +55,17 @@
 #define KVM_VMM_SIZE (16UL<<20)
 #define KVM_VMM_SHIFT 24
 #define KVM_VMM_BASE 0xD000000000000000UL
-#define VMM_SIZE (8UL<<20)
+#define VMM_SIZE (4UL<<20)
+
+/*Define vmm log buffer*/
+#define KVM_VMM_LOG_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define VMM_LOG_SIZE  (4UL<<20)
 
 /*
  * Define vm_buffer, used by PAL Services, base address.
  * Note: vmbuffer is in the VMM-BLOCK, the size must be < 8M
  */
-#define KVM_VM_BUFFER_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define KVM_VM_BUFFER_BASE (KVM_VMM_LOG_BASE + VMM_LOG_SIZE)
 #define KVM_VM_BUFFER_SIZE (8UL<<20)
 
 /*Define Virtual machine data layout.*/
@@ -524,4 +528,13 @@ void kvm_sal_emul(struct kvm_vcpu *vcpu);
 
 static inline void kvm_inject_nmi(struct kvm_vcpu *vcpu) {}
 
+#define BYTES_PER_LOG 1024
+
+struct kvm_vmm_log {
+	spinlock_t log_lock;
+	unsigned long w_pointer;
+	unsigned long r_pointer;
+	char log_slot[VMM_LOG_SIZE/BYTES_PER_LOG][BYTES_PER_LOG - 1];
+};
+
 #endif
diff --git a/arch/ia64/kvm/Makefile b/arch/ia64/kvm/Makefile
index bf22fb9..0d17d84 100644
--- a/arch/ia64/kvm/Makefile
+++ b/arch/ia64/kvm/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_KVM) += kvm.o
 FORCE : $(obj)/$(offsets-file)
 EXTRA_CFLAGS_vcpu.o += -mfixed-range=f2-f5,f12-f127
 kvm-intel-objs = vmm.o vmm_ivt.o trampoline.o vcpu.o optvfault.o mmio.o \
-	vtlb.o process.o
+	vtlb.o process.o kvm_lib.o
 #Add link memcpy and memset to avoid possible structure assignment error
 kvm-intel-objs += memcpy.o memset.o
 obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8a3b5fc..17f2cd7 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -50,6 +50,7 @@ static unsigned long kvm_vsa_base;
 static unsigned long kvm_vm_buffer;
 static unsigned long kvm_vm_buffer_size;
 unsigned long kvm_vmm_gp;
+struct kvm_vmm_log *vmm_log;
 
 static long vp_env_info;
 
@@ -496,6 +497,21 @@ static uint32_t kvm_get_exit_reason(struct kvm_vcpu *vcpu)
 	return p_exit_data->exit_reason;
 }
 
+static void vcpu_print_vmm_log(void)
+{
+	unsigned int slot;
+
+	spin_lock(&vmm_log->log_lock);
+
+	while (vmm_log->r_pointer < vmm_log->w_pointer) {
+		slot = vmm_log->r_pointer % (VMM_LOG_SIZE/BYTES_PER_LOG);
+		printk("%s", vmm_log->log_slot[slot]);
+		vmm_log->r_pointer++;
+	}
+
+	spin_unlock(&vmm_log->log_lock);
+}
+
 /*
  * The guest has exited.  See if we can fix it or if we need userspace
  * assistance.
@@ -505,6 +521,9 @@ static int kvm_handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	u32 exit_reason = kvm_get_exit_reason(vcpu);
 	vcpu->arch.last_exit = exit_reason;
 
+	/*vcpu on behalf of VMM to print vmm's kernel log */
+	vcpu_print_vmm_log();
+
 	if (exit_reason < kvm_vti_max_exit_handlers
 			&& kvm_vti_exit_handlers[exit_reason])
 		return kvm_vti_exit_handlers[exit_reason](vcpu, kvm_run);
@@ -1011,6 +1030,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 
 static int kvm_alloc_vmm_area(void)
 {
+
 	if (!kvm_vmm_base && (kvm_vm_buffer_size < KVM_VM_BUFFER_SIZE)) {
 		kvm_vmm_base = __get_free_pages(GFP_KERNEL,
 				get_order(KVM_VMM_SIZE));
@@ -1018,7 +1038,10 @@ static int kvm_alloc_vmm_area(void)
 			return -ENOMEM;
 
 		memset((void *)kvm_vmm_base, 0, KVM_VMM_SIZE);
-		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE;
+		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE + VMM_LOG_SIZE;
+		vmm_log = (struct kvm_vmm_log *)(kvm_vmm_base +
+					(KVM_VMM_LOG_BASE - KVM_VMM_BASE));
+		spin_lock_init(&vmm_log->log_lock);
 
 		printk(KERN_DEBUG"kvm:VMM's Base Addr:0x%lx, vm_buffer:0x%lx\n",
 				kvm_vmm_base, kvm_vm_buffer);
diff --git a/arch/ia64/kvm/kvm_lib.c b/arch/ia64/kvm/kvm_lib.c
new file mode 100644
index 0000000..4c43efe
--- /dev/null
+++ b/arch/ia64/kvm/kvm_lib.c
@@ -0,0 +1,13 @@
+
+/*
+ * vsprintf.c: Let kvm-intel module has the ability to use print functions.
+ *	Just include kernel's library, and disable symbols export.
+ * 	Copyright (C) 2008, Intel Corporation.
+ *  	Xiantao Zhang  (xiantao.zhang@intel.com)
+ *
+ */
+
+#undef CONFIG_MODULES
+
+#include "../../../lib/vsprintf.c"
+#include "../../../lib/ctype.c"
diff --git a/arch/ia64/kvm/vmm.c b/arch/ia64/kvm/vmm.c
index 2275bf4..e4c3597 100644
--- a/arch/ia64/kvm/vmm.c
+++ b/arch/ia64/kvm/vmm.c
@@ -62,5 +62,26 @@ void vmm_spin_unlock(spinlock_t *lock)
 {
 	_vmm_raw_spin_unlock(lock);
 }
+
+asmlinkage int printk(const char *fmt, ...)
+{
+	struct kvm_vmm_log *vmm_log = (struct kvm_vmm_log *)KVM_VMM_LOG_BASE;
+	unsigned int slot;
+	va_list args;
+	int r;
+
+	vmm_spin_lock(&vmm_log->log_lock);
+	slot = vmm_log->w_pointer % (VMM_LOG_SIZE/BYTES_PER_LOG);
+	memset(vmm_log->log_slot[slot], 0, BYTES_PER_LOG - 1);
+	va_start(args, fmt);
+	r = vsnprintf(vmm_log->log_slot[slot], BYTES_PER_LOG - 1, fmt, args);
+	va_end(args);
+	vmm_log->w_pointer++;
+	wmb();
+	vmm_spin_unlock(&vmm_log->log_lock);
+
+	return r;
+}
+
 module_init(kvm_vmm_init)
 module_exit(kvm_vmm_exit)
-- 
1.5.1

Re: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

[Avi Kivity]

Zhang, Xiantao wrote:
> Resend. 
> Avi, 
> 	Please help to apply it, Thanks! 
> Xiantao
>
>   

(sorry for the late review)

> Since this module will be reloated to an isolated address
> space from host side, so kvm-intel can't call printk of host
> kernel. This patch implements the printk function for kvm-intel
> module, so it doesn't need to suffer no-printk pains.
>
>  
> +#define BYTES_PER_LOG 1024
> +
> +struct kvm_vmm_log {
> +	spinlock_t log_lock;
> +	unsigned long w_pointer;
> +	unsigned long r_pointer;
> +	char log_slot[VMM_LOG_SIZE/BYTES_PER_LOG][BYTES_PER_LOG - 1];
> +};
> +
>   

1024 bytes per line?  that's wasteful.

Why not variable size records?

>  
> +static void vcpu_print_vmm_log(void)
> +{
> +	unsigned int slot;
> +
> +	spin_lock(&vmm_log->log_lock);
>   

You're going to impact scalability with this.  Are per-vcpu logs workable?

> @@ -1011,6 +1030,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu
> *vcpu,
>  
>  static int kvm_alloc_vmm_area(void)
>  {
> +
>   

blank line?

> diff --git a/arch/ia64/kvm/kvm_lib.c b/arch/ia64/kvm/kvm_lib.c
> new file mode 100644
> index 0000000..4c43efe
> --- /dev/null
> +++ b/arch/ia64/kvm/kvm_lib.c
> @@ -0,0 +1,13 @@
> +
> +/*
> + * vsprintf.c: Let kvm-intel module has the ability to use print
> functions.
> + *	Just include kernel's library, and disable symbols export.
> + * 	Copyright (C) 2008, Intel Corporation.
> + *  	Xiantao Zhang  (xiantao.zhang@intel.com)
> + *
> + */
> +
> +#undef CONFIG_MODULES
> +
> +#include "../../../lib/vsprintf.c"
> +#include "../../../lib/ctype.c"
>   

I suspect this will start breaking when people start using the new 
printk("%pBLAH") functionality, which will require linking additional files.

Is it possible to pass the format string and the arguments in the log 
record instead, and do the printk() in the kernel?

I can't think of a way on x86, but maybe ia64 varargs are different.

(worst case you can limit the number of arguments and just copy a bunch 
of stack).

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

RE: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

[Zhang, Xiantao]

Avi Kivity wrote:
> Zhang, Xiantao wrote:
>> Resend.
>> Avi,
>> 	Please help to apply it, Thanks!
>> Xiantao
>> 
>> 
> 
> (sorry for the late review)
> 
>> Since this module will be reloated to an isolated address
>> space from host side, so kvm-intel can't call printk of host
>> kernel. This patch implements the printk function for kvm-intel
>> module, so it doesn't need to suffer no-printk pains.
>> 
>> 
>> +#define BYTES_PER_LOG 1024
>> +
>> +struct kvm_vmm_log {
>> +	spinlock_t log_lock;
>> +	unsigned long w_pointer;
>> +	unsigned long r_pointer;
>> +	char log_slot[VMM_LOG_SIZE/BYTES_PER_LOG][BYTES_PER_LOG - 1];
+};
>> +
>> 
> 
> 1024 bytes per line?  that's wasteful.
> 
> Why not variable size records?
It is a ring buffer, and will be recycled one by one. Anyway variable
size should be better, but needs more logic. 

>> 
>> +static void vcpu_print_vmm_log(void)
>> +{
>> +	unsigned int slot;
>> +
>> +	spin_lock(&vmm_log->log_lock);
>> 
> 
> You're going to impact scalability with this.  Are per-vcpu logs
> workable? 

OK, I will change it to per-vcpu style to avoid this possible
scalability issue. 

>> @@ -1011,6 +1030,7 @@ int kvm_arch_vcpu_ioctl_translate(struct
>> kvm_vcpu *vcpu, 
>> 
>>  static int kvm_alloc_vmm_area(void)
>>  {
>> +
>> 
> 
> blank line?
Should be removed. 
>> diff --git a/arch/ia64/kvm/kvm_lib.c b/arch/ia64/kvm/kvm_lib.c
>> new file mode 100644
>> index 0000000..4c43efe
>> --- /dev/null
>> +++ b/arch/ia64/kvm/kvm_lib.c
>> @@ -0,0 +1,13 @@
>> +
>> +/*
>> + * vsprintf.c: Let kvm-intel module has the ability to use print
>> functions. + *	Just include kernel's library, and disable
symbols
>> export. + * 	Copyright (C) 2008, Intel Corporation.
>> + *  	Xiantao Zhang  (xiantao.zhang@intel.com)
>> + *
>> + */
>> +
>> +#undef CONFIG_MODULES
>> +
>> +#include "../../../lib/vsprintf.c"
>> +#include "../../../lib/ctype.c"
>> 
> 
> I suspect this will start breaking when people start using the new
> printk("%pBLAH") functionality, which will require linking additional
> files. 

 If the format string works with vsnprintf, it should be covered. 

> Is it possible to pass the format string and the arguments in the log
> record instead, and do the printk() in the kernel?


> I can't think of a way on x86, but maybe ia64 varargs are different.
> 
> (worst case you can limit the number of arguments and just copy a
> bunch  of stack).

That maybe infeasible, since some args may be transferred by pointer,
and this pointer can't be reached in host side.
Xiantao

Re: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

[Avi Kivity]

Zhang, Xiantao wrote:
>
>>> +static void vcpu_print_vmm_log(void)
>>> +{
>>> +	unsigned int slot;
>>> +
>>> +	spin_lock(&vmm_log->log_lock);
>>>
>>>       
>> You're going to impact scalability with this.  Are per-vcpu logs
>> workable? 
>>     
>
> OK, I will change it to per-vcpu style to avoid this possible
> scalability issue. 
>
>   

Actually, per-vcpu logs have a deficiency where log lines become unordered.

So I suggest a per-vcpu flag that says "there may be something in the
log", but keep a single log buffer.  Since printk()s are rare (and
slow), it's enough that we make the case where the log is empty fast.


>> I suspect this will start breaking when people start using the new
>> printk("%pBLAH") functionality, which will require linking additional
>> files. 
>>     
>
>  If the format string works with vsnprintf, it should be covered. 
>
>   

vsnprintf() may start to be linked with other stuff.  Well, we'll deal
with that when it happens.

>> I can't think of a way on x86, but maybe ia64 varargs are different.
>>
>> (worst case you can limit the number of arguments and just copy a
>> bunch  of stack).
>>     
>
> That maybe infeasible, since some args may be transferred by pointer,
> and this pointer can't be reached in host side.
>   

Okay.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

RE: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

[Zhang, Xiantao]

[-- Attachment #1: Type: text/plain, Size: 8279 bytes --]

Hi, Avi 
Based on your comments. I worked out the new patch. Now I use the
vmm_has_log field of vcpu to indicate whether vmm has log to print. If
this flag is not set, it doesn't need to acquire the spin_lock to avoid
possible scalability issue. 
Xiantao

>From 688296c4a4784704e48b16dea9580abc271fa497 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Thu, 11 Sep 2008 17:04:22 +0800
Subject: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

Since this module will be reloated to an isolated address
space from host side, so kvm-intel can't call printk of host
kernel. This patch implements the printk function for kvm-intel
module, so it doesn't need to suffer no-printk pains.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 arch/ia64/include/asm/kvm_host.h |   19 +++++++++++++++++--
 arch/ia64/kvm/Makefile           |    2 +-
 arch/ia64/kvm/kvm-ia64.c         |   25 ++++++++++++++++++++++++-
 arch/ia64/kvm/kvm_lib.c          |   13 +++++++++++++
 arch/ia64/kvm/vmm.c              |   23 +++++++++++++++++++++++
 5 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 arch/ia64/kvm/kvm_lib.c

diff --git a/arch/ia64/include/asm/kvm_host.h
b/arch/ia64/include/asm/kvm_host.h
index 1efe513..b71aba0 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -55,13 +55,17 @@
 #define KVM_VMM_SIZE (16UL<<20)
 #define KVM_VMM_SHIFT 24
 #define KVM_VMM_BASE 0xD000000000000000UL
-#define VMM_SIZE (8UL<<20)
+#define VMM_SIZE (4UL<<20)
+
+/*Define vmm log buffer*/
+#define KVM_VMM_LOG_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define VMM_LOG_SIZE  (4UL<<20)
 
 /*
  * Define vm_buffer, used by PAL Services, base address.
  * Note: vmbuffer is in the VMM-BLOCK, the size must be < 8M
  */
-#define KVM_VM_BUFFER_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define KVM_VM_BUFFER_BASE (KVM_VMM_LOG_BASE + VMM_LOG_SIZE)
 #define KVM_VM_BUFFER_SIZE (8UL<<20)
 
 /*Define Virtual machine data layout.*/
@@ -324,6 +328,8 @@ struct kvm_vcpu_arch {
 #define KVM_MP_STATE_INIT_RECEIVED     2
 #define KVM_MP_STATE_HALTED            3
 	int mp_state;
+	/*Flag to indicate vmm log.*/
+	int vmm_has_log;
 
 #define MAX_PTC_G_NUM			3
 	int ptc_g_count;
@@ -524,4 +530,13 @@ void kvm_sal_emul(struct kvm_vcpu *vcpu);
 
 static inline void kvm_inject_nmi(struct kvm_vcpu *vcpu) {}
 
+#define BYTES_PER_LOG 256
+
+struct kvm_vmm_log {
+	spinlock_t log_lock;
+	unsigned long w_pointer;
+	unsigned long r_pointer;
+	char log_slot[VMM_LOG_SIZE/BYTES_PER_LOG][BYTES_PER_LOG - 1];
+};
+
 #endif
diff --git a/arch/ia64/kvm/Makefile b/arch/ia64/kvm/Makefile
index bf22fb9..0d17d84 100644
--- a/arch/ia64/kvm/Makefile
+++ b/arch/ia64/kvm/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_KVM) += kvm.o
 FORCE : $(obj)/$(offsets-file)
 EXTRA_CFLAGS_vcpu.o += -mfixed-range=f2-f5,f12-f127
 kvm-intel-objs = vmm.o vmm_ivt.o trampoline.o vcpu.o optvfault.o mmio.o
\
-	vtlb.o process.o
+	vtlb.o process.o kvm_lib.o
 #Add link memcpy and memset to avoid possible structure assignment
error
 kvm-intel-objs += memcpy.o memset.o
 obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8a3b5fc..b92f6d1 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -50,6 +50,7 @@ static unsigned long kvm_vsa_base;
 static unsigned long kvm_vm_buffer;
 static unsigned long kvm_vm_buffer_size;
 unsigned long kvm_vmm_gp;
+struct kvm_vmm_log *vmm_log;
 
 static long vp_env_info;
 
@@ -496,6 +497,21 @@ static uint32_t kvm_get_exit_reason(struct kvm_vcpu
*vcpu)
 	return p_exit_data->exit_reason;
 }
 
+static void vcpu_print_vmm_log(struct kvm_vcpu *vcpu)
+{
+	unsigned int slot;
+
+	if (vcpu->arch.vmm_has_log) {
+		spin_lock(&vmm_log->log_lock);
+		while (vmm_log->r_pointer < vmm_log->w_pointer) {
+			slot = vmm_log->r_pointer %
(VMM_LOG_SIZE/BYTES_PER_LOG);
+			printk("%s", vmm_log->log_slot[slot]);
+			vmm_log->r_pointer++;
+		}
+		vcpu->arch.vmm_has_log = 0;
+		spin_unlock(&vmm_log->log_lock);
+	}
+}
 /*
  * The guest has exited.  See if we can fix it or if we need userspace
  * assistance.
@@ -505,6 +521,9 @@ static int kvm_handle_exit(struct kvm_run *kvm_run,
struct kvm_vcpu *vcpu)
 	u32 exit_reason = kvm_get_exit_reason(vcpu);
 	vcpu->arch.last_exit = exit_reason;
 
+	/*vcpu on behalf of VMM to print vmm's kernel log */
+	vcpu_print_vmm_log(vcpu);
+
 	if (exit_reason < kvm_vti_max_exit_handlers
 			&& kvm_vti_exit_handlers[exit_reason])
 		return kvm_vti_exit_handlers[exit_reason](vcpu,
kvm_run);
@@ -1011,6 +1030,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu
*vcpu,
 
 static int kvm_alloc_vmm_area(void)
 {
+
 	if (!kvm_vmm_base && (kvm_vm_buffer_size < KVM_VM_BUFFER_SIZE))
{
 		kvm_vmm_base = __get_free_pages(GFP_KERNEL,
 				get_order(KVM_VMM_SIZE));
@@ -1018,7 +1038,10 @@ static int kvm_alloc_vmm_area(void)
 			return -ENOMEM;
 
 		memset((void *)kvm_vmm_base, 0, KVM_VMM_SIZE);
-		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE;
+		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE + VMM_LOG_SIZE;
+		vmm_log = (struct kvm_vmm_log *)(kvm_vmm_base +
+					(KVM_VMM_LOG_BASE -
KVM_VMM_BASE));
+		spin_lock_init(&vmm_log->log_lock);
 
 		printk(KERN_DEBUG"kvm:VMM's Base Addr:0x%lx,
vm_buffer:0x%lx\n",
 				kvm_vmm_base, kvm_vm_buffer);
diff --git a/arch/ia64/kvm/kvm_lib.c b/arch/ia64/kvm/kvm_lib.c
new file mode 100644
index 0000000..4c43efe
--- /dev/null
+++ b/arch/ia64/kvm/kvm_lib.c
@@ -0,0 +1,13 @@
+
+/*
+ * vsprintf.c: Let kvm-intel module has the ability to use print
functions.
+ *	Just include kernel's library, and disable symbols export.
+ * 	Copyright (C) 2008, Intel Corporation.
+ *  	Xiantao Zhang  (xiantao.zhang@intel.com)
+ *
+ */
+
+#undef CONFIG_MODULES
+
+#include "../../../lib/vsprintf.c"
+#include "../../../lib/ctype.c"
diff --git a/arch/ia64/kvm/vmm.c b/arch/ia64/kvm/vmm.c
index 2275bf4..c8b3c90 100644
--- a/arch/ia64/kvm/vmm.c
+++ b/arch/ia64/kvm/vmm.c
@@ -62,5 +62,28 @@ void vmm_spin_unlock(spinlock_t *lock)
 {
 	_vmm_raw_spin_unlock(lock);
 }
+
+asmlinkage int printk(const char *fmt, ...)
+{
+	struct kvm_vmm_log *vmm_log = (struct kvm_vmm_log
*)KVM_VMM_LOG_BASE;
+	struct kvm_vcpu *vcpu = current_vcpu;
+	unsigned int slot;
+	va_list args;
+	int r;
+
+	vmm_spin_lock(&vmm_log->log_lock);
+	slot = vmm_log->w_pointer % (VMM_LOG_SIZE/BYTES_PER_LOG);
+	memset(vmm_log->log_slot[slot], 0, BYTES_PER_LOG - 1);
+	va_start(args, fmt);
+	r = vsnprintf(vmm_log->log_slot[slot], BYTES_PER_LOG - 1, fmt,
args);
+	va_end(args);
+	vcpu->arch.vmm_has_log = 1;
+	vmm_log->w_pointer++;
+	wmb();
+	vmm_spin_unlock(&vmm_log->log_lock);
+
+	return r;
+}
+
 module_init(kvm_vmm_init)
 module_exit(kvm_vmm_exit)
-- 
1.5.1

Avi Kivity wrote:
> Zhang, Xiantao wrote:
>> 
>>>> +static void vcpu_print_vmm_log(void)
>>>> +{
>>>> +	unsigned int slot;
>>>> +
>>>> +	spin_lock(&vmm_log->log_lock);
>>>> 
>>>> 
>>> You're going to impact scalability with this.  Are per-vcpu logs
>>> workable? 
>>> 
>> 
>> OK, I will change it to per-vcpu style to avoid this possible
>> scalability issue. 
>> 
>> 
> 
> Actually, per-vcpu logs have a deficiency where log lines become
> unordered. 
> 
> So I suggest a per-vcpu flag that says "there may be something in the
> log", but keep a single log buffer.  Since printk()s are rare (and
> slow), it's enough that we make the case where the log is empty fast.
> 
> 
>>> I suspect this will start breaking when people start using the new
>>> printk("%pBLAH") functionality, which will require linking
>>> additional files. 
>>> 
>> 
>>  If the format string works with vsnprintf, it should be covered.
>> 
>> 
> 
> vsnprintf() may start to be linked with other stuff.  Well, we'll deal
> with that when it happens.
> 
>>> I can't think of a way on x86, but maybe ia64 varargs are different.
>>> 
>>> (worst case you can limit the number of arguments and just copy a
>>> bunch  of stack). 
>>> 
>> 
>> That maybe infeasible, since some args may be transferred by pointer,
>> and this pointer can't be reached in host side.
>> 
> 
> Okay.


[-- Attachment #2: 0001-kvm-ia64-Add-printk-support-for-kvm-intel-modules.patch --]
[-- Type: application/octet-stream, Size: 6405 bytes --]

From 688296c4a4784704e48b16dea9580abc271fa497 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Thu, 11 Sep 2008 17:04:22 +0800
Subject: [PATCH] kvm/ia64: Add printk support for kvm-intel modules.

Since this module will be reloated to an isolated address
space from host side, so kvm-intel can't call printk of host
kernel. This patch implements the printk function for kvm-intel
module, so it doesn't need to suffer no-printk pains.

Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 arch/ia64/include/asm/kvm_host.h |   19 +++++++++++++++++--
 arch/ia64/kvm/Makefile           |    2 +-
 arch/ia64/kvm/kvm-ia64.c         |   25 ++++++++++++++++++++++++-
 arch/ia64/kvm/kvm_lib.c          |   13 +++++++++++++
 arch/ia64/kvm/vmm.c              |   23 +++++++++++++++++++++++
 5 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 arch/ia64/kvm/kvm_lib.c

diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index 1efe513..b71aba0 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -55,13 +55,17 @@
 #define KVM_VMM_SIZE (16UL<<20)
 #define KVM_VMM_SHIFT 24
 #define KVM_VMM_BASE 0xD000000000000000UL
-#define VMM_SIZE (8UL<<20)
+#define VMM_SIZE (4UL<<20)
+
+/*Define vmm log buffer*/
+#define KVM_VMM_LOG_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define VMM_LOG_SIZE  (4UL<<20)
 
 /*
  * Define vm_buffer, used by PAL Services, base address.
  * Note: vmbuffer is in the VMM-BLOCK, the size must be < 8M
  */
-#define KVM_VM_BUFFER_BASE (KVM_VMM_BASE + VMM_SIZE)
+#define KVM_VM_BUFFER_BASE (KVM_VMM_LOG_BASE + VMM_LOG_SIZE)
 #define KVM_VM_BUFFER_SIZE (8UL<<20)
 
 /*Define Virtual machine data layout.*/
@@ -324,6 +328,8 @@ struct kvm_vcpu_arch {
 #define KVM_MP_STATE_INIT_RECEIVED     2
 #define KVM_MP_STATE_HALTED            3
 	int mp_state;
+	/*Flag to indicate vmm log.*/
+	int vmm_has_log;
 
 #define MAX_PTC_G_NUM			3
 	int ptc_g_count;
@@ -524,4 +530,13 @@ void kvm_sal_emul(struct kvm_vcpu *vcpu);
 
 static inline void kvm_inject_nmi(struct kvm_vcpu *vcpu) {}
 
+#define BYTES_PER_LOG 256
+
+struct kvm_vmm_log {
+	spinlock_t log_lock;
+	unsigned long w_pointer;
+	unsigned long r_pointer;
+	char log_slot[VMM_LOG_SIZE/BYTES_PER_LOG][BYTES_PER_LOG - 1];
+};
+
 #endif
diff --git a/arch/ia64/kvm/Makefile b/arch/ia64/kvm/Makefile
index bf22fb9..0d17d84 100644
--- a/arch/ia64/kvm/Makefile
+++ b/arch/ia64/kvm/Makefile
@@ -52,7 +52,7 @@ obj-$(CONFIG_KVM) += kvm.o
 FORCE : $(obj)/$(offsets-file)
 EXTRA_CFLAGS_vcpu.o += -mfixed-range=f2-f5,f12-f127
 kvm-intel-objs = vmm.o vmm_ivt.o trampoline.o vcpu.o optvfault.o mmio.o \
-	vtlb.o process.o
+	vtlb.o process.o kvm_lib.o
 #Add link memcpy and memset to avoid possible structure assignment error
 kvm-intel-objs += memcpy.o memset.o
 obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8a3b5fc..b92f6d1 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -50,6 +50,7 @@ static unsigned long kvm_vsa_base;
 static unsigned long kvm_vm_buffer;
 static unsigned long kvm_vm_buffer_size;
 unsigned long kvm_vmm_gp;
+struct kvm_vmm_log *vmm_log;
 
 static long vp_env_info;
 
@@ -496,6 +497,21 @@ static uint32_t kvm_get_exit_reason(struct kvm_vcpu *vcpu)
 	return p_exit_data->exit_reason;
 }
 
+static void vcpu_print_vmm_log(struct kvm_vcpu *vcpu)
+{
+	unsigned int slot;
+
+	if (vcpu->arch.vmm_has_log) {
+		spin_lock(&vmm_log->log_lock);
+		while (vmm_log->r_pointer < vmm_log->w_pointer) {
+			slot = vmm_log->r_pointer % (VMM_LOG_SIZE/BYTES_PER_LOG);
+			printk("%s", vmm_log->log_slot[slot]);
+			vmm_log->r_pointer++;
+		}
+		vcpu->arch.vmm_has_log = 0;
+		spin_unlock(&vmm_log->log_lock);
+	}
+}
 /*
  * The guest has exited.  See if we can fix it or if we need userspace
  * assistance.
@@ -505,6 +521,9 @@ static int kvm_handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	u32 exit_reason = kvm_get_exit_reason(vcpu);
 	vcpu->arch.last_exit = exit_reason;
 
+	/*vcpu on behalf of VMM to print vmm's kernel log */
+	vcpu_print_vmm_log(vcpu);
+
 	if (exit_reason < kvm_vti_max_exit_handlers
 			&& kvm_vti_exit_handlers[exit_reason])
 		return kvm_vti_exit_handlers[exit_reason](vcpu, kvm_run);
@@ -1011,6 +1030,7 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
 
 static int kvm_alloc_vmm_area(void)
 {
+
 	if (!kvm_vmm_base && (kvm_vm_buffer_size < KVM_VM_BUFFER_SIZE)) {
 		kvm_vmm_base = __get_free_pages(GFP_KERNEL,
 				get_order(KVM_VMM_SIZE));
@@ -1018,7 +1038,10 @@ static int kvm_alloc_vmm_area(void)
 			return -ENOMEM;
 
 		memset((void *)kvm_vmm_base, 0, KVM_VMM_SIZE);
-		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE;
+		kvm_vm_buffer = kvm_vmm_base + VMM_SIZE + VMM_LOG_SIZE;
+		vmm_log = (struct kvm_vmm_log *)(kvm_vmm_base +
+					(KVM_VMM_LOG_BASE - KVM_VMM_BASE));
+		spin_lock_init(&vmm_log->log_lock);
 
 		printk(KERN_DEBUG"kvm:VMM's Base Addr:0x%lx, vm_buffer:0x%lx\n",
 				kvm_vmm_base, kvm_vm_buffer);
diff --git a/arch/ia64/kvm/kvm_lib.c b/arch/ia64/kvm/kvm_lib.c
new file mode 100644
index 0000000..4c43efe
--- /dev/null
+++ b/arch/ia64/kvm/kvm_lib.c
@@ -0,0 +1,13 @@
+
+/*
+ * vsprintf.c: Let kvm-intel module has the ability to use print functions.
+ *	Just include kernel's library, and disable symbols export.
+ * 	Copyright (C) 2008, Intel Corporation.
+ *  	Xiantao Zhang  (xiantao.zhang@intel.com)
+ *
+ */
+
+#undef CONFIG_MODULES
+
+#include "../../../lib/vsprintf.c"
+#include "../../../lib/ctype.c"
diff --git a/arch/ia64/kvm/vmm.c b/arch/ia64/kvm/vmm.c
index 2275bf4..c8b3c90 100644
--- a/arch/ia64/kvm/vmm.c
+++ b/arch/ia64/kvm/vmm.c
@@ -62,5 +62,28 @@ void vmm_spin_unlock(spinlock_t *lock)
 {
 	_vmm_raw_spin_unlock(lock);
 }
+
+asmlinkage int printk(const char *fmt, ...)
+{
+	struct kvm_vmm_log *vmm_log = (struct kvm_vmm_log *)KVM_VMM_LOG_BASE;
+	struct kvm_vcpu *vcpu = current_vcpu;
+	unsigned int slot;
+	va_list args;
+	int r;
+
+	vmm_spin_lock(&vmm_log->log_lock);
+	slot = vmm_log->w_pointer % (VMM_LOG_SIZE/BYTES_PER_LOG);
+	memset(vmm_log->log_slot[slot], 0, BYTES_PER_LOG - 1);
+	va_start(args, fmt);
+	r = vsnprintf(vmm_log->log_slot[slot], BYTES_PER_LOG - 1, fmt, args);
+	va_end(args);
+	vcpu->arch.vmm_has_log = 1;
+	vmm_log->w_pointer++;
+	wmb();
+	vmm_spin_unlock(&vmm_log->log_lock);
+
+	return r;
+}
+
 module_init(kvm_vmm_init)
 module_exit(kvm_vmm_exit)
-- 
1.5.1