[PATCH] KVM: is_long_mode should check for EFER_LMA

[PATCH] KVM: is_long_mode should check for EFER_LMA

[Amit Shah]

>From bfed574c93b36a19e2976ddcaae7939dd6c6fc41 Mon Sep 17 00:00:00 2001
From: Amit Shah <amit.shah-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
Date: Sat, 3 Nov 2007 02:38:00 +0530
Subject: [PATCH] KVM: is_long_mode should check for EFER_LMA

is_long_mode currently checks the LongModeEnable bit in
EFER instead of the LongModeActive bit. This should work
for most cases, but for some broken implementations that
set the LME bit before enabling PAE in CR4 to enter long
mode.

This is noticed on a solaris guest on an AMD host (but might
not be specific to AMD).

Signed-off-by: Amit Shah <amit.shah-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
---
 drivers/kvm/x86.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/kvm/x86.h b/drivers/kvm/x86.h
index 663b822..5c10761 100644
--- a/drivers/kvm/x86.h
+++ b/drivers/kvm/x86.h
@@ -104,7 +104,7 @@ static inline int kvm_mmu_reload(struct kvm_vcpu *vcpu)
 static inline int is_long_mode(struct kvm_vcpu *vcpu)
 {
 #ifdef CONFIG_X86_64
-	return vcpu->shadow_efer & EFER_LME;
+	return vcpu->shadow_efer & EFER_LMA;
 #else
 	return 0;
 #endif
-- 
1.4.4.2


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH] KVM: is_long_mode should check for EFER_LMA

[Avi Kivity]

Amit Shah wrote:
> >From bfed574c93b36a19e2976ddcaae7939dd6c6fc41 Mon Sep 17 00:00:00 2001
> From: Amit Shah <amit.shah-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> Date: Sat, 3 Nov 2007 02:38:00 +0530
> Subject: [PATCH] KVM: is_long_mode should check for EFER_LMA
>
> is_long_mode currently checks the LongModeEnable bit in
> EFER instead of the LongModeActive bit. This should work
> for most cases, but for some broken implementations that
> set the LME bit before enabling PAE in CR4 to enter long
> mode.
>
> This is noticed on a solaris guest on an AMD host (but might
> not be specific to AMD).
>
>   

Patch looks good and obviously correct to me.  But:

- why do you say 'broken implementations'?  do you mean 'broken 
guests'?  I think that behavior is as specified.
- what guest action triggered the failure? (e.g. mov cr4, or what?)
- please supply a patch to the testsuite that adds a regression test for 
this issue.


-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH] KVM: is_long_mode should check for EFER_LMA

[Amit Shah]

On Sunday 04 November 2007 12:47:32 Avi Kivity wrote:
> Amit Shah wrote:
> > >From bfed574c93b36a19e2976ddcaae7939dd6c6fc41 Mon Sep 17 00:00:00 2001
> >
> > From: Amit Shah <amit.shah-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> > Date: Sat, 3 Nov 2007 02:38:00 +0530
> > Subject: [PATCH] KVM: is_long_mode should check for EFER_LMA
> >
> > is_long_mode currently checks the LongModeEnable bit in
> > EFER instead of the LongModeActive bit. This should work
> > for most cases, but for some broken implementations that
> > set the LME bit before enabling PAE in CR4 to enter long
> > mode.
> >
> > This is noticed on a solaris guest on an AMD host (but might
> > not be specific to AMD).
>
> Patch looks good and obviously correct to me.  But:
>
> - why do you say 'broken implementations'?  do you mean 'broken
> guests'?  I think that behavior is as specified.

Yes, broken guest implementations. It was pointed out to me that solaris does 
set PAE before setting LME, but possibly in the dated version of solaris that 
I have, this wasn't the case.

> - what guest action triggered the failure? (e.g. mov cr4, or what?)

mov reg, cr

That triggers realmode_set_cr -> set_cr4 ->

        if (is_long_mode(vcpu)) {
                if (!(cr4 & X86_CR4_PAE)) {
                        printk(KERN_DEBUG "set_cr4: #GP, clearing PAE while "
                               "in long mode\n");
                        inject_gp(vcpu);
                        return;
                }


> - please supply a patch to the testsuite that adds a regression test for
> this issue.

I'm not sure how the test suite works. I took a brief look at user/test/x86/, 
but can't find what's to be done there.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

Re: [PATCH] KVM: is_long_mode should check for EFER_LMA

[Avi Kivity]

Amit Shah wrote:
> On Sunday 04 November 2007 12:47:32 Avi Kivity wrote:
>   
>> Amit Shah wrote:
>>     
>>> >From bfed574c93b36a19e2976ddcaae7939dd6c6fc41 Mon Sep 17 00:00:00 2001
>>>
>>> From: Amit Shah <amit.shah-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
>>> Date: Sat, 3 Nov 2007 02:38:00 +0530
>>> Subject: [PATCH] KVM: is_long_mode should check for EFER_LMA
>>>
>>> is_long_mode currently checks the LongModeEnable bit in
>>> EFER instead of the LongModeActive bit. This should work
>>> for most cases, but for some broken implementations that
>>> set the LME bit before enabling PAE in CR4 to enter long
>>> mode.
>>>
>>> This is noticed on a solaris guest on an AMD host (but might
>>> not be specific to AMD).
>>>       
>> Patch looks good and obviously correct to me.  But:
>>
>> - why do you say 'broken implementations'?  do you mean 'broken
>> guests'?  I think that behavior is as specified.
>>     
>
> Yes, broken guest implementations. It was pointed out to me that solaris does 
> set PAE before setting LME, but possibly in the dated version of solaris that 
> I have, this wasn't the case.
>
>   
>> - what guest action triggered the failure? (e.g. mov cr4, or what?)
>>     
>
> mov reg, cr
>
> That triggers realmode_set_cr -> set_cr4 ->
>
>         if (is_long_mode(vcpu)) {
>                 if (!(cr4 & X86_CR4_PAE)) {
>                         printk(KERN_DEBUG "set_cr4: #GP, clearing PAE while "
>                                "in long mode\n");
>                         inject_gp(vcpu);
>                         return;
>                 }
>
>
>   

The Intel manual says:

> Prior to activating IA-32e mode, PAE must be enabled by setting 
> CR4.PAE = 1. PAE expands
> the size of page-directory entries (PDE) and page-table entries (PTE) 
> from 32 bits to 64 bits.
> This change is made to support physical-address sizes of greater than 
> 32 bits. An attempt to acti-
> vate IA-32e mode prior to enabling PAE results in a general-protection 
> exception, #GP.

My reading is that activating long mode is the setting of cr0.pg that 
sets LMA, as opposed to enabling long mode (enabling LME).  The AMD 
manual is similar.  So both the patch and the guest are correct.

But, this changes a huge number of calls to is_long_mode() (most of them 
for the better, because we are usually interested if it is active, not 
the silly enabled thing).  Please go over all callers and double check.  
At least the entry sequence should be a bit different.

>> - please supply a patch to the testsuite that adds a regression test for
>> this issue.
>>     
>
> I'm not sure how the test suite works. I took a brief look at user/test/x86/, 
> but can't find what's to be done there.
>   

You need to add a file, similar to access.c (though much much shorter) 
that performs the same series of instructions.  Trap the #GP handler so 
you can set some variable there (access.c does something similar).

(access.c checks some combinations of pte flags and guest access methods 
against expected results, expected exception behavior, and expected 
processor-written pte flags)

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/