Re: [Xen-devel] [PATCH v3 1/2] nvmx: implement support for MSR bitmaps

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2020 12:13, Roger Pau Monné wrote:
> On Tue, Feb 04, 2020 at 10:32:47AM +0100, Jan Beulich wrote:
>> On 03.02.2020 18:37, Roger Pau Monne wrote:
>>> @@ -182,6 +192,11 @@ void nvmx_vcpu_destroy(struct vcpu *v)
>>>          free_domheap_page(v->arch.hvm.vmx.vmwrite_bitmap);
>>>          v->arch.hvm.vmx.vmwrite_bitmap = NULL;
>>>      }
>>> +    if ( nvmx->msr_merged )
>>> +    {
>>> +        free_domheap_page(nvmx->msr_merged);
>>> +        nvmx->msr_merged = NULL;
>>> +    }
>>
>> Can this not be done ...
>>
>>>  }
>>>   
>>>  void nvmx_domain_relinquish_resources(struct domain *d)
>>
>> ... in this function, thus happening earlier upon domain
>> cleanup, and leaving less resources allocated in case a domain
>> ends up as zombie (due to another bug elsewhere)? Actually -
>> aren't you extending an existing bug this way? When
>> nestedhvm_vcpu_initialise() fails, nestedhvm_vcpu_destroy()
>> won't be called afaict.
> 
> nestedhvm_vcpu_destroy will be called by hvm_vcpu_initialise (the
> caller of nestedhvm_vcpu_initialise) AFAICT.

Unless nestedhvm_vcpu_initialise() itself fails:

    if ( nestedhvm_enabled(d)
         && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown: 
nestedhvm_vcpu_destroy */
        goto fail5;
...
 fail6:
    nestedhvm_vcpu_destroy(v);
 fail5:
    free_compat_arg_xlat(v);

The common issue of these functions not being idempotent.

>> Hence nvmx_vcpu_initialise() not
>> cleaning up after itself in case of failure looks to be a
>> memory leak. As of b3344bb1cae0 any such will be taken care
>> of implicitly as long as the freeing happens on the
>> relinquish-resources paths.
> 
> I can move the new addition to nvmx_domain_relinquish_resources, I've
> originally added it to nvmx_vcpu_destroy because that's where other
> pages are also freed.

Right, hence me mentioning a pre-existing issue that you're
widening a little the way things are currently done.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel