Re: [Qemu-devel] [PATCH 4/4] target-ppc: Handle NMI guest exit

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2362 bytes --]

On Fri, Nov 13, 2015 at 08:03:51AM +0100, Thomas Huth wrote:
> On 13/11/15 02:57, David Gibson wrote:
> > On Thu, Nov 12, 2015 at 10:40:11AM +0100, Thomas Huth wrote:
> >> On 12/11/15 09:09, Thomas Huth wrote:
> >>> On 11/11/15 18:16, Aravinda Prasad wrote:
> [...]
> >>>> +    qemu_mutex_lock(&spapr->mc_in_progress);
> >>>
> >>> Using a mutex here is definitely wrong. The kvm_arch_handle_exit() code
> >>> is run under the Big QEMU Lock™ (see qemu_mutex_lock_iothread() in
> >>> kvm_cpu_exec()),
> [...]
> >> Ok, I now had a look into the LoPAPR spec, and if I've got that right,
> >> you really have to serialize the NMIs in case they happen at multiple
> >> CPUs at the same time. So I guess the best thing you can do here is
> >> something like:
> >>
> >>    while (spapr->mc_in_progress) {
> >>        /*
> >>         * There is already another NMI in progress, thus we need
> >>         * to yield here to wait until it has been finsihed
> >>         */
> >>        qemu_mutex_unlock_iothread();
> >>        usleep(10);
> >>        qemu_mutex_lock_iothread();
> >>    }
> >>    spapr->mc_in_progress = true;
> [...]
> > You should be able to avoid the nasty usleep by using a pthread
> > condition variable.  So here you'd have
> > 
> >     while (spapr->mc_in_progress) {
> >         pthread_cond_wait(&mc_delivery_cond, &qemu_iothread_lock);
> >     }
> >     spapr->mc_in_progress = true;
> > 
> > Or.. there may be qemu wrappers around the pthread functions you
> > should be using.  Once delivery of a single MC is complete, you'd use
> > pthread_cond_signal() to wake up any additional ones.
> > 
> > pthread_cond_wait automatically drops the specified mutex internally,
> > so access to mc_in_progress itself is still protected by the iothread
> > mutex.
> 
> That's a nice one, didn't know that function yet! And actually, there is
> already a QEMU wrapper function: qemu_cond_wait() - so this should be
> used instead since threads on Windows are working differently in QEMU as
> far as I know.

Ah, good point, you definitely need to use the qemu wrapper in order
to get proper windows support.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]