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

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

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/