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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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.

> 
> About the PCI hotplug change and qemu-xen-traditional, I actually don't
> know what would be the effect.
> 
> Why qemu-xen-traditional would like this change for vcpu only:
>   - we might hit the race window via several consecutive `xl vcpu-set`
>     calls, or via direct change of xenstore entries.
> but after few try, I could not manage to have a missing cpu hotplug
> event.
> 
> 
> So should I just add in the patch comment something like:
> "Since some measure have been put in place in qemu-xen-traditional
> against this race, we don't change the ACPI table for it."
> ?
> 
> > Also iirc someone responded to v1 with a comment saying a race
> > like this is an OS bug - I don#t think I've seen a response to this.
> 
> I can not find this comment. But the fact that a race can happen does
> not look like an OS bug. Also, I've discrabed in the patch comment the
> behavior of QEMU (hardware) and what an OS is suppose to do with the
> AML as specified in the ACPI document.
> 
> -- 
> Anthony PERARD