[PATCH][SVM] HVM fix for SWINT event injection

[PATCH][SVM] HVM fix for SWINT event injection

[Woller, Thomas]

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

This patch resolves issues with booting/installing AMD-V HVM guests,
including 32bit win2000, and 64bit vista/longhorn guests on
xen-unstable.

This patch modifies the AMD-V reinjection logic (intr.c) to not inject
any SWINT that the processor was unable to execute (exitintinfo valid w/
type=4).  The processor indicates that a SWINT was unsuccessful by
filling in the exitintinfo field on an #PF intercept, during a world
switch from SVM/guest mode.  

There were several cases observed during boot of win2000 and
vista/longhorn 64b:

Win2000 -
A VMEXIT_EXCEPTION_PF (#PF intercept) occurs during execution of an "Int
10" instruction.  
The exitintinfo field is properly filled out by the processor containing
type=4/vector=0x10.
The call into paging_fault() to handle the #PF is not resolved
(not_a_shadow_fault/0 returned), so the eventinj information is filled
out in the #PF vmexit handler (to inject later).
The svm_intr_assist() code is executed with both the eventinj, and the
exitininfo fields filled out.
In this case the #PF needs to be injected, ignoring the SWINT in the
exitintinfo fields.

Vista/Longhorn 64b -
Similar to above, but the call into paging_fault() is resolved
(EXCRET_fault_fixed returned), and in this case the SWINT in exitintifo
should also be ignored.

Please apply to xen-unstable.
Applies cleanly to c/s 15651.
Signed-off-by Tom Woller <thomas.woller@amd.com> 

Note that HVM guests will not boot on AMD-V with xen-staging.hg c/s
15652.  
Keir, do you want me to take a look at staging c/s 15652 on AMD-V w/
HVM?

  --Tom

thomas.woller@amd.com  +1-512-602-0059
AMD Corporation - Operating Systems Research Center
5204 E. Ben White Blvd. UBC1
Austin, Texas 78741


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

diff -r 5682f899c7ae xen/arch/x86/hvm/svm/intr.c
--- a/xen/arch/x86/hvm/svm/intr.c	Fri Jul 27 09:06:58 2007 +0100
+++ b/xen/arch/x86/hvm/svm/intr.c	Thu Jul 26 23:14:42 2007 -0500
@@ -93,7 +93,7 @@ asmlinkage void svm_intr_assist(void)
      * occurs (e.g., due to lack of shadow mapping of guest IDT or guest-kernel
      * stack).
      */
-    if ( vmcb->exitintinfo.fields.v )
+    if ( vmcb->exitintinfo.fields.v && (vmcb->exitintinfo.fields.type != 4) )
     {
         vmcb->eventinj = vmcb->exitintinfo;
         vmcb->exitintinfo.bytes = 0;

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

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

Re: [PATCH][SVM] HVM fix for SWINT event injection

[Keir Fraser]

On 27/7/07 16:31, "Woller, Thomas" <thomas.woller@amd.com> wrote:

> Please apply to xen-unstable.
> Applies cleanly to c/s 15651.
> Signed-off-by Tom Woller <thomas.woller@amd.com>

The right thing to do here is copy the exitintinfo->eventinj only if
eventinj.fields.v==0. And to add a comment that in future the two events
should be 'merged' in the architecturally correct manner (e.g., turned into
#DF in some cases).

At least I *think* this is the right thing to do -- but now I think about
it, is it actually ever okay to copy a SWINT from exitintinfo into eventinj?
At the time the #PF vmexit happens, will EIP have been incremented across
the SWINT instruction? If not, does eventinj of a SWINT correctly cause EIP
to be incremented?

> Note that HVM guests will not boot on AMD-V with xen-staging.hg c/s
> 15652.  
> Keir, do you want me to take a look at staging c/s 15652 on AMD-V w/
> HVM?

Yes please!

 Thanks,
 Keir

RE: [PATCH][SVM] HVM fix for SWINT event injection

[Woller, Thomas]

> The right thing to do here is copy the exitintinfo->eventinj 
> only if eventinj.fields.v==0. And to add a comment that in 
> future the two events should be 'merged' in the 
> architecturally correct manner (e.g., turned into #DF in some cases).
I agree with both in principle.

Overall, if the eventinj field is filled when calling into
svm_intr_assist() then this event/exception should most likely take
precedence over exitintinfo.  I did some profiling on some HVM guests
(not all), but in normal conditions.  During these tests I only saw both
fields filled, *when* SWINT is in the exitintinfo field, and with a #PF
in the eventinj field. Not very elaborate testing though, just booting
HVM guests and running some rudimentary tests.

What I did see though was that sometimes condition that 
1) exitintinfo field had SWINT information, and
2) the eventinj.fields.v was not true (i.e. paging_fault() handled the
VMEXIT_EXECEPTION_PF).  
And in this case, I discovered that we still do not want to move the
SWINT exitintinfo over to eventinj - just let the instruction start
re-execution.

> 
> At least I *think* this is the right thing to do -- but now I 
> think about it, is it actually ever okay to copy a SWINT from 
> exitintinfo into eventinj?
I believe the answer to this is no.

> At the time the #PF vmexit happens, will EIP have been 
> incremented across the SWINT instruction? 
No, FAI can find out, but I was wondering this myself for all cases.  I
did not observe any cases with SWINT in exitintinfo, and the EIP *past*
the "int NN"/SWINT instruction.  So, just re-executing the SWINT
instruction seems to be acceptable here.

> If not, does 
> eventinj of a SWINT correctly cause EIP to be incremented?
Reinjecting the SWINT found in exitintinfo seems to cause the SWINT to
be executed twice (not good).  So, just never injecting SWINT at this
time seems to be the best approach.  

Now, then, looking at the guest instruction in this case, might be
useful - and if the eip is not pointing to the "int nn" instruction in
question, then perhaps injecting the SWINT?  I have not seen this case
in practice though.. If exitintinfo has SWINT, then the next instruction
is the unexecuted "int nn" instruction.  but if there is a series of
SWINT instructions (can this happen?):
Int 10
Int 10
Int 10
Then, no telling how to tell if the instruction was executed or that the
next instruction is same as previous.
So, I don't think that looking at the next guest instruction is worth
any effort at this time.

So, bottom line is that the code most likely needs:
1) if eventinj is filled, then do not move over info from exitintinfo
2) never inject SWINT from exitintinfo (allow instruction to start up
execution again)

