[PULL 00/20] pc,virtio,pci: fixes, features

Michael S. Tsirkin posted 20 patches 4 years, 10 months ago
Only 19 patches received!
There is a newer version of this series
include/hw/acpi/aml-build.h      |   6 +-
include/hw/acpi/pci.h            |   1 +
include/hw/acpi/pcihp.h          |   9 +-
include/hw/acpi/utils.h          |   3 +-
include/hw/i386/microvm.h        |   4 -
include/hw/i386/pc.h             |   4 -
include/hw/i386/x86.h            |   4 +
include/hw/pci/pci.h             |   1 +
hw/acpi/aml-build.c              |  28 +++++
hw/acpi/pci.c                    |   1 -
hw/acpi/pcihp.c                  | 104 ++++++++++++++++++-
hw/acpi/piix4.c                  |  10 +-
hw/acpi/utils.c                  |  17 ++-
hw/arm/virt-acpi-build.c         |  12 +--
hw/i386/acpi-build.c             | 173 +++++++++++++++++++++++++------
hw/i386/acpi-microvm.c           |  32 +++---
hw/i386/microvm.c                |  66 ------------
hw/i386/pc.c                     |  63 ------------
hw/i386/x86.c                    |  64 ++++++++++++
hw/isa/vt82c686.c                |   5 +
hw/pci/pci.c                     |   1 +
hw/virtio/vhost-user.c           | 217 +++++++++++++++++++++++++--------------
hw/virtio/virtio-mmio.c          |  74 +++++++++----
hw/virtio/virtio-pmem.c          |   2 +-
hw/acpi/trace-events             |   2 +
tests/data/acpi/pc/DSDT          | Bin 5065 -> 6002 bytes
tests/data/acpi/pc/DSDT.acpihmat | Bin 6390 -> 7327 bytes
tests/data/acpi/pc/DSDT.bridge   | Bin 6924 -> 8668 bytes
tests/data/acpi/pc/DSDT.cphp     | Bin 5529 -> 6466 bytes
tests/data/acpi/pc/DSDT.dimmpxm  | Bin 6719 -> 7656 bytes
tests/data/acpi/pc/DSDT.hpbridge | Bin 5026 -> 5969 bytes
tests/data/acpi/pc/DSDT.ipmikcs  | Bin 5137 -> 6074 bytes
tests/data/acpi/pc/DSDT.memhp    | Bin 6424 -> 7361 bytes
tests/data/acpi/pc/DSDT.nohpet   | Bin 4923 -> 5860 bytes
tests/data/acpi/pc/DSDT.numamem  | Bin 5071 -> 6008 bytes
tests/data/acpi/pc/DSDT.roothp   | Bin 5261 -> 6210 bytes
36 files changed, 595 insertions(+), 308 deletions(-)
[PULL 00/20] pc,virtio,pci: fixes, features
Posted by Michael S. Tsirkin 4 years, 10 months ago
The following changes since commit f0f20022a0c744930935fdb7020a8c18347d391a:

  Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-03-21' into staging (2021-03-22 10:05:45 +0000)

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 5971d4a968d51a80daaad53ddaec2b285115af62:

  acpi: Move setters/getters of oem fields to X86MachineState (2021-03-22 11:39:02 -0400)

----------------------------------------------------------------
pc,virtio,pci: fixes, features

Fixes all over the place.
ACPI index support.

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

----------------------------------------------------------------
David Hildenbrand (4):
      acpi: Set proper maximum size for "etc/table-loader" blob
      microvm: Don't open-code "etc/table-loader"
      acpi: Move maximum size logic into acpi_add_rom_blob()
      acpi: Set proper maximum size for "etc/acpi/rsdp" blob

Greg Kurz (6):
      vhost-user: Drop misleading EAGAIN checks in slave_read()
      vhost-user: Fix double-close on slave_read() error path
      vhost-user: Factor out duplicated slave_fd teardown code
      vhost-user: Convert slave channel to QIOChannelSocket
      vhost-user: Introduce nested event loop in vhost_user_read()
      vhost-user: Monitor slave channel in vhost_user_read()

Igor Mammedov (6):
      tests: acpi: temporary whitelist DSDT changes
      pci: introduce acpi-index property for PCI device
      pci: acpi: ensure that acpi-index is unique
      acpi: add aml_to_decimalstring() and aml_call6() helpers
      pci: acpi: add _DSM method to PCI devices
      tests: acpi: update expected blobs

Isaku Yamahata (1):
      acpi:piix4, vt82c686: reinitialize acpi PM device on reset

