On 26.06.19 16:22, Collin Walling wrote: > On 6/26/19 8:14 AM, Cornelia Huck wrote: >> On Wed, 26 Jun 2019 11:12:04 +0200 >> Christian Borntraeger <[email protected]> wrote: >> >>> On 25.06.19 17:17, Collin Walling wrote: >>>> index a606547..4c26754 100644 >>>> --- a/target/s390x/cpu.h >>>> +++ b/target/s390x/cpu.h >>>> @@ -39,7 +39,13 @@ >>>> >>>> #define MMU_USER_IDX 0 >>>> >>>> -#define S390_MAX_CPUS 248 >>>> +/* >>>> + * HACK: The introduction of additional facility bytes in the Read Info >>>> + * struct consumes space used for CPU entries, thus we must reduce the >>>> + * original maximum CPUs of 248 by one for each new byte or risk smashing >>>> + * the stack. >>>> + */ >>>> +#define S390_MAX_CPUS 247 >>> >>> I think we decided to not change that. Only if the cpu model contains the >>> diag318 >>> feature we are limited to 247 but only for the sclp response. >>> So we said: >>> - we continue to allow 248 cpus >>> - the sclp response will be limited to 247 CPUs if the feature is one >>> - (optional) we print a warning that the guest might not see all CPUs >>> >> >> Yes, that's what I remember as well... and printing/logging a warning >> is a good idea. >> > > I recall this conversation, but I encountered a bit of a hangup when > running some tests with the new changes. > > Since we're adding a new field in the ReadInfo struct, we're permanently > intruding on the space used for CPU entries. A machine with these > changes and 248 CPUs resulted in stack smash when the guest would start > up. This happened with diag318 on *and* off. This is a limitation to the > 4k SCCB size right now :/
I think we discussed to only indicate 247 CPUs, which should solve the issue? > > Prior to these patches, I was restricting the max_cpus depending on the > compat version. I failed to do some tests with earlier versions to catch > this error (there are a lot of moving parts... sorry). > > I do not think the new byte and the full 248 CPU count can co-exist. We > might be able to union byte 134 and some extra space with the first CPU > entry... but that could get messy. > > -- Thanks, David / dhildenb
