[Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Stefan Hajnoczi]

The virtio_queue_set_notification() nesting introduced for AioContext polling
raised an assertion with virtio-net (even in non-polling mode).  Converting
virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
nesting fashion would be invasive and isn't worth it.

Patch 1 contains the revert to resolve the bug that Doug noticed.

Patch 2 is a less efficient but safe alternative.

Stefan Hajnoczi (2):
  Revert "virtio: turn vq->notification into a nested counter"
  virtio: disable notifications again after poll succeeded

 hw/virtio/virtio.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/2] Revert "virtio: turn vq->notification into a nested counter"

[Stefan Hajnoczi]

This reverts commit aff8fd18f1786fc5af259a9bc0077727222f51ca.

Both virtio-net and virtio-crypto do not balance
virtio_queue_set_notification() enable and disable calls.  This makes
the notifications_disabled counter unreliable and Doug Goldstein
reported the following assertion failure:

  #3  0x00007ffff44d1c62 in __GI___assert_fail (
      assertion=assertion@entry=0x555555ae8e8a "vq->notification_disabled > 0",
      file=file@entry=0x555555ae89c0 "/home/doug/work/qemu/hw/virtio/virtio.c",
      line=line@entry=215,
      function=function@entry=0x555555ae9630 <__PRETTY_FUNCTION__.43707>
      "virtio_queue_set_notification") at assert.c:101
  #4  0x00005555557f25d6 in virtio_queue_set_notification (vq=0x55555666aa90,
      enable=enable@entry=1) at /home/doug/work/qemu/hw/virtio/virtio.c:215
  #5  0x00005555557dc311 in virtio_net_has_buffers (q=<optimized out>,
      q=<optimized out>, bufsize=102)
      at /home/doug/work/qemu/hw/net/virtio-net.c:1008
  #6  virtio_net_receive (nc=<optimized out>, buf=0x555557386b88 "", size=102)
      at /home/doug/work/qemu/hw/net/virtio-net.c:1148
  #7  0x00005555559cad33 in nc_sendv_compat (flags=<optimized out>, iovcnt=1,
      iov=0x7fffead746d0, nc=0x55555788b340) at net/net.c:705
  #8  qemu_deliver_packet_iov (sender=<optimized out>, flags=<optimized out>,
      iov=0x7fffead746d0, iovcnt=1, opaque=0x55555788b340) at net/net.c:732
  #9  0x00005555559cd929 in qemu_net_queue_deliver (size=<optimized out>,
      data=<optimized out>, flags=<optimized out>, sender=<optimized out>,
      queue=0x55555788b550) at net/queue.c:164
  #10 qemu_net_queue_flush (queue=0x55555788b550) at net/queue.c:261

This patch is safe to revert since it's just an optimization for
virtqueue polling.  The next patch will improve the situation again
without resorting to nesting.

Reported-by: Doug Goldstein <cardoe@cardoe.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aa4f38f..f04ab7a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -88,8 +88,8 @@ struct VirtQueue
     /* Last used index value we have signalled on */
     bool signalled_used_valid;
 
-    /* Nested host->guest notification disabled counter */
-    unsigned int notification_disabled;
+    /* Notification enabled? */
+    bool notification;
 
     uint16_t queue_index;
 
@@ -202,7 +202,7 @@ static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
 static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
 {
     hwaddr pa;
-    if (vq->notification_disabled) {
+    if (!vq->notification) {
         return;
     }
     pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
@@ -211,13 +211,7 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val)
 
 void virtio_queue_set_notification(VirtQueue *vq, int enable)
 {
-    if (enable) {
-        assert(vq->notification_disabled > 0);
-        vq->notification_disabled--;
-    } else {
-        vq->notification_disabled++;
-    }
-
+    vq->notification = enable;
     if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_set_avail_event(vq, vring_avail_idx(vq));
     } else if (enable) {
@@ -1020,7 +1014,7 @@ void virtio_reset(void *opaque)
         virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
         vdev->vq[i].signalled_used = 0;
         vdev->vq[i].signalled_used_valid = false;
-        vdev->vq[i].notification_disabled = 0;
+        vdev->vq[i].notification = true;
         vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
         vdev->vq[i].inuse = 0;
     }
@@ -1831,7 +1825,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
         vdev->vq[i].vring.desc = qemu_get_be64(f);
         qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
         vdev->vq[i].signalled_used_valid = false;
-        vdev->vq[i].notification_disabled = 0;
+        vdev->vq[i].notification = true;
 
         if (vdev->vq[i].vring.desc) {
             /* XXX virtio-1 devices */
-- 
2.9.3

[Qemu-devel] [PATCH 2/2] virtio: disable notifications again after poll succeeded

[Stefan Hajnoczi]

While AioContext is in polling mode virtqueue notifications are not
necessary.  Some device virtqueue handlers enable notifications.  Make
sure they stay disabled to avoid unnecessary vmexits.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f04ab7a..34065c7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2126,6 +2126,9 @@ static bool virtio_queue_host_notifier_aio_poll(void *opaque)
     }
 
     virtio_queue_notify_aio_vq(vq);
+
+    /* In case the handler function re-enabled notifications */
+    virtio_queue_set_notification(vq, 0);
     return true;
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Doug Goldstein]

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