Laurent Vivier (1):
      virtio: Fix virtio_mmio_read()/virtio_mmio_write()

Marian Postevca (1):
      acpi: Move setters/getters of oem fields to X86MachineState

Wang Liang (1):
      virtio-pmem: fix virtio_pmem_resp assign problem

 include/hw/acpi/aml-build.h      |   6 +-
 include/hw/acpi/pci.h            |   1 +
 include/hw/acpi/pcihp.h          |   9 +-
 include/hw/acpi/utils.h          |   3 +-
 include/hw/i386/microvm.h        |   4 -
 include/hw/i386/pc.h             |   4 -
 include/hw/i386/x86.h            |   4 +
 include/hw/pci/pci.h             |   1 +
 hw/acpi/aml-build.c              |  28 +++++
 hw/acpi/pci.c                    |   1 -
 hw/acpi/pcihp.c                  | 104 ++++++++++++++++++-
 hw/acpi/piix4.c                  |  10 +-
 hw/acpi/utils.c                  |  17 ++-
 hw/arm/virt-acpi-build.c         |  12 +--
 hw/i386/acpi-build.c             | 173 +++++++++++++++++++++++++------
 hw/i386/acpi-microvm.c           |  32 +++---
 hw/i386/microvm.c                |  66 ------------
 hw/i386/pc.c                     |  63 ------------
 hw/i386/x86.c                    |  64 ++++++++++++
 hw/isa/vt82c686.c                |   5 +
 hw/pci/pci.c                     |   1 +
 hw/virtio/vhost-user.c           | 217 +++++++++++++++++++++++++--------------
 hw/virtio/virtio-mmio.c          |  74 +++++++++----
 hw/virtio/virtio-pmem.c          |   2 +-
 hw/acpi/trace-events             |   2 +
 tests/data/acpi/pc/DSDT          | Bin 5065 -> 6002 bytes
 tests/data/acpi/pc/DSDT.acpihmat | Bin 6390 -> 7327 bytes
 tests/data/acpi/pc/DSDT.bridge   | Bin 6924 -> 8668 bytes
 tests/data/acpi/pc/DSDT.cphp     | Bin 5529 -> 6466 bytes
 tests/data/acpi/pc/DSDT.dimmpxm  | Bin 6719 -> 7656 bytes
 tests/data/acpi/pc/DSDT.hpbridge | Bin 5026 -> 5969 bytes
 tests/data/acpi/pc/DSDT.ipmikcs  | Bin 5137 -> 6074 bytes
 tests/data/acpi/pc/DSDT.memhp    | Bin 6424 -> 7361 bytes
 tests/data/acpi/pc/DSDT.nohpet   | Bin 4923 -> 5860 bytes
 tests/data/acpi/pc/DSDT.numamem  | Bin 5071 -> 6008 bytes
 tests/data/acpi/pc/DSDT.roothp   | Bin 5261 -> 6210 bytes
 36 files changed, 595 insertions(+), 308 deletions(-)


Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Peter Maydell 4 years, 10 months ago
On Mon, 22 Mar 2021 at 15:44, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit f0f20022a0c744930935fdb7020a8c18347d391a:
>
>   Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-03-21' into staging (2021-03-22 10:05:45 +0000)
>
> 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 5971d4a968d51a80daaad53ddaec2b285115af62:
>
>   acpi: Move setters/getters of oem fields to X86MachineState (2021-03-22 11:39:02 -0400)
>
> ----------------------------------------------------------------
> pc,virtio,pci: fixes, features
>
> Fixes all over the place.
> ACPI index support.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>

This triggers a new clang runtime sanitizer warning:

MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k
PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt
PASS 2 qtest-mips64el/qom-test /mips64el/qom/none
PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum
PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim
PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e
PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston
PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61

and similarly for eg

MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
QTEST_QEMU_IMG=./qemu-img
G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test
--tap -k
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e
../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e

thanks
-- PMM

Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Peter Maydell 4 years, 10 months ago
On Mon, 22 Mar 2021 at 16:41, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 22 Mar 2021 at 15:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The following changes since commit f0f20022a0c744930935fdb7020a8c18347d391a:
> >
> >   Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-03-21' into staging (2021-03-22 10:05:45 +0000)
> >
> > 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 5971d4a968d51a80daaad53ddaec2b285115af62:
> >
> >   acpi: Move setters/getters of oem fields to X86MachineState (2021-03-22 11:39:02 -0400)
> >
> > ----------------------------------------------------------------
> > pc,virtio,pci: fixes, features
> >
> > Fixes all over the place.
> > ACPI index support.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
>
> This triggers a new clang runtime sanitizer warning:

