Re: [PATCH v3 2/7] spapr_pci.c: simplify spapr_pci_unplug_request() function handling

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

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

On Tue, Feb 16, 2021 at 02:44:44PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 2/16/21 2:16 PM, Greg Kurz wrote:
> > On Tue, 16 Feb 2021 13:09:43 -0300
> > Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> > 
> > > 
> > > 
> > > On 2/16/21 12:50 PM, Greg Kurz wrote:
> > > > On Thu, 11 Feb 2021 19:52:41 -0300
> > > > Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> > > > 
> > > > > When hotunplugging a PCI function we'll branch out the logic in two cases,
> > > > > function zero and non-zero. If non-zero, we'll call spapr_drc_detach() and
> > > > > nothing else. If it's function zero, we'll loop it once between all the
> > > > > functions in the slot to call spapr_drc_detach() on them, and afterwards
> > > > > we'll do another backwards loop where we'll signal the event to the guest.
> > > > > 
> > > > > We can simplify this logic. We can ignore all the DRC handling for non-zero
> > > > > functions, since we'll end up doing that regardless when unplugging function
> > > > > zero. And for function zero, everything can be done in a single loop, since
> > > > > tt doesn't matter if we end up marking the function DRCs as unplug pending in
> > > > > backwards order or not, as long as we call spapr_drc_detach() before issuing
> > > > > the hotunplug event to the guest.
> > > > > 
> > > > > This will also avoid a possible scenario where the user starts to hotunplug
> > > > > the slot, starting with a non-zero function, and then delays/forgets to
> > > > > hotunplug function zero afterwards. This would keep the function DRC marked
> > > > > as unplug requested indefinitely.
> > > > > 
> > > > 
> > > > ... or until the guest is reset, which will no longer happen with this
> > > > patch applied, i.e. breaks the long standing policy that machine reset
> > > > causes pending hot-unplug requests to succeed. I don't see an obvious
> > > > reason to special case non-zero PCI functions.
> > > 
> > > It's not possible to hotunplug the non-zero functions during machine reset for
> > > multifunction PCI devices. We need to unplug the entire slot, and that will only
> > > happen when function zero is unplugged. In fact, I think bad things will happen
> > > in this case you mentioned if we are forcing the removal of non-zero functions
> > > without function zero (spoiler: didn't test it).
> > > 
> > 
> > I've tested with the aggregation of two e1000e emulated devices:
> > 
> > device_add e1000e,addr=10.1,id=netfn1
> > device_add e1000e,multifunction=on,addr=10.0,id=netfn0
> > 
> > And I don't quite see what "bad things" could happen. We're resetting the
> > machine to a stable state and the new OS instance will just not see the
> > removed function (just like only function netfn0 got added).
> 
> 
> Interesting. Thanks for looking this up.
> 
> Given that the intention of this patch was a simplification of the existing
> design, without changing what we currently do regarding PCI functions and unplug,
> and apparently it just did that, let's drop it.

I think that's best.  As Greg says, I think maintaining the behaviour
that reset completes pending hotplugs should be retained, and the
usual constraints on non-zero function hot-unplug don't apply at reset
time.

-- 
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: 833 bytes --]