[Qemu-devel] Re: [PATCH 0/9] S390x KVM support

[Alexander Graf]

On 20.10.2009, at 18:40, Carsten Otte wrote:

> Alexander Graf wrote:
> > This is the resulting code. Please comment on things you like and  
> > also on the
> > ones you don't :-).
> I've reviewed and tested it, great work Alex :-)

Thanks :-).

> > Also to actually run this code you need a patch for an ugly bug in  
> > the kernel
> > module: http://alex.csgraf.de/psw.patch
> Well, it was not exactly a bug. Kuli does'nt need the psw to be  
> updated in kvm_run at all times. Your hotfix updates the psw in a  
> union, even if the exit reason indicates that s390_sieic is not  
> valid. The patch at the end of this mail moves the PSW out of the  
> union. I think it makes most sense if Avi picks this patch and you  
> adopt your series to it. This way the user interface of the kvm  
> module works for both kuli and qemu.

Alright, expect v2 today!

On a sidenode, please post the patch to kvm-devel too, so Marcelo has  
the chance to pick it up.


> This patch moves s390 processor status word into the base kvm_run
> struct and keeps it up-to date on all userspace exits.
>
> Signed-off-by: Carsten Otte <address@hidden>
> ---
> arch/s390/kvm/kvm-s390.c |   14 ++++++++------
> include/linux/kvm.h      |    8 ++++++--
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> Index: kvm/include/linux/kvm.h
> ===================================================================
> --- kvm.orig/include/linux/kvm.h        2009-10-20 15:00:40.000000000 +0200
> +++ kvm/include/linux/kvm.h     2009-10-20 16:51:48.000000000 +0200
> @@ -7,6 +7,7 @@
> * Note: you must update KVM_API_VERSION if you change this interface.
> */
> +#include <linux/autoconf.h>
> #include <linux/types.h>
> #include <linux/compiler.h>
> #include <linux/ioctl.h>
> @@ -116,6 +117,11 @@
>         __u64 cr8;
>         __u64 apic_base;
> +#ifdef CONFIG_S390
> +       /* the processor status word for s390 */
> +       __u64 psw_mask; /* psw upper half */
> +       __u64 psw_addr; /* psw lower half */
> +#endif
>         union {
>                 /* KVM_EXIT_UNKNOWN */
>                 struct {
> @@ -167,8 +173,6 @@
>                 /* KVM_EXIT_S390_SIEIC */
>                 struct {
>                         __u8 icptcode;
> -                       __u64 mask; /* psw upper half */
> -                       __u64 addr; /* psw lower half */
>                         __u16 ipa;
>                         __u32 ipb;
>                 } s390_sieic;
> Index: kvm/arch/s390/kvm/kvm-s390.c
> ===================================================================
> --- kvm.orig/arch/s390/kvm/kvm-s390.c	2009-10-20 15:01:02.000000000  
> +0200
> +++ kvm/arch/s390/kvm/kvm-s390.c        2009-10-20 18:13:45.000000000 +0200
> @@ -421,7 +421,8 @@
>         if (atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_RUNNING)
>                 rc = -EBUSY;
>         else
> -               vcpu->arch.sie_block->gpsw = psw;
> +               vcpu->run->psw_mask = psw.mask;
> +               vcpu->run->psw_addr = psw.addr;
>         vcpu_put(vcpu);
>         return rc;
> }
> @@ -509,9 +510,6 @@
>         switch (kvm_run->exit_reason) {
>         case KVM_EXIT_S390_SIEIC:
> -               vcpu->arch.sie_block->gpsw.mask = kvm_run->s390_sieic.mask;
> -               vcpu->arch.sie_block->gpsw.addr = kvm_run->s390_sieic.addr;
> -               break;
>         case KVM_EXIT_UNKNOWN:
>         case KVM_EXIT_INTR:
>         case KVM_EXIT_S390_RESET:
> @@ -520,6 +518,9 @@
>                 BUG();
>         }

Why leave the switch? Do you want the BUG() that badly?

Most of the time that's just wasted cycles, right?

> +       vcpu->arch.sie_block->gpsw.mask = kvm_run->psw_mask;
> +       vcpu->arch.sie_block->gpsw.addr = kvm_run->psw_addr;
> +
>         might_fault();
>         do {
> @@ -539,8 +540,6 @@
>                 /* intercept cannot be handled in-kernel, prepare kvm-run */
>                 kvm_run->exit_reason         = KVM_EXIT_S390_SIEIC;
>                 kvm_run->s390_sieic.icptcode = vcpu->arch.sie_block->icptcode;
> -               kvm_run->s390_sieic.mask     = vcpu->arch.sie_block->gpsw.mask;
> -               kvm_run->s390_sieic.addr     = vcpu->arch.sie_block->gpsw.addr;
>                 kvm_run->s390_sieic.ipa      = vcpu->arch.sie_block->ipa;
>                 kvm_run->s390_sieic.ipb      = vcpu->arch.sie_block->ipb;
>                 rc = 0;
> @@ -552,6 +551,9 @@
>                 rc = 0;
>         }
> +       kvm_run->psw_mask     = vcpu->arch.sie_block->gpsw.mask;
> +       kvm_run->psw_addr     = vcpu->arch.sie_block->gpsw.addr;
> +

I think you should not indent the equal signs here. They're not  
aligned to the ones above anymore anyways.

Apart from that, great patch!
I'll put all the pieces together today, so expect to be running Qemu  
backed virtualization on your mainframe :-).

Alex