From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Egger <Christoph.Egger@amd.com>
Subject: Re: [PATCH 05/14] Nested Virtualization: core
Date: Tue, 10 Aug 2010 10:36:59 +0200
Message-ID: <201008101037.00389.Christoph.Egger@amd.com>
References: <201008051701.36033.Christoph.Egger@amd.com>
	<20100809123848.GY13291@whitby.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <20100809123848.GY13291@whitby.uk.xensource.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Tim Deegan <Tim.Deegan@citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>
List-Id: xen-devel@lists.xenproject.org


Hi Tim,

It looks like I have missed this from your first review. Thanks for bringing
this up again.

On Monday 09 August 2010 14:38:48 Tim Deegan wrote:
> Hi,
>
> > +int
> > +nestedhvm_vcpu_destroy(struct vcpu *v)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!nestedhvm_enabled(v->domain))
> > +		return 0;
> > +
> > +	if (nestedhvm_vcpu_in_guestmode(v)) {
> > +		nestedhvm_vcpu_exit_guestmode(v);
> > +		ret = -EBUSY;
> > +	}
>
> This still bothers me, since we go on to complete the teardown anyway,
> and this function's only caller does nothing interesting with the
> result.

I removed "vcpu in guestmode" check completely.

> > +	/* Enable interrupts or the guest won't see any interrupts
> > +	 * after nested guest exited.
> > +	 */
> > +	nestedsvm_vcpu_stgi(v);

This stgi call is gone.

> > +
> > +	hvm_nestedhvm_vcpu_destroy(v);
>
> While I'm complaining about unchecked return values, I should point out
> that your implementation of this returns an integer (which in turn is
> always 0).

I changed this to 'return hvm_nestedhvm_vcpu_destroy(v);'

> > +	return ret;

This return is gone.

> > +}


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