qemu-commits
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-commits] [qemu/qemu] 3009ed: vhost-user: fix VHOST_USER_ADD/REM_ME


From: Peter Maydell
Subject: [Qemu-commits] [qemu/qemu] 3009ed: vhost-user: fix VHOST_USER_ADD/REM_MEM_REG truncation
Date: Tue, 17 Nov 2020 06:14:15 -0800

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 3009edff8192991293fe9e2b50b0d90db83c4a89
      
https://github.com/qemu/qemu/commit/3009edff8192991293fe9e2b50b0d90db83c4a89
  Author: Stefan Hajnoczi <stefanha@redhat.com>
  Date:   2020-11-12 (Thu, 12 Nov 2020)

  Changed paths:
    M contrib/libvhost-user/libvhost-user.h
    M docs/interop/vhost-user.rst
    M hw/virtio/vhost-user.c

  Log Message:
  -----------
  vhost-user: fix VHOST_USER_ADD/REM_MEM_REG truncation

QEMU currently truncates the mmap_offset field when sending
VHOST_USER_ADD_MEM_REG and VHOST_USER_REM_MEM_REG messages. The struct
layout looks like this:

  typedef struct VhostUserMemoryRegion {
      uint64_t guest_phys_addr;
      uint64_t memory_size;
      uint64_t userspace_addr;
      uint64_t mmap_offset;
  } VhostUserMemoryRegion;

  typedef struct VhostUserMemRegMsg {
      uint32_t padding;
      /* WARNING: there is a 32-bit hole here! */
      VhostUserMemoryRegion region;
  } VhostUserMemRegMsg;

The payload size is calculated as follows when sending the message in
hw/virtio/vhost-user.c:

  msg->hdr.size = sizeof(msg->payload.mem_reg.padding) +
      sizeof(VhostUserMemoryRegion);

This calculation produces an incorrect result of only 36 bytes.
sizeof(VhostUserMemRegMsg) is actually 40 bytes.

The consequence of this is that the final field, mmap_offset, is
truncated. This breaks x86_64 TCG guests on s390 hosts. Other guest/host
combinations may get lucky if either of the following holds:
1. The guest memory layout does not need mmap_offset != 0.
2. The host is little-endian and mmap_offset <= 0xffffffff so the
   truncation has no effect.

Fix this by extending the existing 32-bit padding field to 64-bit. Now
the padding reflects the actual compiler padding. This can be verified
using pahole(1).

Also document the layout properly in the vhost-user specification.  The
vhost-user spec did not document the exact layout. It would be
impossible to implement the spec without looking at the QEMU source
code.

Existing vhost-user frontends and device backends continue to work after
this fix has been applied. The only change in the wire protocol is that
QEMU now sets hdr.size to 40 instead of 36. If a vhost-user
implementation has a hardcoded size check for 36 bytes, then it will
fail with new QEMUs. Both QEMU and DPDK/SPDK don't check the exact
payload size, so they continue to work.

Fixes: f1aeb14b0809e313c74244d838645ed25e85ea63 ("Transmit vhost-user memory 
regions individually")
Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20201109174355.1069147-1-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Fixes: f1aeb14b0809 ("Transmit vhost-user memory regions individually")
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>


  Commit: e5e856c1eb4b85fe96472cf7aed48e336fe5ce74
      
https://github.com/qemu/qemu/commit/e5e856c1eb4b85fe96472cf7aed48e336fe5ce74
  Author: Stefan Hajnoczi <stefanha@redhat.com>
  Date:   2020-11-12 (Thu, 12 Nov 2020)

  Changed paths:
    M block/export/meson.build
    M configure
    M meson.build
    M meson_options.txt

  Log Message:
  -----------
  meson: move vhost_user_blk_server to meson.build

The --enable/disable-vhost-user-blk-server options were implemented in
./configure. There has been confusion about them and part of the problem
is that the shell syntax used for setting the default value is not easy
to read. Move the option over to meson where the conditions are easier
to understand:

  have_vhost_user_blk_server = (targetos == 'linux')

  if get_option('vhost_user_blk_server').enabled()
      if targetos != 'linux'
          error('vhost_user_blk_server requires linux')
      endif
  elif get_option('vhost_user_blk_server').disabled() or not have_system
      have_vhost_user_blk_server = false
  endif

This patch does not change behavior.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20201110171121.1265142-2-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>


  Commit: eb6a388624ec40d9be5c965ad05531ef1b5f4eb3
      
https://github.com/qemu/qemu/commit/eb6a388624ec40d9be5c965ad05531ef1b5f4eb3
  Author: Stefan Hajnoczi <stefanha@redhat.com>
  Date:   2020-11-12 (Thu, 12 Nov 2020)

  Changed paths:
    M meson.build

  Log Message:
  -----------
  vhost-user-blk-server: depend on CONFIG_VHOST_USER

I interpreted CONFIG_VHOST_USER as controlling only QEMU's vhost-user
device frontends. However, virtiofsd and contrib/ vhost-user device
backends are also controlled by CONFIG_VHOST_USER. Make the
vhost-user-blk server depend on CONFIG_VHOST_USER for consistency.

Now the following error is printed when the vhost-user-blk server is
enabled without CONFIG_VHOST_USER:

  $ ./configure --disable-vhost-user --enable-vhost-user-blk ...
  ../meson.build:761:8: ERROR: Problem encountered: vhost_user_blk_server 
requires vhost-user support

Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Suggested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20201110171121.1265142-3-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>


  Commit: d88618f717f948d97ae6f4401b014e30d52c68ec
      
https://github.com/qemu/qemu/commit/d88618f717f948d97ae6f4401b014e30d52c68ec
  Author: Stefan Hajnoczi <stefanha@redhat.com>
  Date:   2020-11-12 (Thu, 12 Nov 2020)

  Changed paths:
    M configure

  Log Message:
  -----------
  configure: mark vhost-user Linux-only

The vhost-user protocol uses the Linux eventfd feature and is typically
connected to Linux kvm.ko ioeventfd and irqfd file descriptors. The
protocol specification in docs/interop/vhost-user.rst does not describe
how platforms without eventfd support work.

The QEMU vhost-user devices compile on other POSIX host operating
systems because eventfd usage is abstracted in QEMU. The libvhost-user
programs in contrib/ do not compile but we failed to notice since they
are not built by default.

Make it clear that vhost-user is only supported on Linux for the time
being. If someone wishes to support it on other platforms then the
details can be added to vhost-user.rst and CI jobs can test the feature
to prevent bitrot.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20201110171121.1265142-4-stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>


  Commit: 727a06326c9ebf4ad8ed80eb533278bc60202804
      
https://github.com/qemu/qemu/commit/727a06326c9ebf4ad8ed80eb533278bc60202804
  Author: Philippe Mathieu-Daudé <philmd@redhat.com>
  Date:   2020-11-12 (Thu, 12 Nov 2020)

  Changed paths:
    M hw/i386/acpi-build.c

  Log Message:
  -----------
  hw/i386/acpi-build: Fix maybe-uninitialized error when ACPI hotplug off

GCC 9.3.0 thinks that 'method' can be left uninitialized. This code
is already in the "if (bsel || pcihp_bridge_en)" block statement,
but it isn't smart enough to figure it out.

Restrict the code to be used only in the "if (bsel || pcihp_bridge_en)"
block statement to fix (on Ubuntu):

  ../hw/i386/acpi-build.c: In function 'build_append_pci_bus_devices':
  ../hw/i386/acpi-build.c:496:9: error: 'method' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]
    496 |         aml_append(parent_scope, method);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors

