Re: [RFC PATCH 5/6] KVM: PPC: Book3E: Add ONE_REG AltiVec support

[Scott Wood <scottwood@freescale.com>]

On 07/03/2013 07:11:52 AM, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, June 05, 2013 1:40 AM
> > To: Caraman Mihai Claudiu-B02008
> > Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org; Caraman Mihai Claudiu-B02008
> > Subject: Re: [RFC PATCH 5/6] KVM: PPC: Book3E: Add ONE_REG AltiVec
> > support
> >
> > On 06/03/2013 03:54:27 PM, Mihai Caraman wrote:
> > > Add ONE_REG support for AltiVec on Book3E.
> > >
> > > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > > ---
> > >  arch/powerpc/kvm/booke.c |   32 ++++++++++++++++++++++++++++++++
> > >  1 files changed, 32 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > > index 01eb635..019496d 100644
> > > --- a/arch/powerpc/kvm/booke.c
> > > +++ b/arch/powerpc/kvm/booke.c
> > > @@ -1570,6 +1570,22 @@ int kvm_vcpu_ioctl_get_one_reg(struct  
> kvm_vcpu
> > > *vcpu, struct kvm_one_reg *reg)
> > >  	case KVM_REG_PPC_DEBUG_INST:
> > >  		val = get_reg_val(reg->id, KVMPPC_INST_EHPRIV);
> > >  		break;
> > > +#ifdef CONFIG_ALTIVEC
> > > +	case KVM_REG_PPC_VR0 ... KVM_REG_PPC_VR31:
> > > +		if (!cpu_has_feature(CPU_FTR_ALTIVEC)) {
> > > +			r = -ENXIO;
> > > +			break;
> > > +		}
> > > +		val.vval = vcpu->arch.vr[reg->id - KVM_REG_PPC_VR0];
> > > +		break;
> > > +	case KVM_REG_PPC_VSCR:
> > > +		if (!cpu_has_feature(CPU_FTR_ALTIVEC)) {
> > > +			r = -ENXIO;
> > > +			break;
> > > +		}
> > > +		val = get_reg_val(reg->id, vcpu->arch.vscr.u[3]);
> > > +		break;
> >
> > Why u[3]?
> 
> AltiVec PEM manual says: "The VSCR has two defined bits, the AltiVec  
> non-Java
> mode (NJ) bit (VSCR[15]) and the AltiVec saturation (SAT) bit  
> (VSCR[31]);
> the remaining bits are reserved."
> 
> I think this is the reason Paul M. exposed KVM_REG_PPC_VSCR width as  
> 32-bit.

Ugh.  It's documented as a 32-bit register in the ISA, but it can only  
be accessed via a vector register (seems like an odd design choice, but  
whatever).  And the kernel chose to represent it as a 128-bit vector,  
while KVM chose to represent it as the register (not the access  
thereto) is architected.  It would have been nice to be consistent...   
At least put in a comment explaining this.

-Scott