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

[Michael S. Tsirkin]

On Wed, Mar 11, 2020 at 12:19:59AM +0200, Liran Alon wrote:
> 
> 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?

So it would be like this:

{
        "x-vmport-fixes" : "0x7" /* VMPORT_FOO | VMPORT_BAR */
}

I prefer DEFINE_PROP_BIT myself. But this version is not too terrible I
think.

-- 
MST