Re: [PATCH v4 16/21] i386: track explicit 'hv-*' features enablement/disablement

[Igor Mammedov]

On Mon, 15 Feb 2021 09:56:19 +0100
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Fri, 12 Feb 2021 16:26:03 +0100
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Vitaly Kuznetsov <vkuznets@redhat.com> writes:
> >>   
> >> > Igor Mammedov <imammedo@redhat.com> writes:
> >> >    
> >> >>
> >> >> Please try reusing scratch CPU approach, see
> >> >>   kvm_arm_get_host_cpu_features()
> >> >> for an example. You will very likely end up with simpler series,
> >> >> compared to reinventing wheel.    
> >> >
> >> > Even if I do that (and I serioulsy doubt it's going to be easier than
> >> > just adding two 'u64's, kvm_arm_get_host_cpu_features() alone is 200
> >> > lines long) this is not going to give us what we need to distinguish
> >> > between
> >> >
> >> > 'hv-passthrough,hv-evmcs'
> >> >
> >> > and 
> >> >
> >> > 'hv-passthrough'
> >> >
> >> > when 'hv-evmcs' *is* supported by the host. When guest CPU lacks VMX we
> >> > don't want to enable it unless it was requested explicitly (former but
> >> > not the later).    
> >> 
> >> ... and if for whatever reason we decide that this is also bad/not
> >> needed, I can just drop patches 16-18 from the series (leaving
> >> 'hv-passthrough,hv-feature=off' problem to better times).  
> > that's also an option,
> > we would need to make sure that hv-passthrough is mutually exclusive
> > with ''all'' other hv- properties to avoid above combination being
> > ever (mis)used.  
> 
> That's an option to finally get these patches merged, not a good option
> for end users. 
> 
> 'hv-passthrough,hv-feature' works today and it's useful. Should we drop
> it?
well,
try suggested idea about using scratch CPU and it might get merged sooner.
(it's not like I'm suggesting you to rewrite half of QEMU, just some of
patches, which most likely would simplify series from my point of view
and would be easier to maintain)

> 
> 'hv-passthrough/hv-default' and 'hv-passthrough/hv-default,hv-evmcs'
> should give us sane results.
> 
> 'hv-passthrough,hv-feature=off' is convenient.
> 
> Why droppping this all? To save 9 (nine) lines of code in the parser? 
it's doing what generic property parsing is capable off, provided you
fish out hv-passthrough value in advance like arm/virt does (I think ppc
does similar hing also), so I consider it as unnecessary code duplication/
complication and maintenance burden.

If it were a hotfix during hard-freeze may be I'd agree (with promise to
rework it later to something more palatable), but it's not, for patches in
state they are now I'm not confident enough to ACK them.