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

[Liran Alon]

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:
> > > > > > Hi,
> > > > > >
> > > > > > 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
> > etc.).
> >
> > 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
> backport.
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?

> > 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 machine-type.
> 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?
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.
> > > userspace properties should use the non-abbreviated
> > > vm-executable since vmx is easy to confuse with vm extensions.
> > I really wish you would reconsider this. VMX is a really common term in
> > VMware terminology.
> > It is found in binary names, ".vmx" file, ".vmx" file properties, VMware
> > Tools prints, open-vm-tools source code and etc.
> Well that at least is easy to google.
>
>         .vmx
>
>         <vmname>.vmx
>
>         This is the primary configuration file, which stores settings
>         chosen in the New Virtual Machine Wizard or virtual machine settings
>         editor. If you created the virtual machine under an earlier version of
>         VMware Workstation on a Linux host, this file may have a .cfg extension
>
> so .vmx as used here has nothing to do with VM Executable version or
> type. Looks like it's just a source of confusion on the vmware
> side too :)
Well, the ".vmx" file is the configuration file for the VM given to VMX. 
But I agree VMware terminology is weird. :)
> > In contrast, even though I have dealt for many years with VMware
> > technologies, I have never known that VMX==vm-executable.
> Well you said that's what it stands for. I have no idea.  From what you
> say now maybe vmx basically is being used as a prefix for all things
> vmware.
No. It's just use to specify things related to VMX. i.e. The host VMM.
> In that case vmport-version and vmport-type or even
> vmware-version and vmware-type will do just as well.
vmware-version is also confusing. As one could confuse it with the 
product version number.
VMware called this field "vmx-version" and "vmx-type". I don't know if 
they have another field that maybe is called "vmware-version"...
> > I still think it will introduce much confusion. On the other hard, I don't
> > see much confusing with this use of VMX with Intel VT-x
> > because it is only used inside vmport.c and in vmport properties names. And
> > the properties names match the names of the guest
> > code that interface with vmport in open-vm-tools source code.
> >
> > If you still have a strong opinion on this, I will change it as you say in
> > v3... But please consider above arguments.
> I'm just saying don't use vmx. It's too late to try to give
> it a different meaning.
We are giving it here the same meaning VMware gave it. In the context of 
VMware VMPort.
>   Figure out what it's supposed to
> stand for and write it out in full.
VMX stands for the host VMM. But I don't see why I need to be in the 
position explaining the reason behind VMware terminology.
I'm just suggesting to use it as-is to avoid confusion.

It seems you are still not convinced by above arguments, so I will 
change this in v3 to what you preferred "vm-exec-version" & "vm-exec-type".
I think this is a mistake but you have the final call as the maintainer 
and I accept that.

-Liran