Re: [PATCH V3 2/3] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/07/2015 02:01 PM, George Dunlap wrote:
> On 07/06/2015 04:51 PM, Razvan Cojocaru wrote:
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 896acf7..f8df7d2 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -161,6 +161,22 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>>          break;
>>      }
>>  
>> +    case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
>> +    {
>> +        bool_t status = ad->monitor.guest_request_enabled;
>> +
>> +        rc = status_check(mop, status);
>> +        if ( rc )
>> +            return rc;
>> +
>> +        ad->monitor.guest_request_sync = mop->u.guest_request.sync;
>> +
>> +        domain_pause(d);
>> +        ad->monitor.guest_request_enabled = !status;
> 
> If I'm reading this right, what this hypercall does is *toggle* guest
> requests?
> 
> Wouldn't it make more sense to either set it or clear it, rather than
> have the caller need to keep track (or guess) what state it's in?

First of all, thanks for the review!

No, it doesn't just get toggled (you can see that all the other cases
above in that file handle things the same way), although I that was my
impression too when I first came across that code.

The code is a bit involved:

 31 /*
 32  * Sanity check whether option is already enabled/disabled
 33  */
 34 static inline
 35 int status_check(struct xen_domctl_monitor_op *mop, bool_t status)
 36 {
 37     bool_t requested_status = (mop->op == XEN_DOMCTL_MONITOR_OP_ENABLE);
 38
 39     if ( status == requested_status )
 40         return -EEXIST;
 41
 42     return 0;
 43 }

So this function checks to see if the requested status is the same as
the current status. If it's the same, it returns -EEXIST, corresponding
to the "if ( rc ) return rc;" part in the quoted code above.

If it returns 0, then toggling the status is the right thing to do.
Again, the rest of the cases above this one handle it in the exact same
manner, this bit has been more or less of a copy / paste job.


Thanks,
Razvan