[Qemu-commits] [qemu/qemu] ace33a: hw/xen: Clarify (lack of) error handling in transa...

[Richard Henderson]

  Branch: refs/heads/staging
  Home:   https://github.com/qemu/qemu
  Commit: ace33a0e5a5191f79899397adb766dda7208cb93
      
https://github.com/qemu/qemu/commit/ace33a0e5a5191f79899397adb766dda7208cb93
  Author: David Woodhouse <dwmw@amazon.co.uk>
  Date:   2023-08-01 (Tue, 01 Aug 2023)

  Changed paths:
    M hw/i386/kvm/xenstore_impl.c

  Log Message:
  -----------
  hw/xen: Clarify (lack of) error handling in transaction_commit()

Coverity was unhappy (CID 1508359) because we didn't check the return of
init_walk_op() in transaction_commit(), despite doing so at every other
call site.

Strictly speaking, this is a false positive since it can never fail. It
only fails for invalid user input (transaction ID or path), and both of
those are hard-coded to known sane values in this invocation.

But Coverity doesn't know that, and neither does the casual reader of the
code.

Returning an error here would be weird, since the transaction *is*
committed by this point; all the walk_op is doing is firing watches on
the newly-committed changed nodes. So make it a g_assert(!ret), since
it really should never happen.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Message-Id: <20076888f6bdf06a65aafc5cf954260965d45b97.camel@infradead.org>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>


  Commit: aa36243514a777f76c8b8a19b1f8a71f27ec6c78
      
https://github.com/qemu/qemu/commit/aa36243514a777f76c8b8a19b1f8a71f27ec6c78
  Author: Anthony PERARD <anthony.perard@citrix.com>
  Date:   2023-08-01 (Tue, 01 Aug 2023)

  Changed paths:
    M hw/block/xen-block.c

  Log Message:
  -----------
  xen-block: Avoid leaks on new error path

Commit 189829399070 ("xen-block: Use specific blockdev driver")
introduced a new error path, without taking care of allocated
resources.

So only allocate the qdicts after the error check, and free both
`filename` and `driver` when we are about to return and thus taking
care of both success and error path.

Coverity only spotted the leak of qdicts (*_layer variables).

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: Coverity CID 1508722, 1398649
Fixes: 189829399070 ("xen-block: Use specific blockdev driver")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20230704171819.42564-1-anthony.perard@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>


  Commit: f4f71363fcdb1092ff64d2bba6f9af39570c2f2b
      
https://github.com/qemu/qemu/commit/f4f71363fcdb1092ff64d2bba6f9af39570c2f2b
  Author: Anthony PERARD <anthony.perard@citrix.com>
  Date:   2023-08-01 (Tue, 01 Aug 2023)

  Changed paths:
    M util/thread-pool.c

  Log Message:
  -----------
  thread-pool: signal "request_cond" while locked

thread_pool_free() might have been called on the `pool`, which would
be a reason for worker_thread() to quit. In this case,
`pool->request_cond` is been destroyed.

If worker_thread() didn't managed to signal `request_cond` before it
been destroyed by thread_pool_free(), we got:
    util/qemu-thread-posix.c:198: qemu_cond_signal: Assertion 
