Re: [Xen-devel] Locking on vm-event operations (monitor)

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 07/22/2016 12:27 PM, Corneliu ZUZU wrote:
> Hi,
> 
> I've been inspecting vm-event code parts to try and understand when and
> why domain pausing/locking is done. If I understood correctly, domain
> pausing is done solely to force all the vCPUs of that domain to see a
> configuration update and act upon it (e.g. in the case of a
> XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG that enables CR3 monitoring,
> domain pausing/unpausing ensures immediate enabling of CR3 load-exiting
> for all vCPUs), not to synchronize concurrent operations (lock-behavior).
> 
> As for locking, I see that for example vm_event_init_domain(),
> vm_event_cleanup_domain() and monitor_domctl() are all protected by the
> domctl-lock, but I don't think that's enough.
> Here are a few code-paths that led me to believe that:
> * do_hvm_op()->monitor_guest_request() reads d.monitor.guest_request_*
> resources, but it doesn't seem to take the domctl lock, so it seems
> possible for that to happen _while_ those resources are
> initialized/cleaned-up
> * monitor_guest_request() also calls
> monitor_traps()->...->vm_event_wait_slot()->...->vm_event_grab_slot()
> which attempts a vm_event_ring_lock(ved), which could also be called
> _while_ that's initialized (vm_event_enable()) or cleaned-up
> (vm_event_disable())
> * hvm_monitor_cr() - e.g. on the code-path
> vmx_vmexit_handler(EXIT_REASON_CR_ACCESS)->vmx_cr_access(VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR)->hvm_mov_to_cr()->hvm_set_cr0()->hvm_monitor_crX()
> there doesn't seem to be taken into account the possibility of a
> concurrent monitor_init_domain()/monitor_cleanup_domain()
> 
> Am I missing something with these conclusions?

Your conclusions look correct, but I assume that the reason why this has
not been addressed in the past is that introspection applications are
expected to be well-behaved. Specifically, in a codebase where the
choice between uint64_t and long int matters speed-wise, and where
unlikely()s also matter, an extra lock may be an issue.

The typical flow of an introspection application is:

1. Initialize everything.
2. Subscribe to relevant events.
3. Event processing loop.
4. Unsubscribe from events.
5. Do a last-run of event processing (already queued in the ring buffer).
6. Uninitialize everything (no events are possible here because of steps
4-5).

> As a resolution for this, I've been thinking of adding a 'subsys_lock'
> field in the vm_event_domain structure, either spinlock or rw-lock,
> which would be initialised/uninitialised when d.vm_event is
> allocated/freed (domain_create()/complete_domain_destroy()).

I have nothing against this. Having as many assurances as possible that
things will work is definitely a plus in my book - with the comment that
I would prefer a rwlock to an ordinary spinlock, and that "subsys_lock"
sounds obscure to me, although I admit that I can't think of a good name
at the moment.


Thanks,
Razvan

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