[Qemu-devel] [PULL 0/3] virtio, pci: fixes

[Qemu-devel] [PULL 0/3] virtio, pci: fixes

[Michael S. Tsirkin]

The following changes since commit df9046363220e57d45818312759b954c033c58ab:

  Update version for v2.9.0-rc2 release (2017-03-28 19:11:16 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to b8adbc657802482e4da1767bf983ebfdf9bfe9fc:

  virtio: fix vring_align() on 64-bit windows (2017-03-29 02:35:24 +0300)

----------------------------------------------------------------
virtio, pci: fixes

More fixes for 2.9.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Alexey Kardashevskiy (1):
      pci: Add missing drop of bus master AS reference

Andrew Baumann (1):
      virtio: fix vring_align() on 64-bit windows

Halil Pasic (1):
      event_notifier: prevent accidental use after close

 include/hw/virtio/virtio.h  | 2 +-
 hw/pci/pci.c                | 2 ++
 util/event_notifier-posix.c | 2 ++
 util/event_notifier-win32.c | 1 +
 4 files changed, 6 insertions(+), 1 deletion(-)

[Qemu-devel] [PULL 1/3] event_notifier: prevent accidental use after close

[Michael S. Tsirkin]

From: Halil Pasic <pasic@linux.vnet.ibm.com>

Let's set the handles to the underlying facilities to their extremal
value so no accidental misuse can happen, and to make it obvious that the
notifier is dysfunctional. E.g. if we just close an fd but do not touch
the int holding the fd eventually a read/write could succeed again when
the fd gets reused, and corrupt the file addressed by the fd.

Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 util/event_notifier-posix.c | 2 ++
 util/event_notifier-win32.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/util/event_notifier-posix.c b/util/event_notifier-posix.c
index 7e40252..acdbe3b 100644
--- a/util/event_notifier-posix.c
+++ b/util/event_notifier-posix.c
@@ -81,8 +81,10 @@ void event_notifier_cleanup(EventNotifier *e)
 {
     if (e->rfd != e->wfd) {
         close(e->rfd);
+        e->rfd = -1;
     }
     close(e->wfd);
+    e->wfd = -1;
 }
 
 int event_notifier_get_fd(const EventNotifier *e)
diff --git a/util/event_notifier-win32.c b/util/event_notifier-win32.c
index 519fb59..62c53b0 100644
--- a/util/event_notifier-win32.c
+++ b/util/event_notifier-win32.c
@@ -25,6 +25,7 @@ int event_notifier_init(EventNotifier *e, int active)
 void event_notifier_cleanup(EventNotifier *e)
 {
     CloseHandle(e->event);
+    e->event = NULL;
 }
 
 HANDLE event_notifier_get_handle(EventNotifier *e)
-- 
MST

[Qemu-devel] [PULL 2/3] pci: Add missing drop of bus master AS reference

[Michael S. Tsirkin]

From: Alexey Kardashevskiy <aik@ozlabs.ru>

The recent introduction of a bus master container added
memory_region_add_subregion() into the PCI device registering path but
missed memory_region_del_subregion() in the unregistering path leaving
a reference to the root memory region of the new container.

This adds missing memory_region_del_subregion().

Fixes: 3716d5902d743 ("pci: introduce a bus master container")
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e6b08e1..bd8043c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
     pci_config_free(pci_dev);
 
+    memory_region_del_subregion(&pci_dev->bus_master_container_region,
+                                &pci_dev->bus_master_enable_region);
     address_space_destroy(&pci_dev->bus_master_as);
 }
 
-- 
MST

[Qemu-devel] [PULL 3/3] virtio: fix vring_align() on 64-bit windows

[Michael S. Tsirkin]

From: Andrew Baumann <Andrew.Baumann@microsoft.com>

long is 32-bits on 64-bit windows, which caused the top half of the
address to be truncated; this patch changes it to use the
QEMU_ALIGN_UP macro which does not suffer the same problem

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/virtio/virtio.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 15efcf2..7b6edba 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -34,7 +34,7 @@ struct VirtQueue;
 static inline hwaddr vring_align(hwaddr addr,
                                              unsigned long align)
 {
-    return (addr + align - 1) & ~(align - 1);
+    return QEMU_ALIGN_UP(addr, align);
 }
 
 typedef struct VirtQueue VirtQueue;
