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

[Alexander Graf]

On 20.09.2012, at 14:49, Christian Borntraeger <address@hidden> wrote:

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

But the spec explicitly says that the fields are only valid when the CAP is 
present. So if you think that is unnecessary, please patch the spec.

The way we get around constant ioctls for cap checks is that we usually check 
only once and remember the result in a global variable. Please check other 
arch's kvm.c files for reference.

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

The difference is that get_registers fetches more registers, so not putting 
them looks awkward.

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

What does reset on normal systems look like? Does it clear register state?

Alex

> 
>> Alex
> 
> Christian
>