[Top][All Lists]

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

Re: [PATCH v2 00/16]: hw/i386/vmport: Bug fixes and improvements

From: Liran Alon
Subject: Re: [PATCH v2 00/16]: hw/i386/vmport: Bug fixes and improvements
Date: Wed, 11 Mar 2020 00:19:59 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 11/03/2020 0:00, Michael S. Tsirkin wrote:
On Tue, Mar 10, 2020 at 11:57:49PM +0200, Liran Alon wrote:
On 10/03/2020 23:44, Michael S. Tsirkin wrote:
On Tue, Mar 10, 2020 at 02:29:42PM -0700, Liran Alon wrote:
On 10/03/2020 22:56, Michael S. Tsirkin wrote:
On Tue, Mar 10, 2020 at 08:09:09PM +0200, Liran Alon wrote:
On 10/03/2020 19:44, Michael S. Tsirkin wrote:
On Tue, Mar 10, 2020 at 06:53:16PM +0200, Liran Alon wrote:

This series aims to fix several bugs in VMPort and improve it by supporting
more VMPort commands and make command results more configurable to
user via QEMU command-line.

This functionality was proven to be useful to run various VMware VMs
when attempting to run them as-is on top of QEMU/KVM.

For more details, see commit messages.
Well two versions in one day and some review comments weren't addressed.
There is a single review comment that wasn't addressed which is replacing an
enum with a comment. And I explicitly mentioned that it's because I want
additional opinion on this.
I don't see why such a small thing should block review for 15 patches...
All the rest of the comments (Which were great) have been addressed. Unless
I have mistakenly missed something, which please point it out if I did.
OK I just took a quick peek, two things quickly jumped out at me.
Thanks for having a look.
version property really should be a boolean and have some documentation
saying what functionality enables.
I thought that having a version number approach is more generic and easy to
maintain going forward.
If I understand correctly, this is also the approach taken by qxl & qxl-vga.

The more elaborate alternative could have been introducing compat_flags (As
PVSCSI does) but it seems like it will pollute the property space with a lot
of useless VMPort properties.
(E.g. x-read-eax-bug, x-no-report-unsupported-cmd, x-no-report-vmx-type and

What is the advantage of having a boolean such as "x-vmport-v2" instead of
having a single "version" property?
It's not clear what should happen going forward. Let's say version is
incremented again. This then becomes challenging for downstreams to
As all conditions are in the form of "if (s->version > X)" then incrementing
version from 1 to 2 doesn't break any condition of "if (s->version > 1)".
What is the challenge of backporting I'm missing?
the challenge is figuring out which parts does version apply to.
It might be easy if there's just code, harder if there's
also data, etc.
You mean things such as the following?
s->some_field = (s->version > X) ? A : B;

True that it could be a bit more difficult to spot.

Will it suffice if I would just add documentation above "version" property
on what is was the functionality in "version==1"?
(Though, it's just easy to scan the vmport.c code for if's involving
">version"... "version" is more of an internal field for machine-type
compatibility and not really meant to be used by user)

Which approach do you prefer?
I just dislike versions, they are hard to maintain.

Individual ones is cleanest imho. Self-documenting.
I agree. That's the PVSCSI approach of compat_flags. Have many properties
but each define bit in a compat_flags that specifies behavior.
The disadvantage it have is that it over-complicates code and introduce many
properties that will never be used as it's just for internal binding to
But if not, I'd do something like "x-vmport-fixes" and
set bits there for each bugfix.
Hmm this could a nice and simple approach.
Will it be OK then in this case to define "x-vmport-fixes" value in
hw_compat_4_2[] to a hard-coded value (e.g. "20") without directly encoding
each individual bit via vmport.h constants?
Well how are you going to check a specific flag then?
In the code itself I will have constants of course.
I meant just in hw_compat_4_2[] machine-type compat entry because the bitmask value there should be specified as a string value.

I will note though that it seems this "x-vmport-fixes" bitmap seems to be
the first of it's kind. But I'm OK with this approach.
So just to be clear before implementing your suggesting approach, this doesn't bother you right?

reply via email to

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