[PATCH] Xen PV-on-HVM guest support (v2)

[PATCH] Xen PV-on-HVM guest support (v2)

[Ed Swierk]

Support for Xen PV-on-HVM guests can be implemented almost entirely in
userspace, except for handling one annoying MSR that maps a Xen
hypercall blob into guest address space.

A generic mechanism to delegate MSR writes to userspace seems overkill
and risks encouraging similar MSR abuse in the future.  Thus this patch
adds special support for the Xen HVM MSR.

I implemented a new ioctl, KVM_XEN_HVM_CONFIG, that lets userspace tell
KVM which MSR the guest will write to, as well as the starting address
and size of the hypercall blobs (one each for 32-bit and 64-bit) that
userspace has loaded from files.  When the guest writes to the MSR, KVM
copies one page of the blob from userspace to the guest.

I've tested this patch with a hacked-up version of Gerd's userspace
code, booting a number of guests (CentOS 5.3 i386 and x86_64, and
FreeBSD 8.0-RC1 amd64) and exercising PV network and block devices.

v2: fix ioctl struct padding; renumber CAP and ioctl constants; check
kvm_write_guest() return value; change printks to KERN_DEBUG (I think
they're worth keeping for debugging userspace)

Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>

---
Index: kvm-kmod/include/asm-x86/kvm.h
===================================================================
--- kvm-kmod.orig/include/asm-x86/kvm.h
+++ kvm-kmod/include/asm-x86/kvm.h
@@ -59,6 +59,7 @@
 #define __KVM_HAVE_MSIX
 #define __KVM_HAVE_MCE
 #define __KVM_HAVE_PIT_STATE2
+#define __KVM_HAVE_XEN_HVM
 
 /* Architectural interrupt line count. */
 #define KVM_NR_INTERRUPTS 256
Index: kvm-kmod/include/linux/kvm.h
===================================================================
--- kvm-kmod.orig/include/linux/kvm.h
+++ kvm-kmod/include/linux/kvm.h
@@ -476,6 +476,9 @@ struct kvm_ioeventfd {
 #endif
 #define KVM_CAP_IOEVENTFD 36
 #define KVM_CAP_SET_IDENTITY_MAP_ADDR 37
+#ifdef __KVM_HAVE_XEN_HVM
+#define KVM_CAP_XEN_HVM 38
+#endif
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -528,6 +531,15 @@ struct kvm_x86_mce {
 };
 #endif
 
+#ifdef KVM_CAP_XEN_HVM
+struct kvm_xen_hvm_config {
+	__u32 msr;
+	__u8 pad[2];
+	__u8 blob_size[2];
+	__u64 blob_addr[2];
+};
+#endif
+
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
 
 struct kvm_irqfd {
@@ -586,6 +598,7 @@ struct kvm_irqfd {
 #define KVM_CREATE_PIT2		   _IOW(KVMIO, 0x77, struct kvm_pit_config)
 #define KVM_SET_BOOT_CPU_ID        _IO(KVMIO, 0x78)
 #define KVM_IOEVENTFD             _IOW(KVMIO, 0x79, struct kvm_ioeventfd)
+#define KVM_XEN_HVM_CONFIG        _IOW(KVMIO, 0x7a, struct kvm_xen_hvm_config)
 
 /*
  * ioctls for vcpu fds
Index: kvm-kmod/include/linux/kvm_host.h
===================================================================
--- kvm-kmod.orig/include/linux/kvm_host.h
+++ kvm-kmod/include/linux/kvm_host.h
@@ -236,6 +236,10 @@ struct kvm {
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;
 #endif
+
+#ifdef KVM_CAP_XEN_HVM
+	struct kvm_xen_hvm_config xen_hvm_config;
+#endif
 };
 
 /* The guest did something we don't support. */
Index: kvm-kmod/x86/x86.c
===================================================================
--- kvm-kmod.orig/x86/x86.c
+++ kvm-kmod/x86/x86.c
@@ -875,6 +875,35 @@ static int set_msr_mce(struct kvm_vcpu *
 	return 0;
 }
 
+#ifdef KVM_CAP_XEN_HVM
+static int xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
+{
+	int blob = !!(vcpu->arch.shadow_efer & EFER_LME);
+	u32 pnum = data & ~PAGE_MASK;
+	u64 paddr = data & PAGE_MASK;
+	u8 *page;
+	int r = 1;
+
+	if (pnum >= vcpu->kvm->xen_hvm_config.blob_size[blob])
+		goto out;
+	page = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!page)
+		goto out;
+	if (copy_from_user(page, (u8 *)vcpu->kvm->xen_hvm_config.blob_addr[blob]
+			   + pnum * PAGE_SIZE, PAGE_SIZE))
+		goto out_free;
+	if (kvm_write_guest(vcpu->kvm, paddr, page, PAGE_SIZE))
+		goto out_free;
+	printk(KERN_DEBUG "kvm: copied xen hvm blob %d page %d to 0x%llx\n",
+	       blob, pnum, paddr);
+	r = 0;
+out_free:
+	kfree(page);
+out:
+	return r;
+}
+#endif
+
 int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {
 	switch (msr) {
@@ -990,6 +1019,10 @@ int kvm_set_msr_common(struct kvm_vcpu *
 			"0x%x data 0x%llx\n", msr, data);
 		break;
 	default:
+#ifdef KVM_CAP_XEN_HVM
+		if (msr && (msr == vcpu->kvm->xen_hvm_config.msr))
+			return xen_hvm_config(vcpu, data);
+#endif
 		if (!ignore_msrs) {
 			pr_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
 				msr, data);
@@ -2453,6 +2486,17 @@ long kvm_arch_vm_ioctl(struct file *filp
 		r = 0;
 		break;
 	}
+#ifdef KVM_CAP_XEN_HVM
+	case KVM_XEN_HVM_CONFIG: {
+		r = -EFAULT;
+		if (copy_from_user(&kvm->xen_hvm_config, argp,
+				   sizeof(struct kvm_xen_hvm_config)))
+			goto out;
+		printk(KERN_DEBUG "kvm: configured xen hvm\n");
+		r = 0;
+		break;
+	}
+#endif
 	default:
 		;
 	}

Re: [PATCH] Xen PV-on-HVM guest support (v2)

[Avi Kivity]

On 10/15/2009 02:41 PM, Ed Swierk wrote:
> Support for Xen PV-on-HVM guests can be implemented almost entirely in
> userspace, except for handling one annoying MSR that maps a Xen
> hypercall blob into guest address space.
>
> A generic mechanism to delegate MSR writes to userspace seems overkill
> and risks encouraging similar MSR abuse in the future.  Thus this patch
> adds special support for the Xen HVM MSR.
>
> I implemented a new ioctl, KVM_XEN_HVM_CONFIG, that lets userspace tell
> KVM which MSR the guest will write to, as well as the starting address
> and size of the hypercall blobs (one each for 32-bit and 64-bit) that
> userspace has loaded from files.  When the guest writes to the MSR, KVM
> copies one page of the blob from userspace to the guest.
>
> I've tested this patch with a hacked-up version of Gerd's userspace
> code, booting a number of guests (CentOS 5.3 i386 and x86_64, and
> FreeBSD 8.0-RC1 amd64) and exercising PV network and block devices.
>
> v2: fix ioctl struct padding; renumber CAP and ioctl constants; check
> kvm_write_guest() return value; change printks to KERN_DEBUG (I think
> they're worth keeping for debugging userspace)
>
>
>
> +#ifdef KVM_CAP_XEN_HVM
> +struct kvm_xen_hvm_config {
> +	__u32 msr;
> +	__u8 pad[2];
> +	__u8 blob_size[2];
> +	__u64 blob_addr[2];
> +};
> +#endif
>    

Please change the arrays to separate variables (e.g. blob_size_32, 
blob_size_64), so readers don't have to guess the meaning.

Also, reserve a bunch of space at the end in case we need more hackery.

Is the msr number really variable?  Isn't it an ABI?

>    * ioctls for vcpu fds
> Index: kvm-kmod/include/linux/kvm_host.h
> ===================================================================
> --- kvm-kmod.orig/include/linux/kvm_host.h
> +++ kvm-kmod/include/linux/kvm_host.h
> @@ -236,6 +236,10 @@ struct kvm {
>   	unsigned long mmu_notifier_seq;
>   	long mmu_notifier_count;
>   #endif
> +
> +#ifdef KVM_CAP_XEN_HVM
> +	struct kvm_xen_hvm_config xen_hvm_config;
> +#endif
>   };
>    

struct kvm_arch is a better place for this.

>   /* The guest did something we don't support. */
> Index: kvm-kmod/x86/x86.c
> ===================================================================
> --- kvm-kmod.orig/x86/x86.c
> +++ kvm-kmod/x86/x86.c
> @@ -875,6 +875,35 @@ static int set_msr_mce(struct kvm_vcpu *
>   	return 0;
>   }
>
> +#ifdef KVM_CAP_XEN_HVM
>    

No need for the ifdef - it will always be defined for x86.

> +static int xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
> +{
> +	int blob = !!(vcpu->arch.shadow_efer&  EFER_LME);
>    

Can use is_long_mode() for this.

> +	u32 pnum = data&  ~PAGE_MASK;
> +	u64 paddr = data&  PAGE_MASK;
> +	u8 *page;
> +	int r = 1;
> +
> +	if (pnum>= vcpu->kvm->xen_hvm_config.blob_size[blob])
> +		goto out;
> +	page = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!page)
> +		goto out;
> +	if (copy_from_user(page, (u8 *)vcpu->kvm->xen_hvm_config.blob_addr[blob]
> +			   + pnum * PAGE_SIZE, PAGE_SIZE))
> +		goto out_free;
>    

We want to return -EFAULT here (but make sure the entire code path 
allows this).

> +	if (kvm_write_guest(vcpu->kvm, paddr, page, PAGE_SIZE))
> +		goto out_free;
> +	printk(KERN_DEBUG "kvm: copied xen hvm blob %d page %d to 0x%llx\n",
> +	       blob, pnum, paddr);
> +	r = 0;
> +out_free:
> +	kfree(page);
> +out:
> +	return r;
> +}
> +#endif
> +
>   int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   {
>   	switch (msr) {
> @@ -990,6 +1019,10 @@ int kvm_set_msr_common(struct kvm_vcpu *
>   			"0x%x data 0x%llx\n", msr, data);
>   		break;
>   	default:
> +#ifdef KVM_CAP_XEN_HVM
> +		if (msr&&  (msr == vcpu->kvm->xen_hvm_config.msr))
> +			return xen_hvm_config(vcpu, data);
> +#endif
>    

Again, can skip the ifdef.

>   		if (!ignore_msrs) {
>   			pr_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
>   				msr, data);
> @@ -2453,6 +2486,17 @@ long kvm_arch_vm_ioctl(struct file *filp
>   		r = 0;
>   		break;
>   	}
> +#ifdef KVM_CAP_XEN_HVM
> +	case KVM_XEN_HVM_CONFIG: {
> +		r = -EFAULT;
> +		if (copy_from_user(&kvm->xen_hvm_config, argp,
> +				   sizeof(struct kvm_xen_hvm_config)))
> +			goto out;
> +		printk(KERN_DEBUG "kvm: configured xen hvm\n");
> +		r = 0;
> +		break;
> +	}
> +#endif
>   	default:
>   		;
>   	}
>    

Do we need support for reading the msr?

IMO you can drop the debugging printk()s.  I don't see how they add much 
value.

Please submit the patch against a current kernel tree, not kvm-kmod.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Xen PV-on-HVM guest support (v2)

[Jan Kiszka]

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

Ed Swierk wrote:
> Support for Xen PV-on-HVM guests can be implemented almost entirely in
> userspace, except for handling one annoying MSR that maps a Xen
> hypercall blob into guest address space.
> 
> A generic mechanism to delegate MSR writes to userspace seems overkill
> and risks encouraging similar MSR abuse in the future.  Thus this patch
> adds special support for the Xen HVM MSR.
> 
> I implemented a new ioctl, KVM_XEN_HVM_CONFIG, that lets userspace tell
> KVM which MSR the guest will write to, as well as the starting address
> and size of the hypercall blobs (one each for 32-bit and 64-bit) that
> userspace has loaded from files.  When the guest writes to the MSR, KVM
> copies one page of the blob from userspace to the guest.
> 
> I've tested this patch with a hacked-up version of Gerd's userspace
> code, booting a number of guests (CentOS 5.3 i386 and x86_64, and
> FreeBSD 8.0-RC1 amd64) and exercising PV network and block devices.
> 
> v2: fix ioctl struct padding; renumber CAP and ioctl constants; check
> kvm_write_guest() return value; change printks to KERN_DEBUG (I think
> they're worth keeping for debugging userspace)

I disagree /wrt the print in the IOCTL path (missing configuration can
also be reported on access), and the guest triggered path at least
requires a pr_debug conversion. Looks fine to me otherwise.

Jan

> 
> Signed-off-by: Ed Swierk <eswierk@aristanetworks.com>
> 
> ---
> Index: kvm-kmod/include/asm-x86/kvm.h
> ===================================================================
> --- kvm-kmod.orig/include/asm-x86/kvm.h
> +++ kvm-kmod/include/asm-x86/kvm.h
> @@ -59,6 +59,7 @@
>  #define __KVM_HAVE_MSIX
>  #define __KVM_HAVE_MCE
>  #define __KVM_HAVE_PIT_STATE2
> +#define __KVM_HAVE_XEN_HVM
>  
>  /* Architectural interrupt line count. */
>  #define KVM_NR_INTERRUPTS 256
> Index: kvm-kmod/include/linux/kvm.h
> ===================================================================
> --- kvm-kmod.orig/include/linux/kvm.h
> +++ kvm-kmod/include/linux/kvm.h
> @@ -476,6 +476,9 @@ struct kvm_ioeventfd {
>  #endif
>  #define KVM_CAP_IOEVENTFD 36
>  #define KVM_CAP_SET_IDENTITY_MAP_ADDR 37
> +#ifdef __KVM_HAVE_XEN_HVM
> +#define KVM_CAP_XEN_HVM 38
> +#endif
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -528,6 +531,15 @@ struct kvm_x86_mce {
>  };
>  #endif
>  
> +#ifdef KVM_CAP_XEN_HVM
> +struct kvm_xen_hvm_config {
> +	__u32 msr;
> +	__u8 pad[2];
> +	__u8 blob_size[2];
> +	__u64 blob_addr[2];
> +};
> +#endif
> +
>  #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
>  
>  struct kvm_irqfd {
> @@ -586,6 +598,7 @@ struct kvm_irqfd {
>  #define KVM_CREATE_PIT2		   _IOW(KVMIO, 0x77, struct kvm_pit_config)
>  #define KVM_SET_BOOT_CPU_ID        _IO(KVMIO, 0x78)
>  #define KVM_IOEVENTFD             _IOW(KVMIO, 0x79, struct kvm_ioeventfd)
> +#define KVM_XEN_HVM_CONFIG        _IOW(KVMIO, 0x7a, struct kvm_xen_hvm_config)
>  
>  /*
>   * ioctls for vcpu fds
> Index: kvm-kmod/include/linux/kvm_host.h
> ===================================================================
> --- kvm-kmod.orig/include/linux/kvm_host.h
> +++ kvm-kmod/include/linux/kvm_host.h
> @@ -236,6 +236,10 @@ struct kvm {
>  	unsigned long mmu_notifier_seq;
>  	long mmu_notifier_count;
>  #endif
> +
> +#ifdef KVM_CAP_XEN_HVM
> +	struct kvm_xen_hvm_config xen_hvm_config;
> +#endif
>  };
>  
>  /* The guest did something we don't support. */
> Index: kvm-kmod/x86/x86.c
> ===================================================================
> --- kvm-kmod.orig/x86/x86.c
> +++ kvm-kmod/x86/x86.c
> @@ -875,6 +875,35 @@ static int set_msr_mce(struct kvm_vcpu *
>  	return 0;
>  }
>  
> +#ifdef KVM_CAP_XEN_HVM
> +static int xen_hvm_config(struct kvm_vcpu *vcpu, u64 data)
> +{
> +	int blob = !!(vcpu->arch.shadow_efer & EFER_LME);
> +	u32 pnum = data & ~PAGE_MASK;
> +	u64 paddr = data & PAGE_MASK;
> +	u8 *page;
> +	int r = 1;
> +
> +	if (pnum >= vcpu->kvm->xen_hvm_config.blob_size[blob])
> +		goto out;
> +	page = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!page)
> +		goto out;
> +	if (copy_from_user(page, (u8 *)vcpu->kvm->xen_hvm_config.blob_addr[blob]
> +			   + pnum * PAGE_SIZE, PAGE_SIZE))
> +		goto out_free;
> +	if (kvm_write_guest(vcpu->kvm, paddr, page, PAGE_SIZE))
> +		goto out_free;
> +	printk(KERN_DEBUG "kvm: copied xen hvm blob %d page %d to 0x%llx\n",
> +	       blob, pnum, paddr);
> +	r = 0;
> +out_free:
> +	kfree(page);
> +out:
> +	return r;
> +}
> +#endif
> +
>  int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  {
>  	switch (msr) {
> @@ -990,6 +1019,10 @@ int kvm_set_msr_common(struct kvm_vcpu *
>  			"0x%x data 0x%llx\n", msr, data);
>  		break;
>  	default:
> +#ifdef KVM_CAP_XEN_HVM
> +		if (msr && (msr == vcpu->kvm->xen_hvm_config.msr))
> +			return xen_hvm_config(vcpu, data);
> +#endif
>  		if (!ignore_msrs) {
>  			pr_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
>  				msr, data);
> @@ -2453,6 +2486,17 @@ long kvm_arch_vm_ioctl(struct file *filp
>  		r = 0;
>  		break;
>  	}
> +#ifdef KVM_CAP_XEN_HVM
> +	case KVM_XEN_HVM_CONFIG: {
> +		r = -EFAULT;
> +		if (copy_from_user(&kvm->xen_hvm_config, argp,
> +				   sizeof(struct kvm_xen_hvm_config)))
> +			goto out;
> +		printk(KERN_DEBUG "kvm: configured xen hvm\n");
> +		r = 0;
> +		break;
> +	}
> +#endif
>  	default:
>  		;
>  	}
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] Xen PV-on-HVM guest support (v2)

[Gerd Hoffmann]

   Hi,

> Is the msr number really variable? Isn't it an ABI?

Yes, it is variable.  The guests gets the msr number via cpuid ...

> Do we need support for reading the msr?

I don't think so.

cheers,
   Gerd

Re: [PATCH] Xen PV-on-HVM guest support (v2)

[Avi Kivity]

On 10/15/2009 05:11 PM, Gerd Hoffmann wrote:
>   Hi,
>
>> Is the msr number really variable? Isn't it an ABI?
>
> Yes, it is variable.  The guests gets the msr number via cpuid ...
>
>> Do we need support for reading the msr?
>
> I don't think so.
>

Thanks.  So Ed, I think you're good to go, but please update 
Documentation/kvm/api.txt for your next round.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.