[v1] cpu_s390: Implement getVendorForModel for IBM Z

[PATCH] cpu_s390: Implement getVendorForModel for IBM Z

Posted by Thomas Huth 1 year, 5 months ago

When running "virsh domcapabilities" on a s390x host, all the CPU
models show up with vendor='unknown' - which sounds kind of weird
since the vendor of these mainframe CPUs is well known: IBM.
All CPUs starting with either "z" or "gen" match a real mainframe
CPU by IBM, so let's return the string "IBM" for those now.
The only remaining ones are now the artifical "qemu" and "max"
models from QEMU itself, so it should be OK to get an "unknown"
vendor for those two.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 src/cpu/cpu_s390.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c
index d908a83928..7416ec6dc5 100644
--- a/src/cpu/cpu_s390.c
+++ b/src/cpu/cpu_s390.c
@@ -109,6 +109,16 @@ virCPUs390ValidateFeatures(virCPUDef *cpu)
 }
 
 
+static const char *
+virCPUs390GetVendorForModel(const char *modelName)
+{
+    if (modelName[0] == 'z' || STREQLEN(modelName, "gen", 3))
+        return "IBM";
+
+    return NULL;
+}
+
+
 struct cpuArchDriver cpuDriverS390 = {
     .name = "s390",
     .arch = archs,
@@ -119,4 +129,5 @@ struct cpuArchDriver cpuDriverS390 = {
     .baseline   = NULL,
     .update     = virCPUs390Update,
     .validateFeatures = virCPUs390ValidateFeatures,
+    .getVendorForModel = virCPUs390GetVendorForModel,
 };
-- 
2.31.1

Re: [PATCH] cpu_s390: Implement getVendorForModel for IBM Z

Posted by Boris Fiuczynski 1 year, 5 months ago

Hi Thomas,
thanks for catching this. I agree with the changes but your patch is 
missing the required changes in tests.
I have a patch added as attachment which you can use for squashing the 
required changes into your patch.

With the tests fixed
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>

On 11/24/22 3:07 PM, Thomas Huth wrote:
> When running "virsh domcapabilities" on a s390x host, all the CPU
> models show up with vendor='unknown' - which sounds kind of weird
> since the vendor of these mainframe CPUs is well known: IBM.
> All CPUs starting with either "z" or "gen" match a real mainframe
> CPU by IBM, so let's return the string "IBM" for those now.
> The only remaining ones are now the artifical "qemu" and "max"
> models from QEMU itself, so it should be OK to get an "unknown"
> vendor for those two.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   src/cpu/cpu_s390.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c
> index d908a83928..7416ec6dc5 100644
> --- a/src/cpu/cpu_s390.c
> +++ b/src/cpu/cpu_s390.c
> @@ -109,6 +109,16 @@ virCPUs390ValidateFeatures(virCPUDef *cpu)
>   }
>   
>   
> +static const char *
> +virCPUs390GetVendorForModel(const char *modelName)
> +{
> +    if (modelName[0] == 'z' || STREQLEN(modelName, "gen", 3))
> +        return "IBM";
> +
> +    return NULL;
> +}
> +
> +
>   struct cpuArchDriver cpuDriverS390 = {
>       .name = "s390",
>       .arch = archs,
> @@ -119,4 +129,5 @@ struct cpuArchDriver cpuDriverS390 = {
>       .baseline   = NULL,
>       .update     = virCPUs390Update,
>       .validateFeatures = virCPUs390ValidateFeatures,
> +    .getVendorForModel = virCPUs390GetVendorForModel,
>   };
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] cpu_s390: Implement getVendorForModel for IBM Z

Posted by Thomas Huth 1 year, 5 months ago

On 24/11/2022 17.34, Boris Fiuczynski wrote:
> Hi Thomas,
> thanks for catching this. I agree with the changes but your patch is missing 
> the required changes in tests.

Ooops, sorry, it's been quite a while since I did my last libvirt patch, I 
completely forgot about that part ...

> I have a patch added as attachment which you can use for squashing the 
> required changes into your patch.

Thanks, I'll add that and send a v2.

  Thomas


> With the tests fixed
> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> 
> On 11/24/22 3:07 PM, Thomas Huth wrote:
>> When running "virsh domcapabilities" on a s390x host, all the CPU
>> models show up with vendor='unknown' - which sounds kind of weird
>> since the vendor of these mainframe CPUs is well known: IBM.
>> All CPUs starting with either "z" or "gen" match a real mainframe
>> CPU by IBM, so let's return the string "IBM" for those now.
>> The only remaining ones are now the artifical "qemu" and "max"
>> models from QEMU itself, so it should be OK to get an "unknown"
>> vendor for those two.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   src/cpu/cpu_s390.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c
>> index d908a83928..7416ec6dc5 100644
>> --- a/src/cpu/cpu_s390.c
>> +++ b/src/cpu/cpu_s390.c
>> @@ -109,6 +109,16 @@ virCPUs390ValidateFeatures(virCPUDef *cpu)
>>   }
>> +static const char *
>> +virCPUs390GetVendorForModel(const char *modelName)
>> +{
>> +    if (modelName[0] == 'z' || STREQLEN(modelName, "gen", 3))
>> +        return "IBM";
>> +
>> +    return NULL;
>> +}
>> +
>> +
>>   struct cpuArchDriver cpuDriverS390 = {
>>       .name = "s390",
>>       .arch = archs,
>> @@ -119,4 +129,5 @@ struct cpuArchDriver cpuDriverS390 = {
>>       .baseline   = NULL,
>>       .update     = virCPUs390Update,
>>       .validateFeatures = virCPUs390ValidateFeatures,
>> +    .getVendorForModel = virCPUs390GetVendorForModel,
>>   };
>>
> 
>