RE: [PATCH 07/14] Nested Virtualization: trap

RE: [PATCH 07/14] Nested Virtualization: trap

[Dong, Eddie]

> 
> +int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned
> long cr2) +{
> +    uint64_t exitcode;
> +    bool_t is_intercepted;
> +    struct vcpu *v = current;
> +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
> +
> +    if ( !nestedhvm_enabled(v->domain) ) {
> +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> +        return 0;
> +    }

If it is not nested, we go from here to vendor specific injection code.
If it is nested, I think we'd better to go to another vendor specific handler too.
We may have to resort to more vendor specific information within the process to handle possible different HW stuff. If above suggestion is reasonable, then I think we don't need this wrapper change because we can easily extend current vendor speifc inject_exception to support both nested case and non-nested case.

Thx, Eddie

Re: [PATCH 07/14] Nested Virtualization: trap

[Tim Deegan]

At 03:44 +0100 on 19 Aug (1282189499), Dong, Eddie wrote:
> > 
> > +int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned
> > long cr2) +{
> > +    uint64_t exitcode;
> > +    bool_t is_intercepted;
> > +    struct vcpu *v = current;
> > +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
> > +
> > +    if ( !nestedhvm_enabled(v->domain) ) {
> > +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> > +        return 0;
> > +    }
> 
> If it is not nested, we go from here to vendor specific injection code.
> If it is nested, I think we'd better to go to another vendor specific
> handler too.

That's exactly what this wrapper does.  It's basically:
 if ( in L2 and L1 intercepts )
    force vmexit
 else
    inject directly.

It calls out to arch-specific code to do the intercept check and the
vmexit.  It could be tidied up a bit and the interfaces could change but
this looks like about the least amount of sharing there could be on
this path.  I can't see anything objectionable.

Tim.

> We may have to resort to more vendor specific information within the
> process to handle possible different HW stuff. If above suggestion is
> reasonable, then I think we don't need this wrapper change because we
> can easily extend current vendor speifc inject_exception to support
> both nested case and non-nested case.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 07/14] Nested Virtualization: trap

[Christoph Egger]

