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

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

On 20/09/12 16:13, Alexander Graf wrote:
> 
> 
> On 20.09.2012, at 14:49, Christian Borntraeger <borntraeger@de.ibm.com> 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.

Maybe I should do both. Update the spec that the CAP defines if this is any
useful, if not it will start out with zeroes,but also having a single CAP check
during startup to have the code cleaner.

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

I looked for that in i386, but seems that only ppc does it. So I will look more
into ppc....

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

Yep, obviously the commenting was not enough to explain the why.
> 
>>
>> 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?

A reset does not, a reset clear does.

But you just got me convinced, that we should really follow the level statement
for the put function and simply write everything for KVM_PUT_FULL_STATE and
KVM_PUT_RESET_STATE. The KVM_EXIT_S390_RESET handler must then do the right
thing (e.g. making sure that the general purpose register content for a
non-clear reset is not changed). Will look if that works out.

Christian