`cond->initialized' failed.

One backtrace:
    __GI___assert_fail (assertion=0x55555614abcb "cond->initialized", 
file=0x55555614ab88 "util/qemu-thread-posix.c", line=198,
        function=0x55555614ad80 <__PRETTY_FUNCTION__.17104> "qemu_cond_signal") 
at assert.c:101
    qemu_cond_signal (cond=0x7fffb800db30) at util/qemu-thread-posix.c:198
    worker_thread (opaque=0x7fffb800dab0) at util/thread-pool.c:129
    qemu_thread_start (args=0x7fffb8000b20) at util/qemu-thread-posix.c:505
    start_thread (arg=<optimized out>) at pthread_create.c:486

Reported here:
    https://lore.kernel.org/all/ZJwoK50FcnTSfFZ8@MacBook-Air-de-Roger.local/T/#u

To avoid issue, keep lock while sending a signal to `request_cond`.

Fixes: 900fa208f506 ("thread-pool: replace semaphore with condition variable")
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20230714152720.5077-1-anthony.perard@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>


  Commit: bcb40db010517120dfffccc77cef9e4fcd3235fa
      
https://github.com/qemu/qemu/commit/bcb40db010517120dfffccc77cef9e4fcd3235fa
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2023-08-01 (Tue, 01 Aug 2023)

  Changed paths:
    M hw/arm/xen_arm.c
    M hw/i386/xen/xen-hvm.c
    M hw/xen/xen-hvm-common.c
    M include/hw/xen/xen-hvm-common.h

  Log Message:
  -----------
  xen: Don't pass MemoryListener around by value

Coverity points out (CID 1513106, 1513107) that MemoryListener is a
192 byte struct which we are passing around by value.  Switch to
passing a const pointer into xen_register_ioreq() and then to
xen_do_ioreq_register().  We can also make the file-scope
MemoryListener variables const, since nothing changes them.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230718101057.1110979-1-peter.maydell@linaro.org>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>


  Commit: 856ca10f9ce1fcffeab18546b36a64f79017c905
      
https://github.com/qemu/qemu/commit/856ca10f9ce1fcffeab18546b36a64f79017c905
  Author: Olaf Hering <olaf@aepfle.de>
  Date:   2023-08-01 (Tue, 01 Aug 2023)

  Changed paths:
    M hw/i386/xen/xen_platform.c

  Log Message:
  -----------
  xen-platform: do full PCI reset during unplug of IDE devices

The IDE unplug function needs to reset the entire PCI device, to make
sure all state is initialized to defaults. This is done by calling
pci_device_reset, which resets not only the chip specific registers, but
also all PCI state. This fixes "unplug" in a Xen HVM domU with the
modular legacy xenlinux PV drivers.

Commit ee358e919e38 ("hw/ide/piix: Convert reset handler to
DeviceReset") changed the way how the the disks are unplugged. Prior
this commit the PCI device remained unchanged. After this change,
piix_ide_reset is exercised after the "unplug" command, which was not
the case prior that commit. This function resets the command register.
As a result the ata_piix driver inside the domU will see a disabled PCI
device. The generic PCI code will reenable the PCI device. On the qemu
side, this runs pci_default_write_config/pci_update_mappings. Here a
changed address is returned by pci_bar_address, this is the address
which was truncated in piix_ide_reset. In case of a Xen HVM domU, the
address changes from 0xc120 to 0xc100. This truncation was a bug in
piix_ide_reset, which was fixed in commit 230dfd9257 ("hw/ide/piix:
properly initialize the BMIBA register"). If pci_xen_ide_unplug had used
pci_device_reset, the PCI registers would have been properly reset, and
commit ee358e919e38 would have not introduced a regression for this
specific domU environment.

While the unplug is supposed to hide the IDE disks, the changed BMIBA
address broke the UHCI device. In case the domU has an USB tablet
configured, to recive absolute pointer coordinates for the GUI, it will
cause a hang during device discovery of the partly discovered USB hid
device. Reading the USBSTS word size register will fail. The access ends
up in the QEMU piix-bmdma device, instead of the expected uhci device.
Here a byte size request is expected, and a value of ~0 is returned. As
a result the UCHI driver sees an error state in the register, and turns
off the UHCI controller.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Paul Durrant <paul@xen.org>
Message-Id: <20230720072950.20198-1-olaf@aepfle.de>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>


  Commit: 38a6de80b917b2a822cff0e38d83563ab401c890
      
https://github.com/qemu/qemu/commit/38a6de80b917b2a822cff0e38d83563ab401c890
  Author: Richard Henderson <richard.henderson@linaro.org>
  Date:   2023-08-01 (Tue, 01 Aug 2023)

  Changed paths:
    M hw/arm/xen_arm.c
    M hw/block/xen-block.c
    M hw/i386/kvm/xenstore_impl.c
    M hw/i386/xen/xen-hvm.c
    M hw/i386/xen/xen_platform.c
    M hw/xen/xen-hvm-common.c
    M include/hw/xen/xen-hvm-common.h
    M util/thread-pool.c

  Log Message:
  -----------
  Merge tag 'pull-xen-20230801' of 
https://xenbits.xen.org/git-http/people/aperard/qemu-dm into staging

Misc fixes, for thread-pool, xen, and xen-emulate

* fix an access to `request_cond` QemuCond in thread-pool
* fix issue with PCI devices when unplugging IDE devices in Xen guest
* several fixes for issues pointed out by Coverity

# -----BEGIN PGP SIGNATURE-----
#
# iQEzBAABCgAdFiEE+AwAYwjiLP2KkueYDPVXL9f7Va8FAmTI0qcACgkQDPVXL9f7
# Va9DVAgAlKGhkOhLiOtlwL05iI8/YiT7ekCSoMTWYO8iIyLCKGLVU5yyOAqYiAJD
# dEgXNZOeulcLkn3LDCQYtZJmD42sUHv/xmdJ06zJ9jRvtLAJp5wuwaU9JFDhJPsG
# eYPGBMdO39meUmgQe3X27CEKtht5Z8M9ZABdTLAxMyPANEzFmT7ni9wd/8Uc+tWg
# BMsXQco8e1GSiBUjSky5nSW248FVDIyjkaYWk1poXEfm4gPQ0jf9gg/biEj44cSH
# Tdz6de1kTwJfuYR+h+COQOrq0fUfz4SyVocKvtycZhKGXIqL74DiIGatxdVOwV9Y
# NJ8g4oKDgDeMBZ66kXnTX4Y9nzhPpA==
# =CdlZ
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue 01 Aug 2023 02:38:47 AM PDT
# gpg:                using RSA key F80C006308E22CFD8A92E7980CF5572FD7FB55AF
# gpg: Good signature from "Anthony PERARD <anthony.perard@gmail.com>" [unknown]
# gpg:                 aka "Anthony PERARD <anthony.perard@citrix.com>" 
[unknown]
# gpg: WARNING: This key is not certified with a trusted signature!
# gpg:          There is no indication that the signature belongs to the owner.
# Primary key fingerprint: 5379 2F71 024C 600F 778A  7161 D8D5 7199 DF83 42C8
#      Subkey fingerprint: F80C 0063 08E2 2CFD 8A92  E798 0CF5 572F D7FB 55AF

* tag 'pull-xen-20230801' of 
https://xenbits.xen.org/git-http/people/aperard/qemu-dm:
  xen-platform: do full PCI reset during unplug of IDE devices
  xen: Don't pass MemoryListener around by value
  thread-pool: signal "request_cond" while locked
  xen-block: Avoid leaks on new error path
  hw/xen: Clarify (lack of) error handling in transaction_commit()

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Compare: https://github.com/qemu/qemu/compare/802341823f17...38a6de80b917