[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
  • From: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
  • Date: Tue, 16 Feb 2016 19:48:20 +0200
  • Cc: Keir Fraser <keir@xxxxxxx>, Ian Campbell <ian.campbell@xxxxxxxxxx>, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • Comment: DomainKeys? See http://domainkeys.sourceforge.net/
  • Delivery-date: Tue, 16 Feb 2016 17:48:41 +0000
  • Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=bitdefender.com; b=Ii/ffGnSej/PXNHHfD1rQrskBtb8R5XaP3vI/JDEnvUx/6DsVZwTUnBbWGod1GfG29p50C1uEB0TRjf0zzViSHYm6fCKgmrVIWGWM61kjL6kMDvnn1GBT6QhF7khJsKkdN4Z1+s6JoiXzkUwygqK2S8ab8r6zpV9p2mH2DkUpjizpsQLooIvmcpxBLrFvHKgKpmKzyZ53jyBXwDhP2MIwj9Pa0ck9pp5hlBqEFRemXHkavZHw/uoYziT3Z3dziKkzdAn81+rEupdfHGzM02iM1yqylXjWEC7HF065iz7UMU5qW0O2+JmUHzsaHFUpYm+5wb2+Kf8R12GZpbJS8otHQ==; h=Received:Received:Received:Received:Received:Subject:To:References:Cc:From:Message-ID:Date:User-Agent:MIME-Version:In-Reply-To:Content-Type:X-BitDefender-Scanner:X-BitDefender-Spam:X-BitDefender-SpamStamp:X-BitDefender-CF-Stamp;
  • List-id: Xen developer discussion <xen-devel.lists.xen.org>

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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.