Re: [Xen-devel] [PATCH 2/7] vm-event: VM_EVENT_FLAG_DENY requires VM_EVENT_FLAG_VCPU_PAUSED

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Jun 21, 2016 05:26, "Corneliu ZUZU" <czuzu@xxxxxxxxxxxxxxx> wrote:
>
> On 6/16/2016 7:11 PM, Tamas K Lengyel wrote:
>>
>> On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx> wrote:
>>>
>>> For VM_EVENT_FLAG_DENY to work, the vcpu must be paused (sync = 1) until the
>>> vm-event is handled. A vm-event response having VM_EVENT_FLAG_DENY flag set
>>> should also set the VM_EVENT_FLAG_VCPU_PAUSED flag. Enforce that in
>>> vm_event_register_write_resume().
>>
>> Well, the problem with this is that the user can set the VCPU_PAUSED
>> flag any time it wants. It can happen that Xen hasn't paused the vCPU
>> but the user still sends that flag, in which case the unpause the flag
>> induces will not actually do anything. You should also check if the
>> vCPU is in fact paused rather then just relying on this flag.
>>
>> Tamas
>>
>
> Tamas,
>
> Isn't that also the case with the following block @ vm_event_resume:
>
>         if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>         {
>             if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>                 vm_event_set_registers(v, &rsp);
>
>             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>                 vm_event_toggle_singlestep(d, v);
>
>             vm_event_vcpu_unpause(v);
>         }
>
> , i.e. shouldn't it be fixed to:
>
>         /* check flags which apply only when the vCPU is paused */
>         if ( atomic_read(&v->vm_event_pause_count) )
>         {
>             if ( rsp.flags & VM_EVENT_FLAG_SET_REGISTERS )
>                 vm_event_set_registers(v, &rsp);
>             if ( rsp.flags & VM_EVENT_FLAG_TOGGLE_SINGLESTEP )
>                 vm_event_toggle_singlestep(d, v);
>             if ( rsp.flags & VM_EVENT_FLAG_VCPU_PAUSED )
>                 vm_event_vcpu_unpause(v);
>         }
> ?
>
> If this holds, the check for vCPU pause can also be removed from vm_event_toggle_singlestep (maybe turned into an ASSERT which could also be added to vm_event_set_registers).
>

Yes, reworking that whole part as you outlined above would be nice!

Tamas

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