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

["Michael S. Tsirkin" <mst@redhat.com>]

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:
> > > > > > > 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?

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.


> > 
> > > 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?

Well how are you going to check a specific flag then?

> 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
>