With a backtrace:
$ UBSAN_OPTIONS=print_stacktrace=1
QTEST_QEMU_BINARY=build/clang/qemu-system-mips64el
./build/clang/tests/qtest/endianness-test -p
/mips64el/endianness/fuloong2e
/mips64el/endianness/fuloong2e: ../../hw/pci/pci.c:252:30: runtime
error: shift exponent -1 is negative
    #0 0x55a17bc17a1f in pci_irq_state
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/pci/pci.c:252:30
    #1 0x55a17bc17a1f in pci_irq_handler
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/pci/pci.c:1453
    #2 0x55a17b7ed0a5 in pm_update_sci
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/isa/vt82c686.c:147:5
    #3 0x55a17b7ecce3 in via_pm_reset
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/isa/vt82c686.c:173:5
    #4 0x55a17c546cc7 in resettable_phase_hold
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:182:13
    #5 0x55a17c53839a in bus_reset_child_foreach
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/bus.c:97:13
    #6 0x55a17c546bc2 in resettable_phase_hold
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:173:5
    #7 0x55a17c5435ca in device_reset_child_foreach
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/qdev.c:366:9
    #8 0x55a17c546bc2 in resettable_phase_hold
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:173:5
    #9 0x55a17c53839a in bus_reset_child_foreach
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/bus.c:97:13
    #10 0x55a17c546bc2 in resettable_phase_hold
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:173:5
    #11 0x55a17c545ee0 in resettable_assert_reset
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:60:5
    #12 0x55a17c545dbf in resettable_reset
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:45:5
    #13 0x55a17c545d68 in qemu_devices_reset
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/reset.c:69:9
    #14 0x55a17c47b3eb in qemu_system_reset
/home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/runstate.c:444:9
    #15 0x55a17ba225ee in qdev_machine_creation_done
/home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/machine.c:1279:5
    #16 0x55a17c4bdb03 in qemu_machine_creation_done
/home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/vl.c:2567:5
    #17 0x55a17c4bdb03 in qmp_x_exit_preconfig
/home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/vl.c:2590
    #18 0x55a17c4c2c0b in qemu_init
/home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/vl.c:3611:9
    #19 0x55a17b756db5 in main
/home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/main.c:49:5
    #20 0x7f3a9c9f6bf6 in __libc_start_main
/build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
    #21 0x55a17b731969 in _start
(/home/petmay01/linaro/qemu-for-merges/build/clang/qemu-system-mips64el+0x1140969)

OK

Suggests the relevant commit is
"acpi:piix4, vt82c686: reinitialize acpi PM device on reset"

This happens because pm_update_sci() calls pci_irq_handler(),
which calls pci_intx(pci_dev), which returns -1, which is not
a valid interrupt number to call pci_irq_handler() with.

Q: given that pci_irq_handler() says it must only be called with
an irqnum in [0..3], shouldn't pci_set_irq() be a bit more
cautious than to pull a byte directly out of PCI_INTERRUPT_PIN
and assume it's valid? (Is this guest-writable, or is it read-only?)

