Re: [PATCH] virtio_console: remove pointless check for debugfs_create_dir()

[Amit Shah]

On Tue, 2021-02-16 at 16:04 +0100, Greg Kroah-Hartman wrote:
> It is impossible for debugfs_create_dir() to return NULL, so checking
> for it gives people a false sense that they actually are doing something
> if an error occurs.  As there is no need to ever change kernel logic if
> debugfs is working "properly" or not, there is no need to check the
> return value of debugfs calls, so remove the checks here as they will
> never be triggered and are wrong.
> 
> Cc: Amit Shah 
> Cc: Arnd Bergmann 
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/char/virtio_console.c | 23 +--
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1836cc56e357..59dfd9c421a1 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1456,18 +1456,15 @@ static int add_port(struct ports_device *portdev, u32 
> id)
>*/
>   send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
>  
> - if (pdrvdata.debugfs_dir) {
> - /*
> -  * Finally, create the debugfs file that we can use to
> -  * inspect a port's state at any time
> -  */
> - snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
> -  port->portdev->vdev->index, id);
> - port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
> -  pdrvdata.debugfs_dir,
> -  port,
> -  _debugfs_fops);
> - }
> + /*
> +  * Finally, create the debugfs file that we can use to
> +  * inspect a port's state at any time
> +  */
> + snprintf(debugfs_name, sizeof(debugfs_name), "vport%up%u",
> +  port->portdev->vdev->index, id);
> + port->debugfs_file = debugfs_create_file(debugfs_name, 0444,
> +  pdrvdata.debugfs_dir,
> +  port, _debugfs_fops);
>   return 0;
>  
>  free_inbufs:
> @@ -2244,8 +2241,6 @@ static int __init init(void)
>   }
>  
>   pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> - if (!pdrvdata.debugfs_dir)
> - pr_warn("Error creating debugfs dir for virtio-ports\n");
>   INIT_LIST_HEAD();
>   INIT_LIST_HEAD();
> 

Reviewed-by: Amit Shah 

Re: [PATCH] char: virtio: Select VIRTIO from VIRTIO_CONSOLE.

[Amit Shah]

On Mon, Aug 31, 2020 at 06:58:50PM +0200, Michal Suchanek wrote:
> Make it possible to have virtio console built-in when
> other virtio drivers are modular.
> 
> Signed-off-by: Michal Suchanek 

Reviewed-by: Amit Shah 

> ---
>  drivers/char/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 3a144c000a38..9bd9917ca9af 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -93,8 +93,9 @@ config PPDEV
>  
>  config VIRTIO_CONSOLE
>   tristate "Virtio console"
> - depends on VIRTIO && TTY
> + depends on TTY
>   select HVC_DRIVER
> + select VIRTIO
>   help
> Virtio console for use with hypervisors.
>  
> -- 
> 2.28.0
> 

Re: [PATCH 5/5] virtio_console: Constify some static variables

[Amit Shah]

On (Wed) 01 Jul 2020 [22:09:50], Rikard Falkeborn wrote:
> The id_table and feature_table pointers in struct virtio_driver are
> pointers to const. Mark the corresponding static variables const to
> allow the compiler to put them in read-only memory.
> 
> Before:
>textdata bss dec hex filename
>   25447 713  76   26236667c drivers/char/virtio_console.o
> 
> After:
>textdata bss dec hex filename
>   25488 673  76   26237667d drivers/char/virtio_console.o
> 
> Signed-off-by: Rikard Falkeborn 

Reviewed-by: Amit Shah 

Please CC me on the entire series instead of individual patches in the
future.

Thanks,

Amit
-- 
http://amitshah.net/

Re: [PATCH] virtio_console: remove vq buf while unpluging port

[Amit Shah]

On Fri, 2019-05-24 at 20:51 +0200, Greg KH wrote:
> On Sun, Apr 28, 2019 at 09:50:04AM +0800, zhenwei pi wrote:
> > A bug can be easily reproduced:
> > Host# cat guest-agent.xml
> > 
> >   
> >> state="connected"/>
> > 
> > Host# virsh attach-device instance guest-agent.xml
> > Host# virsh detach-device instance guest-agent.xml
> > Host# virsh attach-device instance guest-agent.xml
> > 
> > and guest report: virtio-ports vport0p1: Error allocating inbufs
> > 
> > The reason is that the port is unplugged and the vq buf still
> > remained.
> > So, fix two cases in this patch:
> > 1, fix memory leak with attach-device/detach-device.
> > 2, fix logic bug with attach-device/detach-device/attach-device.

The "leak" happens because the host-side of the connection is still
connected.  This is by design -- if a guest has written data before
being unplugged, the port isn't released till the host connection goes
down to ensure a host process reads all the data out of the port.

Can you try similar, but also disconnecting the host side and see if
that fixes things?

> Amit, any ideas if this is valid or not and if this should be
> applied?

This had indeed been missed, thanks!


Amit

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> > 
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > > 
> > > > Lightly tested.
> > > 
> > > Amit - any feedback before I push these patches?
> > 
> > The changes look good spec-wise.
> > 
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality.  Can you give those a try before pushing?
> > 
> > Amit
> 
> I pushed before I did that test, will try to find the time later.  BTW
> do you still want to be on the maintainers list?

Yes, I want to be on the maintainers list; but won't mind
co-maintainers.  The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.

Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> > 
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > > 
> > > > Lightly tested.
> > > 
> > > Amit - any feedback before I push these patches?
> > 
> > The changes look good spec-wise.
> > 
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality.  Can you give those a try before pushing?
> > 
> > Amit
> 
> I pushed before I did that test, will try to find the time later.  BTW
> do you still want to be on the maintainers list?

Yes, I want to be on the maintainers list; but won't mind
co-maintainers.  The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.

Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> > 
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > > 
> > > > Lightly tested.
> > > 
> > > Amit - any feedback before I push these patches?
> > 
> > The changes look good spec-wise.
> > 
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality.  Can you give those a try before pushing?
> > 
> > Amit
> 
> I pushed before I did that test, will try to find the time later.  BTW
> do you still want to be on the maintainers list?

Yes, I want to be on the maintainers list; but won't mind
co-maintainers.  The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.

Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> > 
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > > Turns out virtio console tries to take a buffer out of an active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > > 
> > > > Lightly tested.
> > > 
> > > Amit - any feedback before I push these patches?
> > 
> > The changes look good spec-wise.
> > 
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality.  Can you give those a try before pushing?
> > 
> > Amit
> 
> I pushed before I did that test, will try to find the time later.  BTW
> do you still want to be on the maintainers list?

Yes, I want to be on the maintainers list; but won't mind
co-maintainers.  The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.

Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

(apologies if you received a dup)

On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> > 
> > Lightly tested.
> 
> Amit - any feedback before I push these patches?

The changes look good spec-wise.

There are a bunch of tests in avocado-vt that test virtio-console
functionality.  Can you give those a try before pushing?

Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

(apologies if you received a dup)

On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> > 
> > Lightly tested.
> 
> Amit - any feedback before I push these patches?

The changes look good spec-wise.

There are a bunch of tests in avocado-vt that test virtio-console
functionality.  Can you give those a try before pushing?

Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> > 
> > Lightly tested.
> 
> Amit - any feedback before I push these patches?

The changes look good spec-wise.

There are a bunch of tests in avocado-vt that test virtio-console
functionality.  Can you give those a try before pushing?


Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> > 
> > Lightly tested.
> 
> Amit - any feedback before I push these patches?

The changes look good spec-wise.

There are a bunch of tests in avocado-vt that test virtio-console
functionality.  Can you give those a try before pushing?


Amit
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On 24 April 2018 11:41:29 AM GMT-07:00, "Michael S. Tsirkin"  
wrote:
>On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
>> Turns out virtio console tries to take a buffer out of an active vq.
>> Works by sheer luck, and is explicitly forbidden by spec.  And while
>> going over it I saw that error handling is also broken -
>> failure is easy to trigger if I force allocations to fail.
>> 
>> Lightly tested.
>
>Amit - any feedback before I push these patches?

I'm traveling this week, will be able to take a look early next week.

Amit

-- 
http://amitshah.net

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On 24 April 2018 11:41:29 AM GMT-07:00, "Michael S. Tsirkin"  
wrote:
>On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
>> Turns out virtio console tries to take a buffer out of an active vq.
>> Works by sheer luck, and is explicitly forbidden by spec.  And while
>> going over it I saw that error handling is also broken -
>> failure is easy to trigger if I force allocations to fail.
>> 
>> Lightly tested.
>
>Amit - any feedback before I push these patches?

I'm traveling this week, will be able to take a look early next week.

Amit

-- 
http://amitshah.net

[PATCH v2 2/2] xen: events: free irqs in error condition

[Amit Shah]

In case of errors in irq setup for MSI, free up the allocated irqs.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi <mirha...@amazon.com>
CC: <sta...@vger.kernel.org>
CC: Roger Pau Monné <roger@citrix.com>
CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
CC: Eduardo Valentin <edu...@amazon.com>
CC: Juergen Gross <jgr...@suse.com>
CC: Thomas Gleixner <t...@linutronix.de>
CC: "K. Y. Srinivasan" <k...@microsoft.com>
CC: Liu Shuo <shuo.a....@intel.com>
CC: Anoob Soman <anoob.so...@citrix.com>
Signed-off-by: Amit Shah <a...@amazon.com>
---
 drivers/xen/events/events_base.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index c86d10e..a299586 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
 
ret = irq_set_msi_desc(irq, msidesc);
if (ret < 0)
-   goto error_irq;
+   goto error_desc;
 out:
mutex_unlock(_mapping_update_lock);
return irq;
 error_irq:
+   while (--nvec >= i)
+   xen_free_irq(irq + nvec);
+error_desc:
while (i > 0) {
i--;
__unbind_from_irq(irq + i);
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups

[Amit Shah]

When an MSI descriptor was not available, the error path would try to
unbind an irq that was never acquired - potentially unbinding an
unrelated irq.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi <mirha...@amazon.com>
CC: <sta...@vger.kernel.org>
CC: Roger Pau Monné <roger@citrix.com>
CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
CC: Eduardo Valentin <edu...@amazon.com>
CC: Juergen Gross <jgr...@suse.com>
CC: Thomas Gleixner <t...@linutronix.de>
CC: "K. Y. Srinivasan" <k...@microsoft.com>
CC: Liu Shuo <shuo.a....@intel.com>
CC: Anoob Soman <anoob.so...@citrix.com>
Signed-off-by: Amit Shah <a...@amazon.com>
---
 drivers/xen/events/events_base.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1ab4bd1..c86d10e 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
mutex_unlock(_mapping_update_lock);
return irq;
 error_irq:
-   for (; i >= 0; i--)
+   while (i > 0) {
+   i--;
__unbind_from_irq(irq + i);
+   }
mutex_unlock(_mapping_update_lock);
return ret;
 }
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2 2/2] xen: events: free irqs in error condition

[Amit Shah]

In case of errors in irq setup for MSI, free up the allocated irqs.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi 
CC: 
CC: Roger Pau Monné 
CC: Boris Ostrovsky 
CC: Eduardo Valentin 
CC: Juergen Gross 
CC: Thomas Gleixner 
CC: "K. Y. Srinivasan" 
CC: Liu Shuo 
CC: Anoob Soman 
Signed-off-by: Amit Shah 
---
 drivers/xen/events/events_base.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index c86d10e..a299586 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -750,11 +750,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
 
ret = irq_set_msi_desc(irq, msidesc);
if (ret < 0)
-   goto error_irq;
+   goto error_desc;
 out:
mutex_unlock(_mapping_update_lock);
return irq;
 error_irq:
+   while (--nvec >= i)
+   xen_free_irq(irq + nvec);
+error_desc:
while (i > 0) {
i--;
__unbind_from_irq(irq + i);
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2 1/2] xen: fix out-of-bounds irq unbind for MSI message groups

[Amit Shah]

When an MSI descriptor was not available, the error path would try to
unbind an irq that was never acquired - potentially unbinding an
unrelated irq.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi 
CC: 
CC: Roger Pau Monné 
CC: Boris Ostrovsky 
CC: Eduardo Valentin 
CC: Juergen Gross 
CC: Thomas Gleixner 
CC: "K. Y. Srinivasan" 
CC: Liu Shuo 
CC: Anoob Soman 
Signed-off-by: Amit Shah 
---
 drivers/xen/events/events_base.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1ab4bd1..c86d10e 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -755,8 +755,10 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
mutex_unlock(_mapping_update_lock);
return irq;
 error_irq:
-   for (; i >= 0; i--)
+   while (i > 0) {
+   i--;
__unbind_from_irq(irq + i);
+   }
mutex_unlock(_mapping_update_lock);
return ret;
 }
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2 0/2] xen: fix bugs in error conditions

[Amit Shah]

Hello,

These bugs were found during code review.  Details in the commits.

Please review and apply.

v2:
 - fix up patch 2 properly (Roger Pau Monné)

CC: Roger Pau Monné <roger@citrix.com>
CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
CC: Eduardo Valentin <edu...@amazon.com>
CC: Juergen Gross <jgr...@suse.com>
CC: Thomas Gleixner <t...@linutronix.de>
CC: "K. Y. Srinivasan" <k...@microsoft.com>
CC: Liu Shuo <shuo.a@intel.com>
CC: Anoob Soman <anoob.so...@citrix.com>

Amit Shah (2):
  xen: fix out-of-bounds irq unbind for MSI message groups
  xen: events: free irqs in error condition

 drivers/xen/events/events_base.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2 0/2] xen: fix bugs in error conditions

[Amit Shah]

Hello,

These bugs were found during code review.  Details in the commits.

Please review and apply.

v2:
 - fix up patch 2 properly (Roger Pau Monné)

CC: Roger Pau Monné 
CC: Boris Ostrovsky 
CC: Eduardo Valentin 
CC: Juergen Gross 
CC: Thomas Gleixner 
CC: "K. Y. Srinivasan" 
CC: Liu Shuo 
CC: Anoob Soman 

Amit Shah (2):
  xen: fix out-of-bounds irq unbind for MSI message groups
  xen: events: free irqs in error condition

 drivers/xen/events/events_base.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH 0/2] xen: fix bugs in error conditions

[Amit Shah]

Hello,

These bugs were found during code review.  Details in the commits.

Please review and apply.

CC: Roger Pau Monné <roger@citrix.com>
CC: David Vrabel <david.vra...@citrix.com>
CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
CC: Eduardo Valentin <edu...@amazon.com>
CC: Juergen Gross <jgr...@suse.com>
CC: Thomas Gleixner <t...@linutronix.de>
CC: "K. Y. Srinivasan" <k...@microsoft.com>
CC: Liu Shuo <shuo.a@intel.com>
CC: Anoob Soman <anoob.so...@citrix.com>


Amit Shah (2):
  xen: fix out-of-bounds irq unbind for MSI message groups
  xen: events: free irqs in error condition

 drivers/xen/events/events_base.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH 0/2] xen: fix bugs in error conditions

[Amit Shah]

Hello,

These bugs were found during code review.  Details in the commits.

Please review and apply.

CC: Roger Pau Monné 
CC: David Vrabel 
CC: Boris Ostrovsky 
CC: Eduardo Valentin 
CC: Juergen Gross 
CC: Thomas Gleixner 
CC: "K. Y. Srinivasan" 
CC: Liu Shuo 
CC: Anoob Soman 


Amit Shah (2):
  xen: fix out-of-bounds irq unbind for MSI message groups
  xen: events: free irqs in error condition

 drivers/xen/events/events_base.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH 2/2] xen: events: free irqs in error condition

[Amit Shah]

In case of errors in irq setup for MSI, free up the allocated irqs.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi <mirha...@amazon.com>
CC: <sta...@vger.kernel.org>
CC: Roger Pau Monné <roger@citrix.com>
CC: David Vrabel <david.vra...@citrix.com>
CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
CC: Eduardo Valentin <edu...@amazon.com>
CC: Juergen Gross <jgr...@suse.com>
CC: Thomas Gleixner <t...@linutronix.de>
CC: "K. Y. Srinivasan" <k...@microsoft.com>
CC: Liu Shuo <shuo.a@intel.com>
CC: Anoob Soman <anoob.so...@citrix.com>
Signed-off-by: Amit Shah <a...@amazon.com>
---
 drivers/xen/events/events_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index b6b8b29..96aa575 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -758,6 +758,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
 error_irq:
for (; i >= 0; i--)
__unbind_from_irq(irq + i);
+   xen_free_irq(irq);
mutex_unlock(_mapping_update_lock);
return ret;
 }
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH 2/2] xen: events: free irqs in error condition

[Amit Shah]

