[PATCH v4 00/12] virtio: s4 support

[PATCH v4 00/12] virtio: s4 support

[Amit Shah]

Hi,

These patches add support for S4 to virtio (pci) and all drivers.

For each driver, all vqs are removed before hibernation, and then
re-created after restore.  Some driver-specific uninit and init work
is also done in the freeze and restore functions.

All the drivers in testing work fine:

* virtio-blk is used for the only disk in the VM, IO works fine before
  and after.  'dd if=/dev/zero of=/tmp/bigfile bs=1024 count=200000'
  across S4 gives same sha1sum for the file in the guest as well as
  one that's created without invoking S4.

* virtio-console: port IO keeps working fine before and after.
  * If a port is waiting for data from the host (blocking read(2)
    call), this works fine in both the cases: host-side connection is
    available or unavailable after resume.  In case the host-side
    connection isn't available, the blocking call is terminated.  If
    it is available, the call continues to remain in blocked state
    till further data arrives.

* virtio-net: ping remains active across S4.

* virtio-balloon: Works fine before and after.  Forgets the ballooned
  value across S4 (see details in commit log). Maintains ballooned
  value on failed freeze.

All of these tests are run in parallel.

I have some more tests lined up on similar lines above.  I'll reply
here if something breaks.

Please review and apply if appropriate,

v4:
 - Disable / enable napi across S4 (Michael S. Tsirkin)
 - Balloon: lots of improvements (I had neglected this driver thinking
   it was a simple one, but this one needed the most thought!  Check
   the commit log for patch 12 for details.)
 - Net, Blk: Reset device as the first operation on freeze

v3:
 - Reset vqs before deleting them (Sasha Levin)
 - Flush block queue before freeze (Rusty)
 - Detach netdev before freeze (Michael S. Tsirkin)

v2:
 - fix checkpatch errors/warnings

Amit Shah (12):
  virtio: pci: switch to new PM API
  virtio: pci: add PM notification handlers for restore, freeze, thaw,
    poweroff
  virtio: console: Move out vq and vq buf removal into separate
    functions
  virtio: console: Add freeze and restore handlers to support S4
  virtio: blk: Move out vq initialization to separate function
  virtio: blk: Add freeze, restore handlers to support S4
  virtio: net: Move out vq initialization into separate function
  virtio: net: Move out vq and vq buf removal into separate function
  virtio: net: Add freeze, restore handlers to support S4
  virtio: balloon: ensure thread exists before stopping it
  virtio: balloon: Move out vq initialization into separate function
  virtio: balloon: Add freeze, restore handlers to support S4

 drivers/block/virtio_blk.c      |   57 +++++++++++++++--
 drivers/char/virtio_console.c   |  126 +++++++++++++++++++++++++++++--------
 drivers/net/virtio_net.c        |  102 ++++++++++++++++++++++--------
 drivers/virtio/virtio_balloon.c |  131 +++++++++++++++++++++++++++++++++------
 drivers/virtio/virtio_pci.c     |  101 +++++++++++++++++++++++++++++-
 include/linux/virtio.h          |    5 ++
 6 files changed, 439 insertions(+), 83 deletions(-)

-- 
1.7.7.3

[PATCH v4 00/12] virtio: s4 support

[Amit Shah]

Hi,

These patches add support for S4 to virtio (pci) and all drivers.

For each driver, all vqs are removed before hibernation, and then
re-created after restore.  Some driver-specific uninit and init work
is also done in the freeze and restore functions.

All the drivers in testing work fine:

* virtio-blk is used for the only disk in the VM, IO works fine before
  and after.  'dd if=/dev/zero of=/tmp/bigfile bs=1024 count=200000'
  across S4 gives same sha1sum for the file in the guest as well as
  one that's created without invoking S4.

* virtio-console: port IO keeps working fine before and after.
  * If a port is waiting for data from the host (blocking read(2)
    call), this works fine in both the cases: host-side connection is
    available or unavailable after resume.  In case the host-side
    connection isn't available, the blocking call is terminated.  If
    it is available, the call continues to remain in blocked state
    till further data arrives.

* virtio-net: ping remains active across S4.

* virtio-balloon: Works fine before and after.  Forgets the ballooned
  value across S4 (see details in commit log). Maintains ballooned
  value on failed freeze.

All of these tests are run in parallel.

I have some more tests lined up on similar lines above.  I'll reply
here if something breaks.

Please review and apply if appropriate,

v4:
 - Disable / enable napi across S4 (Michael S. Tsirkin)
 - Balloon: lots of improvements (I had neglected this driver thinking
   it was a simple one, but this one needed the most thought!  Check
   the commit log for patch 12 for details.)
 - Net, Blk: Reset device as the first operation on freeze

v3:
 - Reset vqs before deleting them (Sasha Levin)
 - Flush block queue before freeze (Rusty)
 - Detach netdev before freeze (Michael S. Tsirkin)

v2:
 - fix checkpatch errors/warnings

Amit Shah (12):
  virtio: pci: switch to new PM API
  virtio: pci: add PM notification handlers for restore, freeze, thaw,
    poweroff
  virtio: console: Move out vq and vq buf removal into separate
    functions
  virtio: console: Add freeze and restore handlers to support S4
  virtio: blk: Move out vq initialization to separate function
  virtio: blk: Add freeze, restore handlers to support S4
  virtio: net: Move out vq initialization into separate function
  virtio: net: Move out vq and vq buf removal into separate function
  virtio: net: Add freeze, restore handlers to support S4
  virtio: balloon: ensure thread exists before stopping it
  virtio: balloon: Move out vq initialization into separate function
  virtio: balloon: Add freeze, restore handlers to support S4

 drivers/block/virtio_blk.c      |   57 +++++++++++++++--
 drivers/char/virtio_console.c   |  126 +++++++++++++++++++++++++++++--------
 drivers/net/virtio_net.c        |  102 ++++++++++++++++++++++--------
 drivers/virtio/virtio_balloon.c |  131 +++++++++++++++++++++++++++++++++------
 drivers/virtio/virtio_pci.c     |  101 +++++++++++++++++++++++++++++-
 include/linux/virtio.h          |    5 ++
 6 files changed, 439 insertions(+), 83 deletions(-)

-- 
1.7.7.3

[PATCH v4 01/12] virtio: pci: switch to new PM API

[Amit Shah]

The older PM API doesn't have a way to get notifications on hibernate
events.  Switch to the newer one that gives us those notifications.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 03d1984..23e1532 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 }
 
 #ifdef CONFIG_PM
-static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
+static int virtio_pci_suspend(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
 	pci_save_state(pci_dev);
 	pci_set_power_state(pci_dev, PCI_D3hot);
 	return 0;
 }
 
-static int virtio_pci_resume(struct pci_dev *pci_dev)
+static int virtio_pci_resume(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
 	pci_restore_state(pci_dev);
 	pci_set_power_state(pci_dev, PCI_D0);
 	return 0;
 }
+
+static const struct dev_pm_ops virtio_pci_pm_ops = {
+	.suspend = virtio_pci_suspend,
+	.resume  = virtio_pci_resume,
+};
 #endif
 
 static struct pci_driver virtio_pci_driver = {
@@ -729,8 +738,7 @@ static struct pci_driver virtio_pci_driver = {
 	.probe		= virtio_pci_probe,
 	.remove		= __devexit_p(virtio_pci_remove),
 #ifdef CONFIG_PM
-	.suspend	= virtio_pci_suspend,
-	.resume		= virtio_pci_resume,
+	.driver.pm	= &virtio_pci_pm_ops,
 #endif
 };
 
-- 
1.7.7.3

[PATCH v4 01/12] virtio: pci: switch to new PM API

[Amit Shah]

The older PM API doesn't have a way to get notifications on hibernate
events.  Switch to the newer one that gives us those notifications.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 03d1984..23e1532 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
 }
 
 #ifdef CONFIG_PM
-static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
+static int virtio_pci_suspend(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
 	pci_save_state(pci_dev);
 	pci_set_power_state(pci_dev, PCI_D3hot);
 	return 0;
 }
 
-static int virtio_pci_resume(struct pci_dev *pci_dev)
+static int virtio_pci_resume(struct device *dev)
 {
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
 	pci_restore_state(pci_dev);
 	pci_set_power_state(pci_dev, PCI_D0);
 	return 0;
 }
+
+static const struct dev_pm_ops virtio_pci_pm_ops = {
+	.suspend = virtio_pci_suspend,
+	.resume  = virtio_pci_resume,
+};
 #endif
 
 static struct pci_driver virtio_pci_driver = {
@@ -729,8 +738,7 @@ static struct pci_driver virtio_pci_driver = {
 	.probe		= virtio_pci_probe,
 	.remove		= __devexit_p(virtio_pci_remove),
 #ifdef CONFIG_PM
-	.suspend	= virtio_pci_suspend,
-	.resume		= virtio_pci_resume,
+	.driver.pm	= &virtio_pci_pm_ops,
 #endif
 };
 
-- 
1.7.7.3

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Rafael J. Wysocki]

Hi,

On Tuesday, December 06, 2011, Amit Shah wrote:
> The older PM API doesn't have a way to get notifications on hibernate
> events.  Switch to the newer one that gives us those notifications.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 03d1984..23e1532 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
>  }
>  
>  #ifdef CONFIG_PM
> -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> +static int virtio_pci_suspend(struct device *dev)
>  {
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +
>  	pci_save_state(pci_dev);
>  	pci_set_power_state(pci_dev, PCI_D3hot);
>  	return 0;
>  }
>  
> -static int virtio_pci_resume(struct pci_dev *pci_dev)
> +static int virtio_pci_resume(struct device *dev)
>  {
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +
>  	pci_restore_state(pci_dev);
>  	pci_set_power_state(pci_dev, PCI_D0);
>  	return 0;
>  }
> +
> +static const struct dev_pm_ops virtio_pci_pm_ops = {
> +	.suspend = virtio_pci_suspend,
> +	.resume  = virtio_pci_resume,
> +};
>  #endif

You seem to have forgotten about hibernation callbacks.  Please use
one the macros defined in include/linux/pm.h if you want to use the same
callback routines for hibernation.

>  static struct pci_driver virtio_pci_driver = {
> @@ -729,8 +738,7 @@ static struct pci_driver virtio_pci_driver = {
>  	.probe		= virtio_pci_probe,
>  	.remove		= __devexit_p(virtio_pci_remove),
>  #ifdef CONFIG_PM
> -	.suspend	= virtio_pci_suspend,
> -	.resume		= virtio_pci_resume,
> +	.driver.pm	= &virtio_pci_pm_ops,
>  #endif
>  };

Thanks,
Rafael

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Rafael J. Wysocki]

Hi,

On Tuesday, December 06, 2011, Amit Shah wrote:
> The older PM API doesn't have a way to get notifications on hibernate
> events.  Switch to the newer one that gives us those notifications.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 03d1984..23e1532 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
>  }
>  
>  #ifdef CONFIG_PM
> -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> +static int virtio_pci_suspend(struct device *dev)
>  {
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +
>  	pci_save_state(pci_dev);
>  	pci_set_power_state(pci_dev, PCI_D3hot);
>  	return 0;
>  }
>  
> -static int virtio_pci_resume(struct pci_dev *pci_dev)
> +static int virtio_pci_resume(struct device *dev)
>  {
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +
>  	pci_restore_state(pci_dev);
>  	pci_set_power_state(pci_dev, PCI_D0);
>  	return 0;
>  }
> +
> +static const struct dev_pm_ops virtio_pci_pm_ops = {
> +	.suspend = virtio_pci_suspend,
> +	.resume  = virtio_pci_resume,
> +};
>  #endif

You seem to have forgotten about hibernation callbacks.  Please use
one the macros defined in include/linux/pm.h if you want to use the same
callback routines for hibernation.

>  static struct pci_driver virtio_pci_driver = {
> @@ -729,8 +738,7 @@ static struct pci_driver virtio_pci_driver = {
>  	.probe		= virtio_pci_probe,
>  	.remove		= __devexit_p(virtio_pci_remove),
>  #ifdef CONFIG_PM
> -	.suspend	= virtio_pci_suspend,
> -	.resume		= virtio_pci_resume,
> +	.driver.pm	= &virtio_pci_pm_ops,
>  #endif
>  };

Thanks,
Rafael

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Amit Shah]

Hi Rafael,

On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> Hi,
> 
> On Tuesday, December 06, 2011, Amit Shah wrote:
> > The older PM API doesn't have a way to get notifications on hibernate
> > events.  Switch to the newer one that gives us those notifications.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> >  1 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index 03d1984..23e1532 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> >  }
> >  
> >  #ifdef CONFIG_PM
> > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > +static int virtio_pci_suspend(struct device *dev)
> >  {
> > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > +
> >  	pci_save_state(pci_dev);
> >  	pci_set_power_state(pci_dev, PCI_D3hot);
> >  	return 0;
> >  }
> >  
> > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > +static int virtio_pci_resume(struct device *dev)
> >  {
> > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > +
> >  	pci_restore_state(pci_dev);
> >  	pci_set_power_state(pci_dev, PCI_D0);
> >  	return 0;
> >  }
> > +
> > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > +	.suspend = virtio_pci_suspend,
> > +	.resume  = virtio_pci_resume,
> > +};
> >  #endif
> 
> You seem to have forgotten about hibernation callbacks.

This patch just moves to the new API keeping everything else the same.
The hibernation callbacks come in patch 2.

>  Please use
> one the macros defined in include/linux/pm.h if you want to use the same
> callback routines for hibernation.

No, they're different functions, so I don't use the maros.

Thanks,

		Amit

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Amit Shah]

Hi Rafael,