thanks
-- PMM

Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Michael S. Tsirkin 4 years, 10 months ago
On Mon, Mar 22, 2021 at 06:46:06PM +0000, Peter Maydell wrote:
> On Mon, 22 Mar 2021 at 16:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Mon, 22 Mar 2021 at 15:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > The following changes since commit f0f20022a0c744930935fdb7020a8c18347d391a:
> > >
> > >   Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-03-21' into staging (2021-03-22 10:05:45 +0000)
> > >
> > > 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 5971d4a968d51a80daaad53ddaec2b285115af62:
> > >
> > >   acpi: Move setters/getters of oem fields to X86MachineState (2021-03-22 11:39:02 -0400)
> > >
> > > ----------------------------------------------------------------
> > > pc,virtio,pci: fixes, features
> > >
> > > Fixes all over the place.
> > > ACPI index support.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> >
> > This triggers a new clang runtime sanitizer warning:
> 
> With a backtrace:
> $ UBSAN_OPTIONS=print_stacktrace=1
> QTEST_QEMU_BINARY=build/clang/qemu-system-mips64el
> ./build/clang/tests/qtest/endianness-test -p
> /mips64el/endianness/fuloong2e
> /mips64el/endianness/fuloong2e: ../../hw/pci/pci.c:252:30: runtime
> error: shift exponent -1 is negative
>     #0 0x55a17bc17a1f in pci_irq_state
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/pci/pci.c:252:30
>     #1 0x55a17bc17a1f in pci_irq_handler
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/pci/pci.c:1453
>     #2 0x55a17b7ed0a5 in pm_update_sci
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/isa/vt82c686.c:147:5
>     #3 0x55a17b7ecce3 in via_pm_reset
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/isa/vt82c686.c:173:5
>     #4 0x55a17c546cc7 in resettable_phase_hold
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:182:13
>     #5 0x55a17c53839a in bus_reset_child_foreach
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/bus.c:97:13
>     #6 0x55a17c546bc2 in resettable_phase_hold
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:173:5
>     #7 0x55a17c5435ca in device_reset_child_foreach
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/qdev.c:366:9
>     #8 0x55a17c546bc2 in resettable_phase_hold
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:173:5
>     #9 0x55a17c53839a in bus_reset_child_foreach
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/bus.c:97:13
>     #10 0x55a17c546bc2 in resettable_phase_hold
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:173:5
>     #11 0x55a17c545ee0 in resettable_assert_reset
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:60:5
>     #12 0x55a17c545dbf in resettable_reset
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/resettable.c:45:5
>     #13 0x55a17c545d68 in qemu_devices_reset
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/reset.c:69:9
>     #14 0x55a17c47b3eb in qemu_system_reset
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/runstate.c:444:9
>     #15 0x55a17ba225ee in qdev_machine_creation_done
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../hw/core/machine.c:1279:5
>     #16 0x55a17c4bdb03 in qemu_machine_creation_done
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/vl.c:2567:5
>     #17 0x55a17c4bdb03 in qmp_x_exit_preconfig
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/vl.c:2590
>     #18 0x55a17c4c2c0b in qemu_init
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/vl.c:3611:9
>     #19 0x55a17b756db5 in main
> /home/petmay01/linaro/qemu-for-merges/build/clang/../../softmmu/main.c:49:5
>     #20 0x7f3a9c9f6bf6 in __libc_start_main
> /build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
>     #21 0x55a17b731969 in _start
> (/home/petmay01/linaro/qemu-for-merges/build/clang/qemu-system-mips64el+0x1140969)
> 
> OK
> 
> Suggests the relevant commit is
> "acpi:piix4, vt82c686: reinitialize acpi PM device on reset"

Yep, Cc'd the authors and dropped for now. Thanks!

> This happens because pm_update_sci() calls pci_irq_handler(),
> which calls pci_intx(pci_dev), which returns -1, which is not
> a valid interrupt number to call pci_irq_handler() with.
> 
> Q: given that pci_irq_handler() says it must only be called with
> an irqnum in [0..3], shouldn't pci_set_irq() be a bit more
> cautious than to pull a byte directly out of PCI_INTERRUPT_PIN
> and assume it's valid? (Is this guest-writable, or is it read-only?)

It's read-only.

> 
> thanks
> -- PMM


-- 
MST


Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Peter Maydell 4 years, 10 months ago
On Mon, 22 Mar 2021 at 22:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Mar 22, 2021 at 06:46:06PM +0000, Peter Maydell wrote:
> > This happens because pm_update_sci() calls pci_irq_handler(),
> > which calls pci_intx(pci_dev), which returns -1, which is not
> > a valid interrupt number to call pci_irq_handler() with.
> >
> > Q: given that pci_irq_handler() says it must only be called with
> > an irqnum in [0..3], shouldn't pci_set_irq() be a bit more
> > cautious than to pull a byte directly out of PCI_INTERRUPT_PIN
> > and assume it's valid? (Is this guest-writable, or is it read-only?)
>
> It's read-only.

Ah, so if a device model (a) doesn't set the value to a correct
interrupt number and then (b) triggers an interrupt for itself,
then that's a device model bug ? It might be worth assert()ing
that the irqnum is valid, just to catch this kind of bug a bit
more obviously.

thanks
-- PMM

Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Michael S. Tsirkin 4 years, 10 months ago
On Tue, Mar 23, 2021 at 10:50:05AM +0000, Peter Maydell wrote:
> On Mon, 22 Mar 2021 at 22:56, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Mar 22, 2021 at 06:46:06PM +0000, Peter Maydell wrote:
> > > This happens because pm_update_sci() calls pci_irq_handler(),
> > > which calls pci_intx(pci_dev), which returns -1, which is not
> > > a valid interrupt number to call pci_irq_handler() with.
> > >
> > > Q: given that pci_irq_handler() says it must only be called with
> > > an irqnum in [0..3], shouldn't pci_set_irq() be a bit more
> > > cautious than to pull a byte directly out of PCI_INTERRUPT_PIN
> > > and assume it's valid? (Is this guest-writable, or is it read-only?)
> >
> > It's read-only.
> 
> Ah, so if a device model (a) doesn't set the value to a correct
> interrupt number and then (b) triggers an interrupt for itself,
> then that's a device model bug ? It might be worth assert()ing
> that the irqnum is valid, just to catch this kind of bug a bit
> more obviously.
> 
> thanks
> -- PMM

