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

[Greg Kurz]

On Tue, 20 Jun 2017 09:53:32 +0800
David Gibson <address@hidden> 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 <address@hidden>
> ---
>  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 ?

Anyway,

Reviewed-by: Greg Kurz <address@hidden>

> -    hp->unplug = spapr_phb_hot_unplug_child;
> +    hp->unplug_request = spapr_pci_unplug_request;
>  }
>  
>  static const TypeInfo spapr_phb_info = {

pgpRakGkNeAcz.pgp
Description: OpenPGP digital signature