Re: [Qemu-devel] [libvirt] Question about the host-model CPU mode

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 10/20/2017 04:12 PM, Christian Borntraeger wrote:
> 
> 
> On 10/20/2017 04:06 PM, David Hildenbrand wrote:
>> On 20.10.2017 16:02, Christian Borntraeger wrote:
>>>
>>>
>>> On 10/20/2017 03:51 PM, David Hildenbrand wrote:
>>> [...]
>>>>> The problem goes much further.
>>>>> A fresh guest with
>>>>>
>>>>>     <os>
>>>>>      <type arch='s390x' machine='s390-ccw-virtio-2.9'>hvm</type>
>>>>>    </os>
>>>>>    <cpu mode='host-model'/>
>>>>>
>>>>> does not start. No migration from an older system is necessary.
>>>>>
>>>>
>>>> Yes, as stated in the documentation "copying host CPU definition from
>>>> capabilities XML" this can not work. And it works just as documented.
>>>> Not saying this is a nice thing :)
>>>>
>>>> I think we should try to fix gs_allowed (if possible) and avoid
>>>> something like that in the future. This would avoid other complexity
>>>> involved when suddenly having X host models.
>>>
>>> Maybe this one is really a proper fix. It will allow the guest to start
>>> and on migration the cpu model will complain if the target cannot provide gs.
>>> Similar things can happen if - for example - the host kernel lacks some features.
>>
>> Right, just what I had in mind.
>>
>>>
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 77169bb..97a08fa 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -430,7 +430,6 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>>>      s390mc->ri_allowed = true;
>>>      s390mc->cpu_model_allowed = true;
>>>      s390mc->css_migration_enabled = true;
>>> -    s390mc->gs_allowed = true;
>>>      mc->init = ccw_init;
>>>      mc->reset = s390_machine_reset;
>>>      mc->hot_add_cpu = s390_hot_add_cpu;
>>> @@ -509,12 +508,6 @@ bool cpu_model_allowed(void)
>>>      return get_machine_class()->cpu_model_allowed;
>>>  }
>>>  
>>> -bool gs_allowed(void)
>>> -{
>>> -    /* for "none" machine this results in true */
>>> -    return get_machine_class()->gs_allowed;
>>> -}
>>> -
>>>  static char *machine_get_loadparm(Object *obj, Error **errp)
>>>  {
>>>      S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>>> @@ -757,7 +750,6 @@ static void ccw_machine_2_9_class_options(MachineClass *mc)
>>>  {
>>>      S390CcwMachineClass *s390mc = S390_MACHINE_CLASS(mc);
>>>  
>>> -    s390mc->gs_allowed = false;
>>>      ccw_machine_2_10_class_options(mc);
>>>      SET_MACHINE_COMPAT(mc, CCW_COMPAT_2_9);
>>>      s390mc->css_migration_enabled = false;
>>> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
>>> index a9a90c2..1de53b0 100644
>>> --- a/include/hw/s390x/s390-virtio-ccw.h
>>> +++ b/include/hw/s390x/s390-virtio-ccw.h
>>> @@ -40,7 +40,6 @@ typedef struct S390CcwMachineClass {
>>>      bool ri_allowed;
>>>      bool cpu_model_allowed;
>>>      bool css_migration_enabled;
>>> -    bool gs_allowed;
>>>  } S390CcwMachineClass;
>>>  
>>>  /* runtime-instrumentation allowed by the machine */
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index a0d5052..3f13fc2 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -362,7 +362,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>              cap_ri = 1;
>>>          }
>>>      }
>>> -    if (gs_allowed()) {
>>> +    if (cpu_model_allowed()) {
>>>          if (kvm_vm_enable_cap(s, KVM_CAP_S390_GS, 0) == 0) {
>>>              cap_gs = 1;
>>>          }
>>>
>>
>> And the last hunk makes sure we keep same handling for machines without
>> CPU model support and therefore no way to mask support. For all recent
>> machines, we expect CPU model checks to be in place.
>>
>> Will have to think about this a bit more. Will you send this as a proper
>> patch?
> 
> After thinking about it :-)
> 

I intend to put some brain-power in this too. Probably next week.

My general impression is, that I have a at places different understanding
of how things should work compared to David. Especially when it comes
to this concept of persistent copying, and also an end-user-digestible
definition of what host-model does. (I think this with copying capabilities
from whatever xml which is subject to convoluted caching is a bit too
hard to digest for an end user not involved in the development of qemu
and libvirt).

I had a conversation with Boris a couple of hours ago, and it seems, things
are pretty convoluted.

If I understand the train of thought here (David) it can be summarized like this:
For a certain QEMU binary no aspect of the cpu-models may depend on the
machine type. In particular, compat properties and compat handling is
not alowed to alter cpu-models (whether the available cpu models nor the
capabilities of each of these).

This we would have to make a part of the external interface. That way
one could be sure that the 'cpu capabilities' are machine-type independent
(that is, the same for all the machine types).

Or did I get this completely wrong? (Based on the answer branches my
think).

Halil