diff mbox

powerpc: book3s: kvm: Use the saved dsisr and dar values

Message ID 1384178577-23721-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com
State New, archived
Headers show

Commit Message

Aneesh Kumar K.V Nov. 11, 2013, 2:02 p.m. UTC 
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>

Don't try to compute these values.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---

NOTE: I am not sure why we were originally computing dsisr and dar. So may be
we need a variant of this patch. But with this and the additional patch
"powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
PR guest to boot.

 arch/powerpc/kvm/book3s_emulate.c | 64 ++-------------------------------------
 1 file changed, 2 insertions(+), 62 deletions(-)

Comments

Paul Mackerras Nov. 28, 2013, 4:06 a.m. UTC | #1 
On Mon, Nov 11, 2013 at 07:32:57PM +0530, Aneesh Kumar K.V wrote:
> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> Don't try to compute these values.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Acked-by: Paul Mackerras <paulus@samba.org>
--
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 Dec. 18, 2013, 9:44 p.m. UTC | #2 
On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:

> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> 
> Don't try to compute these values.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> 
> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
> we need a variant of this patch. But with this and the additional patch
> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
> PR guest to boot.

It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.

commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
Author: Alexander Graf <agraf@suse.de>
Date:   Wed Mar 24 21:48:28 2010 +0100

    KVM: PPC: Implement alignment interrupt

    Mac OS X has some applications - namely the Finder - that require alignment
    interrupts to work properly. So we need to implement them.

    But the spec for 970 and 750 also looks different. While 750 requires the
    DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
    address (DAR), the 970 declares this as an optional feature. So we need
    to reconstruct DSISR and DAR manually.

    Signed-off-by: Alexander Graf <agraf@suse.de>
    Signed-off-by: Avi Kivity <avi@redhat.com>

Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.

So this is clearly a nack on this patch :).


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
Paul Mackerras Dec. 19, 2013, 2:26 a.m. UTC | #3 
On Wed, Dec 18, 2013 at 10:44:08PM +0100, Alexander Graf wrote:
> 
> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
> 
> > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
> > 
> > Don't try to compute these values.
> > 
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> > ---
> > 
> > NOTE: I am not sure why we were originally computing dsisr and dar. So may be
> > we need a variant of this patch. But with this and the additional patch
> > "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
> > PR guest to boot.
> 
> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
> 
> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
> Author: Alexander Graf <agraf@suse.de>
> Date:   Wed Mar 24 21:48:28 2010 +0100
> 
>     KVM: PPC: Implement alignment interrupt
> 
>     Mac OS X has some applications - namely the Finder - that require alignment
>     interrupts to work properly. So we need to implement them.
> 
>     But the spec for 970 and 750 also looks different. While 750 requires the
>     DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>     address (DAR), the 970 declares this as an optional feature. So we need
>     to reconstruct DSISR and DAR manually.
> 
>     Signed-off-by: Alexander Graf <agraf@suse.de>
>     Signed-off-by: Avi Kivity <avi@redhat.com>
> 
> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.

Although it's optional, all IBM POWER cpus, and as far as I know all
PowerPC cpus, set DAR on an alignment interrupt to the effective
address being accessed.  You have a valid point regarding DSISR, but
it would be nice to skip the computations where either the host CPU
provides the bits, or the virtual CPU doesn't.

Paul.
--
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
Aneesh Kumar K.V Dec. 19, 2013, 7:02 a.m. UTC | #4 
Alexander Graf <agraf@suse.de> writes:

> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>
>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>> 
>> Don't try to compute these values.
>> 
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> ---
>> 
>> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
>> we need a variant of this patch. But with this and the additional patch
>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
>> PR guest to boot.
>
> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
>
> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
> Author: Alexander Graf <agraf@suse.de>
> Date:   Wed Mar 24 21:48:28 2010 +0100
>
>     KVM: PPC: Implement alignment interrupt
>
>     Mac OS X has some applications - namely the Finder - that require alignment
>     interrupts to work properly. So we need to implement them.
>
>     But the spec for 970 and 750 also looks different. While 750 requires the
>     DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>     address (DAR), the 970 declares this as an optional feature. So we need
>     to reconstruct DSISR and DAR manually.
>
>     Signed-off-by: Alexander Graf <agraf@suse.de>
>     Signed-off-by: Avi Kivity <avi@redhat.com>
>
> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.
>
> So this is clearly a nack on this patch :).

I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need
to do that ? According to Paul we should always find DAR.

