On 10/03/2020 16:39, Michael S. Tsirkin wrote:
On Tue, Mar 10, 2020 at 04:24:45PM +0200, Liran Alon wrote:
Re-thinking about this...
QEMU VMPort interface was quite broken already (See first patch in series
"hw/i386/vmport: Propagate IOPort read to vCPU EAX register").
The introduction of that fix already changes the result of all existing
commands from guest perspective which relied on return-value from
vmport_ioport_read().
E.g. CMD_GETVERSION and CMD_GETRAMSIZE.
In theory, we should have also made that bug-fix be tied to machine-type. To
similarly avoid the issue of migrating a VM from a working VMPort command
implementation to a non-working one.
i.e. In case of migrating from new QEMU to old QEMU. Do we wish to create a
property-flag for that fix as-well?
Yes, I meants that too. Just include everything in the same property.
It ugly the code with a lot of "if"s for maintaining compatibility for
guests that somehow relies on interface being broken and unusable.
Can do this but am wondering if it's worth it.
Or can we just drop all the machine-type
flags alltogether (Including the suggested "commands-v2")
and declare this the first actually working VMPort implementation?
-Liran
It's hard to be sure no one used this
Well... Both implemented commands (CMD_GETVERSION and CMD_GETRRAMSIZE)
fails to return their proper value.
CMD_GETVERSION will always return VMPORT_MAGIC that happened to be in
EAX previously (i.e. return 0x564D5868 instead of 6).
CMD_GETRAMSIZE will always return VMPORT_MAGIC that happened to be in
EAX previously (i.e. return 0x564D5868 instead of VM RAM size).
If guest somehow relied on this, it is already quite broken...
My belief is that all upstream QEMU users today relies on VMPort only
for the sake of a functioning VMMouse which is indeed not broken because
vmmouse_set_data() explicitly override EAX.
, and the overhead isn't big. But
that would be a maintainer call. In any case you need to call this out
explicitly in the commit log, and I guess block migration for old
machine types.
Blocking migration for old machine-types is a "no go" in my opinion as
vmport is enabled by default. It will cause too many VMs to need be able
to backwards migrate.
So it's either doing nothing (as patch-series is now) or adding a flag
that adds a bunch of ugly "ifs" and is tied to a machine-type.
-Liran
