[PATCH] KVM: VMX: Move private memory slot position

[PATCH] KVM: VMX: Move private memory slot position

[Yang, Sheng]

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

From ebe4ea311305d2910dcdcff2510662da0dc2c742 Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang@intel.com>
Date: Thu, 4 Sep 2008 03:11:48 +0800
Subject: [PATCH] KVM: VMX: Move private memory slot position

PCI device assignment would map guest MMIO spaces as separate slot, so it is
possible that the device has more than 2 MMIO spaces and overwrite current
private memslot.

The patch move private memory slot to the top of userspace visible memory 
slots.

Signed-off-by: Sheng Yang <sheng.yang@intel.com>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/vmx.h |    5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 004d24a..27c3bb7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2448,7 +2448,7 @@ static int vmx_set_tss_addr(struct kvm *kvm, unsigned 
int addr)
 {
 	int ret;
 	struct kvm_userspace_memory_region tss_mem = {
-		.slot = 8,
+		.slot = TSS_PRIVATE_MEMSLOT,
 		.guest_phys_addr = addr,
 		.memory_size = PAGE_SIZE * 3,
 		.flags = 0,
diff --git a/arch/x86/kvm/vmx.h b/arch/x86/kvm/vmx.h
index 16b3cfb..dd0eea9 100644
--- a/arch/x86/kvm/vmx.h
+++ b/arch/x86/kvm/vmx.h
@@ -356,8 +356,9 @@ enum vmcs_field {
 #define IA32_FEATURE_CONTROL_LOCKED_BIT		0x1
 #define IA32_FEATURE_CONTROL_VMXON_ENABLED_BIT	0x4

-#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	9
-#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	10
+#define TSS_PRIVATE_MEMSLOT			(KVM_MEMORY_SLOTS + 0)
+#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 1)
+#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 2)

 #define VMX_NR_VPIDS				(1 << 16)
 #define VMX_VPID_EXTENT_SINGLE_CONTEXT		1
--
1.5.4.5


[-- Attachment #2: 0001-KVM-VMX-Move-private-memory-slot-position.patch --]
[-- Type: text/x-diff, Size: 1723 bytes --]

From ebe4ea311305d2910dcdcff2510662da0dc2c742 Mon Sep 17 00:00:00 2001
From: Sheng Yang <sheng.yang@intel.com>
Date: Thu, 4 Sep 2008 03:11:48 +0800
Subject: [PATCH] KVM: VMX: Move private memory slot position

PCI device assignment would map guest MMIO spaces as separate slot, so it is
possible that the device has more than 2 MMIO spaces and overwrite current
private memslot.

The patch move private memory slot to the top of userspace visible memory slots.

Signed-off-by: Sheng Yang <sheng.yang@intel.com>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/vmx.h |    5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 004d24a..27c3bb7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2448,7 +2448,7 @@ static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr)
 {
 	int ret;
 	struct kvm_userspace_memory_region tss_mem = {
-		.slot = 8,
+		.slot = TSS_PRIVATE_MEMSLOT,
 		.guest_phys_addr = addr,
 		.memory_size = PAGE_SIZE * 3,
 		.flags = 0,
diff --git a/arch/x86/kvm/vmx.h b/arch/x86/kvm/vmx.h
index 16b3cfb..dd0eea9 100644
--- a/arch/x86/kvm/vmx.h
+++ b/arch/x86/kvm/vmx.h
@@ -356,8 +356,9 @@ enum vmcs_field {
 #define IA32_FEATURE_CONTROL_LOCKED_BIT		0x1
 #define IA32_FEATURE_CONTROL_VMXON_ENABLED_BIT	0x4
 
-#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	9
-#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	10
+#define TSS_PRIVATE_MEMSLOT			(KVM_MEMORY_SLOTS + 0)
+#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 1)
+#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 2)
 
 #define VMX_NR_VPIDS				(1 << 16)
 #define VMX_VPID_EXTENT_SINGLE_CONTEXT		1
-- 
1.5.4.5

Re: [PATCH] KVM: VMX: Move private memory slot position

[Yang, Sheng]

On Thursday 04 September 2008 11:30:20 Yang, Sheng wrote:
> From ebe4ea311305d2910dcdcff2510662da0dc2c742 Mon Sep 17 00:00:00 2001
> From: Sheng Yang <sheng.yang@intel.com>
> Date: Thu, 4 Sep 2008 03:11:48 +0800
> Subject: [PATCH] KVM: VMX: Move private memory slot position
>
> PCI device assignment would map guest MMIO spaces as separate slot, so it
> is possible that the device has more than 2 MMIO spaces and overwrite
> current private memslot.
>
> The patch move private memory slot to the top of userspace visible memory
> slots.
>

Avi, these two?

(Oh, it's a little old, next time I will use git-send-email :) )
-- 
regards
Yang, Sheng

> Signed-off-by: Sheng Yang <sheng.yang@intel.com>
> ---
>  arch/x86/kvm/vmx.c |    2 +-
>  arch/x86/kvm/vmx.h |    5 +++--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 004d24a..27c3bb7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2448,7 +2448,7 @@ static int vmx_set_tss_addr(struct kvm *kvm, unsigned
> int addr)
>  {
>  	int ret;
>  	struct kvm_userspace_memory_region tss_mem = {
> -		.slot = 8,
> +		.slot = TSS_PRIVATE_MEMSLOT,
>  		.guest_phys_addr = addr,
>  		.memory_size = PAGE_SIZE * 3,
>  		.flags = 0,
> diff --git a/arch/x86/kvm/vmx.h b/arch/x86/kvm/vmx.h
> index 16b3cfb..dd0eea9 100644
> --- a/arch/x86/kvm/vmx.h
> +++ b/arch/x86/kvm/vmx.h
> @@ -356,8 +356,9 @@ enum vmcs_field {
>  #define IA32_FEATURE_CONTROL_LOCKED_BIT		0x1
>  #define IA32_FEATURE_CONTROL_VMXON_ENABLED_BIT	0x4
>
> -#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	9
> -#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	10
> +#define TSS_PRIVATE_MEMSLOT			(KVM_MEMORY_SLOTS + 0)
> +#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 1)
> +#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 2)
>
>  #define VMX_NR_VPIDS				(1 << 16)
>  #define VMX_VPID_EXTENT_SINGLE_CONTEXT		1
> --
> 1.5.4.5

Re: [PATCH] KVM: VMX: Move private memory slot position

[Avi Kivity]

Yang, Sheng wrote:
> On Thursday 04 September 2008 11:30:20 Yang, Sheng wrote:
>   
>> From ebe4ea311305d2910dcdcff2510662da0dc2c742 Mon Sep 17 00:00:00 2001
>> From: Sheng Yang <sheng.yang@intel.com>
>> Date: Thu, 4 Sep 2008 03:11:48 +0800
>> Subject: [PATCH] KVM: VMX: Move private memory slot position
>>
>> PCI device assignment would map guest MMIO spaces as separate slot, so it
>> is possible that the device has more than 2 MMIO spaces and overwrite
>> current private memslot.
>>
>> The patch move private memory slot to the top of userspace visible memory
>> slots.
>>
>>     
>
> Avi, these two?
>   

Thanks, applied both.

Note that kvm now exports the number of slots using KVM_CAP_NR_MEMSLOTS,
so userspace could be made dynamic.

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

Re: [PATCH] KVM: VMX: Move private memory slot position

[Avi Kivity]

Avi Kivity wrote:
> Yang, Sheng wrote:
>   
>> On Thursday 04 September 2008 11:30:20 Yang, Sheng wrote:
>>   
>>     
>>> From ebe4ea311305d2910dcdcff2510662da0dc2c742 Mon Sep 17 00:00:00 2001
>>> From: Sheng Yang <sheng.yang@intel.com>
>>> Date: Thu, 4 Sep 2008 03:11:48 +0800
>>> Subject: [PATCH] KVM: VMX: Move private memory slot position
>>>
>>> PCI device assignment would map guest MMIO spaces as separate slot, so it
>>> is possible that the device has more than 2 MMIO spaces and overwrite
>>> current private memslot.
>>>
>>> The patch move private memory slot to the top of userspace visible memory
>>> slots.
>>>
>>>     
>>>       
>> Avi, these two?
>>   
>>     
>
> Thanks, applied both.
>
> Note that kvm now exports the number of slots using KVM_CAP_NR_MEMSLOTS,
> so userspace could be made dynamic.
>   

Well, the kernel change causes the host to oops while booting Windows on
an i386 pae host.  No idea why.

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

Re: [PATCH] KVM: VMX: Move private memory slot position

[Yang, Sheng]

On Saturday 13 September 2008 16:55:27 Avi Kivity wrote:
> Avi Kivity wrote:
> > Yang, Sheng wrote:
> >> On Thursday 04 September 2008 11:30:20 Yang, Sheng wrote:
> >>> From ebe4ea311305d2910dcdcff2510662da0dc2c742 Mon Sep 17 00:00:00 2001
> >>> From: Sheng Yang <sheng.yang@intel.com>
> >>> Date: Thu, 4 Sep 2008 03:11:48 +0800
> >>> Subject: [PATCH] KVM: VMX: Move private memory slot position
> >>>
> >>> PCI device assignment would map guest MMIO spaces as separate slot, so
> >>> it is possible that the device has more than 2 MMIO spaces and
> >>> overwrite current private memslot.
> >>>
> >>> The patch move private memory slot to the top of userspace visible
> >>> memory slots.
> >>
> >> Avi, these two?
> >
> > Thanks, applied both.
> >
> > Note that kvm now exports the number of slots using KVM_CAP_NR_MEMSLOTS,
> > so userspace could be made dynamic.
>
> Well, the kernel change causes the host to oops while booting Windows on
> an i386 pae host.  No idea why.

I've found the reason... It's because that kvm_mmu_page->slot_bitmap is 
unsigned long, and if use KVM_MEMORY_SLOTS + xxx, it would beyond 32 in pae, 
then memory corrupted.

But reduce supported memory slot number to 28 or extend slot_bitmap, or other 
methods? Slot_bitmap have bitops, so keep unsigned long would be better... 
Now reduce supported memory slot number seems reasonable to me.

(I also want to have this fix into 2.6.28, for some device would easily 
overlapped with current private memory slot)

--
regards
Yang, Sheng

Re: [PATCH] KVM: VMX: Move private memory slot position

[Avi Kivity]

Yang, Sheng wrote:
> I've found the reason... It's because that kvm_mmu_page->slot_bitmap is 
> unsigned long, and if use KVM_MEMORY_SLOTS + xxx, it would beyond 32 in pae, 
> then memory corrupted.
>
> But reduce supported memory slot number to 28 or extend slot_bitmap, or other 
> methods? Slot_bitmap have bitops, so keep unsigned long would be better... 
> Now reduce supported memory slot number seems reasonable to me.
>
>   

We could change it to DECLARE_BITMAP, and thus support >= 32 slots even 
on i386.  But I agree that 28 slots would be sufficient.

> (I also want to have this fix into 2.6.28, for some device would easily 
> overlapped with current private memory slot)
>   

I've been thinking that we can get rid of internal slots, by placing the 
TSS, real mode identity map, and APIC access page in the bios.  Of 
course we would need a new ioctl to let the kernel know where the 
scratch memory is located and how much of it is available.

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

Re: [PATCH] KVM: VMX: Move private memory slot position

[Sheng Yang]

On Thursday 16 October 2008 16:26:07 Avi Kivity wrote:
> Yang, Sheng wrote:
> > I've found the reason... It's because that kvm_mmu_page->slot_bitmap is
> > unsigned long, and if use KVM_MEMORY_SLOTS + xxx, it would beyond 32 in
> > pae, then memory corrupted.
> >
> > But reduce supported memory slot number to 28 or extend slot_bitmap, or
> > other methods? Slot_bitmap have bitops, so keep unsigned long would be
> > better... Now reduce supported memory slot number seems reasonable to me.
>
> We could change it to DECLARE_BITMAP, and thus support >= 32 slots even
> on i386.  But I agree that 28 slots would be sufficient.

DECLARE_BITMAP looks like a flexible solution, I forgot it...(and I also don't 
like to change a lot of macro in kernel and userspace). I will post the patch 
soon.

>
> > (I also want to have this fix into 2.6.28, for some device would easily
> > overlapped with current private memory slot)
>
> I've been thinking that we can get rid of internal slots, by placing the
> TSS, real mode identity map, and APIC access page in the bios.  Of
> course we would need a new ioctl to let the kernel know where the
> scratch memory is located and how much of it is available.
>

How about put into userspace e.g. pc_init()? I think more easy than keeping 
bios sync with KVM. Anyway, vmx specific thing would become generic. I'd like 
to keep it as what it is at least for now...

--
regards
Yang, Sheng

Re: [PATCH] KVM: VMX: Move private memory slot position

[Avi Kivity]

Sheng Yang wrote:
>> I've been thinking that we can get rid of internal slots, by placing the
>> TSS, real mode identity map, and APIC access page in the bios.  Of
>> course we would need a new ioctl to let the kernel know where the
>> scratch memory is located and how much of it is available.
>>
>>     
>
> How about put into userspace e.g. pc_init()? I think more easy than keeping 
> bios sync with KVM. 

That was my plan -- bios reserves space, qemu tells the kernel where it 
is, kernel uses the space.

> Anyway, vmx specific thing would become generic. I'd like 
> to keep it as what it is at least for now...
>   

It's already exposed to userspace, as userspace tells the kernel where 
to put the slots.  The only advantage is that we reduce slot count and 
don't have to mess with the e820 map.

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