Re: [RFC for-5.1 4/4] spapr: Don't allow unplug of NVLink2 devices

2020-03-30 Thread David Gibson
On Sat, Mar 28, 2020 at 11:32:18PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 26/03/2020 16:40, David Gibson wrote:
> > Currently, we can't properly handle unplug of NVLink2 devices, because we
> > don't have code to tear down their special memory resources.  There's not
> > a lot of impetus to implement that. Since hardware NVLink2 devices can't
> > be hot unplugged, the guest side drivers don't usually support unplug
> > anyway.
> > 
> > Therefore, simply prevent unplug of NVLink2 devices.
> > 
> > Signed-off-by: David Gibson 
> > ---
> >  hw/ppc/spapr_pci.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 55ca9dee1e..5c8262413a 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1666,6 +1666,11 @@ static void spapr_pci_unplug_request(HotplugHandler 
> > *plug_handler,
> >  return;
> >  }
> >  
> > +if (spapr_phb_is_nvlink_dev(pdev, phb)) {
> > +error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
> > +return;
> > +}
> 
> 
> Just this would do as well:
> 
> Object *po = OBJECT(pdev);
> uint64_t tgt = object_property_get_uint(po, "nvlink2-tgt", NULL);
> 
> if (tgt) {
>  error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
>  return;
> }
> 
> honestly, I admin what 1/4 fixes is cryptic but since there is not going
> to be any more new nvlinkX, this does not deserve this many patches
> imho.

Good point, that is a simpler approach.

> 
>   
> 
> > +
> >  /* ensure any other present functions are pending unplug */
> >  if (PCI_FUNC(pdev->devfn) == 0) {
> >  for (i = 1; i < 8; i++) {
> > 
> 

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


signature.asc
Description: PGP signature


Re: [RFC for-5.1 4/4] spapr: Don't allow unplug of NVLink2 devices

2020-03-28 Thread Alexey Kardashevskiy



On 26/03/2020 16:40, David Gibson wrote:
> Currently, we can't properly handle unplug of NVLink2 devices, because we
> don't have code to tear down their special memory resources.  There's not
> a lot of impetus to implement that. Since hardware NVLink2 devices can't
> be hot unplugged, the guest side drivers don't usually support unplug
> anyway.
> 
> Therefore, simply prevent unplug of NVLink2 devices.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr_pci.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 55ca9dee1e..5c8262413a 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1666,6 +1666,11 @@ static void spapr_pci_unplug_request(HotplugHandler 
> *plug_handler,
>  return;
>  }
>  
> +if (spapr_phb_is_nvlink_dev(pdev, phb)) {
> +error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
> +return;
> +}


Just this would do as well:

Object *po = OBJECT(pdev);
uint64_t tgt = object_property_get_uint(po, "nvlink2-tgt", NULL);

if (tgt) {
 error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
 return;
}

honestly, I admin what 1/4 fixes is cryptic but since there is not going
to be any more new nvlinkX, this does not deserve this many patches imho.



> +
>  /* ensure any other present functions are pending unplug */
>  if (PCI_FUNC(pdev->devfn) == 0) {
>  for (i = 1; i < 8; i++) {
> 

-- 
Alexey



Re: [RFC for-5.1 4/4] spapr: Don't allow unplug of NVLink2 devices

2020-03-26 Thread David Gibson
On Thu, Mar 26, 2020 at 01:27:40PM +0100, Greg Kurz wrote:
> On Thu, 26 Mar 2020 16:40:09 +1100
> David Gibson  wrote:
> 
> > Currently, we can't properly handle unplug of NVLink2 devices, because we
> > don't have code to tear down their special memory resources.  There's not
> > a lot of impetus to implement that. Since hardware NVLink2 devices can't
> > be hot unplugged, the guest side drivers don't usually support unplug
> > anyway.
> > 
> > Therefore, simply prevent unplug of NVLink2 devices.
> > 
> 
> This could maybe considered as a valid fix for 5.0 since this prevents
> guest crashes IIUC. But since this requires the two preliminary cleanup
> patches, I understand you may prefer to postpone that to 5.1.

Yeah, it's arguably a bug, but not a regression, so I'm inclined to
leave it to 5.1.

> 
> > Signed-off-by: David Gibson 
> > ---
> 
> Reviewed-by: Greg Kurz 
> 
> >  hw/ppc/spapr_pci.c | 5 +
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 55ca9dee1e..5c8262413a 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1666,6 +1666,11 @@ static void spapr_pci_unplug_request(HotplugHandler 
> > *plug_handler,
> >  return;
> >  }
> >  
> > +if (spapr_phb_is_nvlink_dev(pdev, phb)) {
> > +error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
> > +return;
> > +}
> > +
> >  /* ensure any other present functions are pending unplug */
> >  if (PCI_FUNC(pdev->devfn) == 0) {
> >  for (i = 1; i < 8; i++) {
> 

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


signature.asc
Description: PGP signature


Re: [RFC for-5.1 4/4] spapr: Don't allow unplug of NVLink2 devices

2020-03-26 Thread Greg Kurz
On Thu, 26 Mar 2020 16:40:09 +1100
David Gibson  wrote:

> Currently, we can't properly handle unplug of NVLink2 devices, because we
> don't have code to tear down their special memory resources.  There's not
> a lot of impetus to implement that. Since hardware NVLink2 devices can't
> be hot unplugged, the guest side drivers don't usually support unplug
> anyway.
> 
> Therefore, simply prevent unplug of NVLink2 devices.
> 

This could maybe considered as a valid fix for 5.0 since this prevents
guest crashes IIUC. But since this requires the two preliminary cleanup
patches, I understand you may prefer to postpone that to 5.1.

> Signed-off-by: David Gibson 
> ---

Reviewed-by: Greg Kurz 

>  hw/ppc/spapr_pci.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 55ca9dee1e..5c8262413a 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1666,6 +1666,11 @@ static void spapr_pci_unplug_request(HotplugHandler 
> *plug_handler,
>  return;
>  }
>  
> +if (spapr_phb_is_nvlink_dev(pdev, phb)) {
> +error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
> +return;
> +}
> +
>  /* ensure any other present functions are pending unplug */
>  if (PCI_FUNC(pdev->devfn) == 0) {
>  for (i = 1; i < 8; i++) {




[RFC for-5.1 4/4] spapr: Don't allow unplug of NVLink2 devices

2020-03-25 Thread David Gibson
Currently, we can't properly handle unplug of NVLink2 devices, because we
don't have code to tear down their special memory resources.  There's not
a lot of impetus to implement that. Since hardware NVLink2 devices can't
be hot unplugged, the guest side drivers don't usually support unplug
anyway.

Therefore, simply prevent unplug of NVLink2 devices.

Signed-off-by: David Gibson 
---
 hw/ppc/spapr_pci.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 55ca9dee1e..5c8262413a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1666,6 +1666,11 @@ static void spapr_pci_unplug_request(HotplugHandler 
*plug_handler,
 return;
 }
 
+if (spapr_phb_is_nvlink_dev(pdev, phb)) {
+error_setg(errp, "PCI: Cannot unplug NVLink2 devices");
+return;
+}
+
 /* ensure any other present functions are pending unplug */
 if (PCI_FUNC(pdev->devfn) == 0) {
 for (i = 1; i < 8; i++) {
-- 
2.25.1