| Message ID | 1399224075-18041-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: > Although it's optional IBM POWER cpus always had DAR value set on > alignment interrupt. So don't try to compute these values. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > --- > Changes from V3: > * Use make_dsisr instead of checking feature flag to decide whether to use > saved dsisr or not > > arch/powerpc/include/asm/disassemble.h | 34 +++++++++++++++++++++++++++ > arch/powerpc/kernel/align.c | 34 +-------------------------- > arch/powerpc/kvm/book3s_emulate.c | 43 ++++------------------------------ > 3 files changed, 40 insertions(+), 71 deletions(-) > > diff --git a/arch/powerpc/include/asm/disassemble.h b/arch/powerpc/include/asm/disassemble.h > index 856f8deb557a..6330a61b875a 100644 > --- a/arch/powerpc/include/asm/disassemble.h > +++ b/arch/powerpc/include/asm/disassemble.h > @@ -81,4 +81,38 @@ static inline unsigned int get_oc(u32 inst) > { > return (inst >> 11) & 0x7fff; > } > + > +#define IS_XFORM(inst) (get_op(inst) == 31) > +#define IS_DSFORM(inst) (get_op(inst) >= 56) > + > +/* > + * Create a DSISR value from the instruction > + */ > +static inline unsigned make_dsisr(unsigned instr) > +{ > + unsigned dsisr; > + > + > + /* bits 6:15 --> 22:31 */ > + dsisr = (instr & 0x03ff0000) >> 16; > + > + if (IS_XFORM(instr)) { > + /* bits 29:30 --> 15:16 */ > + dsisr |= (instr & 0x00000006) << 14; > + /* bit 25 --> 17 */ > + dsisr |= (instr & 0x00000040) << 8; > + /* bits 21:24 --> 18:21 */ > + dsisr |= (instr & 0x00000780) << 3; > + } else { > + /* bit 5 --> 17 */ > + dsisr |= (instr & 0x04000000) >> 12; > + /* bits 1: 4 --> 18:21 */ > + dsisr |= (instr & 0x78000000) >> 17; > + /* bits 30:31 --> 12:13 */ > + if (IS_DSFORM(instr)) > + dsisr |= (instr & 0x00000003) << 18; > + } > + > + return dsisr; > +} > #endif /* __ASM_PPC_DISASSEMBLE_H__ */ > diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c > index 94908af308d8..34f55524d456 100644 > --- a/arch/powerpc/kernel/align.c > +++ b/arch/powerpc/kernel/align.c > @@ -25,14 +25,13 @@ > #include <asm/cputable.h> > #include <asm/emulated_ops.h> > #include <asm/switch_to.h> > +#include <asm/disassemble.h> > > struct aligninfo { > unsigned char len; > unsigned char flags; > }; > > -#define IS_XFORM(inst) (((inst) >> 26) == 31) > -#define IS_DSFORM(inst) (((inst) >> 26) >= 56) > > #define INVALID { 0, 0 } > > @@ -192,37 +191,6 @@ static struct aligninfo aligninfo[128] = { > }; > > /* > - * Create a DSISR value from the instruction > - */ > -static inline unsigned make_dsisr(unsigned instr) > -{ > - unsigned dsisr; > - > - > - /* bits 6:15 --> 22:31 */ > - dsisr = (instr & 0x03ff0000) >> 16; > - > - if (IS_XFORM(instr)) { > - /* bits 29:30 --> 15:16 */ > - dsisr |= (instr & 0x00000006) << 14; > - /* bit 25 --> 17 */ > - dsisr |= (instr & 0x00000040) << 8; > - /* bits 21:24 --> 18:21 */ > - dsisr |= (instr & 0x00000780) << 3; > - } else { > - /* bit 5 --> 17 */ > - dsisr |= (instr & 0x04000000) >> 12; > - /* bits 1: 4 --> 18:21 */ > - dsisr |= (instr & 0x78000000) >> 17; > - /* bits 30:31 --> 12:13 */ > - if (IS_DSFORM(instr)) > - dsisr |= (instr & 0x00000003) << 18; > - } > - > - return dsisr; > -} > - > -/* > * The dcbz (data cache block zero) instruction > * gives an alignment fault if used on non-cacheable > * memory. We handle the fault mainly for the > diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c > index 99d40f8977e8..04c38f049dfd 100644 > --- a/arch/powerpc/kvm/book3s_emulate.c > +++ b/arch/powerpc/kvm/book3s_emulate.c > @@ -569,48 +569,14 @@ 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 make_dsisr(inst); > } > > ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) > { > +#ifdef CONFIG_PPC_BOOK3S_64 > + return vcpu->arch.fault_dar; How about PA6T and G5s? Alex
Alexander Graf <agraf@suse.de> writes: > On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >> Although it's optional IBM POWER cpus always had DAR value set on >> alignment interrupt. So don't try to compute these values. >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >> --- >> Changes from V3: >> * Use make_dsisr instead of checking feature flag to decide whether to use >> saved dsisr or not >> .... >> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >> { >> +#ifdef CONFIG_PPC_BOOK3S_64 >> + return vcpu->arch.fault_dar; > > How about PA6T and G5s? > > Paul mentioned that BOOK3S always had DAR value set on alignment interrupt. And the patch is to enable/collect correct DAR value when running with Little Endian PR guest. Now to limit the impact and to enable Little Endian PR guest, I ended up doing the conditional code only for book3s 64 for which we know for sure that we set DAR value. -aneesh
On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: > Alexander Graf <agraf@suse.de> writes: > >> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>> Although it's optional IBM POWER cpus always had DAR value set on >>> alignment interrupt. So don't try to compute these values. >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>> --- >>> Changes from V3: >>> * Use make_dsisr instead of checking feature flag to decide whether to use >>> saved dsisr or not >>> > .... > >>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>> { >>> +#ifdef CONFIG_PPC_BOOK3S_64 >>> + return vcpu->arch.fault_dar; >> How about PA6T and G5s? >> >> > Paul mentioned that BOOK3S always had DAR value set on alignment > interrupt. And the patch is to enable/collect correct DAR value when > running with Little Endian PR guest. Now to limit the impact and to > enable Little Endian PR guest, I ended up doing the conditional code > only for book3s 64 for which we know for sure that we set DAR value. Yes, and I'm asking whether we know that this statement holds true for PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at least developed by IBM, I'd assume its semantics here are similar to POWER4, but for PA6T I wouldn't be so sure. Alex
Alexander Graf <agraf@suse.de> writes: > On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >> Alexander Graf <agraf@suse.de> writes: >> >>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>> Although it's optional IBM POWER cpus always had DAR value set on >>>> alignment interrupt. So don't try to compute these values. >>>> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>> --- >>>> Changes from V3: >>>> * Use make_dsisr instead of checking feature flag to decide whether to use >>>> saved dsisr or not >>>> >> .... >> >>>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>> { >>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>> + return vcpu->arch.fault_dar; >>> How about PA6T and G5s? >>> >>> >> Paul mentioned that BOOK3S always had DAR value set on alignment >> interrupt. And the patch is to enable/collect correct DAR value when >> running with Little Endian PR guest. Now to limit the impact and to >> enable Little Endian PR guest, I ended up doing the conditional code >> only for book3s 64 for which we know for sure that we set DAR value. > > Yes, and I'm asking whether we know that this statement holds true for > PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is > at least developed by IBM, I'd assume its semantics here are similar to > POWER4, but for PA6T I wouldn't be so sure. I will have to defer to Paul on that question. But that should not prevent this patch from going upstream right ? -aneesh
2014-05-05 7:43 GMT-07:00 Alexander Graf <agraf@suse.de>: > On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: > >> Alexander Graf <agraf@suse.de> writes: >> >> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>> >>>> Although it's optional IBM POWER cpus always had DAR value set on >>>> alignment interrupt. So don't try to compute these values. >>>> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>> --- >>>> Changes from V3: >>>> * Use make_dsisr instead of checking feature flag to decide whether to >>>> use >>>> saved dsisr or not >>>> >>>> .... >> >> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>> { >>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>> + return vcpu->arch.fault_dar; >>>> >>> How about PA6T and G5s? >>> >>> >>> Paul mentioned that BOOK3S always had DAR value set on alignment >> interrupt. And the patch is to enable/collect correct DAR value when >> running with Little Endian PR guest. Now to limit the impact and to >> enable Little Endian PR guest, I ended up doing the conditional code >> only for book3s 64 for which we know for sure that we set DAR value. >> > > Yes, and I'm asking whether we know that this statement holds true for > PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at > least developed by IBM, I'd assume its semantics here are similar to > POWER4, but for PA6T I wouldn't be so sure. > > Thanks for looking out for us, obviously IBM doesn't (based on the reply a minute ago). In the end, since there's been no work to enable KVM on PA6T, I'm not too worried. I guess it's one more thing to sort out (and check for) whenever someone does that. I definitely don't have cycles to deal with that myself at this time. I can help find hardware for someone who wants to, but even then I'm guessing the interest is pretty limited. -Olof
[Now without HTML email -- it's what you get for cc:ing me at work instead of my upstream email :)] 2014-05-05 7:43 GMT-07:00 Alexander Graf <agraf@suse.de>: > > On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >> >> Alexander Graf <agraf@suse.de> writes: >> >>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>> >>>> Although it's optional IBM POWER cpus always had DAR value set on >>>> alignment interrupt. So don't try to compute these values. >>>> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>> --- >>>> Changes from V3: >>>> * Use make_dsisr instead of checking feature flag to decide whether to use >>>> saved dsisr or not >>>> >> .... >> >>>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>> { >>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>> + return vcpu->arch.fault_dar; >>> >>> How about PA6T and G5s? >>> >>> >> Paul mentioned that BOOK3S always had DAR value set on alignment >> interrupt. And the patch is to enable/collect correct DAR value when >> running with Little Endian PR guest. Now to limit the impact and to >> enable Little Endian PR guest, I ended up doing the conditional code >> only for book3s 64 for which we know for sure that we set DAR value. > > > Yes, and I'm asking whether we know that this statement holds true for PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at least developed by IBM, I'd assume its semantics here are similar to POWER4, but for PA6T I wouldn't be so sure. > Thanks for looking out for us, obviously IBM doesn't (based on the reply a minute ago). In the end, since there's been no work to enable KVM on PA6T, I'm not too worried. I guess it's one more thing to sort out (and check for) whenever someone does that. I definitely don't have cycles to deal with that myself at this time. I can help find hardware for someone who wants to, but even then I'm guessing the interest is pretty limited. -Olof
Olof Johansson <olofj@google.com> writes: > 2014-05-05 7:43 GMT-07:00 Alexander Graf <agraf@suse.de>: > >> On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >> >>> Alexander Graf <agraf@suse.de> writes: >>> >>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>> >>>>> Although it's optional IBM POWER cpus always had DAR value set on >>>>> alignment interrupt. So don't try to compute these values. >>>>> >>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>>> --- >>>>> Changes from V3: >>>>> * Use make_dsisr instead of checking feature flag to decide whether to >>>>> use >>>>> saved dsisr or not >>>>> >>>>> .... >>> >>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>>> { >>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>> + return vcpu->arch.fault_dar; >>>>> >>>> How about PA6T and G5s? >>>> >>>> >>>> Paul mentioned that BOOK3S always had DAR value set on alignment >>> interrupt. And the patch is to enable/collect correct DAR value when >>> running with Little Endian PR guest. Now to limit the impact and to >>> enable Little Endian PR guest, I ended up doing the conditional code >>> only for book3s 64 for which we know for sure that we set DAR value. >>> >> >> Yes, and I'm asking whether we know that this statement holds true for >> PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at >> least developed by IBM, I'd assume its semantics here are similar to >> POWER4, but for PA6T I wouldn't be so sure. >> >> > Thanks for looking out for us, obviously IBM doesn't (based on the reply a > minute ago). The reason I deferred the question to Paul is really because I don't know enough about PA6T and G5 to comment. I intentionally restricted the changes to BOOK3S_64 because I wanted to make sure I don't break anything else. It is in no way to hint that others don't care. -aneesh
2014-05-05 8:03 GMT-07:00 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>: > Olof Johansson <olofj@google.com> writes: > >> 2014-05-05 7:43 GMT-07:00 Alexander Graf <agraf@suse.de>: >> >>> On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >>> >>>> Alexander Graf <agraf@suse.de> writes: >>>> >>>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>>> >>>>>> Although it's optional IBM POWER cpus always had DAR value set on >>>>>> alignment interrupt. So don't try to compute these values. >>>>>> >>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>>>> --- >>>>>> Changes from V3: >>>>>> * Use make_dsisr instead of checking feature flag to decide whether to >>>>>> use >>>>>> saved dsisr or not >>>>>> >>>>>> .... >>>> >>>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>>>> { >>>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>>> + return vcpu->arch.fault_dar; >>>>>> >>>>> How about PA6T and G5s? >>>>> >>>>> >>>>> Paul mentioned that BOOK3S always had DAR value set on alignment >>>> interrupt. And the patch is to enable/collect correct DAR value when >>>> running with Little Endian PR guest. Now to limit the impact and to >>>> enable Little Endian PR guest, I ended up doing the conditional code >>>> only for book3s 64 for which we know for sure that we set DAR value. >>>> >>> >>> Yes, and I'm asking whether we know that this statement holds true for >>> PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at >>> least developed by IBM, I'd assume its semantics here are similar to >>> POWER4, but for PA6T I wouldn't be so sure. >>> >>> >> Thanks for looking out for us, obviously IBM doesn't (based on the reply a >> minute ago). > > The reason I deferred the question to Paul is really because I don't > know enough about PA6T and G5 to comment. I intentionally restricted the > changes to BOOK3S_64 because I wanted to make sure I don't break > anything else. It is in no way to hint that others don't care. Ah, I see -- the disconnect is that you don't think PA6T and 970 are 64-bit book3s CPUs. They are. -Olof
> Am 05.05.2014 um 16:57 schrieb Olof Johansson <olofj@google.com>: > > [Now without HTML email -- it's what you get for cc:ing me at work > instead of my upstream email :)] > > 2014-05-05 7:43 GMT-07:00 Alexander Graf <agraf@suse.de>: >> >>> On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >>> >>> Alexander Graf <agraf@suse.de> writes: >>> >>>>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>>> >>>>> Although it's optional IBM POWER cpus always had DAR value set on >>>>> alignment interrupt. So don't try to compute these values. >>>>> >>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>>> --- >>>>> Changes from V3: >>>>> * Use make_dsisr instead of checking feature flag to decide whether to use >>>>> saved dsisr or not >>> .... >>> >>>>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>>> { >>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>> + return vcpu->arch.fault_dar; >>>> >>>> How about PA6T and G5s? >>> Paul mentioned that BOOK3S always had DAR value set on alignment >>> interrupt. And the patch is to enable/collect correct DAR value when >>> running with Little Endian PR guest. Now to limit the impact and to >>> enable Little Endian PR guest, I ended up doing the conditional code >>> only for book3s 64 for which we know for sure that we set DAR value. >> >> >> Yes, and I'm asking whether we know that this statement holds true for PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at least developed by IBM, I'd assume its semantics here are similar to POWER4, but for PA6T I wouldn't be so sure. > > Thanks for looking out for us, obviously IBM doesn't (based on the > reply a minute ago). > > In the end, since there's been no work to enable KVM on PA6T, I'm not > too worried. I guess it's one more thing to sort out (and check for) > whenever someone does that. > > I definitely don't have cycles to deal with that myself at this time. > I can help find hardware for someone who wants to, but even then I'm > guessing the interest is pretty limited. I know of at least 1 person who successfully runs PR KVM on a PA6T, so it's neither neglected nor non-working. If you can get me access to a pa6t system I can easily check whether alignment interrupts generate dar and dsisr properly :). Alex
> Am 05.05.2014 um 16:50 schrieb "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>: > > Alexander Graf <agraf@suse.de> writes: > >>> On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >>> Alexander Graf <agraf@suse.de> writes: >>> >>>>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>>> Although it's optional IBM POWER cpus always had DAR value set on >>>>> alignment interrupt. So don't try to compute these values. >>>>> >>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>>> --- >>>>> Changes from V3: >>>>> * Use make_dsisr instead of checking feature flag to decide whether to use >>>>> saved dsisr or not >>> .... >>> >>>>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>>> { >>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>> + return vcpu->arch.fault_dar; >>>> How about PA6T and G5s? >>> Paul mentioned that BOOK3S always had DAR value set on alignment >>> interrupt. And the patch is to enable/collect correct DAR value when >>> running with Little Endian PR guest. Now to limit the impact and to >>> enable Little Endian PR guest, I ended up doing the conditional code >>> only for book3s 64 for which we know for sure that we set DAR value. >> >> Yes, and I'm asking whether we know that this statement holds true for >> PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is >> at least developed by IBM, I'd assume its semantics here are similar to >> POWER4, but for PA6T I wouldn't be so sure. > > I will have to defer to Paul on that question. But that should not > prevent this patch from going upstream right ? Regressions are big no-gos. Alex > > -aneesh >
Am 05.05.14 16:57, schrieb Olof Johansson: > [Now without HTML email -- it's what you get for cc:ing me at work > instead of my upstream email :)] > > 2014-05-05 7:43 GMT-07:00 Alexander Graf <agraf@suse.de>: >> On 05/05/2014 04:26 PM, Aneesh Kumar K.V wrote: >>> Alexander Graf <agraf@suse.de> writes: >>> >>>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>>> Although it's optional IBM POWER cpus always had DAR value set on >>>>> alignment interrupt. So don't try to compute these values. >>>>> >>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >>>>> --- >>>>> Changes from V3: >>>>> * Use make_dsisr instead of checking feature flag to decide whether to use >>>>> saved dsisr or not >>>>> >>> .... >>> >>>>> ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) >>>>> { >>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>> + return vcpu->arch.fault_dar; >>>> How about PA6T and G5s? >>>> >>>> >>> Paul mentioned that BOOK3S always had DAR value set on alignment >>> interrupt. And the patch is to enable/collect correct DAR value when >>> running with Little Endian PR guest. Now to limit the impact and to >>> enable Little Endian PR guest, I ended up doing the conditional code >>> only for book3s 64 for which we know for sure that we set DAR value. >> >> Yes, and I'm asking whether we know that this statement holds true for PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is at least developed by IBM, I'd assume its semantics here are similar to POWER4, but for PA6T I wouldn't be so sure. >> > Thanks for looking out for us, obviously IBM doesn't (based on the > reply a minute ago). > > In the end, since there's been no work to enable KVM on PA6T, I'm not > too worried. I guess it's one more thing to sort out (and check for) > whenever someone does that. > > I definitely don't have cycles to deal with that myself at this time. > I can help find hardware for someone who wants to, but even then I'm > guessing the interest is pretty limited. > > > -Olof > -- > 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 > Just for info: "PR" KVM works great on my PA6T machine. I booted the Lubuntu 14.04 PowerPC live DVD on a QEMU virtual machine with "PR" KVM successfully. But Mac OS X Jaguar, Panther, and Tiger don't boot with KVM on Mac-on-Linux and QEMU. See http://forum.hyperion-entertainment.biz/viewtopic.php?f=35&t=1747. -- Christian
On Mon, 2014-05-05 at 19:56 +0530, Aneesh Kumar K.V wrote: > > Paul mentioned that BOOK3S always had DAR value set on alignment > interrupt. And the patch is to enable/collect correct DAR value when > running with Little Endian PR guest. Now to limit the impact and to > enable Little Endian PR guest, I ended up doing the conditional code > only for book3s 64 for which we know for sure that we set DAR value. Only BookS ? Afaik, the kernel align.c unconditionally uses DAR on every processor type. It's DSISR that may or may not be populated but afaik DAR always is. Cheers, Ben.
On Mon, 2014-05-05 at 16:43 +0200, Alexander Graf wrote: > > Paul mentioned that BOOK3S always had DAR value set on alignment > > interrupt. And the patch is to enable/collect correct DAR value when > > running with Little Endian PR guest. Now to limit the impact and to > > enable Little Endian PR guest, I ended up doing the conditional code > > only for book3s 64 for which we know for sure that we set DAR value. > > Yes, and I'm asking whether we know that this statement holds true for > PA6T and G5 chips which I wouldn't consider IBM POWER. Since the G5 is > at least developed by IBM, I'd assume its semantics here are similar to > POWER4, but for PA6T I wouldn't be so sure. I am not aware of any PowerPC processor that does not set DAR on alignment interrupts. Paul, are you ? Cheers, Ben.
On Mon, May 05, 2014 at 01:19:30PM +0200, Alexander Graf wrote: > On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: > >+#ifdef CONFIG_PPC_BOOK3S_64 > >+ return vcpu->arch.fault_dar; > > How about PA6T and G5s? G5 sets DAR on an alignment interrupt. As for PA6T, I don't know for sure, but if it doesn't, ordinary alignment interrupts wouldn't be handled properly, since the code in arch/powerpc/kernel/align.c assumes DAR contains the address being accessed on all PowerPC CPUs. Did PA Semi ever publish a user manual for the PA6T, I wonder? Paul.
On 06.05.14 02:41, Paul Mackerras wrote: > On Mon, May 05, 2014 at 01:19:30PM +0200, Alexander Graf wrote: >> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>> +#ifdef CONFIG_PPC_BOOK3S_64 >>> + return vcpu->arch.fault_dar; >> How about PA6T and G5s? > G5 sets DAR on an alignment interrupt. > > As for PA6T, I don't know for sure, but if it doesn't, ordinary > alignment interrupts wouldn't be handled properly, since the code in > arch/powerpc/kernel/align.c assumes DAR contains the address being > accessed on all PowerPC CPUs. Now that's a good point. If we simply behave like Linux, I'm fine. This definitely deserves a comment on the #ifdef in the code. Alex
Alexander Graf <agraf@suse.de> writes: > On 06.05.14 02:41, Paul Mackerras wrote: >> On Mon, May 05, 2014 at 01:19:30PM +0200, Alexander Graf wrote: >>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>> + return vcpu->arch.fault_dar; >>> How about PA6T and G5s? >> G5 sets DAR on an alignment interrupt. >> >> As for PA6T, I don't know for sure, but if it doesn't, ordinary >> alignment interrupts wouldn't be handled properly, since the code in >> arch/powerpc/kernel/align.c assumes DAR contains the address being >> accessed on all PowerPC CPUs. > > Now that's a good point. If we simply behave like Linux, I'm fine. This > definitely deserves a comment on the #ifdef in the code. > Will update and send V5 -aneesh
Alexander Graf <agraf@suse.de> writes: > On 06.05.14 02:41, Paul Mackerras wrote: >> On Mon, May 05, 2014 at 01:19:30PM +0200, Alexander Graf wrote: >>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>> + return vcpu->arch.fault_dar; >>> How about PA6T and G5s? >> G5 sets DAR on an alignment interrupt. >> >> As for PA6T, I don't know for sure, but if it doesn't, ordinary >> alignment interrupts wouldn't be handled properly, since the code in >> arch/powerpc/kernel/align.c assumes DAR contains the address being >> accessed on all PowerPC CPUs. > > Now that's a good point. If we simply behave like Linux, I'm fine. This > definitely deserves a comment on the #ifdef in the code. How about ? #ifdef CONFIG_PPC_BOOK3S_64 /* * Linux always expect a valid dar as per alignment * interrupt handling code (fix_alignment()). Don't compute the dar * value here, instead used the saved dar value. Right now we restrict * this only for BOOK3S-64. */ return vcpu->arch.fault_dar; #else -aneesh
On 05/06/2014 04:12 PM, Aneesh Kumar K.V wrote: > Alexander Graf <agraf@suse.de> writes: > >> On 06.05.14 02:41, Paul Mackerras wrote: >>> On Mon, May 05, 2014 at 01:19:30PM +0200, Alexander Graf wrote: >>>> On 05/04/2014 07:21 PM, Aneesh Kumar K.V wrote: >>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>> + return vcpu->arch.fault_dar; >>>> How about PA6T and G5s? >>> G5 sets DAR on an alignment interrupt. >>> >>> As for PA6T, I don't know for sure, but if it doesn't, ordinary >>> alignment interrupts wouldn't be handled properly, since the code in >>> arch/powerpc/kernel/align.c assumes DAR contains the address being >>> accessed on all PowerPC CPUs. >> Now that's a good point. If we simply behave like Linux, I'm fine. This >> definitely deserves a comment on the #ifdef in the code. > > How about ? > > #ifdef CONFIG_PPC_BOOK3S_64 > /* > * Linux always expect a valid dar as per alignment > * interrupt handling code (fix_alignment()). Don't compute the dar > * value here, instead used the saved dar value. Right now we restrict > * this only for BOOK3S-64. > */ /* Linux's fix_alignment() assumes that DAR is valid, so can we */ Alex > return vcpu->arch.fault_dar; > #else > > > -aneesh >
diff --git a/arch/powerpc/include/asm/disassemble.h b/arch/powerpc/include/asm/disassemble.h index 856f8deb557a..6330a61b875a 100644 --- a/arch/powerpc/include/asm/disassemble.h +++ b/arch/powerpc/include/asm/disassemble.h @@ -81,4 +81,38 @@ static inline unsigned int get_oc(u32 inst) { return (inst >> 11) & 0x7fff; } + +#define IS_XFORM(inst) (get_op(inst) == 31) +#define IS_DSFORM(inst) (get_op(inst) >= 56) + +/* + * Create a DSISR value from the instruction + */ +static inline unsigned make_dsisr(unsigned instr) +{ + unsigned dsisr; + + + /* bits 6:15 --> 22:31 */ + dsisr = (instr & 0x03ff0000) >> 16; + + if (IS_XFORM(instr)) { + /* bits 29:30 --> 15:16 */ + dsisr |= (instr & 0x00000006) << 14; + /* bit 25 --> 17 */ + dsisr |= (instr & 0x00000040) << 8; + /* bits 21:24 --> 18:21 */ + dsisr |= (instr & 0x00000780) << 3; + } else { + /* bit 5 --> 17 */ + dsisr |= (instr & 0x04000000) >> 12; + /* bits 1: 4 --> 18:21 */ + dsisr |= (instr & 0x78000000) >> 17; + /* bits 30:31 --> 12:13 */ + if (IS_DSFORM(instr)) + dsisr |= (instr & 0x00000003) << 18; + } + + return dsisr; +} #endif /* __ASM_PPC_DISASSEMBLE_H__ */ diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c index 94908af308d8..34f55524d456 100644 --- a/arch/powerpc/kernel/align.c +++ b/arch/powerpc/kernel/align.c @@ -25,14 +25,13 @@ #include <asm/cputable.h> #include <asm/emulated_ops.h> #include <asm/switch_to.h> +#include <asm/disassemble.h> struct aligninfo { unsigned char len; unsigned char flags; }; -#define IS_XFORM(inst) (((inst) >> 26) == 31) -#define IS_DSFORM(inst) (((inst) >> 26) >= 56) #define INVALID { 0, 0 } @@ -192,37 +191,6 @@ static struct aligninfo aligninfo[128] = { }; /* - * Create a DSISR value from the instruction - */ -static inline unsigned make_dsisr(unsigned instr) -{ - unsigned dsisr; - - - /* bits 6:15 --> 22:31 */ - dsisr = (instr & 0x03ff0000) >> 16; - - if (IS_XFORM(instr)) { - /* bits 29:30 --> 15:16 */ - dsisr |= (instr & 0x00000006) << 14; - /* bit 25 --> 17 */ - dsisr |= (instr & 0x00000040) << 8; - /* bits 21:24 --> 18:21 */ - dsisr |= (instr & 0x00000780) << 3; - } else { - /* bit 5 --> 17 */ - dsisr |= (instr & 0x04000000) >> 12; - /* bits 1: 4 --> 18:21 */ - dsisr |= (instr & 0x78000000) >> 17; - /* bits 30:31 --> 12:13 */ - if (IS_DSFORM(instr)) - dsisr |= (instr & 0x00000003) << 18; - } - - return dsisr; -} - -/* * The dcbz (data cache block zero) instruction * gives an alignment fault if used on non-cacheable * memory. We handle the fault mainly for the diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c index 99d40f8977e8..04c38f049dfd 100644 --- a/arch/powerpc/kvm/book3s_emulate.c +++ b/arch/powerpc/kvm/book3s_emulate.c @@ -569,48 +569,14 @@ 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 make_dsisr(inst); } ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) { +#ifdef CONFIG_PPC_BOOK3S_64 + return vcpu->arch.fault_dar; +#else ulong dar = 0; ulong ra = get_ra(inst); ulong rb = get_rb(inst); @@ -635,4 +601,5 @@ ulong kvmppc_alignment_dar(struct kvm_vcpu *vcpu, unsigned int inst) } return dar; +#endif }
Although it's optional IBM POWER cpus always had DAR value set on alignment interrupt. So don't try to compute these values. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> --- Changes from V3: * Use make_dsisr instead of checking feature flag to decide whether to use saved dsisr or not arch/powerpc/include/asm/disassemble.h | 34 +++++++++++++++++++++++++++ arch/powerpc/kernel/align.c | 34 +-------------------------- arch/powerpc/kvm/book3s_emulate.c | 43 ++++------------------------------ 3 files changed, 40 insertions(+), 71 deletions(-)