From mboxrd@z Thu Jan 1 00:00:00 1970 From: Corneliu ZUZU Subject: Re: [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side. Date: Tue, 16 Feb 2016 19:48:20 +0200 Message-ID: <56C360E4.4080707@bitdefender.com> References: <1455606432-18648-1-git-send-email-czuzu@bitdefender.com> <1455606514-18705-1-git-send-email-czuzu@bitdefender.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5160611010830515592==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Keir Fraser , Ian Campbell , Razvan Cojocaru , Andrew Cooper , Xen-devel , Stefano Stabellini , Jan Beulich List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============5160611010830515592== Content-Type: multipart/alternative; boundary="------------050106020309050809080503" This is a multi-part message in MIME format. --------------050106020309050809080503 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 2/16/2016 6:02 PM, Tamas K Lengyel wrote: > > > @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct > xen_domctl_monitor_op *mop) > > case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: > { > > > So since we will now have two separate booleans, requested_status and > old_status and then manually verify they are opposite.. > > - bool_t status = ad->monitor.mov_to_msr_enabled; > + bool_t old_status = ad->monitor.mov_to_msr_enabled; > > > ...here we should set the field to requested_status, not !old_status. > While they are technically equivalent, the code would read better to > other way around. > > > - ad->monitor.mov_to_msr_enabled = !status; > + ad->monitor.mov_to_msr_enabled = !old_status; > > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP: > { > - bool_t status = ad->monitor.singlestep_enabled; > + bool_t old_status = ad->monitor.singlestep_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > > > Here as well.. > > - ad->monitor.singlestep_enabled = !status; > + ad->monitor.singlestep_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT: > { > - bool_t status = ad->monitor.software_breakpoint_enabled; > + bool_t old_status = ad->monitor.software_breakpoint_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > > > ..and here.. > > - ad->monitor.software_breakpoint_enabled = !status; > + ad->monitor.software_breakpoint_enabled = !old_status; > domain_unpause(d); > break; > } > > case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: > { > - bool_t status = ad->monitor.guest_request_enabled; > + bool_t old_status = ad->monitor.guest_request_enabled; > > - rc = status_check(mop, status); > - if ( rc ) > - return rc; > + if ( unlikely(old_status == requested_status) ) > + return -EEXIST; > > domain_pause(d); > ad->monitor.guest_request_sync = mop->u.guest_request.sync; > > > ..and here. > > - ad->monitor.guest_request_enabled = !status; > + ad->monitor.guest_request_enabled = !old_status; > domain_unpause(d); > break; > } > > > Otherwise the patch looks good. > > Thanks, > Tamas > Oh, right, that would look better. Shall I send a v5 then with that change? (and if yes I guess it won't hurt if I also include the left-shift sanity checks I mentioned I should have added (?)) Thanks, Corneliu. --------------050106020309050809080503 Content-Type: text/html; charset=utf-8 Content-Length: 8113 Content-Transfer-Encoding: quoted-printable
On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:

@@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)

=C2=A0 =C2=A0 =C2=A0case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
=C2=A0 =C2=A0 =C2=A0{

So since we will now have two separate booleans, requested_status and old_status and then manually verify they are opposite..
=C2=A0
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t status =3D ad->monitor.mov_to_msr_enabled;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t old_status =3D ad->monitor.mov_to_msr_enabled;

...here we should set the field to requested_status, not !old_status. While they are technically equivalent, the code would read better to other way around.
=C2=A0

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.mov_to_msr_enabled =3D !status;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.mov_to_msr_enabled =3D !old_status;=C2=A0
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_unpause(d);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
=C2=A0 =C2=A0 =C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t status =3D ad->monitor.singlestep_enabled;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t old_status =3D ad->monitor.singlestep_enabled;

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D status_check(mop, status);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rc )
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return rc;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( unlikely(old_status =3D=3D requested_status) )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EEXIST;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_pause(d);

Here as well..
=C2=A0
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.singlestep_enabled =3D !status;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.singlestep_enabled =3D !old_status;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_unpause(d);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
=C2=A0 =C2=A0 =C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t status =3D ad->monitor.software_breakpoint_enabled;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t old_status =3D ad->monitor.software_breakpoint_enabled;

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D status_check(mop, status);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rc )
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return rc;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( unlikely(old_status =3D=3D requested_status) )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EEXIST;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_pause(d);

..and here..
=C2=A0
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.software_breakpoint_enabled =3D !status;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.software_breakpoint_enabled =3D !old_status;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_unpause(d);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
=C2=A0 =C2=A0 =C2=A0}

=C2=A0 =C2=A0 =C2=A0case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
=C2=A0 =C2=A0 =C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t status =3D ad->monitor.guest_request_enabled;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 bool_t old_status =3D ad->monitor.guest_request_enabled;

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D status_check(mop, status);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( rc )
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return rc;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if ( unlikely(old_status =3D=3D requested_status) )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EEXIST;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_pause(d);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ad->monitor.guest_request_sync =3D mop->u.guest_request.sync;

..and here.
=C2=A0
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.guest_request_enabled =3D !status;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 ad->monitor.guest_request_enabled =3D !old_status;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0domain_unpause(d);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
=C2=A0 =C2=A0 =C2=A0}

Otherwise the patch looks good.

Thanks,
Tamas


Oh, right, that would look better. Shall I send a v5 then with that change=3F (and if yes I guess it won't hurt if I also include the left-shift sanity checks I mentioned I should have added (=3F))

Thanks,
Corneliu.
--------------050106020309050809080503-- --===============5160611010830515592== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============5160611010830515592==--