Re: [Xen-devel] [PATCH V2] common/vm_event: synchronize vCPU state in vm_event_resume()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/10/2016 01:12 PM, Jan Beulich wrote:
>>>> On 10.08.16 at 09:35, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/common/vm_event.c
>> +++ b/xen/common/vm_event.c
>> @@ -388,6 +388,13 @@ void vm_event_resume(struct domain *d, struct 
>> vm_event_domain *ved)
>>          v = d->vcpu[rsp.vcpu_id];
>>  
>>          /*
>> +         * Make sure the vCPU state has been synchronized for the custom
>> +         * handlers.
>> +         */
>> +        if ( atomic_read(&v->vm_event_pause_count) )
>> +            sync_vcpu_execstate(v);
> 
> It seems to me that the latest with this change using a simple
> atomic_t won't suffice anymore - you'd now really need to
> make sure the user mode tools can't resume that vCPU behind
> your back, which aiui can only be achieved by using a suitable
> lock (perhaps a read/write one if reading the pause count is
> more common than updating it).

I'm not sure how that would happen, vm_event_vcpu_pause() increments
v->vm_event_pause_count, and then calls vcpu_pause_nosync(v), which then
increments it's own counter.

So there doesn't seem to be a possibility of v->vm_event_pause_count
being > 0 while the vCPU is unpaused, and there should also be no
possibility that vm_event_resume() and vm_event_pause() could happen on
the same vCPU at the same time. If there are unbalanced vcpu_pause() /
vcpu_unpause() somewhere else in the code, that surely would be bug.

Am I missing a scenario?


Thanks,
Razvan

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