Re: [Qemu-devel] [PATCH 5/5] spapr: Use unplug_request for PCI hot unplug

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

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

On Wed, Jun 21, 2017 at 11:50:08AM +0200, Greg Kurz wrote:
> On Tue, 20 Jun 2017 09:53:32 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > AIUI, ->unplug_request in the HotplugHandler is used for "soft"
> > unplug, where acknowledgement from the guest is required before
> > completing the unplug, whereas ->unplug is used for "hard" unplug
> > where qemu unilaterally removes the device, and the guest just has to
> > cope with its sudden absence.  For spapr we (correctly) use
> > ->unplug_request for CPU and memory hot unplug but we use ->unplug for  
> > PCI.
> > 
> > While I think it might be possible to support "hard" PCI unplug within
> > the PAPR model, that's not how it actually works now.  Although it's
> > called from ->unplug, the PCI unplug path will usually just mark the
> > device for removal, with completion of the unplug delayed until
> > userspace responds to the unplug notification. If the guest doesn't
> > respond as expected, that could delay the unplug completiong
> > arbitrarily long.
> > 
> > To reflect that, change the PCI unplug path to be called from
> > ->unplug_request.  
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index f2543ef..bda8938 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1469,8 +1469,8 @@ out:
> >      }
> >  }
> >  
> > -static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> > -                                       DeviceState *plugged_dev, Error **errp)
> > +static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> > +                                     DeviceState *plugged_dev, Error **errp)
> >  {
> >      sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> >      PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> > @@ -1485,6 +1485,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> >      }
> >  
> >      g_assert(drc);
> > +    g_assert(drc->dev == plugged_dev);
> >  
> >      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >      if (!drck->release_pending(drc)) {
> > @@ -1973,7 +1974,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
> >      dc->user_creatable = true;
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >      hp->plug = spapr_phb_hot_plug_child;
> 
> Maybe you can rename spapr_phb_hot_plug_child() to spapr_pci_plug() for
> consistency ?

Good idea.  I've made that change.

> 
> Anyway,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > -    hp->unplug = spapr_phb_hot_unplug_child;
> > +    hp->unplug_request = spapr_pci_unplug_request;
> >  }
> >  
> >  static const TypeInfo spapr_phb_info = {
> 



-- 
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 --]