Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer

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

[...]
>> --- a/target-s390x/kvm.c
>> +++ b/target-s390x/kvm.c
>> @@ -88,50 +88,77 @@ void kvm_arch_reset_vcpu(CPUS390XState *env)
>>     /* FIXME: add code to reset vcpu. */
>> }
>>
>> +/* we want to have the prefix, the GPRS, the ACRS and the CRS up to date */
>> +#define QEMU_NEEDED_REGS  (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | \
>> +                         KVM_SYNC_ACRS   | KVM_SYNC_CRS)
>> +
>> +/* But qemu only changes the GPRS */
>> +#define QEMU_DIRTY_REGS  (KVM_SYNC_GPRS)
>> +
>> int kvm_arch_put_registers(CPUS390XState *env, int level)
>> {
>>     struct kvm_regs regs;
>>     int ret;
>>     int i;
>>
>> -    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
>> -    if (ret < 0) {
>> -        return ret;
>> -    }
>> -
>> -    for (i = 0; i < 16; i++) {
>> -        regs.gprs[i] = env->regs[i];
>> -    }
>> -
>> -    ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, &regs);
>> -    if (ret < 0) {
>> -        return ret;
>> -    }
>> -
>>     env->kvm_run->psw_addr = env->psw.addr;
>>     env->kvm_run->psw_mask = env->psw.mask;
>>
>> -    return ret;
>> +    if ((env->kvm_run->kvm_valid_regs & QEMU_NEEDED_REGS) == QEMU_NEEDED_REGS) {
> 
> Isn't this also missing a check for KVM_CAP_SYNC_REGS?

I ommitted the check for two reasons: 
- since kvm_check_extension is an ioctl by itself, this would counter the original idea of sync regs
- the kvm_run structure contains zeroes at the sync reg location in kernels that dont support that,
  (the area was unused and the page was allocated with GFP_ZERO) 
  So by having any of the kvm_valid_regs != 0 we know that SYNC REGS must be supported


> Also, on reset we probably also want to write the other registers back, right?

Well, on reset we call the initial cpu reset ioctl, which does reset the other registers mandated
by a cpu reset. Furthermore, it is not different to the current implementation....

What we might want to do is to also consider a clear reset (which also zeroes the FPRs and ARs as
well as memory etc) as an additional option. But this should not be the default, e.g. to make
kdump or standalone dump possible.
I think this would then be an addon-patch by itself.

> Alex

Christian