Re: [Xen-devel] [PATCH v2] x86/nhvm: properly clean up after failure to set up all vCPU-s

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.02.13 at 13:24, Tim Deegan <tim@xxxxxxx> wrote:
> At 12:19 +0000 on 21 Feb (1361449175), Jan Beulich wrote:
>> >>> On 21.02.13 at 12:44, Tim Deegan <tim@xxxxxxx> wrote:
>> > At 11:26 +0000 on 21 Feb (1361445983), Jan Beulich wrote:
>> >> Otherwise we may leak memory when setting up nHVM fails half way.
>> >> 
>> >> This implies that the individual destroy functions will have to remain
>> >> capable (in the VMX case they first need to be made so, following
>> >> 26486:7648ef657fe7 and 26489:83a3fa9c8434) of being called for a vCPU
>> >> that the corresponding init function was never run on.
>> >> 
>> >> Once at it, also clean up some inefficiencies in the corresponding
>> >> parameter validation code.
>> >> 
>> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> >> ---
>> >> v2: nVMX fixes required by 26486:7648ef657fe7 and 26489:83a3fa9c8434.
>> >> 
>> >> --- a/xen/arch/x86/hvm/hvm.c
>> >> +++ b/xen/arch/x86/hvm/hvm.c
>> >> @@ -3916,20 +3916,25 @@ long do_hvm_op(unsigned long op, XEN_GUE
>> >>                      rc = -EPERM;
>> >>                      break;
>> >>                  }
>> >> +                if ( !a.value )
>> >> +                    break;
>> > 
>> > Surely setting from 1 to 0 should either disable nested-hvm entirely
>> > (including calling nestedhvm_vcpu_destroy()) or fail.  Otherwise I think
>> > alternating 1 and 0 will cause nestedhvm_vcpu_initialise() to allocate
>> > fresh state every time (& leak the old state).
>> 
>> No, that's precisely not the case with this patch (but was before
>> in case of failure on other than the first vCPU).
>> 
>> Of course we can _change_ to the model of fully disabling nHVM
>> in that case, but honestly I don't see the point, and the code is
>> simpler without doing so.
> 
> Agreed, but AFAICT after this patch when the value is set to 1 and then
> 0 and then 1 again, it will call nestedhvm_vcpu_initialise() again, and
> certainly the nvmx version of that doesn't DTRT on an
> already-initialised vcpu.  What am I missing here?

Oh, yes, right you are. And similarly for SVM. Let me adjust that,
and post a v3 eventually.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel