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

Re: [Xen-devel] [PATCH 0/1] ARM: Implement support for write-ctrlreg vm-events

On 3/3/2016 6:15 PM, Razvan Cojocaru wrote:
On 03/03/2016 04:10 PM, Corneliu ZUZU wrote:

     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?

     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.
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).



Xen-devel mailing list



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