In case of errors in irq setup for MSI, free up the allocated irqs.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi 
CC: 
CC: Roger Pau Monné 
CC: David Vrabel 
CC: Boris Ostrovsky 
CC: Eduardo Valentin 
CC: Juergen Gross 
CC: Thomas Gleixner 
CC: "K. Y. Srinivasan" 
CC: Liu Shuo 
CC: Anoob Soman 
Signed-off-by: Amit Shah 
---
 drivers/xen/events/events_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index b6b8b29..96aa575 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -758,6 +758,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
 error_irq:
for (; i >= 0; i--)
__unbind_from_irq(irq + i);
+   xen_free_irq(irq);
mutex_unlock(_mapping_update_lock);
return ret;
 }
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH 1/2] xen: fix out-of-bounds irq unbind for MSI message groups

[Amit Shah]

When an MSI descriptor was not available, the error path would try to
unbind an irq that was never acquired - potentially unbinding an
unrelated irq.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi <mirha...@amazon.com>
CC: <sta...@vger.kernel.org>
CC: Roger Pau Monné <roger@citrix.com>
CC: David Vrabel <david.vra...@citrix.com>
CC: Boris Ostrovsky <boris.ostrov...@oracle.com>
CC: Eduardo Valentin <edu...@amazon.com>
CC: Juergen Gross <jgr...@suse.com>
CC: Thomas Gleixner <t...@linutronix.de>
CC: "K. Y. Srinivasan" <k...@microsoft.com>
CC: Liu Shuo <shuo.a@intel.com>
CC: Anoob Soman <anoob.so...@citrix.com>
Signed-off-by: Amit Shah <a...@amazon.com>
---
 drivers/xen/events/events_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1ab4bd1..b6b8b29 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -749,6 +749,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
}
 
ret = irq_set_msi_desc(irq, msidesc);
+   i--;
if (ret < 0)
goto error_irq;
 out:
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH 1/2] xen: fix out-of-bounds irq unbind for MSI message groups

[Amit Shah]

When an MSI descriptor was not available, the error path would try to
unbind an irq that was never acquired - potentially unbinding an
unrelated irq.

Fixes: 4892c9b4ada9f9 ("xen: add support for MSI message groups")
Reported-by: Hooman Mirhadi 
CC: 
CC: Roger Pau Monné 
CC: David Vrabel 
CC: Boris Ostrovsky 
CC: Eduardo Valentin 
CC: Juergen Gross 
CC: Thomas Gleixner 
CC: "K. Y. Srinivasan" 
CC: Liu Shuo 
CC: Anoob Soman 
Signed-off-by: Amit Shah 
---
 drivers/xen/events/events_base.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 1ab4bd1..b6b8b29 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -749,6 +749,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
msi_desc *msidesc,
}
 
ret = irq_set_msi_desc(irq, msidesc);
+   i--;
if (ret < 0)
goto error_irq;
 out:
-- 
2.7.3.AMZN

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH 13/35] drivers/char: Convert remaining use of pr_warning to pr_warn

[Amit Shah]

On (Thu) 16 Feb 2017 [23:11:26], Joe Perches wrote:
> To enable eventual removal of pr_warning
> 
> This makes pr_warn use consistent for drivers/char
> 
> Prior to this patch, there were 1 use of pr_warning and
> 40 uses of pr_warn in drivers/char
> 
> Signed-off-by: Joe Perches <j...@perches.com>

Reviewed-by: Amit Shah <a...@kernel.org>

Thanks,

Amit
-- 
http://log.amitshah.net/

Re: [PATCH 13/35] drivers/char: Convert remaining use of pr_warning to pr_warn

[Amit Shah]

On (Thu) 16 Feb 2017 [23:11:26], Joe Perches wrote:
> To enable eventual removal of pr_warning
> 
> This makes pr_warn use consistent for drivers/char
> 
> Prior to this patch, there were 1 use of pr_warning and
> 40 uses of pr_warn in drivers/char
> 
> Signed-off-by: Joe Perches 

Reviewed-by: Amit Shah 

Thanks,

Amit
-- 
http://log.amitshah.net/

Re: [PATCH] MAINTAINERS: update my email address

[Amit Shah]

On (Fri) 03 Feb 2017 [17:11:49], Michael S. Tsirkin wrote:
> On Fri, Feb 03, 2017 at 04:48:14PM +0530, Amit Shah wrote:
> > I'm leaving my job at Red Hat, this email address will stop working next 
> > week.
> > Update it to one that I will have access to later.
> > 
> > Signed-off-by: Amit Shah <amit.s...@redhat.com>
> 
> It's great that we'll still have you around!  Do you want to send a pull
> request with this patch? Or want me to merge it?

Yes, I expect to be around :)

Please merge it via your tree, thanks.


Amit

Re: [PATCH] MAINTAINERS: update my email address

[Amit Shah]

On (Fri) 03 Feb 2017 [17:11:49], Michael S. Tsirkin wrote:
> On Fri, Feb 03, 2017 at 04:48:14PM +0530, Amit Shah wrote:
> > I'm leaving my job at Red Hat, this email address will stop working next 
> > week.
> > Update it to one that I will have access to later.
> > 
> > Signed-off-by: Amit Shah 
> 
> It's great that we'll still have you around!  Do you want to send a pull
> request with this patch? Or want me to merge it?

Yes, I expect to be around :)

Please merge it via your tree, thanks.


Amit

[PATCH] MAINTAINERS: update my email address

[Amit Shah]

I'm leaving my job at Red Hat, this email address will stop working next week.
Update it to one that I will have access to later.

Signed-off-by: Amit Shah <amit.s...@redhat.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3960e7f..187b961 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13065,7 +13065,7 @@ F:  drivers/input/serio/userio.c
 F: include/uapi/linux/userio.h
 
 VIRTIO CONSOLE DRIVER
-M:     Amit Shah <amit.s...@redhat.com>
+M:     Amit Shah <a...@kernel.org>
 L: virtualizat...@lists.linux-foundation.org
 S: Maintained
 F: drivers/char/virtio_console.c
-- 
1.8.3.1

[PATCH] MAINTAINERS: update my email address

[Amit Shah]

I'm leaving my job at Red Hat, this email address will stop working next week.
Update it to one that I will have access to later.

Signed-off-by: Amit Shah 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3960e7f..187b961 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13065,7 +13065,7 @@ F:  drivers/input/serio/userio.c
 F: include/uapi/linux/userio.h
 
 VIRTIO CONSOLE DRIVER
-M: Amit Shah 
+M: Amit Shah 
 L: virtualizat...@lists.linux-foundation.org
 S: Maintained
 F: drivers/char/virtio_console.c
-- 
1.8.3.1

Re: [PATCH] virtio-console: avoid DMA from stack

[Amit Shah]

On (Wed) 01 Feb 2017 [00:02:27], Omar Sandoval wrote:
> From: Omar Sandoval <osan...@fb.com>
> 
> put_chars() stuffs the buffer it gets into an sg, but that buffer may be
> on the stack. This breaks with CONFIG_VMAP_STACK=y (for me, it
> manifested as printks getting turned into NUL bytes).

Seems reasonable.  I wonder since all implementations of hvc do a
memcpy, if we can abstract it - but that'll need some work.

Reviewed-by: Amit Shah <amit.s...@redhat.com>

Michael, please add to the virtio queue.

Amit

Re: [PATCH] virtio-console: avoid DMA from stack

[Amit Shah]

On (Wed) 01 Feb 2017 [00:02:27], Omar Sandoval wrote:
> From: Omar Sandoval 
> 
> put_chars() stuffs the buffer it gets into an sg, but that buffer may be
> on the stack. This breaks with CONFIG_VMAP_STACK=y (for me, it
> manifested as printks getting turned into NUL bytes).

Seems reasonable.  I wonder since all implementations of hvc do a
memcpy, if we can abstract it - but that'll need some work.

Reviewed-by: Amit Shah 

Michael, please add to the virtio queue.

Amit

Re: [PATCH v2] virtio_console: fix a crash in config_work_handler

[Amit Shah]

On (Mon) 16 Jan 2017 [10:45:02], G. Campana wrote:
> Using control_work instead of config_work as the 3rd argument to
> container_of results in an invalid portdev pointer. Indeed, the work
> structure is initialized as below:
> 
> INIT_WORK(>config_work, _work_handler);
> 
> It leads to a crash when portdev->vdev is dereferenced later. This bug
> is triggered when the guest uses a virtio-console without multiport
> feature and receives a config_changed virtio interrupt.
> 
> Signed-off-by: G. Campana <gcamp...@quarkslab.com>

Reviewed-by: Amit Shah <amit.s...@redhat.com>

Michael, can you please pick this up?

Amit

Re: [PATCH v2] virtio_console: fix a crash in config_work_handler

[Amit Shah]

On (Mon) 16 Jan 2017 [10:45:02], G. Campana wrote:
> Using control_work instead of config_work as the 3rd argument to
> container_of results in an invalid portdev pointer. Indeed, the work
> structure is initialized as below:
> 
> INIT_WORK(>config_work, _work_handler);
> 
> It leads to a crash when portdev->vdev is dereferenced later. This bug
> is triggered when the guest uses a virtio-console without multiport
> feature and receives a config_changed virtio interrupt.
> 
> Signed-off-by: G. Campana 

Reviewed-by: Amit Shah 

Michael, can you please pick this up?

Amit

Re: [PATCH] virtio_console: fix a crash in config_work_handler

[Amit Shah]

On (Sat) 14 Jan 2017 [11:38:39], G. Campana wrote:
> Using control_work instead of config_work as the 3rd argument to
> container_of results in an invalid portdev pointer. Indeed, the work
> structure is initialized as below:
> 
> INIT_WORK(>config_work, _work_handler);
> 
> It leads to a crash when portdev->vdev is dereferenced later. This bug
> is triggered when the guest uses a virtio-console without multiport
> feature and receives a config_changed virtio interrupt.

Thanks, the patch is fine, but needs signed-off-by.

Please also post to virtualizat...@lists.linux-foundation.org


Amit

Re: [PATCH] virtio_console: fix a crash in config_work_handler

[Amit Shah]

On (Sat) 14 Jan 2017 [11:38:39], G. Campana wrote:
> Using control_work instead of config_work as the 3rd argument to
> container_of results in an invalid portdev pointer. Indeed, the work
> structure is initialized as below:
> 
> INIT_WORK(>config_work, _work_handler);
> 
> It leads to a crash when portdev->vdev is dereferenced later. This bug
> is triggered when the guest uses a virtio-console without multiport
> feature and receives a config_changed virtio interrupt.

Thanks, the patch is fine, but needs signed-off-by.

Please also post to virtualizat...@lists.linux-foundation.org


Amit

Re: [PATCH] virtio: console: Unlock vqs while freeing buffers

[Amit Shah]