On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
> The virtio_queue_set_notification() nesting introduced for AioContext polling
> raised an assertion with virtio-net (even in non-polling mode).  Converting
> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> nesting fashion would be invasive and isn't worth it.
> 
> Patch 1 contains the revert to resolve the bug that Doug noticed.
> 
> Patch 2 is a less efficient but safe alternative.
> 
> Stefan Hajnoczi (2):
>   Revert "virtio: turn vq->notification into a nested counter"
>   virtio: disable notifications again after poll succeeded
> 
>  hw/virtio/virtio.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 

So I just gave this series a whirl and it fixes the assert but causes
another issue for me. While iPXE is getting a DHCP address the screen
immediately flashes over to the UEFI shell. Its like a timeout is
getting hit and just dropping me to the shell.

-- 
Doug Goldstein


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

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Michael S. Tsirkin]

On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
> > The virtio_queue_set_notification() nesting introduced for AioContext polling
> > raised an assertion with virtio-net (even in non-polling mode).  Converting
> > virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> > nesting fashion would be invasive and isn't worth it.
> > 
> > Patch 1 contains the revert to resolve the bug that Doug noticed.
> > 
> > Patch 2 is a less efficient but safe alternative.
> > 
> > Stefan Hajnoczi (2):
> >   Revert "virtio: turn vq->notification into a nested counter"
> >   virtio: disable notifications again after poll succeeded
> > 
> >  hw/virtio/virtio.c | 21 +++++++++------------
> >  1 file changed, 9 insertions(+), 12 deletions(-)
> > 
> 
> So I just gave this series a whirl and it fixes the assert but causes
> another issue for me. While iPXE is getting a DHCP address the screen
> immediately flashes over to the UEFI shell. Its like a timeout is
> getting hit and just dropping me to the shell.
> 
> -- 
> Doug Goldstein
> 

Is this just with UEFI or with seabios as well?

-- 
MST

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Stefan Hajnoczi]

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

On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
> > The virtio_queue_set_notification() nesting introduced for AioContext polling
> > raised an assertion with virtio-net (even in non-polling mode).  Converting
> > virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> > nesting fashion would be invasive and isn't worth it.
> > 
> > Patch 1 contains the revert to resolve the bug that Doug noticed.
> > 
> > Patch 2 is a less efficient but safe alternative.
> > 
> > Stefan Hajnoczi (2):
> >   Revert "virtio: turn vq->notification into a nested counter"
> >   virtio: disable notifications again after poll succeeded
> > 
> >  hw/virtio/virtio.c | 21 +++++++++------------
> >  1 file changed, 9 insertions(+), 12 deletions(-)
> > 
> 
> So I just gave this series a whirl and it fixes the assert but causes
> another issue for me. While iPXE is getting a DHCP address the screen
> immediately flashes over to the UEFI shell. Its like a timeout is
> getting hit and just dropping me to the shell.

Sounds like an separate problem.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Doug Goldstein]

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