On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> Hi,
> 
> On Tuesday, December 06, 2011, Amit Shah wrote:
> > The older PM API doesn't have a way to get notifications on hibernate
> > events.  Switch to the newer one that gives us those notifications.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> >  1 files changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > index 03d1984..23e1532 100644
> > --- a/drivers/virtio/virtio_pci.c
> > +++ b/drivers/virtio/virtio_pci.c
> > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> >  }
> >  
> >  #ifdef CONFIG_PM
> > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > +static int virtio_pci_suspend(struct device *dev)
> >  {
> > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > +
> >  	pci_save_state(pci_dev);
> >  	pci_set_power_state(pci_dev, PCI_D3hot);
> >  	return 0;
> >  }
> >  
> > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > +static int virtio_pci_resume(struct device *dev)
> >  {
> > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > +
> >  	pci_restore_state(pci_dev);
> >  	pci_set_power_state(pci_dev, PCI_D0);
> >  	return 0;
> >  }
> > +
> > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > +	.suspend = virtio_pci_suspend,
> > +	.resume  = virtio_pci_resume,
> > +};
> >  #endif
> 
> You seem to have forgotten about hibernation callbacks.

This patch just moves to the new API keeping everything else the same.
The hibernation callbacks come in patch 2.

>  Please use
> one the macros defined in include/linux/pm.h if you want to use the same
> callback routines for hibernation.

No, they're different functions, so I don't use the maros.

Thanks,

		Amit

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Rafael J. Wysocki]

On Wednesday, December 07, 2011, Amit Shah wrote:
> Hi Rafael,
> 
> On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Tuesday, December 06, 2011, Amit Shah wrote:
> > > The older PM API doesn't have a way to get notifications on hibernate
> > > events.  Switch to the newer one that gives us those notifications.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > index 03d1984..23e1532 100644
> > > --- a/drivers/virtio/virtio_pci.c
> > > +++ b/drivers/virtio/virtio_pci.c
> > > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> > >  }
> > >  
> > >  #ifdef CONFIG_PM
> > > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > > +static int virtio_pci_suspend(struct device *dev)
> > >  {
> > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > +
> > >  	pci_save_state(pci_dev);
> > >  	pci_set_power_state(pci_dev, PCI_D3hot);
> > >  	return 0;
> > >  }
> > >  
> > > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > > +static int virtio_pci_resume(struct device *dev)
> > >  {
> > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > +
> > >  	pci_restore_state(pci_dev);
> > >  	pci_set_power_state(pci_dev, PCI_D0);
> > >  	return 0;
> > >  }
> > > +
> > > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > +	.suspend = virtio_pci_suspend,
> > > +	.resume  = virtio_pci_resume,
> > > +};
> > >  #endif
> > 
> > You seem to have forgotten about hibernation callbacks.
> 
> This patch just moves to the new API keeping everything else the same.
> The hibernation callbacks come in patch 2.

OK, but then hibernation will be broken between the two patches, so perhaps
it's better to merge them into one?

> >  Please use
> > one the macros defined in include/linux/pm.h if you want to use the same
> > callback routines for hibernation.
> 
> No, they're different functions, so I don't use the maros.

OK

Thanks,
Rafael

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Rafael J. Wysocki]

On Wednesday, December 07, 2011, Amit Shah wrote:
> Hi Rafael,
> 
> On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Tuesday, December 06, 2011, Amit Shah wrote:
> > > The older PM API doesn't have a way to get notifications on hibernate
> > > events.  Switch to the newer one that gives us those notifications.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > index 03d1984..23e1532 100644
> > > --- a/drivers/virtio/virtio_pci.c
> > > +++ b/drivers/virtio/virtio_pci.c
> > > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> > >  }
> > >  
> > >  #ifdef CONFIG_PM
> > > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > > +static int virtio_pci_suspend(struct device *dev)
> > >  {
> > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > +
> > >  	pci_save_state(pci_dev);
> > >  	pci_set_power_state(pci_dev, PCI_D3hot);
> > >  	return 0;
> > >  }
> > >  
> > > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > > +static int virtio_pci_resume(struct device *dev)
> > >  {
> > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > +
> > >  	pci_restore_state(pci_dev);
> > >  	pci_set_power_state(pci_dev, PCI_D0);
> > >  	return 0;
> > >  }
> > > +
> > > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > +	.suspend = virtio_pci_suspend,
> > > +	.resume  = virtio_pci_resume,
> > > +};
> > >  #endif
> > 
> > You seem to have forgotten about hibernation callbacks.
> 
> This patch just moves to the new API keeping everything else the same.
> The hibernation callbacks come in patch 2.

OK, but then hibernation will be broken between the two patches, so perhaps
it's better to merge them into one?

> >  Please use
> > one the macros defined in include/linux/pm.h if you want to use the same
> > callback routines for hibernation.
> 
> No, they're different functions, so I don't use the maros.

OK

Thanks,
Rafael

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Amit Shah]

On (Wed) 07 Dec 2011 [10:48:24], Rafael J. Wysocki wrote:
> On Wednesday, December 07, 2011, Amit Shah wrote:
> > Hi Rafael,
> > 
> > On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> > > Hi,
> > > 
> > > On Tuesday, December 06, 2011, Amit Shah wrote:
> > > > The older PM API doesn't have a way to get notifications on hibernate
> > > > events.  Switch to the newer one that gives us those notifications.
> > > > 
> > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> > > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > > index 03d1984..23e1532 100644
> > > > --- a/drivers/virtio/virtio_pci.c
> > > > +++ b/drivers/virtio/virtio_pci.c
> > > > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_PM
> > > > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > > > +static int virtio_pci_suspend(struct device *dev)
> > > >  {
> > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > +
> > > >  	pci_save_state(pci_dev);
> > > >  	pci_set_power_state(pci_dev, PCI_D3hot);
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > > > +static int virtio_pci_resume(struct device *dev)
> > > >  {
> > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > +
> > > >  	pci_restore_state(pci_dev);
> > > >  	pci_set_power_state(pci_dev, PCI_D0);
> > > >  	return 0;
> > > >  }
> > > > +
> > > > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > > +	.suspend = virtio_pci_suspend,
> > > > +	.resume  = virtio_pci_resume,
> > > > +};
> > > >  #endif
> > > 
> > > You seem to have forgotten about hibernation callbacks.
> > 
> > This patch just moves to the new API keeping everything else the same.
> > The hibernation callbacks come in patch 2.
> 
> OK, but then hibernation will be broken between the two patches, so perhaps
> it's better to merge them into one?

The hibernation support doesn't exist now.  It's added in the later
patches of the series (3 onwards).

		Amit

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Amit Shah]

On (Wed) 07 Dec 2011 [10:48:24], Rafael J. Wysocki wrote:
> On Wednesday, December 07, 2011, Amit Shah wrote:
> > Hi Rafael,
> > 
> > On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> > > Hi,
> > > 
> > > On Tuesday, December 06, 2011, Amit Shah wrote:
> > > > The older PM API doesn't have a way to get notifications on hibernate
> > > > events.  Switch to the newer one that gives us those notifications.
> > > > 
> > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> > > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > > index 03d1984..23e1532 100644
> > > > --- a/drivers/virtio/virtio_pci.c
> > > > +++ b/drivers/virtio/virtio_pci.c
> > > > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_PM
> > > > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > > > +static int virtio_pci_suspend(struct device *dev)
> > > >  {
> > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > +
> > > >  	pci_save_state(pci_dev);
> > > >  	pci_set_power_state(pci_dev, PCI_D3hot);
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > > > +static int virtio_pci_resume(struct device *dev)
> > > >  {
> > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > +
> > > >  	pci_restore_state(pci_dev);
> > > >  	pci_set_power_state(pci_dev, PCI_D0);
> > > >  	return 0;
> > > >  }
> > > > +
> > > > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > > +	.suspend = virtio_pci_suspend,
> > > > +	.resume  = virtio_pci_resume,
> > > > +};
> > > >  #endif
> > > 
> > > You seem to have forgotten about hibernation callbacks.
> > 
> > This patch just moves to the new API keeping everything else the same.
> > The hibernation callbacks come in patch 2.
> 
> OK, but then hibernation will be broken between the two patches, so perhaps
> it's better to merge them into one?

The hibernation support doesn't exist now.  It's added in the later
patches of the series (3 onwards).

		Amit

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Rafael J. Wysocki]

On Wednesday, December 07, 2011, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [10:48:24], Rafael J. Wysocki wrote:
> > On Wednesday, December 07, 2011, Amit Shah wrote:
> > > Hi Rafael,
> > > 
> > > On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> > > > Hi,
> > > > 
> > > > On Tuesday, December 06, 2011, Amit Shah wrote:
> > > > > The older PM API doesn't have a way to get notifications on hibernate
> > > > > events.  Switch to the newer one that gives us those notifications.
> > > > > 
> > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> > > > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > > > index 03d1984..23e1532 100644
> > > > > --- a/drivers/virtio/virtio_pci.c
> > > > > +++ b/drivers/virtio/virtio_pci.c
> > > > > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> > > > >  }
> > > > >  
> > > > >  #ifdef CONFIG_PM
> > > > > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > > > > +static int virtio_pci_suspend(struct device *dev)
> > > > >  {
> > > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > > +
> > > > >  	pci_save_state(pci_dev);
> > > > >  	pci_set_power_state(pci_dev, PCI_D3hot);
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > > > > +static int virtio_pci_resume(struct device *dev)
> > > > >  {
> > > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > > +
> > > > >  	pci_restore_state(pci_dev);
> > > > >  	pci_set_power_state(pci_dev, PCI_D0);
> > > > >  	return 0;
> > > > >  }
> > > > > +
> > > > > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > > > +	.suspend = virtio_pci_suspend,
> > > > > +	.resume  = virtio_pci_resume,
> > > > > +};
> > > > >  #endif
> > > > 
> > > > You seem to have forgotten about hibernation callbacks.
> > > 
> > > This patch just moves to the new API keeping everything else the same.
> > > The hibernation callbacks come in patch 2.
> > 
> > OK, but then hibernation will be broken between the two patches, so perhaps
> > it's better to merge them into one?
> 
> The hibernation support doesn't exist now.  It's added in the later
> patches of the series (3 onwards).

OK then, sorry for the noise.

Rafael

Re: [PATCH v4 01/12] virtio: pci: switch to new PM API

[Rafael J. Wysocki]

On Wednesday, December 07, 2011, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [10:48:24], Rafael J. Wysocki wrote:
> > On Wednesday, December 07, 2011, Amit Shah wrote:
> > > Hi Rafael,
> > > 
> > > On (Tue) 06 Dec 2011 [23:12:36], Rafael J. Wysocki wrote:
> > > > Hi,
> > > > 
> > > > On Tuesday, December 06, 2011, Amit Shah wrote:
> > > > > The older PM API doesn't have a way to get notifications on hibernate
> > > > > events.  Switch to the newer one that gives us those notifications.
> > > > > 
> > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_pci.c |   16 ++++++++++++----
> > > > >  1 files changed, 12 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> > > > > index 03d1984..23e1532 100644
> > > > > --- a/drivers/virtio/virtio_pci.c
> > > > > +++ b/drivers/virtio/virtio_pci.c
> > > > > @@ -708,19 +708,28 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
> > > > >  }
> > > > >  
> > > > >  #ifdef CONFIG_PM
> > > > > -static int virtio_pci_suspend(struct pci_dev *pci_dev, pm_message_t state)
> > > > > +static int virtio_pci_suspend(struct device *dev)
> > > > >  {
> > > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > > +
> > > > >  	pci_save_state(pci_dev);
> > > > >  	pci_set_power_state(pci_dev, PCI_D3hot);
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > -static int virtio_pci_resume(struct pci_dev *pci_dev)
> > > > > +static int virtio_pci_resume(struct device *dev)
> > > > >  {
> > > > > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > > > > +
> > > > >  	pci_restore_state(pci_dev);
> > > > >  	pci_set_power_state(pci_dev, PCI_D0);
> > > > >  	return 0;
> > > > >  }
> > > > > +
> > > > > +static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > > > +	.suspend = virtio_pci_suspend,
> > > > > +	.resume  = virtio_pci_resume,
> > > > > +};
> > > > >  #endif
> > > > 
> > > > You seem to have forgotten about hibernation callbacks.
> > > 
> > > This patch just moves to the new API keeping everything else the same.
> > > The hibernation callbacks come in patch 2.
> > 
> > OK, but then hibernation will be broken between the two patches, so perhaps
> > it's better to merge them into one?
> 
> The hibernation support doesn't exist now.  It's added in the later
> patches of the series (3 onwards).

OK then, sorry for the noise.

Rafael

[PATCH v4 02/12] virtio: pci: add PM notification handlers for restore, freeze, thaw, poweroff

[Amit Shah]

Handle thaw, restore and freeze notifications from the PM core.  Expose
these to individual virtio drivers that can quiesce and resume vq
operations.  For drivers not implementing the thaw() method, use the
restore method instead.

These functions also save device-specific data so that the device can be
put in pre-suspend state after resume, and disable and enable the PCI
device in the freeze and resume functions, respectively.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   85 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h      |    5 +++
 2 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 23e1532..bd33603 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -55,6 +55,10 @@ struct virtio_pci_device
 	unsigned msix_vectors;
 	/* Vectors allocated, excluding per-vq vectors if any */
 	unsigned msix_used_vectors;
+
+	/* Status saved during hibernate/restore */
+	u8 saved_status;
+
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
 };
@@ -726,9 +730,90 @@ static int virtio_pci_resume(struct device *dev)
 	return 0;
 }
 