On (Tue) 11 Oct 2016 [12:05:15], Matt Redfearn wrote:
> Commit c6017e793b93 ("virtio: console: add locks around buffer removal
> in port unplug path") added locking around the freeing of buffers in the
> vq. However, when free_buf() is called with can_sleep = true and rproc
> is enabled, it calls dma_free_coherent() directly, requiring interrupts
> to be enabled. Currently a WARNING is triggered due to the spin locking
> around free_buf, with a call stack like this:
> 
> WARNING: CPU: 3 PID: 121 at ./include/linux/dma-mapping.h:433
> free_buf+0x1a8/0x288
> Call Trace:
> [<8040c538>] show_stack+0x74/0xc0
> [<80757240>] dump_stack+0xd0/0x110
> [<80430d98>] __warn+0xfc/0x130
> [<80430ee0>] warn_slowpath_null+0x2c/0x3c
> [<807e7c6c>] free_buf+0x1a8/0x288
> [<807ea590>] remove_port_data+0x50/0xac
> [<807ea6a0>] unplug_port+0xb4/0x1bc
> [<807ea858>] virtcons_remove+0xb0/0xfc
> [<807b6734>] virtio_dev_remove+0x58/0xc0
> [<807f918c>] __device_release_driver+0xac/0x134
> [<807f924c>] device_release_driver+0x38/0x50
> [<807f7edc>] bus_remove_device+0xfc/0x130
> [<807f4b74>] device_del+0x17c/0x21c
> [<807f4c38>] device_unregister+0x24/0x38
> [<807b6b50>] unregister_virtio_device+0x28/0x44
> 
> Fix this by restructuring the loops to allow the locks to only be taken
> where it is necessary to protect the vqs, and release it while the
> buffer is being freed.
> 
> Fixes: c6017e793b93 ("virtio: console: add locks around buffer removal in 
> port unplug path")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Matt Redfearn <matt.redfe...@imgtec.com>

Reviewed-by: Amit Shah <amit.s...@redhat.com>

Michael, can you pick this up?

Thanks,

Amit

Re: [PATCH] virtio: console: Unlock vqs while freeing buffers

[Amit Shah]

On (Tue) 11 Oct 2016 [12:05:15], Matt Redfearn wrote:
> Commit c6017e793b93 ("virtio: console: add locks around buffer removal
> in port unplug path") added locking around the freeing of buffers in the
> vq. However, when free_buf() is called with can_sleep = true and rproc
> is enabled, it calls dma_free_coherent() directly, requiring interrupts
> to be enabled. Currently a WARNING is triggered due to the spin locking
> around free_buf, with a call stack like this:
> 
> WARNING: CPU: 3 PID: 121 at ./include/linux/dma-mapping.h:433
> free_buf+0x1a8/0x288
> Call Trace:
> [<8040c538>] show_stack+0x74/0xc0
> [<80757240>] dump_stack+0xd0/0x110
> [<80430d98>] __warn+0xfc/0x130
> [<80430ee0>] warn_slowpath_null+0x2c/0x3c
> [<807e7c6c>] free_buf+0x1a8/0x288
> [<807ea590>] remove_port_data+0x50/0xac
> [<807ea6a0>] unplug_port+0xb4/0x1bc
> [<807ea858>] virtcons_remove+0xb0/0xfc
> [<807b6734>] virtio_dev_remove+0x58/0xc0
> [<807f918c>] __device_release_driver+0xac/0x134
> [<807f924c>] device_release_driver+0x38/0x50
> [<807f7edc>] bus_remove_device+0xfc/0x130
> [<807f4b74>] device_del+0x17c/0x21c
> [<807f4c38>] device_unregister+0x24/0x38
> [<807b6b50>] unregister_virtio_device+0x28/0x44
> 
> Fix this by restructuring the loops to allow the locks to only be taken
> where it is necessary to protect the vqs, and release it while the
> buffer is being freed.
> 
> Fixes: c6017e793b93 ("virtio: console: add locks around buffer removal in 
> port unplug path")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Matt Redfearn 

Reviewed-by: Amit Shah 

Michael, can you pick this up?

Thanks,

Amit

Re: [PATCH 02/11] virtio_console: Less function calls in init_vqs() after error detection

[Amit Shah]

Hi,

On (Wed) 14 Sep 2016 [16:01:28], SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Wed, 14 Sep 2016 14:00:35 +0200
> 
> The kfree() function was called in up to five cases
> by the init_vqs() function during error handling even if
> the passed variable contained a null pointer.
> 
> * Return directly after a call of the function "kmalloc_array" failed
>   at the beginning.
> 
> * Split a condition check for memory allocation failures so that
>   each pointer from these function calls will be checked immediately.
> 
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html
> 
> * Adjust jump targets according to the Linux coding style convention.

So I've seen this series and I'm not yet sure how I feel about the
patches - f.e. in this one, it adds more lines than it removes to
achieve the same effect.  I think the code is currently more readable
than after these changes.  And even if kfree is called multiple times,
it isn't a huge bother -- it's error case anyway, very unlikely to
trigger, but keeps everything very readble.


Amit

Re: [PATCH 02/11] virtio_console: Less function calls in init_vqs() after error detection

[Amit Shah]

Hi,

On (Wed) 14 Sep 2016 [16:01:28], SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Wed, 14 Sep 2016 14:00:35 +0200
> 
> The kfree() function was called in up to five cases
> by the init_vqs() function during error handling even if
> the passed variable contained a null pointer.
> 
> * Return directly after a call of the function "kmalloc_array" failed
>   at the beginning.
> 
> * Split a condition check for memory allocation failures so that
>   each pointer from these function calls will be checked immediately.
> 
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html
> 
> * Adjust jump targets according to the Linux coding style convention.

So I've seen this series and I'm not yet sure how I feel about the
patches - f.e. in this one, it adds more lines than it removes to
achieve the same effect.  I think the code is currently more readable
than after these changes.  And even if kfree is called multiple times,
it isn't a huge bother -- it's error case anyway, very unlikely to
trigger, but keeps everything very readble.


Amit

Re: [PATCH] virtio_console: Stop doing DMA on the stack

[Amit Shah]

On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote:
> virtio_console uses a small DMA buffer for control requests.  Move
> that buffer into heap memory.
> 
> Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
> systems (which is currently most of them), but it breaks completely
> if the stack is virtually mapped.
> 
> Tested by typing both directions using picocom aimed at /dev/hvc0.
> 
> Signed-off-by: Andy Lutomirski <l...@kernel.org>

Looks fine,

Reviewed-by: Amit Shah <amit.s...@redhat.com>

> ---
> 
> Hi all-
> 
> This is currently broken in tip:x86/asm.  If you (Amit) like this patch,
> would it make sense for Ingo to add it to -tip?

Yes, I'm fine with that.


 Amit

Re: [PATCH] virtio_console: Stop doing DMA on the stack

[Amit Shah]

On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote:
> virtio_console uses a small DMA buffer for control requests.  Move
> that buffer into heap memory.
> 
> Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
> systems (which is currently most of them), but it breaks completely
> if the stack is virtually mapped.
> 
> Tested by typing both directions using picocom aimed at /dev/hvc0.
> 
> Signed-off-by: Andy Lutomirski 

Looks fine,

Reviewed-by: Amit Shah 

> ---
> 
> Hi all-
> 
> This is currently broken in tip:x86/asm.  If you (Amit) like this patch,
> would it make sense for Ingo to add it to -tip?

Yes, I'm fine with that.


 Amit

Re: [PATCH kernel 0/2] speed up live migration by skipping free pages

[Amit Shah]

On (Mon) 25 Apr 2016 [14:04:06], Michael S. Tsirkin wrote:
> On Mon, Apr 25, 2016 at 11:36:41AM +0530, Amit Shah wrote:
> > On (Tue) 19 Apr 2016 [22:34:32], Liang Li wrote:
> > > Current QEMU live migration implementation mark all guest's RAM pages
> > > as dirtied in the ram bulk stage, all these pages will be processed
> > > and it consumes quite a lot of CPU cycles and network bandwidth.
> > > 
> > > From guest's point of view, it doesn't care about the content in free
> > > page. We can make use of this fact and skip processing the free
> > > pages, this can save a lot CPU cycles and reduce the network traffic
> > > significantly while speed up the live migration process obviously.
> > > 
> > > This patch set is the kernel side implementation.
> > > 
> > > The virtio-balloon driver is extended to send the free page bitmap
> > > from guest to QEMU.
> > > 
> > > After getting the free page bitmap, QEMU can use it to filter out
> > > guest's free pages. This make the live migration process much more
> > > efficient.
> > > 
> > > In order to skip more free pages, we add an interface to let the user
> > > decide whether dropping the cache in guest during live migration.
> > 
> > So if virtio-balloon is the way to go (i.e. speed is acceptable), I
> > just have one point then.  My main concern with using (or not using)
> > virtio-balloon was that a guest admin is going to disable the
> > virtio-balloon driver entirely because the admin won't want the guest
> > to give away pages to the host, esp. when the guest is to be a
> > high-performant one.
> 
> The result will be the reverse of high-performance.
> 
> If you don't want to inflate a balloon, don't.
> 
> If you do but guest doesn't respond to inflate requests,
> it's quite reasonable for host to kill it -
> there is no way to distinguish between that and
> guest being malicious.

With the new command I'm suggesting, the guest will let the host know
that it has enabled this option, and it won't free up any RAM for the
host.

Also, just because a guest doesn't release some memory (which the
guest owns anyway) doesn't make it malicious, and killing such guests
is never going to end well for that hosting provider.

> I don't know of management tools doing that but
> it's rather reasonable. What does happen is
> some random guest memory is pushed it out to swap,
> which is likely much worse than dropping unused memory
> by moving it into the balloon.

Even if the host (admin) gave a guarantee that there won't be any
ballooning activity involved that will slow down the guest, a guest
admin can be paranoid enough to disable ballooning.  If, however, this
is made known to the host, it's likely a win-win situation because the
host knows the guest needs its RAM, and the guest can still use the
driver to send stats which the host can use during migration for
speedups.


Amit

Re: [PATCH kernel 0/2] speed up live migration by skipping free pages

[Amit Shah]

On (Mon) 25 Apr 2016 [14:04:06], Michael S. Tsirkin wrote:
> On Mon, Apr 25, 2016 at 11:36:41AM +0530, Amit Shah wrote:
> > On (Tue) 19 Apr 2016 [22:34:32], Liang Li wrote:
> > > Current QEMU live migration implementation mark all guest's RAM pages
> > > as dirtied in the ram bulk stage, all these pages will be processed
> > > and it consumes quite a lot of CPU cycles and network bandwidth.
> > > 
> > > From guest's point of view, it doesn't care about the content in free
> > > page. We can make use of this fact and skip processing the free
> > > pages, this can save a lot CPU cycles and reduce the network traffic
> > > significantly while speed up the live migration process obviously.
> > > 
> > > This patch set is the kernel side implementation.
> > > 
> > > The virtio-balloon driver is extended to send the free page bitmap
> > > from guest to QEMU.
> > > 
> > > After getting the free page bitmap, QEMU can use it to filter out
> > > guest's free pages. This make the live migration process much more
> > > efficient.
> > > 
> > > In order to skip more free pages, we add an interface to let the user
> > > decide whether dropping the cache in guest during live migration.
> > 
> > So if virtio-balloon is the way to go (i.e. speed is acceptable), I
> > just have one point then.  My main concern with using (or not using)
> > virtio-balloon was that a guest admin is going to disable the
> > virtio-balloon driver entirely because the admin won't want the guest
> > to give away pages to the host, esp. when the guest is to be a
> > high-performant one.
> 
> The result will be the reverse of high-performance.
> 
> If you don't want to inflate a balloon, don't.
> 
> If you do but guest doesn't respond to inflate requests,
> it's quite reasonable for host to kill it -
> there is no way to distinguish between that and
> guest being malicious.

With the new command I'm suggesting, the guest will let the host know
that it has enabled this option, and it won't free up any RAM for the
host.

Also, just because a guest doesn't release some memory (which the
guest owns anyway) doesn't make it malicious, and killing such guests
is never going to end well for that hosting provider.

> I don't know of management tools doing that but
> it's rather reasonable. What does happen is
> some random guest memory is pushed it out to swap,
> which is likely much worse than dropping unused memory
> by moving it into the balloon.

Even if the host (admin) gave a guarantee that there won't be any
ballooning activity involved that will slow down the guest, a guest
admin can be paranoid enough to disable ballooning.  If, however, this
is made known to the host, it's likely a win-win situation because the
host knows the guest needs its RAM, and the guest can still use the
driver to send stats which the host can use during migration for
speedups.


Amit

Re: [PATCH kernel 0/2] speed up live migration by skipping free pages

[Amit Shah]

On (Tue) 19 Apr 2016 [22:34:32], Liang Li wrote:
> Current QEMU live migration implementation mark all guest's RAM pages
> as dirtied in the ram bulk stage, all these pages will be processed
> and it consumes quite a lot of CPU cycles and network bandwidth.
> 
> From guest's point of view, it doesn't care about the content in free
> page. We can make use of this fact and skip processing the free
> pages, this can save a lot CPU cycles and reduce the network traffic
> significantly while speed up the live migration process obviously.
> 
> This patch set is the kernel side implementation.
> 
> The virtio-balloon driver is extended to send the free page bitmap
> from guest to QEMU.
> 
> After getting the free page bitmap, QEMU can use it to filter out
> guest's free pages. This make the live migration process much more
> efficient.
> 
> In order to skip more free pages, we add an interface to let the user
> decide whether dropping the cache in guest during live migration.

So if virtio-balloon is the way to go (i.e. speed is acceptable), I
just have one point then.  My main concern with using (or not using)
virtio-balloon was that a guest admin is going to disable the
virtio-balloon driver entirely because the admin won't want the guest
to give away pages to the host, esp. when the guest is to be a
high-performant one.

In this case, if a new command can be added to the balloon spec where
a guest driver indicates it's not going to participate in ballooning
activity (ie a guest will ignore any ballooning requests from the
host), but use the driver just for stats-sharing purposes, that can be
a workable solution here as well.  In that case, we can keep the
MM-related stuff inside the balloon driver, and also get the benefit
of the guest having control over how it uses its memory,
disincentivising guest admins from disabling the balloon entirely (it
will also benefit the guest to keep this driver loaded in such a
state, if migration is faster!).

Amit

Re: [PATCH kernel 0/2] speed up live migration by skipping free pages

[Amit Shah]

On (Tue) 19 Apr 2016 [22:34:32], Liang Li wrote:
> Current QEMU live migration implementation mark all guest's RAM pages
> as dirtied in the ram bulk stage, all these pages will be processed
> and it consumes quite a lot of CPU cycles and network bandwidth.
> 
> From guest's point of view, it doesn't care about the content in free
> page. We can make use of this fact and skip processing the free
> pages, this can save a lot CPU cycles and reduce the network traffic
> significantly while speed up the live migration process obviously.
> 
> This patch set is the kernel side implementation.
> 
> The virtio-balloon driver is extended to send the free page bitmap
> from guest to QEMU.
> 
> After getting the free page bitmap, QEMU can use it to filter out
> guest's free pages. This make the live migration process much more
> efficient.
> 
> In order to skip more free pages, we add an interface to let the user
> decide whether dropping the cache in guest during live migration.

So if virtio-balloon is the way to go (i.e. speed is acceptable), I
just have one point then.  My main concern with using (or not using)
virtio-balloon was that a guest admin is going to disable the
virtio-balloon driver entirely because the admin won't want the guest
to give away pages to the host, esp. when the guest is to be a
high-performant one.

In this case, if a new command can be added to the balloon spec where
a guest driver indicates it's not going to participate in ballooning
activity (ie a guest will ignore any ballooning requests from the
host), but use the driver just for stats-sharing purposes, that can be
a workable solution here as well.  In that case, we can keep the
MM-related stuff inside the balloon driver, and also get the benefit
of the guest having control over how it uses its memory,
disincentivising guest admins from disabling the balloon entirely (it
will also benefit the guest to keep this driver loaded in such a
state, if migration is faster!).

Amit

Re: [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Thu) 10 Mar 2016 [07:44:19], Li, Liang Z wrote:
> 
> Hi Amit,
> 
>  Could provide more information on how to use virtio-serial to exchange data? 
>  Thread , Wiki or code are all OK. 
>  I have not find some useful information yet.

See this commit in the Linux sources:

108fc82596e3b66b819df9d28c1ebbc9ab5de14c

that adds a way to send guest trace data over to the host.  I think
that's the most relevant to your use-case.  However, you'll have to
add an in-kernel user of virtio-serial (like the virtio-console code
-- the code that deals with tty and hvc currently).  There's no other
non-tty user right now, and this is the right kind of use-case to add
one for!

For many other (userspace) use-cases, see the qemu-guest-agent in the
qemu sources.

The API is documented in the wiki:

http://www.linux-kvm.org/page/Virtio-serial_API

and the feature pages have some information that may help as well:

https://fedoraproject.org/wiki/Features/VirtioSerial

There are some links in here too:

http://log.amitshah.net/2010/09/communication-between-guests-and-hosts/

Hope this helps.


Amit

Re: [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Thu) 10 Mar 2016 [07:44:19], Li, Liang Z wrote:
> 
> Hi Amit,
> 
>  Could provide more information on how to use virtio-serial to exchange data? 
>  Thread , Wiki or code are all OK. 
>  I have not find some useful information yet.

See this commit in the Linux sources:

108fc82596e3b66b819df9d28c1ebbc9ab5de14c

that adds a way to send guest trace data over to the host.  I think
that's the most relevant to your use-case.  However, you'll have to
add an in-kernel user of virtio-serial (like the virtio-console code
-- the code that deals with tty and hvc currently).  There's no other
non-tty user right now, and this is the right kind of use-case to add
one for!

For many other (userspace) use-cases, see the qemu-guest-agent in the
qemu sources.

The API is documented in the wiki:

http://www.linux-kvm.org/page/Virtio-serial_API

and the feature pages have some information that may help as well:

https://fedoraproject.org/wiki/Features/VirtioSerial

There are some links in here too:

http://log.amitshah.net/2010/09/communication-between-guests-and-hosts/

Hope this helps.


Amit

Re: [Qemu-devel] [RFC kernel 0/2]A PV solution for KVM live migration optimization

[Amit Shah]

On (Thu) 10 Mar 2016 [12:31:32], Jitendra Kolhe wrote:
> On 3/8/2016 4:44 PM, Amit Shah wrote:
> >>>> Hi,
> >>>>   An interesting solution; I know a few different people have been 
> >>>> looking at
> >>>> how to speed up ballooned VM migration.
> >>>>
> >>>
> >>> Ooh, different solutions for the same purpose, and both based on the 
> >>> balloon.
> >>
> >> We were also tying to address similar problem, without actually needing to 
> >> modify
> >> the guest driver. Please find patch details under mail with subject.
> >> migration: skip sending ram pages released by virtio-balloon driver
> >
> > The scope of this patch series seems to be wider: don't send free
> > pages to a dest at all, vs. don't send pages that are ballooned out.
> 
> Hi,
> 
> Thanks for your response. The scope of this patch series doesn’t seem to take 
> care 
> of ballooned out pages. To balloon out a guest ram page the guest balloon 
> driver does 
> a alloc_page() and then return the guest pfn to Qemu, so ballooned out pages 
> will not 
> be seen as free ram pages by the guest.
> Thus we will still end up scanning (for zero page) for ballooned out pages 
> during 
> migration. It would be ideal if we could have both solutions.

Yes, of course it would be nice to have both solutions.  My response was to the 
line:

> >>> Ooh, different solutions for the same purpose, and both based on the 
> >>> balloon.

which sounded misleading to me for a couple of reasons: 1, as you
describe, pages being considered by this patchset and yours are
different; and 2, as I mentioned in the other mail, this patchset
doesn't really depend on the balloon, and I believe it should not.


Amit

Re: [Qemu-devel] [RFC kernel 0/2]A PV solution for KVM live migration optimization

[Amit Shah]

On (Thu) 10 Mar 2016 [12:31:32], Jitendra Kolhe wrote:
> On 3/8/2016 4:44 PM, Amit Shah wrote:
> >>>> Hi,
> >>>>   An interesting solution; I know a few different people have been 
> >>>> looking at
> >>>> how to speed up ballooned VM migration.
> >>>>
> >>>
> >>> Ooh, different solutions for the same purpose, and both based on the 
> >>> balloon.
> >>
> >> We were also tying to address similar problem, without actually needing to 
> >> modify
> >> the guest driver. Please find patch details under mail with subject.
> >> migration: skip sending ram pages released by virtio-balloon driver
> >
> > The scope of this patch series seems to be wider: don't send free
> > pages to a dest at all, vs. don't send pages that are ballooned out.
> 
> Hi,
> 
> Thanks for your response. The scope of this patch series doesn’t seem to take 
> care 
> of ballooned out pages. To balloon out a guest ram page the guest balloon 
> driver does 
> a alloc_page() and then return the guest pfn to Qemu, so ballooned out pages 
> will not 
> be seen as free ram pages by the guest.
> Thus we will still end up scanning (for zero page) for ballooned out pages 
> during 
> migration. It would be ideal if we could have both solutions.

Yes, of course it would be nice to have both solutions.  My response was to the 
line:

> >>> Ooh, different solutions for the same purpose, and both based on the 
> >>> balloon.

which sounded misleading to me for a couple of reasons: 1, as you
describe, pages being considered by this patchset and yours are
different; and 2, as I mentioned in the other mail, this patchset
doesn't really depend on the balloon, and I believe it should not.


Amit

Re: [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Thu) 03 Mar 2016 [18:44:24], Liang Li wrote:
> The current QEMU live migration implementation mark the all the
> guest's RAM pages as dirtied in the ram bulk stage, all these pages
> will be processed and that takes quit a lot of CPU cycles.
> 
> From guest's point of view, it doesn't care about the content in free
> pages. We can make use of this fact and skip processing the free
> pages in the ram bulk stage, it can save a lot CPU cycles and reduce
> the network traffic significantly while speed up the live migration
> process obviously.
> 
> This patch set is the QEMU side implementation.
> 
> The virtio-balloon is extended so that QEMU can get the free pages
> information from the guest through virtio.
> 
> After getting the free pages information (a bitmap), QEMU can use it
> to filter out the guest's free pages in the ram bulk stage. This make
> the live migration process much more efficient.
> 
> This RFC version doesn't take the post-copy and RDMA into
> consideration, maybe both of them can benefit from this PV solution
> by with some extra modifications.

I like the idea, just have to prove (review) and test it a lot to
ensure we don't end up skipping pages that matter.

However, there are a couple of points:

In my opinion, the information that's exchanged between the guest and
the host should be exchanged over a virtio-serial channel rather than
virtio-balloon.  First, there's nothing related to the balloon here.
It just happens to be memory info.  Second, I would never enable
balloon in a guest that I want to be performance-sensitive.  So even
if you add this as part of balloon, you'll find no one is using this
solution.

Secondly, I suggest virtio-serial, because it's meant exactly to
exchange free-flowing information between a host and a guest, and you
don't need to extend any part of the protocol for it (hence no changes
necessary to the spec).  You can see how spice, vnc, etc., use
virtio-serial to exchange data.


Amit

Re: [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Thu) 03 Mar 2016 [18:44:24], Liang Li wrote:
> The current QEMU live migration implementation mark the all the
> guest's RAM pages as dirtied in the ram bulk stage, all these pages
> will be processed and that takes quit a lot of CPU cycles.
> 
> From guest's point of view, it doesn't care about the content in free
> pages. We can make use of this fact and skip processing the free
> pages in the ram bulk stage, it can save a lot CPU cycles and reduce
> the network traffic significantly while speed up the live migration
> process obviously.
> 
> This patch set is the QEMU side implementation.
> 
> The virtio-balloon is extended so that QEMU can get the free pages
> information from the guest through virtio.
> 
> After getting the free pages information (a bitmap), QEMU can use it
> to filter out the guest's free pages in the ram bulk stage. This make
> the live migration process much more efficient.
> 
> This RFC version doesn't take the post-copy and RDMA into
> consideration, maybe both of them can benefit from this PV solution
> by with some extra modifications.

I like the idea, just have to prove (review) and test it a lot to
ensure we don't end up skipping pages that matter.

However, there are a couple of points:

In my opinion, the information that's exchanged between the guest and
the host should be exchanged over a virtio-serial channel rather than
virtio-balloon.  First, there's nothing related to the balloon here.
It just happens to be memory info.  Second, I would never enable
balloon in a guest that I want to be performance-sensitive.  So even
if you add this as part of balloon, you'll find no one is using this
solution.

Secondly, I suggest virtio-serial, because it's meant exactly to
exchange free-flowing information between a host and a guest, and you
don't need to extend any part of the protocol for it (hence no changes
necessary to the spec).  You can see how spice, vnc, etc., use
virtio-serial to exchange data.


