Re: [Qemu-devel] [PATCH v1 2/2] s390x/cpumodel: improve defintion search without an IBC

[David Hildenbrand]

On 01.06.2017 19:06, Jason J. Herne wrote:
> On 05/31/2017 03:34 PM, David Hildenbrand wrote:
>> Currently, under z/VM on a 0x2827, QEMU will detect a 0x2828 if no
>> IBC value is provided. QEMU will simply take the last model of that HW
>> generation, which happens to be the BC version.
>>
>> Let's improve our search for that case by selecting the latest CPU
>> definition that matches the CPU type. This might still detect e.g. a
>> GA2 version on a GA1 system, but as we don't have further information at
>> hand, there isn't too much we can do about it.
>>
> 
> Isn't it better to choose the earliest machine possible in this case? 
> Systems
> are backwards compatible far more often than they are forwards compatible.
> 
> Said in a more verbose way, I'd rather pretend to be a GA1 on a GA2 
> (ignoring
> some new features) than pretend to be a GA2 on a GA1 and attempt to use
> something that does not exist.
> 
> I'm sure you've thought of all of this though, so what am I missing?
> 

Hi Jason,

excellent question. And of course I thought of it:

Fact 1: All GAX models share the same base feature set. A GAX++ might
support "more features".

Fact 2: Without an IBC, the guest can't detect the GA version.

a) If we have no IBC (esp. unblocked_ibc == 0), the IBC we will present
to the guest in read_SCP_info() will be 0. The guest will not know which
GA version it has. The problem of missing IBC propagates.

b) If we don't have a feature of the GA++ version, also our guest won't
have it. So in summary, the guest also has no idea of its GA version.

So this gives more "flexibility" and is backwards compatible.

Think about it like this: You're running on 0x2827 GA2.

Old QEMU version indicated "0x2828 GA1 == 0x2827 GA2". After you updated
QEMU, you suddenly detect "0x2827 GA1". You're previous libvirt guest
might suddenly refuse to run.

So it boils down to "it's just a QEMU representation that is more
flexible, the guest can't see the difference".

Thanks!

> 
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  target/s390x/cpu_models.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 9e23535..b6220c8 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -184,6 +184,7 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, 
>> uint8_t gen, uint8_t ec_ga,
>>                                      S390FeatBitmap features)
>>  {
>>      const S390CPUDef *last_compatible = NULL;
>> +    const S390CPUDef *matching_cpu_type = NULL;
>>      int i;
>>
>>      if (!gen) {
>> @@ -218,8 +219,16 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, 
>> uint8_t gen, uint8_t ec_ga,
>>          if (def->type == type && def->ec_ga == ec_ga) {
>>              return def;
>>          }
>> +        /* remember if we've at least seen one with the same cpu type */
>> +        if (def->type == type) {
>> +            matching_cpu_type = def;
>> +        }
>>          last_compatible = def;
>>      }
>> +    /* prefer the model with the same cpu type, esp. don't take the BC for 
>> EC */
>> +    if (matching_cpu_type) {
>> +        return matching_cpu_type;
>> +    }
>>      return last_compatible;
>>  }
>>
> 


-- 

Thanks,

David