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

[Liran Alon]

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