| Message ID | 20170620015332.13874-6-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 20/06/2017 03:53, David Gibson 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> Reviewed-by: Laurent Vivier <lvivier@redhat.com> > --- > 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; > - hp->unplug = spapr_phb_hot_unplug_child; > + hp->unplug_request = spapr_pci_unplug_request; > } > > static const TypeInfo spapr_phb_info = { >
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 ? 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 = {
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 = { >
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; - hp->unplug = spapr_phb_hot_unplug_child; + hp->unplug_request = spapr_pci_unplug_request; } static const TypeInfo spapr_phb_info = {
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(-)