Re: [qemu-s390x] [PATCH v4] s390: diagnose 318 info reset and migration support

[Christian Borntraeger]

On 13.05.19 11:57, David Hildenbrand wrote:
> On 13.05.19 11:51, Christian Borntraeger wrote:
>>
>>
>> On 13.05.19 11:40, David Hildenbrand wrote:
>>> On 13.05.19 11:34, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 13.05.19 10:03, David Hildenbrand wrote:
>>>>>>> +    if ((SCCB_SIZE - sizeof(ReadInfo)) / sizeof(CPUEntry) < 
>>>>>>> S390_MAX_CPUS)
>>>>>>> +        mc->max_cpus = S390_MAX_CPUS - 8;
>>>>>>
>>>>>> This is too complicated, just set it always to 240.
>>>>>>
>>>>>> However, I am still not sure how to best handle this scenario. One
>>>>>> solution is
>>>>>>
>>>>>> 1. Set it statically to 240 for machine > 4.1
>>>>>> 2. Keep the old machines unmodifed
>>>>>> 3. Don't indicate the CPU feature for machines <= 4.0
>>>>>>
>>>>>> #3 is the problematic part, as it mixes host CPU features and machines.
>>>>>> Bad. The host CPU model should always look the same on all machines. I
>>>>>> don't like this.
>>>>>>
>>>>>
>>>>> FWIW, #3 is only an issue when modeling it via the CPU model, like
>>>>> Christian suggested.
>>>>>
>>>>> I suggest the following
>>>>>
>>>>> 1. Set the max #cpus for 4.1 to 240 (already done)
>>>>> 2. Keep it for the other machines unmodified (as suggested by Thomas)
>>>>> 3. Create the layout of the SCCB depending on the machine type (to be 
>>>>> done)
>>>>>
>>>>> If we want to model diag318 via a CPU feature (which makes sense for
>>>>> migration):
>>>>>
>>>>> 4. Disable diag318 with a warning if used with a machine < 4.1
>>>>>
>>>>
>>>> I think there is a simpler solution. It is perfectly fine to fail the 
>>>> startup
>>>> if we cannot fulfil the cpu model. So lets just allow 248 and allow this 
>>>> feature 
>>>> also for older machines. And if somebody chooses both at the same time,
>>>> lets fails the startup.
>>>
>>> To which knob do you want to glue the layout of the SCLP response? Like
>>> I described?  Do you mean instead of warning and masking the feature off
>>> as I suggested, simply failing?
>>
>> The sclp response will depend on the dia318 cpu model flag. If its on, the 
>> sclp
>> response will have it, otherwise not.
>> - host-passthrough: not migration safe anyway
>> - host-model: if the target has diag318 good, otherwise we reject migration 
>>>
>>> In that case, -machine ..-4.0 -cpu host will not work on new HW with new
>>> KVM. Just noting.
>>
>> Only if you have 248 CPUs (which is unlikely). My point was to do that for 
>> all
>> machine levels.
>>
> 
> The issue with this approach is that e.g. libvirt is not aware of this
> restriction. It could query "max_cpus" and expand the host-cpu model,
> but starting a guest with > 240 cpus would fail. Maybe this is acceptable.

As of today we do the cpu model check in the same way. libvirt actually tries
to run QEMU and handles failures.

For a failure, the user still has still to use >240 CPUs in its XML. The only 
downside
is that libvirt will not reject this right away.

During startup we would then print an error message like

"The diag318 cpu feature is only supported for 240 and less CPUs."

This is of similar quality as
"Selected CPU GA level is too new. Maximum supported model in the 
configuration: \'%s\'",

and others that we have today.

So yes, I think this would be acceptable.



> 
> The approach you describe is actually pretty nice. We would have to
> 
> 1. Modify SCLP code to change the layout based on the feature availability
> 2. Check against the cpus when trying to enable the cpu model.
>