+static int virtio_pci_freeze(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	struct virtio_driver *drv;
+	int ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	ret = 0;
+	vp_dev->saved_status = vp_get_status(&vp_dev->vdev);
+	if (drv && drv->freeze)
+		ret = drv->freeze(&vp_dev->vdev);
+
+	if (!ret)
+		pci_disable_device(pci_dev);
+	return ret;
+}
+
+static int restore_common(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	int ret;
+
+	ret = pci_enable_device(pci_dev);
+	if (ret)
+		return ret;
+	pci_set_master(pci_dev);
+	vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
+	vp_finalize_features(&vp_dev->vdev);
+
+	return ret;
+}
+
+static int virtio_pci_restore(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	struct virtio_driver *drv;
+	int ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	ret = restore_common(dev);
+	if (!ret && drv && drv->restore)
+		ret = drv->restore(&vp_dev->vdev);
+
+	return ret;
+}
+
+static int virtio_pci_thaw(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	struct virtio_driver *drv;
+	int ret;
+
+	ret = restore_common(dev);
+	if (ret)
+		return ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+	if (!drv)
+		return ret;
+
+	if (drv->thaw)
+		ret = drv->thaw(&vp_dev->vdev);
+	else if (drv->restore)
+		ret = drv->restore(&vp_dev->vdev);
+
+	return ret;
+}
+
 static const struct dev_pm_ops virtio_pci_pm_ops = {
 	.suspend = virtio_pci_suspend,
 	.resume  = virtio_pci_resume,
+	.freeze  = virtio_pci_freeze,
+	.thaw    = virtio_pci_thaw,
+	.restore = virtio_pci_restore,
+	.poweroff = virtio_pci_suspend,
 };
 #endif
 
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 4c069d8..92902ab 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -146,6 +146,11 @@ struct virtio_driver {
 	int (*probe)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
 	void (*config_changed)(struct virtio_device *dev);
+#ifdef CONFIG_PM
+	int (*freeze)(struct virtio_device *dev);
+	int (*thaw)(struct virtio_device *dev);
+	int (*restore)(struct virtio_device *dev);
+#endif
 };
 
 int register_virtio_driver(struct virtio_driver *drv);
-- 
1.7.7.3

[PATCH v4 02/12] virtio: pci: add PM notification handlers for restore, freeze, thaw, poweroff

[Amit Shah]

Handle thaw, restore and freeze notifications from the PM core.  Expose
these to individual virtio drivers that can quiesce and resume vq
operations.  For drivers not implementing the thaw() method, use the
restore method instead.

These functions also save device-specific data so that the device can be
put in pre-suspend state after resume, and disable and enable the PCI
device in the freeze and resume functions, respectively.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_pci.c |   85 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/virtio.h      |    5 +++
 2 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 23e1532..bd33603 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -55,6 +55,10 @@ struct virtio_pci_device
 	unsigned msix_vectors;
 	/* Vectors allocated, excluding per-vq vectors if any */
 	unsigned msix_used_vectors;
+
+	/* Status saved during hibernate/restore */
+	u8 saved_status;
+
 	/* Whether we have vector per vq */
 	bool per_vq_vectors;
 };
@@ -726,9 +730,90 @@ static int virtio_pci_resume(struct device *dev)
 	return 0;
 }
 
+static int virtio_pci_freeze(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	struct virtio_driver *drv;
+	int ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	ret = 0;
+	vp_dev->saved_status = vp_get_status(&vp_dev->vdev);
+	if (drv && drv->freeze)
+		ret = drv->freeze(&vp_dev->vdev);
+
+	if (!ret)
+		pci_disable_device(pci_dev);
+	return ret;
+}
+
+static int restore_common(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	int ret;
+
+	ret = pci_enable_device(pci_dev);
+	if (ret)
+		return ret;
+	pci_set_master(pci_dev);
+	vp_set_status(&vp_dev->vdev, vp_dev->saved_status);
+	vp_finalize_features(&vp_dev->vdev);
+
+	return ret;
+}
+
+static int virtio_pci_restore(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	struct virtio_driver *drv;
+	int ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+
+	ret = restore_common(dev);
+	if (!ret && drv && drv->restore)
+		ret = drv->restore(&vp_dev->vdev);
+
+	return ret;
+}
+
+static int virtio_pci_thaw(struct device *dev)
+{
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+	struct virtio_driver *drv;
+	int ret;
+
+	ret = restore_common(dev);
+	if (ret)
+		return ret;
+
+	drv = container_of(vp_dev->vdev.dev.driver,
+			   struct virtio_driver, driver);
+	if (!drv)
+		return ret;
+
+	if (drv->thaw)
+		ret = drv->thaw(&vp_dev->vdev);
+	else if (drv->restore)
+		ret = drv->restore(&vp_dev->vdev);
+
+	return ret;
+}
+
 static const struct dev_pm_ops virtio_pci_pm_ops = {
 	.suspend = virtio_pci_suspend,
 	.resume  = virtio_pci_resume,
+	.freeze  = virtio_pci_freeze,
+	.thaw    = virtio_pci_thaw,
+	.restore = virtio_pci_restore,
+	.poweroff = virtio_pci_suspend,
 };
 #endif
 
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 4c069d8..92902ab 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -146,6 +146,11 @@ struct virtio_driver {
 	int (*probe)(struct virtio_device *dev);
 	void (*remove)(struct virtio_device *dev);
 	void (*config_changed)(struct virtio_device *dev);
+#ifdef CONFIG_PM
+	int (*freeze)(struct virtio_device *dev);
+	int (*thaw)(struct virtio_device *dev);
+	int (*restore)(struct virtio_device *dev);
+#endif
 };
 
 int register_virtio_driver(struct virtio_driver *drv);
-- 
1.7.7.3

[PATCH v4 03/12] virtio: console: Move out vq and vq buf removal into separate functions

[Amit Shah]

This common code will be shared with the PM freeze function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   68 ++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8e3c46d..e14f5aa 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1271,6 +1271,20 @@ static void remove_port(struct kref *kref)
 	kfree(port);
 }
 
+static void remove_port_data(struct port *port)
+{
+	struct port_buffer *buf;
+
+	/* Remove unused data this port might have received. */
+	discard_port_data(port);
+
+	reclaim_consumed_buffers(port);
+
+	/* Remove buffers we queued up for the Host to send us data in. */
+	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
+		free_buf(buf);
+}
+
 /*
  * Port got unplugged.  Remove port from portdev's list and drop the
  * kref reference.  If no userspace has this port opened, it will
@@ -1278,8 +1292,6 @@ static void remove_port(struct kref *kref)
  */
 static void unplug_port(struct port *port)
 {
-	struct port_buffer *buf;
-
 	spin_lock_irq(&port->portdev->ports_lock);
 	list_del(&port->list);
 	spin_unlock_irq(&port->portdev->ports_lock);
@@ -1300,14 +1312,7 @@ static void unplug_port(struct port *port)
 		hvc_remove(port->cons.hvc);
 	}
 
-	/* Remove unused data this port might have received. */
-	discard_port_data(port);
-
-	reclaim_consumed_buffers(port);
-
-	/* Remove buffers we queued up for the Host to send us data in. */
-	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+	remove_port_data(port);
 
 	/*
 	 * We should just assume the device itself has gone off --
@@ -1659,6 +1664,28 @@ static const struct file_operations portdev_fops = {
 	.owner = THIS_MODULE,
 };
 
+static void remove_vqs(struct ports_device *portdev)
+{
+	portdev->vdev->config->del_vqs(portdev->vdev);
+	kfree(portdev->in_vqs);
+	kfree(portdev->out_vqs);
+}
+
+static void remove_controlq_data(struct ports_device *portdev)
+{
+	struct port_buffer *buf;
+	unsigned int len;
+
+	if (!use_multiport(portdev))
+		return;
+
+	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
+		free_buf(buf);
+
+	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
+		free_buf(buf);
+}
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
@@ -1764,9 +1791,7 @@ free_vqs:
 	/* The host might want to notify mgmt sw about device add failure */
 	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
 			   VIRTIO_CONSOLE_DEVICE_READY, 0);
-	vdev->config->del_vqs(vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
+	remove_vqs(portdev);
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:
@@ -1804,21 +1829,8 @@ static void virtcons_remove(struct virtio_device *vdev)
 	 * have to just stop using the port, as the vqs are going
 	 * away.
 	 */
-	if (use_multiport(portdev)) {
-		struct port_buffer *buf;
-		unsigned int len;
-
-		while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-			free_buf(buf);
-
-		while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-			free_buf(buf);
-	}
-
-	vdev->config->del_vqs(vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
-
+	remove_controlq_data(portdev);
+	remove_vqs(portdev);
 	kfree(portdev);
 }
 
-- 
1.7.7.3

[PATCH v4 03/12] virtio: console: Move out vq and vq buf removal into separate functions

[Amit Shah]

This common code will be shared with the PM freeze function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   68 ++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8e3c46d..e14f5aa 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1271,6 +1271,20 @@ static void remove_port(struct kref *kref)
 	kfree(port);
 }
 
+static void remove_port_data(struct port *port)
+{
+	struct port_buffer *buf;
+
+	/* Remove unused data this port might have received. */
+	discard_port_data(port);
+
+	reclaim_consumed_buffers(port);
+
+	/* Remove buffers we queued up for the Host to send us data in. */
+	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
+		free_buf(buf);
+}
+
 /*
  * Port got unplugged.  Remove port from portdev's list and drop the
  * kref reference.  If no userspace has this port opened, it will
@@ -1278,8 +1292,6 @@ static void remove_port(struct kref *kref)
  */
 static void unplug_port(struct port *port)
 {
-	struct port_buffer *buf;
-
 	spin_lock_irq(&port->portdev->ports_lock);
 	list_del(&port->list);
 	spin_unlock_irq(&port->portdev->ports_lock);
@@ -1300,14 +1312,7 @@ static void unplug_port(struct port *port)
 		hvc_remove(port->cons.hvc);
 	}
 
-	/* Remove unused data this port might have received. */
-	discard_port_data(port);
-
-	reclaim_consumed_buffers(port);
-
-	/* Remove buffers we queued up for the Host to send us data in. */
-	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+	remove_port_data(port);
 
 	/*
 	 * We should just assume the device itself has gone off --
@@ -1659,6 +1664,28 @@ static const struct file_operations portdev_fops = {
 	.owner = THIS_MODULE,
 };
 
+static void remove_vqs(struct ports_device *portdev)
+{
+	portdev->vdev->config->del_vqs(portdev->vdev);
+	kfree(portdev->in_vqs);
+	kfree(portdev->out_vqs);
+}
+
+static void remove_controlq_data(struct ports_device *portdev)
+{
+	struct port_buffer *buf;
+	unsigned int len;
+
+	if (!use_multiport(portdev))
+		return;
+
+	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
+		free_buf(buf);
+
+	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
+		free_buf(buf);
+}
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
@@ -1764,9 +1791,7 @@ free_vqs:
 	/* The host might want to notify mgmt sw about device add failure */
 	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
 			   VIRTIO_CONSOLE_DEVICE_READY, 0);
