Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Feb 16, 2016 at 10:48 AM, Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx> wrote:

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 (?))

Please do send another revision with these changes. As I understood Jan prefers the sanity-checks to be added as a separate patch, so do that.

Thanks,

Tamas

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