Re: [Qemu-devel] [PATCH v2] s390x/cpumodel: model PTFF subfunctions for Multiple-epoch facility

[David Hildenbrand]

On 06.02.2018 18:19, Cornelia Huck wrote:
> On Mon, 5 Feb 2018 13:37:17 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> On 05.02.2018 13:22, Cornelia Huck wrote:
>>> On Mon, 5 Feb 2018 12:27:33 +0100
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> On 05.02.2018 12:22, Christian Borntraeger wrote:  
>>>>> Looks sane on a z14.
>>>>> Tested-by: Christian Borntraeger <address@hidden>
>>>>>
>>>>>
>>>>> On 02/05/2018 11:29 AM, David Hildenbrand wrote:    
>>>>>> --- a/target/s390x/kvm.c
>>>>>> +++ b/target/s390x/kvm.c
>>>>>> @@ -2221,6 +2221,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel 
>>>>>> *model, Error **errp)
>>>>>>          return;
>>>>>>      }
>>>>>>
>>>>>> +    /* PTFF subfunctions might be indicated although kernel support 
>>>>>> missing */
>>>>>> +    if (!test_bit(S390_FEAT_MULTIPLE_EPOCH, model->features)) {
>>>>>> +        clear_bit(S390_FEAT_PTFF_QSIE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_QTOUE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_STOE, model->features);
>>>>>> +        clear_bit(S390_FEAT_PTFF_STOUE, model->features);
>>>>>> +    }
>>>>>> +
>>>>>>      /* with cpu model support, CMM is only indicated if really 
>>>>>> available */
>>>>>>      if (kvm_s390_cmma_available()) {
>>>>>>          set_bit(S390_FEAT_CMM, model->features);
>>>>>>    
>>>>>
>>>>> Do you also want to add something to check_consistency ?
>>>>>
>>>>> Right now the following user error 
>>>>> -cpu z14,mepoch=off,mepochptff=on
>>>>> is accepted.
>>>>> On the other hand we also have no consistency checks for other 
>>>>> subfunctions.
>>>>>     
>>>>
>>>> Thought about that, but that implies that a CPU model runable now, will
>>>> not run without warnings. Especially if migrating. We could add such
>>>> checks if we would push this into stable.
> 
> I'm currently wondering whether this change would actually be
> applicable and useful for stable. Given the way stable is usually used,
> probably not.
> 
>>>>  
>>>
>>> So, adding this check for the z14 stuff would work iff pushed into
>>> stable - but for the other subfunctions the ship has already sailed?
>>>   
>>
>> I don't know if we really have problems with other subfunctions. We
>> could also add consistency checks there (the problem here is that we
>> actually have to add missing subfunctions). So it is easier to check for
>> consistency with already existing subfunctions.
> 
> Hm, so not really worth the hassle, just keep this as-is (and apply
> this patch as-is)?
> 

Think this would be best. (if we would have considered this earlier, we
would now have "mepoch-base" (now "mepoch") and "mepoch" as
"mepoch-base,ptff-stoe,ptff-stoue..."), just as e.g. handled for the
tod-clock-steering features. But unfortunately we missed that.

Such bugs happen as the features are merged before there is chance to
rewiew documentation (before an updated PoP is out). It is what it is.

-- 

Thanks,

David / dhildenb