Christoph Egger
2010-Aug-05 15:01 UTC
[Xen-devel] [PATCH 05/14] Nested Virtualization: core
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:38 UTC
Re: [Xen-devel] [PATCH 05/14] Nested Virtualization: core
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.> + /* Enable interrupts or the guest won''t see any interrupts > + * after nested guest exited. > + */ > + nestedsvm_vcpu_stgi(v); > + > + 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).> + return ret; > +}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:36 UTC
Re: [Xen-devel] [PATCH 05/14] Nested Virtualization: core
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel