Re: [qemu-s390x] [PATCH 2/3] s390x/kvm: Handle bpb feature

[Christian Borntraeger]

On 01/17/2018 04:10 PM, David Hildenbrand wrote:
> 
>>> And exactly for this reason I tend to nack patch nr 3 (if that is of any
>>> weight :) ).
>>
>> I have communicated the mistake to asll relevant parties - it will not 
>> happen again
>> (famous last words).
> 
> An I already saw it happen in the past. (I think I really have to dig
> out that one feature to make a point :P ). Mistakes happen, but we don't
> have to propagate them to customers if we can catch them early :)
> 
>>
>>>
>>> As soon as we enable bits for CPU models, we guarantee that migration
>>> works. While introducing this change we already had one example where
>>> this was not the case. Not good. (and remember having another such
>>> exception)
>>
>> The point is migration continues to work. In fact I had a different version
>> of this patch set that did it the other way around. Keep 82 a transparent
>> and add a new cpu misc facility that takes care of the migration state.
>>>
>>> It is easier to patch a feature in than silently enabling *anything*
>>> somebody thinks is transparent (but its not). Especially not for the
>>> host model. The expanded host model is migration safe.
>>
>> I really do not care about -cpu host (host-passthrough) for migration 
>> safety, 
>> because its not. And as you said: host-model (expanded) will work.
>>
> 
> It will if the world would be perfect.
> 
> expand "-cpu host" -> -cpu z14-base,stfle_82=on
> 
> stfle_82 would now not be properly migrated. Yes, it might work somehow
> right now. But it is not clean.
> 
>>>
>>> And as we saw, in the unlikely event of such heavy changes, we need to
>>> touch fw/linux/qemu either way.
>>>
>>> But there is more I dislike about the approach in patch 3:
>>>
>>> 1. feature names. We need aliases. Different QEMU versions on the same
>>> hw might end up not understanding what a feature means. (old one: only
>>> knows stfl_123, new one knows stfl_123 a.k.a crazy_feat)
>>
>> I plan to keep the old names. e.g. stfle131 is better than sea_esop2.
> 
> 
> Oh god no. With vx, te, iep one at least has a rough idea what is happening.
> 
> -cpu z14-base,stfle123,stfle234,stfle323 ... :(
> 
> 
> This all smells like a huge hack for a scenario that happened once. I
> prefer to do it the clean way. Enable only what you checked works and
> what you can actually give a name.
> 
> Especially we will lose the ability to know which bit was valid for
> which hardware generation - which is key when working with IBC.
> 
> I am not sure if giving all that up is worth it.
> 

I will spin up a second patch that enables stfle81 and name it "ppa15".
We can then discuss patch 3 on the slow path with enough time to think
about this.