On Fri, Nov 22, 2024 at 11:17:39AM +0100, Kevin Wolf wrote:
> Am 20.11.2024 um 14:19 hat Peter Maydell geschrieben:
> > On Wed, 20 Nov 2024 at 10:52, Kevin Wolf <kw...@redhat.com> wrote:
> > >
> > > The following changes since commit
> > > e6459afb1ff4d86b361b14f4a2fc43f0d2b4d679:
> > >
> > > Merge tag 'pull-target-arm-20241119' of
> > > https://git.linaro.org/people/pmaydell/qemu-arm into staging (2024-11-19
> > > 14:23:34 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > > https://repo.or.cz/qemu/kevin.git tags/for-upstream
> > >
> > > for you to fetch changes up to 83987bf722b6b692bc745b47901be76a1c97140b:
> > >
> > > vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-20
> > > 11:47:49 +0100)
> > >
> > > ----------------------------------------------------------------
> > > Block layer patches
> > >
> > > - Fix qmp_device_add() to not throw non-scalar options away (fixes
> > > iothread-vq-mapping being silently ignored in device_add)
> > > - Fix qdev property crash with integer PCI addresses and JSON -device
> > > - iotests: Fix mypy failure
> > > - parallels: Avoid potential integer overflow
> > > - Fix crash in migration_is_running()
> >
> > Hi; the hotplug_blk.py:HotPlug.test avocado seems to be failing:
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/8423313170
> > https://gitlab.com/qemu-project/qemu/-/jobs/8423313162
> >
> > [stdlog] 2024-11-20 12:49:35,669 avocado.test test L0740 ERROR| FAIL
> > 1-tests/avocado/hotplug_blk.py:HotPlug.test -> AssertionError: 1 != 0
> > : Guest command failed: test -e /sys/block/vda
> >
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8423313162/artifacts/build/tests/results/latest/test-results/09-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> >
> > Looks like the test called device_add, it succeeded, but
> > it didn't see the /sys/block/vda node appear in the guest.
> >
> > (The test logic of "try the command, if it fails sleep for 1
> > second then try a second time and if that fails call it a
> > test error" doesn't seem super robust in the face of slow
> > CI runners, but OTOH it failed the same way on both jobs,
> > so I don't think that is the culprit here.)
>
> This looks like a bug in the test case that was previously cancelled out
> by a QEMU bug. :-/
>
> {
> "execute": "device_add",
> "arguments": {
> "driver": "virtio-blk-pci",
> "drive": "disk",
> "id": "virtio-disk0",
> "bus": "pci.1",
> "addr": 1
> },
> "id": "__qmp#00002"
> }
>
> What it actually meant is "addr": "1". It's an unfortunate interface,
> but string "1" and integer 1 mean different things for PCI address
> properties... Going through QemuOpts turned everything into strings, so
> that masked the bug in the test case.
I never realized that "1" and "1" mean different things in PCI
addresses !
IIUC, "1" decodes as slot 1, function 0, while integer 1 decodes as
slot 0, function 1 . ie raw integer value is "slot << 3 | function"
IIUC from the code ?
This is both majorly surprising, and rather obscure - did we document
the integer value encoding semantics anywhere ?
AFAICT, they first arrived back in
commit 768a9ebe188bd0a6172a9a4e64777d21fff7f014
Author: Paolo Bonzini <pbonz...@redhat.com>
Date: Thu Feb 9 09:53:32 2012 +0100
qdev: accept both strings and integers for PCI addresses
Visitors allow a limited form of polymorphism. Exploit it to support
setting the non-legacy PCI address property both as a DD.F string
and as an 8-bit integer.
The 8-bit integer form is just too clumsy, it is unlikely that we will
ever drop it.
I'm guessing that last sentence is a mistake - refering to the new
integer syntax as clumsy & unable to be dropped doesn't make sense.
I assume it was referring to the historical string syntax as the
clumsy part.
> Should I just fix the test case and move on, or are we concerned about
> other users having a similar bug and want to move the change to 10.0,
> keeping device_add with iothread-vq-mapping broken in 9.2?
I think neither syntax really makes sense from a pure QAPI design
POV, as both are inventing a special way to encode 2 distinct
fields within one field which is a QAPI anti-pattern. The "right"
QAPI answer is to express them as 2 distinct fields at the QAPI
level.
Having two distinct ways to express the same concept is redundant.
Perhaps passable if one syntax was actually following QAPI best
practice, even better if one syntax were deprecated.
Is it worth thinking about going through the tedium of deprecating
both of them and replacing the syntax with a preferred QAPI design
pattern ?
If we don't want to replace the existing string format which is
widely used, then is the integer format compelling enough to keep
as an option, given it will easily surprise people who don't read
the non-existant docs about it ? ie deprecate integer syntax and
leave ony the string syntax ?
FWIW, the options for a QAPI design following best practice
design would be
1. "slot" and "function" properties at top level
{
"execute": "device_add",
"arguments": {
"driver": "virtio-blk-pci",
"drive": "disk",
"id": "virtio-disk0",
"bus": "pci.1",
"slot": 1,
"function": 0,
},
"id": "__qmp#00002"
}
2. two element array
{
"execute": "device_add",
"arguments": {
"driver": "virtio-blk-pci",
"drive": "disk",
"id": "virtio-disk0",
"bus": "pci.1",
"addr": [1, 0]
},
"id": "__qmp#00002"
}
3. As 1 but with nested struct
{
"execute": "device_add",
"arguments": {
"driver": "virtio-blk-pci",
"drive": "disk",
"id": "virtio-disk0",
"bus": "pci.1",
"addr": {
"slot": 1,
"function": 0
}
},
"id": "__qmp#00002"
}
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