On Thursday 19 August 2010 10:35:52 Tim Deegan wrote:
> At 03:44 +0100 on 19 Aug (1282189499), Dong, Eddie wrote:
> > > +int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned
> > > long cr2) +{
> > > +    uint64_t exitcode;
> > > +    bool_t is_intercepted;
> > > +    struct vcpu *v = current;
> > > +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
> > > +
> > > +    if ( !nestedhvm_enabled(v->domain) ) {
> > > +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> > > +        return 0;
> > > +    }
> >
> > If it is not nested, we go from here to vendor specific injection code.
> > If it is nested, I think we'd better to go to another vendor specific
> > handler too.
>
> That's exactly what this wrapper does.  It's basically:
>  if ( in L2 and L1 intercepts )
>     force vmexit
>  else
>     inject directly.
>
> It calls out to arch-specific code to do the intercept check and the
> vmexit.  It could be tidied up a bit and the interfaces could change but
> this looks like about the least amount of sharing there could be on
> this path.  I can't see anything objectionable.

Correct. The key to make this possible is the generic exitcode
mechanism.

Eddie: In an other mail you said, that the generic exitcode is
overcomplicated. I have the impression that you do not
realise that the unification of the exitcodes makes that much
code shareable at all. No, I do not unified *all* exitcodes - I don't know
if that is even possible.
I only unified a subset that is actually used in the generic code.

(I also have the feeling Keir didn't realize that either given
to his last comments to patch 5/15)

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

RE: [PATCH 07/14] Nested Virtualization: trap

[Dong, Eddie]

>> It calls out to arch-specific code to do the intercept check and the
>> vmexit.  It could be tidied up a bit and the interfaces could change
>> but this looks like about the least amount of sharing there could be
>> on this path.  I can't see anything objectionable.
> 
> Correct. The key to make this possible is the generic exitcode
> mechanism.
> 
> Eddie: In an other mail you said, that the generic exitcode is
> overcomplicated. I have the impression that you do not
> realise that the unification of the exitcodes makes that much
> code shareable at all. No, I do not unified *all* exitcodes - I don't
> know if that is even possible.
> I only unified a subset that is actually used in the generic code.

Even with this partial unified generic code, we have to create 280+ new LOC.

I am not convinced  these unified vmexit/entry handler save code, I had a rough estimation:

patch4, hook API head definition: 159 Lines of patch, estimate 100 Line of new code. Note this is not a must code, but as common API definition due to the wrapping reason .
patch5, 837 lines of patch:  800 new lines for common code. Assume 20% overhead due to wrapping (common<->arch crossing) and additional conversion (generic <-> arch), that means we saved 800*80%=640 Lines, but add 800*20%=160 Lines. We saved 640-160=480 lines.
patch7, 50 lines of common, 20-30 lines are overhead due to wrapping (Line 14-41). The saving is very limited.

However at least patch 10 added 140 new lines for wrapper only to convert between common & arch format (see my another mail), which means 140* 2=280 new lines.

So the total saving at most is 480-280=200 lines.  On the other hand, light weight wrapper, i.e. arch specifc code goes to common only when it request some common functionality (more primary APIs), like single layer virtualization does. It could also save 200 lines or even more.

Is it worthy to bothering us so much for now?

Thx , eddie

RE: [PATCH 07/14] Nested Virtualization: trap

[Dong, Eddie]

Tim Deegan wrote:
> At 03:44 +0100 on 19 Aug (1282189499), Dong, Eddie wrote:
>>> 
>>> +int hvm_inject_exception(unsigned int trapnr, int errcode,
>>> unsigned long cr2) +{ +    uint64_t exitcode;
>>> +    bool_t is_intercepted;
>>> +    struct vcpu *v = current;
>>> +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
>>> +
>>> +    if ( !nestedhvm_enabled(v->domain) ) {
>>> +        hvm_funcs.inject_exception(trapnr, errcode, cr2); +       
>>> return 0; +    }
>> 
>> If it is not nested, we go from here to vendor specific injection
>> code. If it is nested, I think we'd better to go to another vendoral
>> specific handler too.
> 
> That's exactly what this wrapper does.  It's basically:
>  if ( in L2 and L1 intercepts )
>     force vmexit
>  else
>     inject directly.
> 
> It calls out to arch-specific code to do the intercept check and the
> vmexit.  It could be tidied up a bit and the interfaces could change
> but this looks like about the least amount of sharing there could be
> on this path.  I can't see anything objectionable.

Is there any real data to show the saving LOC here? I see the real code start from "exitcode = nestedhvm_exception2exitcode(trapnr);" (half of this function code is just for wrapper check.). The real work is ~20LOC. However we added at least 2 new wrapper APIs: nestedhvm_exception2exitcode & nestedhvm_exception2exitcode. 

Especially when you look at "nsvm_vmcb_exitcode_native2generic" in patch 10, which generates ~70 new LOC and another 70 new LOC in "nsvm_vmcb_exitcode_generic2native". If VMX side has to adop this framework, then another new 140 LOC has to be generated. (Total 280 new LOC)

If you look back to 5 years ago, in single layer virtualization side, we adopted the wrapper function because it is a must to make SVM work. However for nested virtualization, at least this wrapper is just a wrapper for wrapping with not clear or trivial benefits but clearly sacrificing readability/simplicity/flexibility etc.

BTW, from concept point of view, we don't call (virtual) vm exit as exception.

Thx, Eddie

Re: [PATCH 07/14] Nested Virtualization: trap

[Tim Deegan]

At 14:53 +0100 on 19 Aug (1282229594), Dong, Eddie wrote:
> Tim Deegan wrote:
> > At 03:44 +0100 on 19 Aug (1282189499), Dong, Eddie wrote:
> >>> 
> >>> +int hvm_inject_exception(unsigned int trapnr, int errcode,
> >>> unsigned long cr2) +{ +    uint64_t exitcode;
> >>> +    bool_t is_intercepted;
> >>> +    struct vcpu *v = current;
> >>> +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
> >>> +
> >>> +    if ( !nestedhvm_enabled(v->domain) ) {
> >>> +        hvm_funcs.inject_exception(trapnr, errcode, cr2); +       
> >>> return 0; +    }
> >> 
> >> If it is not nested, we go from here to vendor specific injection
> >> code. If it is nested, I think we'd better to go to another vendoral
> >> specific handler too.
> > 
> > That's exactly what this wrapper does.  It's basically:
> >  if ( in L2 and L1 intercepts )
> >     force vmexit
> >  else
> >     inject directly.
> > 
> > It calls out to arch-specific code to do the intercept check and the
> > vmexit.  It could be tidied up a bit and the interfaces could change
> > but this looks like about the least amount of sharing there could be
> > on this path.  I can't see anything objectionable.
> 
> Is there any real data to show the saving LOC here?

Lines of code aren't important by themselves; it's duplication of logic
that I object to.  For pure LOC we could certainly compress this
function in other ways.

> I see the real
> code start from "exitcode = nestedhvm_exception2exitcode(trapnr);"
> (half of this function code is just for wrapper check.). The real work
> is ~20LOC. However we added at least 2 new wrapper APIs:
> nestedhvm_exception2exitcode & nestedhvm_exception2exitcode.

That's one. :)  And it wouldn't be needed if the call to arch-specific
code to cause a vmexit and the test for interception took trapnr
directly (which they probably could).

If the new namespace of vmexit codes goes away entirely, that's fine by
me, btw.  Maybe the generic code can pass exit codes as opaque numbers,
with a few flags to steer it?  Christoph, what do you think?  You'll
have the best idea of how useful the new namespace is.

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

RE: [PATCH 07/14] Nested Virtualization: trap

[Dong, Eddie]

> 
> If the new namespace of vmexit codes goes away entirely, that's fine
> by me, btw.  Maybe the generic code can pass exit codes as opaque

I suggest we don't generate a new namespace for l2 exit. If the vmexit/ementry handler of L2 guest can go to arch specific file, we are mostly in same page :)
For the rest of common functions, it is relatively easy to reach agreement IMO, including interrupt injection (I am kind of neutral though like arch-specific solution a little bit more).
I believe we will have some (medium) in EPT/NPT/Shadow side :)

> numbers, with a few flags to steer it?  Christoph, what do you think?
> You'll have the best idea of how useful the new namespace is.

Thanks, Eddie

Re: [PATCH 07/14] Nested Virtualization: trap

[Tim Deegan]

At 04:12 +0100 on 23 Aug (1282536722), Dong, Eddie wrote:
> > 
> > If the new namespace of vmexit codes goes away entirely, that's fine
> > by me, btw.  Maybe the generic code can pass exit codes as opaque
> 
> I suggest we don't generate a new namespace for l2 exit. If the
> vmexit/ementry handler of L2 guest can go to arch specific file, we
> are mostly in same page :)

Well, since Christoph's done the work to pull out common logic I'd like
to see some of it used; but if it's going to be impossible, then we'll
start with whatever the two of you can agree on. :)

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 07/14] Nested Virtualization: trap

