Re: [Qemu-devel] [qemu-s390x] [PATCH v2 2/3] s390: cpu feature for diagnose 318 andlimit max VCPUs to 247

[David Hildenbrand]

On 11.12.18 22:12, Collin Walling wrote:
> On 12/11/18 11:47 AM, Collin Walling wrote:
>> On 12/7/18 7:08 AM, Cornelia Huck wrote:
>>> On Thu,  6 Dec 2018 17:24:17 -0500
>>> Collin Walling <address@hidden> wrote:
>>>
>>>> Diagnose 318 is a new z14.2 CPU feature. Since we are able to emulate
>>>> it entirely via KVM, we can add guest support for earlier models. A
>>>> new CPU feature for diagnose 318 (shortened to diag318) will be made
>>>> available to guests starting with the zEC12-full CPU model.
>>>>
>>>> The z14.2 adds a new read SCP info byte (let's call it byte 134) to
>>>> detect the availability of diag318. Because of this, we have room for
>>>> one less VCPU and thus limit the max VPUs supported in a configuration
>>>> to 247 (down from 248).
>>>>
>>>> Signed-off-by: Collin Walling <address@hidden>.
>>>> ---
>>>>  hw/s390x/sclp.c                 | 2 ++
>>>>  include/hw/s390x/sclp.h         | 2 ++
>>>>  target/s390x/cpu.h              | 2 +-
>>>>  target/s390x/cpu_features.c     | 3 +++
>>>>  target/s390x/cpu_features.h     | 1 +
>>>>  target/s390x/cpu_features_def.h | 3 +++
>>>>  target/s390x/gen-features.c     | 1 +
>>>>  target/s390x/kvm.c              | 1 +
>>>>  8 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index 8c2320e..594b4a4 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>> @@ -52,7 +52,7 @@
>>>>  
>>>>  #define MMU_USER_IDX 0
>>>>  
>>>> -#define S390_MAX_CPUS 248
>>>> +#define S390_MAX_CPUS 247
>>>
>>> Isn't that already problematic if you try to migrate from an older QEMU
>>> with all possible vcpus defined? IOW, don't you really need a way that
>>> older machines can still run with one more vcpu?
>>>
>>
>> Good call. I'll run some tests on this and see what happens. I'll report
>> here on those results.
>>
> 
> Migrating to a machine that supports less vCPUs will report
> 
> error: unsupported configuration: Maximum CPUs greater than specified machine 
> type limit
> 
> I revisited the code to see if there's a way to dynamically set the max vcpu 
> count based 
> on the read scp info size, but it gets really tricky and code looks very 
> complicated.
> (Having a packed struct contain the CPU entries whose maximum is determined 
> by hardware
> limitations makes things difficult -- but who said s390 is easy? :) )
> 
> In reality, do we often have guests running with 248 or even 247 vcpus? If 
> so, I imagine
> the performance isn't too significant?
Gluing CPU feature availability to machines is plain ugly. This sounds
like going back to pre-cpu model times ;)

There are two alternatives:

a) Don't model it as a CPU feature in QEMU. Glue it completely to the
QEMU machine. This goes hand-in-hand with the proposal I made in the KVM
thread, that diag318 is to be handled completely in QEMU, always. The
KVM setting part is optional (if KVM + HW support it).

Then we can have two different max_cpus/ReadInfo layouts based on the
machine type. No need to worry about QEMU cpu features.

Once we have other SCLP features (eventually requiring KVM/HW support)
announced in the same feature block, things might get more involved, but
I guess we could handle it somehow.


b) Glue the ReadInfo layout to the CPU feature, we would have to
default-disable the CPU feature for legacy machines. And bail out if
more CPUs are used when the feature is enabled. Hairy.


I guess a) would be the best thing to do. After all this really does not
sound like a CPU feature but more like a machine feature. But there is
usually a fine line between them.

-- 

Thanks,

David / dhildenb