-aneesh

--
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 Dec. 19, 2013, 12:35 p.m. UTC | #5 
> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
> 
> Alexander Graf <agraf@suse.de> writes:
> 
>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>> 
>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>> 
>>> Don't try to compute these values.
>>> 
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>> ---
>>> 
>>> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
>>> we need a variant of this patch. But with this and the additional patch
>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
>>> PR guest to boot.
>> 
>> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
>> 
>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>> Author: Alexander Graf <agraf@suse.de>
>> Date:   Wed Mar 24 21:48:28 2010 +0100
>> 
>>    KVM: PPC: Implement alignment interrupt
>> 
>>    Mac OS X has some applications - namely the Finder - that require alignment
>>    interrupts to work properly. So we need to implement them.
>> 
>>    But the spec for 970 and 750 also looks different. While 750 requires the
>>    DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>>    address (DAR), the 970 declares this as an optional feature. So we need
>>    to reconstruct DSISR and DAR manually.
>> 
>>    Signed-off-by: Alexander Graf <agraf@suse.de>
>>    Signed-off-by: Avi Kivity <avi@redhat.com>
>> 
>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.
>> 
>> So this is clearly a nack on this patch :).
> 
> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need
> to do that ? According to Paul we should always find DAR.

Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a proper DAR value - we can then remove that part.

But for DSISR I'm not convinced CPUs above 970 handle this correctly. So we would at least need a guest cpu check to find out whether the vcpu expects a working dsisr and emulate it then.

I don't really fully understand the problem though. Why does the calculation break at all for you?


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
Aneesh Kumar K.V Dec. 20, 2013, 4:37 a.m. UTC | #6 
Alexander Graf <agraf@suse.de> writes:

>> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
>> 
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>>> 
>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>> 
>>>> Don't try to compute these values.
>>>> 
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>> ---
>>>> 
>>>> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
>>>> we need a variant of this patch. But with this and the additional patch
>>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
>>>> PR guest to boot.
>>> 
>>> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
>>> 
>>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>>> Author: Alexander Graf <agraf@suse.de>
>>> Date:   Wed Mar 24 21:48:28 2010 +0100
>>> 
>>>    KVM: PPC: Implement alignment interrupt
>>> 
>>>    Mac OS X has some applications - namely the Finder - that require alignment
>>>    interrupts to work properly. So we need to implement them.
>>> 
>>>    But the spec for 970 and 750 also looks different. While 750 requires the
>>>    DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>>>    address (DAR), the 970 declares this as an optional feature. So we need
>>>    to reconstruct DSISR and DAR manually.
>>> 
>>>    Signed-off-by: Alexander Graf <agraf@suse.de>
>>>    Signed-off-by: Avi Kivity <avi@redhat.com>
>>> 
>>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.
>>> 
>>> So this is clearly a nack on this patch :).
>> 
>> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need
>> to do that ? According to Paul we should always find DAR.
>
> Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a proper DAR value - we can then remove that part.
>
> But for DSISR I'm not convinced CPUs above 970 handle this
> correctly. So we would at least need a guest cpu check to find out
> whether the vcpu expects a working dsisr and emulate it then.

>
> I don't really fully understand the problem though. Why does the
> calculation break at all for you?


IIRC this was to get little endian PR setup to work. This is to avoid
handling new instructions, because in little endian mode we get
alignment interrupt for a larger instructon set

-aneesh