Sure, we can do this. Patch?

-- 
MST


Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Igor Mammedov 4 years, 10 months ago
On Tue, 23 Mar 2021 10:13:58 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

CCing Philippe,

maybe he has an idea how to fix it from mips side.

> On Tue, Mar 23, 2021 at 10:50:05AM +0000, Peter Maydell wrote:
> > On Mon, 22 Mar 2021 at 22:56, Michael S. Tsirkin <mst@redhat.com> wrote:  
> > > On Mon, Mar 22, 2021 at 06:46:06PM +0000, Peter Maydell wrote:  
> > > > This happens because pm_update_sci() calls pci_irq_handler(),
> > > > which calls pci_intx(pci_dev), which returns -1, which is not
> > > > a valid interrupt number to call pci_irq_handler() with.
> > > >
> > > > Q: given that pci_irq_handler() says it must only be called with
> > > > an irqnum in [0..3], shouldn't pci_set_irq() be a bit more
> > > > cautious than to pull a byte directly out of PCI_INTERRUPT_PIN
> > > > and assume it's valid? (Is this guest-writable, or is it read-only?)  
> > >
> > > It's read-only.  
> > 
> > Ah, so if a device model (a) doesn't set the value to a correct
> > interrupt number and then (b) triggers an interrupt for itself,
> > then that's a device model bug ? It might be worth assert()ing
> > that the irqnum is valid, just to catch this kind of bug a bit
> > more obviously.
> > 
> > thanks
> > -- PMM  
> 
> Sure, we can do this. Patch?
> 


Re: [PULL 00/20] pc,virtio,pci: fixes, features
Posted by Michael S. Tsirkin 4 years, 10 months ago
On Mon, Mar 22, 2021 at 04:41:01PM +0000, Peter Maydell wrote:
> On Mon, 22 Mar 2021 at 15:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The following changes since commit f0f20022a0c744930935fdb7020a8c18347d391a:
> >
> >   Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-03-21' into staging (2021-03-22 10:05:45 +0000)
> >
> > 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 5971d4a968d51a80daaad53ddaec2b285115af62:
> >
> >   acpi: Move setters/getters of oem fields to X86MachineState (2021-03-22 11:39:02 -0400)
> >
> > ----------------------------------------------------------------
> > pc,virtio,pci: fixes, features
> >
> > Fixes all over the place.
> > ACPI index support.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> 
> This triggers a new clang runtime sanitizer warning:
> 
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/qom-test --tap -k
> PASS 1 qtest-mips64el/qom-test /mips64el/qom/loongson3-virt
> PASS 2 qtest-mips64el/qom-test /mips64el/qom/none
> PASS 3 qtest-mips64el/qom-test /mips64el/qom/magnum
> PASS 4 qtest-mips64el/qom-test /mips64el/qom/mipssim
> PASS 5 qtest-mips64el/qom-test /mips64el/qom/malta
> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
> PASS 6 qtest-mips64el/qom-test /mips64el/qom/fuloong2e
> PASS 7 qtest-mips64el/qom-test /mips64el/qom/boston
> PASS 8 qtest-mips64el/qom-test /mips64el/qom/pica61
> 
> and similarly for eg
> 
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_IMG=./qemu-img
> G_TEST_DBUS_DAEMON=/home/petmay01/linaro/qemu-for-merges/tests/dbus-vmstate-daemon.sh
> QTEST_QEMU_BINARY=./qemu-system-mips64el tests/qtest/endianness-test
> --tap -k
> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
> PASS 1 qtest-mips64el/endianness-test /mips64el/endianness/fuloong2e
> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
> PASS 2 qtest-mips64el/endianness-test /mips64el/endianness/split/fuloong2e
> ../../hw/pci/pci.c:252:30: runtime error: shift exponent -1 is negative
> PASS 3 qtest-mips64el/endianness-test /mips64el/endianness/combine/fuloong2e
> 
> thanks
> -- PMM

Weird I don't see any related changes. Something subtle going on.
I'd like to reproduce this.
Which clang flags do you use?

-- 
MST