credit scheduler issues in 64bit hypervisor

credit scheduler issues in 64bit hypervisor

[Zheng, Jeff]

When we use credit scheduler in 64bit hypervisor, we see these issues:
+ Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
+ Booting 32pae VMX domain will get a kernel panic.
+ Booting indows SP1/SP2 Guest will crash guest OS.

We don't see these issues with BVT scheduler.

Bests
Jeff

RE: credit scheduler issues in 64bit hypervisor

[Ian Pratt]

> When we use credit scheduler in 64bit hypervisor, we see these issues:
> + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
> + Booting 32pae VMX domain will get a kernel panic.
> + Booting indows SP1/SP2 Guest will crash guest OS.
> 
> We don't see these issues with BVT scheduler.

Is this specific to 64b hypervisor? We do most HVM testing on 32b PAE
and haven't seen these issues.

Ian

> 
> Bests
> Jeff
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: credit scheduler issues in 64bit hypervisor

[Emmanuel Ackaouy]

On Wed, Jun 28, 2006 at 11:40:07AM +0800, Zheng, Jeff wrote:
> When we use credit scheduler in 64bit hypervisor, we see these issues:
> + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
> + Booting 32pae VMX domain will get a kernel panic.
> + Booting indows SP1/SP2 Guest will crash guest OS.
> 
> We don't see these issues with BVT scheduler.

Which tree and which changeset are you working from?
Are you at least up to changeset d7543cff88ae (Cleanups
and fixes to VMCS lifecycle)? You need to be.

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

We are using changeset 10508, the changeset you mentioned is 10318, so we must have the fixes.
-Xin 

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com 
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of 
>Emmanuel Ackaouy
>Sent: 2006年6月28日 21:08
>To: Zheng, Jeff
>Cc: xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel] credit scheduler issues in 64bit hypervisor
>
>On Wed, Jun 28, 2006 at 11:40:07AM +0800, Zheng, Jeff wrote:
>> When we use credit scheduler in 64bit hypervisor, we see 
>these issues:
>> + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
>> + Booting 32pae VMX domain will get a kernel panic.
>> + Booting indows SP1/SP2 Guest will crash guest OS.
>> 
>> We don't see these issues with BVT scheduler.
>
>Which tree and which changeset are you working from?
>Are you at least up to changeset d7543cff88ae (Cleanups
>and fixes to VMCS lifecycle)? You need to be.
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel
>

RE: credit scheduler issues in 64bit hypervisor

[Zheng, Jeff]

Yes. This is specific to 64bit hypervisor. We see these issue at CS10508 and CS10482.

We didn't see these issues on 32bit/32bit pae hypervisor.

Bests
Jeff 

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com 
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Ian Pratt
>Sent: 2006年6月28日 20:43
>To: Zheng, Jeff; xen-devel@lists.xensource.com
>Subject: RE: [Xen-devel] credit scheduler issues in 64bit hypervisor 
>
>> When we use credit scheduler in 64bit hypervisor, we see 
>these issues:
>> + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
>> + Booting 32pae VMX domain will get a kernel panic.
>> + Booting indows SP1/SP2 Guest will crash guest OS.
>> 
>> We don't see these issues with BVT scheduler.
>
>Is this specific to 64b hypervisor? We do most HVM testing on 32b PAE
>and haven't seen these issues.
>
>Ian
>
>> 
>> Bests
>> Jeff
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel
>

RE: credit scheduler issues in 64bit hypervisor

[Zheng, Jeff]

We saw these issue on ChangeSet 10508 and 10482.

Bests
Jeff 

>-----Original Message-----
>From: Emmanuel Ackaouy [mailto:ack@xensource.com] 
>Sent: 2006年6月28日 21:08
>To: Zheng, Jeff
>Cc: xen-devel@lists.xensource.com
>Subject: Re: [Xen-devel] credit scheduler issues in 64bit hypervisor
>
>On Wed, Jun 28, 2006 at 11:40:07AM +0800, Zheng, Jeff wrote:
>> When we use credit scheduler in 64bit hypervisor, we see 
>these issues:
>> + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
>> + Booting 32pae VMX domain will get a kernel panic.
>> + Booting indows SP1/SP2 Guest will crash guest OS.
>> 
>> We don't see these issues with BVT scheduler.
>
>Which tree and which changeset are you working from?
>Are you at least up to changeset d7543cff88ae (Cleanups
>and fixes to VMCS lifecycle)? You need to be.
>

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

I'm using sedf scheduler with changeset 10520 on my VT machine which has 4 cpus.
I created a 4 vcpus SMP VMX guest and then a windows guest installer, I got:

(XEN) (GUEST: 14) ata0-0: PCHS=4063/16/63 translation=lba LCHS=1015/64/63
(XEN) (GUEST: 14) ata0 master: QEMU HARDDISK ATA-2 Hard-Disk (2000 MBytes)
(XEN) (GUEST: 14) ata0  slave: Unknown device
(XEN) (GUEST: 14) ata1 master: QEMU CD-ROM ATAPI-4 CD-Rom/DVD-Rom
(XEN) (GUEST: 14) ata1  slave: Unknown device
(XEN) (GUEST: 14) 
(XEN) (GUEST: 14) Booting from CD-Rom...
(XEN) (GUEST: 14) Invalid %cs=0x58 for protected mode
(XEN) (GUEST: 14) 
(XEN) (GUEST: 14) Halt called from %eip 0xD32DA 

But if I only create the windows guest, all is OK.
So seems we have bug if one physical cpu is used by 2 different VMX guest, I'm looking at it.

-Xin
 

>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com 
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Zheng, Jeff
>Sent: 2006年6月28日 11:40
>To: xen-devel@lists.xensource.com
>Subject: [Xen-devel] credit scheduler issues in 64bit hypervisor 
>
>When we use credit scheduler in 64bit hypervisor, we see these issues:
>+ Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
>+ Booting 32pae VMX domain will get a kernel panic.
>+ Booting indows SP1/SP2 Guest will crash guest OS.
>
>We don't see these issues with BVT scheduler.
>
>Bests
>Jeff
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel
>

Re: credit scheduler issues in 64bit hypervisor

[Vincent Hanquez]

On Thu, Jun 29, 2006 at 07:46:57PM +0800, Li, Xin B wrote:
> But if I only create the windows guest, all is OK.
> So seems we have bug if one physical cpu is used by 2 different VMX
> guest, I'm looking at it.

it seems to be more or less similar with:

http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=679

-- 
Vincent Hanquez

RE: credit scheduler issues in 64bit hypervisor

[Rik van Riel]

On Wed, 28 Jun 2006, Ian Pratt wrote:
> > When we use credit scheduler in 64bit hypervisor, we see these issues:
> > + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
> > + Booting 32pae VMX domain will get a kernel panic.
> > + Booting indows SP1/SP2 Guest will crash guest OS.
> > 
> > We don't see these issues with BVT scheduler.
> 
> Is this specific to 64b hypervisor? We do most HVM testing on 32b PAE
> and haven't seen these issues.

I am also seeing an instant reboot if I try the credit
scheduler on my x86-64 system.  The reboot happens every
time I try to start a VT guest domain - haven't tried
paravirt, since I need one of the VT domains :)

Please don't make the credit scheduler the default until
it actually works.

-- 
All Rights Reversed

Re: credit scheduler issues in 64bit hypervisor

[Steven Hand]

>On Wed, 28 Jun 2006, Ian Pratt wrote:
>> > When we use credit scheduler in 64bit hypervisor, we see these issues:
>> > + Booting 32bit VMX domain hangs at "ACPI: Unable to locate RSDP".
>> > + Booting 32pae VMX domain will get a kernel panic.
>> > + Booting indows SP1/SP2 Guest will crash guest OS.
>> > 
>> > We don't see these issues with BVT scheduler.
>> 
>> Is this specific to 64b hypervisor? We do most HVM testing on 32b PAE
>> and haven't seen these issues.
>
>I am also seeing an instant reboot if I try the credit
>scheduler on my x86-64 system.  The reboot happens every
>time I try to start a VT guest domain - haven't tried
>paravirt, since I need one of the VT domains :)

Have you got any debug output from Xen ? (and are you running a debug 
build?). 

>Please don't make the credit scheduler the default until
>it actually works.

I'm not sure this is a credit scheduler bug - it seems rather more likely
to be a vmx bug shaken out by the more aggressive load balancing of the 
new scheduler. Masking such bugs by using another scheduler doesn't seem
like a great idea to me.

Of course, difficult to be certain of the exact cause without some debug
output or similar. 


cheers,

S.

Re: credit scheduler issues in 64bit hypervisor

[Rik van Riel]

On Sat, 1 Jul 2006, Steven Hand wrote:

