Re: [Xen-devel] [PATCH] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.11.2019 12:14, Julien Grall wrote:
> Hi,
> 
> On 27/11/2019 09:44, Jan Beulich wrote:
>> On 26.11.2019 18:17, Paul Durrant wrote:
>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> A guest will setup a shared page with the hypervisor for each vCPU via
>>> XENPMU_init. The page will then get mapped in the hypervisor and only
>>> released when XEMPMU_finish is called.
>>>
>>> This means that if the guest is not shutdown gracefully (such as via xl
>>> destroy), the page will stay mapped in the hypervisor.
>>
>> Isn't this still too weak a description? It's not the tool stack
>> invoking XENPMU_finish, but the guest itself afaics. I.e. a
>> misbehaving guest could prevent proper cleanup even with graceful
>> shutdown.
>>
>>> @@ -2224,6 +2221,9 @@ int domain_relinquish_resources(struct domain *d)
>>>       if ( is_hvm_domain(d) )
>>>           hvm_domain_relinquish_resources(d);
>>>   
>>> +    for_each_vcpu ( d, v )
>>> +        vpmu_destroy(v);
>>> +
>>>       return 0;
>>>   }
>>
>> I think simple things which may allow shrinking the page lists
>> should be done early in the function. As vpmu_destroy() looks
>> to be idempotent, how about leveraging the very first
>> for_each_vcpu() loop in the function (there are too many of them
>> there anyway, at least for my taste)?
> 
> This is not entirely obvious that vpmu_destroy() is idempotent.
> 
> For instance, I can't find out who is clearing VCPU_CONTEXT_ALLOCATED. 
> so I think vcpu_arch_destroy() would be executed over and over.
> 
> I don't know whether this is an issue, but I can't figure out that is it 
> not one. Did I miss anything?

If the function wasn't idempotent, then calling it unconditionally
from domain_relinquish_resources() would be wrong too. After all
the guest may have invoked XENPMU_finish.

As to VCPU_CONTEXT_ALLOCATED - I don't think this ever gets cleared
anywhere. But this by itself doesn't make the function non-
idempotent. The vpmu_clear_last invocation, for example, will
happen just once. And {amd,core2}_vpmu_destroy() look okay at the
first glance, too.

Jan

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