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

[David Hildenbrand]

On 17.01.2018 17:04, Christian Borntraeger wrote:
> 
> 
> 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.
> 

christian++ :)

-- 

Thanks,

David / dhildenb