> Have you got any debug output from Xen ? (and are you running a debug 
> build?). 

I have no serial console on this system, and it rebooted
instantly - so unfortunately, no debugging output.

I will try this on one of my test systems at work, which
do have serial console.

> >Please don't make the credit scheduler the default until
> >it actually works.
> 
> I'm not sure this is a credit scheduler bug - it seems rather more likely
> to be a vmx bug shaken out by the more aggressive load balancing of the 
> new scheduler. Masking such bugs by using another scheduler doesn't seem
> like a great idea to me.

Point taken.  Though at this point I can't rule out the
scheduler yet, either :)

> Of course, difficult to be certain of the exact cause without some debug
> output or similar. 

*nod*

-- 
All Rights Reversed

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

>>I am also seeing an instant reboot if I try the credit
>>scheduler on my x86-64 system.  The reboot happens every
>>time I try to start a VT guest domain - haven't tried
>>paravirt, since I need one of the VT domains :)
>
>Have you got any debug output from Xen ? (and are you running a debug 
>build?). 
>
>>Please don't make the credit scheduler the default until
>>it actually works.
>
>I'm not sure this is a credit scheduler bug - it seems rather 
>more likely
>to be a vmx bug shaken out by the more aggressive load 
>balancing of the 
>new scheduler. Masking such bugs by using another scheduler 
>doesn't seem
>like a great idea to me.
>