[Christoph Egger]

On Thursday 19 August 2010 16:30:56 Tim Deegan wrote:
> At 14:53 +0100 on 19 Aug (1282229594), Dong, Eddie wrote:
> > I see the real
> > code start from "exitcode = nestedhvm_exception2exitcode(trapnr);"
> > (half of this function code is just for wrapper check.). The real work
> > is ~20LOC. However we added at least 2 new wrapper APIs:
> > nestedhvm_exception2exitcode & nestedhvm_exception2exitcode.
>
> That's one. :)  And it wouldn't be needed if the call to arch-specific
> code to cause a vmexit and the test for interception took trapnr
> directly (which they probably could).
>
> If the new namespace of vmexit codes goes away entirely, that's fine by
> me, btw.

Using namespace is the C-way to "define" a class and prevents
namespace pollution for other things in the future.

> Maybe the generic code can pass exit codes as opaque numbers, 
> with a few flags to steer it?  Christoph, what do you think?  You'll
> have the best idea of how useful the new namespace is.

Look at the nh_structdata patch, you find a nh_forcevmexit structure
within nestedhvm. It contains the generic exit code and additional
information for the conversion and the vmexit injection code in exitinfo1
and exitinfo2. The meanings of exitinfo1 and exitinfo2 are directly related
to the generic exitcode in use and are described in the nh_core patch
in the C comments within the enum nestedhvm_intercepts.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[PATCH 07/14] Nested Virtualization: trap

