| Message ID | cover.1721607331.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/22/24 10:16, Michael S. Tsirkin wrote: > A couple of fixes are outstanding, will merge later. > > > The following changes since commit a87a7c449e532130d4fa8faa391ff7e1f04ed660: > > Merge tag 'pull-loongarch-20240719' ofhttps://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 +1000) > > are available in the Git repository at: > > https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > > for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2: > > virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400) > > ---------------------------------------------------------------- > virtio,pci,pc: features,fixes > > pci: Initial support for SPDM Responders > cxl: Add support for scan media, feature commands, device patrol scrub > control, DDR5 ECS control, firmware updates > virtio: in-order support > virtio-net: support for SR-IOV emulation (note: known issues on s390, > might get reverted if not fixed) > smbios: memory device size is now configurable per Machine > cpu: architecture agnostic code to support vCPU Hotplug > > Fixes, cleanups all over the place. > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> Fails ubsan testing: https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access within null pointer of type 'struct vhost_net' r~
On Tue, Jul 23, 2024 at 07:32:56AM +1000, Richard Henderson wrote: > On 7/22/24 10:16, Michael S. Tsirkin wrote: > > A couple of fixes are outstanding, will merge later. > > > > > > The following changes since commit a87a7c449e532130d4fa8faa391ff7e1f04ed660: > > > > Merge tag 'pull-loongarch-20240719' ofhttps://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 +1000) > > > > are available in the Git repository at: > > > > https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream > > > > for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2: > > > > virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400) > > > > ---------------------------------------------------------------- > > virtio,pci,pc: features,fixes > > > > pci: Initial support for SPDM Responders > > cxl: Add support for scan media, feature commands, device patrol scrub > > control, DDR5 ECS control, firmware updates > > virtio: in-order support > > virtio-net: support for SR-IOV emulation (note: known issues on s390, > > might get reverted if not fixed) > > smbios: memory device size is now configurable per Machine > > cpu: architecture agnostic code to support vCPU Hotplug > > > > Fixes, cleanups all over the place. > > > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > Fails ubsan testing: > > https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 > > ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access within > null pointer of type 'struct vhost_net' > > > r~ Thanks! this is just make check under ubsan build, right?
On 7/23/24 10:20, Michael S. Tsirkin wrote: >> Fails ubsan testing: >> >> https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 >> >> ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access within >> null pointer of type 'struct vhost_net' >> >> >> r~ > > Thanks! this is just make check under ubsan build, right? Yep. For avoidance of doubt, the configure line is at the top of the log. r~
On 22.07.24 23:32, Richard Henderson wrote: > On 7/22/24 10:16, Michael S. Tsirkin wrote: >> A couple of fixes are outstanding, will merge later. >> >> >> The following changes since commit >> a87a7c449e532130d4fa8faa391ff7e1f04ed660: >> >> Merge tag 'pull-loongarch-20240719' >> ofhttps://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 >> +1000) >> >> are available in the Git repository at: >> >> https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git >> tags/for_upstream >> >> for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2: >> >> virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400) >> >> ---------------------------------------------------------------- >> virtio,pci,pc: features,fixes >> >> pci: Initial support for SPDM Responders >> cxl: Add support for scan media, feature commands, device patrol scrub >> control, DDR5 ECS control, firmware updates >> virtio: in-order support >> virtio-net: support for SR-IOV emulation (note: known issues on s390, >> might get reverted if not >> fixed) >> smbios: memory device size is now configurable per Machine >> cpu: architecture agnostic code to support vCPU Hotplug >> >> Fixes, cleanups all over the place. >> >> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > Fails ubsan testing: > > https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 > > ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access > within null pointer of type 'struct vhost_net' Honestly, I saw this piece of code, but concluded it already doesn’t make sense, so I assumed someone™ who wrote this would know why it’s been written this way, and I should rather not touch it. Specifically, the problem is that get_vhost_net() can return a NULL pointer[1], which is fine, but virtio_net_get_vhost() never checks this. I assumed this was written with intent (i.e. `(uintptr_t)&net->dev == (uintptr_t)net`, so that NULL remains NULL), because it’s so obvious that get_vhost_net() can happily return NULL under many circumstances, but maybe not. The same theoretically applies to virtio_crypto_get_vhost(), although I don’t think that can ever be NULL in practice. I’ll re-send the reset patch in a series with two patches that fix those two functions to check for NULL and explicitly return NULL if necessary. In the meantime, it probably makes sense to drop it from this pull request. Hanna [1] For some reason, it uses integer 0 throughout to signify NULL. That was another reason that put me off touching this.
On Tue, Jul 23, 2024 at 12:18:48PM +0200, Hanna Czenczek wrote: > On 22.07.24 23:32, Richard Henderson wrote: > > On 7/22/24 10:16, Michael S. Tsirkin wrote: > > > A couple of fixes are outstanding, will merge later. > > > > > > > > > The following changes since commit > > > a87a7c449e532130d4fa8faa391ff7e1f04ed660: > > > > > > Merge tag 'pull-loongarch-20240719' > > > ofhttps://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 > > > +1000) > > > > > > are available in the Git repository at: > > > > > > https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git > > > tags/for_upstream > > > > > > for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2: > > > > > > virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400) > > > > > > ---------------------------------------------------------------- > > > virtio,pci,pc: features,fixes > > > > > > pci: Initial support for SPDM Responders > > > cxl: Add support for scan media, feature commands, device patrol scrub > > > control, DDR5 ECS control, firmware updates > > > virtio: in-order support > > > virtio-net: support for SR-IOV emulation (note: known issues on s390, > > > might get reverted if not > > > fixed) > > > smbios: memory device size is now configurable per Machine > > > cpu: architecture agnostic code to support vCPU Hotplug > > > > > > Fixes, cleanups all over the place. > > > > > > Signed-off-by: Michael S. Tsirkin<mst@redhat.com> > > > > Fails ubsan testing: > > > > https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 > > > > ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access > > within null pointer of type 'struct vhost_net' > > Honestly, I saw this piece of code, but concluded it already doesn’t make > sense, so I assumed someone™ who wrote this would know why it’s been written > this way, and I should rather not touch it. > > Specifically, the problem is that get_vhost_net() can return a NULL > pointer[1], which is fine, but virtio_net_get_vhost() never checks this. I > assumed this was written with intent (i.e. `(uintptr_t)&net->dev == > (uintptr_t)net`, so that NULL remains NULL), because it’s so obvious that > get_vhost_net() can happily return NULL under many circumstances, but maybe > not. > > The same theoretically applies to virtio_crypto_get_vhost(), although I > don’t think that can ever be NULL in practice. > > I’ll re-send the reset patch in a series with two patches that fix those two > functions to check for NULL and explicitly return NULL if necessary. In the > meantime, it probably makes sense to drop it from this pull request. > > Hanna > > [1] For some reason, it uses integer 0 throughout to signify NULL. That was > another reason that put me off touching this. drop what specifically?
On 23.07.24 12:45, Michael S. Tsirkin wrote: > On Tue, Jul 23, 2024 at 12:18:48PM +0200, Hanna Czenczek wrote: >> On 22.07.24 23:32, Richard Henderson wrote: >>> On 7/22/24 10:16, Michael S. Tsirkin wrote: >>>> A couple of fixes are outstanding, will merge later. >>>> >>>> >>>> The following changes since commit >>>> a87a7c449e532130d4fa8faa391ff7e1f04ed660: >>>> >>>> Merge tag 'pull-loongarch-20240719' >>>> ofhttps://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 >>>> +1000) >>>> >>>> are available in the Git repository at: >>>> >>>> https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git >>>> tags/for_upstream >>>> >>>> for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2: >>>> >>>> virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400) >>>> >>>> ---------------------------------------------------------------- >>>> virtio,pci,pc: features,fixes >>>> >>>> pci: Initial support for SPDM Responders >>>> cxl: Add support for scan media, feature commands, device patrol scrub >>>> control, DDR5 ECS control, firmware updates >>>> virtio: in-order support >>>> virtio-net: support for SR-IOV emulation (note: known issues on s390, >>>> might get reverted if not >>>> fixed) >>>> smbios: memory device size is now configurable per Machine >>>> cpu: architecture agnostic code to support vCPU Hotplug >>>> >>>> Fixes, cleanups all over the place. >>>> >>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>> Fails ubsan testing: >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 >>> >>> ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access >>> within null pointer of type 'struct vhost_net' >> Honestly, I saw this piece of code, but concluded it already doesn’t make >> sense, so I assumed someone™ who wrote this would know why it’s been written >> this way, and I should rather not touch it. >> >> Specifically, the problem is that get_vhost_net() can return a NULL >> pointer[1], which is fine, but virtio_net_get_vhost() never checks this. I >> assumed this was written with intent (i.e. `(uintptr_t)&net->dev == >> (uintptr_t)net`, so that NULL remains NULL), because it’s so obvious that >> get_vhost_net() can happily return NULL under many circumstances, but maybe >> not. >> >> The same theoretically applies to virtio_crypto_get_vhost(), although I >> don’t think that can ever be NULL in practice. >> >> I’ll re-send the reset patch in a series with two patches that fix those two >> functions to check for NULL and explicitly return NULL if necessary. In the >> meantime, it probably makes sense to drop it from this pull request. >> >> Hanna >> >> [1] For some reason, it uses integer 0 throughout to signify NULL. That was >> another reason that put me off touching this. > drop what specifically? My patch, "virtio: Always reset vhost devices". Judging from the CI trace, the problem to me appears to be that not making get_vhost() depend on vhost_started lets ubsan find a way to have get_vhost_net() return NULL (which I don’t find surprising, that’s why we check get_vhost()’s return value for whether it’s NULL). It then complains about the `&net->dev` in virtio_net_get_vhost(), because net is NULL, and it’s not entirely clear that `&net->dev == NULL`, too (it is, which is why I decided to leave that piece of code be, originally). So dropping my patch should make CI succeed again. Hanna
On Tue, Jul 23, 2024 at 10:44:36AM +1000, Richard Henderson wrote: > On 7/23/24 10:20, Michael S. Tsirkin wrote: > > > Fails ubsan testing: > > > > > > https://gitlab.com/qemu-project/qemu/-/jobs/7397450714 > > > > > > ../publish/hw/net/virtio-net.c:3895:18: runtime error: member access within > > > null pointer of type 'struct vhost_net' > > > > > > > > > r~ > > > > Thanks! this is just make check under ubsan build, right? > > Yep. For avoidance of doubt, the configure line is at the top of the log. > > > r~ I wonder why I see a ton of other ubsan errors btw. But anyway, sent v2 with a couple of patches dropped.
A couple of fixes are outstanding, will merge later. The following changes since commit a87a7c449e532130d4fa8faa391ff7e1f04ed660: Merge tag 'pull-loongarch-20240719' of https://gitlab.com/gaosong/qemu into staging (2024-07-19 16:28:28 +1000) are available in the Git repository at: https://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream for you to fetch changes up to 67d834362c55d6fca6504975bc34755606f17cf2: virtio: Always reset vhost devices (2024-07-21 14:45:56 -0400) ---------------------------------------------------------------- virtio,pci,pc: features,fixes pci: Initial support for SPDM Responders cxl: Add support for scan media, feature commands, device patrol scrub control, DDR5 ECS control, firmware updates virtio: in-order support virtio-net: support for SR-IOV emulation (note: known issues on s390, might get reverted if not fixed) smbios: memory device size is now configurable per Machine cpu: architecture agnostic code to support vCPU Hotplug Fixes, cleanups all over the place. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> ---------------------------------------------------------------- Akihiko Odaki (8): hw/pci: Do not add ROM BAR for SR-IOV VF hw/pci: Fix SR-IOV VF number calculation pcie_sriov: Ensure PF and VF are mutually exclusive pcie_sriov: Check PCI Express for SR-IOV PF pcie_sriov: Allow user to create SR-IOV device virtio-pci: Implement SR-IOV PF virtio-net: Implement SR-IOV VF docs: Document composable SR-IOV device Alistair Francis (1): hw/pci: Add all Data Object Types defined in PCIe r6.0 Clément Mathieu--Drif (4): intel_iommu: fix FRCD construction macro intel_iommu: move VTD_FRCD_PV and VTD_FRCD_PP declarations intel_iommu: fix type of the mask field in VTDIOTLBPageInvInfo intel_iommu: make type match Davidlohr Bueso (3): hw/cxl: Add get scan media capabilities cmd support hw/cxl: Add get scan media results cmd support hw/cxl: Support firmware updates Eric Auger (6): Revert "virtio-iommu: Clear IOMMUDevice when VFIO device is unplugged" virtio-iommu: Remove probe_done virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices virtio-iommu: Remove the end point on detach hw/vfio/common: Add vfio_listener_region_del_iommu trace event virtio-iommu: Add trace point on virtio_iommu_detach_endpoint_from_domain Fan Ni (1): hw/cxl/cxl-mailbox-utils: remove unneeded mailbox output payload space zeroing Gregory Price (1): cxl/mailbox: move mailbox effect definitions to a header Hanna Czenczek (1): virtio: Always reset vhost devices Huai-Cheng Kuo (1): backends: Initial support for SPDM socket support Hyeonggon Yoo (2): hw/cxl/mbox: replace sanitize_running() with cxl_dev_media_disabled() hw/cxl/events: discard all event records during sanitation Igor Mammedov (1): smbios: make memory device size configurable per Machine Jonah Palmer (6): virtio: Add bool to VirtQueueElement virtio: virtqueue_pop - VIRTIO_F_IN_ORDER support virtio: virtqueue_ordered_fill - VIRTIO_F_IN_ORDER support virtio: virtqueue_ordered_flush - VIRTIO_F_IN_ORDER support vhost,vhost-user: Add VIRTIO_F_IN_ORDER to vhost feature bits virtio: Add VIRTIO_F_IN_ORDER property definition Jonathan Cameron (1): hw/cxl: Check for multiple mappings of memory backends. Manos Pitsidianakis (2): virtio-snd: add max size bounds check in input cb virtio-snd: check for invalid param shift operands Salil Mehta (7): accel/kvm: Extract common KVM vCPU {creation,parking} code hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file hw/acpi: Update ACPI GED framework to support vCPU Hotplug hw/acpi: Update GED _EVT method AML with CPU scan hw/acpi: Update CPUs AML with cpu-(ctrl)dev change physmem: Add helper function to destroy CPU AddressSpace gdbstub: Add helper function to unregister GDB register space Shiju Jose (3): hw/cxl/cxl-mailbox-utils: Add support for feature commands (8.2.9.6) hw/cxl/cxl-mailbox-utils: Add device patrol scrub control feature hw/cxl/cxl-mailbox-utils: Add device DDR5 ECS control feature Stefano Garzarella (2): MAINTAINERS: add Stefano Garzarella as vhost/vhost-user reviewer contrib/vhost-user-blk: fix overflowing expression Sunil V L (9): hw/riscv/virt-acpi-build.c: Add namespace devices for PLIC and APLIC hw/riscv/virt-acpi-build.c: Update the HID of RISC-V UART tests/acpi: Allow DSDT acpi table changes for aarch64 acpi/gpex: Create PCI link devices outside PCI root bridge tests/acpi: update expected DSDT blob for aarch64 and microvm tests/qtest/bios-tables-test.c: Remove the fall back path tests/acpi: Add empty ACPI data files for RISC-V tests/qtest/bios-tables-test.c: Enable basic testing for RISC-V tests/acpi: Add expected ACPI AML files for RISC-V Wilfred Mallawa (1): hw/nvme: Add SPDM over DOE support Yi Liu (1): MAINTAINERS: Add myself as a VT-d reviewer Zhao Liu (1): hw/cxl/cxl-host: Fix segmentation fault when getting cxl-fmw property Zheyu Ma (1): hw/virtio/virtio-crypto: Fix op_code assignment in virtio_crypto_create_asym_session accel/kvm/kvm-cpus.h | 1 - hw/i386/intel_iommu_internal.h | 6 +- include/exec/cpu-common.h | 8 + include/exec/gdbstub.h | 6 + include/hw/acpi/cpu.h | 7 +- include/hw/acpi/generic_event_device.h | 5 + include/hw/boards.h | 4 + include/hw/core/cpu.h | 1 + include/hw/cxl/cxl_device.h | 88 ++- include/hw/cxl/cxl_mailbox.h | 18 + include/hw/pci/pci_device.h | 13 +- include/hw/pci/pcie_doe.h | 5 + include/hw/pci/pcie_sriov.h | 18 + include/hw/virtio/virtio-iommu.h | 1 - include/hw/virtio/virtio-pci.h | 1 + include/hw/virtio/virtio.h | 6 +- include/sysemu/kvm.h | 25 + include/sysemu/spdm-socket.h | 74 ++ accel/kvm/kvm-all.c | 95 ++- backends/spdm-socket.c | 216 ++++++ contrib/vhost-user-blk/vhost-user-blk.c | 2 +- gdbstub/gdbstub.c | 13 + hw/acpi/acpi-cpu-hotplug-stub.c | 6 + hw/acpi/cpu.c | 18 +- hw/acpi/generic_event_device.c | 50 ++ hw/arm/virt.c | 1 + hw/audio/virtio-snd.c | 13 +- hw/block/vhost-user-blk.c | 1 + hw/core/cpu-common.c | 5 +- hw/core/machine.c | 6 + hw/cxl/cxl-events.c | 13 + hw/cxl/cxl-host.c | 3 +- hw/cxl/cxl-mailbox-utils.c | 966 +++++++++++++++++++++++-- hw/i386/acpi-build.c | 3 +- hw/i386/intel_iommu.c | 2 +- hw/i386/pc_piix.c | 1 + hw/i386/pc_q35.c | 1 + hw/mem/cxl_type3.c | 64 +- hw/net/vhost_net.c | 2 + hw/nvme/ctrl.c | 62 ++ hw/pci-host/gpex-acpi.c | 13 +- hw/pci/pci.c | 76 +- hw/pci/pcie_sriov.c | 300 ++++++-- hw/riscv/virt-acpi-build.c | 34 +- hw/scsi/vhost-scsi.c | 1 + hw/scsi/vhost-user-scsi.c | 1 + hw/smbios/smbios.c | 11 +- hw/vfio/common.c | 3 +- hw/virtio/vhost-user-fs.c | 1 + hw/virtio/vhost-user-vsock.c | 1 + hw/virtio/virtio-crypto.c | 2 +- hw/virtio/virtio-iommu.c | 88 +-- hw/virtio/virtio-net-pci.c | 1 + hw/virtio/virtio-pci.c | 20 +- hw/virtio/virtio.c | 139 +++- net/vhost-vdpa.c | 1 + system/physmem.c | 29 + tests/qtest/bios-tables-test.c | 40 +- MAINTAINERS | 9 + accel/kvm/trace-events | 5 +- backends/Kconfig | 4 + backends/meson.build | 2 + docs/specs/acpi_hw_reduced_hotplug.rst | 3 +- docs/specs/index.rst | 1 + docs/specs/spdm.rst | 134 ++++ docs/system/index.rst | 1 + docs/system/sriov.rst | 36 + hw/vfio/trace-events | 3 +- hw/virtio/trace-events | 1 + tests/data/acpi/aarch64/virt/DSDT | Bin 5196 -> 5196 bytes tests/data/acpi/aarch64/virt/DSDT.acpihmatvirt | Bin 5282 -> 5282 bytes tests/data/acpi/aarch64/virt/DSDT.memhp | Bin 6557 -> 6557 bytes tests/data/acpi/aarch64/virt/DSDT.pxb | Bin 7679 -> 7679 bytes tests/data/acpi/aarch64/virt/DSDT.topology | Bin 5398 -> 5398 bytes tests/data/acpi/riscv64/virt/APIC | Bin 0 -> 116 bytes tests/data/acpi/riscv64/virt/DSDT | Bin 0 -> 3576 bytes tests/data/acpi/riscv64/virt/FACP | Bin 0 -> 276 bytes tests/data/acpi/riscv64/virt/MCFG | Bin 0 -> 60 bytes tests/data/acpi/riscv64/virt/RHCT | Bin 0 -> 332 bytes tests/data/acpi/riscv64/virt/SPCR | Bin 0 -> 80 bytes tests/data/acpi/x86/microvm/DSDT.pcie | Bin 3023 -> 3023 bytes 81 files changed, 2500 insertions(+), 290 deletions(-) create mode 100644 include/hw/cxl/cxl_mailbox.h create mode 100644 include/sysemu/spdm-socket.h create mode 100644 backends/spdm-socket.c create mode 100644 docs/specs/spdm.rst create mode 100644 docs/system/sriov.rst create mode 100644 tests/data/acpi/riscv64/virt/APIC create mode 100644 tests/data/acpi/riscv64/virt/DSDT create mode 100644 tests/data/acpi/riscv64/virt/FACP create mode 100644 tests/data/acpi/riscv64/virt/MCFG create mode 100644 tests/data/acpi/riscv64/virt/RHCT create mode 100644 tests/data/acpi/riscv64/virt/SPCR