On 1/12/17 2:05 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
>> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
>>> The virtio_queue_set_notification() nesting introduced for AioContext polling
>>> raised an assertion with virtio-net (even in non-polling mode).  Converting
>>> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
>>> nesting fashion would be invasive and isn't worth it.
>>>
>>> Patch 1 contains the revert to resolve the bug that Doug noticed.
>>>
>>> Patch 2 is a less efficient but safe alternative.
>>>
>>> Stefan Hajnoczi (2):
>>>   Revert "virtio: turn vq->notification into a nested counter"
>>>   virtio: disable notifications again after poll succeeded
>>>
>>>  hw/virtio/virtio.c | 21 +++++++++------------
>>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>>
>>
>> So I just gave this series a whirl and it fixes the assert but causes
>> another issue for me. While iPXE is getting a DHCP address the screen
>> immediately flashes over to the UEFI shell. Its like a timeout is
>> getting hit and just dropping me to the shell.
>>
>> -- 
>> Doug Goldstein
>>
> 
> Is this just with UEFI or with seabios as well?
> 

Sorry for the delay on responding. I've now tested seabios and UEFI
(using OVMF). The issue is only happening via UEFI and it works fine
with seabios.

-- 
Doug Goldstein


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

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Doug Goldstein]

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

On 1/13/17 6:02 AM, Stefan Hajnoczi wrote:
> On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
>> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
>>> The virtio_queue_set_notification() nesting introduced for AioContext polling
>>> raised an assertion with virtio-net (even in non-polling mode).  Converting
>>> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
>>> nesting fashion would be invasive and isn't worth it.
>>>
>>> Patch 1 contains the revert to resolve the bug that Doug noticed.
>>>
>>> Patch 2 is a less efficient but safe alternative.
>>>
>>> Stefan Hajnoczi (2):
>>>   Revert "virtio: turn vq->notification into a nested counter"
>>>   virtio: disable notifications again after poll succeeded
>>>
>>>  hw/virtio/virtio.c | 21 +++++++++------------
>>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>>
>>
>> So I just gave this series a whirl and it fixes the assert but causes
>> another issue for me. While iPXE is getting a DHCP address the screen
>> immediately flashes over to the UEFI shell. Its like a timeout is
>> getting hit and just dropping me to the shell.
> 
> Sounds like an separate problem.
> 
> Stefan
> 

Is there any debug output that I can provide to help troubleshoot it?
I've built 23425cc and UEFI via OVMF is able to get an IP address via
DHCP inside of iPXE. I've also taken master and only applied the first
patch in this series (the revert) and it too works. Its only when I add
the 2nd patch into the mix or don't revert out the "virtio: turn
vq->notification into a nested counter" patch that it fails.

-- 
Doug Goldstein


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

Re: [Qemu-devel] [PATCH 2/2] virtio: disable notifications again after poll succeeded

[Michael S. Tsirkin]

On Thu, Jan 12, 2017 at 11:46:11AM +0000, Stefan Hajnoczi wrote:
> While AioContext is in polling mode virtqueue notifications are not
> necessary.  Some device virtqueue handlers enable notifications.  Make
> sure they stay disabled to avoid unnecessary vmexits.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

So I'll put just the revert in today's pull request,
let's make sure this one is not causing regressions.

> ---
>  hw/virtio/virtio.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f04ab7a..34065c7 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2126,6 +2126,9 @@ static bool virtio_queue_host_notifier_aio_poll(void *opaque)
>      }
>  
>      virtio_queue_notify_aio_vq(vq);
> +
> +    /* In case the handler function re-enabled notifications */
> +    virtio_queue_set_notification(vq, 0);
>      return true;
>  }
>  
> -- 
> 2.9.3

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Richard Henderson]

On 01/12/2017 03:46 AM, Stefan Hajnoczi wrote:
> The virtio_queue_set_notification() nesting introduced for AioContext polling
> raised an assertion with virtio-net (even in non-polling mode).  Converting
> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> nesting fashion would be invasive and isn't worth it.
>
> Patch 1 contains the revert to resolve the bug that Doug noticed.
>
> Patch 2 is a less efficient but safe alternative.
>
> Stefan Hajnoczi (2):
>   Revert "virtio: turn vq->notification into a nested counter"
>   virtio: disable notifications again after poll succeeded
>
>  hw/virtio/virtio.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)

Tested-by: Richard Henderson <rth@twiddle.net>

This problem affected Alpha as well.  I tested the two patches together.


r~

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Stefan Hajnoczi]

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

