[Qemu-devel] [PATCH v1 1/1] spapr_pci: remove all child functions in function zero unplug

[Qemu-devel] [PATCH v1 1/1] spapr_pci: remove all child functions in function zero unplug

[Daniel Henrique Barboza]

There is nothing wrong with how sPAPR handles multifunction PCI
hot unplugs. The problem is that x86 does it simpler. Instead of
removing each non-zero function and then removing function zero,
x86 can remove any function of the slot to trigger the hot unplug.

Libvirt will be directly impacted by this difference, in the
(hopefully soon) PCI Multifunction hot plug/unplug support. For
hot plugs, both x86 and sPAPR will operate the same way: a XML
with all desired functions to be added, then consecutive hotplugs
of all non-zero functions first, zero last. For hot unplugs, at
least in the current state, a XML with the devices to be removed
must also be provided because of how sPAPR operates - x86 does
not need it - since any function unplug will unplug the whole
PCIe slot. This difference puts extra strain in the management
layer, which needs to either handle both archs differently in
the unplug scenario or choose treat x86 like sPAPR, forcing x86
users to cope with sPAPR internals.

This patch changes spapr_pci_unplug_request to handle the
unplug of function zero differently. When removing function zero,
instead of error-ing out if there are any remaining function
DRCs which needs detaching, detach those. This has no effect in
any existing scripts that are detaching the non-zero functions
before function zero, and can be used by management as a shortcut
to remove the whole PCI multifunction device without specifying
each child function.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_pci.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index deb0b0c80c..9f176f463e 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
                 state = func_drck->dr_entity_sense(func_drc);
                 if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
                     && !spapr_drc_unplug_requested(func_drc)) {
-                    error_setg(errp,
-                               "PCI: slot %d, function %d still present. "
-                               "Must unplug all non-0 functions first.",
-                               slotnr, i);
-                    return;
+                    /*
+                     * Attempting to remove function 0 of a multifunction
+                     * device will will cascade into removing all child
+                     * functions, even if their unplug weren't requested
+                     * beforehand.
+                     */
+                    spapr_drc_detach(func_drc);
                 }
             }
         }
-- 
2.21.0

Re: [Qemu-devel] [PATCH v1 1/1] spapr_pci: remove all child functions in function zero unplug

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1516 bytes --]

On 8/22/19 2:59 PM, Daniel Henrique Barboza wrote:
> There is nothing wrong with how sPAPR handles multifunction PCI
> hot unplugs. The problem is that x86 does it simpler. Instead of
> removing each non-zero function and then removing function zero,
> x86 can remove any function of the slot to trigger the hot unplug.
> 

> +++ b/hw/ppc/spapr_pci.c
> @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>                  state = func_drck->dr_entity_sense(func_drc);
>                  if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
>                      && !spapr_drc_unplug_requested(func_drc)) {
> -                    error_setg(errp,
> -                               "PCI: slot %d, function %d still present. "
> -                               "Must unplug all non-0 functions first.",
> -                               slotnr, i);
> -                    return;
> +                    /*
> +                     * Attempting to remove function 0 of a multifunction
> +                     * device will will cascade into removing all child
> +                     * functions, even if their unplug weren't requested

s/weren't/wasn't/

> +                     * beforehand.
> +                     */
> +                    spapr_drc_detach(func_drc);
>                  }
>              }
>          }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v1 1/1] spapr_pci: remove all child functions in function zero unplug

[David Gibson]

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

On Thu, Aug 22, 2019 at 04:11:45PM -0500, Eric Blake wrote:
> On 8/22/19 2:59 PM, Daniel Henrique Barboza wrote:
> > There is nothing wrong with how sPAPR handles multifunction PCI
> > hot unplugs. The problem is that x86 does it simpler. Instead of
> > removing each non-zero function and then removing function zero,
> > x86 can remove any function of the slot to trigger the hot unplug.
> > 
> 
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> >                  state = func_drck->dr_entity_sense(func_drc);
> >                  if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
> >                      && !spapr_drc_unplug_requested(func_drc)) {
> > -                    error_setg(errp,
> > -                               "PCI: slot %d, function %d still present. "
> > -                               "Must unplug all non-0 functions first.",
> > -                               slotnr, i);
> > -                    return;
> > +                    /*
> > +                     * Attempting to remove function 0 of a multifunction
> > +                     * device will will cascade into removing all child
> > +                     * functions, even if their unplug weren't requested
> 
> s/weren't/wasn't/

Actually, I think this is technically a subjunctive, which would make
it "were" rather than "was".  Modern English usage doesn't really do
the subjunctive, though.
</more-pedantic-than-thou>

With my maintainer hat on, rather than pedant hat, the meaning is
clear so I really don't care, especially from a contributer whose
first language isn't English.

> 
> > +                     * beforehand.
> > +                     */
> > +                    spapr_drc_detach(func_drc);
> >                  }
> >              }
> >          }
> > 
> 




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

Re: [Qemu-devel] [PATCH v1 1/1] spapr_pci: remove all child functions in function zero unplug

[David Gibson]

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

On Thu, Aug 22, 2019 at 04:59:18PM -0300, Daniel Henrique Barboza wrote:
> There is nothing wrong with how sPAPR handles multifunction PCI
> hot unplugs. The problem is that x86 does it simpler. Instead of
> removing each non-zero function and then removing function zero,
> x86 can remove any function of the slot to trigger the hot unplug.
> 
> Libvirt will be directly impacted by this difference, in the
> (hopefully soon) PCI Multifunction hot plug/unplug support. For
> hot plugs, both x86 and sPAPR will operate the same way: a XML
> with all desired functions to be added, then consecutive hotplugs
> of all non-zero functions first, zero last. For hot unplugs, at
> least in the current state, a XML with the devices to be removed
> must also be provided because of how sPAPR operates - x86 does
> not need it - since any function unplug will unplug the whole
> PCIe slot. This difference puts extra strain in the management
> layer, which needs to either handle both archs differently in
> the unplug scenario or choose treat x86 like sPAPR, forcing x86
> users to cope with sPAPR internals.
> 
> This patch changes spapr_pci_unplug_request to handle the
> unplug of function zero differently. When removing function zero,
> instead of error-ing out if there are any remaining function
> DRCs which needs detaching, detach those. This has no effect in
> any existing scripts that are detaching the non-zero functions
> before function zero, and can be used by management as a shortcut
> to remove the whole PCI multifunction device without specifying
> each child function.

Makes sense to me, applied to ppc-for-4.2.

> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_pci.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index deb0b0c80c..9f176f463e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1700,11 +1700,13 @@ static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>                  state = func_drck->dr_entity_sense(func_drc);
>                  if (state == SPAPR_DR_ENTITY_SENSE_PRESENT
>                      && !spapr_drc_unplug_requested(func_drc)) {
> -                    error_setg(errp,
> -                               "PCI: slot %d, function %d still present. "
> -                               "Must unplug all non-0 functions first.",
> -                               slotnr, i);
> -                    return;
> +                    /*
> +                     * Attempting to remove function 0 of a multifunction
> +                     * device will will cascade into removing all child
> +                     * functions, even if their unplug weren't requested
> +                     * beforehand.
> +                     */
> +                    spapr_drc_detach(func_drc);
>                  }
>              }
>          }

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