From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH 2/7] vm-event: VM_EVENT_FLAG_DENY requires VM_EVENT_FLAG_VCPU_PAUSED Date: Tue, 21 Jun 2016 09:09:31 -0600 Message-ID: References: <1466085888-7428-1-git-send-email-czuzu@bitdefender.com> <1466086077-7518-1-git-send-email-czuzu@bitdefender.com> <53ff0b6b-0c3a-bca7-b17e-969dd829a807@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5837884489530415786==" Return-path: In-Reply-To: <53ff0b6b-0c3a-bca7-b17e-969dd829a807@bitdefender.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Corneliu ZUZU Cc: Andrew Cooper , Jan Beulich , Razvan Cojocaru , Xen-devel List-Id: xen-devel@lists.xenproject.org --===============5837884489530415786== Content-Type: multipart/alternative; boundary=001a114a47224118dc0535cb35b8 --001a114a47224118dc0535cb35b8 Content-Type: text/plain; charset=UTF-8 On Jun 21, 2016 05:26, "Corneliu ZUZU" wrote: > > On 6/16/2016 7:11 PM, Tamas K Lengyel wrote: >> >> On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU 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 --001a114a47224118dc0535cb35b8 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Jun 21, 2016 05:26, "Corneliu ZUZU" <czuzu@bitdefender.com> wrote:
>
> On 6/16/2016 7:11 PM, Tamas K Lengyel wrote:
>>
>> On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
>>>
>>> For VM_EVENT_FLAG_DENY to work, the vcpu must be paused (sync = =3D 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 th= at in
>>> vm_event_register_write_resume().
>>
>> Well, the problem with this is that the user can set the VCPU_PAUS= ED
>> flag any time it wants. It can happen that Xen hasn't paused t= he 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 th= e
>> 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_resum= e:
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rsp.flags & VM_EVENT_FLAG_VCPU_PA= USED )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rsp.flags & VM_EVEN= T_FLAG_SET_REGISTERS )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_event_set_r= egisters(v, &rsp);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rsp.flags & VM_EVEN= T_FLAG_TOGGLE_SINGLESTEP )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_event_toggl= e_singlestep(d, v);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_event_vcpu_unpause(v); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
> , i.e. shouldn't it be fixed to:
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* check flags which apply only when the v= CPU is paused */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( atomic_read(&v->vm_event_pause= _count) )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rsp.flags & VM_EVEN= T_FLAG_SET_REGISTERS )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_event_set_r= egisters(v, &rsp);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rsp.flags & VM_EVEN= T_FLAG_TOGGLE_SINGLESTEP )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_event_toggl= e_singlestep(d, v);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rsp.flags & VM_EVEN= T_FLAG_VCPU_PAUSED )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 vm_event_vcpu_= unpause(v);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> ?
>
> If this holds, the check for vCPU pause can also be removed from vm_ev= ent_toggle_singlestep (maybe turned into an ASSERT which could also be adde= d to vm_event_set_registers).
>

Yes, reworking that whole part as you outlined above would b= e nice!

Tamas

--001a114a47224118dc0535cb35b8-- --===============5837884489530415786== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============5837884489530415786==--