Re: [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 10/27/2017 02:31 PM, Halil Pasic wrote:
> 
> 
> On 10/25/2017 08:13 PM, Jason J. Herne wrote:
>> On 10/20/2017 10:54 AM, Christian Borntraeger wrote:
>>> Starting a guest with
>>>     <os>
>>>      <type arch='s390x' machine='s390-ccw-virtio-2.9'>hvm</type>
>>>    </os>
>>>    <cpu mode='host-model'/>
>>>
>>> on an IBM z14 results in
>>>
>>> "qemu-system-s390x: Some features requested in the CPU model are not
>>> available in the configuration: gs"
>>>
>>> This is because guarded storage is fenced for compat machines that did not have
>>> guarded storage support, but libvirt expands the cpu model according to the
>>> latest available machine.
>>>
>>> While this prevents future migration abort (by not starting the guest at all),
>>> not being able to start a "host-model" guest is very much unexpected.  As it
>>> turns out, even if we would modify libvirt to not expand the cpu model to
>>> contain "gs" for compat machines, it cannot guarantee that a migration will
>>> succeed. For example if the kernel changes its features (or the user has
>>> nested=1 on one host but not on the other) the migration will fail
>>> nevertheless.  So instead of fencing "gs" for machines <= 2.9 lets allow it for
>>> all machine types that support the CPU model. This will make "host-model"
>>> runnable all the time, while relying on the CPU model to reject invalid
>>> migration attempts.
>> ...
>>> -    if (gs_allowed()) {
>>> +    if (cpu_model_allowed()) {
>>>           if (kvm_vm_enable_cap(s, KVM_CAP_S390_GS, 0) == 0) {
>>>               cap_gs = 1;
> 
> 
> @Jason
> 
> Hi Jason,
> 
> I don't have access to a z14 at the moment, and since you do, I would
> like to try out something.
> 
> I will first describe my concern, and then the test scenario.
> 
> The last line above, cap_gs = 1, has the side effect of returning
> true ever after.
> 
> int kvm_s390_get_gs(void)                                                       
> {                                                                               
>     return cap_gs;                                                              
> }  
> 
> Now considering
> static bool gscb_needed(void *opaque)
> {
>     return kvm_s390_get_gs();
> }

Yes, we should also replace that with

 return s390_has_feat(S390_FEAT_GUARDED_STORAGE)

I can fixup my patch or provide a 2nd one.