I will test more HVM guests with the instrumented the code to determine
if there is an occurances with both exitintinfo AND eventinj filled out.
I don't believe that we'll see any in usual testing, but do need to
handle "merging" at some point in the future.


> 
> > Note that HVM guests will not boot on AMD-V with xen-staging.hg c/s 
> > 15652.
> > Keir, do you want me to take a look at staging c/s 15652 on AMD-V w/
HVM?
> 
> Yes please!
I'll take a look at it at the guests that don't boot and see what's up.

tom
 

Re: [PATCH][SVM] HVM fix for SWINT event injection

[Keir Fraser]

Thanks for the detailed answer! It sounds like never injecting a SWINT is
the right answer -- if we ever did, we could never tell whether
exitintinfo==swint because of INTn execution in guest context (in which case
RIP points at the SWINT instruction) or because of event injection (in which
case RIP points past the SWINT instruction). Intel increments RIP on
successful SWINT injection, which avoids this nasty edge case.

What I'll do is reorder SVM's intr.c core code to be the same as for VMX
(it's currently bogusly different) but with the added constraint that we do
not propagate exitintinfo.swint into eventinj. I'll add a comment to explain
why.

 -- Keir

On 30/7/07 20:06, "Woller, Thomas" <thomas.woller@amd.com> wrote:

>> The right thing to do here is copy the exitintinfo->eventinj
>> only if eventinj.fields.v==0. And to add a comment that in
>> future the two events should be 'merged' in the
>> architecturally correct manner (e.g., turned into #DF in some cases).
> I agree with both in principle.
> 
> Overall, if the eventinj field is filled when calling into
> svm_intr_assist() then this event/exception should most likely take
> precedence over exitintinfo.  I did some profiling on some HVM guests
> (not all), but in normal conditions.  During these tests I only saw both
> fields filled, *when* SWINT is in the exitintinfo field, and with a #PF
> in the eventinj field. Not very elaborate testing though, just booting
> HVM guests and running some rudimentary tests.
> 
> What I did see though was that sometimes condition that
> 1) exitintinfo field had SWINT information, and
> 2) the eventinj.fields.v was not true (i.e. paging_fault() handled the
> VMEXIT_EXECEPTION_PF).
> And in this case, I discovered that we still do not want to move the
> SWINT exitintinfo over to eventinj - just let the instruction start
> re-execution.
> 
>> 
>> At least I *think* this is the right thing to do -- but now I
>> think about it, is it actually ever okay to copy a SWINT from
>> exitintinfo into eventinj?
> I believe the answer to this is no.
> 
>> At the time the #PF vmexit happens, will EIP have been
>> incremented across the SWINT instruction?
> No, FAI can find out, but I was wondering this myself for all cases.  I
> did not observe any cases with SWINT in exitintinfo, and the EIP *past*
> the "int NN"/SWINT instruction.  So, just re-executing the SWINT
> instruction seems to be acceptable here.
> 
>> If not, does 
>> eventinj of a SWINT correctly cause EIP to be incremented?
> Reinjecting the SWINT found in exitintinfo seems to cause the SWINT to
> be executed twice (not good).  So, just never injecting SWINT at this
> time seems to be the best approach.
> 
> Now, then, looking at the guest instruction in this case, might be
> useful - and if the eip is not pointing to the "int nn" instruction in
> question, then perhaps injecting the SWINT?  I have not seen this case
> in practice though.. If exitintinfo has SWINT, then the next instruction
> is the unexecuted "int nn" instruction.  but if there is a series of
> SWINT instructions (can this happen?):
> Int 10
> Int 10
> Int 10
> Then, no telling how to tell if the instruction was executed or that the
> next instruction is same as previous.
> So, I don't think that looking at the next guest instruction is worth
> any effort at this time.
> 
> So, bottom line is that the code most likely needs:
> 1) if eventinj is filled, then do not move over info from exitintinfo
> 2) never inject SWINT from exitintinfo (allow instruction to start up
> execution again)
> 
> I will test more HVM guests with the instrumented the code to determine
> if there is an occurances with both exitintinfo AND eventinj filled out.
> I don't believe that we'll see any in usual testing, but do need to
> handle "merging" at some point in the future.
> 
> 
>> 
>>> Note that HVM guests will not boot on AMD-V with xen-staging.hg c/s
>>> 15652.
>>> Keir, do you want me to take a look at staging c/s 15652 on AMD-V w/
> HVM?
>> 
>> Yes please!
> I'll take a look at it at the guests that don't boot and see what's up.
> 
> tom
>  
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: [PATCH][SVM] HVM fix for SWINT event injection