On Fri, Jan 13, 2017 at 09:15:49AM -0600, Doug Goldstein wrote:
> On 1/13/17 6:02 AM, Stefan Hajnoczi wrote:
> > On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
> >> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
> >>> The virtio_queue_set_notification() nesting introduced for AioContext polling
> >>> raised an assertion with virtio-net (even in non-polling mode).  Converting
> >>> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> >>> nesting fashion would be invasive and isn't worth it.
> >>>
> >>> Patch 1 contains the revert to resolve the bug that Doug noticed.
> >>>
> >>> Patch 2 is a less efficient but safe alternative.
> >>>
> >>> Stefan Hajnoczi (2):
> >>>   Revert "virtio: turn vq->notification into a nested counter"
> >>>   virtio: disable notifications again after poll succeeded
> >>>
> >>>  hw/virtio/virtio.c | 21 +++++++++------------
> >>>  1 file changed, 9 insertions(+), 12 deletions(-)
> >>>
> >>
> >> So I just gave this series a whirl and it fixes the assert but causes
> >> another issue for me. While iPXE is getting a DHCP address the screen
> >> immediately flashes over to the UEFI shell. Its like a timeout is
> >> getting hit and just dropping me to the shell.
> > 
> > Sounds like an separate problem.
> > 
> > Stefan
> > 
> 
> Is there any debug output that I can provide to help troubleshoot it?
> I've built 23425cc and UEFI via OVMF is able to get an IP address via
> DHCP inside of iPXE. I've also taken master and only applied the first
> patch in this series (the revert) and it too works. Its only when I add
> the 2nd patch into the mix or don't revert out the "virtio: turn
> vq->notification into a nested counter" patch that it fails.

The code in Patch 2 should not be executed in your QEMU configuration,
so I wonder how Patch 2 can cause the DHCP failure.

Please verify as follows:

  $ gdb --args path/to/qemu-system-x86_64 ...
  (gdb) handle SIGUSR1 noprint nostop pass
  (gdb) handle SIGPIPE noprint nostop pass
  (gdb) b virtio_queue_host_notifier_aio_poll_begin
  (gdb) r

I predict the breakpoint will not be hit.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Doug Goldstein]

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

On 1/16/17 5:46 AM, Stefan Hajnoczi wrote:
> On Fri, Jan 13, 2017 at 09:15:49AM -0600, Doug Goldstein wrote:
>> On 1/13/17 6:02 AM, Stefan Hajnoczi wrote:
>>> On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
>>>> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
>>>>> The virtio_queue_set_notification() nesting introduced for AioContext polling
>>>>> raised an assertion with virtio-net (even in non-polling mode).  Converting
>>>>> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
>>>>> nesting fashion would be invasive and isn't worth it.
>>>>>
>>>>> Patch 1 contains the revert to resolve the bug that Doug noticed.
>>>>>
>>>>> Patch 2 is a less efficient but safe alternative.
>>>>>
>>>>> Stefan Hajnoczi (2):
>>>>>   Revert "virtio: turn vq->notification into a nested counter"
>>>>>   virtio: disable notifications again after poll succeeded
>>>>>
>>>>>  hw/virtio/virtio.c | 21 +++++++++------------
>>>>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> So I just gave this series a whirl and it fixes the assert but causes
>>>> another issue for me. While iPXE is getting a DHCP address the screen
>>>> immediately flashes over to the UEFI shell. Its like a timeout is
>>>> getting hit and just dropping me to the shell.
>>>
>>> Sounds like an separate problem.
>>>
>>> Stefan
>>>
>>
>> Is there any debug output that I can provide to help troubleshoot it?
>> I've built 23425cc and UEFI via OVMF is able to get an IP address via
>> DHCP inside of iPXE. I've also taken master and only applied the first
>> patch in this series (the revert) and it too works. Its only when I add
>> the 2nd patch into the mix or don't revert out the "virtio: turn
>> vq->notification into a nested counter" patch that it fails.
> 
> The code in Patch 2 should not be executed in your QEMU configuration,
> so I wonder how Patch 2 can cause the DHCP failure.
> 
> Please verify as follows:
> 
>   $ gdb --args path/to/qemu-system-x86_64 ...
>   (gdb) handle SIGUSR1 noprint nostop pass
>   (gdb) handle SIGPIPE noprint nostop pass
>   (gdb) b virtio_queue_host_notifier_aio_poll_begin
>   (gdb) r
> 
> I predict the breakpoint will not be hit.
> 
> Stefan
> 


Correct. That did not hit. I reapplied both patches to my current master
and it did not hit but it still dropped me to the EFI shell while iPXE
was negotiating DHCP.

