[Qemu-devel] [PATCH] s390: use sync regs for register transfer
From: Christian Borntraeger borntrae...@de.ibm.com
Newer kernels provide the guest registers in kvm_run. Lets use
those if available (i.e. the capability is set). 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 update the registers according to the level statement,
i.e. we put all registers for KVM_PUT_FULL_STATE and _RESET_STATE.
Signed-off-by: Christian Borntraeger borntrae...@de.ibm.com
Signed-off-by: Jens Freimann jf...@linux.vnet.ibm.com
---
target-s390x/kvm.c | 119 -
1 file changed, 99 insertions(+), 20 deletions(-)
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 07edf93..321ab49 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -67,8 +67,11 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
KVM_CAP_LAST_INFO
};
+static int cap_sync_regs;
+
int kvm_arch_init(KVMState *s)
{
+cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
return 0;
}
@@ -88,49 +91,125 @@ 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_sregs sregs;
struct kvm_regs regs;
int ret;
int i;
-ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, regs);
-if (ret 0) {
-return ret;
-}
+/* always save the PSW and the GPRS*/
+env-kvm_run-psw_addr = env-psw.addr;
+env-kvm_run-psw_mask = env-psw.mask;
-for (i = 0; i 16; i++) {
-regs.gprs[i] = env-regs[i];
+if (cap_sync_regs env-kvm_run-kvm_valid_regs KVM_SYNC_GPRS) {
+for (i = 0; i 16; i++) {
+env-kvm_run-s.regs.gprs[i] = env-regs[i];
+env-kvm_run-kvm_dirty_regs |= KVM_SYNC_GPRS;
+}
+} 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;
+}
}
-ret = kvm_vcpu_ioctl(env, KVM_SET_REGS, regs);
-if (ret 0) {
-return ret;
+/* Do we need to save more than that? */
+if (level == KVM_PUT_RUNTIME_STATE) {
+return 0;
}
-env-kvm_run-psw_addr = env-psw.addr;
-env-kvm_run-psw_mask = env-psw.mask;
+if (cap_sync_regs
+env-kvm_run-kvm_valid_regs KVM_SYNC_ACRS
+env-kvm_run-kvm_valid_regs KVM_SYNC_CRS) {
+for (i = 0; i 16; i++) {
+env-kvm_run-s.regs.acrs[i] = env-aregs[i];
+env-kvm_run-s.regs.crs[i] = env-cregs[i];
+}
+env-kvm_run-kvm_dirty_regs |= KVM_SYNC_ACRS;
+env-kvm_run-kvm_dirty_regs |= KVM_SYNC_CRS;
+} else {
+for (i = 0; i 16; i++) {
+sregs.acrs[i] = env-aregs[i];
+sregs.crs[i] = env-cregs[i];
+}
+ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, sregs);
+if (ret 0) {
+return ret;
+}
+}
-return ret;
+/* Finally the prefix */
+if (cap_sync_regs env-kvm_run-kvm_valid_regs KVM_SYNC_PREFIX) {
+env-kvm_run-s.regs.prefix = env-psa;
+env-kvm_run-kvm_dirty_regs |= KVM_SYNC_PREFIX;
+} else {
+/* prefix is only supported via sync regs */
+}
+return 0;
}
int kvm_arch_get_registers(CPUS390XState *env)
{
-int ret;
+struct kvm_sregs sregs;
struct kvm_regs regs;
+int ret;
int i;
-ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, regs);
-if (ret 0) {
-return ret;
+/* get the PSW */
+env-psw.addr = env-kvm_run-psw_addr;
+env-psw.mask = env-kvm_run-psw_mask;
+
+/* the GPRS */
+if (cap_sync_regs env-kvm_run-kvm_valid_regs KVM_SYNC_GPRS) {
+for (i = 0; i 16; i++) {
+env-regs[i] = env-kvm_run-s.regs.gprs[i];
+}
+} else {
+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];
+}
}
-for (i = 0; i 16; i++) {
-env-regs[i] = regs.gprs[i];
+/* The ACRS and CRS */
+if (cap_sync_regs
+env-kvm_run-kvm_valid_regs KVM_SYNC_ACRS
+env-kvm_run-kvm_valid_regs KVM_SYNC_CRS) {
+for (i = 0; i 16; i++) {
+env-aregs[i] = env-kvm_run-s.regs.acrs[i];
+env-cregs[i] = env-kvm_run-s.regs.crs[i];
+}
+} else {
+ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, sregs);
+
Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer
On 22.08.2012, at 13:54, Jens Freimann wrote:
From: Christian Borntraeger borntrae...@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 borntrae...@de.ibm.com
Signed-off-by: Jens Freimann jf...@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
Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer
[...]
--- 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
Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer
On 20.09.2012, at 14:49, Christian Borntraeger borntrae...@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
Re: [Qemu-devel] [PATCH] s390: use sync regs for register transfer
On 20/09/12 16:13, Alexander Graf wrote:
On 20.09.2012, at 14:49, Christian Borntraeger borntrae...@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
[Qemu-devel] [PATCH] s390: use sync regs for register transfer
From: Christian Borntraeger borntrae...@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 borntrae...@de.ibm.com
Signed-off-by: Jens Freimann jf...@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)
{
+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;
}
--
1.7.11.5