Amit

Re: [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Fri) 04 Mar 2016 [15:02:47], Jitendra Kolhe wrote:
> > >
> > > * Liang Li (liang.z...@intel.com) wrote:
> > > > The current QEMU live migration implementation mark the all the
> > > > guest's RAM pages as dirtied in the ram bulk stage, all these pages
> > > > will be processed and that takes quit a lot of CPU cycles.
> > > >
> > > > From guest's point of view, it doesn't care about the content in free
> > > > pages. We can make use of this fact and skip processing the free pages
> > > > in the ram bulk stage, it can save a lot CPU cycles and reduce the
> > > > network traffic significantly while speed up the live migration
> > > > process obviously.
> > > >
> > > > This patch set is the QEMU side implementation.
> > > >
> > > > The virtio-balloon is extended so that QEMU can get the free pages
> > > > information from the guest through virtio.
> > > >
> > > > After getting the free pages information (a bitmap), QEMU can use it
> > > > to filter out the guest's free pages in the ram bulk stage. This make
> > > > the live migration process much more efficient.
> > >
> > > Hi,
> > >   An interesting solution; I know a few different people have been 
> > > looking at
> > > how to speed up ballooned VM migration.
> > >
> >
> > Ooh, different solutions for the same purpose, and both based on the 
> > balloon.
> 
> We were also tying to address similar problem, without actually needing to 
> modify
> the guest driver. Please find patch details under mail with subject.
> migration: skip sending ram pages released by virtio-balloon driver

The scope of this patch series seems to be wider: don't send free
pages to a dest at all, vs. don't send pages that are ballooned out.

Amit

Re: [RFC qemu 0/4] A PV solution for live migration optimization

[Amit Shah]

On (Fri) 04 Mar 2016 [15:02:47], Jitendra Kolhe wrote:
> > >
> > > * Liang Li (liang.z...@intel.com) wrote:
> > > > The current QEMU live migration implementation mark the all the
> > > > guest's RAM pages as dirtied in the ram bulk stage, all these pages
> > > > will be processed and that takes quit a lot of CPU cycles.
> > > >
> > > > From guest's point of view, it doesn't care about the content in free
> > > > pages. We can make use of this fact and skip processing the free pages
> > > > in the ram bulk stage, it can save a lot CPU cycles and reduce the
> > > > network traffic significantly while speed up the live migration
> > > > process obviously.
> > > >
> > > > This patch set is the QEMU side implementation.
> > > >
> > > > The virtio-balloon is extended so that QEMU can get the free pages
> > > > information from the guest through virtio.
> > > >
> > > > After getting the free pages information (a bitmap), QEMU can use it
> > > > to filter out the guest's free pages in the ram bulk stage. This make
> > > > the live migration process much more efficient.
> > >
> > > Hi,
> > >   An interesting solution; I know a few different people have been 
> > > looking at
> > > how to speed up ballooned VM migration.
> > >
> >
> > Ooh, different solutions for the same purpose, and both based on the 
> > balloon.
> 
> We were also tying to address similar problem, without actually needing to 
> modify
> the guest driver. Please find patch details under mail with subject.
> migration: skip sending ram pages released by virtio-balloon driver

The scope of this patch series seems to be wider: don't send free
pages to a dest at all, vs. don't send pages that are ballooned out.

Amit

Re: [PATCH 12/25] virtio_console: Use bool function return values of true/false not 1/0

[Amit Shah]

On (Mon) 30 Mar 2015 [16:46:10], Joe Perches wrote:
> Use the normal return values for bool functions
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/char/virtio_console.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 72d7028..50754d20 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device 
> *portdev)
>* early_init
>*/
>   if (!portdev->vdev)
> - return 0;
> + return false;

Reviewed-by: Amit Shah 

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 12/25] virtio_console: Use bool function return values of true/false not 1/0

[Amit Shah]

On (Mon) 30 Mar 2015 [16:46:10], Joe Perches wrote:
 Use the normal return values for bool functions
 
 Signed-off-by: Joe Perches j...@perches.com
 ---
  drivers/char/virtio_console.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
 index 72d7028..50754d20 100644
 --- a/drivers/char/virtio_console.c
 +++ b/drivers/char/virtio_console.c
 @@ -355,7 +355,7 @@ static inline bool use_multiport(struct ports_device 
 *portdev)
* early_init
*/
   if (!portdev-vdev)
 - return 0;
 + return false;

Reviewed-by: Amit Shah amit.s...@redhat.com

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] virtio_console: avoid config access from irq

[Amit Shah]

On (Sat) 28 Feb 2015 [18:42:25], Michael S. Tsirkin wrote:
> when multiport is off, virtio console invokes config access from irq
> context, config access is blocking on s390.
> Fix this up by scheduling work from config irq - similar to what we do
> for multiport configs.
> 
> Signed-off-by: Michael S. Tsirkin 

Reviewed-by: Amit Shah 

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] virtio_console: init work unconditionally

[Amit Shah]

On (Sat) 28 Feb 2015 [18:41:34], Michael S. Tsirkin wrote:
> when multiport is off, we don't initialize config work,
> but we then cancel uninitialized control_work on freeze.
> 
> Signed-off-by: Michael S. Tsirkin 

Reviewed-by: Amit Shah 

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] virtio_console: init work unconditionally

[Amit Shah]

On (Sat) 28 Feb 2015 [18:41:34], Michael S. Tsirkin wrote:
 when multiport is off, we don't initialize config work,
 but we then cancel uninitialized control_work on freeze.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com

Reviewed-by: Amit Shah amit.s...@redhat.com

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] virtio_console: avoid config access from irq

[Amit Shah]

On (Sat) 28 Feb 2015 [18:42:25], Michael S. Tsirkin wrote:
 when multiport is off, virtio console invokes config access from irq
 context, config access is blocking on s390.
 Fix this up by scheduling work from config irq - similar to what we do
 for multiport configs.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com

Reviewed-by: Amit Shah amit.s...@redhat.com

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 5/5] virtio: don't require a config space on the console device.

[Amit Shah]

On (Mon) 09 Feb 2015 [10:26:29], Rusty Russell wrote:
> virtio: don't require a config space on the console device.
> 
> Strictly, it's only needed when we have features (size or multiport).
> 
> Signed-off-by: Rusty Russell 

Reviewed-by: Amit Shah 

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 26afb56a8073..fae2dbbf5745 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1986,7 +1986,10 @@ static int virtcons_probe(struct virtio_device *vdev)
>   bool multiport;
>   bool early = early_put_chars != NULL;
>  
> - if (!vdev->config->get) {
> + /* We only need a config space if features are offered */
> + if (!vdev->config->get &&
> + (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
> +  || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) {
>   dev_err(>dev, "%s failure: config access disabled\n",
>   __func__);
>   return -EINVAL;

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 5/5] virtio: don't require a config space on the console device.

[Amit Shah]

On (Mon) 09 Feb 2015 [10:26:29], Rusty Russell wrote:
 virtio: don't require a config space on the console device.
 
 Strictly, it's only needed when we have features (size or multiport).
 
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

Reviewed-by: Amit Shah amit.s...@redhat.com

 diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
 index 26afb56a8073..fae2dbbf5745 100644
 --- a/drivers/char/virtio_console.c
 +++ b/drivers/char/virtio_console.c
 @@ -1986,7 +1986,10 @@ static int virtcons_probe(struct virtio_device *vdev)
   bool multiport;
   bool early = early_put_chars != NULL;
  
 - if (!vdev-config-get) {
 + /* We only need a config space if features are offered */
 + if (!vdev-config-get 
 + (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)
 +  || virtio_has_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT))) {
   dev_err(vdev-dev, %s failure: config access disabled\n,
   __func__);
   return -EINVAL;

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v3 04/16] virtio/console: verify device has config space

[Amit Shah]

On (Tue) 20 Jan 2015 [13:09:55], Michael S. Tsirkin wrote:
> On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah wrote:
> > On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
> > > Some devices might not implement config space access
> > > (e.g. remoteproc used not to - before 3.9).
> > > virtio/console needs config space access so make it
> > > fail gracefully if not there.
> > 
> > Do we know any such devices?  Wondering what prompted this patch.  If
> > it's just theoretical, I'd rather let it be like this, and pull this
> > in when there's a device that doesn't have config space.
> 
> Yes, with virtio 1.0 config space can be in a separate BAR now.  If
> that's not enabled by BIOS (e.g. out of space), we won't have config
> space.

I'm still not sure whether we should pull in this patch before
actually seeing a failure.

You do have a dev_err which tells why the probe failed, so it's an
acceptable compromise I suppose.

> > Also, just the console functionality (i.e. F_MULTIPORT is unset) is
> > available w/o config space access.
> 
> Supporting this by gracefully disabling F_MULTIPORT
> would require getting this info from driver before
> features are finalized.
> Alternatively, check F_MULTIPORT and only fail if set?
> Let me know, I'll cook up a patch.

Yes, failing only if F_MULTIPORT is set is a better option (if we have
to fail).

> > In fact, getting this patch in
> > would mean remoteproc wouldn't even run in its pre-config days...
> 
> It seems to have get callback unconditionally now - or did I miss
> something?

What I meant was remoteproc doesn't depend on the config space, only
uses the console functionality.  If remoteproc devices didn't expose a
config space, this patch would cause it to lose its console
functionality for no apparent reason.


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v3 04/16] virtio/console: verify device has config space

[Amit Shah]

On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
> Some devices might not implement config space access
> (e.g. remoteproc used not to - before 3.9).
> virtio/console needs config space access so make it
> fail gracefully if not there.

Do we know any such devices?  Wondering what prompted this patch.  If
it's just theoretical, I'd rather let it be like this, and pull this
in when there's a device that doesn't have config space.

Also, just the console functionality (i.e. F_MULTIPORT is unset) is
available w/o config space access.  In fact, getting this patch in
would mean remoteproc wouldn't even run in its pre-config days...

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index de03df9..26afb56 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device *vdev)
>   bool multiport;
>   bool early = early_put_chars != NULL;
>  
> + if (!vdev->config->get) {
> + dev_err(>dev, "%s failure: config access disabled\n",
> + __func__);
> + return -EINVAL;
> + }
> +


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v3 04/16] virtio/console: verify device has config space

[Amit Shah]

On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
 Some devices might not implement config space access
 (e.g. remoteproc used not to - before 3.9).
 virtio/console needs config space access so make it
 fail gracefully if not there.

Do we know any such devices?  Wondering what prompted this patch.  If
it's just theoretical, I'd rather let it be like this, and pull this
in when there's a device that doesn't have config space.

Also, just the console functionality (i.e. F_MULTIPORT is unset) is
available w/o config space access.  In fact, getting this patch in
would mean remoteproc wouldn't even run in its pre-config days...

 diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
 index de03df9..26afb56 100644
 --- a/drivers/char/virtio_console.c
 +++ b/drivers/char/virtio_console.c
 @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device *vdev)
   bool multiport;
   bool early = early_put_chars != NULL;
  
 + if (!vdev-config-get) {
 + dev_err(vdev-dev, %s failure: config access disabled\n,
 + __func__);
 + return -EINVAL;
 + }
 +


Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v3 04/16] virtio/console: verify device has config space

[Amit Shah]

On (Tue) 20 Jan 2015 [13:09:55], Michael S. Tsirkin wrote:
 On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah wrote:
  On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
   Some devices might not implement config space access
   (e.g. remoteproc used not to - before 3.9).
   virtio/console needs config space access so make it
   fail gracefully if not there.
  
  Do we know any such devices?  Wondering what prompted this patch.  If
  it's just theoretical, I'd rather let it be like this, and pull this
  in when there's a device that doesn't have config space.
 
 Yes, with virtio 1.0 config space can be in a separate BAR now.  If
 that's not enabled by BIOS (e.g. out of space), we won't have config
 space.

I'm still not sure whether we should pull in this patch before
actually seeing a failure.

You do have a dev_err which tells why the probe failed, so it's an
acceptable compromise I suppose.

  Also, just the console functionality (i.e. F_MULTIPORT is unset) is
  available w/o config space access.
 
 Supporting this by gracefully disabling F_MULTIPORT
 would require getting this info from driver before
 features are finalized.
 Alternatively, check F_MULTIPORT and only fail if set?
 Let me know, I'll cook up a patch.

Yes, failing only if F_MULTIPORT is set is a better option (if we have
to fail).

  In fact, getting this patch in
  would mean remoteproc wouldn't even run in its pre-config days...
 
 It seems to have get callback unconditionally now - or did I miss
 something?

What I meant was remoteproc doesn't depend on the config space, only
uses the console functionality.  If remoteproc devices didn't expose a
config space, this patch would cause it to lose its console
functionality for no apparent reason.


Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v2 01/15] virtio_pci: fix virtio spec compliance on restore

[Amit Shah]

On (Tue) 07 Oct 2014 [15:53:55], Michael S. Tsirkin wrote:
> On Mon, Oct 06, 2014 at 06:10:40PM +0300, Michael S. Tsirkin wrote:
> > On restore, virtio pci does the following:
> > + set features
> > + init vqs etc - device can be used at this point!
> > + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
> > 
> > This is in violation of the virtio spec, which
> > requires the following order:
> > - ACKNOWLEDGE
> > - DRIVER
> > - init vqs
> > - DRIVER_OK
> > 
> > This behaviour will break with hypervisors that assume spec compliant
> > behaviour.  It seems like a good idea to have this patch applied to
> > stable branches to reduce the support butden for the hypervisors.
> 
> Tested suspend to ram with virtio net and blk.

I'd recommend running a continuous ping for virtio-net and a dd
process running across s3/s4 for testing.

That's what I had run while doing the original set of patches.

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v2 01/15] virtio_pci: fix virtio spec compliance on restore

[Amit Shah]

On (Mon) 06 Oct 2014 [18:10:40], Michael S. Tsirkin wrote:
> On restore, virtio pci does the following:
> + set features
> + init vqs etc - device can be used at this point!
> + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
> 
> This is in violation of the virtio spec, which
> requires the following order:
> - ACKNOWLEDGE
> - DRIVER
> - init vqs
> - DRIVER_OK
> 
> This behaviour will break with hypervisors that assume spec compliant
> behaviour.  It seems like a good idea to have this patch applied to
> stable branches to reduce the support butden for the hypervisors.
> 
> Cc: sta...@vger.kernel.org
> Cc: Amit Shah 
> Signed-off-by: Michael S. Tsirkin 

I didn't see my previous questions answered from the initial posting
-- can you please respond to them?

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 v2 01/15] virtio_pci: fix virtio spec compliance on restore

[Amit Shah]

