Re: [PATCH V2] firmware: Change level-triggered GPE event to a edge one for qemu-xen

From: Anthony PERARD <anthony.perard@citrix.com>
To: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Jan Beulich <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH V2] firmware: Change level-triggered GPE event to a edge one for qemu-xen
Date: Fri, 13 Dec 2013 18:17:03 +0000	[thread overview]
Message-ID: <20131213181703.GY10855@perard.uk.xensource.com> (raw)
In-Reply-To: <20131213174408.GA15174@phenom.dumpdata.com>

On Fri, Dec 13, 2013 at 12:44:08PM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Dec 13, 2013 at 05:11:58PM +0000, Anthony PERARD wrote:
> > On Fri, Dec 13, 2013 at 11:51:19AM +0000, Jan Beulich wrote:
> > > >>> On 12.12.13 at 16:30, Anthony PERARD <anthony.perard@citrix.com> wrote:
> > > > Change in V2:
> > > >   - better patch comment:
> > > >     patch those not fix race, but reduce the window
> > > >     include patch description of the quoted commit
> > > 
> > > Thanks - quite a bit better to understand.
> > > 
> > > >   - change also apply to pci hotplug.
> > > 
> > > The one thing I'm still missing for both changes is a brief word on
> > > why qemu-xen-traditional doesn't want/need this and - as iirc
> > > you said - if that one manages to get this implemented without a
> > > similar race, why upstream qemu can't do things in a similar way.
> > 
> > In qemu-trad, instead of sending an SCI interrupt for every new vcpu, we
> > loop through every xenstore key that represent vcpus availability and
> > only send an SCI only when the loop is over. And it looks like one `xl
> > vcpu-set` provoc only one loop.
> > 
> > But adding a vcpu in qemu-xen is done via a QMP command, "cpu-add id=X".
> > qemu-xen have no way to know if there will be a next cpu-add command, so
> > we can not apply the same thing.
> 
> Can it have a workqueue (does such thing exist in QEMU?) with a list - so
> that every time you get an cpu-add it puts the 'vcpuX' on this command list
> and the thread wakes up, reads up all of the commands it needs, and then
> dispatches it?
> 
> That could also be used for VCPU hotplug .. Actually it could be used
> for any QMP command.
> 
> > 
> > For why qemu-xen-traditional doesn't not need this:
> >   - a single `xl vcpu-set` can not trigger the race
> > 
> > For why qemu-xen-traditional doesn't not want this:
> >   - avoid unnecessary change, especially in the ACPI table
> 
> That is not right. The reason I didn't do it is b/c it was not enough.
> I could still trigger the the race with the change.

Oh, I see, so this change won't be very useful to qemu-trad. I did not
read qemu-trad's code properly.

Thanks,

-- 
Anthony PERARD