-	vdev->config->del_vqs(vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
+	remove_vqs(portdev);
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:
@@ -1804,21 +1829,8 @@ static void virtcons_remove(struct virtio_device *vdev)
 	 * have to just stop using the port, as the vqs are going
 	 * away.
 	 */
-	if (use_multiport(portdev)) {
-		struct port_buffer *buf;
-		unsigned int len;
-
-		while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-			free_buf(buf);
-
-		while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-			free_buf(buf);
-	}
-
-	vdev->config->del_vqs(vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
-
+	remove_controlq_data(portdev);
+	remove_vqs(portdev);
 	kfree(portdev);
 }
 
-- 
1.7.7.3

[PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Amit Shah]

Remove all vqs and associated buffers in the freeze callback which
prepares us to go into hibernation state.  On restore, re-create all the
vqs and populate the input vqs with buffers to get to the pre-hibernate
state.

Note: Any outstanding unconsumed buffers are discarded; which means
there's a possibility of data loss in case the host or the guest didn't
consume any data already present in the vqs.  This can be addressed in a
later patch series, perhaps in virtio common code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   58 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e14f5aa..fd2fd6f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1844,6 +1844,60 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
+#ifdef CONFIG_PM
+static int virtcons_freeze(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+
+	portdev = vdev->priv;
+
+	vdev->config->reset(vdev);
+
+	cancel_work_sync(&portdev->control_work);
+	remove_controlq_data(portdev);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		/*
+		 * We'll ask the host later if the new invocation has
+		 * the port opened or closed.
+		 */
+		port->host_connected = false;
+		remove_port_data(port);
+	}
+	remove_vqs(portdev);
+
+	return 0;
+}
+
+static int virtcons_restore(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+	int ret;
+
+	portdev = vdev->priv;
+
+	ret = init_vqs(portdev);
+	if (ret)
+		return ret;
+
+	if (use_multiport(portdev))
+		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		port->in_vq = portdev->in_vqs[port->id];
+		port->out_vq = portdev->out_vqs[port->id];
+
+		fill_queue(port->in_vq, &port->inbuf_lock);
+
+		/* Get port open/close status on the host */
+		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+	}
+	return 0;
+}
+#endif
+
 static struct virtio_driver virtio_console = {
 	.feature_table = features,
 	.feature_table_size = ARRAY_SIZE(features),
@@ -1853,6 +1907,10 @@ static struct virtio_driver virtio_console = {
 	.probe =	virtcons_probe,
 	.remove =	virtcons_remove,
 	.config_changed = config_intr,
+#ifdef CONFIG_PM
+	.freeze =	virtcons_freeze,
+	.restore =	virtcons_restore,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

[PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Amit Shah]

Remove all vqs and associated buffers in the freeze callback which
prepares us to go into hibernation state.  On restore, re-create all the
vqs and populate the input vqs with buffers to get to the pre-hibernate
state.

Note: Any outstanding unconsumed buffers are discarded; which means
there's a possibility of data loss in case the host or the guest didn't
consume any data already present in the vqs.  This can be addressed in a
later patch series, perhaps in virtio common code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/char/virtio_console.c |   58 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e14f5aa..fd2fd6f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1844,6 +1844,60 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
+#ifdef CONFIG_PM
+static int virtcons_freeze(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+
+	portdev = vdev->priv;
+
+	vdev->config->reset(vdev);
+
+	cancel_work_sync(&portdev->control_work);
+	remove_controlq_data(portdev);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		/*
+		 * We'll ask the host later if the new invocation has
+		 * the port opened or closed.
+		 */
+		port->host_connected = false;
+		remove_port_data(port);
+	}
+	remove_vqs(portdev);
+
+	return 0;
+}
+
+static int virtcons_restore(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port;
+	int ret;
+
+	portdev = vdev->priv;
+
+	ret = init_vqs(portdev);
+	if (ret)
+		return ret;
+
+	if (use_multiport(portdev))
+		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
+
+	list_for_each_entry(port, &portdev->ports, list) {
+		port->in_vq = portdev->in_vqs[port->id];
+		port->out_vq = portdev->out_vqs[port->id];
+
+		fill_queue(port->in_vq, &port->inbuf_lock);
+
+		/* Get port open/close status on the host */
+		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
+	}
+	return 0;
+}
+#endif
+
 static struct virtio_driver virtio_console = {
 	.feature_table = features,
 	.feature_table_size = ARRAY_SIZE(features),
@@ -1853,6 +1907,10 @@ static struct virtio_driver virtio_console = {
 	.probe =	virtcons_probe,
 	.remove =	virtcons_remove,
 	.config_changed = config_intr,
+#ifdef CONFIG_PM
+	.freeze =	virtcons_freeze,
+	.restore =	virtcons_restore,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

Re: [PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:42AM +0530, Amit Shah wrote:
> Remove all vqs and associated buffers in the freeze callback which
> prepares us to go into hibernation state.  On restore, re-create all the
> vqs and populate the input vqs with buffers to get to the pre-hibernate
> state.
> 
> Note: Any outstanding unconsumed buffers are discarded; which means
> there's a possibility of data loss in case the host or the guest didn't
> consume any data already present in the vqs.  This can be addressed in a
> later patch series, perhaps in virtio common code.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c |   58 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e14f5aa..fd2fd6f 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1844,6 +1844,60 @@ static unsigned int features[] = {
>  	VIRTIO_CONSOLE_F_MULTIPORT,
>  };
>  
> +#ifdef CONFIG_PM
> +static int virtcons_freeze(struct virtio_device *vdev)
> +{
> +	struct ports_device *portdev;
> +	struct port *port;
> +
> +	portdev = vdev->priv;
> +
> +	vdev->config->reset(vdev);


So here, cancel_work_sync might still be running.
If it does run, might it try to access the device
config?  Could not determine this quickly, if yes it's a problem.

> +
> +	cancel_work_sync(&portdev->control_work);
> +	remove_controlq_data(portdev);
> +
> +	list_for_each_entry(port, &portdev->ports, list) {
> +		/*
> +		 * We'll ask the host later if the new invocation has
> +		 * the port opened or closed.
> +		 */
> +		port->host_connected = false;
> +		remove_port_data(port);
> +	}
> +	remove_vqs(portdev);
> +
> +	return 0;
> +}
> +
> +static int virtcons_restore(struct virtio_device *vdev)
> +{
> +	struct ports_device *portdev;
> +	struct port *port;
> +	int ret;
> +
> +	portdev = vdev->priv;
> +
> +	ret = init_vqs(portdev);
> +	if (ret)
> +		return ret;
> +
> +	if (use_multiport(portdev))
> +		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
> +
> +	list_for_each_entry(port, &portdev->ports, list) {
> +		port->in_vq = portdev->in_vqs[port->id];
> +		port->out_vq = portdev->out_vqs[port->id];
> +
> +		fill_queue(port->in_vq, &port->inbuf_lock);
> +
> +		/* Get port open/close status on the host */
> +		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
> +	}
> +	return 0;
> +}
> +#endif
> +
>  static struct virtio_driver virtio_console = {
>  	.feature_table = features,
>  	.feature_table_size = ARRAY_SIZE(features),
> @@ -1853,6 +1907,10 @@ static struct virtio_driver virtio_console = {
>  	.probe =	virtcons_probe,
>  	.remove =	virtcons_remove,
>  	.config_changed = config_intr,
> +#ifdef CONFIG_PM
> +	.freeze =	virtcons_freeze,
> +	.restore =	virtcons_restore,
> +#endif
>  };
>  
>  static int __init init(void)
> -- 
> 1.7.7.3

Re: [PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:42AM +0530, Amit Shah wrote:
> Remove all vqs and associated buffers in the freeze callback which
> prepares us to go into hibernation state.  On restore, re-create all the
> vqs and populate the input vqs with buffers to get to the pre-hibernate
> state.
> 
> Note: Any outstanding unconsumed buffers are discarded; which means
> there's a possibility of data loss in case the host or the guest didn't
> consume any data already present in the vqs.  This can be addressed in a
> later patch series, perhaps in virtio common code.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/char/virtio_console.c |   58 +++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index e14f5aa..fd2fd6f 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1844,6 +1844,60 @@ static unsigned int features[] = {
>  	VIRTIO_CONSOLE_F_MULTIPORT,
>  };
>  
> +#ifdef CONFIG_PM
> +static int virtcons_freeze(struct virtio_device *vdev)
> +{
> +	struct ports_device *portdev;
> +	struct port *port;
> +
> +	portdev = vdev->priv;
> +
> +	vdev->config->reset(vdev);


So here, cancel_work_sync might still be running.
If it does run, might it try to access the device
config?  Could not determine this quickly, if yes it's a problem.

> +
> +	cancel_work_sync(&portdev->control_work);
> +	remove_controlq_data(portdev);
> +
> +	list_for_each_entry(port, &portdev->ports, list) {
> +		/*
> +		 * We'll ask the host later if the new invocation has
> +		 * the port opened or closed.
> +		 */
> +		port->host_connected = false;
> +		remove_port_data(port);
> +	}
> +	remove_vqs(portdev);
> +
> +	return 0;
> +}
> +
> +static int virtcons_restore(struct virtio_device *vdev)
> +{
> +	struct ports_device *portdev;
> +	struct port *port;
> +	int ret;
> +
> +	portdev = vdev->priv;
> +
> +	ret = init_vqs(portdev);
> +	if (ret)
> +		return ret;
> +
> +	if (use_multiport(portdev))
> +		fill_queue(portdev->c_ivq, &portdev->cvq_lock);
> +
> +	list_for_each_entry(port, &portdev->ports, list) {
> +		port->in_vq = portdev->in_vqs[port->id];
> +		port->out_vq = portdev->out_vqs[port->id];
> +
> +		fill_queue(port->in_vq, &port->inbuf_lock);
> +
> +		/* Get port open/close status on the host */
> +		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
> +	}
> +	return 0;
> +}
> +#endif
> +
>  static struct virtio_driver virtio_console = {
>  	.feature_table = features,
>  	.feature_table_size = ARRAY_SIZE(features),
> @@ -1853,6 +1907,10 @@ static struct virtio_driver virtio_console = {
>  	.probe =	virtcons_probe,
>  	.remove =	virtcons_remove,
>  	.config_changed = config_intr,
> +#ifdef CONFIG_PM
> +	.freeze =	virtcons_freeze,
> +	.restore =	virtcons_restore,
> +#endif
>  };
>  
>  static int __init init(void)
> -- 
> 1.7.7.3

Re: [PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:43:24], Michael S. Tsirkin wrote:

> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index e14f5aa..fd2fd6f 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1844,6 +1844,60 @@ static unsigned int features[] = {
> >  	VIRTIO_CONSOLE_F_MULTIPORT,
> >  };
> >  
> > +#ifdef CONFIG_PM
> > +static int virtcons_freeze(struct virtio_device *vdev)
> > +{
> > +	struct ports_device *portdev;
> > +	struct port *port;
> > +
> > +	portdev = vdev->priv;
> > +
> > +	vdev->config->reset(vdev);
> 
> 
> So here, cancel_work_sync might still be running.
> If it does run, might it try to access the device
> config?  Could not determine this quickly, if yes it's a problem.

Similar to the other comment: I don't see why just resetting device
can cause config queue access to go bad.

		Amit

Re: [PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:43:24], Michael S. Tsirkin wrote:

> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index e14f5aa..fd2fd6f 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1844,6 +1844,60 @@ static unsigned int features[] = {
> >  	VIRTIO_CONSOLE_F_MULTIPORT,
> >  };
> >  
> > +#ifdef CONFIG_PM
> > +static int virtcons_freeze(struct virtio_device *vdev)
> > +{
> > +	struct ports_device *portdev;
> > +	struct port *port;
> > +
> > +	portdev = vdev->priv;
> > +
> > +	vdev->config->reset(vdev);
> 
> 
> So here, cancel_work_sync might still be running.
> If it does run, might it try to access the device
> config?  Could not determine this quickly, if yes it's a problem.

Similar to the other comment: I don't see why just resetting device
can cause config queue access to go bad.

		Amit

Re: [PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 04:29:55PM +0530, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [12:43:24], Michael S. Tsirkin wrote:
> 
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index e14f5aa..fd2fd6f 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1844,6 +1844,60 @@ static unsigned int features[] = {
> > >  	VIRTIO_CONSOLE_F_MULTIPORT,
> > >  };
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtcons_freeze(struct virtio_device *vdev)
> > > +{
> > > +	struct ports_device *portdev;
> > > +	struct port *port;
> > > +
> > > +	portdev = vdev->priv;
> > > +
> > > +	vdev->config->reset(vdev);
> > 
> > 
> > So here, cancel_work_sync might still be running.
> > If it does run, might it try to access the device
> > config?  Could not determine this quickly, if yes it's a problem.
> 
> Similar to the other comment: I don't see why just resetting device
> can cause config queue access to go bad.
> 
> 		Amit

Hmm, not sure anymore. Sorry.

Re: [PATCH v4 04/12] virtio: console: Add freeze and restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 04:29:55PM +0530, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [12:43:24], Michael S. Tsirkin wrote:
> 
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index e14f5aa..fd2fd6f 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1844,6 +1844,60 @@ static unsigned int features[] = {
> > >  	VIRTIO_CONSOLE_F_MULTIPORT,
> > >  };
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtcons_freeze(struct virtio_device *vdev)
> > > +{
> > > +	struct ports_device *portdev;
> > > +	struct port *port;
> > > +
> > > +	portdev = vdev->priv;
> > > +
> > > +	vdev->config->reset(vdev);
> > 
> > 
> > So here, cancel_work_sync might still be running.
> > If it does run, might it try to access the device
> > config?  Could not determine this quickly, if yes it's a problem.
> 
> Similar to the other comment: I don't see why just resetting device
> can cause config queue access to go bad.
> 
> 		Amit

Hmm, not sure anymore. Sorry.

[PATCH v4 05/12] virtio: blk: Move out vq initialization to separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4d0b70a..467f218 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -349,6 +349,18 @@ static void virtblk_config_changed(struct virtio_device *vdev)
 	queue_work(virtblk_wq, &vblk->config_work);
 }
 
+static int init_vq(struct virtio_blk *vblk)
+{
+	int err = 0;
+
+	/* We expect one virtqueue, for output. */
+	vblk->vq = virtio_find_single_vq(vblk->vdev, blk_done, "requests");
+	if (IS_ERR(vblk->vq))
+		err = PTR_ERR(vblk->vq);
+
+	return err;
+}
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -390,12 +402,9 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	sg_init_table(vblk->sg, vblk->sg_elems);
 	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
-	/* We expect one virtqueue, for output. */
-	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
-	if (IS_ERR(vblk->vq)) {
-		err = PTR_ERR(vblk->vq);
+	err = init_vq(vblk);
+	if (err)
 		goto out_free_vblk;
-	}
 
 	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
 	if (!vblk->pool) {
-- 
1.7.7.3

[PATCH v4 05/12] virtio: blk: Move out vq initialization to separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4d0b70a..467f218 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -349,6 +349,18 @@ static void virtblk_config_changed(struct virtio_device *vdev)
 	queue_work(virtblk_wq, &vblk->config_work);
 }
 
+static int init_vq(struct virtio_blk *vblk)
+{
+	int err = 0;
+
+	/* We expect one virtqueue, for output. */
+	vblk->vq = virtio_find_single_vq(vblk->vdev, blk_done, "requests");
+	if (IS_ERR(vblk->vq))
+		err = PTR_ERR(vblk->vq);
+
+	return err;
+}
+
 static int __devinit virtblk_probe(struct virtio_device *vdev)
 {
 	struct virtio_blk *vblk;
@@ -390,12 +402,9 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	sg_init_table(vblk->sg, vblk->sg_elems);
 	INIT_WORK(&vblk->config_work, virtblk_config_changed_work);
 
-	/* We expect one virtqueue, for output. */
-	vblk->vq = virtio_find_single_vq(vdev, blk_done, "requests");
-	if (IS_ERR(vblk->vq)) {
-		err = PTR_ERR(vblk->vq);
+	err = init_vq(vblk);
+	if (err)
 		goto out_free_vblk;
-	}
 
 	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
 	if (!vblk->pool) {
-- 
1.7.7.3

[PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Amit Shah]

Delete the vq and flush any pending requests from the block queue on the
freeze callback to prepare for hibernation.

Re-create the vq in the restore callback to resume normal function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 467f218..a9147a6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	ida_simple_remove(&vd_index_ida, index);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	/* Ensure we don't receive any more interrupts */
+	vdev->config->reset(vdev);
+
+	flush_work(&vblk->config_work);
+
+	spin_lock_irq(vblk->disk->queue->queue_lock);
+	blk_stop_queue(vblk->disk->queue);
+	spin_unlock_irq(vblk->disk->queue->queue_lock);
+	blk_sync_queue(vblk->disk->queue);
+
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+	int ret;
+
+	ret = init_vq(vdev->priv);
+	if (!ret) {
+		spin_lock_irq(vblk->disk->queue->queue_lock);
+		blk_start_queue(vblk->disk->queue);
+		spin_unlock_irq(vblk->disk->queue->queue_lock);
+	}
+	return ret;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -593,6 +627,10 @@ static struct virtio_driver __refdata virtio_blk = {
 	.probe			= virtblk_probe,
 	.remove			= __devexit_p(virtblk_remove),
 	.config_changed		= virtblk_config_changed,
+#ifdef CONFIG_PM
+	.freeze			= virtblk_freeze,
+	.restore		= virtblk_restore,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

[PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Amit Shah]

Delete the vq and flush any pending requests from the block queue on the
freeze callback to prepare for hibernation.

Re-create the vq in the restore callback to resume normal function.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 467f218..a9147a6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	ida_simple_remove(&vd_index_ida, index);
 }
 
+#ifdef CONFIG_PM
+static int virtblk_freeze(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+
+	/* Ensure we don't receive any more interrupts */
+	vdev->config->reset(vdev);
+
+	flush_work(&vblk->config_work);
+
+	spin_lock_irq(vblk->disk->queue->queue_lock);
+	blk_stop_queue(vblk->disk->queue);
+	spin_unlock_irq(vblk->disk->queue->queue_lock);
+	blk_sync_queue(vblk->disk->queue);
+
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int virtblk_restore(struct virtio_device *vdev)
+{
+	struct virtio_blk *vblk = vdev->priv;
+	int ret;
+
+	ret = init_vq(vdev->priv);
+	if (!ret) {
+		spin_lock_irq(vblk->disk->queue->queue_lock);
+		blk_start_queue(vblk->disk->queue);
+		spin_unlock_irq(vblk->disk->queue->queue_lock);
+	}
+	return ret;
+}
+#endif
+
 static const struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -593,6 +627,10 @@ static struct virtio_driver __refdata virtio_blk = {
 	.probe			= virtblk_probe,
 	.remove			= __devexit_p(virtblk_remove),
 	.config_changed		= virtblk_config_changed,
+#ifdef CONFIG_PM
+	.freeze			= virtblk_freeze,
+	.restore		= virtblk_restore,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

Re: [PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:44AM +0530, Amit Shah wrote:
> Delete the vq and flush any pending requests from the block queue on the
> freeze callback to prepare for hibernation.
> 
> Re-create the vq in the restore callback to resume normal function.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 467f218..a9147a6 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
>  	ida_simple_remove(&vd_index_ida, index);
>  }
>  
> +#ifdef CONFIG_PM
> +static int virtblk_freeze(struct virtio_device *vdev)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	/* Ensure we don't receive any more interrupts */
> +	vdev->config->reset(vdev);
> +
> +	flush_work(&vblk->config_work);

It bothers me that config work can be running
after reset here. If it does, it will not get sane
values from reading config.


Also, can there be stuff in the reqs list?
If yes is this a problem?

> +
> +	spin_lock_irq(vblk->disk->queue->queue_lock);
> +	blk_stop_queue(vblk->disk->queue);
> +	spin_unlock_irq(vblk->disk->queue->queue_lock);
> +	blk_sync_queue(vblk->disk->queue);
> +
> +	vdev->config->del_vqs(vdev);
> +	return 0;
> +}
> +

Thinking about it, looks like there's a bug in
virtblk_remove: if we get a config change after
flush_work we schedule another work.
That's a problem for sure as structure is removed.


> +static int virtblk_restore(struct virtio_device *vdev)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +	int ret;
> +
> +	ret = init_vq(vdev->priv);
> +	if (!ret) {
> +		spin_lock_irq(vblk->disk->queue->queue_lock);
> +		blk_start_queue(vblk->disk->queue);
> +		spin_unlock_irq(vblk->disk->queue->queue_lock);
> +	}
> +	return ret;
> +}
> +#endif
> +
>  static const struct virtio_device_id id_table[] = {
>  	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
>  	{ 0 },
> @@ -593,6 +627,10 @@ static struct virtio_driver __refdata virtio_blk = {
>  	.probe			= virtblk_probe,
>  	.remove			= __devexit_p(virtblk_remove),
>  	.config_changed		= virtblk_config_changed,
> +#ifdef CONFIG_PM
> +	.freeze			= virtblk_freeze,
> +	.restore		= virtblk_restore,
> +#endif
>  };
>  
>  static int __init init(void)
> -- 
> 1.7.7.3

Re: [PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:44AM +0530, Amit Shah wrote:
> Delete the vq and flush any pending requests from the block queue on the
> freeze callback to prepare for hibernation.
> 
> Re-create the vq in the restore callback to resume normal function.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 467f218..a9147a6 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
>  	ida_simple_remove(&vd_index_ida, index);
>  }
>  
> +#ifdef CONFIG_PM
> +static int virtblk_freeze(struct virtio_device *vdev)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +
> +	/* Ensure we don't receive any more interrupts */
> +	vdev->config->reset(vdev);
> +
> +	flush_work(&vblk->config_work);

It bothers me that config work can be running
after reset here. If it does, it will not get sane
values from reading config.


Also, can there be stuff in the reqs list?
If yes is this a problem?

> +
> +	spin_lock_irq(vblk->disk->queue->queue_lock);
> +	blk_stop_queue(vblk->disk->queue);
> +	spin_unlock_irq(vblk->disk->queue->queue_lock);
> +	blk_sync_queue(vblk->disk->queue);
> +
> +	vdev->config->del_vqs(vdev);
> +	return 0;
> +}
> +

Thinking about it, looks like there's a bug in
virtblk_remove: if we get a config change after
flush_work we schedule another work.
That's a problem for sure as structure is removed.


> +static int virtblk_restore(struct virtio_device *vdev)
> +{
> +	struct virtio_blk *vblk = vdev->priv;
> +	int ret;
> +
> +	ret = init_vq(vdev->priv);
> +	if (!ret) {
> +		spin_lock_irq(vblk->disk->queue->queue_lock);
> +		blk_start_queue(vblk->disk->queue);
> +		spin_unlock_irq(vblk->disk->queue->queue_lock);
> +	}
> +	return ret;
> +}
> +#endif
> +
>  static const struct virtio_device_id id_table[] = {
>  	{ VIRTIO_ID_BLOCK, VIRTIO_DEV_ANY_ID },
>  	{ 0 },
> @@ -593,6 +627,10 @@ static struct virtio_driver __refdata virtio_blk = {
>  	.probe			= virtblk_probe,
>  	.remove			= __devexit_p(virtblk_remove),
>  	.config_changed		= virtblk_config_changed,
> +#ifdef CONFIG_PM
> +	.freeze			= virtblk_freeze,
> +	.restore		= virtblk_restore,
> +#endif
>  };
>  
>  static int __init init(void)
> -- 
> 1.7.7.3

Re: [PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:37:02], Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2011 at 01:18:44AM +0530, Amit Shah wrote:
> > Delete the vq and flush any pending requests from the block queue on the
> > freeze callback to prepare for hibernation.
> > 
> > Re-create the vq in the restore callback to resume normal function.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 38 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 467f218..a9147a6 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> >  	ida_simple_remove(&vd_index_ida, index);
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static int virtblk_freeze(struct virtio_device *vdev)
> > +{
> > +	struct virtio_blk *vblk = vdev->priv;
> > +
> > +	/* Ensure we don't receive any more interrupts */
> > +	vdev->config->reset(vdev);
> > +
> > +	flush_work(&vblk->config_work);
> 
> It bothers me that config work can be running
> after reset here. If it does, it will not get sane
> values from reading config.

Why so?

The reset only ensures the host doesn't write anything more, isn't it?
Why would the values be affected?

> Also, can there be stuff in the reqs list?
> If yes is this a problem?

Should be all cleared by the two commands below.  At least that's my
expectation.  If not, let me know!

> > +	spin_lock_irq(vblk->disk->queue->queue_lock);
> > +	blk_stop_queue(vblk->disk->queue);
> > +	spin_unlock_irq(vblk->disk->queue->queue_lock);
> > +	blk_sync_queue(vblk->disk->queue);
> > +
> > +	vdev->config->del_vqs(vdev);
> > +	return 0;
> > +}
> > +
> 
> Thinking about it, looks like there's a bug in
> virtblk_remove: if we get a config change after
> flush_work we schedule another work.
> That's a problem for sure as structure is removed.

Yep, it is a potential issue.

		Amit

Re: [PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:37:02], Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2011 at 01:18:44AM +0530, Amit Shah wrote:
> > Delete the vq and flush any pending requests from the block queue on the
> > freeze callback to prepare for hibernation.
> > 
> > Re-create the vq in the restore callback to resume normal function.
> > 
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 38 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 467f218..a9147a6 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> >  	ida_simple_remove(&vd_index_ida, index);
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static int virtblk_freeze(struct virtio_device *vdev)
> > +{
> > +	struct virtio_blk *vblk = vdev->priv;
> > +
> > +	/* Ensure we don't receive any more interrupts */
> > +	vdev->config->reset(vdev);
> > +
> > +	flush_work(&vblk->config_work);
> 
> It bothers me that config work can be running
> after reset here. If it does, it will not get sane
> values from reading config.

Why so?

The reset only ensures the host doesn't write anything more, isn't it?
Why would the values be affected?

> Also, can there be stuff in the reqs list?
> If yes is this a problem?

Should be all cleared by the two commands below.  At least that's my
expectation.  If not, let me know!

> > +	spin_lock_irq(vblk->disk->queue->queue_lock);
> > +	blk_stop_queue(vblk->disk->queue);
> > +	spin_unlock_irq(vblk->disk->queue->queue_lock);
> > +	blk_sync_queue(vblk->disk->queue);
> > +
> > +	vdev->config->del_vqs(vdev);
> > +	return 0;
> > +}
> > +
> 
> Thinking about it, looks like there's a bug in
> virtblk_remove: if we get a config change after
> flush_work we schedule another work.
> That's a problem for sure as structure is removed.

Yep, it is a potential issue.

		Amit

Re: [PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 04:26:47PM +0530, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [12:37:02], Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2011 at 01:18:44AM +0530, Amit Shah wrote:
> > > Delete the vq and flush any pending requests from the block queue on the
> > > freeze callback to prepare for hibernation.
> > > 
> > > Re-create the vq in the restore callback to resume normal function.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 38 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index 467f218..a9147a6 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> > >  	ida_simple_remove(&vd_index_ida, index);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtblk_freeze(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_blk *vblk = vdev->priv;
> > > +
> > > +	/* Ensure we don't receive any more interrupts */
> > > +	vdev->config->reset(vdev);
> > > +
> > > +	flush_work(&vblk->config_work);
> > 
> > It bothers me that config work can be running
> > after reset here. If it does, it will not get sane
> > values from reading config.
> 
> Why so?
> 
> The reset only ensures the host doesn't write anything more, isn't it?
> Why would the values be affected?

Generally, not only that. Reset also clears configuration to the
reset value :) As since accesses are done byte
by byte you might get a value that is different from
*both* old and new one as a result.

But that is a general comment, specifically for block,
I don't know if there is a problem with this.

Same for console.

> > Also, can there be stuff in the reqs list?
> > If yes is this a problem?
> 
> Should be all cleared by the two commands below.  At least that's my
> expectation.  If not, let me know!
> 
> > > +	spin_lock_irq(vblk->disk->queue->queue_lock);
> > > +	blk_stop_queue(vblk->disk->queue);
> > > +	spin_unlock_irq(vblk->disk->queue->queue_lock);
> > > +	blk_sync_queue(vblk->disk->queue);
> > > +
> > > +	vdev->config->del_vqs(vdev);
> > > +	return 0;
> > > +}
> > > +
> > 
> > Thinking about it, looks like there's a bug in
> > virtblk_remove: if we get a config change after
> > flush_work we schedule another work.
> > That's a problem for sure as structure is removed.
> 
> Yep, it is a potential issue.
> 
> 		Amit

Sent a patch.

Re: [PATCH v4 06/12] virtio: blk: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 04:26:47PM +0530, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [12:37:02], Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2011 at 01:18:44AM +0530, Amit Shah wrote:
> > > Delete the vq and flush any pending requests from the block queue on the
> > > freeze callback to prepare for hibernation.
> > > 
> > > Re-create the vq in the restore callback to resume normal function.
> > > 
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  drivers/block/virtio_blk.c |   38 ++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 38 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > > index 467f218..a9147a6 100644
> > > --- a/drivers/block/virtio_blk.c
> > > +++ b/drivers/block/virtio_blk.c
> > > @@ -568,6 +568,40 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> > >  	ida_simple_remove(&vd_index_ida, index);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtblk_freeze(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_blk *vblk = vdev->priv;
> > > +
> > > +	/* Ensure we don't receive any more interrupts */
> > > +	vdev->config->reset(vdev);
> > > +
> > > +	flush_work(&vblk->config_work);
> > 
> > It bothers me that config work can be running
> > after reset here. If it does, it will not get sane
> > values from reading config.
> 
> Why so?
> 
> The reset only ensures the host doesn't write anything more, isn't it?
> Why would the values be affected?

Generally, not only that. Reset also clears configuration to the
reset value :) As since accesses are done byte
by byte you might get a value that is different from
*both* old and new one as a result.

But that is a general comment, specifically for block,
I don't know if there is a problem with this.

Same for console.

> > Also, can there be stuff in the reqs list?
> > If yes is this a problem?
> 
> Should be all cleared by the two commands below.  At least that's my
> expectation.  If not, let me know!
> 
> > > +	spin_lock_irq(vblk->disk->queue->queue_lock);
> > > +	blk_stop_queue(vblk->disk->queue);
> > > +	spin_unlock_irq(vblk->disk->queue->queue_lock);
> > > +	blk_sync_queue(vblk->disk->queue);
> > > +
> > > +	vdev->config->del_vqs(vdev);
> > > +	return 0;
> > > +}
> > > +
> > 
> > Thinking about it, looks like there's a bug in
> > virtblk_remove: if we get a config change after
> > flush_work we schedule another work.
> > That's a problem for sure as structure is removed.
> 
> Yep, it is a potential issue.
> 
> 		Amit

Sent a patch.

[PATCH v4 07/12] virtio: net: Move out vq initialization into separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   47 +++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6ee8410..6baa563 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -954,15 +954,38 @@ static void virtnet_config_changed(struct virtio_device *vdev)
 	virtnet_update_status(vi);
 }
 
+static int init_vqs(struct virtnet_info *vi)
+{
+	struct virtqueue *vqs[3];
+	vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
+	const char *names[] = { "input", "output", "control" };
+	int nvqs, err;
+
+	/* We expect two virtqueues, receive then send,
+	 * and optionally control. */
+	nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
+
+	err = vi->vdev->config->find_vqs(vi->vdev, nvqs, vqs, callbacks, names);
+	if (err)
+		return err;
+
+	vi->rvq = vqs[0];
+	vi->svq = vqs[1];
+
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
+		vi->cvq = vqs[2];
+
+		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
+			vi->dev->features |= NETIF_F_HW_VLAN_FILTER;
+	}
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int err;
 	struct net_device *dev;
 	struct virtnet_info *vi;
-	struct virtqueue *vqs[3];
-	vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
-	const char *names[] = { "input", "output", "control" };
-	int nvqs;
 
 	/* Allocate ourselves a network device with room for our info */
 	dev = alloc_etherdev(sizeof(struct virtnet_info));
@@ -1034,24 +1057,10 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
 		vi->mergeable_rx_bufs = true;
 
-	/* We expect two virtqueues, receive then send,
-	 * and optionally control. */
-	nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
-
-	err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+	err = init_vqs(vi);
 	if (err)
 		goto free_stats;
 
-	vi->rvq = vqs[0];
-	vi->svq = vqs[1];
-
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
-		vi->cvq = vqs[2];
-
-		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
-			dev->features |= NETIF_F_HW_VLAN_FILTER;
-	}
-
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");
-- 
1.7.7.3

[PATCH v4 07/12] virtio: net: Move out vq initialization into separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   47 +++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6ee8410..6baa563 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -954,15 +954,38 @@ static void virtnet_config_changed(struct virtio_device *vdev)
 	virtnet_update_status(vi);
 }
 
+static int init_vqs(struct virtnet_info *vi)
+{
+	struct virtqueue *vqs[3];
+	vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
+	const char *names[] = { "input", "output", "control" };
+	int nvqs, err;
+
+	/* We expect two virtqueues, receive then send,
+	 * and optionally control. */
+	nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
+
+	err = vi->vdev->config->find_vqs(vi->vdev, nvqs, vqs, callbacks, names);
+	if (err)
+		return err;
+
+	vi->rvq = vqs[0];
+	vi->svq = vqs[1];
+
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
+		vi->cvq = vqs[2];
+
+		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
+			vi->dev->features |= NETIF_F_HW_VLAN_FILTER;
+	}
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int err;
 	struct net_device *dev;
 	struct virtnet_info *vi;
-	struct virtqueue *vqs[3];
-	vq_callback_t *callbacks[] = { skb_recv_done, skb_xmit_done, NULL};
-	const char *names[] = { "input", "output", "control" };
-	int nvqs;
 
 	/* Allocate ourselves a network device with room for our info */
 	dev = alloc_etherdev(sizeof(struct virtnet_info));
@@ -1034,24 +1057,10 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
 		vi->mergeable_rx_bufs = true;
 
-	/* We expect two virtqueues, receive then send,
-	 * and optionally control. */
-	nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
-
-	err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+	err = init_vqs(vi);
 	if (err)
 		goto free_stats;
 
-	vi->rvq = vqs[0];
-	vi->svq = vqs[1];
-
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ)) {
-		vi->cvq = vqs[2];
-
-		if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VLAN))
-			dev->features |= NETIF_F_HW_VLAN_FILTER;
-	}
-
 	err = register_netdev(dev);
 	if (err) {
 		pr_debug("virtio_net: registering device failed\n");
-- 
1.7.7.3

[PATCH v4 08/12] virtio: net: Move out vq and vq buf removal into separate function

[Amit Shah]

The remove and PM freeze functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6baa563..697a0fc 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1123,24 +1123,29 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	BUG_ON(vi->num != 0);
 }
 
-static void __devexit virtnet_remove(struct virtio_device *vdev)
+static void remove_vq_common(struct virtnet_info *vi)
 {
-	struct virtnet_info *vi = vdev->priv;
-
 	/* Stop all the virtqueues. */
-	vdev->config->reset(vdev);
-
+	vi->vdev->config->reset(vi->vdev);
 
-	unregister_netdev(vi->dev);
 	cancel_delayed_work_sync(&vi->refill);
 
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
-	vdev->config->del_vqs(vi->vdev);
+	vi->vdev->config->del_vqs(vi->vdev);
 
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
+}
+
+static void __devexit virtnet_remove(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	unregister_netdev(vi->dev);
+
+	remove_vq_common(vi);
 
 	free_percpu(vi->stats);
 	free_netdev(vi->dev);
-- 
1.7.7.3

[PATCH v4 08/12] virtio: net: Move out vq and vq buf removal into separate function

[Amit Shah]

The remove and PM freeze functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6baa563..697a0fc 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1123,24 +1123,29 @@ static void free_unused_bufs(struct virtnet_info *vi)
 	BUG_ON(vi->num != 0);
 }
 
-static void __devexit virtnet_remove(struct virtio_device *vdev)
+static void remove_vq_common(struct virtnet_info *vi)
 {
-	struct virtnet_info *vi = vdev->priv;
-
 	/* Stop all the virtqueues. */
-	vdev->config->reset(vdev);
-
+	vi->vdev->config->reset(vi->vdev);
 
-	unregister_netdev(vi->dev);
 	cancel_delayed_work_sync(&vi->refill);
 
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
-	vdev->config->del_vqs(vi->vdev);
+	vi->vdev->config->del_vqs(vi->vdev);
 
 	while (vi->pages)
 		__free_pages(get_a_page(vi, GFP_KERNEL), 0);
+}
+
+static void __devexit virtnet_remove(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	unregister_netdev(vi->dev);
+
+	remove_vq_common(vi);
 
 	free_percpu(vi->stats);
 	free_netdev(vi->dev);
-- 
1.7.7.3

[PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Amit Shah]

Remove all the vqs, disable napi and detach from the netdev on
hibernation.

Re-create vqs after restoring from a hibernated image, re-enable napi
and re-attach the netdev.  This keeps networking working across
hibernation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 697a0fc..1378f3c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
 	free_netdev(vi->dev);
 }
 
+#ifdef CONFIG_PM
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	netif_device_detach(vi->dev);
+	if (netif_running(vi->dev))
+		napi_disable(&vi->napi);
+
+	remove_vq_common(vi);
+
+	return 0;
+}
+
+static int virtnet_restore(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+	int err;
+
+	err = init_vqs(vi);
+	if (err)
+		return err;
+
+	try_fill_recv(vi, GFP_KERNEL);
+	if (netif_running(vi->dev))
+		virtnet_napi_enable(vi);
+
+	netif_device_attach(vi->dev);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1175,6 +1207,10 @@ static struct virtio_driver virtio_net_driver = {
 	.probe =	virtnet_probe,
 	.remove =	__devexit_p(virtnet_remove),
 	.config_changed = virtnet_config_changed,
+#ifdef CONFIG_PM
+	.freeze =	virtnet_freeze,
+	.restore =	virtnet_restore,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

[PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Amit Shah]

Remove all the vqs, disable napi and detach from the netdev on
hibernation.

Re-create vqs after restoring from a hibernated image, re-enable napi
and re-attach the netdev.  This keeps networking working across
hibernation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/net/virtio_net.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 697a0fc..1378f3c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
 	free_netdev(vi->dev);
 }
 
+#ifdef CONFIG_PM
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+
+	netif_device_detach(vi->dev);
+	if (netif_running(vi->dev))
+		napi_disable(&vi->napi);
+
+	remove_vq_common(vi);
+
+	return 0;
+}
+
+static int virtnet_restore(struct virtio_device *vdev)
+{
+	struct virtnet_info *vi = vdev->priv;
+	int err;
+
+	err = init_vqs(vi);
+	if (err)
+		return err;
+
+	try_fill_recv(vi, GFP_KERNEL);
+	if (netif_running(vi->dev))
+		virtnet_napi_enable(vi);
+
+	netif_device_attach(vi->dev);
+	return 0;
+}
+#endif
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1175,6 +1207,10 @@ static struct virtio_driver virtio_net_driver = {
 	.probe =	virtnet_probe,
 	.remove =	__devexit_p(virtnet_remove),
 	.config_changed = virtnet_config_changed,
+#ifdef CONFIG_PM
+	.freeze =	virtnet_freeze,
+	.restore =	virtnet_restore,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 697a0fc..1378f3c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
>  	free_netdev(vi->dev);
>  }
>  
> +#ifdef CONFIG_PM
> +static int virtnet_freeze(struct virtio_device *vdev)
> +{
> +	struct virtnet_info *vi = vdev->priv;
> +
> +	netif_device_detach(vi->dev);
> +	if (netif_running(vi->dev))
> +		napi_disable(&vi->napi);
> +

Could refill_work still be running at this point?
If yes it can re-enable napi and cause other kind of trouble.

> +	remove_vq_common(vi);
> +
> +	return 0;
> +}

Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote:
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 697a0fc..1378f3c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
>  	free_netdev(vi->dev);
>  }
>  
> +#ifdef CONFIG_PM
> +static int virtnet_freeze(struct virtio_device *vdev)
> +{
> +	struct virtnet_info *vi = vdev->priv;
> +
> +	netif_device_detach(vi->dev);
> +	if (netif_running(vi->dev))
> +		napi_disable(&vi->napi);
> +

Could refill_work still be running at this point?
If yes it can re-enable napi and cause other kind of trouble.

> +	remove_vq_common(vi);
> +
> +	return 0;
> +}

Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:28:24], Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 697a0fc..1378f3c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
> >  	free_netdev(vi->dev);
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static int virtnet_freeze(struct virtio_device *vdev)
> > +{
> > +	struct virtnet_info *vi = vdev->priv;
> > +
> > +	netif_device_detach(vi->dev);
> > +	if (netif_running(vi->dev))
> > +		napi_disable(&vi->napi);
> > +
> 
> Could refill_work still be running at this point?

Yes, it could.  So moving the cancel_delayed_work_sync() before
disabling napi would work fine?  Anything else that might similar treatment?

		Amit

Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:28:24], Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote:
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 697a0fc..1378f3c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
> >  	free_netdev(vi->dev);
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static int virtnet_freeze(struct virtio_device *vdev)
> > +{
> > +	struct virtnet_info *vi = vdev->priv;
> > +
> > +	netif_device_detach(vi->dev);
> > +	if (netif_running(vi->dev))
> > +		napi_disable(&vi->napi);
> > +
> 
> Could refill_work still be running at this point?

Yes, it could.  So moving the cancel_delayed_work_sync() before
disabling napi would work fine?  Anything else that might similar treatment?

		Amit

Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 04:05:29PM +0530, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [12:28:24], Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote:
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 697a0fc..1378f3c 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
> > >  	free_netdev(vi->dev);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtnet_freeze(struct virtio_device *vdev)
> > > +{
> > > +	struct virtnet_info *vi = vdev->priv;
> > > +
> > > +	netif_device_detach(vi->dev);
> > > +	if (netif_running(vi->dev))
> > > +		napi_disable(&vi->napi);
> > > +
> > 
> > Could refill_work still be running at this point?
> 
> Yes, it could.  So moving the cancel_delayed_work_sync() before
> disabling napi would work fine?

No, because napi poll can schedule that.
Further, refill can reschedule itself.

It also looks like we have a bug in virtio net cleanup now:
cancel_delayed_work_sync is called after unregister, so
it will be calling napi API on an invalid device.
And, if it schedules itself it will run after device is gone.

I think we need some locking to fix this.

>  Anything else that might similar treatment?
> 
> 		Amit



-- 
MST

Re: [PATCH v4 09/12] virtio: net: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 04:05:29PM +0530, Amit Shah wrote:
> On (Wed) 07 Dec 2011 [12:28:24], Michael S. Tsirkin wrote:
> > On Wed, Dec 07, 2011 at 01:18:47AM +0530, Amit Shah wrote:
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 697a0fc..1378f3c 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1151,6 +1151,38 @@ static void __devexit virtnet_remove(struct virtio_device *vdev)
> > >  	free_netdev(vi->dev);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM
> > > +static int virtnet_freeze(struct virtio_device *vdev)
> > > +{
> > > +	struct virtnet_info *vi = vdev->priv;
> > > +
> > > +	netif_device_detach(vi->dev);
> > > +	if (netif_running(vi->dev))
> > > +		napi_disable(&vi->napi);
> > > +
> > 
> > Could refill_work still be running at this point?
> 
> Yes, it could.  So moving the cancel_delayed_work_sync() before
> disabling napi would work fine?

No, because napi poll can schedule that.
Further, refill can reschedule itself.

It also looks like we have a bug in virtio net cleanup now:
cancel_delayed_work_sync is called after unregister, so
it will be calling napi API on an invalid device.
And, if it schedules itself it will run after device is gone.

I think we need some locking to fix this.

>  Anything else that might similar treatment?
> 
> 		Amit



-- 
MST

[PATCH v4 10/12] virtio: balloon: ensure thread exists before stopping it

[Amit Shah]

The vballoon thread could have exited earlier and not re-started.
Ensure we don't try to stop a non-existent thread.

This can happen if the balloon driver goes into S4 state and the thread
exits (this code lands in the next patch).  If, however, on restore, the
vqs fail to initialise, the vballoon thread will not be re-created.
Upon a subsequent module removal in that state, we will end up
dereferencing an invalid pointer without this patch.
---
 drivers/virtio/virtio_balloon.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 94fd738..22f7c69 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -338,7 +338,9 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb = vdev->priv;
 
-	kthread_stop(vb->thread);
+	/* Thread may not have started on restore after a suspend */
+	if (vb->thread)
+		kthread_stop(vb->thread);
 
 	/* There might be pages left in the balloon: free them. */
 	while (vb->num_pages)
-- 
1.7.7.3

[PATCH v4 10/12] virtio: balloon: ensure thread exists before stopping it

[Amit Shah]

The vballoon thread could have exited earlier and not re-started.
Ensure we don't try to stop a non-existent thread.

This can happen if the balloon driver goes into S4 state and the thread
exits (this code lands in the next patch).  If, however, on restore, the
vqs fail to initialise, the vballoon thread will not be re-created.
Upon a subsequent module removal in that state, we will end up
dereferencing an invalid pointer without this patch.
---
 drivers/virtio/virtio_balloon.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 94fd738..22f7c69 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -338,7 +338,9 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb = vdev->priv;
 
-	kthread_stop(vb->thread);
+	/* Thread may not have started on restore after a suspend */
+	if (vb->thread)
+		kthread_stop(vb->thread);
 
 	/* There might be pages left in the balloon: free them. */
 	while (vb->num_pages)
-- 
1.7.7.3

[PATCH v4 11/12] virtio: balloon: Move out vq initialization into separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c |   48 ++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 22f7c69..8bf99be 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -275,32 +275,21 @@ static int balloon(void *_vballoon)
 	return 0;
 }
 
-static int virtballoon_probe(struct virtio_device *vdev)
+static int init_vqs(struct virtio_balloon *vb)
 {
-	struct virtio_balloon *vb;
 	struct virtqueue *vqs[3];
 	vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
 	const char *names[] = { "inflate", "deflate", "stats" };
 	int err, nvqs;
 
-	vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
-	if (!vb) {
-		err = -ENOMEM;
-		goto out;
-	}
-
-	INIT_LIST_HEAD(&vb->pages);
-	vb->num_pages = 0;
-	init_waitqueue_head(&vb->config_change);
-	vb->vdev = vdev;
-	vb->need_stats_update = 0;
-
-	/* We expect two virtqueues: inflate and deflate,
-	 * and optionally stat. */
+	/*
+	 * We expect two virtqueues: inflate and deflate, and
+	 * optionally stat.
+	 */
 	nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
-	err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names);
 	if (err)
-		goto out_free_vb;
+		return err;
 
 	vb->inflate_vq = vqs[0];
 	vb->deflate_vq = vqs[1];
@@ -317,6 +306,29 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			BUG();
 		virtqueue_kick(vb->stats_vq);
 	}
+	return 0;
+}
+
+static int virtballoon_probe(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb;
+	int err;
+
+	vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
+	if (!vb) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&vb->pages);
+	vb->num_pages = 0;
+	init_waitqueue_head(&vb->config_change);
+	vb->vdev = vdev;
+	vb->need_stats_update = 0;
+
+	err = init_vqs(vb);
+	if (err)
+		goto out_free_vb;
 
 	vb->thread = kthread_run(balloon, vb, "vballoon");
 	if (IS_ERR(vb->thread)) {
-- 
1.7.7.3

[PATCH v4 11/12] virtio: balloon: Move out vq initialization into separate function

[Amit Shah]

The probe and PM restore functions will share this code.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c |   48 ++++++++++++++++++++++++--------------
 1 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 22f7c69..8bf99be 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -275,32 +275,21 @@ static int balloon(void *_vballoon)
 	return 0;
 }
 
-static int virtballoon_probe(struct virtio_device *vdev)
+static int init_vqs(struct virtio_balloon *vb)
 {
-	struct virtio_balloon *vb;
 	struct virtqueue *vqs[3];
 	vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
 	const char *names[] = { "inflate", "deflate", "stats" };
 	int err, nvqs;
 
-	vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
-	if (!vb) {
-		err = -ENOMEM;
-		goto out;
-	}
-
-	INIT_LIST_HEAD(&vb->pages);
-	vb->num_pages = 0;
-	init_waitqueue_head(&vb->config_change);
-	vb->vdev = vdev;
-	vb->need_stats_update = 0;
-
-	/* We expect two virtqueues: inflate and deflate,
-	 * and optionally stat. */
+	/*
+	 * We expect two virtqueues: inflate and deflate, and
+	 * optionally stat.
+	 */
 	nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
-	err = vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names);
+	err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names);
 	if (err)
-		goto out_free_vb;
+		return err;
 
 	vb->inflate_vq = vqs[0];
 	vb->deflate_vq = vqs[1];
@@ -317,6 +306,29 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			BUG();
 		virtqueue_kick(vb->stats_vq);
 	}
+	return 0;
+}
+
+static int virtballoon_probe(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb;
+	int err;
+
+	vdev->priv = vb = kmalloc(sizeof(*vb), GFP_KERNEL);
+	if (!vb) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&vb->pages);
+	vb->num_pages = 0;
+	init_waitqueue_head(&vb->config_change);
+	vb->vdev = vdev;
+	vb->need_stats_update = 0;
+
+	err = init_vqs(vb);
+	if (err)
+		goto out_free_vb;
 
 	vb->thread = kthread_run(balloon, vb, "vballoon");
 	if (IS_ERR(vb->thread)) {
-- 
1.7.7.3

[PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Amit Shah]

Handling balloon hibernate / restore is tricky.  If the balloon was
inflated before going into the hibernation state, upon resume, the host
will not have any memory of that.  Any pages that were passed on to the
host earlier would most likely be invalid, and the host will have to
re-balloon to the previous value to get in the pre-hibernate state.

So the only sane thing for the guest to do here is to discard all the
pages that were put in the balloon.  When to discard the pages is the
next question.

One solution is to deflate the balloon just before writing the image to
the disk (in the freeze() PM callback).  However, asking for pages from
the host just to discard them immediately after seems wasteful of
resources.  Hence, it makes sense to do this by just fudging our
counters soon after wakeup.  This means we don't deflate the balloon
before sleep, and also don't put unnecessary pressure on the host.

This also helps in the thaw case: if the freeze fails for whatever
reason, the balloon should continue to remain in the inflated state.
This was tested by issuing 'swapoff -a' and trying to go into the S4
state.  That fails, and the balloon stays inflated, as expected.  Both
the host and the guest are happy.

Now to not race with a host issuing ballooning requests while we are in
the process of freezing, we just exit from the vballoon kthread when the
processes are asked to freeze.  Upon thaw and restore, we re-start the
thread.

Finally, in the restore() callback, we empty the list of pages that were
previously given off to the host, add the appropriate number of pages to
the totalram_pages counter, reset the num_pages counter to 0, and
all is fine.

As a last step, delete the vqs on the freeze callback to prepare for
hibernation, and re-create them in the restore and thaw callbacks to
resume normal operation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c |   79 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8bf99be..10ec638 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -258,7 +258,13 @@ static int balloon(void *_vballoon)
 	while (!kthread_should_stop()) {
 		s64 diff;
 
-		try_to_freeze();
+		/*
+		 * On suspend, we want to exit this thread.  We will
+		 * start a new thread on resume.
+		 */
+		if (freezing(current))
+			break;
+
 		wait_event_interruptible(vb->config_change,
 					 (diff = towards_target(vb)) != 0
 					 || vb->need_stats_update
@@ -365,6 +371,72 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Ensure we don't get any more requests from the host */
+	vdev->config->reset(vdev);
+
+	/*
+	 * The kthread is already gone as a result of the PM code
+	 * issuing a freeze request.
+	 */
+
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int restore_common(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb = vdev->priv;
+	int err;
+
+	/*
+	 * If init_vqs below fails, a subsequent module removal
+	 * shouldn't cause us to dereference invalid pointers!
+	 */
+	vb->thread = NULL;
+
+	err = init_vqs(vdev->priv);
+	if (err)
+		return err;
+
+	vb->thread = kthread_run(balloon, vb, "vballoon");
+	if (IS_ERR(vb->thread)) {
+		err = PTR_ERR(vb->thread);
+		vb->thread = NULL;
+	}
+	return err;
+}
+
+static int virtballoon_thaw(struct virtio_device *vdev)
+{
+	return restore_common(vdev);
+}
+
+static int virtballoon_restore(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb = vdev->priv;
+	struct page *page, *page2;
+
+	/* We're starting from a clean slate */
+	vb->num_pages = 0;
+
+	/*
+	 * If a request wasn't complete at the time of freezing, this
+	 * could have been set.
+	 */
+	vb->need_stats_update = 0;
+
+	/* We don't have these pages in the balloon anymore! */
+	list_for_each_entry_safe(page, page2, &vb->pages, lru) {
+		list_del(&page->lru);
+		totalram_pages++;
+	}
+	return restore_common(vdev);
+}
+#endif
+
 static unsigned int features[] = {
 	VIRTIO_BALLOON_F_MUST_TELL_HOST,
 	VIRTIO_BALLOON_F_STATS_VQ,
@@ -379,6 +451,11 @@ static struct virtio_driver virtio_balloon_driver = {
 	.probe =	virtballoon_probe,
 	.remove =	__devexit_p(virtballoon_remove),
 	.config_changed = virtballoon_changed,
+#ifdef CONFIG_PM
+	.freeze	=	virtballoon_freeze,
+	.restore =	virtballoon_restore,
+	.thaw =		virtballoon_thaw,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

[PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Amit Shah]

Handling balloon hibernate / restore is tricky.  If the balloon was
inflated before going into the hibernation state, upon resume, the host
will not have any memory of that.  Any pages that were passed on to the
host earlier would most likely be invalid, and the host will have to
re-balloon to the previous value to get in the pre-hibernate state.

So the only sane thing for the guest to do here is to discard all the
pages that were put in the balloon.  When to discard the pages is the
next question.

One solution is to deflate the balloon just before writing the image to
the disk (in the freeze() PM callback).  However, asking for pages from
the host just to discard them immediately after seems wasteful of
resources.  Hence, it makes sense to do this by just fudging our
counters soon after wakeup.  This means we don't deflate the balloon
before sleep, and also don't put unnecessary pressure on the host.

This also helps in the thaw case: if the freeze fails for whatever
reason, the balloon should continue to remain in the inflated state.
This was tested by issuing 'swapoff -a' and trying to go into the S4
state.  That fails, and the balloon stays inflated, as expected.  Both
the host and the guest are happy.

Now to not race with a host issuing ballooning requests while we are in
the process of freezing, we just exit from the vballoon kthread when the
processes are asked to freeze.  Upon thaw and restore, we re-start the
thread.

Finally, in the restore() callback, we empty the list of pages that were
previously given off to the host, add the appropriate number of pages to
the totalram_pages counter, reset the num_pages counter to 0, and
all is fine.

As a last step, delete the vqs on the freeze callback to prepare for
hibernation, and re-create them in the restore and thaw callbacks to
resume normal operation.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 drivers/virtio/virtio_balloon.c |   79 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8bf99be..10ec638 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -258,7 +258,13 @@ static int balloon(void *_vballoon)
 	while (!kthread_should_stop()) {
 		s64 diff;
 
-		try_to_freeze();
+		/*
+		 * On suspend, we want to exit this thread.  We will
+		 * start a new thread on resume.
+		 */
+		if (freezing(current))
+			break;
+
 		wait_event_interruptible(vb->config_change,
 					 (diff = towards_target(vb)) != 0
 					 || vb->need_stats_update
@@ -365,6 +371,72 @@ static void __devexit virtballoon_remove(struct virtio_device *vdev)
 	kfree(vb);
 }
 
+#ifdef CONFIG_PM
+static int virtballoon_freeze(struct virtio_device *vdev)
+{
+	/* Ensure we don't get any more requests from the host */
+	vdev->config->reset(vdev);
+
+	/*
+	 * The kthread is already gone as a result of the PM code
+	 * issuing a freeze request.
+	 */
+
+	vdev->config->del_vqs(vdev);
+	return 0;
+}
+
+static int restore_common(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb = vdev->priv;
+	int err;
+
+	/*
+	 * If init_vqs below fails, a subsequent module removal
+	 * shouldn't cause us to dereference invalid pointers!
+	 */
+	vb->thread = NULL;
+
+	err = init_vqs(vdev->priv);
+	if (err)
+		return err;
+
+	vb->thread = kthread_run(balloon, vb, "vballoon");
+	if (IS_ERR(vb->thread)) {
+		err = PTR_ERR(vb->thread);
+		vb->thread = NULL;
+	}
+	return err;
+}
+
+static int virtballoon_thaw(struct virtio_device *vdev)
+{
+	return restore_common(vdev);
+}
+
+static int virtballoon_restore(struct virtio_device *vdev)
+{
+	struct virtio_balloon *vb = vdev->priv;
+	struct page *page, *page2;
+
+	/* We're starting from a clean slate */
+	vb->num_pages = 0;
+
+	/*
+	 * If a request wasn't complete at the time of freezing, this
+	 * could have been set.
+	 */
+	vb->need_stats_update = 0;
+
+	/* We don't have these pages in the balloon anymore! */
+	list_for_each_entry_safe(page, page2, &vb->pages, lru) {
+		list_del(&page->lru);
+		totalram_pages++;
+	}
+	return restore_common(vdev);
+}
+#endif
+
 static unsigned int features[] = {
 	VIRTIO_BALLOON_F_MUST_TELL_HOST,
 	VIRTIO_BALLOON_F_STATS_VQ,
@@ -379,6 +451,11 @@ static struct virtio_driver virtio_balloon_driver = {
 	.probe =	virtballoon_probe,
 	.remove =	__devexit_p(virtballoon_remove),
 	.config_changed = virtballoon_changed,
+#ifdef CONFIG_PM
+	.freeze	=	virtballoon_freeze,
+	.restore =	virtballoon_restore,
+	.thaw =		virtballoon_thaw,
+#endif
 };
 
 static int __init init(void)
-- 
1.7.7.3

Re: [PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [01:18:50], Amit Shah wrote:

[snip]

> Now to not race with a host issuing ballooning requests while we are in
> the process of freezing, we just exit from the vballoon kthread when the
> processes are asked to freeze.  Upon thaw and restore, we re-start the
> thread.

Actually this isn't necessary.  I over-zealously killed the thread
when it's not really necessary: the thread is frozen before calling
the freeze() callback and is thawed only after the restore() or thaw()
callbacks are done, so we're exactly in the same state with or without
keeping the kthread around (just that the PID of the kthread will
change).  So I'll back out this change for the next revision.


		Amit

Re: [PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [01:18:50], Amit Shah wrote:

[snip]

> Now to not race with a host issuing ballooning requests while we are in
> the process of freezing, we just exit from the vballoon kthread when the
> processes are asked to freeze.  Upon thaw and restore, we re-start the
> thread.

Actually this isn't necessary.  I over-zealously killed the thread
when it's not really necessary: the thread is frozen before calling
the freeze() callback and is thawed only after the restore() or thaw()
callbacks are done, so we're exactly in the same state with or without
keeping the kthread around (just that the PID of the kthread will
change).  So I'll back out this change for the next revision.


		Amit

Re: (resend) [PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:50AM +0530, Amit Shah wrote:
> Now to not race with a host issuing ballooning requests while we are in
> the process of freezing, we just exit from the vballoon kthread when the
> processes are asked to freeze.  Upon thaw and restore, we re-start the
> thread.

...

> ---
>  drivers/virtio/virtio_balloon.c |   79 ++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 78 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 8bf99be..10ec638 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -258,7 +258,13 @@ static int balloon(void *_vballoon)
>  	while (!kthread_should_stop()) {
>  		s64 diff;
>  
> -		try_to_freeze();
> +		/*
> +		 * On suspend, we want to exit this thread.  We will
> +		 * start a new thread on resume.
> +		 */
> +		if (freezing(current))
> +			break;
> +
>  		wait_event_interruptible(vb->config_change,
>  					 (diff = towards_target(vb)) != 0
>  					 || vb->need_stats_update

...

Note: this relies on kthreads being frozen before devices.
Looking at kernel/power/hibernate.c this is the case,
but I think we should add a comment to note this.

Also Cc linux-pm crowd in case I got it wrong.


---

Resending due to corrupted headers. Sorry about the noise.

-- 
MST

Re: (resend) [PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Michael S. Tsirkin]

On Wed, Dec 07, 2011 at 01:18:50AM +0530, Amit Shah wrote:
> Now to not race with a host issuing ballooning requests while we are in
> the process of freezing, we just exit from the vballoon kthread when the
> processes are asked to freeze.  Upon thaw and restore, we re-start the
> thread.

...

> ---
>  drivers/virtio/virtio_balloon.c |   79 ++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 78 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 8bf99be..10ec638 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -258,7 +258,13 @@ static int balloon(void *_vballoon)
>  	while (!kthread_should_stop()) {
>  		s64 diff;
>  
> -		try_to_freeze();
> +		/*
> +		 * On suspend, we want to exit this thread.  We will
> +		 * start a new thread on resume.
> +		 */
> +		if (freezing(current))
> +			break;
> +
>  		wait_event_interruptible(vb->config_change,
>  					 (diff = towards_target(vb)) != 0
>  					 || vb->need_stats_update

...

Note: this relies on kthreads being frozen before devices.
Looking at kernel/power/hibernate.c this is the case,
but I think we should add a comment to note this.

Also Cc linux-pm crowd in case I got it wrong.


---

Resending due to corrupted headers. Sorry about the noise.

-- 
MST

Re: (resend) [PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:34:48], Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2011 at 01:18:50AM +0530, Amit Shah wrote:
> > Now to not race with a host issuing ballooning requests while we are in
> > the process of freezing, we just exit from the vballoon kthread when the
> > processes are asked to freeze.  Upon thaw and restore, we re-start the
> > thread.
> 
> ...
> 
> > ---
> >  drivers/virtio/virtio_balloon.c |   79 ++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 78 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 8bf99be..10ec638 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -258,7 +258,13 @@ static int balloon(void *_vballoon)
> >  	while (!kthread_should_stop()) {
> >  		s64 diff;
> >  
> > -		try_to_freeze();
> > +		/*
> > +		 * On suspend, we want to exit this thread.  We will
> > +		 * start a new thread on resume.
> > +		 */
> > +		if (freezing(current))
> > +			break;
> > +
> >  		wait_event_interruptible(vb->config_change,
> >  					 (diff = towards_target(vb)) != 0
> >  					 || vb->need_stats_update
> 
> ...
> 
> Note: this relies on kthreads being frozen before devices.
> Looking at kernel/power/hibernate.c this is the case,
> but I think we should add a comment to note this.

Yes, it does.  And for that reason, I mentioned that stopping the
thread doesn't buy us anything; I'll revert this change in the next
submission.

> Also Cc linux-pm crowd in case I got it wrong.

		Amit

Re: (resend) [PATCH v4 12/12] virtio: balloon: Add freeze, restore handlers to support S4

[Amit Shah]

On (Wed) 07 Dec 2011 [12:34:48], Michael S. Tsirkin wrote:
> On Wed, Dec 07, 2011 at 01:18:50AM +0530, Amit Shah wrote:
> > Now to not race with a host issuing ballooning requests while we are in
> > the process of freezing, we just exit from the vballoon kthread when the
> > processes are asked to freeze.  Upon thaw and restore, we re-start the
> > thread.
> 
> ...
> 
> > ---
> >  drivers/virtio/virtio_balloon.c |   79 ++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 78 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 8bf99be..10ec638 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -258,7 +258,13 @@ static int balloon(void *_vballoon)
> >  	while (!kthread_should_stop()) {
> >  		s64 diff;
> >  
> > -		try_to_freeze();
> > +		/*
> > +		 * On suspend, we want to exit this thread.  We will
> > +		 * start a new thread on resume.
> > +		 */
> > +		if (freezing(current))
> > +			break;
> > +
> >  		wait_event_interruptible(vb->config_change,
> >  					 (diff = towards_target(vb)) != 0
> >  					 || vb->need_stats_update
> 
> ...
> 
> Note: this relies on kthreads being frozen before devices.
> Looking at kernel/power/hibernate.c this is the case,
> but I think we should add a comment to note this.

Yes, it does.  And for that reason, I mentioned that stopping the
thread doesn't buy us anything; I'll revert this change in the next
submission.

> Also Cc linux-pm crowd in case I got it wrong.

		Amit

Re: [PATCH v4 00/12] virtio: s4 support

[Rusty Russell]

On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> Hi,
> 
> These patches add support for S4 to virtio (pci) and all drivers.

Dumb meta-question: why do we want to hibernate virtual machines?

I figure there's a reason, but it seems a bit weird :)

Thanks,
Rusty.

Re: [PATCH v4 00/12] virtio: s4 support

[Rusty Russell]

On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> Hi,
> 
> These patches add support for S4 to virtio (pci) and all drivers.

Dumb meta-question: why do we want to hibernate virtual machines?

I figure there's a reason, but it seems a bit weird :)

Thanks,
Rusty.

Re: [PATCH v4 00/12] virtio: s4 support

[Amit Shah]

On (Wed) 07 Dec 2011 [17:54:29], Rusty Russell wrote:
> On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > Hi,
> > 
> > These patches add support for S4 to virtio (pci) and all drivers.
> 
> Dumb meta-question: why do we want to hibernate virtual machines?

Not a dumb question at all :)

But that doesn't mean I can't give a dumb answer: "Because We Can".

> I figure there's a reason, but it seems a bit weird :)

Well, there is one reason right now: migrating storage along with
VMs.  The guest needs to sync all data to the disk before the target
host accesses the image file.  One way to make sure guests don't access
the disk is by adding a new guest command to stop disk accesses.
However, we already have one way of making guests stop doing whatever
they are by putting them into S4 state, and then waking them up on the
remote, with them thinking nothing about them has changed.

(Did I manage to make this sound desirable after the answer above? :)

	     Amit

Re: [PATCH v4 00/12] virtio: s4 support

[Amit Shah]

On (Wed) 07 Dec 2011 [17:54:29], Rusty Russell wrote:
> On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > Hi,
> > 
> > These patches add support for S4 to virtio (pci) and all drivers.
> 
> Dumb meta-question: why do we want to hibernate virtual machines?

Not a dumb question at all :)

But that doesn't mean I can't give a dumb answer: "Because We Can".

> I figure there's a reason, but it seems a bit weird :)

Well, there is one reason right now: migrating storage along with
VMs.  The guest needs to sync all data to the disk before the target
host accesses the image file.  One way to make sure guests don't access
the disk is by adding a new guest command to stop disk accesses.
However, we already have one way of making guests stop doing whatever
they are by putting them into S4 state, and then waking them up on the
remote, with them thinking nothing about them has changed.

(Did I manage to make this sound desirable after the answer above? :)

	     Amit

Re: [PATCH v4 00/12] virtio: s4 support

[Rusty Russell]

On Wed, 7 Dec 2011 13:14:56 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 07 Dec 2011 [17:54:29], Rusty Russell wrote:
> > I figure there's a reason, but it seems a bit weird :)
> 
> Well, there is one reason right now: migrating storage along with
> VMs.  The guest needs to sync all data to the disk before the target
> host accesses the image file.

I don't see why.  Sure, if qemu (or whatever) is doing buffering, it
needs to flush that.  It doesn't, last I looked, it just maps to
read/write.

What am I missing?

Thanks,
Rusty.

Re: [PATCH v4 00/12] virtio: s4 support

[Rusty Russell]

On Wed, 7 Dec 2011 13:14:56 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) 07 Dec 2011 [17:54:29], Rusty Russell wrote:
> > I figure there's a reason, but it seems a bit weird :)
> 
> Well, there is one reason right now: migrating storage along with
> VMs.  The guest needs to sync all data to the disk before the target
> host accesses the image file.

I don't see why.  Sure, if qemu (or whatever) is doing buffering, it
needs to flush that.  It doesn't, last I looked, it just maps to
read/write.

What am I missing?

Thanks,
Rusty.

Re: [PATCH v4 00/12] virtio: s4 support

[Amit Shah]

On (Wed) 07 Dec 2011 [21:22:41], Rusty Russell wrote:
> On Wed, 7 Dec 2011 13:14:56 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Wed) 07 Dec 2011 [17:54:29], Rusty Russell wrote:
> > > I figure there's a reason, but it seems a bit weird :)
> > 
> > Well, there is one reason right now: migrating storage along with
> > VMs.  The guest needs to sync all data to the disk before the target
> > host accesses the image file.
> 
> I don't see why.  Sure, if qemu (or whatever) is doing buffering, it
> needs to flush that.  It doesn't, last I looked, it just maps to
> read/write.
> 
> What am I missing?

I'm sorry, I got confused.  The external file migration Gleb
mentioned, though, is something that libvirt uses right now and causes
guest time craziness so S4 is one (and natural) way to solve it.

		Amit

Re: [PATCH v4 00/12] virtio: s4 support

[Amit Shah]

On (Wed) 07 Dec 2011 [21:22:41], Rusty Russell wrote:
> On Wed, 7 Dec 2011 13:14:56 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > On (Wed) 07 Dec 2011 [17:54:29], Rusty Russell wrote:
> > > I figure there's a reason, but it seems a bit weird :)
> > 
> > Well, there is one reason right now: migrating storage along with
> > VMs.  The guest needs to sync all data to the disk before the target
> > host accesses the image file.
> 
> I don't see why.  Sure, if qemu (or whatever) is doing buffering, it
> needs to flush that.  It doesn't, last I looked, it just maps to
> read/write.
> 
> What am I missing?

I'm sorry, I got confused.  The external file migration Gleb
mentioned, though, is something that libvirt uses right now and causes
guest time craziness so S4 is one (and natural) way to solve it.

		Amit

Re: [PATCH v4 00/12] virtio: s4 support

[Gleb Natapov]

On Wed, Dec 07, 2011 at 05:54:29PM +1030, Rusty Russell wrote:
> On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > Hi,
> > 
> > These patches add support for S4 to virtio (pci) and all drivers.
> 
> Dumb meta-question: why do we want to hibernate virtual machines?
> 
For instance you want to hibernate your laptop while it is running a virtual
machine. You can pause them of course, but then time will be incorrect
inside a guest after resume.

> I figure there's a reason, but it seems a bit weird :)
> 
> Thanks,
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
			Gleb.

Re: [PATCH v4 00/12] virtio: s4 support

[Gleb Natapov]

On Wed, Dec 07, 2011 at 05:54:29PM +1030, Rusty Russell wrote:
> On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > Hi,
> > 
> > These patches add support for S4 to virtio (pci) and all drivers.
> 
> Dumb meta-question: why do we want to hibernate virtual machines?
> 
For instance you want to hibernate your laptop while it is running a virtual
machine. You can pause them of course, but then time will be incorrect
inside a guest after resume.

> I figure there's a reason, but it seems a bit weird :)
> 
> Thanks,
> Rusty.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
			Gleb.

Re: [PATCH v4 00/12] virtio: s4 support

[Dor Laor]

On 12/07/2011 12:44 PM, Gleb Natapov wrote:
> On Wed, Dec 07, 2011 at 05:54:29PM +1030, Rusty Russell wrote:
>> On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah<amit.shah@redhat.com>  wrote:
>>> Hi,
>>>
>>> These patches add support for S4 to virtio (pci) and all drivers.
>>
>> Dumb meta-question: why do we want to hibernate virtual machines?
>>
> For instance you want to hibernate your laptop while it is running a virtual
> machine. You can pause them of course, but then time will be incorrect
> inside a guest after resume.

Exactly. Today with do that through live migration into a file, we call 
it external hibernation. The problem that it is transparent from the 
guest and thus it loses time keeping. Even NTP can't handle it since if 
it has more than 0.5% delay it stops syncing.

The solution is to do it through s4 where the guest gets a notification 
when it resumes.


>
>> I figure there's a reason, but it seems a bit weird :)
>>
>> Thanks,
>> Rusty.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v4 00/12] virtio: s4 support

[Dor Laor]

On 12/07/2011 12:44 PM, Gleb Natapov wrote:
> On Wed, Dec 07, 2011 at 05:54:29PM +1030, Rusty Russell wrote:
>> On Wed,  7 Dec 2011 01:18:38 +0530, Amit Shah<amit.shah@redhat.com>  wrote:
>>> Hi,
>>>
>>> These patches add support for S4 to virtio (pci) and all drivers.
>>
>> Dumb meta-question: why do we want to hibernate virtual machines?
>>
> For instance you want to hibernate your laptop while it is running a virtual
> machine. You can pause them of course, but then time will be incorrect
> inside a guest after resume.

Exactly. Today with do that through live migration into a file, we call 
it external hibernation. The problem that it is transparent from the 
guest and thus it loses time keeping. Even NTP can't handle it since if 
it has more than 0.5% delay it stops syncing.

The solution is to do it through s4 where the guest gets a notification 
when it resumes.


>
>> I figure there's a reason, but it seems a bit weird :)
>>
>> Thanks,
>> Rusty.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> 			Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/