[PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Ravi Sahita]

As reported by Wei Lu on July 27 2015

Signed-off-by: Ravi Sahita <ravi.sahita@intel.com>
---
 xen/arch/x86/hvm/hvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ec1d797..6a56216 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2463,7 +2463,9 @@ void hvm_vcpu_destroy(struct vcpu *v)
 {
     hvm_all_ioreq_servers_remove_vcpu(v->domain, v);
 
-    altp2m_vcpu_destroy(v);
+    if ( hvm_altp2m_supported() )
+        altp2m_vcpu_destroy(v);
+
     nestedhvm_vcpu_destroy(v);
 
     free_compat_arg_xlat(v);
-- 
1.9.1

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Andrew Cooper]

On 29/07/15 17:39, Ravi Sahita wrote:
> As reported by Wei Lu on July 27 2015

Reported-by: Wei Liu <wei.liu2@citrix.com>

> Signed-off-by: Ravi Sahita <ravi.sahita@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/hvm/hvm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ec1d797..6a56216 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2463,7 +2463,9 @@ void hvm_vcpu_destroy(struct vcpu *v)
>  {
>      hvm_all_ioreq_servers_remove_vcpu(v->domain, v);
>  
> -    altp2m_vcpu_destroy(v);
> +    if ( hvm_altp2m_supported() )
> +        altp2m_vcpu_destroy(v);
> +
>      nestedhvm_vcpu_destroy(v);
>  
>      free_compat_arg_xlat(v);

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Wei Liu]

On Wed, Jul 29, 2015 at 05:44:38PM +0100, Andrew Cooper wrote:
> On 29/07/15 17:39, Ravi Sahita wrote:
> > As reported by Wei Lu on July 27 2015
> 
> Reported-by: Wei Liu <wei.liu2@citrix.com>
> 
> > Signed-off-by: Ravi Sahita <ravi.sahita@intel.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 

Tested-by: Wei Liu <wei.liu2@citrix.com>

> > ---
> >  xen/arch/x86/hvm/hvm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ec1d797..6a56216 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2463,7 +2463,9 @@ void hvm_vcpu_destroy(struct vcpu *v)
> >  {
> >      hvm_all_ioreq_servers_remove_vcpu(v->domain, v);
> >  
> > -    altp2m_vcpu_destroy(v);
> > +    if ( hvm_altp2m_supported() )
> > +        altp2m_vcpu_destroy(v);
> > +
> >      nestedhvm_vcpu_destroy(v);
> >  
> >      free_compat_arg_xlat(v);

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Ian Campbell]

On Wed, 2015-07-29 at 17:44 +0100, Andrew Cooper wrote:
> On 29/07/15 17:39, Ravi Sahita wrote:
> > As reported by Wei Lu on July 27 2015
> 
> Reported-by: Wei Liu <wei.liu2@citrix.com>

Which leaves an effectively empty commit message:

    x86/hvm.c: fix regression in guest destruction

    Reported-by: Wei Liu <    wei.liu2@citrix.com    >
    Signed-off-by: Ravi Sahita <    ravi.sahita@intel.com    >
    Reviewed-by: Andrew Cooper <    andrew.cooper3@citrix.com    >
    Tested-by: Wei Liu <    wei.liu2@citrix.com    >

If I were maintainer of this code I'd be very grumpy about the lack of a
commit log (What regression? What is the fix? Why is it correct?) for this
patch. I asked on IRC and Andy suggested instead:

    x86/hvm.c: Don't tear down altp2m state if it was never set up

which I have applied with. I still don't think the commit message is very
satisfactory, but I'm not maintainer of any of this code so meh.

Ian.

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Ian Campbell]

On Thu, 2015-07-30 at 10:21 +0100, Ian Campbell wrote:

> which I have applied with. I still don't think the commit message is very
> satisfactory, but I'm not maintainer of any of this code so meh.

For the benefit of the archives perhaps someone could explain why gating a
per-vcpu teardown on a host level feature setting is correct?

In particular what ensures that altp2m_vcpu_initialise has been called,
given that this is only called from HVMOP_altp2m_set_domain_state. What
happens if that HVMOP is never touched?

Do things work both for altp2m disabled on the Xen command line and
disabled/enabled in the guest config? If so how?

