Re: [Qemu-devel] [PATCH 0/6] PCI hotplug improvements

[Gleb Natapov]

On Wed, Mar 07, 2012 at 02:44:13PM -0700, Alex Williamson wrote:
> On Wed, 2012-03-07 at 23:00 +0200, Gleb Natapov wrote:
> > On Wed, Mar 07, 2012 at 12:51:48PM -0700, Alex Williamson wrote:
> > > On Wed, 2012-03-07 at 20:59 +0200, Gleb Natapov wrote:
> > > > On Wed, Mar 07, 2012 at 10:20:49AM -0700, Alex Williamson wrote:
> > > > > On Wed, 2012-03-07 at 14:43 +0200, Gleb Natapov wrote:
> > > > > > On Tue, Mar 06, 2012 at 05:13:36PM -0700, Alex Williamson wrote:
> > > > > > > Here's a re-work of the patch that added _STA for the purpose of
> > > > > > > using it as an ack from the guest.  Instead of that, add a 
> > > > > > > notifier
> > > > > > > for device access.  Once the guest reads from device config space,
> > > > > > > it owns it.  Until that point, we can remove it directly.  As 
> > > > > > > pointed
> > > > > > > out by MST, this passes test b) below, which the _STA method 
> > > > > > > would not.
> > > > > > > As a bonus, no bios change is required for this.  Patches 5 & 6 
> > > > > > > are
> > > > > > > just cleanups that can be applied independently.  Thanks,
> > > > > > > 
> > > > > > While I agree with Michael that using _STA as ack is a hack I think
> > > > > > this approach is not less of a hack. It is unlikely that this is 
> > > > > > how it
> > > > > > work on bare metal and we should follow real HW if possible.
> > > > > 
> > > > > The test below is the only thing that proved to me it was less of a
> > > > > hack.  Introducing a _LCK method for a slot may be another way to do
> > > > > this.  Unfortunately it's not required that the OSPM call _LCK and 
> > > > > it's
> > > > > not mentioned in the msft document referenced previously.
> > > > >  
> > > > I do not understand where this requirement, that device_del should work
> > > > if non-acpi guest is running, is coming from? Because if there is no
> > > > such requirement then the hack is not needed.
> > > 
> > > Where do I file my TPS report? ;)  This is simply trying to add some
> > > definition to "when is a hot attach completed".  Once we know that, we
> > > can consider the device owned by the guest after that point.  Prior to
> > > that, we can allow the cancellation of a hot attach, by directly
> > > removing the device.  After that point, we have to ask permission from
> > > the guest or deal with surprise removal.
> > > 
> > That just heuristic based on observation of several guests, no? device_add
> > sends sci interrupt to a guest. How do we know that some guest will not
> > get upset if it will not find promised device after getting the interrupt
> > and executing Notify()?
> 
> There's no promise of a device with a device check notify.  It's defined
What do you mean?

> as both addition and removal (I imagine this is what we'd call if we did
> a surprise removal).  The guest needs to go test if there's something
Notify is called with different parameters for addition and removal.
Addition and removal look very different for a guest.

> there (we don't provide a _STA method, so it has to go probe the
We should. Just not use it to ack insertion.

> device).  Hopefully guests are robust enough to not fall over when they
> discover there was nothing there and there still is nothing there.  This
> is the same thing that might happen on a physical system if a power
> latch is broken and the card fails to power up.
> 
> > 
> > > There are two use cases I know of that make the non-ACPI device_del, or
> > > really device_add cancellation, useful.  The first is what we've been
> > > discussing, that not all guests will support ACPI based PCI hotplug and
> > > devices can be lost to the guest until shutdown, including assigned
> > I do not know how non ACPI based PCI hotplug works. I can only assume
> > that they have a way to notify device that it can be removed. The patch
> > series is about ACPI though. The patch series also does not solve lost
> > device problem since guest can rescan the bus and claim device forever.
> > You showed this in your original mail yourself.
> 
> That falls under surprise removal, which as noted before, I'm not
> attempting to address here.  Once the guest has accessed the device,
> using the spec defined probe method, we have to assume it owns it.
I do not understand what do you mean by surprise removal. I am not
talking about surprise removal here.

> 
> > > devices.  The second is something we more commonly run into in testing,
> > > that between and 'add' & 'del' (or del & add), there's some undefined
> > > delay required to prevent us stepping on ourselves.  For instance, if we
> > > do an 'add' immediately followed by a 'del', we clear the 'up' register,
> > > set the 'down' register and hope that the guest removes a device that it
> > > never knew existed.  This code allows that to work as expected, removing
> > That's QEMU problem and it should be solved in QEMU. What if we will not
> > clear 'up' bit and let guest process both events add and del?
> 
> This *is* an attempt to solve it in Qemu.  Letting the guest deal with
> both events seems far riskier than this approach.  If we just want to
> clear bits, PCNF should write back each slot to the up/down register as
> it sends the Notify.  That's hardly much of an improvement IMHO though.
> 
I already proposed dropping up/down thing and moving to how cpu hotplug
works. I also do not see why you do not like clearing bits in PCNF.

And we also should have a way to initiate device ejection from a guest.
If Windows user press eject button for a device in the GUI next
device_del should succeed without even sending sci.

> > > the device even thought the guest never saw it.  In the other direction
> > > (del->add), PCI won't create a device in a slot that's already occupied,
> > > so we never actually get to the race there.  Overall, an improvement in
> > > usability IMHO.
> > > 
> > > Once we define an end point for addition, we could actually take it a
> > > step further and add a timeout parameter to device_add, where if the
> > > guest hasn't taken the device before the timeout, we automatically
> > > remove it and device_add returns error.  We might consider doing the
> > > same for device_del.  Thanks,
> > >
> > IMO end point of addition should be sending of sci interrupt After that
> > device should not be removed without guest participation. We can provide
> > forced device_del that yanks device anyway for the cases when device was
> > erroneously added while non-ACPI guest were running.
> 
> In effect, you'd rather give the user a loaded gun for surprise removal,
> pat them on the back and move on?  Thanks,
> 
I'd rather have things working like real HW does. And on real HW you
have this option if nothing else works. And on VM you want this option
anyway since guest may not cooperate but management is determined to get
its assigned device back, so it either has to kill the guest or force
device removal. We have enough ammo in teh monitor to harm a guest already.
This is not user interface this is management interface.

--
                        Gleb.