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

[Christian Borntraeger]

[...]
>> --- 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