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

[Carsten Otte]

Avi Kivity wrote:
> > @@ -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?
Yes, but with a zero user base I think it's okay. I'd update our 
userspace. Once we pull CONFIG_EXPERIMENTAL we keep the API stable.

> 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.
Yea, that's better.

> > --- 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.
This is a plain bug, will fix.


> I'd also appreciate an explanation of what this is all about.
The processor status word does contain various bits about the CPU's 
state, such as interrupt mask bits, current address space, and the 
current instruction address. The status is kept in the in-kernel sie 
control block data structure and has so far only been mirrored into 
kvm_run during exit_reason == s390_sieic exits because user space needs 
to work on it. It was never part of get_regs/set_regs and friends as 
performance optimization: it's needed on almost every exit, having it in 
kvm_run saves doing syscalls.
The gdb stub requires an up-to-date copy at every exit, and therefore 
the patch moves it out of the union and updates it at all userland exits.