On (Mon) 06 Oct 2014 [18:10:40], Michael S. Tsirkin wrote:
 On restore, virtio pci does the following:
 + set features
 + init vqs etc - device can be used at this point!
 + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
 
 This is in violation of the virtio spec, which
 requires the following order:
 - ACKNOWLEDGE
 - DRIVER
 - init vqs
 - DRIVER_OK
 
 This behaviour will break with hypervisors that assume spec compliant
 behaviour.  It seems like a good idea to have this patch applied to
 stable branches to reduce the support butden for the hypervisors.
 
 Cc: sta...@vger.kernel.org
 Cc: Amit Shah amit.s...@redhat.com
 Signed-off-by: Michael S. Tsirkin m...@redhat.com

I didn't see my previous questions answered from the initial posting
-- can you please respond to them?

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 v2 01/15] virtio_pci: fix virtio spec compliance on restore

[Amit Shah]

On (Tue) 07 Oct 2014 [15:53:55], Michael S. Tsirkin wrote:
 On Mon, Oct 06, 2014 at 06:10:40PM +0300, Michael S. Tsirkin wrote:
  On restore, virtio pci does the following:
  + set features
  + init vqs etc - device can be used at this point!
  + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
  
  This is in violation of the virtio spec, which
  requires the following order:
  - ACKNOWLEDGE
  - DRIVER
  - init vqs
  - DRIVER_OK
  
  This behaviour will break with hypervisors that assume spec compliant
  behaviour.  It seems like a good idea to have this patch applied to
  stable branches to reduce the support butden for the hypervisors.
 
 Tested suspend to ram with virtio net and blk.

I'd recommend running a continuous ping for virtio-net and a dd
process running across s3/s4 for testing.

That's what I had run while doing the original set of patches.

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 RFC] virtio_pci: fix virtio spec compliance on restore

[Amit Shah]

On (Tue) 23 Sep 2014 [13:32:22], Michael S. Tsirkin wrote:
> On restore, virtio pci does the following:
> + set features
> + init vqs etc - device can be used at this point!
> + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
> 
> This is in violation of the virtio spec, which
> requires the following order:
> - ACKNOWLEDGE
> - DRIVER
> - init vqs
> - DRIVER_OK
> 
> Cc: sta...@vger.kernel.org
> Cc: Amit Shah 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> Lightly tested.
> Will repost as non-RFC once testing is done, sending
> out now for early flames/comments.

What tests are you running?

> Thanks!
> 
>  drivers/virtio/virtio_pci.c | 36 
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 58f7e45..58cbf6e 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -785,6 +785,7 @@ 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;
> + unsigned status = 0;
>   int ret;
>  
>   drv = container_of(vp_dev->vdev.dev.driver,
> @@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
>   return ret;
>  
>   pci_set_master(pci_dev);
> + /* We always start by resetting the device, in case a previous
> +  * driver messed it up. */
> + vp_reset(_dev->vdev);
> +
> + /* Acknowledge that we've seen the device. */
> + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
> + vp_set_status(_dev->vdev, status);
> +
> + /* Maybe driver failed before freeze.
> +  * Restore the failed status, for debugging. */
> + status |= vp_dev->saved_status & VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(_dev->vdev, status);
> +
> + if (!drv)
> + return 0;
> +
> + /* We have a driver! */
> + status |= VIRTIO_CONFIG_S_DRIVER;
> + vp_set_status(_dev->vdev, status);
> +
>   vp_finalize_features(_dev->vdev);
>  
> - if (drv && drv->restore)
> - ret = drv->restore(_dev->vdev);
> + if (!drv->restore)
> + return 0;

So in this case DRIVER_OK will never be set?

> +
> + ret = drv->restore(_dev->vdev);
> + if (ret) {
> + status |= VIRTIO_CONFIG_S_FAILED;
> + vp_set_status(_dev->vdev, status);
> + return ret;
> + }
>  
>   /* Finally, tell the device we're all set */
> - if (!ret)
> - vp_set_status(_dev->vdev, vp_dev->saved_status);
> + status |= VIRTIO_CONFIG_S_DRIVER_OK;
> + vp_set_status(_dev->vdev, status);
>  
>   return ret;
>  }

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 RFC] virtio_pci: fix virtio spec compliance on restore

[Amit Shah]

On (Tue) 23 Sep 2014 [13:32:22], Michael S. Tsirkin wrote:
 On restore, virtio pci does the following:
 + set features
 + init vqs etc - device can be used at this point!
 + set ACKNOWLEDGE,DRIVER and DRIVER_OK status bits
 
 This is in violation of the virtio spec, which
 requires the following order:
 - ACKNOWLEDGE
 - DRIVER
 - init vqs
 - DRIVER_OK
 
 Cc: sta...@vger.kernel.org
 Cc: Amit Shah amit.s...@redhat.com
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
 
 Lightly tested.
 Will repost as non-RFC once testing is done, sending
 out now for early flames/comments.

What tests are you running?

 Thanks!
 
  drivers/virtio/virtio_pci.c | 36 
  1 file changed, 32 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
 index 58f7e45..58cbf6e 100644
 --- a/drivers/virtio/virtio_pci.c
 +++ b/drivers/virtio/virtio_pci.c
 @@ -785,6 +785,7 @@ 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;
 + unsigned status = 0;
   int ret;
  
   drv = container_of(vp_dev-vdev.dev.driver,
 @@ -795,14 +796,41 @@ static int virtio_pci_restore(struct device *dev)
   return ret;
  
   pci_set_master(pci_dev);
 + /* We always start by resetting the device, in case a previous
 +  * driver messed it up. */
 + vp_reset(vp_dev-vdev);
 +
 + /* Acknowledge that we've seen the device. */
 + status |= VIRTIO_CONFIG_S_ACKNOWLEDGE;
 + vp_set_status(vp_dev-vdev, status);
 +
 + /* Maybe driver failed before freeze.
 +  * Restore the failed status, for debugging. */
 + status |= vp_dev-saved_status  VIRTIO_CONFIG_S_FAILED;
 + vp_set_status(vp_dev-vdev, status);
 +
 + if (!drv)
 + return 0;
 +
 + /* We have a driver! */
 + status |= VIRTIO_CONFIG_S_DRIVER;
 + vp_set_status(vp_dev-vdev, status);
 +
   vp_finalize_features(vp_dev-vdev);
  
 - if (drv  drv-restore)
 - ret = drv-restore(vp_dev-vdev);
 + if (!drv-restore)
 + return 0;

So in this case DRIVER_OK will never be set?

 +
 + ret = drv-restore(vp_dev-vdev);
 + if (ret) {
 + status |= VIRTIO_CONFIG_S_FAILED;
 + vp_set_status(vp_dev-vdev, status);
 + return ret;
 + }
  
   /* Finally, tell the device we're all set */
 - if (!ret)
 - vp_set_status(vp_dev-vdev, vp_dev-saved_status);
 + status |= VIRTIO_CONFIG_S_DRIVER_OK;
 + vp_set_status(vp_dev-vdev, status);
  
   return ret;
  }

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Amit Shah]

On (Wed) 27 Aug 2014 [16:43:40], Pranith Kumar wrote:
> The nocb callbacks generated before the nocb kthreads are spawned are
> enqueued in the nocb queue for later processing. Commit fbce7497ee5af ("rcu:
> Parallelize and economize NOCB kthread wakeups") introduced nocb leader 
> kthreads
> which checked the nocb_leader_wake flag to see if there were any such pending
> callbacks. A case was reported in which newly spawned leader kthreads were not
> processing the pending callbacks as this flag was not set, which led to a boot
> hang.
> 
> The following commit ensures that the newly spawned nocb kthreads process the
> pending callbacks by allowing the kthreads to run immediately after spawning
> instead of waiting. This is done by inverting the logic of nocb_leader_wake
> tests to nocb_leader_sleep which allows us to use the default initialization 
> of
> this flag to 0 to let the kthreads run.
> 
> Reported-by: Amit Shah 
> Signed-off-by: Pranith Kumar 
> Link: http://www.spinics.net/lists/kernel/msg1802899.html
> ---
>  kernel/rcu/tree.h|  2 +-
>  kernel/rcu/tree_plugin.h | 24 
>  2 files changed, 13 insertions(+), 13 deletions(-)

I'd have split this into two patches: one for the variable rename and
one for fixing the bug.

However, the backport Paul posted does work fine for me on master, so
you can add my

Tested-by: Amit Shah 

Thanks,

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [3.16 stable PATCH v2 1/2] virtio: rng: delay hwrng_register() till driver is ready

[Amit Shah]

Hey Greg,

Can you add these two patches to the 3.16 queue?

Thanks,

On (Tue) 12 Aug 2014 [13:23:45], Amit Shah wrote:
> Instead of calling hwrng_register() in the probe routing, call it in the
> scan routine.  This ensures that when hwrng_register() is successful,
> and it requests a few random bytes to seed the kernel's pool at init,
> we're ready to service that request.
> 
> This will also enable us to remove the workaround added previously to
> check whether probe was completed, and only then ask for data from the
> host.  The revert follows in the next commit.
> 
> There's a slight behaviour change here on unsuccessful hwrng_register().
> Previously, when hwrng_register() failed, the probe() routine would
> fail, and the vqs would be torn down, and driver would be marked not
> initialized.  Now, the vqs will remain initialized, driver would be
> marked initialized as well, but won't be available in the list of RNGs
> available to hwrng core.  To fix the failures, the procedure remains the
> same, i.e. unload and re-load the module, and hope things succeed the
> next time around.
> 
> Signed-off-by: Amit Shah 
> Signed-off-by: Rusty Russell 
> (cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
> Signed-off-by: Amit Shah 
> ---
>  drivers/char/hw_random/virtio-rng.c | 25 +++--
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/hw_random/virtio-rng.c 
> b/drivers/char/hw_random/virtio-rng.c
> index e9b15bc..f4e04f3 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -36,6 +36,7 @@ struct virtrng_info {
>   bool busy;
>   char name[25];
>   int index;
> + bool hwrng_register_done;
>  };
>  
>  static bool probe_done;
> @@ -137,15 +138,6 @@ static int probe_common(struct virtio_device *vdev)
>   return err;
>   }
>  
> - err = hwrng_register(>hwrng);
> - if (err) {
> - vdev->config->del_vqs(vdev);
> - vi->vq = NULL;
> - kfree(vi);
> - ida_simple_remove(_index_ida, index);
> - return err;
> - }
> -
>   probe_done = true;
>   return 0;
>  }
> @@ -153,9 +145,11 @@ static int probe_common(struct virtio_device *vdev)
>  static void remove_common(struct virtio_device *vdev)
>  {
>   struct virtrng_info *vi = vdev->priv;
> +
>   vdev->config->reset(vdev);
>   vi->busy = false;
> - hwrng_unregister(>hwrng);
> + if (vi->hwrng_register_done)
> + hwrng_unregister(>hwrng);
>   vdev->config->del_vqs(vdev);
>   ida_simple_remove(_index_ida, vi->index);
>   kfree(vi);
> @@ -171,6 +165,16 @@ static void virtrng_remove(struct virtio_device *vdev)
>   remove_common(vdev);
>  }
>  
> +static void virtrng_scan(struct virtio_device *vdev)
> +{
> + struct virtrng_info *vi = vdev->priv;
> + int err;
> +
> + err = hwrng_register(>hwrng);
> + if (!err)
> + vi->hwrng_register_done = true;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int virtrng_freeze(struct virtio_device *vdev)
>  {
> @@ -195,6 +199,7 @@ static struct virtio_driver virtio_rng_driver = {
>   .id_table = id_table,
>   .probe =virtrng_probe,
>   .remove =   virtrng_remove,
> + .scan = virtrng_scan,
>  #ifdef CONFIG_PM_SLEEP
>   .freeze =   virtrng_freeze,
>   .restore =  virtrng_restore,
> -- 
> 1.9.3
> 

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [3.16 stable PATCH v2 1/2] virtio: rng: delay hwrng_register() till driver is ready

[Amit Shah]

Hey Greg,

Can you add these two patches to the 3.16 queue?

Thanks,

On (Tue) 12 Aug 2014 [13:23:45], Amit Shah wrote:
 Instead of calling hwrng_register() in the probe routing, call it in the
 scan routine.  This ensures that when hwrng_register() is successful,
 and it requests a few random bytes to seed the kernel's pool at init,
 we're ready to service that request.
 
 This will also enable us to remove the workaround added previously to
 check whether probe was completed, and only then ask for data from the
 host.  The revert follows in the next commit.
 
 There's a slight behaviour change here on unsuccessful hwrng_register().
 Previously, when hwrng_register() failed, the probe() routine would
 fail, and the vqs would be torn down, and driver would be marked not
 initialized.  Now, the vqs will remain initialized, driver would be
 marked initialized as well, but won't be available in the list of RNGs
 available to hwrng core.  To fix the failures, the procedure remains the
 same, i.e. unload and re-load the module, and hope things succeed the
 next time around.
 
 Signed-off-by: Amit Shah amit.s...@redhat.com
 Signed-off-by: Rusty Russell ru...@rustcorp.com.au
 (cherry picked from commit 5c06273401f2eb7b290cadbae18ee00f8f65e893)
 Signed-off-by: Amit Shah amit.s...@redhat.com
 ---
  drivers/char/hw_random/virtio-rng.c | 25 +++--
  1 file changed, 15 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/char/hw_random/virtio-rng.c 
 b/drivers/char/hw_random/virtio-rng.c
 index e9b15bc..f4e04f3 100644
 --- a/drivers/char/hw_random/virtio-rng.c
 +++ b/drivers/char/hw_random/virtio-rng.c
 @@ -36,6 +36,7 @@ struct virtrng_info {
   bool busy;
   char name[25];
   int index;
 + bool hwrng_register_done;
  };
  
  static bool probe_done;
 @@ -137,15 +138,6 @@ static int probe_common(struct virtio_device *vdev)
   return err;
   }
  
 - err = hwrng_register(vi-hwrng);
 - if (err) {
 - vdev-config-del_vqs(vdev);
 - vi-vq = NULL;
 - kfree(vi);
 - ida_simple_remove(rng_index_ida, index);
 - return err;
 - }
 -
   probe_done = true;
   return 0;
  }
 @@ -153,9 +145,11 @@ static int probe_common(struct virtio_device *vdev)
  static void remove_common(struct virtio_device *vdev)
  {
   struct virtrng_info *vi = vdev-priv;
 +
   vdev-config-reset(vdev);
   vi-busy = false;
 - hwrng_unregister(vi-hwrng);
 + if (vi-hwrng_register_done)
 + hwrng_unregister(vi-hwrng);
   vdev-config-del_vqs(vdev);
   ida_simple_remove(rng_index_ida, vi-index);
   kfree(vi);
 @@ -171,6 +165,16 @@ static void virtrng_remove(struct virtio_device *vdev)
   remove_common(vdev);
  }
  
 +static void virtrng_scan(struct virtio_device *vdev)
 +{
 + struct virtrng_info *vi = vdev-priv;
 + int err;
 +
 + err = hwrng_register(vi-hwrng);
 + if (!err)
 + vi-hwrng_register_done = true;
 +}
 +
  #ifdef CONFIG_PM_SLEEP
  static int virtrng_freeze(struct virtio_device *vdev)
  {
 @@ -195,6 +199,7 @@ static struct virtio_driver virtio_rng_driver = {
   .id_table = id_table,
   .probe =virtrng_probe,
   .remove =   virtrng_remove,
 + .scan = virtrng_scan,
  #ifdef CONFIG_PM_SLEEP
   .freeze =   virtrng_freeze,
   .restore =  virtrng_restore,
 -- 
 1.9.3
 

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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] rcu: Make nocb leader kthreads process pending callbacks after spawning

[Amit Shah]

On (Wed) 27 Aug 2014 [16:43:40], Pranith Kumar wrote:
 The nocb callbacks generated before the nocb kthreads are spawned are
 enqueued in the nocb queue for later processing. Commit fbce7497ee5af (rcu:
 Parallelize and economize NOCB kthread wakeups) introduced nocb leader 
 kthreads
 which checked the nocb_leader_wake flag to see if there were any such pending
 callbacks. A case was reported in which newly spawned leader kthreads were not
 processing the pending callbacks as this flag was not set, which led to a boot
 hang.
 
 The following commit ensures that the newly spawned nocb kthreads process the
 pending callbacks by allowing the kthreads to run immediately after spawning
 instead of waiting. This is done by inverting the logic of nocb_leader_wake
 tests to nocb_leader_sleep which allows us to use the default initialization 
 of
 this flag to 0 to let the kthreads run.
 
 Reported-by: Amit Shah amit.s...@redhat.com
 Signed-off-by: Pranith Kumar bobby.pr...@gmail.com
 Link: http://www.spinics.net/lists/kernel/msg1802899.html
 ---
  kernel/rcu/tree.h|  2 +-
  kernel/rcu/tree_plugin.h | 24 
  2 files changed, 13 insertions(+), 13 deletions(-)

I'd have split this into two patches: one for the variable rename and
one for fixing the bug.

However, the backport Paul posted does work fine for me on master, so
you can add my

Tested-by: Amit Shah amit.s...@redhat.com

Thanks,

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Sat) 23 Aug 2014 [03:43:38], Pranith Kumar wrote:
> On Fri, Aug 22, 2014 at 5:53 PM, Paul E. McKenney
>  wrote:
> >
> > Hmmm...  Please try replacing the synchronize_rcu() in
> > __sysrq_swap_key_ops() with (say) schedule_timeout_interruptible(HZ / 10).
> > I bet that gets rid of the hang.  (And also introduces a low-probability
> > bug, but should be OK for testing.)
> >
> > The other thing to try is to revert your patch that turned my event
> > traces into printk()s, then put an ftrace_dump(DUMP_ALL); just after
> > the synchronize_rcu() -- that might make it so that the ftrace data
> > actually gets dumped out.
> >
> 
> I was able to reproduce this error on my Ubuntu 14.04 machine. I think
> I found the root cause of the problem after several kvm runs.
> 
> The problem is that earlier we were waiting on nocb_head and now we
> are waiting on nocb_leader_wake.
> 
> So there are a lot of nocb callbacks which are enqueued before the
> nocb thread is spawned. This sets up nocb_head to be non-null, because
> of which the nocb kthread used to wake up immediately after sleeping.
> 
> Now that we have switched to nocb_leader_wake, this is not being set
> when there are pending callbacks, unless the callbacks overflow the
> qhimark. The pending callbacks were around 7000 when the boot hangs.
> 
> So setting the qhimark using the boot parameter rcutree.qhimark=5000
> is one way to allow us to boot past the point by forcefully waking up
> the nocb kthread. I am not sure this is fool-proof.
> 
> Another option to start the nocb kthreads with nocb_leader_wake set,
> so that it can handle any pending callbacks. The following patch also
> allows us to boot properly.
> 
> Phew! Let me know if this makes any sense :)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 00dc411..4c397aa 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -2386,6 +2386,9 @@ static int rcu_nocb_kthread(void *arg)
> struct rcu_head **tail;
> struct rcu_data *rdp = arg;
> 
> +   if (rdp->nocb_leader == rdp)
> +   rdp->nocb_leader_wake = true;
> +
> /* Each pass through this loop invokes one batch of callbacks */
> for (;;) {
> /* Wait for callbacks. */

Yes, this patch helps my case as well.

Thanks!

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Sat) 23 Aug 2014 [03:43:38], Pranith Kumar wrote:
 On Fri, Aug 22, 2014 at 5:53 PM, Paul E. McKenney
 paul...@linux.vnet.ibm.com wrote:
 
  Hmmm...  Please try replacing the synchronize_rcu() in
  __sysrq_swap_key_ops() with (say) schedule_timeout_interruptible(HZ / 10).
  I bet that gets rid of the hang.  (And also introduces a low-probability
  bug, but should be OK for testing.)
 
  The other thing to try is to revert your patch that turned my event
  traces into printk()s, then put an ftrace_dump(DUMP_ALL); just after
  the synchronize_rcu() -- that might make it so that the ftrace data
  actually gets dumped out.
 
 
 I was able to reproduce this error on my Ubuntu 14.04 machine. I think
 I found the root cause of the problem after several kvm runs.
 
 The problem is that earlier we were waiting on nocb_head and now we
 are waiting on nocb_leader_wake.
 
 So there are a lot of nocb callbacks which are enqueued before the
 nocb thread is spawned. This sets up nocb_head to be non-null, because
 of which the nocb kthread used to wake up immediately after sleeping.
 
 Now that we have switched to nocb_leader_wake, this is not being set
 when there are pending callbacks, unless the callbacks overflow the
 qhimark. The pending callbacks were around 7000 when the boot hangs.
 
 So setting the qhimark using the boot parameter rcutree.qhimark=5000
 is one way to allow us to boot past the point by forcefully waking up
 the nocb kthread. I am not sure this is fool-proof.
 
 Another option to start the nocb kthreads with nocb_leader_wake set,
 so that it can handle any pending callbacks. The following patch also
 allows us to boot properly.
 
 Phew! Let me know if this makes any sense :)
 
 diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
 index 00dc411..4c397aa 100644
 --- a/kernel/rcu/tree_plugin.h
 +++ b/kernel/rcu/tree_plugin.h
 @@ -2386,6 +2386,9 @@ static int rcu_nocb_kthread(void *arg)
 struct rcu_head **tail;
 struct rcu_data *rdp = arg;
 
 +   if (rdp-nocb_leader == rdp)
 +   rdp-nocb_leader_wake = true;
 +
 /* Each pass through this loop invokes one batch of callbacks */
 for (;;) {
 /* Wait for callbacks. */

Yes, this patch helps my case as well.

Thanks!

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [22:44:05], Amit Shah wrote:
> Hm, found it:
> 
> The stall happens in do_initcalls().
> 
> pm_sysrq_init() is the function that causes the hang.  When I #if 0
> the line
> 
> register_sysrq_key('o', _poweroff_op);
> 
> in pm_sysrq_init(), the boot proceeds normally.
> 
> Now what this is, and what relation this has to rcu and that patch in
> particular is next...

... and enabling the following debug options makes the bug disappear:

CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_SELFTEST=y
CONFIG_DEBUG_OBJECTS_FREE=y
CONFIG_DEBUG_OBJECTS_TIMERS=y
CONFIG_DEBUG_OBJECTS_WORK=y
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER=y
CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT=1

Anyway, so it looks like a race somewhere in the schedule_work_on()
chain.  Not sure how to capture the debug messages there w/o disabling
these debug options.  I'll keep trying, though.


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [07:48:19], Paul E. McKenney wrote:
> On Fri, Aug 22, 2014 at 06:26:49PM +0530, Amit Shah wrote:
> > On (Fri) 22 Aug 2014 [18:06:51], Amit Shah wrote:
> > > On (Fri) 22 Aug 2014 [17:54:53], Amit Shah wrote:
> > > > On (Mon) 18 Aug 2014 [21:01:49], Paul E. McKenney wrote:
> > > > 
> > > > > The odds are low over the next few days.  I am adding nastier 
> > > > > rcutorture
> > > > > testing, however.  It would still be very good to get debug 
> > > > > information
> > > > > from your setup.  One approach would be to convert the trace function
> > > > > calls into printk(), if that would help.
> > > > 
> > > > I added a few printks on the lines of the traces in cases where
> > > > rcu_nocb_poll was checked -- since that reproduces the hang.  Are the
> > > > following traces sufficient, or should I keep adding more printks?
> > > > 
> > > > In the case of rcu-trace-nopoll.txt, the messages stop after a while
> > > > (when the guest locks up hard).  That's when I kill the qemu process.
> > > 
> > > And this is bt from gdb when the endless 
> > > 
> > >   RCUDEBUG __call_rcu_nocb_enqueue 2146 rcu_preempt 0 WakeNot
> > > 
> > > messages are being spewed.
> > > 
> > > I can't time it, but hope it gives some indication along with the printks.
> > 
> > ... and after the system 'locks up', this is the state it's in:
> > 
> > ^C
> > Program received signal SIGINT, Interrupt.
> > native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
> > 50   }
> > (gdb) bt
> > #0  native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
> > #1  0x8100b9c1 in arch_safe_halt () at 
> > ./arch/x86/include/asm/paravirt.h:111
> > #2  default_idle () at arch/x86/kernel/process.c:311
> > #3  0x8100c107 in arch_cpu_idle () at arch/x86/kernel/process.c:302
> > #4  0x8106a25a in cpuidle_idle_call () at kernel/sched/idle.c:120
> > #5  cpu_idle_loop () at kernel/sched/idle.c:220
> > #6  cpu_startup_entry (state=) at kernel/sched/idle.c:268
> > #7  0x813e068b in rest_init () at init/main.c:418
> > #8  0x81a8cf5a in start_kernel () at init/main.c:680
> > #9  0x81a8c4ba in x86_64_start_reservations 
> > (real_mode_data=) at arch/x86/kernel/head64.c:193
> > #10 0x81a8c607 in x86_64_start_kernel (real_mode_data=0x13f90 
> >  )
> > at arch/x86/kernel/head64.c:182
> > #11 0x in ?? ()
> > 
> > 
> > Wondering why it's doing this.  Am stepping through
> > cpu_startup_entry() to see if I get any clues.
> 
> This looks to me like normal behavior in the x86 ACPI idle loop.
> My guess is that the lockup is caused by indefinite blocking, in
> which case we would expect all the CPUs to be in the idle loop.