This is caused by a vmcs bug, the root cause is on x86_64, a VMX domain
is killed without any vmentry (caused by "Error: Device 768 (vbd) could
not be connected. Hotplug scripts not working."), but then a VMCLEAR is
still executed on its unlaunched VMCS.
the following patch fixes it.

Signed-off-by: Xin Li <xin.b.li@intel.com>

diff -r 130a5badf2b7 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c       Fri Jun 30 22:02:58 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmcs.c       Sat Jul 01 23:10:31 2006 +0800
@@ -67,7 +67,12 @@ static void __vmx_clear_vmcs(void *info)
 static void __vmx_clear_vmcs(void *info)
 {
     struct vcpu *v = info;
+
+    if ( !v->arch.hvm_vmx.launched )
+        return;
+
     __vmpclear(virt_to_maddr(v->arch.hvm_vmx.vmcs));
+
     v->arch.hvm_vmx.active_cpu = -1;
     v->arch.hvm_vmx.launched   = 0;
 }

RE: credit scheduler issues in 64bit hypervisor

[Rik van Riel]

On Sat, 1 Jul 2006, Li, Xin B wrote:

> This is caused by a vmcs bug, the root cause is on x86_64, a VMX domain
> is killed without any vmentry (caused by "Error: Device 768 (vbd) could
> not be connected. Hotplug scripts not working."), but then a VMCLEAR is
> still executed on its unlaunched VMCS.

I guess that might explain the lack of console output :)

-- 
All Rights Reversed

Re: credit scheduler issues in 64bit hypervisor

[Keir Fraser]

On 1 Jul 2006, at 16:22, Li, Xin B wrote:

> This is caused by a vmcs bug, the root cause is on x86_64, a VMX domain
> is killed without any vmentry (caused by "Error: Device 768 (vbd) could
> not be connected. Hotplug scripts not working."), but then a VMCLEAR is
> still executed on its unlaunched VMCS.
> the following patch fixes it.
>
> Signed-off-by: Xin Li <xin.b.li@intel.com>

This patch is itself buggy: Just because a VMCS hasn't been launched 
doesn't mean it hasn't been activated on some CPU. I think the original 
bug would be better fixed by only calling vmx_clear_vmcs() in 
vmx_destroy_vmcs() if arch_vmx->vmcs != NULL. Even better would be not 
to allocate the VMCS so darn late.

  -- Keir

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

 

>-----Original Message-----
>From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
>Sent: 2006年7月2日 0:02
>To: Li, Xin B
>Cc: xen-devel@lists.xensource.com; Ian Pratt; Rik van Riel; 
>Steven Hand; Zheng, Jeff
>Subject: Re: [Xen-devel] credit scheduler issues in 64bit hypervisor
>
>
>On 1 Jul 2006, at 16:22, Li, Xin B wrote:
>
>> This is caused by a vmcs bug, the root cause is on x86_64, a 
>VMX domain
>> is killed without any vmentry (caused by "Error: Device 768 
>(vbd) could
>> not be connected. Hotplug scripts not working."), but then a 
>VMCLEAR is
>> still executed on its unlaunched VMCS.
>> the following patch fixes it.
>>
>> Signed-off-by: Xin Li <xin.b.li@intel.com>
>
>This patch is itself buggy: Just because a VMCS hasn't been launched 
>doesn't mean it hasn't been activated on some CPU. I think the 
>original 
>bug would be better fixed by only calling vmx_clear_vmcs() in 
>vmx_destroy_vmcs() if arch_vmx->vmcs != NULL. Even better would be not 
>to allocate the VMCS so darn late.
>

Yes, it's buggy, and prevent the first vmclear to a vmcs.
-Xin

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

>>-----Original Message-----
>>From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
>>Sent: 2006年7月2日 0:02
>>To: Li, Xin B
>>Cc: xen-devel@lists.xensource.com; Ian Pratt; Rik van Riel; 
>>Steven Hand; Zheng, Jeff
>>Subject: Re: [Xen-devel] credit scheduler issues in 64bit hypervisor
>>
>>
>>On 1 Jul 2006, at 16:22, Li, Xin B wrote:
>>
>>> This is caused by a vmcs bug, the root cause is on x86_64, a 
>>VMX domain
>>> is killed without any vmentry (caused by "Error: Device 768 
>>(vbd) could
>>> not be connected. Hotplug scripts not working."), but then a 
>>VMCLEAR is
>>> still executed on its unlaunched VMCS.
>>> the following patch fixes it.
>>>
>>> Signed-off-by: Xin Li <xin.b.li@intel.com>
>>
>>This patch is itself buggy: Just because a VMCS hasn't been launched 
>>doesn't mean it hasn't been activated on some CPU. 

Hmm, thinking about a VMCS is migrating from cpu A to cpu B, and on cpu A vmclear has been executed, but just before the VMCS is launched on cpu B, the domain is killed, what will happen?
I'm not sure if vmclear on a VMCS in cleared state is allowed. If not, we still need this fix.


>>I think the original 
>>bug would be better fixed by only calling vmx_clear_vmcs() in 
>>vmx_destroy_vmcs() if arch_vmx->vmcs != NULL. Even better 
>would be not 
>>to allocate the VMCS so darn late.
>>
>
>Yes, it's buggy, and prevent the first vmclear to a vmcs.

I found even without my fix the first vmclear to a newly allocated vmcs is prevented, this is because arch_vmx->active_cpu = -1is executed  before vmx_clear_vmcs(v) in construct_vmcs().
We may workaound it by setting active_cpu to smp_processor_id(), and launched to 1here, but surely this is not what we want.

-Xin

Re: credit scheduler issues in 64bit hypervisor

[Keir Fraser]

On 2 Jul 2006, at 05:01, Li, Xin B wrote:

>>> This patch is itself buggy: Just because a VMCS hasn't been launched
>>> doesn't mean it hasn't been activated on some CPU.
>
> Hmm, thinking about a VMCS is migrating from cpu A to cpu B, and on 
> cpu A vmclear has been executed, but just before the VMCS is launched 
> on cpu B, the domain is killed, what will happen?
> I'm not sure if vmclear on a VMCS in cleared state is allowed. If not, 
> we still need this fix.

active_cpu will be B in this case, so we'll execute VMCLEAR on CPU B. 
'Launched' is just an extra sub-state of an active VMCS. This is all 
taken from Section 20.1 of Vol. 3 of the Intel Reference Manual.

>>> I think the original
>>> bug would be better fixed by only calling vmx_clear_vmcs() in
>>> vmx_destroy_vmcs() if arch_vmx->vmcs != NULL. Even better
>> would be not
>>> to allocate the VMCS so darn late.
>>>
>>
>> Yes, it's buggy, and prevent the first vmclear to a vmcs.
>
> I found even without my fix the first vmclear to a newly allocated 
> vmcs is prevented, this is because arch_vmx->active_cpu = -1is 
> executed  before vmx_clear_vmcs(v) in construct_vmcs().
> We may workaound it by setting active_cpu to smp_processor_id(), and 
> launched to 1here, but surely this is not what we want.

Yes, that's broken. I'll fix to use __vmx_clear_vmcs().

  -- Keir

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

 >> This is caused by a vmcs bug, the root cause is on x86_64, a 
>VMX domain
>> is killed without any vmentry (caused by "Error: Device 768 
>(vbd) could
>> not be connected. Hotplug scripts not working."), but then a 
>VMCLEAR is
>> still executed on its unlaunched VMCS.
>> the following patch fixes it.
>>
>> Signed-off-by: Xin Li <xin.b.li@intel.com>
>
>This patch is itself buggy: Just because a VMCS hasn't been launched 
>doesn't mean it hasn't been activated on some CPU. I think the 
>original 
>bug would be better fixed by only calling vmx_clear_vmcs() in 
>vmx_destroy_vmcs() if arch_vmx->vmcs != NULL. Even better would be not 
>to allocate the VMCS so darn late.
>

Yes, such a fix should work.

diff -r 130a5badf2b7 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c       Fri Jun 30 22:02:58 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmcs.c       Sun Jul 02 22:33:47 2006 +0800
@@ -495,6 +511,9 @@ void vmx_destroy_vmcs(struct vcpu *v)
 void vmx_destroy_vmcs(struct vcpu *v)
 {
     struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
+
+    if ( arch_vmx->vmcs == NULL )
+        return;

     vmx_clear_vmcs(v);

Hmm, I still prefer allocating VMCS just before launching, can you
explain your concerns?
thanks
-Xin

Re: credit scheduler issues in 64bit hypervisor

[Keir Fraser]

On 2 Jul 2006, at 15:38, Li, Xin B wrote:

> Hmm, I still prefer allocating VMCS just before launching, can you
> explain your concerns?

If the VMCS allocation fails you have no path for indicating failure to 
the caller -- context switch is always expected to succeed. Critical 
dynamic allocations should really happen on domain/vcpu creation. Also 
the short VMCS lifetime (first launch to dom0_destroy) means that 
getvcpucontext at other times -- on a very new HVM guest or one very 
close to death -- probably crashes or hangs right now.

  -- Keir

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

>> Hmm, I still prefer allocating VMCS just before launching, can you
>> explain your concerns?
>
>If the VMCS allocation fails you have no path for indicating 
>failure to 
>the caller -- context switch is always expected to succeed. Critical 
>dynamic allocations should really happen on domain/vcpu creation. Also 
>the short VMCS lifetime (first launch to dom0_destroy) means that 
>getvcpucontext at other times -- on a very new HVM guest or one very 
>close to death -- probably crashes or hangs right now.
>

OK, that's reasonable :-), I'll send out the patch.
Thanks
-Xin

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

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

>>If the VMCS allocation fails you have no path for indicating 
>>failure to 
>>the caller -- context switch is always expected to succeed. Critical 
>>dynamic allocations should really happen on domain/vcpu 
>creation. Also 
>>the short VMCS lifetime (first launch to dom0_destroy) means that 
>>getvcpucontext at other times -- on a very new HVM guest or one very 
>>close to death -- probably crashes or hangs right now.
>>
>
>OK, that's reasonable :-), I'll send out the patch.

This patch is to move vmcs and IO bitmap allocation into
vmx_initialize_guest_resources.
And some necessary code rearrangement.

Signed-off-by: Xin Li <xin.b.li@intel.com>


[-- Attachment #2: construct_vmcs.4.patch --]
[-- Type: application/octet-stream, Size: 22502 bytes --]

diff -r fd6d12935b56 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Tue Jul 04 18:32:01 2006 +0800
@@ -46,9 +46,9 @@
 #include <public/hvm/ioreq.h>
 #include <public/hvm/hvm_info_table.h>
 
-int hvm_enabled = 0;
-
-unsigned int opt_hvm_debug_level = 0;
+int hvm_enabled;
+
+unsigned int opt_hvm_debug_level;
 integer_param("hvm_debug", opt_hvm_debug_level);
 
 struct hvm_function_table hvm_funcs;
diff -r fd6d12935b56 xen/arch/x86/hvm/svm/vmcb.c
--- a/xen/arch/x86/hvm/svm/vmcb.c	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/arch/x86/hvm/svm/vmcb.c	Tue Jul 04 18:32:01 2006 +0800
@@ -265,8 +265,8 @@ static int construct_init_vmcb_guest(str
     vmcb->rsp = 0;
     vmcb->rip = regs->eip;
 
-    eflags = regs->eflags & ~HVM_EFLAGS_RESERVED_0; /* clear 0s */
-    eflags |= HVM_EFLAGS_RESERVED_1; /* set 1s */
+    eflags = regs->eflags & ~HVM_RFLAGS_RESERVED_0; /* clear 0s */
+    eflags |= HVM_RFLAGS_RESERVED_1; /* set 1s */
 
     vmcb->rflags = eflags;
 
diff -r fd6d12935b56 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmcs.c	Tue Jul 04 18:32:01 2006 +0800
@@ -41,34 +41,68 @@
 #include <asm/shadow_64.h>
 #endif
 
-int vmcs_size;
-
-struct vmcs_struct *vmx_alloc_vmcs(void)
+static int vmcs_size;
+static int vmcs_order;
+static u32 vmcs_revision_id;
+
+void vmx_init_vmcs_config(void)
+{
+    u32 vmx_msr_low, vmx_msr_high;
+
+    if ( vmcs_size )
+        return;
+
+    rdmsr(MSR_IA32_VMX_BASIC_MSR, vmx_msr_low, vmx_msr_high);
+
+    vmcs_size = vmx_msr_high & 0x1fff;
+
+    vmcs_revision_id = vmx_msr_low;
+
+    vmcs_order = get_order_from_bytes(vmcs_size);
+}
+
+static inline void vmx_free_vmcs(struct vmcs_struct *vmcs)
+{
+    free_xenheap_pages(vmcs, vmcs_order);
+}
+
+struct vmcs_struct *vmx_alloc_vmcs(int is_host_vmcs)
 {
     struct vmcs_struct *vmcs;
-    u32 vmx_msr_low, vmx_msr_high;
-
-    rdmsr(MSR_IA32_VMX_BASIC_MSR, vmx_msr_low, vmx_msr_high);
-    vmcs_size = vmx_msr_high & 0x1fff;
-    vmcs = alloc_xenheap_pages(get_order_from_bytes(vmcs_size));
-    memset((char *)vmcs, 0, vmcs_size); /* don't remove this */
-
-    vmcs->vmcs_revision_id = vmx_msr_low;
+    int error;
+
+    if ( (vmcs = alloc_xenheap_pages(vmcs_order)) == NULL )
+    {
+        printk("Failed to allocate VMCS.\n");
+        return NULL;
+    }
+
+    memset(vmcs, 0, vmcs_size); /* don't remove this */
+
+    vmcs->vmcs_revision_id = vmcs_revision_id;
+
+    if ( is_host_vmcs )
+        return vmcs;
+
+    if ( (error = __vmclear(virt_to_maddr(vmcs))) != 0 )
+    {
+        printk("Failed to VMCLEAR the newly allocated VMCS (%d).\n", error);
+        vmx_free_vmcs(vmcs);
+        return NULL;
+    }
+
     return vmcs;
 }
 
-static void free_vmcs(struct vmcs_struct *vmcs)
-{
-    int order;
-
-    order = get_order_from_bytes(vmcs_size);
-    free_xenheap_pages(vmcs, order);
-}
-
 static void __vmx_clear_vmcs(void *info)
 {
     struct vcpu *v = info;
-    __vmpclear(virt_to_maddr(v->arch.hvm_vmx.vmcs));
+
+    if ( !v->arch.hvm_vmx.launched )
+        return;
+
+    __vmclear(virt_to_maddr(v->arch.hvm_vmx.vmcs));
+
     v->arch.hvm_vmx.active_cpu = -1;
     v->arch.hvm_vmx.launched   = 0;
 }
@@ -131,8 +165,6 @@ static inline int construct_vmcs_control
 static inline int construct_vmcs_controls(struct arch_vmx_struct *arch_vmx)
 {
     int error = 0;
-    void *io_bitmap_a;
-    void *io_bitmap_b;
 
     error |= __vmwrite(PIN_BASED_VM_EXEC_CONTROL,
                        MONITOR_PIN_BASED_EXEC_CONTROLS);
@@ -141,19 +173,8 @@ static inline int construct_vmcs_control
 
     error |= __vmwrite(VM_ENTRY_CONTROLS, MONITOR_VM_ENTRY_CONTROLS);
 
-    /* need to use 0x1000 instead of PAGE_SIZE */
-    io_bitmap_a = (void*) alloc_xenheap_pages(get_order_from_bytes(0x1000));
-    io_bitmap_b = (void*) alloc_xenheap_pages(get_order_from_bytes(0x1000));
-    memset(io_bitmap_a, 0xff, 0x1000);
-    /* don't bother debug port access */
-    clear_bit(PC_DEBUG_PORT, io_bitmap_a);
-    memset(io_bitmap_b, 0xff, 0x1000);
-
-    error |= __vmwrite(IO_BITMAP_A, (u64) virt_to_maddr(io_bitmap_a));
-    error |= __vmwrite(IO_BITMAP_B, (u64) virt_to_maddr(io_bitmap_b));
-
-    arch_vmx->io_bitmap_a = io_bitmap_a;
-    arch_vmx->io_bitmap_b = io_bitmap_b;
+    error |= __vmwrite(IO_BITMAP_A, (u64)virt_to_maddr(arch_vmx->io_bitmap_a));
+    error |= __vmwrite(IO_BITMAP_B, (u64)virt_to_maddr(arch_vmx->io_bitmap_b));
 
     return error;
 }
@@ -282,7 +303,7 @@ static inline int construct_init_vmcs_gu
     int error = 0;
     union vmcs_arbytes arbytes;
     unsigned long dr7;
-    unsigned long eflags;
+    unsigned long rflags;
 
     /* MSR */
     error |= __vmwrite(VM_EXIT_MSR_LOAD_ADDR, 0);
@@ -291,8 +312,10 @@ static inline int construct_init_vmcs_gu
     error |= __vmwrite(VM_EXIT_MSR_STORE_COUNT, 0);
     error |= __vmwrite(VM_EXIT_MSR_LOAD_COUNT, 0);
     error |= __vmwrite(VM_ENTRY_MSR_LOAD_COUNT, 0);
+
     /* interrupt */
     error |= __vmwrite(VM_ENTRY_INTR_INFO_FIELD, 0);
+
     /* mask */
     error |= __vmwrite(CR0_GUEST_HOST_MASK, -1UL);
     error |= __vmwrite(CR4_GUEST_HOST_MASK, -1UL);
@@ -363,19 +386,21 @@ static inline int construct_init_vmcs_gu
 
     arbytes.fields.seg_type = 0xb;          /* 32-bit TSS (busy) */
     error |= __vmwrite(GUEST_TR_AR_BYTES, arbytes.bytes);
+
     /* CR3 is set in vmx_final_setup_guest */
-
     error |= __vmwrite(GUEST_RSP, 0);
     error |= __vmwrite(GUEST_RIP, regs->eip);
 
     /* Guest EFLAGS */
-    eflags = regs->eflags & ~HVM_EFLAGS_RESERVED_0; /* clear 0s */
-    eflags |= HVM_EFLAGS_RESERVED_1; /* set 1s */
-    error |= __vmwrite(GUEST_RFLAGS, eflags);
+    rflags = regs->eflags & ~HVM_RFLAGS_RESERVED_0; /* clear 0s */
+    rflags |= HVM_RFLAGS_RESERVED_1; /* set 1s */
+    error |= __vmwrite(GUEST_RFLAGS, rflags);
 
     error |= __vmwrite(GUEST_INTERRUPTIBILITY_INFO, 0);
+
     __asm__ __volatile__ ("mov %%dr7, %0\n" : "=r" (dr7));
     error |= __vmwrite(GUEST_DR7, dr7);
+
     error |= __vmwrite(VMCS_LINK_POINTER, 0xffffffff);
     error |= __vmwrite(VMCS_LINK_POINTER_HIGH, 0xffffffff);
 
@@ -400,13 +425,11 @@ static inline int construct_vmcs_host(vo
     error |= __vmwrite(HOST_GS_SELECTOR, __HYPERVISOR_DS);
     error |= __vmwrite(HOST_FS_BASE, 0);
     error |= __vmwrite(HOST_GS_BASE, 0);
-
 #else
     rdmsrl(MSR_FS_BASE, fs_base);
     rdmsrl(MSR_GS_BASE, gs_base);
     error |= __vmwrite(HOST_FS_BASE, fs_base);
     error |= __vmwrite(HOST_GS_BASE, gs_base);
-
 #endif
     error |= __vmwrite(HOST_CS_SELECTOR, __HYPERVISOR_CS);
 
@@ -417,7 +440,7 @@ static inline int construct_vmcs_host(vo
     __asm__ __volatile__ ("mov %%cr4,%0" : "=r" (crn) : );
     error |= __vmwrite(HOST_CR4, crn);
 
-    error |= __vmwrite(HOST_RIP, (unsigned long) vmx_asm_vmexit_handler);
+    error |= __vmwrite(HOST_RIP, (unsigned long)vmx_asm_vmexit_handler);
 #ifdef __x86_64__
     /* TBD: support cr8 for 64-bit guest */
     __vmwrite(VIRTUAL_APIC_PAGE_ADDR, 0);
@@ -429,67 +452,44 @@ static inline int construct_vmcs_host(vo
 }
 
 /*
- * Need to extend to support full virtualization.
+ * the working VMCS pointer has been set properly
+ * just before entering this function.
  */
 static int construct_vmcs(struct vcpu *v,
                           cpu_user_regs_t *regs)
 {
     struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
     int error;
-    long rc;
-
-    memset(arch_vmx, 0, sizeof(struct arch_vmx_struct));
-
-    spin_lock_init(&arch_vmx->vmcs_lock);
-
-    /*
-     * Create a new VMCS
-     */
-    if (!(arch_vmx->vmcs = vmx_alloc_vmcs())) {
-        printk("Failed to create a new VMCS\n");
-        return -ENOMEM;
-    }
-
-    __vmx_clear_vmcs(v);
-    vmx_load_vmcs(v);
-
-    if ((error = construct_vmcs_controls(arch_vmx))) {
-        printk("construct_vmcs: construct_vmcs_controls failed\n");
-        rc = -EINVAL;
-        goto err_out;
+
+    if ( (error = construct_vmcs_controls(arch_vmx)) ) {
+        printk("construct_vmcs: construct_vmcs_controls failed.\n");
+        return error;
     }
 
     /* host selectors */
-    if ((error = construct_vmcs_host())) {
-        printk("construct_vmcs: construct_vmcs_host failed\n");
-        rc = -EINVAL;
-        goto err_out;
+    if ( (error = construct_vmcs_host()) ) {
+        printk("construct_vmcs: construct_vmcs_host failed.\n");
+        return error;
     }
 
     /* guest selectors */
-    if ((error = construct_init_vmcs_guest(regs))) {
-        printk("construct_vmcs: construct_vmcs_guest failed\n");
-        rc = -EINVAL;
-        goto err_out;
-    }
-
-    if ((error |= __vmwrite(EXCEPTION_BITMAP,
-                            MONITOR_DEFAULT_EXCEPTION_BITMAP))) {
-        printk("construct_vmcs: setting Exception bitmap failed\n");
-        rc = -EINVAL;
-        goto err_out;
-    }
-
-    if (regs->eflags & EF_TF)
-        __vm_set_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
+    if ( (error = construct_init_vmcs_guest(regs)) ) {
+        printk("construct_vmcs: construct_vmcs_guest failed.\n");
+        return error;
+    }
+
+    if ( (error = __vmwrite(EXCEPTION_BITMAP,
+                            MONITOR_DEFAULT_EXCEPTION_BITMAP)) ) {
+        printk("construct_vmcs: setting exception bitmap failed.\n");
+        return error;
+    }
+
+    if ( regs->eflags & EF_TF )
+        error = __vm_set_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
     else
-        __vm_clear_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
-
-    return 0;
-
-err_out:
-    vmx_destroy_vmcs(v);
-    return rc;
+        error = __vm_clear_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
+
+    return error;
 }
 
 void vmx_destroy_vmcs(struct vcpu *v)
@@ -501,14 +501,14 @@ void vmx_destroy_vmcs(struct vcpu *v)
 
     vmx_clear_vmcs(v);
 
-    free_vmcs(arch_vmx->vmcs);
+    free_xenheap_pages(arch_vmx->io_bitmap_a, IO_BITMAP_ORDER);
+    free_xenheap_pages(arch_vmx->io_bitmap_b, IO_BITMAP_ORDER);
+
+    arch_vmx->io_bitmap_a = NULL;
+    arch_vmx->io_bitmap_b = NULL;
+
+    vmx_free_vmcs(arch_vmx->vmcs);
     arch_vmx->vmcs = NULL;
-
-    free_xenheap_pages(arch_vmx->io_bitmap_a, get_order_from_bytes(0x1000));
-    arch_vmx->io_bitmap_a = NULL;
-
-    free_xenheap_pages(arch_vmx->io_bitmap_b, get_order_from_bytes(0x1000));
-    arch_vmx->io_bitmap_b = NULL;
 }
 
 void vm_launch_fail(unsigned long eflags)
@@ -547,19 +547,20 @@ void arch_vmx_do_resume(struct vcpu *v)
 
 void arch_vmx_do_launch(struct vcpu *v)
 {
-    int error;
     cpu_user_regs_t *regs = &current->arch.guest_context.user_regs;
 
-    error = construct_vmcs(v, regs);
-    if ( error < 0 )
+    vmx_load_vmcs(v);
+
+    if ( construct_vmcs(v, regs) < 0 )
     {
-        if (v->vcpu_id == 0) {
-            printk("Failed to construct a new VMCS for BSP.\n");
+        if ( v->vcpu_id == 0 ) {
+            printk("Failed to construct VMCS for BSP.\n");
         } else {
-            printk("Failed to construct a new VMCS for AP %d\n", v->vcpu_id);
+            printk("Failed to construct VMCS for AP %d.\n", v->vcpu_id);
         }
         domain_crash_synchronous();
     }
+
     vmx_do_launch(v);
     reset_stack_and_jump(vmx_asm_do_vmentry);
 }
diff -r fd6d12935b56 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Tue Jul 04 18:32:01 2006 +0800
@@ -54,7 +54,7 @@ static void vmx_ctxt_switch_from(struct 
 static void vmx_ctxt_switch_from(struct vcpu *v);
 static void vmx_ctxt_switch_to(struct vcpu *v);
 
-void vmx_final_setup_guest(struct vcpu *v)
+static int vmx_initialize_guest_resources(struct vcpu *v)
 {
     v->arch.schedule_tail    = arch_vmx_do_launch;
     v->arch.ctxt_switch_from = vmx_ctxt_switch_from;
@@ -64,10 +64,41 @@ void vmx_final_setup_guest(struct vcpu *
     {
         struct domain *d = v->domain;
         struct vcpu *vc;
-
-        /* Initialize monitor page table */
-        for_each_vcpu(d, vc)
+        void *io_bitmap_a, *io_bitmap_b;
+
+        for_each_vcpu ( d, vc ) {
+            /* Initialize monitor page table */
             vc->arch.monitor_table = pagetable_null();
+
+            memset(&vc->arch.hvm_vmx, 0, sizeof(struct arch_vmx_struct));
+
+            /* Create VMCS for each vcpu */
+            if ( (vc->arch.hvm_vmx.vmcs = vmx_alloc_vmcs(0)) == NULL ) {
+                printk("Failed to create VMCS for vcpu %d.\n", vc->vcpu_id);
+                return 0;
+            }
+
+            spin_lock_init(&vc->arch.hvm_vmx.vmcs_lock);
+
+            if ( (io_bitmap_a = alloc_xenheap_pages(IO_BITMAP_ORDER)) == NULL ) {
+                printk("Failed to allocate io bitmap a for vcpu %d.\n", vc->vcpu_id);
+                return 0;
+            }
+
+            if ( (io_bitmap_b = alloc_xenheap_pages(IO_BITMAP_ORDER)) == NULL ) {
+                printk("Failed to allocate io bitmap b for vcpu %d.\n", vc->vcpu_id);
+                return 0;
+            }
+
+            memset(io_bitmap_a, 0xff, 0x1000);
+            memset(io_bitmap_b, 0xff, 0x1000);
+
+            /* don't bother debug port access */
+            clear_bit(PC_DEBUG_PORT, io_bitmap_a);
+
+            vc->arch.hvm_vmx.io_bitmap_a = io_bitmap_a;
+            vc->arch.hvm_vmx.io_bitmap_b = io_bitmap_b;
+        }
 
         /*
          * Required to do this once per domain
@@ -82,6 +113,8 @@ void vmx_final_setup_guest(struct vcpu *
                            SHM_enable|SHM_refcounts|
                            SHM_translate|SHM_external|SHM_wr_pt_pte);
     }
+
+    return 1;
 }
 
 static void vmx_relinquish_guest_resources(struct domain *d)
@@ -90,9 +123,9 @@ static void vmx_relinquish_guest_resourc
 
     for_each_vcpu ( d, v )
     {
+        vmx_destroy_vmcs(v);
         if ( !test_bit(_VCPUF_initialised, &v->vcpu_flags) )
             continue;
-        vmx_destroy_vmcs(v);
         free_monitor_pagetable(v);
         kill_timer(&v->arch.hvm_vmx.hlt_timer);
         if ( hvm_apic_support(v->domain) && (VLAPIC(v) != NULL) )
@@ -438,16 +471,10 @@ static void vmx_ctxt_switch_to(struct vc
     vmx_restore_dr(v);
 }
 
-void stop_vmx(void)
+static void stop_vmx(void)
 {
     if (read_cr4() & X86_CR4_VMXE)
         __vmxoff();
-}
-
-int vmx_initialize_guest_resources(struct vcpu *v)
-{
-    vmx_final_setup_guest(v);
-    return 1;
 }
 
 void vmx_migrate_timers(struct vcpu *v)
@@ -529,7 +556,7 @@ static void fixup_vm86_seg_bases(struct 
     BUG_ON(err);
 }
 
-void vmx_load_cpu_guest_regs(struct vcpu *v, struct cpu_user_regs *regs)
+static void vmx_load_cpu_guest_regs(struct vcpu *v, struct cpu_user_regs *regs)
 {
     vmx_vmcs_enter(v);
 
@@ -555,7 +582,7 @@ void vmx_load_cpu_guest_regs(struct vcpu
     vmx_vmcs_exit(v);
 }
 
-int vmx_realmode(struct vcpu *v)
+static int vmx_realmode(struct vcpu *v)
 {
     unsigned long rflags;
 
@@ -563,7 +590,7 @@ int vmx_realmode(struct vcpu *v)
     return rflags & X86_EFLAGS_VM;
 }
 
-int vmx_instruction_length(struct vcpu *v)
+static int vmx_instruction_length(struct vcpu *v)
 {
     unsigned long inst_len;
 
@@ -572,7 +599,7 @@ int vmx_instruction_length(struct vcpu *
     return inst_len;
 }
 
-unsigned long vmx_get_ctrl_reg(struct vcpu *v, unsigned int num)
+static unsigned long vmx_get_ctrl_reg(struct vcpu *v, unsigned int num)
 {
     switch ( num )
     {
@@ -589,7 +616,7 @@ unsigned long vmx_get_ctrl_reg(struct vc
 }
 
 /* SMP VMX guest support */
-void vmx_init_ap_context(struct vcpu_guest_context *ctxt,
+static void vmx_init_ap_context(struct vcpu_guest_context *ctxt,
                          int vcpuid, int trampoline_vector)
 {
     int i;
@@ -636,18 +663,37 @@ static int check_vmx_controls(u32 ctrls,
     return 1;
 }
 
+/* Setup HVM interfaces */
+static void vmx_init_hvm_funcs(void)
+{
+    if ( hvm_enabled )
+        return;
+
+    hvm_funcs.disable = stop_vmx;
+
+    hvm_funcs.initialize_guest_resources = vmx_initialize_guest_resources;
+    hvm_funcs.relinquish_guest_resources = vmx_relinquish_guest_resources;
+
+    hvm_funcs.store_cpu_guest_regs = vmx_store_cpu_guest_regs;
+    hvm_funcs.load_cpu_guest_regs = vmx_load_cpu_guest_regs;
+
+    hvm_funcs.realmode = vmx_realmode;
+    hvm_funcs.paging_enabled = vmx_paging_enabled;
+    hvm_funcs.instruction_length = vmx_instruction_length;
+    hvm_funcs.get_guest_ctrl_reg = vmx_get_ctrl_reg;
+
+    hvm_funcs.init_ap_context = vmx_init_ap_context;
+}
+
 int start_vmx(void)
 {
+    u32 eax, edx;
     struct vmcs_struct *vmcs;
-    u32 ecx;
-    u32 eax, edx;
-    u64 phys_vmcs;      /* debugging */
 
     /*
      * Xen does not fill x86_capability words except 0.
      */
-    ecx = cpuid_ecx(1);
-    boot_cpu_data.x86_capability[4] = ecx;
+    boot_cpu_data.x86_capability[4] = cpuid_ecx(1);
 
     if (!(test_bit(X86_FEATURE_VMXE, &boot_cpu_data.x86_capability)))
         return 0;
@@ -659,8 +705,7 @@ int start_vmx(void)
             printk("VMX disabled by Feature Control MSR.\n");
             return 0;
         }
-    }
-    else {
+    } else {
         wrmsr(IA32_FEATURE_CONTROL_MSR,
               IA32_FEATURE_CONTROL_MSR_LOCK |
               IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON, 0);
@@ -681,37 +726,22 @@ int start_vmx(void)
 
     set_in_cr4(X86_CR4_VMXE);   /* Enable VMXE */
 
-    if (!(vmcs = vmx_alloc_vmcs())) {
+    vmx_init_vmcs_config();
+
+    if ( !(vmcs = vmx_alloc_vmcs(1)) ) {
         printk("Failed to allocate VMCS\n");
         return 0;
     }
 
-    phys_vmcs = (u64) virt_to_maddr(vmcs);
-
-    if (__vmxon(phys_vmcs)) {
+    if ( __vmxon(virt_to_maddr(vmcs)) ) {
         printk("VMXON failed\n");
         return 0;
     }
-
     printk("VMXON is done\n");
 
     vmx_save_init_msrs();
 
-    /* Setup HVM interfaces */
-    hvm_funcs.disable = stop_vmx;
-
-    hvm_funcs.initialize_guest_resources = vmx_initialize_guest_resources;
-    hvm_funcs.relinquish_guest_resources = vmx_relinquish_guest_resources;
-
-    hvm_funcs.store_cpu_guest_regs = vmx_store_cpu_guest_regs;
-    hvm_funcs.load_cpu_guest_regs = vmx_load_cpu_guest_regs;
-
-    hvm_funcs.realmode = vmx_realmode;
-    hvm_funcs.paging_enabled = vmx_paging_enabled;
-    hvm_funcs.instruction_length = vmx_instruction_length;
-    hvm_funcs.get_guest_ctrl_reg = vmx_get_ctrl_reg;
-
-    hvm_funcs.init_ap_context = vmx_init_ap_context;
+    vmx_init_hvm_funcs();
 
     hvm_enabled = 1;
 
diff -r fd6d12935b56 xen/include/asm-x86/hvm/support.h
--- a/xen/include/asm-x86/hvm/support.h	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/include/asm-x86/hvm/support.h	Tue Jul 04 18:32:01 2006 +0800
@@ -105,8 +105,8 @@ enum hval_bitmaps {
  * This works for both 32bit & 64bit eflags filteration
  * done in construct_init_vmc[sb]_guest()
  */
-#define HVM_EFLAGS_RESERVED_0          0xffc08028 /* bitmap for 0 */
-#define HVM_EFLAGS_RESERVED_1          0x00000002 /* bitmap for 1 */
+#define HVM_RFLAGS_RESERVED_0          0xffc08028 /* bitmap for 0 */
+#define HVM_RFLAGS_RESERVED_1          0x00000002 /* bitmap for 1 */
 
 #if HVM_DEBUG
 #define DBG_LEVEL_0                 (1 << 0)
diff -r fd6d12935b56 xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Tue Jul 04 18:32:01 2006 +0800
@@ -25,11 +25,8 @@
 #include <public/hvm/vmx_assist.h>
 
 extern int start_vmx(void);
-extern void stop_vmx(void);
 extern void vmcs_dump_vcpu(void);
-void vmx_final_setup_guest(struct vcpu *v);
-
-void vmx_enter_scheduler(void);
+extern void vmx_init_vmcs_config(void);
 
 enum {
     VMX_CPU_STATE_PAE_ENABLED=0,
@@ -45,8 +42,6 @@ struct vmcs_struct {
     u32 vmcs_revision_id;
     unsigned char data [0]; /* vmcs size is read from MSR */
 };
-
-extern int vmcs_size;
 
 enum {
     VMX_INDEX_MSR_LSTAR = 0,
@@ -63,6 +58,10 @@ struct vmx_msr_state {
     unsigned long msr_items[VMX_MSR_COUNT];
     unsigned long shadow_gs;
 };
+
+/* io bitmap is 4KBytes in size */
+#define IO_BITMAP_SIZE      0x1000
+#define IO_BITMAP_ORDER     (get_order_from_bytes(IO_BITMAP_SIZE))
 
 struct arch_vmx_struct {
     /* Virtual address of VMCS. */
@@ -101,7 +100,7 @@ struct arch_vmx_struct {
 
 void vmx_do_resume(struct vcpu *);
 
-struct vmcs_struct *vmx_alloc_vmcs(void);
+struct vmcs_struct *vmx_alloc_vmcs(int is_host_vmcs);
 void vmx_destroy_vmcs(struct vcpu *v);
 void vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);
diff -r fd6d12935b56 xen/include/asm-x86/hvm/vmx/vmx.h
--- a/xen/include/asm-x86/hvm/vmx/vmx.h	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h	Tue Jul 04 18:32:01 2006 +0800
@@ -199,15 +199,22 @@ extern unsigned int cpu_rev;
 #define MODRM_EAX_07    ".byte 0x38\n" /* [EAX], with reg/opcode: /7 */
 #define MODRM_EAX_ECX   ".byte 0xc1\n" /* [EAX], [ECX] */
 
-static inline void __vmptrld(u64 addr)
-{
+static inline int __vmptrld(u64 addr)
+{
+    int error = 0;
+
     __asm__ __volatile__ ( VMPTRLD_OPCODE
                            MODRM_EAX_06
-                           /* CF==1 or ZF==1 --> crash (ud2) */
-                           "ja 1f ; ud2 ; 1:\n"
-                           :
-                           : "a" (&addr) 
-                           : "memory");
+                           /* CF==1 or ZF==1 --> error */
+                           "ja 1f; dec %0; jc 1f; dec %0; 1:\n"
+                           : "=r"(error)
+                           : "a" (&addr), "0"(error)
+                           : "memory");
+
+    if ( error )
+        printk("vmptrld returned %d\n", error);
+
+    return error;
 }
 
 static inline void __vmptrst(u64 addr)
@@ -219,15 +226,22 @@ static inline void __vmptrst(u64 addr)
                            : "memory");
 }
 
-static inline void __vmpclear(u64 addr)
-{
+static inline int __vmclear(u64 addr)
+{
+    int error = 0;
+
     __asm__ __volatile__ ( VMCLEAR_OPCODE
                            MODRM_EAX_06
-                           /* CF==1 or ZF==1 --> crash (ud2) */
-                           "ja 1f ; ud2 ; 1:\n"
-                           :
-                           : "a" (&addr) 
-                           : "memory");
+                           /* CF==1 or ZF==1 --> error */
+                           "ja 1f; dec %0; jc 1f; dec %0; 1:\n"
+                           : "=r"(error)
+                           : "a" (&addr), "0"(error)
+                           : "memory");
+
+    if ( error )
+        printk("vmclear returned %d\n", error);
+
+    return error;
 }
 
 #define __vmread(x, ptr) ___vmread((x), (ptr), sizeof(*(ptr)))

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: credit scheduler issues in 64bit hypervisor

[Keir Fraser]

On 4 Jul 2006, at 13:00, Li, Xin B wrote:

>> OK, that's reasonable :-), I'll send out the patch.
>
> This patch is to move vmcs and IO bitmap allocation into
> vmx_initialize_guest_resources.
> And some necessary code rearrangement.

This patch messes with more stuff than it needs to. In particular, 
every place we VMCLEAR/VMPTRLD we expect the operation to succeed -- 
crashing on error is the most sensible thing to do. I don't see that 
returning error to the caller is helpful, and in fact you have callers 
who don't even check the error return. It keeps the interface to those 
functions simpler to have them return void and for them to bug out 
internally if they fail (as they currently do).

  -- Keir

RE: credit scheduler issues in 64bit hypervisor

[Li, Xin B]

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

>>> OK, that's reasonable :-), I'll send out the patch.
>>
>> This patch is to move vmcs and IO bitmap allocation into
>> vmx_initialize_guest_resources.
>> And some necessary code rearrangement.
>
>This patch messes with more stuff than it needs to.


OK, I splitted it. This patch only moves vmcs and IO bitmap allocation
into vmx_initialize_guest_resources.


> In particular, 
>every place we VMCLEAR/VMPTRLD we expect the operation to succeed -- 
>crashing on error is the most sensible thing to do. I don't see that 
>returning error to the caller is helpful, and in fact you have callers 
>who don't even check the error return. It keeps the interface to those 
>functions simpler to have them return void and for them to bug out 
>internally if they fail (as they currently do).
>

I really don't like crashing on such errors, but yes, this small changes
to VMCLEAR/VMPTRLD make no sense at this point.
For long term, we need follow IA32 SPEC to handle all the faliure VMX
instruction cases, basically we should crash the domain not the system,
and give some basic description.
I will send separate patches later, maybe after 3.0.3,
-Xin

[-- Attachment #2: con_vmcs.patch --]
[-- Type: application/octet-stream, Size: 13159 bytes --]

diff -r fd6d12935b56 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmcs.c	Wed Jul 05 04:27:09 2006 +0800
@@ -41,34 +41,62 @@
 #include <asm/shadow_64.h>
 #endif
 
-int vmcs_size;
-
-struct vmcs_struct *vmx_alloc_vmcs(void)
+static int vmcs_size;
+static int vmcs_order;
+static u32 vmcs_revision_id;
+
+void vmx_init_vmcs_config(void)
+{
+    u32 vmx_msr_low, vmx_msr_high;
+
+    if ( vmcs_size )
+        return;
+
+    rdmsr(MSR_IA32_VMX_BASIC_MSR, vmx_msr_low, vmx_msr_high);
+
+    vmcs_size = vmx_msr_high & 0x1fff;
+
+    vmcs_revision_id = vmx_msr_low;
+
+    vmcs_order = get_order_from_bytes(vmcs_size);
+}
+
+static inline void vmx_free_vmcs(struct vmcs_struct *vmcs)
+{
+    free_xenheap_pages(vmcs, vmcs_order);
+}
+
+struct vmcs_struct *vmx_alloc_vmcs(int is_host_vmcs)
 {
     struct vmcs_struct *vmcs;
-    u32 vmx_msr_low, vmx_msr_high;
-
-    rdmsr(MSR_IA32_VMX_BASIC_MSR, vmx_msr_low, vmx_msr_high);
-    vmcs_size = vmx_msr_high & 0x1fff;
-    vmcs = alloc_xenheap_pages(get_order_from_bytes(vmcs_size));
-    memset((char *)vmcs, 0, vmcs_size); /* don't remove this */
-
-    vmcs->vmcs_revision_id = vmx_msr_low;
+
+    if ( (vmcs = alloc_xenheap_pages(vmcs_order)) == NULL )
+    {
+        printk("Failed to allocate VMCS.\n");
+        return NULL;
+    }
+
+    memset(vmcs, 0, vmcs_size); /* don't remove this */
+
+    vmcs->vmcs_revision_id = vmcs_revision_id;
+
+    if ( is_host_vmcs )
+        return vmcs;
+
+    __vmpclear(virt_to_maddr(vmcs));
+
     return vmcs;
 }
 
-static void free_vmcs(struct vmcs_struct *vmcs)
-{
-    int order;
-
-    order = get_order_from_bytes(vmcs_size);
-    free_xenheap_pages(vmcs, order);
-}
-
 static void __vmx_clear_vmcs(void *info)
 {
     struct vcpu *v = info;
+
+    if ( !v->arch.hvm_vmx.launched )
+        return;
+
     __vmpclear(virt_to_maddr(v->arch.hvm_vmx.vmcs));
+
     v->arch.hvm_vmx.active_cpu = -1;
     v->arch.hvm_vmx.launched   = 0;
 }
@@ -131,8 +159,6 @@ static inline int construct_vmcs_control
 static inline int construct_vmcs_controls(struct arch_vmx_struct *arch_vmx)
 {
     int error = 0;
-    void *io_bitmap_a;
-    void *io_bitmap_b;
 
     error |= __vmwrite(PIN_BASED_VM_EXEC_CONTROL,
                        MONITOR_PIN_BASED_EXEC_CONTROLS);
@@ -141,19 +167,8 @@ static inline int construct_vmcs_control
 
     error |= __vmwrite(VM_ENTRY_CONTROLS, MONITOR_VM_ENTRY_CONTROLS);
 
-    /* need to use 0x1000 instead of PAGE_SIZE */
-    io_bitmap_a = (void*) alloc_xenheap_pages(get_order_from_bytes(0x1000));
-    io_bitmap_b = (void*) alloc_xenheap_pages(get_order_from_bytes(0x1000));
-    memset(io_bitmap_a, 0xff, 0x1000);
-    /* don't bother debug port access */
-    clear_bit(PC_DEBUG_PORT, io_bitmap_a);
-    memset(io_bitmap_b, 0xff, 0x1000);
-
-    error |= __vmwrite(IO_BITMAP_A, (u64) virt_to_maddr(io_bitmap_a));
-    error |= __vmwrite(IO_BITMAP_B, (u64) virt_to_maddr(io_bitmap_b));
-
-    arch_vmx->io_bitmap_a = io_bitmap_a;
-    arch_vmx->io_bitmap_b = io_bitmap_b;
+    error |= __vmwrite(IO_BITMAP_A, (u64)virt_to_maddr(arch_vmx->io_bitmap_a));
+    error |= __vmwrite(IO_BITMAP_B, (u64)virt_to_maddr(arch_vmx->io_bitmap_b));
 
     return error;
 }
@@ -429,67 +444,44 @@ static inline int construct_vmcs_host(vo
 }
 
 /*
- * Need to extend to support full virtualization.
+ * the working VMCS pointer has been set properly
+ * just before entering this function.
  */
 static int construct_vmcs(struct vcpu *v,
                           cpu_user_regs_t *regs)
 {
     struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
     int error;
-    long rc;
-
-    memset(arch_vmx, 0, sizeof(struct arch_vmx_struct));
-
-    spin_lock_init(&arch_vmx->vmcs_lock);
-
-    /*
-     * Create a new VMCS
-     */
-    if (!(arch_vmx->vmcs = vmx_alloc_vmcs())) {
-        printk("Failed to create a new VMCS\n");
-        return -ENOMEM;
-    }
-
-    __vmx_clear_vmcs(v);
-    vmx_load_vmcs(v);
-
-    if ((error = construct_vmcs_controls(arch_vmx))) {
-        printk("construct_vmcs: construct_vmcs_controls failed\n");
-        rc = -EINVAL;
-        goto err_out;
+
+    if ( (error = construct_vmcs_controls(arch_vmx)) ) {
+        printk("construct_vmcs: construct_vmcs_controls failed.\n");
+        return error;
     }
 
     /* host selectors */
-    if ((error = construct_vmcs_host())) {
-        printk("construct_vmcs: construct_vmcs_host failed\n");
-        rc = -EINVAL;
-        goto err_out;
+    if ( (error = construct_vmcs_host()) ) {
+        printk("construct_vmcs: construct_vmcs_host failed.\n");
+        return error;
     }
 
     /* guest selectors */
-    if ((error = construct_init_vmcs_guest(regs))) {
-        printk("construct_vmcs: construct_vmcs_guest failed\n");
-        rc = -EINVAL;
-        goto err_out;
-    }
-
-    if ((error |= __vmwrite(EXCEPTION_BITMAP,
-                            MONITOR_DEFAULT_EXCEPTION_BITMAP))) {
-        printk("construct_vmcs: setting Exception bitmap failed\n");
-        rc = -EINVAL;
-        goto err_out;
-    }
-
-    if (regs->eflags & EF_TF)
-        __vm_set_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
+    if ( (error = construct_init_vmcs_guest(regs)) ) {
+        printk("construct_vmcs: construct_vmcs_guest failed.\n");
+        return error;
+    }
+
+    if ( (error = __vmwrite(EXCEPTION_BITMAP,
+                            MONITOR_DEFAULT_EXCEPTION_BITMAP)) ) {
+        printk("construct_vmcs: setting exception bitmap failed.\n");
+        return error;
+    }
+
+    if ( regs->eflags & EF_TF )
+        error = __vm_set_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
     else
-        __vm_clear_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
-
-    return 0;
-
-err_out:
-    vmx_destroy_vmcs(v);
-    return rc;
+        error = __vm_clear_bit(EXCEPTION_BITMAP, EXCEPTION_BITMAP_DB);
+
+    return error;
 }
 
 void vmx_destroy_vmcs(struct vcpu *v)
@@ -501,14 +493,14 @@ void vmx_destroy_vmcs(struct vcpu *v)
 
     vmx_clear_vmcs(v);
 
-    free_vmcs(arch_vmx->vmcs);
+    free_xenheap_pages(arch_vmx->io_bitmap_a, IO_BITMAP_ORDER);
+    free_xenheap_pages(arch_vmx->io_bitmap_b, IO_BITMAP_ORDER);
+
+    arch_vmx->io_bitmap_a = NULL;
+    arch_vmx->io_bitmap_b = NULL;
+
+    vmx_free_vmcs(arch_vmx->vmcs);
     arch_vmx->vmcs = NULL;
-
-    free_xenheap_pages(arch_vmx->io_bitmap_a, get_order_from_bytes(0x1000));
-    arch_vmx->io_bitmap_a = NULL;
-
-    free_xenheap_pages(arch_vmx->io_bitmap_b, get_order_from_bytes(0x1000));
-    arch_vmx->io_bitmap_b = NULL;
 }
 
 void vm_launch_fail(unsigned long eflags)
@@ -547,19 +539,20 @@ void arch_vmx_do_resume(struct vcpu *v)
 
 void arch_vmx_do_launch(struct vcpu *v)
 {
-    int error;
     cpu_user_regs_t *regs = &current->arch.guest_context.user_regs;
 
-    error = construct_vmcs(v, regs);
-    if ( error < 0 )
+    vmx_load_vmcs(v);
+
+    if ( construct_vmcs(v, regs) < 0 )
     {
-        if (v->vcpu_id == 0) {
-            printk("Failed to construct a new VMCS for BSP.\n");
+        if ( v->vcpu_id == 0 ) {
+            printk("Failed to construct VMCS for BSP.\n");
         } else {
-            printk("Failed to construct a new VMCS for AP %d\n", v->vcpu_id);
+            printk("Failed to construct VMCS for AP %d.\n", v->vcpu_id);
         }
         domain_crash_synchronous();
     }
+
     vmx_do_launch(v);
     reset_stack_and_jump(vmx_asm_do_vmentry);
 }
diff -r fd6d12935b56 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed Jul 05 04:27:09 2006 +0800
@@ -54,7 +54,7 @@ static void vmx_ctxt_switch_from(struct 
 static void vmx_ctxt_switch_from(struct vcpu *v);
 static void vmx_ctxt_switch_to(struct vcpu *v);
 
-void vmx_final_setup_guest(struct vcpu *v)
+static int vmx_initialize_guest_resources(struct vcpu *v)
 {
     v->arch.schedule_tail    = arch_vmx_do_launch;
     v->arch.ctxt_switch_from = vmx_ctxt_switch_from;
@@ -64,10 +64,41 @@ void vmx_final_setup_guest(struct vcpu *
     {
         struct domain *d = v->domain;
         struct vcpu *vc;
-
-        /* Initialize monitor page table */
-        for_each_vcpu(d, vc)
+        void *io_bitmap_a, *io_bitmap_b;
+
+        for_each_vcpu ( d, vc ) {
+            /* Initialize monitor page table */
             vc->arch.monitor_table = pagetable_null();
+
+            memset(&vc->arch.hvm_vmx, 0, sizeof(struct arch_vmx_struct));
+
+            /* Create VMCS for each vcpu */
+            if ( (vc->arch.hvm_vmx.vmcs = vmx_alloc_vmcs(0)) == NULL ) {
+                printk("Failed to create VMCS for vcpu %d.\n", vc->vcpu_id);
+                return 0;
+            }
+
+            spin_lock_init(&vc->arch.hvm_vmx.vmcs_lock);
+
+            if ( (io_bitmap_a = alloc_xenheap_pages(IO_BITMAP_ORDER)) == NULL ) {
+                printk("Failed to allocate io bitmap a for vcpu %d.\n", vc->vcpu_id);
+                return 0;
+            }
+
+            if ( (io_bitmap_b = alloc_xenheap_pages(IO_BITMAP_ORDER)) == NULL ) {
+                printk("Failed to allocate io bitmap b for vcpu %d.\n", vc->vcpu_id);
+                return 0;
+            }
+
+            memset(io_bitmap_a, 0xff, 0x1000);
+            memset(io_bitmap_b, 0xff, 0x1000);
+
+            /* don't bother debug port access */
+            clear_bit(PC_DEBUG_PORT, io_bitmap_a);
+
+            vc->arch.hvm_vmx.io_bitmap_a = io_bitmap_a;
+            vc->arch.hvm_vmx.io_bitmap_b = io_bitmap_b;
+        }
 
         /*
          * Required to do this once per domain
@@ -82,6 +113,8 @@ void vmx_final_setup_guest(struct vcpu *
                            SHM_enable|SHM_refcounts|
                            SHM_translate|SHM_external|SHM_wr_pt_pte);
     }
+
+    return 1;
 }
 
 static void vmx_relinquish_guest_resources(struct domain *d)
@@ -90,9 +123,9 @@ static void vmx_relinquish_guest_resourc
 
     for_each_vcpu ( d, v )
     {
+        vmx_destroy_vmcs(v);
         if ( !test_bit(_VCPUF_initialised, &v->vcpu_flags) )
             continue;
-        vmx_destroy_vmcs(v);
         free_monitor_pagetable(v);
         kill_timer(&v->arch.hvm_vmx.hlt_timer);
         if ( hvm_apic_support(v->domain) && (VLAPIC(v) != NULL) )
@@ -442,12 +475,6 @@ void stop_vmx(void)
 {
     if (read_cr4() & X86_CR4_VMXE)
         __vmxoff();
-}
-
-int vmx_initialize_guest_resources(struct vcpu *v)
-{
-    vmx_final_setup_guest(v);
-    return 1;
 }
 
 void vmx_migrate_timers(struct vcpu *v)
@@ -638,16 +665,13 @@ static int check_vmx_controls(u32 ctrls,
 
 int start_vmx(void)
 {
+    u32 eax, edx;
     struct vmcs_struct *vmcs;
-    u32 ecx;
-    u32 eax, edx;
-    u64 phys_vmcs;      /* debugging */
 
     /*
      * Xen does not fill x86_capability words except 0.
      */
-    ecx = cpuid_ecx(1);
-    boot_cpu_data.x86_capability[4] = ecx;
+    boot_cpu_data.x86_capability[4] = cpuid_ecx(1);
 
     if (!(test_bit(X86_FEATURE_VMXE, &boot_cpu_data.x86_capability)))
         return 0;
@@ -659,8 +683,7 @@ int start_vmx(void)
             printk("VMX disabled by Feature Control MSR.\n");
             return 0;
         }
-    }
-    else {
+    } else {
         wrmsr(IA32_FEATURE_CONTROL_MSR,
               IA32_FEATURE_CONTROL_MSR_LOCK |
               IA32_FEATURE_CONTROL_MSR_ENABLE_VMXON, 0);
@@ -681,18 +704,17 @@ int start_vmx(void)
 
     set_in_cr4(X86_CR4_VMXE);   /* Enable VMXE */
 
-    if (!(vmcs = vmx_alloc_vmcs())) {
+    vmx_init_vmcs_config();
+
+    if ( !(vmcs = vmx_alloc_vmcs(1)) ) {
         printk("Failed to allocate VMCS\n");
         return 0;
     }
 
-    phys_vmcs = (u64) virt_to_maddr(vmcs);
-
-    if (__vmxon(phys_vmcs)) {
+    if ( __vmxon(virt_to_maddr(vmcs)) ) {
         printk("VMXON failed\n");
         return 0;
     }
-
     printk("VMXON is done\n");
 
     vmx_save_init_msrs();
diff -r fd6d12935b56 xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Mon Jul 03 16:07:20 2006 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Wed Jul 05 04:27:09 2006 +0800
@@ -27,9 +27,7 @@ extern int start_vmx(void);
 extern int start_vmx(void);
 extern void stop_vmx(void);
 extern void vmcs_dump_vcpu(void);
-void vmx_final_setup_guest(struct vcpu *v);
-
-void vmx_enter_scheduler(void);
+extern void vmx_init_vmcs_config(void);
 
 enum {
     VMX_CPU_STATE_PAE_ENABLED=0,
@@ -45,8 +43,6 @@ struct vmcs_struct {
     u32 vmcs_revision_id;
     unsigned char data [0]; /* vmcs size is read from MSR */
 };
-
-extern int vmcs_size;
 
 enum {
     VMX_INDEX_MSR_LSTAR = 0,
@@ -63,6 +59,10 @@ struct vmx_msr_state {
     unsigned long msr_items[VMX_MSR_COUNT];
     unsigned long shadow_gs;
 };
+
+/* io bitmap is 4KBytes in size */
+#define IO_BITMAP_SIZE      0x1000
+#define IO_BITMAP_ORDER     (get_order_from_bytes(IO_BITMAP_SIZE))
 
 struct arch_vmx_struct {
     /* Virtual address of VMCS. */
@@ -101,7 +101,7 @@ struct arch_vmx_struct {
 
 void vmx_do_resume(struct vcpu *);
 
-struct vmcs_struct *vmx_alloc_vmcs(void);
+struct vmcs_struct *vmx_alloc_vmcs(int is_host_vmcs);
 void vmx_destroy_vmcs(struct vcpu *v);
 void vmx_vmcs_enter(struct vcpu *v);
 void vmx_vmcs_exit(struct vcpu *v);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: credit scheduler issues in 64bit hypervisor

[Keir Fraser]

On 5 Jul 2006, at 02:51, Li, Xin B wrote:

> I really don't like crashing on such errors, but yes, this small 
> changes
> to VMCLEAR/VMPTRLD make no sense at this point.
> For long term, we need follow IA32 SPEC to handle all the faliure VMX
> instruction cases, basically we should crash the domain not the system,
> and give some basic description.

Are there any cases where the domain itself can cause those 
instructions to fail? I agree that we should have a function that reads 
and decodes the reason for failure, and maybe also dumps other 
interesting state, but BUG still seems the right failure mode to me.

  -- Keir