-- 
MST

Re: [Qemu-devel] [PULL 0/3] virtio, pci: fixes

[Peter Maydell]

On 29 March 2017 at 02:18, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit df9046363220e57d45818312759b954c033c58ab:
>
>   Update version for v2.9.0-rc2 release (2017-03-28 19:11:16 +0100)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to b8adbc657802482e4da1767bf983ebfdf9bfe9fc:
>
>   virtio: fix vring_align() on 64-bit windows (2017-03-29 02:35:24 +0300)
>
> ----------------------------------------------------------------
> virtio, pci: fixes
>
> More fixes for 2.9.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------
> Alexey Kardashevskiy (1):
>       pci: Add missing drop of bus master AS reference
>
> Andrew Baumann (1):
>       virtio: fix vring_align() on 64-bit windows
>
> Halil Pasic (1):
>       event_notifier: prevent accidental use after close
>
>  include/hw/virtio/virtio.h  | 2 +-
>  hw/pci/pci.c                | 2 ++
>  util/event_notifier-posix.c | 2 ++
>  util/event_notifier-win32.c | 1 +
>  4 files changed, 6 insertions(+), 1 deletion(-)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 2/3] pci: Add missing drop of bus master AS reference

[John Snow]

On 03/28/2017 09:18 PM, Michael S. Tsirkin wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> The recent introduction of a bus master container added
> memory_region_add_subregion() into the PCI device registering path but
> missed memory_region_del_subregion() in the unregistering path leaving
> a reference to the root memory region of the new container.
> 
> This adds missing memory_region_del_subregion().
> 
> Fixes: 3716d5902d743 ("pci: introduce a bus master container")
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci/pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e6b08e1..bd8043c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>      pci_dev->bus->devices[pci_dev->devfn] = NULL;
>      pci_config_free(pci_dev);
>  
> +    memory_region_del_subregion(&pci_dev->bus_master_container_region,
> +                                &pci_dev->bus_master_enable_region);
>      address_space_destroy(&pci_dev->bus_master_as);
>  }
>  
> 

This causes a regression in iotest 051:

051 7s ... - output mismatch (see 051.out.bad)
--- /home/bos/jhuston/src/qemu/tests/qemu-iotests/051.pc.out	2017-03-27
12:30:59.945613766 -0400
+++ 051.out.bad	2017-03-30 19:32:20.708131152 -0400
@@ -133,7 +133,10 @@

 Testing: -drive if=virtio
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
+(qemu) QEMU_PROG: /home/bos/jhuston/src/qemu/memory.c:2078:
memory_region_del_subregion: Assertion `subregion->container == mr' failed.
+./common.config: line 109: 27667 Aborted                 (core dumped)
( if [ -n "${QEMU_NEED_PID}" ]; then
+    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )

 Testing: -drive if=none,id=disk -device ide-cd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
Failures: 051
Failed 1 of 1 tests

Re: [Qemu-devel] [PULL 2/3] pci: Add missing drop of bus master AS reference

[Max Reitz]

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

On 29.03.2017 03:18, Michael S. Tsirkin wrote:
> From: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> The recent introduction of a bus master container added
> memory_region_add_subregion() into the PCI device registering path but
> missed memory_region_del_subregion() in the unregistering path leaving
> a reference to the root memory region of the new container.
> 
> This adds missing memory_region_del_subregion().
> 
> Fixes: 3716d5902d743 ("pci: introduce a bus master container")
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci/pci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e6b08e1..bd8043c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>      pci_dev->bus->devices[pci_dev->devfn] = NULL;
>      pci_config_free(pci_dev);
>  
> +    memory_region_del_subregion(&pci_dev->bus_master_container_region,
> +                                &pci_dev->bus_master_enable_region);
>      address_space_destroy(&pci_dev->bus_master_as);
>  }

I have yet to investigate how and why, but this may result in a failed
assertion:

$ qemu-system-x86_64 -drive if=virtio
qemu-system-x86_64: ./memory.c:2078: memory_region_del_subregion:
Assertion `subregion->container == mr' failed.
[1]    15352 abort (core dumped)  qemu-system-x86_64 -drive if=virtio

I guess the subregion has not yet been added in this case and therefore
should not be deleted...?

(Caught by iotest 051.)

Max


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