Hm, found it:

The stall happens in do_initcalls().

pm_sysrq_init() is the function that causes the hang.  When I #if 0
the line

register_sysrq_key('o', _poweroff_op);

in pm_sysrq_init(), the boot proceeds normally.

Now what this is, and what relation this has to rcu and that patch in
particular is next...


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [18:06:51], Amit Shah wrote:
> On (Fri) 22 Aug 2014 [17:54:53], Amit Shah wrote:
> > On (Mon) 18 Aug 2014 [21:01:49], Paul E. McKenney wrote:
> > 
> > > The odds are low over the next few days.  I am adding nastier rcutorture
> > > testing, however.  It would still be very good to get debug information
> > > from your setup.  One approach would be to convert the trace function
> > > calls into printk(), if that would help.
> > 
> > I added a few printks on the lines of the traces in cases where
> > rcu_nocb_poll was checked -- since that reproduces the hang.  Are the
> > following traces sufficient, or should I keep adding more printks?
> > 
> > In the case of rcu-trace-nopoll.txt, the messages stop after a while
> > (when the guest locks up hard).  That's when I kill the qemu process.
> 
> And this is bt from gdb when the endless 
> 
>   RCUDEBUG __call_rcu_nocb_enqueue 2146 rcu_preempt 0 WakeNot
> 
> messages are being spewed.
> 
> I can't time it, but hope it gives some indication along with the printks.

... and after the system 'locks up', this is the state it's in:

^C
Program received signal SIGINT, Interrupt.
native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
50   }
(gdb) bt
#0  native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
#1  0x8100b9c1 in arch_safe_halt () at 
./arch/x86/include/asm/paravirt.h:111
#2  default_idle () at arch/x86/kernel/process.c:311
#3  0x8100c107 in arch_cpu_idle () at arch/x86/kernel/process.c:302
#4  0x8106a25a in cpuidle_idle_call () at kernel/sched/idle.c:120
#5  cpu_idle_loop () at kernel/sched/idle.c:220
#6  cpu_startup_entry (state=) at kernel/sched/idle.c:268
#7  0x813e068b in rest_init () at init/main.c:418
#8  0x81a8cf5a in start_kernel () at init/main.c:680
#9  0x81a8c4ba in x86_64_start_reservations (real_mode_data=) at arch/x86/kernel/head64.c:193
#10 0x81a8c607 in x86_64_start_kernel (real_mode_data=0x13f90 
 )
at arch/x86/kernel/head64.c:182
#11 0x in ?? ()


Wondering why it's doing this.  Am stepping through
cpu_startup_entry() to see if I get any clues.


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [17:54:53], Amit Shah wrote:
> On (Mon) 18 Aug 2014 [21:01:49], Paul E. McKenney wrote:
> 
> > The odds are low over the next few days.  I am adding nastier rcutorture
> > testing, however.  It would still be very good to get debug information
> > from your setup.  One approach would be to convert the trace function
> > calls into printk(), if that would help.
> 
> I added a few printks on the lines of the traces in cases where
> rcu_nocb_poll was checked -- since that reproduces the hang.  Are the
> following traces sufficient, or should I keep adding more printks?
> 
> In the case of rcu-trace-nopoll.txt, the messages stop after a while
> (when the guest locks up hard).  That's when I kill the qemu process.

And this is bt from gdb when the endless 

  RCUDEBUG __call_rcu_nocb_enqueue 2146 rcu_preempt 0 WakeNot

messages are being spewed.

I can't time it, but hope it gives some indication along with the printks.

Program received signal SIGINT, Interrupt.
io_serial_out (p=0x82940780 , offset=, 
value=) at drivers/tty/serial/8250/8250_core.c:439
439   }
(gdb) bt
#0  io_serial_out (p=0x82940780 , offset=, value=) at drivers/tty/serial/8250/8250_core.c:439
#1  0x812b260a in serial_port_out (up=, 
offset=, value=) at 
include/linux/serial_core.h:214
#2  0x812b4781 in serial8250_console_putchar (port=0x82940780 
, ch=111) at drivers/tty/serial/8250/8250_core.c:2990
#3  0x812af07d in uart_console_write (port=0x82940780 
, 
s=0x828dd96a  "t\n8fff]\nes: 4KB 0, 2MB 0, 4MB 0, 1GB 
0\n6K bss, 33036K reserved)\n2 17:46:45 IST 2014\n", count=60, 
putchar=0x812b4758 ) at 
drivers/tty/serial/serial_core.c:1747
#4  0x812b470c in serial8250_console_write (co=, 
s=, count=60) at drivers/tty/serial/8250/8250_core.c:3025
#5  0x8107f517 in call_console_drivers (level=, len=60, 
text=) at kernel/printk/printk.c:1421
#6  0x81080498 in console_unlock () at kernel/printk/printk.c:2244
#7  0x81080b39 in vprintk_emit (facility=, 
level=, dict=, dictlen=, 
fmt=, args=) at kernel/printk/printk.c:1786
#8  0x813e5235 in printk (fmt=) at 
kernel/printk/printk.c:1851
#9  0x8108e46b in __call_rcu_nocb_enqueue (rdp=0x88000fbcce00, 
rhp=, rhtp=, rhcount=, 
rhcount_lazy=, flags=) at 
kernel/rcu/tree_plugin.h:2144
#10 0x81091140 in __call_rcu_nocb (flags=, 
lazy=, rhp=, rdp=)
at kernel/rcu/tree_plugin.h:2166
#11 __call_rcu (head=0x88000e6c5390, func=0x81131346 
, rsp=0x818389c0 , cpu=, 
lazy=) at kernel/rcu/tree.c:2687
#12 0x81091673 in call_rcu (head=, func=) 
at kernel/rcu/tree_plugin.h:678
#13 0x81131756 in put_object (object=0x88000e6c5308) at 
mm/kmemleak.c:471
#14 0x81131b8c in delete_object_full (ptr=) at 
mm/kmemleak.c:641
#15 0x813e1782 in kmemleak_free (ptr=) at 
mm/kmemleak.c:944
#16 0x81128782 in kmemleak_free_recursive (flags=, 
ptr=) at include/linux/kmemleak.h:50
#17 slab_free_hook (s=0x82940780 , 
x=0x88000e991c68) at mm/slub.c:1265
#18 0x8112a725 in slab_free (addr=, x=, 
page=, s=) at mm/slub.c:2644
#19 kmem_cache_free (s=, x=0x88000e991c68) at mm/slub.c:2681
#20 0x8121d84c in ida_get_new_above (ida=0x82940780 
, starting_id=, p_id=) at 
lib/idr.c:999
#21 0x8121dbe6 in ida_simple_get (ida=0x82940780 
, start=1016, end=, gfp_mask=0) at 
lib/idr.c:1101
#22 0x81188f19 in __kernfs_new_node (root=, name=0x0 
, mode=33060, flags=514) at fs/kernfs/dir.c:530
#23 0x81189e22 in kernfs_new_node (parent=0x88000e651000, 
name=, mode=33060, flags=) at fs/kernfs/dir.c:558
#24 0x8118b3a3 in __kernfs_create_file (parent=, 
name=, mode=, size=4096, 
ops=0x81424a80 , priv=, ns=0x0 
, name_is_static=true, key=0x81bc3a20 <__key.17290>)
at fs/kernfs/file.c:920
#25 0x8118bb6e in sysfs_add_file_mode_ns (parent=0x88000e651000, 
attr=0x88000e621358, is_bin=, mode=, 
ns=) at fs/sysfs/file.c:256
#26 0x8118c4c0 in create_files (update=, grp=, kobj=, parent=) at fs/sysfs/group.c:58
#27 internal_create_group (kobj=0x88000e67a1a8, update=, 
grp=0x88000e621298) at fs/sysfs/group.c:116
#28 0x8118c562 in sysfs_create_group (kobj=, 
grp=) at fs/sysfs/group.c:138
#29 0x81aa09e9 in kernel_add_sysfs_param (name_skip=, 
kparam=, name=) at kernel/params.c:783
#30 param_sysfs_builtin () at kernel/params.c:820
#31 param_sysfs_init () at kernel/params.c:940
#32 0x810003f4 in do_one_initcall (fn=0x81aa0886 
) at init/main.c:791
#33 0x81a8d08a in do_initcall_level (level=) at 
init/main.c:857
#34 do_initcalls () at init/main.c:865
#35 do_basic_setup () at init/main.c:884
#36 kernel_init_freeable () at init/main.c:1005
#37 0x813e084d in kernel_init (unused=) at 
in

