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