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

[Halil Pasic]

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();
}

const VMStateDescription vmstate_gscb = {
    .name = "cpu/gscb",
    .version_id = 1,
    .minimum_version_id = 1,
    .needed = gscb_needed,
    .fields = (VMStateField[]) {
        VMSTATE_UINT64_ARRAY(env.gscb, S390CPU, 4),
        VMSTATE_END_OF_LIST()
        }
};

const VMStateDescription vmstate_s390_cpu = {
    .name = "cpu",
    .post_load = cpu_post_load,
    .pre_save = cpu_pre_save,
    .version_id = 4,
    .minimum_version_id = 3,
    .fields      = (VMStateField[]) {
        VMSTATE_UINT64_ARRAY(env.regs, S390CPU, 16),
        VMSTATE_UINT64(env.psw.mask, S390CPU),
        VMSTATE_UINT64(env.psw.addr, S390CPU),
        VMSTATE_UINT64(env.psa, S390CPU),
        VMSTATE_UINT32(env.todpr, S390CPU),
        VMSTATE_UINT64(env.pfault_token, S390CPU),
        VMSTATE_UINT64(env.pfault_compare, S390CPU),
        VMSTATE_UINT64(env.pfault_select, S390CPU),
        VMSTATE_UINT64(env.cputm, S390CPU),
        VMSTATE_UINT64(env.ckc, S390CPU),
        VMSTATE_UINT64(env.gbea, S390CPU),
        VMSTATE_UINT64(env.pp, S390CPU),
        VMSTATE_UINT32_ARRAY(env.aregs, S390CPU, 16),
        VMSTATE_UINT64_ARRAY(env.cregs, S390CPU, 16),
        VMSTATE_UINT8(env.cpu_state, S390CPU),
        VMSTATE_UINT8(env.sigp_order, S390CPU),
        VMSTATE_UINT32_V(irqstate_saved_size, S390CPU, 4),
        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
                               irqstate_saved_size),
        VMSTATE_END_OF_LIST()
    },
    .subsections = (const VMStateDescription*[]) {
        &vmstate_fpu,
        &vmstate_vregs,
        &vmstate_riccb,
        &vmstate_exval,
        &vmstate_gscb,
        NULL
    },
};

I would expect the vmstate_gscb subsection being sent, even if gs is disabled
via cpu-model if kernel and possibly machine has gs support (and qemu
has cpu-models).

So the test scenario I want you to play trough is the following. Take
the latest-greatest qemu with this patch applied. Make sure gs works
(is provided to the guest) with a 2.9 machine version, and a fully
specified cpu-model. Now disable gs explicitly.

Try to migrate this to another machine having a 2.9 binary. I expect
the migration failing because, the subsection is going to be sent by
the latest-greatest binary, but is unknown to the 2.9 binary.

Notice this is despite the fact that gs is explicitly disabled.

Now that I think about it, maybe the 2.9 binary is going to reject
the explicit gs flag altogether, because it's unknown.

Isn't this a problem? I'm afraid like this the only migration-safe
variant is -base, but that would essentially make adding features
incrementally impossible. 

But I hypothesize trying to migrate with z13 or even z13-base
would also trigger the unknown subsection problem.

Unfortunately I can't test this because my kernel never
makes kvm_vm_enable_cap(s, KVM_CAP_S390_GS, 0) return 0, because
I lack HW support in the host.

Regards,
Halil


> 
> Ok, honestly, I dislike this idea because it means we are effectively lying 
> now. We will happily accept a +gs cpu model with a 2.9 compat machine when 
> the point of compat machines is to mimick the older version of Qemu which 
> does not support GS.  Yes, model checking will prevent the worst side 
> effects, namely, exploding migrations.
> 
> I'd far prefer a solution that makes host-model dependent on the machine 
> type. But I understand some of the backlash on that idea. Still, it seems 
> like the cleanest approach even if it will be more work.
> 
> With all of that said, I know we want this fixed and your patch seems to fix 
> the problem. So if you need an R-b...
> 
> Reviewed-by: Jason J. Herne <address@hidden>
> 
> Can we have a new tag? :-D
> Reviewed-by-with-reservations: Jason J. Herne <address@hidden>
>