Re: [PATCH tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [17:54:53], Amit Shah wrote:
 On (Mon) 18 Aug 2014 [21:01:49], Paul E. McKenney wrote:
 
  The odds are low over the next few days.  I am adding nastier rcutorture
  testing, however.  It would still be very good to get debug information
  from your setup.  One approach would be to convert the trace function
  calls into printk(), if that would help.
 
 I added a few printks on the lines of the traces in cases where
 rcu_nocb_poll was checked -- since that reproduces the hang.  Are the
 following traces sufficient, or should I keep adding more printks?
 
 In the case of rcu-trace-nopoll.txt, the messages stop after a while
 (when the guest locks up hard).  That's when I kill the qemu process.

And this is bt from gdb when the endless 

  RCUDEBUG __call_rcu_nocb_enqueue 2146 rcu_preempt 0 WakeNot

messages are being spewed.

I can't time it, but hope it gives some indication along with the printks.

Program received signal SIGINT, Interrupt.
io_serial_out (p=0x82940780 serial8250_ports, offset=optimized out, 
value=optimized out) at drivers/tty/serial/8250/8250_core.c:439
439   }
(gdb) bt
#0  io_serial_out (p=0x82940780 serial8250_ports, offset=optimized 
out, value=optimized out) at drivers/tty/serial/8250/8250_core.c:439
#1  0x812b260a in serial_port_out (up=optimized out, 
offset=optimized out, value=optimized out) at 
include/linux/serial_core.h:214
#2  0x812b4781 in serial8250_console_putchar (port=0x82940780 
serial8250_ports, ch=111) at drivers/tty/serial/8250/8250_core.c:2990
#3  0x812af07d in uart_console_write (port=0x82940780 
serial8250_ports, 
s=0x828dd96a text+58 t\n8fff]\nes: 4KB 0, 2MB 0, 4MB 0, 1GB 
0\n6K bss, 33036K reserved)\n2 17:46:45 IST 2014\n, count=60, 
putchar=0x812b4758 serial8250_console_putchar) at 
drivers/tty/serial/serial_core.c:1747
#4  0x812b470c in serial8250_console_write (co=optimized out, 
s=optimized out, count=60) at drivers/tty/serial/8250/8250_core.c:3025
#5  0x8107f517 in call_console_drivers (level=optimized out, len=60, 
text=optimized out) at kernel/printk/printk.c:1421
#6  0x81080498 in console_unlock () at kernel/printk/printk.c:2244
#7  0x81080b39 in vprintk_emit (facility=optimized out, 
level=optimized out, dict=optimized out, dictlen=optimized out, 
fmt=optimized out, args=optimized out) at kernel/printk/printk.c:1786
#8  0x813e5235 in printk (fmt=optimized out) at 
kernel/printk/printk.c:1851
#9  0x8108e46b in __call_rcu_nocb_enqueue (rdp=0x88000fbcce00, 
rhp=optimized out, rhtp=optimized out, rhcount=optimized out, 
rhcount_lazy=optimized out, flags=optimized out) at 
kernel/rcu/tree_plugin.h:2144
#10 0x81091140 in __call_rcu_nocb (flags=optimized out, 
lazy=optimized out, rhp=optimized out, rdp=optimized out)
at kernel/rcu/tree_plugin.h:2166
#11 __call_rcu (head=0x88000e6c5390, func=0x81131346 
free_object_rcu, rsp=0x818389c0 rcu_preempt_state, cpu=optimized 
out, 
lazy=optimized out) at kernel/rcu/tree.c:2687
#12 0x81091673 in call_rcu (head=optimized out, func=optimized out) 
at kernel/rcu/tree_plugin.h:678
#13 0x81131756 in put_object (object=0x88000e6c5308) at 
mm/kmemleak.c:471
#14 0x81131b8c in delete_object_full (ptr=optimized out) at 
mm/kmemleak.c:641
#15 0x813e1782 in kmemleak_free (ptr=optimized out) at 
mm/kmemleak.c:944
#16 0x81128782 in kmemleak_free_recursive (flags=optimized out, 
ptr=optimized out) at include/linux/kmemleak.h:50
#17 slab_free_hook (s=0x82940780 serial8250_ports, 
x=0x88000e991c68) at mm/slub.c:1265
#18 0x8112a725 in slab_free (addr=optimized out, x=optimized out, 
page=optimized out, s=optimized out) at mm/slub.c:2644
#19 kmem_cache_free (s=optimized out, x=0x88000e991c68) at mm/slub.c:2681
#20 0x8121d84c in ida_get_new_above (ida=0x82940780 
serial8250_ports, starting_id=optimized out, p_id=optimized out) at 
lib/idr.c:999
#21 0x8121dbe6 in ida_simple_get (ida=0x82940780 
serial8250_ports, start=1016, end=optimized out, gfp_mask=0) at 
lib/idr.c:1101
#22 0x81188f19 in __kernfs_new_node (root=optimized out, name=0x0 
irq_stack_union, mode=33060, flags=514) at fs/kernfs/dir.c:530
#23 0x81189e22 in kernfs_new_node (parent=0x88000e651000, 
name=optimized out, mode=33060, flags=optimized out) at fs/kernfs/dir.c:558
#24 0x8118b3a3 in __kernfs_create_file (parent=optimized out, 
name=optimized out, mode=optimized out, size=4096, 
ops=0x81424a80 sysfs_file_kfops_rw, priv=optimized out, ns=0x0 
irq_stack_union, name_is_static=true, key=0x81bc3a20 __key.17290)
at fs/kernfs/file.c:920
#25 0x8118bb6e in sysfs_add_file_mode_ns (parent=0x88000e651000, 
attr=0x88000e621358, is_bin=optimized out, mode=optimized out, 
ns=optimized out) at fs/sysfs/file.c:256
#26

Re: [PATCH tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [18:06:51], Amit Shah wrote:
 On (Fri) 22 Aug 2014 [17:54:53], Amit Shah wrote:
  On (Mon) 18 Aug 2014 [21:01:49], Paul E. McKenney wrote:
  
   The odds are low over the next few days.  I am adding nastier rcutorture
   testing, however.  It would still be very good to get debug information
   from your setup.  One approach would be to convert the trace function
   calls into printk(), if that would help.
  
  I added a few printks on the lines of the traces in cases where
  rcu_nocb_poll was checked -- since that reproduces the hang.  Are the
  following traces sufficient, or should I keep adding more printks?
  
  In the case of rcu-trace-nopoll.txt, the messages stop after a while
  (when the guest locks up hard).  That's when I kill the qemu process.
 
 And this is bt from gdb when the endless 
 
   RCUDEBUG __call_rcu_nocb_enqueue 2146 rcu_preempt 0 WakeNot
 
 messages are being spewed.
 
 I can't time it, but hope it gives some indication along with the printks.

... and after the system 'locks up', this is the state it's in:

^C
Program received signal SIGINT, Interrupt.
native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
50   }
(gdb) bt
#0  native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
#1  0x8100b9c1 in arch_safe_halt () at 
./arch/x86/include/asm/paravirt.h:111
#2  default_idle () at arch/x86/kernel/process.c:311
#3  0x8100c107 in arch_cpu_idle () at arch/x86/kernel/process.c:302
#4  0x8106a25a in cpuidle_idle_call () at kernel/sched/idle.c:120
#5  cpu_idle_loop () at kernel/sched/idle.c:220
#6  cpu_startup_entry (state=optimized out) at kernel/sched/idle.c:268
#7  0x813e068b in rest_init () at init/main.c:418
#8  0x81a8cf5a in start_kernel () at init/main.c:680
#9  0x81a8c4ba in x86_64_start_reservations (real_mode_data=optimized 
out) at arch/x86/kernel/head64.c:193
#10 0x81a8c607 in x86_64_start_kernel (real_mode_data=0x13f90 
cpu_lock_stats+29184 error: Cannot access memory at address 0x13f90)
at arch/x86/kernel/head64.c:182
#11 0x in ?? ()


Wondering why it's doing this.  Am stepping through
cpu_startup_entry() to see if I get any clues.


Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [07:48:19], Paul E. McKenney wrote:
 On Fri, Aug 22, 2014 at 06:26:49PM +0530, Amit Shah wrote:
  On (Fri) 22 Aug 2014 [18:06:51], Amit Shah wrote:
   On (Fri) 22 Aug 2014 [17:54:53], Amit Shah wrote:
On (Mon) 18 Aug 2014 [21:01:49], Paul E. McKenney wrote:

 The odds are low over the next few days.  I am adding nastier 
 rcutorture
 testing, however.  It would still be very good to get debug 
 information
 from your setup.  One approach would be to convert the trace function
 calls into printk(), if that would help.

I added a few printks on the lines of the traces in cases where
rcu_nocb_poll was checked -- since that reproduces the hang.  Are the
following traces sufficient, or should I keep adding more printks?

In the case of rcu-trace-nopoll.txt, the messages stop after a while
(when the guest locks up hard).  That's when I kill the qemu process.
   
   And this is bt from gdb when the endless 
   
 RCUDEBUG __call_rcu_nocb_enqueue 2146 rcu_preempt 0 WakeNot
   
   messages are being spewed.
   
   I can't time it, but hope it gives some indication along with the printks.
  
  ... and after the system 'locks up', this is the state it's in:
  
  ^C
  Program received signal SIGINT, Interrupt.
  native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
  50   }
  (gdb) bt
  #0  native_safe_halt () at ./arch/x86/include/asm/irqflags.h:50
  #1  0x8100b9c1 in arch_safe_halt () at 
  ./arch/x86/include/asm/paravirt.h:111
  #2  default_idle () at arch/x86/kernel/process.c:311
  #3  0x8100c107 in arch_cpu_idle () at arch/x86/kernel/process.c:302
  #4  0x8106a25a in cpuidle_idle_call () at kernel/sched/idle.c:120
  #5  cpu_idle_loop () at kernel/sched/idle.c:220
  #6  cpu_startup_entry (state=optimized out) at kernel/sched/idle.c:268
  #7  0x813e068b in rest_init () at init/main.c:418
  #8  0x81a8cf5a in start_kernel () at init/main.c:680
  #9  0x81a8c4ba in x86_64_start_reservations 
  (real_mode_data=optimized out) at arch/x86/kernel/head64.c:193
  #10 0x81a8c607 in x86_64_start_kernel (real_mode_data=0x13f90 
  cpu_lock_stats+29184 error: Cannot access memory at address 0x13f90)
  at arch/x86/kernel/head64.c:182
  #11 0x in ?? ()
  
  
  Wondering why it's doing this.  Am stepping through
  cpu_startup_entry() to see if I get any clues.
 
 This looks to me like normal behavior in the x86 ACPI idle loop.
 My guess is that the lockup is caused by indefinite blocking, in
 which case we would expect all the CPUs to be in the idle loop.

Hm, found it:

The stall happens in do_initcalls().

pm_sysrq_init() is the function that causes the hang.  When I #if 0
the line

register_sysrq_key('o', sysrq_poweroff_op);

in pm_sysrq_init(), the boot proceeds normally.

Now what this is, and what relation this has to rcu and that patch in
particular is next...


Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 22 Aug 2014 [22:44:05], Amit Shah wrote:
 Hm, found it:
 
 The stall happens in do_initcalls().
 
 pm_sysrq_init() is the function that causes the hang.  When I #if 0
 the line
 
 register_sysrq_key('o', sysrq_poweroff_op);
 
 in pm_sysrq_init(), the boot proceeds normally.
 
 Now what this is, and what relation this has to rcu and that patch in
 particular is next...

... and enabling the following debug options makes the bug disappear:

CONFIG_DEBUG_OBJECTS=y
CONFIG_DEBUG_OBJECTS_SELFTEST=y
CONFIG_DEBUG_OBJECTS_FREE=y
CONFIG_DEBUG_OBJECTS_TIMERS=y
CONFIG_DEBUG_OBJECTS_WORK=y
CONFIG_DEBUG_OBJECTS_RCU_HEAD=y
CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER=y
CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT=1

Anyway, so it looks like a race somewhere in the schedule_work_on()
chain.  Not sure how to capture the debug messages there w/o disabling
these debug options.  I'll keep trying, though.


Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 2/2] Install udev rules in system-default directory

[Amit Shah]

On (Tue) 19 Aug 2014 [19:20:20], Paolo Bonzini wrote:
> Il 07/08/2014 15:09, Amit Shah ha scritto:
> > +udevrules_DATA  = 90-virtio-rng.rules
> 
> If rng-tools is packaged with "make dist" you need this to be
> 
> dist_udevrules_DATA= 90-virtio-rng.rules
> 
> or alternatively
> 
> EXTRA_DIST = 90-virtio-rng.rules
> udevrules_DATA = 90-virtio-rng.rules
> 
> > +AC_ARG_WITH([udevrulesdir],
> > +   AS_HELP_STRING([--with-udevrulesdir=DIR], [Directory for udev rules]),
> > +   [],
> > +   [with_udevrulesdir=$($PKG_CONFIG --variable=udevdir udev)"/rules.d"])
> > +AC_SUBST([udevrulesdir], [$with_udevrulesdir])
> 
> You can use AM_CONDITIONAL to skip the installation if
> $with_udevrulesdir is "no" (aka --without-udevrulesdir).  Then you'd have
> 
> EXTRA_DIST = 90-virtio-rng.rules
> if INSTALL_UDEV_RULES
> udevrules_DATA = 90-virtio-rng.rules
> endif
> 
> I don't know offhand if this works:
> 
> if INSTALL_UDEV_RULES
> dist_udevrules_DATA= 90-virtio-rng.rules
> endif
> 
> but I think so; you can check with "./configure --without-udevrulesdir
> && make && make dist".

Thanks!  Will give this a shot for curiosity's sake.  The
khwrngd-based patch has already been merged upstream, so this is now
obsolete.


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 2/2] Install udev rules in system-default directory

[Amit Shah]

On (Tue) 19 Aug 2014 [19:20:20], Paolo Bonzini wrote:
 Il 07/08/2014 15:09, Amit Shah ha scritto:
  +udevrules_DATA  = 90-virtio-rng.rules
 
 If rng-tools is packaged with make dist you need this to be
 
 dist_udevrules_DATA= 90-virtio-rng.rules
 
 or alternatively
 
 EXTRA_DIST = 90-virtio-rng.rules
 udevrules_DATA = 90-virtio-rng.rules
 
  +AC_ARG_WITH([udevrulesdir],
  +   AS_HELP_STRING([--with-udevrulesdir=DIR], [Directory for udev rules]),
  +   [],
  +   [with_udevrulesdir=$($PKG_CONFIG --variable=udevdir udev)/rules.d])
  +AC_SUBST([udevrulesdir], [$with_udevrulesdir])
 
 You can use AM_CONDITIONAL to skip the installation if
 $with_udevrulesdir is no (aka --without-udevrulesdir).  Then you'd have
 
 EXTRA_DIST = 90-virtio-rng.rules
 if INSTALL_UDEV_RULES
 udevrules_DATA = 90-virtio-rng.rules
 endif
 
 I don't know offhand if this works:
 
 if INSTALL_UDEV_RULES
 dist_udevrules_DATA= 90-virtio-rng.rules
 endif
 
 but I think so; you can check with ./configure --without-udevrulesdir
  make  make dist.