--
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 Dec. 20, 2013, 6:38 a.m. UTC | #7 
> Am 20.12.2013 um 05:37 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
> 
> Alexander Graf <agraf@suse.de> writes:
> 
>>> Am 19.12.2013 um 08:02 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>:
>>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>>> On 11.11.2013, at 15:02, Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>>>> 
>>>>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>>>>> 
>>>>> Don't try to compute these values.
>>>>> 
>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>>>>> ---
>>>>> 
>>>>> NOTE: I am not sure why we were originally computing dsisr and dar. So may be
>>>>> we need a variant of this patch. But with this and the additional patch
>>>>> "powerpc: book3s: PR: Enable Little Endian PR guest" I am able to get a Little Endian
>>>>> PR guest to boot.
>>>> 
>>>> It's quite easy to find out - git blame tells you all the history and points you to commit ca7f4203b.
>>>> 
>>>> commit ca7f4203b9b66e12d0d9968ff7dfe781f3a9695a
>>>> Author: Alexander Graf <agraf@suse.de>
>>>> Date:   Wed Mar 24 21:48:28 2010 +0100
>>>> 
>>>>   KVM: PPC: Implement alignment interrupt
>>>> 
>>>>   Mac OS X has some applications - namely the Finder - that require alignment
>>>>   interrupts to work properly. So we need to implement them.
>>>> 
>>>>   But the spec for 970 and 750 also looks different. While 750 requires the
>>>>   DSISR and DAR fields to reflect some instruction bits (DSISR) and the fault
>>>>   address (DAR), the 970 declares this as an optional feature. So we need
>>>>   to reconstruct DSISR and DAR manually.
>>>> 
>>>>   Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>   Signed-off-by: Avi Kivity <avi@redhat.com>
>>>> 
>>>> Read this as "on 970, alignment interrupts don't give us DSISR and DAR of the faulting instruction" as otherwise I wouldn't have implemented it.
>>>> 
>>>> So this is clearly a nack on this patch :).
>>> 
>>> I can possibly do a if (cpu_has_feature(CPU_FTR_ARCH_201)). But do we need
>>> to do that ? According to Paul we should always find DAR.
>> 
>> Paul only mentioned DAR, not DSISR. Please verify whether 970 gives us a proper DAR value - we can then remove that part.
>> 
>> But for DSISR I'm not convinced CPUs above 970 handle this
>> correctly. So we would at least need a guest cpu check to find out
>> whether the vcpu expects a working dsisr and emulate it then.
> 
>> 
>> I don't really fully understand the problem though. Why does the
>> calculation break at all for you?
> 
> 
> IIRC this was to get little endian PR setup to work. This is to avoid
> handling new instructions, because in little endian mode we get
> alignment interrupt for a larger instructon set

Ok, please limit dar/dsisr calculation to book3s_32 vcpus then :).

Alex

> 
> -aneesh
> 
--
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
diff mbox

Patch

diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index 99d40f8..62768f9 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -569,70 +569,10 @@  unprivileged:
 
 u32 kvmppc_alignment_dsisr(struct kvm_vcpu *vcpu, unsigned int inst)
 {
-	u32 dsisr = 0;
-
-	/*
-	 * This is what the spec says about DSISR bits (not mentioned = 0):
-	 *
-	 * 12:13		[DS]	Set to bits 30:31
-	 * 15:16		[X]	Set to bits 29:30
-	 * 17			[X]	Set to bit 25
-	 *			[D/DS]	Set to bit 5
-	 * 18:21		[X]	Set to bits 21:24
-	 *			[D/DS]	Set to bits 1:4
-	 * 22:26			Set to bits 6:10 (RT/RS/FRT/FRS)
-	 * 27:31			Set to bits 11:15 (RA)
-	 */
-
-	switch (get_op(inst)) {
-	/* D-form */
-	case OP_LFS:
-	case OP_LFD:
-	case OP_STFD:
-	case OP_STFS:
-		dsisr |= (inst >> 12) & 0x4000;	/* bit 17 */
-		dsisr |= (inst >> 17) & 0x3c00; /* bits 18:21 */
-		break;
-	/* X-form */
-	case 31:
-		dsisr |= (inst << 14) & 0x18000; /* bits 15:16 */
-		dsisr |= (inst << 8)  & 0x04000; /* bit 17 */
-		dsisr |= (inst << 3)  & 0x03c00; /* bits 18:21 */
-		break;
-	default:
-		printk(KERN_INFO "KVM: Unaligned instruction 0x%x\n", inst);
-		break;
-	}
-
-	dsisr |= (inst >> 16) & 0x03ff; /* bits 22:31 */
-
-	return dsisr;
+	return vcpu->arch.fault_dsisr;
 }
 
 ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst)
 {
-	ulong dar = 0;
-	ulong ra = get_ra(inst);
-	ulong rb = get_rb(inst);
-
-	switch (get_op(inst)) {
-	case OP_LFS:
-	case OP_LFD:
-	case OP_STFD:
-	case OP_STFS:
-		if (ra)
-			dar = kvmppc_get_gpr(vcpu, ra);
-		dar += (s32)((s16)inst);
-		break;
-	case 31:
-		if (ra)
-			dar = kvmppc_get_gpr(vcpu, ra);
-		dar += kvmppc_get_gpr(vcpu, rb);
-		break;
-	default:
-		printk(KERN_INFO "KVM: Unaligned instruction 0x%x\n", inst);
-		break;
-	}
-
-	return dar;
+	return vcpu->arch.fault_dar;
 }