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

[Liran Alon]

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