thr3ads.net - Xen devel - [Xen-devel] [PATCH 07/14] Nested Virtualization: trap [Aug 2010]

If this information is useful, please help other people find it:
Share via:

Christoph Egger

2010-Aug-05 15:02 UTC

[Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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


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

Tim Deegan

2010-Aug-09 12:44 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

> +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)

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

Christoph Egger

2010-Aug-10 08:55 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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


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

Tim Deegan

2010-Aug-10 10:48 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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)

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

Christoph Egger

2010-Aug-10 12:25 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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


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

Tim Deegan

2010-Aug-10 12:56 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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)

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

Christoph Egger

2010-Aug-10 13:37 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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


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

Dong, Eddie

2010-Aug-19 02:44 UTC

head link

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

> 
> +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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Tim Deegan

2010-Aug-19 08:35 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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)

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

Christoph Egger

2010-Aug-19 10:32 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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


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

Dong, Eddie

2010-Aug-19 13:53 UTC

head link

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Dong, Eddie

2010-Aug-19 14:12 UTC

head link

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

>> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Tim Deegan

2010-Aug-19 14:30 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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)

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

Dong, Eddie

2010-Aug-23 03:12 UTC

head link

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

> 
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger

2010-Aug-23 16:03 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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


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

Tim Deegan

2010-Aug-31 10:34 UTC

head link

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

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)

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

Xen devel - Aug 2010 - [PATCH 07/14] Nested Virtualization: trap

[Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

RE: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap

Re: [Xen-devel] [PATCH 07/14] Nested Virtualization: trap