I then rebased on top of the latest master
(2ccede18bd24fce5db83fef3674563a1f256717b) and applied both patches. The
iPXE issue went away and the assert issue went away.

So I will concur with you, the issue was not in either of these patches
and is now resolved.

-- 
Doug Goldstein


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

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Laszlo Ersek]

On 01/12/17 12:46, Stefan Hajnoczi wrote:
> The virtio_queue_set_notification() nesting introduced for AioContext polling
> raised an assertion with virtio-net (even in non-polling mode).  Converting
> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> nesting fashion would be invasive and isn't worth it.
> 
> Patch 1 contains the revert to resolve the bug that Doug noticed.
> 
> Patch 2 is a less efficient but safe alternative.
> 
> Stefan Hajnoczi (2):
>   Revert "virtio: turn vq->notification into a nested counter"
>   virtio: disable notifications again after poll succeeded
> 
>  hw/virtio/virtio.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 

I confirm that with current master
(2ccede18bd24fce5db83fef3674563a1f256717b), my TCG aarch64 guest,
running ArmVirtQemu UEFI firmware, crashes with the following assertion
failure:

qemu-system-aarch64: .../hw/virtio/virtio.c:215:
virtio_queue_set_notification: Assertion `vq->notification_disabled > 0'
failed.

This guest does not use iPXE's UEFI SNP driver for virtio-net, instead
it uses OVMF's own, built-in VirtioNetDxe driver.

With both patches applied, everything works fine. The assertion failure
is gone, and I could ping a public host from the UEFI shell command line.

Tested-by: Laszlo Ersek <lersek@redhat.com>

I'm unsure if my use case covers polling mode, so it might be prudent to
add the tag to patch #1 only. I set the breakpoint that you gave Doug
(using "virsh start --paused" + attaching GDB to the running QEMU
process, before the firmware got any chance to execute), and the
breakpoint (virtio_queue_host_notifier_aio_poll_begin) didn't fire
during the test.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Michael S. Tsirkin]

On Mon, Jan 16, 2017 at 04:03:28PM -0500, Doug Goldstein wrote:
> On 1/16/17 5:46 AM, Stefan Hajnoczi wrote:
> > On Fri, Jan 13, 2017 at 09:15:49AM -0600, Doug Goldstein wrote:
> >> On 1/13/17 6:02 AM, Stefan Hajnoczi wrote:
> >>> On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
> >>>> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
> >>>>> The virtio_queue_set_notification() nesting introduced for AioContext polling
> >>>>> raised an assertion with virtio-net (even in non-polling mode).  Converting
> >>>>> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> >>>>> nesting fashion would be invasive and isn't worth it.
> >>>>>
> >>>>> Patch 1 contains the revert to resolve the bug that Doug noticed.
> >>>>>
> >>>>> Patch 2 is a less efficient but safe alternative.
> >>>>>
> >>>>> Stefan Hajnoczi (2):
> >>>>>   Revert "virtio: turn vq->notification into a nested counter"
> >>>>>   virtio: disable notifications again after poll succeeded
> >>>>>
> >>>>>  hw/virtio/virtio.c | 21 +++++++++------------
> >>>>>  1 file changed, 9 insertions(+), 12 deletions(-)
> >>>>>
> >>>>
> >>>> So I just gave this series a whirl and it fixes the assert but causes
> >>>> another issue for me. While iPXE is getting a DHCP address the screen
> >>>> immediately flashes over to the UEFI shell. Its like a timeout is
> >>>> getting hit and just dropping me to the shell.
> >>>
> >>> Sounds like an separate problem.
> >>>
> >>> Stefan
> >>>
> >>
> >> Is there any debug output that I can provide to help troubleshoot it?
> >> I've built 23425cc and UEFI via OVMF is able to get an IP address via
> >> DHCP inside of iPXE. I've also taken master and only applied the first
> >> patch in this series (the revert) and it too works. Its only when I add
> >> the 2nd patch into the mix or don't revert out the "virtio: turn
> >> vq->notification into a nested counter" patch that it fails.
> > 
> > The code in Patch 2 should not be executed in your QEMU configuration,
> > so I wonder how Patch 2 can cause the DHCP failure.
> > 
> > Please verify as follows:
> > 
> >   $ gdb --args path/to/qemu-system-x86_64 ...
> >   (gdb) handle SIGUSR1 noprint nostop pass
> >   (gdb) handle SIGPIPE noprint nostop pass
> >   (gdb) b virtio_queue_host_notifier_aio_poll_begin
> >   (gdb) r
> > 
> > I predict the breakpoint will not be hit.
> > 
> > Stefan
> > 
> 
> 
> Correct. That did not hit. I reapplied both patches to my current master
> and it did not hit but it still dropped me to the EFI shell while iPXE
> was negotiating DHCP.
> 
> I then rebased on top of the latest master
> (2ccede18bd24fce5db83fef3674563a1f256717b) and applied both patches. The
> iPXE issue went away and the assert issue went away.
> 
> So I will concur with you, the issue was not in either of these patches
> and is now resolved.
> 
> -- 
> Doug Goldstein
> 




Thanks for the testing, will merge these shortly!

-- 
MST

Re: [Qemu-devel] [PATCH 0/2] virtio: revert virtio_queue_set_notification() nesting

[Stefan Hajnoczi]

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

On Mon, Jan 16, 2017 at 04:03:28PM -0500, Doug Goldstein wrote:
> On 1/16/17 5:46 AM, Stefan Hajnoczi wrote:
> > On Fri, Jan 13, 2017 at 09:15:49AM -0600, Doug Goldstein wrote:
> >> On 1/13/17 6:02 AM, Stefan Hajnoczi wrote:
> >>> On Thu, Jan 12, 2017 at 10:57:53AM -0600, Doug Goldstein wrote:
> >>>> On 1/12/17 5:46 AM, Stefan Hajnoczi wrote:
> >>>>> The virtio_queue_set_notification() nesting introduced for AioContext polling
> >>>>> raised an assertion with virtio-net (even in non-polling mode).  Converting
> >>>>> virtio-net and virtio-crypto to use virtio_queue_set_notification() in a
> >>>>> nesting fashion would be invasive and isn't worth it.
> >>>>>
> >>>>> Patch 1 contains the revert to resolve the bug that Doug noticed.
> >>>>>
> >>>>> Patch 2 is a less efficient but safe alternative.
> >>>>>
> >>>>> Stefan Hajnoczi (2):
> >>>>>   Revert "virtio: turn vq->notification into a nested counter"
> >>>>>   virtio: disable notifications again after poll succeeded
> >>>>>
> >>>>>  hw/virtio/virtio.c | 21 +++++++++------------
> >>>>>  1 file changed, 9 insertions(+), 12 deletions(-)
> >>>>>
> >>>>
> >>>> So I just gave this series a whirl and it fixes the assert but causes
> >>>> another issue for me. While iPXE is getting a DHCP address the screen
> >>>> immediately flashes over to the UEFI shell. Its like a timeout is
> >>>> getting hit and just dropping me to the shell.
> >>>
> >>> Sounds like an separate problem.
> >>>
> >>> Stefan
> >>>
> >>
> >> Is there any debug output that I can provide to help troubleshoot it?
> >> I've built 23425cc and UEFI via OVMF is able to get an IP address via
> >> DHCP inside of iPXE. I've also taken master and only applied the first
> >> patch in this series (the revert) and it too works. Its only when I add
> >> the 2nd patch into the mix or don't revert out the "virtio: turn
> >> vq->notification into a nested counter" patch that it fails.
> > 
> > The code in Patch 2 should not be executed in your QEMU configuration,
> > so I wonder how Patch 2 can cause the DHCP failure.
> > 
> > Please verify as follows:
> > 
> >   $ gdb --args path/to/qemu-system-x86_64 ...
> >   (gdb) handle SIGUSR1 noprint nostop pass
> >   (gdb) handle SIGPIPE noprint nostop pass
> >   (gdb) b virtio_queue_host_notifier_aio_poll_begin
> >   (gdb) r
> > 
> > I predict the breakpoint will not be hit.
> > 
> > Stefan
> > 
> 
> 
> Correct. That did not hit. I reapplied both patches to my current master
> and it did not hit but it still dropped me to the EFI shell while iPXE
> was negotiating DHCP.
> 
> I then rebased on top of the latest master
> (2ccede18bd24fce5db83fef3674563a1f256717b) and applied both patches. The
> iPXE issue went away and the assert issue went away.
> 
> So I will concur with you, the issue was not in either of these patches
> and is now resolved.

Great, thanks for confirming!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]