Patchwork [RFC,03/17] KVM: PPC64: booke: Add EPCR support in sregs

login
register
mail settings
Submitter Mihai Caraman
Date June 25, 2012, 12:26 p.m.
Message ID <1340627195-11544-4-git-send-email-mihai.caraman@freescale.com>
Download mbox | patch
Permalink /patch/167069/
State New
Headers show

Comments

Mihai Caraman - June 25, 2012, 12:26 p.m.
Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
for 64-bit hosts.

Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
---
 arch/powerpc/kvm/booke.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Avi Kivity - June 25, 2012, 12:59 p.m.
On 06/25/2012 03:26 PM, Mihai Caraman wrote:
> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
> for 64-bit hosts.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index f9fa260..d15c4b5 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1052,6 +1052,9 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
>  	u64 tb = get_tb();
>  
>  	sregs->u.e.features |= KVM_SREGS_E_BASE;
> +#ifdef CONFIG_64BIT
> +	sregs->u.e.features |= KVM_SREGS_E_64;
> +#endif
>  

This is an ABI, but I see no trace of it in Documentation.
Caraman Mihai Claudiu-B02008 - June 25, 2012, 1:24 p.m.
> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Monday, June 25, 2012 4:00 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
> Subject: Re: [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR support in
> sregs
> 
> On 06/25/2012 03:26 PM, Mihai Caraman wrote:
> > Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs for
> > 64-bit hosts.
> >
> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> > ---
> >  arch/powerpc/kvm/booke.c |   14 ++++++++++++++
> >  1 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> > f9fa260..d15c4b5 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -1052,6 +1052,9 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
> >  	u64 tb = get_tb();
> >
> >  	sregs->u.e.features |= KVM_SREGS_E_BASE;
> > +#ifdef CONFIG_64BIT
> > +	sregs->u.e.features |= KVM_SREGS_E_64; #endif
> >
> 
> This is an ABI, but I see no trace of it in Documentation.

The ppc sregs documentation in api.txt redirects to arch/powerpc/include/asm/kvm.h.
Isn't this enough?

Mike

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity - June 25, 2012, 1:36 p.m.
On 06/25/2012 04:24 PM, Caraman Mihai Claudiu-B02008 wrote:
>> -----Original Message-----
>> From: Avi Kivity [mailto:avi@redhat.com]
>> Sent: Monday, June 25, 2012 4:00 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
>> Subject: Re: [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR support in
>> sregs
>> 
>> On 06/25/2012 03:26 PM, Mihai Caraman wrote:
>> > Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs for
>> > 64-bit hosts.
>> >
>> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>> > ---
>> >  arch/powerpc/kvm/booke.c |   14 ++++++++++++++
>> >  1 files changed, 14 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
>> > f9fa260..d15c4b5 100644
>> > --- a/arch/powerpc/kvm/booke.c
>> > +++ b/arch/powerpc/kvm/booke.c
>> > @@ -1052,6 +1052,9 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
>> >  	u64 tb = get_tb();
>> >
>> >  	sregs->u.e.features |= KVM_SREGS_E_BASE;
>> > +#ifdef CONFIG_64BIT
>> > +	sregs->u.e.features |= KVM_SREGS_E_64; #endif
>> >
>> 
>> This is an ABI, but I see no trace of it in Documentation.
> 
> The ppc sregs documentation in api.txt redirects to arch/powerpc/include/asm/kvm.h.
> Isn't this enough?

I guess it's okay.
Scott Wood - June 26, 2012, 10:34 p.m.
On 06/25/2012 07:26 AM, Mihai Caraman wrote:
> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
> for 64-bit hosts.
> 
> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> ---
>  arch/powerpc/kvm/booke.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index f9fa260..d15c4b5 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -1052,6 +1052,9 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
>  	u64 tb = get_tb();
>  
>  	sregs->u.e.features |= KVM_SREGS_E_BASE;
> +#ifdef CONFIG_64BIT
> +	sregs->u.e.features |= KVM_SREGS_E_64;0
> +#endif
>  
>  	sregs->u.e.csrr0 = vcpu->arch.csrr0;
>  	sregs->u.e.csrr1 = vcpu->arch.csrr1;
> @@ -1063,6 +1066,9 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
>  	sregs->u.e.dec = kvmppc_get_dec(vcpu, tb);
>  	sregs->u.e.tb = tb;
>  	sregs->u.e.vrsave = vcpu->arch.vrsave;
> +#ifdef CONFIG_64BIT
> +	sregs->u.e.epcr = vcpu->arch.epcr;
> +#endif
>  }
>  
>  static int set_sregs_base(struct kvm_vcpu *vcpu,
> @@ -1071,6 +1077,11 @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
>  	if (!(sregs->u.e.features & KVM_SREGS_E_BASE))
>  		return 0;
>  
> +#ifdef CONFIG_64BIT
> +	if (!(sregs->u.e.features & KVM_SREGS_E_64))
> +		return 0;
> +#endif

This means that a QEMU targeting a 32-bit guest won't be able to set any
special registers, if it sets feature bits manually rather than getting
them from GET_SREGS.

This check should only qualify whether we look at sregs.u.e.epcr, not
whether this function works at all.

BTW, shouldn't the BASE check return an error rather than silently no-op?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - June 27, 2012, 3:23 p.m.
On 06/27/2012 06:41 AM, Caraman Mihai Claudiu-B02008 wrote:
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Wednesday, June 27, 2012 1:35 AM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
>> Subject: Re: [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR support in
>> sregs
>>
>> On 06/25/2012 07:26 AM, Mihai Caraman wrote:
>>> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs for
>>> 64-bit hosts.
>>>
>>> Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
>>> ---
>>>  arch/powerpc/kvm/booke.c |   14 ++++++++++++++
>>>  1 files changed, 14 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
>>> f9fa260..d15c4b5 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -1052,6 +1052,9 @@ static void get_sregs_base(struct kvm_vcpu *vcpu,
>>>  	u64 tb = get_tb();
>>>
>>>  	sregs->u.e.features |= KVM_SREGS_E_BASE;
>>> +#ifdef CONFIG_64BIT
>>> +	sregs->u.e.features |= KVM_SREGS_E_64;0 #endif
>>>
>>>  	sregs->u.e.csrr0 = vcpu->arch.csrr0;
>>>  	sregs->u.e.csrr1 = vcpu->arch.csrr1; @@ -1063,6 +1066,9 @@ static
>>> void get_sregs_base(struct kvm_vcpu *vcpu,
>>>  	sregs->u.e.dec = kvmppc_get_dec(vcpu, tb);
>>>  	sregs->u.e.tb = tb;
>>>  	sregs->u.e.vrsave = vcpu->arch.vrsave;
>>> +#ifdef CONFIG_64BIT
>>> +	sregs->u.e.epcr = vcpu->arch.epcr;
>>> +#endif
>>>  }
>>>
>>>  static int set_sregs_base(struct kvm_vcpu *vcpu, @@ -1071,6 +1077,11
>>> @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
>>>  	if (!(sregs->u.e.features & KVM_SREGS_E_BASE))
>>>  		return 0;
>>>
>>> +#ifdef CONFIG_64BIT
>>> +	if (!(sregs->u.e.features & KVM_SREGS_E_64))
>>> +		return 0;
>>> +#endif
>>
>> This means that a QEMU targeting a 32-bit guest won't be able to set any
>> special registers, if it sets feature bits manually rather than getting
>> them from GET_SREGS.
> 
> I had some concerns about his. I only check qemu ppc code which uses get/set
> approach and I followed the BASE model. Now I see that qemu x86 set them manually :(
> Why do we care if the caller set or not BASE?

BASE contains things which should be present on all booke chips.  If
that's not set something's wrong.

None of the other feature bits are handled that way.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - July 4, 2012, 1:33 p.m.
On 25.06.2012, at 14:26, Mihai Caraman wrote:

> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
> for 64-bit hosts.

Please also implement a ONE_REG interface while at it. Over time, I'd like to move towards ONE_REG instead of the messy regs/sregs API.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Caraman Mihai Claudiu-B02008 - July 5, 2012, 11:49 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, July 04, 2012 4:34 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-ppc] [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR
> support in sregs
> 
> 
> On 25.06.2012, at 14:26, Mihai Caraman wrote:
> 
> > Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
> > for 64-bit hosts.
> 
> Please also implement a ONE_REG interface while at it. Over time, I'd
> like to move towards ONE_REG instead of the messy regs/sregs API.

ONE_REG doesn't seem to be implemented at all for book3e, I looked at
kvm_vcpu_ioctl_set_one_reg/kvm_vcpu_ioctl_get_one_reg in booke.c file.

I can take care of it soon but in a different patch set. It's ok like this?

-Mike

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - July 5, 2012, 12:12 p.m.
On 07/05/2012 01:49 PM, Caraman Mihai Claudiu-B02008 wrote:
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, July 04, 2012 4:34 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-ppc] [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR
>> support in sregs
>>
>>
>> On 25.06.2012, at 14:26, Mihai Caraman wrote:
>>
>>> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
>>> for 64-bit hosts.
>> Please also implement a ONE_REG interface while at it. Over time, I'd
>> like to move towards ONE_REG instead of the messy regs/sregs API.
> ONE_REG doesn't seem to be implemented at all for book3e, I looked at
> kvm_vcpu_ioctl_set_one_reg/kvm_vcpu_ioctl_get_one_reg in booke.c file.
>
> I can take care of it soon but in a different patch set. It's ok like this?

Do it in a different patch, but as part of this patch set.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Caraman Mihai Claudiu-B02008 - July 5, 2012, 12:54 p.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, July 05, 2012 3:13 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-ppc] [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR
> support in sregs
> 
> On 07/05/2012 01:49 PM, Caraman Mihai Claudiu-B02008 wrote:
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Wednesday, July 04, 2012 4:34 PM
> >> To: Caraman Mihai Claudiu-B02008
> >> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
> >> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
> >> Subject: Re: [Qemu-ppc] [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR
> >> support in sregs
> >>
> >>
> >> On 25.06.2012, at 14:26, Mihai Caraman wrote:
> >>
> >>> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
> >>> for 64-bit hosts.
> >> Please also implement a ONE_REG interface while at it. Over time, I'd
> >> like to move towards ONE_REG instead of the messy regs/sregs API.
> > ONE_REG doesn't seem to be implemented at all for book3e, I looked at
> > kvm_vcpu_ioctl_set_one_reg/kvm_vcpu_ioctl_get_one_reg in booke.c file.
> >
> > I can take care of it soon but in a different patch set. It's ok like
> this?
> 
> Do it in a different patch, but as part of this patch set.

Hmm ... then if you don't disagree I will do it as a prerequisite patch since I want
to keep this patchset strictly for 64-bit support.
I am not familiar with ONE_REG, is qemu tailored to use it? I need a way to test it.

-Mike

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - July 11, 2012, 6:07 p.m.
On 05.07.2012, at 14:54, Caraman Mihai Claudiu-B02008 wrote:

>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, July 05, 2012 3:13 PM
>> To: Caraman Mihai Claudiu-B02008
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-ppc] [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR
>> support in sregs
>> 
>> On 07/05/2012 01:49 PM, Caraman Mihai Claudiu-B02008 wrote:
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Wednesday, July 04, 2012 4:34 PM
>>>> To: Caraman Mihai Claudiu-B02008
>>>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-
>>>> dev@lists.ozlabs.org; qemu-ppc@nongnu.org
>>>> Subject: Re: [Qemu-ppc] [RFC PATCH 03/17] KVM: PPC64: booke: Add EPCR
>>>> support in sregs
>>>> 
>>>> 
>>>> On 25.06.2012, at 14:26, Mihai Caraman wrote:
>>>> 
>>>>> Add KVM_SREGS_E_64 feature and EPCR spr support in get/set sregs
>>>>> for 64-bit hosts.
>>>> Please also implement a ONE_REG interface while at it. Over time, I'd
>>>> like to move towards ONE_REG instead of the messy regs/sregs API.
>>> ONE_REG doesn't seem to be implemented at all for book3e, I looked at
>>> kvm_vcpu_ioctl_set_one_reg/kvm_vcpu_ioctl_get_one_reg in booke.c file.
>>> 
>>> I can take care of it soon but in a different patch set. It's ok like
>> this?
>> 
>> Do it in a different patch, but as part of this patch set.
> 
> Hmm ... then if you don't disagree I will do it as a prerequisite patch since I want
> to keep this patchset strictly for 64-bit support.

I don't understand? You just make the same functionality available through 2 interfaces to user space.

> I am not familiar with ONE_REG, is qemu tailored to use it? I need a way to test it.

ONE_REG is just a more extensible ioctl. Just read up on it in the documentation and you'll see what I mean :)


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index f9fa260..d15c4b5 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -1052,6 +1052,9 @@  static void get_sregs_base(struct kvm_vcpu *vcpu,
 	u64 tb = get_tb();
 
 	sregs->u.e.features |= KVM_SREGS_E_BASE;
+#ifdef CONFIG_64BIT
+	sregs->u.e.features |= KVM_SREGS_E_64;
+#endif
 
 	sregs->u.e.csrr0 = vcpu->arch.csrr0;
 	sregs->u.e.csrr1 = vcpu->arch.csrr1;
@@ -1063,6 +1066,9 @@  static void get_sregs_base(struct kvm_vcpu *vcpu,
 	sregs->u.e.dec = kvmppc_get_dec(vcpu, tb);
 	sregs->u.e.tb = tb;
 	sregs->u.e.vrsave = vcpu->arch.vrsave;
+#ifdef CONFIG_64BIT
+	sregs->u.e.epcr = vcpu->arch.epcr;
+#endif
 }
 
 static int set_sregs_base(struct kvm_vcpu *vcpu,
@@ -1071,6 +1077,11 @@  static int set_sregs_base(struct kvm_vcpu *vcpu,
 	if (!(sregs->u.e.features & KVM_SREGS_E_BASE))
 		return 0;
 
+#ifdef CONFIG_64BIT
+	if (!(sregs->u.e.features & KVM_SREGS_E_64))
+		return 0;
+#endif
+
 	vcpu->arch.csrr0 = sregs->u.e.csrr0;
 	vcpu->arch.csrr1 = sregs->u.e.csrr1;
 	vcpu->arch.mcsr = sregs->u.e.mcsr;
@@ -1078,6 +1089,9 @@  static int set_sregs_base(struct kvm_vcpu *vcpu,
 	set_guest_dear(vcpu, sregs->u.e.dear);
 	vcpu->arch.vrsave = sregs->u.e.vrsave;
 	kvmppc_set_tcr(vcpu, sregs->u.e.tcr);
+#ifdef CONFIG_64BIT
+	kvmppc_set_epcr(vcpu, sregs->u.e.epcr);
+#endif
 
 	if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_DEC) {
 		vcpu->arch.dec = sregs->u.e.dec;