Thanks!  Will give this a shot for curiosity's sake.  The
khwrngd-based patch has already been merged upstream, so this is now
obsolete.


Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 15 Aug 2014 [08:04:05], Paul E. McKenney wrote:
> On Fri, Aug 15, 2014 at 10:54:11AM +0530, Amit Shah wrote:
> > On (Wed) 13 Aug 2014 [06:00:49], Paul E. McKenney wrote:
> > > On Wed, Aug 13, 2014 at 11:14:39AM +0530, Amit Shah wrote:
> > > > On (Tue) 12 Aug 2014 [14:41:51], Paul E. McKenney wrote:
> > > > > On Tue, Aug 12, 2014 at 02:39:36PM -0700, Paul E. McKenney wrote:
> > > > > > On Tue, Aug 12, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
> > > > > > > On Tue, Aug 12, 2014 at 11:03:21AM +0530, Amit Shah wrote:
> > > > > > 
> > > > > > [ . . . ]
> > > > > > 
> > > > > > > > I know of only virtio-console doing this (via userspace only,
> > > > > > > > though).
> > > > > > > 
> > > > > > > As in userspace within the guest?  That would not work.  The 
> > > > > > > userspace
> > > > > > > that the qemu is running in might.  There is a way to extract 
> > > > > > > ftrace info
> > > > > > > from crash dumps, so one approach would be "sendkey alt-sysrq-c", 
> > > > > > > then
> > > > > > > pull the buffer from the resulting dump.  For all I know, there 
> > > > > > > might also
> > > > > > > be some script that uses the qemu "x" command to get at the 
> > > > > > > ftrace buffer.
> > > > > > > 
> > > > > > > Again, I cannot reproduce this, and I have been through the code 
> > > > > > > several
> > > > > > > times over the past few days, and am not seeing it.  I could start
> > > > > > > sending you random diagnostic patches, but it would be much 
> > > > > > > better if
> > > > > > > we could get the trace data from the failure.
> > > > 
> > > > I think the only recourse I now have is to dump the guest state from
> > > > qemu, and attempt to find the ftrace buffers by poking pages and
> > > > finding some ftrace-like struct... and then dumping the buffers.
> > > 
> > > The data exists in the qemu guest state, so it would be good to have
> > > it one way or another.  My current (perhaps self-serving) guess is that
> > > you have come up with a way to trick qemu into dropping IPIs.
> > 
> > I didn't get around to doing this yet; will get to it next week.
> > 
> > In the meantime, I tried this on RHEL6 (with RHEL6 qemu and gcc and
> > seabios), and that exhibits the problem similarly with my .config.
> 
> And I am running my tests successfully on an x86_64 system running
> Ubuntu 12.04.  Some testing on 14.04 seems to require booting with
> acpi=off, leading to my perhaps self-serving guess above.

It looks like Ubuntu 12.04 has a choice of multiple kernels.  Which
one are you running?

Also, is there a chance you could try this on a RHEL6 box?

Thanks,

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Fri) 15 Aug 2014 [08:04:05], Paul E. McKenney wrote:
 On Fri, Aug 15, 2014 at 10:54:11AM +0530, Amit Shah wrote:
  On (Wed) 13 Aug 2014 [06:00:49], Paul E. McKenney wrote:
   On Wed, Aug 13, 2014 at 11:14:39AM +0530, Amit Shah wrote:
On (Tue) 12 Aug 2014 [14:41:51], Paul E. McKenney wrote:
 On Tue, Aug 12, 2014 at 02:39:36PM -0700, Paul E. McKenney wrote:
  On Tue, Aug 12, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
   On Tue, Aug 12, 2014 at 11:03:21AM +0530, Amit Shah wrote:
  
  [ . . . ]
  
I know of only virtio-console doing this (via userspace only,
though).
   
   As in userspace within the guest?  That would not work.  The 
   userspace
   that the qemu is running in might.  There is a way to extract 
   ftrace info
   from crash dumps, so one approach would be sendkey alt-sysrq-c, 
   then
   pull the buffer from the resulting dump.  For all I know, there 
   might also
   be some script that uses the qemu x command to get at the 
   ftrace buffer.
   
   Again, I cannot reproduce this, and I have been through the code 
   several
   times over the past few days, and am not seeing it.  I could start
   sending you random diagnostic patches, but it would be much 
   better if
   we could get the trace data from the failure.

I think the only recourse I now have is to dump the guest state from
qemu, and attempt to find the ftrace buffers by poking pages and
finding some ftrace-like struct... and then dumping the buffers.
   
   The data exists in the qemu guest state, so it would be good to have
   it one way or another.  My current (perhaps self-serving) guess is that
   you have come up with a way to trick qemu into dropping IPIs.
  
  I didn't get around to doing this yet; will get to it next week.
  
  In the meantime, I tried this on RHEL6 (with RHEL6 qemu and gcc and
  seabios), and that exhibits the problem similarly with my .config.
 
 And I am running my tests successfully on an x86_64 system running
 Ubuntu 12.04.  Some testing on 14.04 seems to require booting with
 acpi=off, leading to my perhaps self-serving guess above.

It looks like Ubuntu 12.04 has a choice of multiple kernels.  Which
one are you running?

Also, is there a chance you could try this on a RHEL6 box?

Thanks,

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Wed) 13 Aug 2014 [06:00:49], Paul E. McKenney wrote:
> On Wed, Aug 13, 2014 at 11:14:39AM +0530, Amit Shah wrote:
> > On (Tue) 12 Aug 2014 [14:41:51], Paul E. McKenney wrote:
> > > On Tue, Aug 12, 2014 at 02:39:36PM -0700, Paul E. McKenney wrote:
> > > > On Tue, Aug 12, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
> > > > > On Tue, Aug 12, 2014 at 11:03:21AM +0530, Amit Shah wrote:
> > > > 
> > > > [ . . . ]
> > > > 
> > > > > > I know of only virtio-console doing this (via userspace only,
> > > > > > though).
> > > > > 
> > > > > As in userspace within the guest?  That would not work.  The userspace
> > > > > that the qemu is running in might.  There is a way to extract ftrace 
> > > > > info
> > > > > from crash dumps, so one approach would be "sendkey alt-sysrq-c", then
> > > > > pull the buffer from the resulting dump.  For all I know, there might 
> > > > > also
> > > > > be some script that uses the qemu "x" command to get at the ftrace 
> > > > > buffer.
> > > > > 
> > > > > Again, I cannot reproduce this, and I have been through the code 
> > > > > several
> > > > > times over the past few days, and am not seeing it.  I could start
> > > > > sending you random diagnostic patches, but it would be much better if
> > > > > we could get the trace data from the failure.
> > 
> > I think the only recourse I now have is to dump the guest state from
> > qemu, and attempt to find the ftrace buffers by poking pages and
> > finding some ftrace-like struct... and then dumping the buffers.
> 
> The data exists in the qemu guest state, so it would be good to have
> it one way or another.  My current (perhaps self-serving) guess is that
> you have come up with a way to trick qemu into dropping IPIs.

I didn't get around to doing this yet; will get to it next week.

In the meantime, I tried this on RHEL6 (with RHEL6 qemu and gcc and
seabios), and that exhibits the problem similarly with my .config.



> > > +
> > >   return true;
> > 
> > I have return 1; here.
> > 
> > I'm on linux.git, c8d6637d0497d62093dbba0694c7b3a80b79bfe1.
> 
> I am working on top of my -rcu tree, which contains the fix from "1" to
> "true" compared to current mainline.  So this will resolve itself, and
> you should be OK fixing up conflict in either direction.

Yep, I did do that.  Just noted here that the hunk didn't directly
apply.

Thanks,

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PULL] virtio-rng: add derating factor for use by hwrng core

[Amit Shah]

Hi Linus,

Sending directly to you with the commit log changes Ted Ts'o pointed
out.  Not sure if Rusty's back after his travel, but this already has
his s-o-b.

Please pull.

The following changes since commit c9d26423e56ce1ab4d786f92aebecf859d419293:

  Merge tag 'pm+acpi-3.17-rc1-2' of 
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm (2014-08-14 
18:13:46 -0600)

are available in the git repository at:


  git://git.kernel.org/pub/scm/linux/kernel/git/amit/virtio.git rng-queue

for you to fetch changes up to 34679ec7a0c45da8161507e1f2e1f72749dfd85c:

  virtio: rng: add derating factor for use by hwrng core (2014-08-15 10:26:01 
+0530)



Amit Shah (1):
  virtio: rng: add derating factor for use by hwrng core

 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3 1/1] virtio: rng: add derating factor for use by hwrng core

[Amit Shah]

The khwrngd thread is started when a hwrng device of sufficient
quality is registered.  The virtio-rng device is backed by the
hypervisor, and we trust the hypervisor to provide real entropy.

A malicious or badly-implemented hypervisor is a scenario that's
irrelevant -- such a setup is bound to cause all sorts of badness, and a
compromised hwrng is the least of the user's worries.

Given this, we might as well assume that the quality of randomness we
receive is perfectly trustworthy.  Hence, we use 100% for the factor,
indicating maximum confidence in the source.

Signed-off-by: Amit Shah 
Reviewed-by: H. Peter Anvin 
Reviewed-by: Amos Kong 
Signed-off-by: Rusty Russell 

---
Pretty small and contained patch; would be great if it is picked up for
3.17.

v2: re-word commit msg (hpa)
v3: re-word commit msg (tytso)
---
 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/virtio-rng.c 
b/drivers/char/hw_random/virtio-rng.c
index 0027137..2e3139e 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev)
.cleanup = virtio_cleanup,
.priv = (unsigned long)vi,
.name = vi->name,
+   .quality = 1000,
};
vdev->priv = vi;
 
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3 1/1] virtio: rng: add derating factor for use by hwrng core

[Amit Shah]

The khwrngd thread is started when a hwrng device of sufficient
quality is registered.  The virtio-rng device is backed by the
hypervisor, and we trust the hypervisor to provide real entropy.

A malicious or badly-implemented hypervisor is a scenario that's
irrelevant -- such a setup is bound to cause all sorts of badness, and a
compromised hwrng is the least of the user's worries.

Given this, we might as well assume that the quality of randomness we
receive is perfectly trustworthy.  Hence, we use 100% for the factor,
indicating maximum confidence in the source.

Signed-off-by: Amit Shah amit.s...@redhat.com
Reviewed-by: H. Peter Anvin h...@linux.intel.com
Reviewed-by: Amos Kong ak...@redhat.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au

---
Pretty small and contained patch; would be great if it is picked up for
3.17.

v2: re-word commit msg (hpa)
v3: re-word commit msg (tytso)
---
 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/virtio-rng.c 
b/drivers/char/hw_random/virtio-rng.c
index 0027137..2e3139e 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev)
.cleanup = virtio_cleanup,
.priv = (unsigned long)vi,
.name = vi-name,
+   .quality = 1000,
};
vdev-priv = vi;
 
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PULL] virtio-rng: add derating factor for use by hwrng core

[Amit Shah]

Hi Linus,

Sending directly to you with the commit log changes Ted Ts'o pointed
out.  Not sure if Rusty's back after his travel, but this already has
his s-o-b.

Please pull.

The following changes since commit c9d26423e56ce1ab4d786f92aebecf859d419293:

  Merge tag 'pm+acpi-3.17-rc1-2' of 
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm (2014-08-14 
18:13:46 -0600)

are available in the git repository at:


  git://git.kernel.org/pub/scm/linux/kernel/git/amit/virtio.git rng-queue

for you to fetch changes up to 34679ec7a0c45da8161507e1f2e1f72749dfd85c:

  virtio: rng: add derating factor for use by hwrng core (2014-08-15 10:26:01 
+0530)



Amit Shah (1):
  virtio: rng: add derating factor for use by hwrng core

 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Wed) 13 Aug 2014 [06:00:49], Paul E. McKenney wrote:
 On Wed, Aug 13, 2014 at 11:14:39AM +0530, Amit Shah wrote:
  On (Tue) 12 Aug 2014 [14:41:51], Paul E. McKenney wrote:
   On Tue, Aug 12, 2014 at 02:39:36PM -0700, Paul E. McKenney wrote:
On Tue, Aug 12, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
 On Tue, Aug 12, 2014 at 11:03:21AM +0530, Amit Shah wrote:

[ . . . ]

  I know of only virtio-console doing this (via userspace only,
  though).
 
 As in userspace within the guest?  That would not work.  The userspace
 that the qemu is running in might.  There is a way to extract ftrace 
 info
 from crash dumps, so one approach would be sendkey alt-sysrq-c, then
 pull the buffer from the resulting dump.  For all I know, there might 
 also
 be some script that uses the qemu x command to get at the ftrace 
 buffer.
 
 Again, I cannot reproduce this, and I have been through the code 
 several
 times over the past few days, and am not seeing it.  I could start
 sending you random diagnostic patches, but it would be much better if
 we could get the trace data from the failure.
  
  I think the only recourse I now have is to dump the guest state from
  qemu, and attempt to find the ftrace buffers by poking pages and
  finding some ftrace-like struct... and then dumping the buffers.
 
 The data exists in the qemu guest state, so it would be good to have
 it one way or another.  My current (perhaps self-serving) guess is that
 you have come up with a way to trick qemu into dropping IPIs.

I didn't get around to doing this yet; will get to it next week.

In the meantime, I tried this on RHEL6 (with RHEL6 qemu and gcc and
seabios), and that exhibits the problem similarly with my .config.

snip

   +
 return true;
  
  I have return 1; here.
  
  I'm on linux.git, c8d6637d0497d62093dbba0694c7b3a80b79bfe1.
 
 I am working on top of my -rcu tree, which contains the fix from 1 to
 true compared to current mainline.  So this will resolve itself, and
 you should be OK fixing up conflict in either direction.

Yep, I did do that.  Just noted here that the hunk didn't directly
apply.

Thanks,

Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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 tip/core/rcu 1/2] rcu: Parallelize and economize NOCB kthread wakeups

[Amit Shah]

On (Tue) 12 Aug 2014 [14:41:51], Paul E. McKenney wrote:
> On Tue, Aug 12, 2014 at 02:39:36PM -0700, Paul E. McKenney wrote:
> > On Tue, Aug 12, 2014 at 09:06:21AM -0700, Paul E. McKenney wrote:
> > > On Tue, Aug 12, 2014 at 11:03:21AM +0530, Amit Shah wrote:
> > 
> > [ . . . ]
> > 
> > > > I know of only virtio-console doing this (via userspace only,
> > > > though).
> > > 
> > > As in userspace within the guest?  That would not work.  The userspace
> > > that the qemu is running in might.  There is a way to extract ftrace info
> > > from crash dumps, so one approach would be "sendkey alt-sysrq-c", then
> > > pull the buffer from the resulting dump.  For all I know, there might also
> > > be some script that uses the qemu "x" command to get at the ftrace buffer.
> > > 
> > > Again, I cannot reproduce this, and I have been through the code several
> > > times over the past few days, and am not seeing it.  I could start
> > > sending you random diagnostic patches, but it would be much better if
> > > we could get the trace data from the failure.

I think the only recourse I now have is to dump the guest state from
qemu, and attempt to find the ftrace buffers by poking pages and
finding some ftrace-like struct... and then dumping the buffers.

> > Hearing no objections, random patch #1.  The compiler could in theory
> > cause trouble without this patch, so there is some possibility that
> > it is a fix.
> 
> #2...  This would have been a problem without the earlier patch, but
> who knows?  (#1 moved from theoretically possible but not on x86 to
> maybe on x86 given a sufficiently malevolent compiler with the
> patch that you located with bisection.)

I tried all 3 patches individually, and all 3 together, no success.

My gcc is gcc-4.8.3-1.fc20.x86_64.  I'm using a fairly uptodate Fedora
20 system on my laptop for these tests.

Curiously, patches 1 and 3 applied fine, but this one had a conflict.

> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 1dc72f523c4a..1da605740e8d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -2137,6 +2137,17 @@ static bool __call_rcu_nocb(struct rcu_data *rdp, 
> struct rcu_head *rhp,

I have this hunk at line 2161, and...

>   trace_rcu_callback(rdp->rsp->name, rhp,
>  -atomic_long_read(>nocb_q_count_lazy),
>  -atomic_long_read(>nocb_q_count));
> +
> + /*
> +  * If called from an extended quiescent state with interrupts
> +  * disabled, invoke the RCU core in order to allow the idle-entry
> +  * deferred-wakeup check to function.
> +  */
> + if (irqs_disabled_flags(flags) &&
> + !rcu_is_watching() &&
> + cpu_online(smp_processor_id()))
> + invoke_rcu_core();
> +
>   return true;

I have return 1; here.

I'm on linux.git, c8d6637d0497d62093dbba0694c7b3a80b79bfe1.


Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/1] virtio: rng: add derating factor for use by hwrng core

[Amit Shah]

The khwrngd thread is started when a hwrng device of sufficient
quality is registered.  The virtio-rng device is backed by the
hypervisor, and we trust the hypervisor to provide real entropy.

A malicious hypervisor is a scenario that's irrelevant -- such a setup
is bound to cause all sorts of badness, and a compromised hwrng is not
the biggest threat.

Given this, we are certain the quality of randomness we receive is
perfectly trustworthy.  Hence, we use 100% for the factor, indicating
maximum confidence in the source.

Signed-off-by: Amit Shah 

---
Pretty small and contained patch; would be great if it is picked up for
3.17.

v2: re-word commit msg
---
 drivers/char/hw_random/virtio-rng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/char/hw_random/virtio-rng.c 
b/drivers/char/hw_random/virtio-rng.c
index 0027137..2e3139e 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -116,6 +116,7 @@ static int probe_common(struct virtio_device *vdev)
.cleanup = virtio_cleanup,
.priv = (unsigned long)vi,
.name = vi->name,
+   .quality = 1000,
};
vdev->priv = vi;
 
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/