On Tue, 20 Jun 2017 09:53:32 +0800 David Gibson <da...@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 <da...@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 ?
Anyway,
Reviewed-by: Greg Kurz <gr...@kaod.org>
> - hp->unplug = spapr_phb_hot_unplug_child;
> + hp->unplug_request = spapr_pci_unplug_request;
> }
>
> static const TypeInfo spapr_phb_info = {
pgpHtIhnlux50.pgp
Description: OpenPGP digital signature