[Woller, Thomas]

> What I'll do is reorder SVM's intr.c core code to be the same 
> as for VMX (it's currently bogusly different) but with the 
> added constraint that we do not propagate exitintinfo.swint 
> into eventinj. I'll add a comment to explain why.
Ok. Shoot me the patch first if you have time, I'll test with all the
HVM guests I have on hand.

Re: 15652
15658 looks good initially, appreciate the quick fixes.

tom 

> -----Original Message-----
> From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
> Sent: Monday, July 30, 2007 4:17 PM
> To: Woller, Thomas; xen-devel@lists.xensource.com
> Subject: Re: [Xen-devel] [PATCH][SVM] HVM fix for SWINT event 
> injection
> 
> Thanks for the detailed answer! It sounds like never 
> injecting a SWINT is the right answer -- if we ever did, we 
> could never tell whether exitintinfo==swint because of INTn 
> execution in guest context (in which case RIP points at the 
> SWINT instruction) or because of event injection (in which 
> case RIP points past the SWINT instruction). Intel increments 
> RIP on successful SWINT injection, which avoids this nasty edge case.
> 
> What I'll do is reorder SVM's intr.c core code to be the same 
> as for VMX (it's currently bogusly different) but with the 
> added constraint that we do not propagate exitintinfo.swint 
> into eventinj. I'll add a comment to explain why.
> 
>  -- Keir
> 
> On 30/7/07 20:06, "Woller, Thomas" <thomas.woller@amd.com> wrote:
> 
> >> The right thing to do here is copy the 
> exitintinfo->eventinj only if 
> >> eventinj.fields.v==0. And to add a comment that in future the two 
> >> events should be 'merged' in the architecturally correct manner 
> >> (e.g., turned into #DF in some cases).
> > I agree with both in principle.
> > 
> > Overall, if the eventinj field is filled when calling into
> > svm_intr_assist() then this event/exception should most likely take 
> > precedence over exitintinfo.  I did some profiling on some 
> HVM guests 
> > (not all), but in normal conditions.  During these tests I only saw 
> > both fields filled, *when* SWINT is in the exitintinfo 
> field, and with 
> > a #PF in the eventinj field. Not very elaborate testing 
> though, just 
> > booting HVM guests and running some rudimentary tests.
> > 
> > What I did see though was that sometimes condition that
> > 1) exitintinfo field had SWINT information, and
> > 2) the eventinj.fields.v was not true (i.e. paging_fault() 
> handled the 
> > VMEXIT_EXECEPTION_PF).
> > And in this case, I discovered that we still do not want to 
> move the 
> > SWINT exitintinfo over to eventinj - just let the instruction start 
> > re-execution.
> > 
> >> 
> >> At least I *think* this is the right thing to do -- but 
> now I think 
> >> about it, is it actually ever okay to copy a SWINT from 
> exitintinfo 
> >> into eventinj?
> > I believe the answer to this is no.
> > 
> >> At the time the #PF vmexit happens, will EIP have been incremented 
> >> across the SWINT instruction?
> > No, FAI can find out, but I was wondering this myself for 
> all cases.  
> > I did not observe any cases with SWINT in exitintinfo, and the EIP 
> > *past* the "int NN"/SWINT instruction.  So, just re-executing the 
> > SWINT instruction seems to be acceptable here.
> > 
> >> If not, does
> >> eventinj of a SWINT correctly cause EIP to be incremented?
> > Reinjecting the SWINT found in exitintinfo seems to cause 
> the SWINT to 
> > be executed twice (not good).  So, just never injecting 
> SWINT at this 
> > time seems to be the best approach.
> > 
> > Now, then, looking at the guest instruction in this case, might be 
> > useful - and if the eip is not pointing to the "int nn" 
> instruction in 
> > question, then perhaps injecting the SWINT?  I have not 
> seen this case 
> > in practice though.. If exitintinfo has SWINT, then the next 
> > instruction is the unexecuted "int nn" instruction.  but if 
> there is a 
> > series of SWINT instructions (can this happen?):
> > Int 10
> > Int 10
> > Int 10
> > Then, no telling how to tell if the instruction was 
> executed or that 
> > the next instruction is same as previous.
> > So, I don't think that looking at the next guest 
> instruction is worth 
> > any effort at this time.
> > 
> > So, bottom line is that the code most likely needs:
> > 1) if eventinj is filled, then do not move over info from 
> exitintinfo
> > 2) never inject SWINT from exitintinfo (allow instruction 
> to start up 
> > execution again)
> > 
> > I will test more HVM guests with the instrumented the code to 
> > determine if there is an occurances with both exitintinfo 
> AND eventinj filled out.
> > I don't believe that we'll see any in usual testing, but do need to 
> > handle "merging" at some point in the future.
> > 
> > 
> >> 
> >>> Note that HVM guests will not boot on AMD-V with 
> xen-staging.hg c/s 
> >>> 15652.
> >>> Keir, do you want me to take a look at staging c/s 15652 
> on AMD-V w/
> > HVM?
> >> 
> >> Yes please!
> > I'll take a look at it at the guests that don't boot and 
> see what's up.
> > 
> > tom
> >  
> > 
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> 
> 
> 
> 

Re: [PATCH][SVM] HVM fix for SWINT event injection

[Keir Fraser]

On 31/7/07 16:30, "Woller, Thomas" <thomas.woller@amd.com> wrote:

>> What I'll do is reorder SVM's intr.c core code to be the same
>> as for VMX (it's currently bogusly different) but with the
>> added constraint that we do not propagate exitintinfo.swint
>> into eventinj. I'll add a comment to explain why.
> Ok. Shoot me the patch first if you have time, I'll test with all the
> HVM guests I have on hand.

http://xenbits.xensource.com/staging/xen-unstable.hg, changeset 15662. It's
rather large, and only run-tested on VT. :-) I'll try and do some SVM
testing at home, but it'd be great if you can give it a spin.

My only question is whether vmcb->exitintinfo is written on every vmexit,
even those where no event was being delivered. I assumed so, and hence do
not bother to zap the field before vmentry, but that'll need adding back in
if I'm wrong.

> Re: 15652
> 15658 looks good initially, appreciate the quick fixes.

Yes, that was an embarrassing bug and not in Eric Liu's original patch!

 -- Keir