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

[Alexander Graf]

On 22.10.2009, at 11:08, Avi Kivity wrote:

> On 10/20/2009 06:40 PM, Carsten Otte wrote:
> > This patch moves s390 processor status word into the base kvm_run
> > struct and keeps it up-to date on all userspace exits.
> >
> > +#include <linux/autoconf.h>
> > #include <linux/types.h>
> > #include <linux/compiler.h>
> > #include <linux/ioctl.h>
>
> Not needed.
>
> > @@ -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
>
> Doesn't this break backward compatibility by changing the structure?
>
> Best to put it after the union (and as a copy, so userspace that  
> expects the previous location still works).  If you're reading it  
> from the kernel, also need a way to tell the kernel which copy to  
> read from.
>
> Also advertise with a KVM_CAP.

I don't think we need to go through the hassle here. There is  
effectively no user of that code for now and the ABI is considered  
unstable.

> Additionally, CONFIG_ in public headers are frowned upon as non- 
> portable.  A workaround is to #define __KVM_S390 in <asm/kvm.h> and  
> depend on that.
>
> > --- 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;
>
> It's traditional to add braces around multi-line else blocks.
>
> I'd also appreciate an explanation of what this is all about.

Explanation in the code or explanation in an email reply?

Alex