Re: [PATCH 1/2] KVM: PPC: e6500: Handle LRAT error exception
On 09/30/2015 01:32 PM, Laurentiu Tudor wrote:
> On 09/25/2015 03:10 AM, Scott Wood wrote:
>> On Thu, 2015-09-24 at 16:11 +0300, Laurentiu Tudor wrote:
[snip]
>>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
>>> b/arch/powerpc/kvm/e500_mmu_host.c
>>> index 12d5c67..99ad88a 100644
>>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>>> @@ -96,6 +96,112 @@ static inline void __write_host_tlbe(struct
>>> kvm_book3e_206_tlb_entry *stlbe,
>>> stlbe->mas2, stlbe->mas7_3);
>>> }
>>>
>>> +#if defined(CONFIG_64BIT) && defined(CONFIG_KVM_BOOKE_HV)
>>> +static int lrat_next(void)
>>> +{
>>
>> Will anything break by removing the CONFIG_64BIT condition, even if we don't
>> have a 32-bit target that uses this?
>
> Not completly certain but i remember getting compile or link errors
> on 32-bit e500mc or e500v2. I can recheck if you want.
>
I double-checked this and indeed it doesn't compile on 32-bit because
lrat_next() calls get_paca().
---
Best Regards, Laurentiu
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: PPC: e6500: Handle LRAT error exception
On 09/30/2015 01:32 PM, Laurentiu Tudor wrote:
> On 09/25/2015 03:10 AM, Scott Wood wrote:
>> On Thu, 2015-09-24 at 16:11 +0300, Laurentiu Tudor wrote:
[snip]
>>> b/arch/powerpc/kvm/e500_mmu_host.c
>>> index 12d5c67..99ad88a 100644
>>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>>> @@ -96,6 +96,112 @@ static inline void __write_host_tlbe(struct
>>> kvm_book3e_206_tlb_entry *stlbe,
>>> stlbe->mas2, stlbe->mas7_3);
>>> }
>>>
>>> +#if defined(CONFIG_64BIT) && defined(CONFIG_KVM_BOOKE_HV)
>>> +static int lrat_next(void)
>>> +{
>>
>> Will anything break by removing the CONFIG_64BIT condition, even if we don't
>> have a 32-bit target that uses this?
>
> Not completly certain but i remember getting compile or link errors
> on 32-bit e500mc or e500v2. I can recheck if you want.
>
>>> +void kvmppc_lrat_map(struct kvm_vcpu *vcpu, gfn_t gfn)
>>> +{
>>> + struct kvm_memory_slot *slot;
>>> + unsigned long pfn;
>>> + unsigned long hva;
>>> + struct vm_area_struct *vma;
>>> + unsigned long psize;
>>> + int tsize;
>>> + unsigned long tsize_pages;
>>> +
>>> + slot = gfn_to_memslot(vcpu->kvm, gfn);
>>> + if (!slot) {
>>> + pr_err_ratelimited("%s: couldn't find memslot for gfn %lx!\n",
>>> +__func__, (long)gfn);
>>> + return;
>>> + }
>>> +
>>> + hva = slot->userspace_addr;
>>
>> What if the faulting address is somewhere in the middle of the slot?
>> Shouldn't you use gfn_to_hva_memslot() like kvmppc_e500_shadow_map()? In
>> fact there's probably a lot of logic that should be shared between these two
>> functions.
>
> So if my understanding is correct most of the gfn -> pfn translation
> stuff done in kvmppc_e500_shadow_map() should also be present in here.
> If that's the case maybe i should first extract this code (which includes
> VM_PFNMAP handling) in a separate function and call it from both
> kvmppc_lrat_map()
> and kvmppc_e500_shadow_map().
>
Off-topic, but just noticed that kvmppc_e500_shadow_map() is marked as inline.
Was that on purpose? Is inlining such a large function worth anything?
---
Best Regards, Laurentiu
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: PPC: e6500: Handle LRAT error exception
On 09/25/2015 03:10 AM, Scott Wood wrote:
> On Thu, 2015-09-24 at 16:11 +0300, Laurentiu Tudor wrote:
>> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S
>> b/arch/powerpc/kvm/bookehv_interrupts.S
>> index 81bd8a07..1e9fa2a 100644
>> --- a/arch/powerpc/kvm/bookehv_interrupts.S
>> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
>> @@ -62,6 +62,7 @@
>> #define NEED_EMU 0x0001 /* emulation -- save nv regs */
>> #define NEED_DEAR0x0002 /* save faulting DEAR */
>> #define NEED_ESR 0x0004 /* save faulting ESR */
>> +#define NEED_LPER0x0008 /* save faulting LPER */
>>
>> /*
>> * On entry:
>> @@ -159,6 +160,12 @@
>> PPC_STL r9, VCPU_FAULT_DEAR(r4)
>> .endif
>>
>> + /* Only supported on 64-bit cores for now */
>> + .if \flags & NEED_LPER
>> + mfspr r7, SPRN_LPER
>> + std r7, VCPU_FAULT_LPER(r4)
>> + .endif
>
> What's the harm in using PPC_STL anyway?
Will do so.
>
>> /*
>> * For input register values, see
>> arch/powerpc/include/asm/kvm_booke_hv_asm.h
>> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
>> b/arch/powerpc/kvm/e500_mmu_host.c
>> index 12d5c67..99ad88a 100644
>> --- a/arch/powerpc/kvm/e500_mmu_host.c
>> +++ b/arch/powerpc/kvm/e500_mmu_host.c
>> @@ -96,6 +96,112 @@ static inline void __write_host_tlbe(struct
>> kvm_book3e_206_tlb_entry *stlbe,
>> stlbe->mas2, stlbe->mas7_3);
>> }
>>
>> +#if defined(CONFIG_64BIT) && defined(CONFIG_KVM_BOOKE_HV)
>> +static int lrat_next(void)
>> +{
>
> Will anything break by removing the CONFIG_64BIT condition, even if we don't
> have a 32-bit target that uses this?
Not completly certain but i remember getting compile or link errors
on 32-bit e500mc or e500v2. I can recheck if you want.
>> +void kvmppc_lrat_map(struct kvm_vcpu *vcpu, gfn_t gfn)
>> +{
>> + struct kvm_memory_slot *slot;
>> + unsigned long pfn;
>> + unsigned long hva;
>> + struct vm_area_struct *vma;
>> + unsigned long psize;
>> + int tsize;
>> + unsigned long tsize_pages;
>> +
>> + slot = gfn_to_memslot(vcpu->kvm, gfn);
>> + if (!slot) {
>> + pr_err_ratelimited("%s: couldn't find memslot for gfn %lx!\n",
>> +__func__, (long)gfn);
>> + return;
>> + }
>> +
>> + hva = slot->userspace_addr;
>
> What if the faulting address is somewhere in the middle of the slot?
> Shouldn't you use gfn_to_hva_memslot() like kvmppc_e500_shadow_map()? In
> fact there's probably a lot of logic that should be shared between these two
> functions.
So if my understanding is correct most of the gfn -> pfn translation
stuff done in kvmppc_e500_shadow_map() should also be present in here.
If that's the case maybe i should first extract this code (which includes
VM_PFNMAP handling) in a separate function and call it from both
kvmppc_lrat_map()
and kvmppc_e500_shadow_map().
>> + down_read(>mm->mmap_sem);
>> + vma = find_vma(current->mm, hva);
>> + if (vma && (hva >= vma->vm_start)) {
>> + psize = vma_kernel_pagesize(vma);
>
> What if it's VM_PFNMAP?
>
>> + } else {
>> + pr_err_ratelimited("%s: couldn't find virtual memory address
>> for gfn
>> %lx!\n",
>> +__func__, (long)gfn);
>> + up_read(>mm->mmap_sem);
>> + return;
>> + }
>> + up_read(>mm->mmap_sem);
>> +
>> + pfn = gfn_to_pfn_memslot(slot, gfn);
>> + if (is_error_noslot_pfn(pfn)) {
>> + pr_err_ratelimited("%s: couldn't get real page for gfn %lx!\n",
>> +__func__, (long)gfn);
>> + return;
>> + }
>> +
>> + tsize = __ilog2(psize) - 10;
>> + tsize_pages = 1 << (tsize + 10 - PAGE_SHIFT);
>
> 1UL << ...
>
> kvmppc_e500_shadow_map needs the same fix.
I'll make a distinct patch with the kvmppc_e500_shadow_map() fix.
>> + gfn &= ~(tsize_pages - 1);
>> + pfn &= ~(tsize_pages - 1);
>> +
>> + write_host_lrate(tsize, gfn, pfn, vcpu->kvm->arch.lpid, true);
>> +
>> + kvm_release_pfn_clean(pfn);
>> +}
>> +
>> +void kvmppc_lrat_invalidate(struct kvm_vcpu *vcpu)
>> +{
>> + uint32_t mas0, mas1 = 0;
>> + int esel;
>> + unsigned long flags;
>> +
>> + local_irq_save(flags);
>> +
>> + /* LRAT does not have a dedicated instruction for invalidation */
>> + for (esel = 0; esel < get_paca()->tcd_ptr->lrat_max; esel++) {
>> + mas0 = MAS0_ATSEL | MAS0_ESEL(esel);
>> + mtspr(SPRN_MAS0, mas0);
>> + asm volatile("isync; tlbre" : : : "memory");
>> + mas1 = mfspr(SPRN_MAS1) & ~MAS1_VALID;
>> + mtspr(SPRN_MAS1, mas1);
>> + asm volatile("isync; tlbwe" : : : "memory");
>> + }
>> + /* Must clear mas8 for other host tlbwe's */
>> + mtspr(SPRN_MAS8, 0);
>> + isync();
>> +
>> + local_irq_restore(flags);
>> +}
>> +#endif /* CONFIG_64BIT
Re: [PATCH 1/2] KVM: PPC: e6500: Handle LRAT error exception
On Wed, 2015-09-30 at 14:27 +0300, Laurentiu Tudor wrote:
> On 09/30/2015 01:32 PM, Laurentiu Tudor wrote:
> > On 09/25/2015 03:10 AM, Scott Wood wrote:
> > > On Thu, 2015-09-24 at 16:11 +0300, Laurentiu Tudor wrote:
>
> [snip]
>
> > > > b/arch/powerpc/kvm/e500_mmu_host.c
> > > > index 12d5c67..99ad88a 100644
> > > > --- a/arch/powerpc/kvm/e500_mmu_host.c
> > > > +++ b/arch/powerpc/kvm/e500_mmu_host.c
> > > > @@ -96,6 +96,112 @@ static inline void __write_host_tlbe(struct
> > > > kvm_book3e_206_tlb_entry *stlbe,
> > > > stlbe->mas2, stlbe->mas7_3);
> > > > }
> > > >
> > > > +#if defined(CONFIG_64BIT) && defined(CONFIG_KVM_BOOKE_HV)
> > > > +static int lrat_next(void)
> > > > +{
> > >
> > > Will anything break by removing the CONFIG_64BIT condition, even if we
> > > don't
> > > have a 32-bit target that uses this?
> >
> > Not completly certain but i remember getting compile or link errors
> > on 32-bit e500mc or e500v2. I can recheck if you want.
> >
> > > > +void kvmppc_lrat_map(struct kvm_vcpu *vcpu, gfn_t gfn)
> > > > +{
> > > > + struct kvm_memory_slot *slot;
> > > > + unsigned long pfn;
> > > > + unsigned long hva;
> > > > + struct vm_area_struct *vma;
> > > > + unsigned long psize;
> > > > + int tsize;
> > > > + unsigned long tsize_pages;
> > > > +
> > > > + slot = gfn_to_memslot(vcpu->kvm, gfn);
> > > > + if (!slot) {
> > > > + pr_err_ratelimited("%s: couldn't find memslot for gfn
> > > > %lx!\n",
> > > > +__func__, (long)gfn);
> > > > + return;
> > > > + }
> > > > +
> > > > + hva = slot->userspace_addr;
> > >
> > > What if the faulting address is somewhere in the middle of the slot?
> > > Shouldn't you use gfn_to_hva_memslot() like kvmppc_e500_shadow_map()?
> > > In
> > > fact there's probably a lot of logic that should be shared between
> > > these two
> > > functions.
> >
> > So if my understanding is correct most of the gfn -> pfn translation
> > stuff done in kvmppc_e500_shadow_map() should also be present in here.
> > If that's the case maybe i should first extract this code (which includes
> > VM_PFNMAP handling) in a separate function and call it from both
> > kvmppc_lrat_map()
> > and kvmppc_e500_shadow_map().
> >
>
> Off-topic, but just noticed that kvmppc_e500_shadow_map() is marked as
> inline.
> Was that on purpose? Is inlining such a large function worth anything?
I don't remember the intent. It was probably a lot smaller back then. That
said, it's only used two places, with probably pretty good temporal
separation between performance-intensive uses of one versus the other (so not
a huge icache concern), and a pretty good portion of the function will be
optimized out in the caller with tlbsel == 0.
-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: PPC: e6500: Handle LRAT error exception
On Thu, 2015-09-24 at 16:11 +0300, Laurentiu Tudor wrote:
> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S
> b/arch/powerpc/kvm/bookehv_interrupts.S
> index 81bd8a07..1e9fa2a 100644
> --- a/arch/powerpc/kvm/bookehv_interrupts.S
> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
> @@ -62,6 +62,7 @@
> #define NEED_EMU 0x0001 /* emulation -- save nv regs */
> #define NEED_DEAR0x0002 /* save faulting DEAR */
> #define NEED_ESR 0x0004 /* save faulting ESR */
> +#define NEED_LPER0x0008 /* save faulting LPER */
>
> /*
> * On entry:
> @@ -159,6 +160,12 @@
> PPC_STL r9, VCPU_FAULT_DEAR(r4)
> .endif
>
> + /* Only supported on 64-bit cores for now */
> + .if \flags & NEED_LPER
> + mfspr r7, SPRN_LPER
> + std r7, VCPU_FAULT_LPER(r4)
> + .endif
What's the harm in using PPC_STL anyway?
> /*
> * For input register values, see
> arch/powerpc/include/asm/kvm_booke_hv_asm.h
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c
> b/arch/powerpc/kvm/e500_mmu_host.c
> index 12d5c67..99ad88a 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -96,6 +96,112 @@ static inline void __write_host_tlbe(struct
> kvm_book3e_206_tlb_entry *stlbe,
> stlbe->mas2, stlbe->mas7_3);
> }
>
> +#if defined(CONFIG_64BIT) && defined(CONFIG_KVM_BOOKE_HV)
> +static int lrat_next(void)
> +{
Will anything break by removing the CONFIG_64BIT condition, even if we don't
have a 32-bit target that uses this?
> +void kvmppc_lrat_map(struct kvm_vcpu *vcpu, gfn_t gfn)
> +{
> + struct kvm_memory_slot *slot;
> + unsigned long pfn;
> + unsigned long hva;
> + struct vm_area_struct *vma;
> + unsigned long psize;
> + int tsize;
> + unsigned long tsize_pages;
> +
> + slot = gfn_to_memslot(vcpu->kvm, gfn);
> + if (!slot) {
> + pr_err_ratelimited("%s: couldn't find memslot for gfn %lx!\n",
> +__func__, (long)gfn);
> + return;
> + }
> +
> + hva = slot->userspace_addr;
What if the faulting address is somewhere in the middle of the slot?
Shouldn't you use gfn_to_hva_memslot() like kvmppc_e500_shadow_map()? In
fact there's probably a lot of logic that should be shared between these two
functions.
> + down_read(>mm->mmap_sem);
> + vma = find_vma(current->mm, hva);
> + if (vma && (hva >= vma->vm_start)) {
> + psize = vma_kernel_pagesize(vma);
What if it's VM_PFNMAP?
> + } else {
> + pr_err_ratelimited("%s: couldn't find virtual memory address
> for gfn
> %lx!\n",
> +__func__, (long)gfn);
> + up_read(>mm->mmap_sem);
> + return;
> + }
> + up_read(>mm->mmap_sem);
> +
> + pfn = gfn_to_pfn_memslot(slot, gfn);
> + if (is_error_noslot_pfn(pfn)) {
> + pr_err_ratelimited("%s: couldn't get real page for gfn %lx!\n",
> +__func__, (long)gfn);
> + return;
> + }
> +
> + tsize = __ilog2(psize) - 10;
> + tsize_pages = 1 << (tsize + 10 - PAGE_SHIFT);
1UL << ...
kvmppc_e500_shadow_map needs the same fix.
> + gfn &= ~(tsize_pages - 1);
> + pfn &= ~(tsize_pages - 1);
> +
> + write_host_lrate(tsize, gfn, pfn, vcpu->kvm->arch.lpid, true);
> +
> + kvm_release_pfn_clean(pfn);
> +}
> +
> +void kvmppc_lrat_invalidate(struct kvm_vcpu *vcpu)
> +{
> + uint32_t mas0, mas1 = 0;
> + int esel;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> +
> + /* LRAT does not have a dedicated instruction for invalidation */
> + for (esel = 0; esel < get_paca()->tcd_ptr->lrat_max; esel++) {
> + mas0 = MAS0_ATSEL | MAS0_ESEL(esel);
> + mtspr(SPRN_MAS0, mas0);
> + asm volatile("isync; tlbre" : : : "memory");
> + mas1 = mfspr(SPRN_MAS1) & ~MAS1_VALID;
> + mtspr(SPRN_MAS1, mas1);
> + asm volatile("isync; tlbwe" : : : "memory");
> + }
> + /* Must clear mas8 for other host tlbwe's */
> + mtspr(SPRN_MAS8, 0);
> + isync();
> +
> + local_irq_restore(flags);
> +}
> +#endif /* CONFIG_64BIT && CONFIG_KVM_BOOKE_HV */
> +
> /*
> * Acquire a mas0 with victim hint, as if we just took a TLB miss.
> *
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c
> index cda695d..5856f8f 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -99,6 +99,10 @@ void kvmppc_e500_tlbil_all(struct kvmppc_vcpu_e500
> *vcpu_e500)
> asm volatile("tlbilxlpid");
> mtspr(SPRN_MAS5, 0);
> local_irq_restore(flags);
> +
> +#ifdef PPC64
> + kvmppc_lrat_invalidate(_e500->vcpu);
> +#endif
Don't you mean CONFIG_PPC64 (or CONFIG_64BIT to be consistent)?
> }
>
> void kvmppc_set_pid(struct kvm_vcpu *vcpu, u32 pid)
> diff --git a/arch/powerpc/mm/fsl_booke_mmu.c
>
