diff mbox

s390: use sync regs for register transfer

Message ID 1345636483-6581-1-git-send-email-jfrei@linux.vnet.ibm.com
State New
Headers show

Commit Message

Jens Freimann Aug. 22, 2012, 11:54 a.m. UTC 
From: Christian Borntraeger <borntraeger@de.ibm.com>

Newer kernels provide the guest registers in kvm_run. Lets use
those if available. This avoids ioctls on cpu_synchronize_state
making intercepts faster.

In addition, we have now the prefix register, the access registers
the control registers up to date. This helps in certain cases,
e.g. for resolving kernel module addresses with gdb on a guest.

On return, we only update the gprs, since qemu does not change
prefix, crs and acrs. Blindly updating those might cause some
expensive flushing in the kernel.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 target-s390x/kvm.c | 75 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 24 deletions(-)

Comments

Alexander Graf Sept. 20, 2012, 12:19 p.m. UTC | #1 
On 22.08.2012, at 13:54, Jens Freimann wrote:

> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Newer kernels provide the guest registers in kvm_run. Lets use
> those if available. This avoids ioctls on cpu_synchronize_state
> making intercepts faster.
> 
> In addition, we have now the prefix register, the access registers
> the control registers up to date. This helps in certain cases,
> e.g. for resolving kernel module addresses with gdb on a guest.
> 
> On return, we only update the gprs, since qemu does not change
> prefix, crs and acrs. Blindly updating those might cause some
> expensive flushing in the kernel.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> ---
> target-s390x/kvm.c | 75 +++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 51 insertions(+), 24 deletions(-)
> 
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 07edf93..4598923 100644
> --- 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?

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


Alex
Christian Borntraeger Sept. 20, 2012, 12:49 p.m. UTC | #2 
[...]
>> --- 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
Alexander Graf Sept. 20, 2012, 2:13 p.m. UTC | #3 
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.

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
>
Christian Borntraeger Sept. 20, 2012, 3:21 p.m. UTC | #4 
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
diff mbox

Patch

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 07edf93..4598923 100644
--- 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) {
+        env->kvm_run->s.regs.prefix = env->psa;
+        for (i = 0; i < 16; i++) {
+            env->kvm_run->s.regs.gprs[i] = env->regs[i];
+            env->kvm_run->kvm_dirty_regs |= QEMU_DIRTY_REGS;
+        }
+    } else {
+        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;
+        }
+        /* no prefix available */
+    }
+    /* sregs unchanged */
+
+    return 0;
 }
 
 int kvm_arch_get_registers(CPUS390XState *env)
 {
     int ret;
     struct kvm_regs regs;
+    struct kvm_sregs sregs;
     int i;
 
-    ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
-    if (ret < 0) {
-        return ret;
-    }
-
-    for (i = 0; i < 16; i++) {
-        env->regs[i] = regs.gprs[i];
-    }
-
     env->psw.addr = env->kvm_run->psw_addr;
     env->psw.mask = env->kvm_run->psw_mask;
 
+    if ((env->kvm_run->kvm_valid_regs & QEMU_NEEDED_REGS) == QEMU_NEEDED_REGS) {
+        env->psa = env->kvm_run->s.regs.prefix;
+        for (i = 0; i < 16; i++) {
+            env->regs[i] = env->kvm_run->s.regs.gprs[i];
+            env->cregs[i] = env->kvm_run->s.regs.crs[i];
+            env->aregs[i] = env->kvm_run->s.regs.acrs[i];
+        }
+    } else {
+        ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
+        if (ret < 0) {
+            return ret;
+        }
+        ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
+        if (ret < 0) {
+            return ret;
+        }
+        for (i = 0; i < 16; i++) {
+            env->regs[i] = regs.gprs[i];
+            env->cregs[i] = sregs.crs[i];
+            env->aregs[i] = sregs.acrs[i];
+        }
+        /* no prefix available */
+    }
+
     return 0;
 }