Fixes: df4008c9c59 ("piix4: don't reserve hw resources when hotplug is off 
globally")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20201107194045.438027-1-philmd@redhat.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


  Commit: 5fd6921cccdbc1428c888d451026ee4fd152c936
      
https://github.com/qemu/qemu/commit/5fd6921cccdbc1428c888d451026ee4fd152c936
  Author: AlexChen <alex.chen@huawei.com>
  Date:   2020-11-17 (Tue, 17 Nov 2020)

  Changed paths:
    M contrib/libvhost-user/libvhost-user.c

  Log Message:
  -----------
  contrib/libvhost-user: Fix bad printf format specifiers

We should use printf format specifier "%u" instead of "%d" for
argument of type "unsigned int".

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Alex Chen <alex.chen@huawei.com>
Message-Id: <5FA28106.6000901@huawei.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


  Commit: 91010f0407a07caeacb11037bb5b493bab7ce203
      
https://github.com/qemu/qemu/commit/91010f0407a07caeacb11037bb5b493bab7ce203
  Author: AlexChen <alex.chen@huawei.com>
  Date:   2020-11-17 (Tue, 17 Nov 2020)

  Changed paths:
    M contrib/vhost-user-blk/vhost-user-blk.c
    M contrib/vhost-user-scsi/vhost-user-scsi.c

  Log Message:
  -----------
  vhost-user-blk/scsi: Fix broken error handling for socket call

When socket() fails, it returns -1, 0 is the normal return value and should not 
return error.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: AlexChen <alex.chen@huawei.com>
Message-Id: <5F9A5B48.9030509@huawei.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


  Commit: 1c7ab0930a3e02e6e54722c20b6b586364f387a7
      
https://github.com/qemu/qemu/commit/1c7ab0930a3e02e6e54722c20b6b586364f387a7
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2020-11-17 (Tue, 17 Nov 2020)

  Changed paths:
    M block/export/meson.build
    M configure
    M contrib/libvhost-user/libvhost-user.c
    M contrib/libvhost-user/libvhost-user.h
    M contrib/vhost-user-blk/vhost-user-blk.c
    M contrib/vhost-user-scsi/vhost-user-scsi.c
    M docs/interop/vhost-user.rst
    M hw/virtio/vhost-user.c
    M meson.build
    M meson_options.txt

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging

pc,vhost: fixes

Fixes all over the place.

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

# gpg: Signature made Tue 17 Nov 2020 09:17:12 GMT
# gpg:                using RSA key 5D09FD0871C8F85B94CA8A0D281F0DB8D28D5469
# gpg:                issuer "mst@redhat.com"
# gpg: Good signature from "Michael S. Tsirkin <mst@kernel.org>" [full]
# gpg:                 aka "Michael S. Tsirkin <mst@redhat.com>" [full]
# Primary key fingerprint: 0270 606B 6F3C DF3D 0B17  0970 C350 3912 AFBE 8E67
#      Subkey fingerprint: 5D09 FD08 71C8 F85B 94CA  8A0D 281F 0DB8 D28D 5469

* remotes/mst/tags/for_upstream:
  vhost-user-blk/scsi: Fix broken error handling for socket call
  contrib/libvhost-user: Fix bad printf format specifiers
  hw/i386/acpi-build: Fix maybe-uninitialized error when ACPI hotplug off
  configure: mark vhost-user Linux-only
  vhost-user-blk-server: depend on CONFIG_VHOST_USER
  meson: move vhost_user_blk_server to meson.build
  vhost-user: fix VHOST_USER_ADD/REM_MEM_REG truncation

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

# Conflicts:
#       meson.build


Compare: https://github.com/qemu/qemu/compare/48aa8f0ac536...1c7ab0930a3e



reply via email to

[Prev in Thread] Current Thread [Next in Thread]