Re: [Qemu-devel] [PATCH 12/23] ppc: move '-cpu foo, compat=xxx' parsing into ppc_cpu_parse_featurestr()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3828 bytes --]

On Fri, Oct 06, 2017 at 11:52:44AM +0200, Igor Mammedov wrote:
> On Fri, 6 Oct 2017 20:17:41 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Oct 06, 2017 at 11:03:52AM +0200, Igor Mammedov wrote:
> > > On Fri, 6 Oct 2017 14:54:47 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Thu, Oct 05, 2017 at 06:24:39PM +0200, Igor Mammedov wrote:  
> > > > > there is a dedicated callback CPUClass::parse_features
> > > > > which purpose is to convert -cpu features into a set of
> > > > > global properties AND deal with compat/legacy features
> > > > > that couldn't be directly translated into CPU's properties.
> > > > > 
> > > > > Create ppc variant of it (ppc_cpu_parse_featurestr) and
> > > > > move 'compat=val' handling from spapr_cpu_core.c into it.
> > > > > That removes a dependency of board/core code on cpu_model
> > > > > parsing and would let to reuse common -cpu parsing
> > > > > introduced by 6063d4c0
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > > 
> > > > Hrm.  I'm a bit unsure about this.  The fact that the board code is
> > > > involved in the parsing here is deliberate.  Basically 'compat=' never
> > > > made sense as a cpu property, it always should have been a machine
> > > > property.
> > > > 
> > > > With this patch we'll still (correctly) fail on a non spapr machine at
> > > > the point where we do:
> > > > 
> > > > +            object_property_set_str(machine, v, "max-cpu-compat", &local_err);
> > > > 
> > > > Though probably with a rather cryptic error.  
> > > I's possible to guard it and apply to only SPAPR machine
> > > or trying to find property first before setting it.  
> > 
> > Yeah, I think we should do that.  Or possibly we could do something
> > with the vhyp interface.  If there was ever a vhyp machine other than
> > spapr (unlikely) then compat modes would also make sense for it (vhyp
> > being non-NULL basically means that our vcpu is never allowed to enter
> > hypervisor mode, so hypervisor privileged registers like LPCR need to
> > be handled by the machine instead).
> I'll use cast to spapr machine here for now, and leave it up to you
> to change it in future to something else if it make sense.

Makes sense.  As long as we get the behaviour right for now, we can
clean up the implementation later.

> btw: when pvn + power9 crushes it does so due to null vhyp
> if I recall correctly

Yeah, pnv is very much a work in progress, it barely works for POWER8
and is hardly tested for POWER9.  It's also entirely possible we're
misisng bits in the POWER9 code to handle the !vhyp case, since
pseries is the only machine that's really been tested with POWER9 at
all so far.

> > > > It still pollutes the cpu code with spapr's past mistake though.  I'm
> > > > not sure if this is a good tradeoff.  
> > > CPUClass::parse_features callback was introduced to deal with 
> > > legacy and compatibility nuances of "-cpu" CLI option, so I'm moving
> > > compat mistake back where it belongs so it will pollute code dealing
> > > with hacks.  
> > 
> > Hm, ok.  Still seems a bit odd to me, but I'll take your word for it.
> all targets that have special handling for -cpu options like
> parse '+-foo' format or 'foo' or fixup value of property or rename property
> to new name do provide target specific CPUClass::parse_features callback
> to deal with it, the rest of the targets use generic
> cpu_common_parse_features().

Yeah, still smacks of the machine ~= target assumption that x86 people
tend to make.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]