Also how come HVMOP_altp2m_set_domain_state does not have a
hvm_altp2m_supported check?

Ian.

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[George Dunlap]

On 07/30/2015 10:32 AM, Ian Campbell wrote:
> On Thu, 2015-07-30 at 10:21 +0100, Ian Campbell wrote:
> 
>> which I have applied with. I still don't think the commit message is very
>> satisfactory, but I'm not maintainer of any of this code so meh.
> 
> For the benefit of the archives perhaps someone could explain why gating a
> per-vcpu teardown on a host level feature setting is correct?
> 
> In particular what ensures that altp2m_vcpu_initialise has been called,
> given that this is only called from HVMOP_altp2m_set_domain_state. What
> happens if that HVMOP is never touched?
> 
> Do things work both for altp2m disabled on the Xen command line and
> disabled/enabled in the guest config? If so how?
> 
> Also how come HVMOP_altp2m_set_domain_state does not have a
> hvm_altp2m_supported check?

So this was all acked & stuff before I had much of a chance to comment
on it, but on my to-do list for 4.7 is to rework a lot of the
initialization / teardown stuff.  In particular:

- Always and only check for whether something has been initialized
(e.g., non-NULL, non-INVALID_MFN) before tearing it down

- Do *all* of the initialization for both altp2m and nestedhvm when
they're actually enabled for the domain, rather than doing a bunch of
the initialization unconditionally up front.

This is all part of the "technical debt" we were talking about when we
considered giving it a freeze exception.

 -George

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Ian Campbell]

On Thu, 2015-07-30 at 10:38 +0100, George Dunlap wrote:
> On 07/30/2015 10:32 AM, Ian Campbell wrote:
> > On Thu, 2015-07-30 at 10:21 +0100, Ian Campbell wrote:
> > 
> > > which I have applied with. I still don't think the commit message is 
> > > very
> > > satisfactory, but I'm not maintainer of any of this code so meh.
> > 
> > For the benefit of the archives perhaps someone could explain why 
> > gating a
> > per-vcpu teardown on a host level feature setting is correct?
> > 
> > In particular what ensures that altp2m_vcpu_initialise has been called,
> > given that this is only called from HVMOP_altp2m_set_domain_state. What
> > happens if that HVMOP is never touched?
> > 
> > Do things work both for altp2m disabled on the Xen command line and
> > disabled/enabled in the guest config? If so how?
> > 
> > Also how come HVMOP_altp2m_set_domain_state does not have a
> > hvm_altp2m_supported check?
> 
> So this was all acked & stuff before I had much of a chance to comment
> on it, but on my to-do list for 4.7 is to rework a lot of the
> initialization / teardown stuff.  In particular:
> 
> - Always and only check for whether something has been initialized
> (e.g., non-NULL, non-INVALID_MFN) before tearing it down
> 
> - Do *all* of the initialization for both altp2m and nestedhvm when
> they're actually enabled for the domain, rather than doing a bunch of
> the initialization unconditionally up front.
> 
> This is all part of the "technical debt" we were talking about when we
> considered giving it a freeze exception.

Thanks, I'm inferring that everything I asked about in the second from last
paragraph is somehow fine, just confusingly achieved in the current code...

I'm done grumping now...

Ian.

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Andrew Cooper]

On 30/07/15 10:32, Ian Campbell wrote:
> Also how come HVMOP_altp2m_set_domain_state does not have a
> hvm_altp2m_supported check?

(For the benefit of not leaving this question unanswered)

There is an early exit at the top of do_altp2m_op(), ahead of even
reading the hypercall body from the guest.

~Andrew

Re: [PATCH for 4.6] x86/hvm.c: fix regression in guest destruction

[Ian Campbell]

On Thu, 2015-07-30 at 10:55 +0100, Andrew Cooper wrote:
> On 30/07/15 10:32, Ian Campbell wrote:
> > Also how come HVMOP_altp2m_set_domain_state does not have a
> > hvm_altp2m_supported check?
> 
> (For the benefit of not leaving this question unanswered)
> 
> There is an early exit at the top of do_altp2m_op(), ahead of even
> reading the hypercall body from the guest.

Thanks.

Ian.