[Top][All Lists]

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

Re: [PATCH 07/14] hw/i386/vmport: Add support for CMD_GETBIOSUUID

From: Liran Alon
Subject: Re: [PATCH 07/14] hw/i386/vmport: Add support for CMD_GETBIOSUUID
Date: Tue, 10 Mar 2020 16:56:29 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

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

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?

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.


reply via email to

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