On 3/3/2016 6:15 PM, Razvan Cojocaru wrote:
On 03/03/2016 04:10 PM, Corneliu ZUZU wrote:
Q2) About VM_EVENT_FLAG_DENY
Q2.1)
Doesn't it require sync = 1 (i.e. the vcpu to be paused) to work?
If so, shouldn't we call vm_event_register_write_resume only after checking
that VM_EVENT_FLAG_VCPU_PAUSED flag is set (vm_event_resume). Moreover, if
we do that, wouldn't it be 'cleaner' to rename
vm_event_register_write_resume->vm_event_deny, check for the
VM_EVENT_FLAG_DENY flag in vm_event_resume instead and call vm_event_deny
from there after this check?
Yes, it does require the VCPU to be paused to work, and yes, it's a good
idea to check that that flag is set in the response.
Beyond that, I'd prefer we keep vm_event_register_write_resume()
because, while today all we do there is check that VM_EVENT_FLAG_DENY is
set, we might want do to other things as well there as well (for
example, maybe validate the content of some register). That was really
the point of the "bigger-named" function.
But that's just my opinion, if Tamas prefers your rename suggestion I'll
consider myself outnumbered.
Oh, ok, then the check for VM_EVENT_FLAG_VCPU_PAUSED would be done in
vm_event_register_write_resume instead of vm_event_resume,
since I suppose vm_event_register_write_resume shouldn't be called only
when the vcpu is paused, we only apply that to the DENY flag, correct?
Q2.2)
VM_EVENT_FLAG_DENY functionality is not implemented w/ this change-set.
What is done instead is that the control-register write is done *before*
sending the vm-event (vm_event_put_request). This way, the user can
override the written register value after receiving the vm-event, which
in effect provides the same flexibility as VM_EVENT_FLAG_DENY does.
Using this strategy instead would simplify both Xen's code and the libxc
user's code.
Thoughts?
That's how I initially did it with CR events, but with an application
dealing with huge numbers of events, an extra hypercall (to re-set the
register) can be quite expensive, so I had to rework it to the present
state. On these grounds I'm opposed to it - and for consistency I would
prefer that all register write events are pre-write events, and deniable
with a single vm_event reply.
Ah, I understand. I figured the utility of the DENY flag was only for
cases where you'd want to actively
override the value written to the register (set register to overridden
value + resume w/ DENY), instead
of just forbidding the write and leaving the register untouched (i.e.
set on the old value).
If the latter is a desirable functionality then it indeed makes sense
to have a DENY flag.
With that said, another thing crossed my mind. Since the DENY flag will
be implemented for ARM w/ the next
revision and the actual write will be done on the scheduler tail,
similarly to X86 ((hvm_do_resume), wouldn't
it be good if we separated the code that checks monitor_write_data from
there into an arch-dependent function,
e.g. vm_event_monitor_write_data? That way the scheduler tail function
won't be 'polluted' w/ that code and IMHO
it will make the vm-events design more clear (since that functionality
will also be in vm_event.c along w/ the other
vm_event_* functions).
HTH,
Razvan
Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
