Xen devel - Aug 2010 - [PATCH 02/14] Nested Virtualization: localevent

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

The functions are called local_event_delivery* because they implicitly act
on current. They don''t need to take a vcpu parameter. If you find you
need a
vcpu parameter then you are using them, or one of their callers,
incorrectly.

 -- Keir

On 05/08/2010 16:00, "Christoph Egger" <Christoph.Egger@amd.com>
wrote:
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


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

I seem to remember we discussed the reason for this a bit some time ago. It
looked to me like you were calling a function that makes sense only on a
running guest (and a locally currently running guest at that) after the
guest was dead, during cleanup/teardown. If I''m remebering correctly
then
the fix would be to not do that then. ;-)

 -- Keir

On 05/08/2010 16:46, "Keir Fraser" <keir.fraser@eu.citrix.com>
wrote:
> The functions are called local_event_delivery* because they implicitly act
> on current. They don''t need to take a vcpu parameter. If you find
you need a
> vcpu parameter then you are using them, or one of their callers,
> incorrectly.
> 
>  -- Keir
> 
> On 05/08/2010 16:00, "Christoph Egger"
<Christoph.Egger@amd.com> wrote:
> 
>> 
>> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

On Thursday 05 August 2010 18:19:14 Keir Fraser wrote:
> I seem to remember we discussed the reason for this a bit some time ago.
Yes, I remember, too.
> It looked to me like you were calling a function that makes sense only on a
> running guest (and a locally currently running guest at that) after the
> guest was dead, during cleanup/teardown. If I''m remembering
correctly then
> the fix would be to not do that then. ;-)
I am not sure if I got you. I try to repeat you with my own words:

This localevent patch on its own is pointless. It makes only sense
when a guest (the level 1 guest) runs its own guest, the level 2 guest.
When the level 2 guest was dead, during cleanup/teardown.


I retry to explain:

This localevent patch on its own is pointless, I agree with you in this case.
The patch is needed when the level 1 guest was running a level 2 guest
and get destroyed. During the destroy process Xen may still want to
inject (pending) interrupts/events.
For this reason, nestedhvm_vcpu_destroy() (added in patch 5/14)
does a nestedsvm_vcpu_stgi() to prevent the interrupts/events
from being blocked by hvm_interrupt_blocked() (see patch 9/14)
and level 1 guest remaining in a zombie state.

nestedsvm_vcpu_stgi() calls local_event_delivery_enable() (only on AMD now).
At the time when nestedsvm_vcpu_stgi() is called from 
nestedhvm_vcpu_destroy(), "current" may be an invalid pointer while
the vcpu pointer is valid.
When local_event_delivery_enable() accesses "current" then Xen
crashes.
That is why local_event_delivery_enable() needs the vcpu argument.

Christoph


>  -- Keir
>
> On 05/08/2010 16:46, "Keir Fraser"
<keir.fraser@eu.citrix.com> wrote:
> > The functions are called local_event_delivery* because they implicitly
> > act on current. They don''t need to take a vcpu parameter. If
you find you
> > need a vcpu parameter then you are using them, or one of their
callers,
> > incorrectly.
> >
> >  -- Keir
> >
> > On 05/08/2010 16:00, "Christoph Egger"
<Christoph.Egger@amd.com> wrote:
> >> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel


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

On 06/08/2010 10:17, "Christoph Egger" <Christoph.Egger@amd.com>
wrote:
> For this reason, nestedhvm_vcpu_destroy() (added in patch 5/14)
> does a nestedsvm_vcpu_stgi() to prevent the interrupts/events
> from being blocked by hvm_interrupt_blocked() (see patch 9/14)
> and level 1 guest remaining in a zombie state.
Ah, this is the crux of it. You shouldn''t need to stgi from the vcpu
destructor. It makes no sense and doing it shouldn''t leave you with a
zombie
domain. Indeed, vcpu_destroy() is called from the very final domain
destructor -- vcpu_destroy''s caller finishes by freeing the domain
structure
itself, so not much chance of hanging around as a zombie! I''m assuming
you
call nestedhvm_vcpu_destroy() on the vcpu_destroy() path here by the way...
If it''s called from some other context then I think its name is
misleading
and should be changed.

 -- Keir



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

On Friday 06 August 2010 16:02:11 Keir Fraser wrote:
> On 06/08/2010 10:17, "Christoph Egger"
<Christoph.Egger@amd.com> wrote:
> > For this reason, nestedhvm_vcpu_destroy() (added in patch 5/14)
> > does a nestedsvm_vcpu_stgi() to prevent the interrupts/events
> > from being blocked by hvm_interrupt_blocked() (see patch 9/14)
> > and level 1 guest remaining in a zombie state.
>
> Ah, this is the crux of it. You shouldn''t need to stgi from the
vcpu
> destructor. It makes no sense and doing it shouldn''t leave you
with a
> zombie domain.
I backed out the ''localevent'' patch in my local tree, removed
the stgi
call in the vcpu destructor and run tests. A lot of things have been
changed since the issue has been found and the real bug might have
already been fixed in the meantime.

I haven''t seen any issues with that changes in my tests so my next
patch series I send will have the localevent patch and the stgi call
dropped.
> Indeed, vcpu_destroy() is called from the very final domain 
> destructor -- vcpu_destroy''s caller finishes by freeing the domain
> structure itself, so not much chance of hanging around as a zombie!
I''m
> assuming you call nestedhvm_vcpu_destroy() on the vcpu_destroy() path here
> by the way...
Yes, your assumption is correct.
> If it''s called from some other context then I think its name 
> is misleading and should be changed.
>
>  -- Keir
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

On 10/08/2010 09:01, "Christoph Egger" <Christoph.Egger@amd.com>
wrote:
>> Ah, this is the crux of it. You shouldn''t need to stgi from
the vcpu
>> destructor. It makes no sense and doing it shouldn''t leave you
with a
>> zombie domain.
> 
> I backed out the ''localevent'' patch in my local tree,
removed the stgi
> call in the vcpu destructor and run tests. A lot of things have been
> changed since the issue has been found and the real bug might have
> already been fixed in the meantime.
> 
> I haven''t seen any issues with that changes in my tests so my next
> patch series I send will have the localevent patch and the stgi call
> dropped.
Thanks. I applied the p2m infrastructure patch that Tim acked, by the way.
Also may as well give Intel a little longer to respond on the common
infrastructure patches.

 -- keir



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