[Christoph Egger]

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


Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_nh07_trap.diff --]
[-- Type: text/x-diff, Size: 2814 bytes --]

# HG changeset patch
# User cegger
# Date 1281003849 -7200
When injecting an exception into L2 guest,
inject a #VMEXIT if L1 guest intercepts the exception

diff -r b1a356e5658d -r 7789f1d7d7e7 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -965,6 +965,63 @@ void hvm_triple_fault(void)
     domain_shutdown(v->domain, SHUTDOWN_reboot);
 }
 
+int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned long cr2)
+{
+    uint64_t exitcode;
+    bool_t is_intercepted;
+    struct vcpu *v = current;
+    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
+
+    if ( !nestedhvm_enabled(v->domain) ) {
+        hvm_funcs.inject_exception(trapnr, errcode, cr2);
+        return 0;
+    }
+
+    if ( nestedhvm_vmentry_emulate(v) ) {
+        hvm_funcs.inject_exception(trapnr, errcode, cr2);
+        return 0;
+    }
+
+    if ( !nestedhvm_vcpu_in_guestmode(v) ) {
+        hvm_funcs.inject_exception(trapnr, errcode, cr2);
+        return 0;
+    }
+
+    exitcode = nestedhvm_exception2exitcode(trapnr);
+    hvm->nh_hostflags.fields.forcevmexit = 1;
+    hvm->nh_forcevmexit.exitcode = exitcode;
+    is_intercepted = hvm_nestedhvm_vm_intercepted_by_guest(v, exitcode);
+    hvm->nh_hostflags.fields.forcevmexit = 0;
+
+    if ( is_intercepted )
+    {
+        enum nestedhvm_vmexits nsret;
+
+        hvm->nh_forcevmexit.exitcode = exitcode;
+        hvm->nh_forcevmexit.exitinfo1 = errcode;
+        hvm->nh_forcevmexit.exitinfo2 = cr2;
+
+        hvm->nh_hostflags.fields.forcevmexit = 1;
+        nsret = nestedhvm_vcpu_vmexit(v, guest_cpu_user_regs(), 0 /* dummy */);
+        hvm->nh_hostflags.fields.forcevmexit = 0;
+
+        switch (nsret) {
+        case NESTEDHVM_VMEXIT_DONE:
+        case NESTEDHVM_VMEXIT_ERROR: /* L1 guest will crash L2 guest */
+            return 1;
+        case NESTEDHVM_VMEXIT_HOST:
+        case NESTEDHVM_VMEXIT_CONTINUE:
+        case NESTEDHVM_VMEXIT_FATALERROR:
+        default:
+            gdprintk(XENLOG_ERR, "unexpected nestedhvm error %i\n", nsret);
+            return -1;
+        }
+    }
+
+    hvm_funcs.inject_exception(trapnr, errcode, cr2);
+    return 0;
+}
+
 bool_t hvm_hap_nested_page_fault(unsigned long gfn)
 {
     p2m_type_t p2mt;
diff -r b1a356e5658d -r 7789f1d7d7e7 xen/include/asm-x86/hvm/hvm.h
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -279,11 +279,7 @@ void hvm_migrate_timers(struct vcpu *v);
 void hvm_do_resume(struct vcpu *v);
 void hvm_migrate_pirqs(struct vcpu *v);
 
-static inline void
-hvm_inject_exception(unsigned int trapnr, int errcode, unsigned long cr2)
-{
-    hvm_funcs.inject_exception(trapnr, errcode, cr2);
-}
+int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned long cr2);
 
 static inline int hvm_event_pending(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: [PATCH 07/14] Nested Virtualization: trap

[Tim Deegan]

> +int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned long cr2)
> +{
> +    uint64_t exitcode;
> +    bool_t is_intercepted;
> +    struct vcpu *v = current;
> +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
> +
> +    if ( !nestedhvm_enabled(v->domain) ) {
> +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> +        return 0;
> +    }
> +
> +    if ( nestedhvm_vmentry_emulate(v) ) {
> +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> +        return 0;
> +    }
> +
> +    if ( !nestedhvm_vcpu_in_guestmode(v) ) {
> +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> +        return 0;
> +    }
> +
> +    exitcode = nestedhvm_exception2exitcode(trapnr);
> +    hvm->nh_hostflags.fields.forcevmexit = 1;
> +    hvm->nh_forcevmexit.exitcode = exitcode;
> +    is_intercepted = hvm_nestedhvm_vm_intercepted_by_guest(v, exitcode);
> +    hvm->nh_hostflags.fields.forcevmexit = 0;
> +
> +    if ( is_intercepted )
> +    {
> +        enum nestedhvm_vmexits nsret;
> +
> +        hvm->nh_forcevmexit.exitcode = exitcode;
> +        hvm->nh_forcevmexit.exitinfo1 = errcode;
> +        hvm->nh_forcevmexit.exitinfo2 = cr2;
> +
> +        hvm->nh_hostflags.fields.forcevmexit = 1;
> +        nsret = nestedhvm_vcpu_vmexit(v, guest_cpu_user_regs(), 0 /* dummy */);
> +        hvm->nh_hostflags.fields.forcevmexit = 0;
> +
> +        switch (nsret) {
> +        case NESTEDHVM_VMEXIT_DONE:
> +        case NESTEDHVM_VMEXIT_ERROR: /* L1 guest will crash L2 guest */
> +            return 1;
> +        case NESTEDHVM_VMEXIT_HOST:
> +        case NESTEDHVM_VMEXIT_CONTINUE:
> +        case NESTEDHVM_VMEXIT_FATALERROR:
> +        default:
> +            gdprintk(XENLOG_ERR, "unexpected nestedhvm error %i\n", nsret);
> +            return -1;

This new [-1,0,1] return value is ignored by almost all callers.  Would
it be possible to get rid of the special case for #PF, and go back to
returning void?

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 07/14] Nested Virtualization: trap

[Christoph Egger]

On Monday 09 August 2010 14:44:57 Tim Deegan wrote:
> > +int hvm_inject_exception(unsigned int trapnr, int errcode, unsigned long
> > cr2) +{
> > +    uint64_t exitcode;
> > +    bool_t is_intercepted;
> > +    struct vcpu *v = current;
> > +    struct nestedhvm *hvm = &VCPU_NESTEDHVM(v);
> > +
> > +    if ( !nestedhvm_enabled(v->domain) ) {
> > +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> > +        return 0;
> > +    }
> > +
> > +    if ( nestedhvm_vmentry_emulate(v) ) {
> > +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> > +        return 0;
> > +    }
> > +
> > +    if ( !nestedhvm_vcpu_in_guestmode(v) ) {
> > +        hvm_funcs.inject_exception(trapnr, errcode, cr2);
> > +        return 0;
> > +    }
> > +
> > +    exitcode = nestedhvm_exception2exitcode(trapnr);
> > +    hvm->nh_hostflags.fields.forcevmexit = 1;
> > +    hvm->nh_forcevmexit.exitcode = exitcode;
> > +    is_intercepted = hvm_nestedhvm_vm_intercepted_by_guest(v, exitcode);
> > +    hvm->nh_hostflags.fields.forcevmexit = 0;
> > +
> > +    if ( is_intercepted )
> > +    {
> > +        enum nestedhvm_vmexits nsret;
> > +
> > +        hvm->nh_forcevmexit.exitcode = exitcode;
> > +        hvm->nh_forcevmexit.exitinfo1 = errcode;
> > +        hvm->nh_forcevmexit.exitinfo2 = cr2;
> > +
> > +        hvm->nh_hostflags.fields.forcevmexit = 1;
> > +        nsret = nestedhvm_vcpu_vmexit(v, guest_cpu_user_regs(), 0 /*
> > dummy */); +        hvm->nh_hostflags.fields.forcevmexit = 0;
> > +
> > +        switch (nsret) {
> > +        case NESTEDHVM_VMEXIT_DONE:
> > +        case NESTEDHVM_VMEXIT_ERROR: /* L1 guest will crash L2 guest */
> > +            return 1;
> > +        case NESTEDHVM_VMEXIT_HOST:
> > +        case NESTEDHVM_VMEXIT_CONTINUE:
> > +        case NESTEDHVM_VMEXIT_FATALERROR:
> > +        default:
> > +            gdprintk(XENLOG_ERR, "unexpected nestedhvm error %i\n",
> > nsret); +            return -1;
>
> This new [-1,0,1] return value is ignored by almost all callers.  Would
> it be possible to get rid of the special case for #PF, and go back to
> returning void?

The return value of hvm_inject_exception() is only meaningful for code
sections that is "aware" of nested virtualization.

hvm_inject_exception() is mostly called from code that is not "aware"
of nested virtualization.

Hence, yes, you are right, most callers will ignore the return value.

There is exactly one reason to have them: Intel seems to want
"shadow-on-shadow". In this case the page fault handler
walks the guests shadow page table. If that fails the page
fault handler wants to inject a VMEXIT(#PF) into the guest to
let the guest fix its shadow page table. If the guest page walk
is successfull the page fault intercept handler wants to inject the
page fault exception into the nested guest.

The page fault intercept handler in
SVM (see [PATCH 10/14] Nested Virtualization: svm specific implementation)
assumes that the guest intercepts a page fault.
It uses the return value to check if hvm_inject_exception() did what is 
expected: Injecting a VMEXIT(#PF), which is the case when the assumption
is correct.
The page fault intercept handler calls svm_inject_exception() to inject
a page fault into the nested guest.

If you can invalidate this error check reason then yes, I can go back
to make hvm_inject_exception() return void.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 07/14] Nested Virtualization: trap

[Tim Deegan]

At 09:55 +0100 on 10 Aug (1281434149), Christoph Egger wrote:
> There is exactly one reason to have them: Intel seems to want
> "shadow-on-shadow". In this case the page fault handler
> walks the guests shadow page table. If that fails the page
> fault handler wants to inject a VMEXIT(#PF) into the guest to
> let the guest fix its shadow page table. If the guest page walk
> is successfull the page fault intercept handler wants to inject the
> page fault exception into the nested guest.

OK, I think I'm getting tied up in the naming scheme.  Let me try to lay
out what I think is going on and you can tell me where I'm going wrong.

If the L0 Xen is using shadow pagetables, then it must be intercepting
#PF.  We expect the guest to be using shadows (and intercepting #PF) too.

When an actual #PF occurs when L2 is running, we VMEXIT to L0, which
walks the pagetables provided by L1.  In the interesting case, L0 sees
that the L1 pagetables justify the fault.  It injects #PF into the VM.

If the L1 guest is using shadows, it intercepts #PF and does its own
shadow pagetable tricks (which might involve it injecting #PF into the
L2 guest).  If it's not, the injected #PF goes straight to the L2.

> The page fault intercept handler in
> SVM (see [PATCH 10/14] Nested Virtualization: svm specific implementation)
> assumes that the guest intercepts a page fault.
> It uses the return value to check if hvm_inject_exception() did what is 
> expected: Injecting a VMEXIT(#PF), which is the case when the assumption
> is correct.
> The page fault intercept handler calls svm_inject_exception() to inject
> a page fault into the nested guest.

The new return code from hvm_inject_exception() is
0: Normal injection into the running L1 or the running L2.
1: VMEXIT from the running L2 to the L1, caused by injecting an
   intercepted exception. (This is the expected case in the scenario
   above).
-1: Any other case.

The logic in svm_vmexit_handler() when the L0 shadow code decides to
inject #PF is now: 

if ( L2 is running and L1 is not using HAP (i.e. sh-on-hap or sh-on-sh) )
{
    Inject #PF (allowing the L1 to intercept);
    If the L1 didn't intercept (or injection failed) 
        then crash the L1.
} else (i.e L1 running directly or hap-on-hap) {
    Inject directly, ignoring the L1 intercepts.
}

I don't see why this change is needed.  AFAICS, all cases are covered by
unconditionally calling hvm_inject_exception() here.  *-on-hap never
takes this path at all because L0 doesn't intercept #PF when it uses
HAP, so the only difference this makes is to catch the case where L1
didn't intercept #PF and it was injected directly into L2.  But this is
an acceptable thing for the L1 to do (even though Xen doesn't ever
behave that way), so I think it's wrong to crash the guest.

What was the reason for calling svm_inject_exception() directly in some
cases?

Cheers,

Tim.

> 
> If you can invalidate this error check reason then yes, I can go back
> to make hvm_inject_exception() return void.
> 
> Christoph
> 
> 
> -- 
> ---to satisfy European Law for business letters:
> Advanced Micro Devices GmbH
> Einsteinring 24, 85609 Dornach b. Muenchen
> Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
> Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> Registergericht Muenchen, HRB Nr. 43632
> 

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 07/14] Nested Virtualization: trap

[Christoph Egger]

On Tuesday 10 August 2010 12:48:27 Tim Deegan wrote:
> At 09:55 +0100 on 10 Aug (1281434149), Christoph Egger wrote:
> > There is exactly one reason to have them: Intel seems to want
> > "shadow-on-shadow". In this case the page fault handler
> > walks the guests shadow page table. If that fails the page
> > fault handler wants to inject a VMEXIT(#PF) into the guest to
> > let the guest fix its shadow page table. If the guest page walk
> > is successfull the page fault intercept handler wants to inject the
> > page fault exception into the nested guest.
>
> OK, I think I'm getting tied up in the naming scheme.  Let me try to lay
> out what I think is going on and you can tell me where I'm going wrong.
>
> If the L0 Xen is using shadow pagetables, then it must be intercepting
> #PF.  We expect the guest to be using shadows (and intercepting #PF) too.
>
> When an actual #PF occurs when L2 is running, we VMEXIT to L0, which
> walks the pagetables provided by L1.  In the interesting case, L0 sees
> that the L1 pagetables justify the fault. It injects #PF into the VM. 

> If the L1 guest is using shadows, it intercepts #PF and does its own
> shadow pagetable tricks (which might involve it injecting #PF into the
> L2 guest).  If it's not, the injected #PF goes straight to the L2.

Correct.

> > The page fault intercept handler in
> > SVM (see [PATCH 10/14] Nested Virtualization: svm specific
> > implementation) assumes that the guest intercepts a page fault.
> > It uses the return value to check if hvm_inject_exception() did what is
> > expected: Injecting a VMEXIT(#PF), which is the case when the assumption
> > is correct.
> > The page fault intercept handler calls svm_inject_exception() to inject
> > a page fault into the nested guest.
>
> The new return code from hvm_inject_exception() is
> 0: Normal injection into the running L1 or the running L2.
> 1: VMEXIT from the running L2 to the L1, caused by injecting an
>    intercepted exception. (This is the expected case in the scenario
>    above).
> -1: Any other case.

Correct.

> The logic in svm_vmexit_handler() when the L0 shadow code decides to
> inject #PF is now:
>
> if ( L2 is running and L1 is not using HAP (i.e. sh-on-hap or sh-on-sh) )
> {
>     Inject #PF (allowing the L1 to intercept);
>     If the L1 didn't intercept (or injection failed)
>         then crash the L1.
> } else (i.e L1 running directly or hap-on-hap) {
>     Inject directly, ignoring the L1 intercepts.
> }

Correct.

> I don't see why this change is needed.  AFAICS, all cases are covered by
> unconditionally calling hvm_inject_exception() here.  *-on-hap never
> takes this path at all because L0 doesn't intercept #PF when it uses
> HAP, so the only difference this makes is to catch the case where L1
> didn't intercept #PF and it was injected directly into L2.

Correct. How should L1 fix its shadow page table in this case?
I expect L2 to go wild because of that.

> But this is an acceptable thing for the L1 to do (even though Xen doesn't
> ever behave that way), so I think it's wrong to crash the guest.

Ok, that is the point where our opinions differ. Can you explain me why
this is acceptable? As I said above, I expect the L2 guest to go wild in
this case.

> What was the reason for calling svm_inject_exception() directly in some
> cases?

svm_inject_exception() always injects a #PF into running L1 or L2. It never
injects a VMEXIT into L1.

> Cheers,
>
> Tim.
>
> > If you can invalidate this error check reason then yes, I can go back
> > to make hvm_inject_exception() return void.
> >
> > Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 07/14] Nested Virtualization: trap

[Tim Deegan]

Hi, 

At 13:25 +0100 on 10 Aug (1281446710), Christoph Egger wrote:
> > I don't see why this change is needed.  AFAICS, all cases are covered by
> > unconditionally calling hvm_inject_exception() here.  *-on-hap never
> > takes this path at all because L0 doesn't intercept #PF when it uses
> > HAP, so the only difference this makes is to catch the case where L1
> > didn't intercept #PF and it was injected directly into L2.
> 
> Correct. How should L1 fix its shadow page table in this case?
> I expect L2 to go wild because of that.

L1 might not be using shadow pagetables.  It might be some sort of
cooperative microkernel allowing the L2 to do its own paging.  Even in
Xen, if we started using HVM containers for performance of 64-bit
direct-paged guests, we might consider having guest pagefaults delivered
straight to the guest (well, except that it would break ptwr_foo).

> > But this is an acceptable thing for the L1 to do (even though Xen doesn't
> > ever behave that way), so I think it's wrong to crash the guest.
> 
> Ok, that is the point where our opinions differ. Can you explain me why
> this is acceptable? As I said above, I expect the L2 guest to go wild in
> this case.

Quite possibly, but it's (a) exactly what would happen on real hardware,
and (b) not L0's problem if L1 misbehaves (except to protect other L1s
from it).

> > What was the reason for calling svm_inject_exception() directly in some
> > cases?
> 
> svm_inject_exception() always injects a #PF into running L1 or L2. It never
> injects a VMEXIT into L1.

Yes; but _why_ do we need to inject directly into L2?

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

Re: [PATCH 07/14] Nested Virtualization: trap

[Christoph Egger]

On Tuesday 10 August 2010 14:56:19 Tim Deegan wrote:
> Hi,
>
> At 13:25 +0100 on 10 Aug (1281446710), Christoph Egger wrote:
> > > I don't see why this change is needed.  AFAICS, all cases are covered
> > > by unconditionally calling hvm_inject_exception() here.  *-on-hap never
> > > takes this path at all because L0 doesn't intercept #PF when it uses
> > > HAP, so the only difference this makes is to catch the case where L1
> > > didn't intercept #PF and it was injected directly into L2.
> >
> > Correct. How should L1 fix its shadow page table in this case?
> > I expect L2 to go wild because of that.
>
> L1 might not be using shadow pagetables.  It might be some sort of
> cooperative microkernel allowing the L2 to do its own paging.  Even in
> Xen, if we started using HVM containers for performance of 64-bit
> direct-paged guests, we might consider having guest pagefaults delivered
> straight to the guest (well, except that it would break ptwr_foo).

Ok, I didn't have such microkernels in mind.

> > > But this is an acceptable thing for the L1 to do (even though Xen
> > > doesn't ever behave that way), so I think it's wrong to crash the
> > > guest.
> >
> > Ok, that is the point where our opinions differ. Can you explain me why
> > this is acceptable? As I said above, I expect the L2 guest to go wild in
> > this case.
>
> Quite possibly, but it's (a) exactly what would happen on real hardware,
> and (b) not L0's problem if L1 misbehaves (except to protect other L1s
> from it).

I see. Ok, I will change hvm_inject_exception() back to return void.

> > > What was the reason for calling svm_inject_exception() directly in some
> > > cases?
> >
> > svm_inject_exception() always injects a #PF into running L1 or L2. It
> > never injects a VMEXIT into L1.
>
> Yes; but _why_ do we need to inject directly into L2?

Not at all. This shouldn't happen in practise. The 'goto out' should prevent 
this.

The reason is, this last line also runs with nestedhvm disabled in the guest 
config file. We don't want to break the non-nestedhvm case, right?
Oh, I see now: hvm_inject_exception() does the right thing for all cases.
We can just leave it there as it is in the upstream source.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632