Re: [PATCH] KVM: PPC: Implement extension to report number of memslots

[Scott Wood]

On Mon, 2015-10-26 at 16:15 +1100, Paul Mackerras wrote:
> On Fri, Oct 16, 2015 at 08:41:31AM +0200, Thomas Huth wrote:
> > Yes, we'll likely need this soon! 32 slots are not enough...
> 
> Would anyone object if I raised the limit for PPC to 512 slots?
> Would that cause problems on embedded PPC, for instance?

I can't think of any reason why it would cause problems.

-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] KVM: PPC: Implement extension to report number of memslots

[Scott Wood]

On Mon, 2015-10-26 at 16:15 +1100, Paul Mackerras wrote:
> On Fri, Oct 16, 2015 at 08:41:31AM +0200, Thomas Huth wrote:
> > Yes, we'll likely need this soon! 32 slots are not enough...
> 
> Would anyone object if I raised the limit for PPC to 512 slots?
> Would that cause problems on embedded PPC, for instance?

I can't think of any reason why it would cause problems.

-Scott

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

Re: [PATCH] KVM: PPC: e500: fix couple of shift operations on 64 bits

[Scott Wood]

On Thu, 2015-10-01 at 15:58 +0300, Laurentiu Tudor wrote:
> Fix couple of cases where we shift left a 32-bit
> value thus might get truncated results on 64-bit
> targets.
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> Suggested-by: Scott Wood <scotttw...@freescale.com>
> ---
>  arch/powerpc/kvm/e500_mmu_host.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Scott Wood <scottw...@freescale.com>

-Scott

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

Re: [PATCH] KVM: PPC: e500: fix couple of shift operations on 64 bits

[Scott Wood]

On Thu, 2015-10-01 at 15:58 +0300, Laurentiu Tudor wrote:
> Fix couple of cases where we shift left a 32-bit
> value thus might get truncated results on 64-bit
> targets.
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> Suggested-by: Scott Wood <scotttw...@freescale.com>
> ---
>  arch/powerpc/kvm/e500_mmu_host.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Scott Wood <scottw...@freescale.com>

-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

[Scott Wood]

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][v2] KVM: PPC: e500: Emulate TMCFG0 TMRN register

[Scott Wood]

On Fri, 2015-09-25 at 18:02 +0300, Laurentiu Tudor wrote:
> Emulate TMCFG0 TMRN register exposing one HW thread per vcpu.
> 
> Signed-off-by: Mihai Caraman <mihai.cara...@freescale.com>
> [laurentiu.tu...@freescale.com: rebased on latest kernel, use
>  define instead of hardcoded value, moved code in own function]
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> ---
> v2:
>  - moved code in its own function
> Needs this patch: https://patchwork.ozlabs.org/patch/521752/
> 
>  arch/powerpc/include/asm/disassemble.h |  5 +
>  arch/powerpc/kvm/e500_emulate.c| 19 +++
>  2 files changed, 24 insertions(+)

Acked-by: Scott Wood <scotttw...@freescale.com>

-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][v2] KVM: PPC: e500: Emulate TMCFG0 TMRN register

[Scott Wood]

On Fri, 2015-09-25 at 18:02 +0300, Laurentiu Tudor wrote:
> Emulate TMCFG0 TMRN register exposing one HW thread per vcpu.
> 
> Signed-off-by: Mihai Caraman <mihai.cara...@freescale.com>
> [laurentiu.tu...@freescale.com: rebased on latest kernel, use
>  define instead of hardcoded value, moved code in own function]
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> ---
> v2:
>  - moved code in its own function
> Needs this patch: https://patchwork.ozlabs.org/patch/521752/
> 
>  arch/powerpc/include/asm/disassemble.h |  5 +
>  arch/powerpc/kvm/e500_emulate.c| 19 +++
>  2 files changed, 24 insertions(+)

Acked-by: Scott Wood <scotttw...@freescale.com>

-Scott

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

Re: [PATCH] KVM: PPC: e6500: support powers of 2K TLB1 sizes

[Scott Wood]

On Fri, 2015-09-25 at 17:30 +0300, Laurentiu Tudor wrote:
> On 09/24/2015 11:23 PM, Scott Wood wrote:
> > On Thu, 2015-09-24 at 15:57 +0300, Laurentiu Tudor wrote:
> > > Book-E MMUv2 present in e6500 cores supports
> > > powers of 2K page sizes while older MMUv1 cores
> > > support only powers of 4K page sizes, or in other
> > > words the LSB of TSIZE on MMUv1 is always 0.
> > > Thus, on MMUv2 we must not strip the LSB.
> > 
> > We can get better TLB utilization by not stripping it, but why "must not" 
> > which makes it sound like a bugfix rather than an optimization?
> 
> Not sure i get it. If a guests wants a 2K^^(2n+1) translation
> size it will instead get a 2K^^2n size, no? Isn't this an issue?

It will get two pages of 2k^^2n size (assuming both halves are accessed).  It 
will work, it will just consume twice as many TLB1 entries.  It's the same as 
if the host pages backing the region are smaller than the mapping that the 
guest tries to create.

-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] KVM: PPC: e6500: support powers of 2K TLB1 sizes

[Scott Wood]

On Thu, 2015-09-24 at 15:57 +0300, Laurentiu Tudor wrote:
> Book-E MMUv2 present in e6500 cores supports
> powers of 2K page sizes while older MMUv1 cores
> support only powers of 4K page sizes, or in other
> words the LSB of TSIZE on MMUv1 is always 0.
> Thus, on MMUv2 we must not strip the LSB.

We can get better TLB utilization by not stripping it, but why "must not" 
which makes it sound like a bugfix rather than an optimization?

> Signed-off-by: Mihai Caraman 
> [laurentiu.tu...@freescale.com: addressed review
>  feedback, split in distinct patch]
> Signed-off-by: Laurentiu Tudor 
> ---
>  arch/powerpc/kvm/e500_mmu_host.c | 28 +---
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c 
> b/arch/powerpc/kvm/e500_mmu_host.c
> index 4d33e19..12d5c67 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -371,6 +371,7 @@ static inline int kvmppc_e500_shadow_map(struct 
> kvmppc_vcpu_e500 *vcpu_e500,
>  
>   unsigned long start, end;
>   unsigned long slot_start, slot_end;
> + int tsize_inc;
>  
>   pfnmap = 1;
>  
> @@ -392,10 +393,20 @@ static inline int kvmppc_e500_shadow_map(struct 
> kvmppc_vcpu_e500 *vcpu_e500,
>   MAS1_TSIZE_SHIFT;
>  
>   /*
> -  * e500 doesn't implement the lowest tsize bit,
> -  * or 1K pages.
> +  * MMUv1 doesn't implement the lowest tsize bit,
> +  * or translations smaller than 4K.
>*/
> - tsize = max(BOOK3E_PAGESZ_4K, tsize & ~1);
> + if (!has_feature(_e500->vcpu, VCPU_FTR_MMU_V2))
> + tsize &= ~1;
> + tsize = max(BOOK3E_PAGESZ_4K, tsize);
> +
> + /*
> +  * Calculate TSIZE increment. MMUv2 supports
> +  * power of 2K translations while MMUv1 is limited
> +  * to power of 4K sizes.
> +  */
> + tsize_inc = has_feature(_e500->vcpu,
> + VCPU_FTR_MMU_V2) ? 1 : 2;

If you calculate tsize_inc first, then the previous if-statement can become 
"tsize &= ~(tsize_inc - 1);".

>  
>   /*
>* Now find the largest tsize (up to what the guest
> @@ -404,7 +415,8 @@ static inline int kvmppc_e500_shadow_map(struct 
> kvmppc_vcpu_e500 *vcpu_e500,
>* aligned.
>*/
>  
> - for (; tsize > BOOK3E_PAGESZ_4K; tsize -= 2) {
> + for (; tsize > BOOK3E_PAGESZ_4K;
> +  tsize -= tsize_inc) {
>   unsigned long gfn_start, gfn_end;
>   tsize_pages = 1 << (tsize - 2);
>  
> @@ -437,10 +449,12 @@ static inline int kvmppc_e500_shadow_map(struct 
> kvmppc_vcpu_e500 *vcpu_e500,
>   tsize = min(__ilog2(psize) - 10, tsize);
>  
>   /*
> -  * e500 doesn't implement the lowest tsize bit,
> -  * or 1K pages.
> +  * MMUv1 doesn't implement the lowest tsize bit,
> +  * or translations smaller than 4K.
>*/

This comment makes it sound like MMUv2 might support translations smaller 
than 4K.

-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] KVM: PPC: e500: Emulate TMCFG0 TMRN register

[Scott Wood]

On Thu, 2015-09-24 at 09:56 +0300, Laurentiu Tudor wrote:
> Emulate TMCFG0 TMRN register exposing one HW thread per vcpu.
> 
> Signed-off-by: Mihai Caraman 
> [laurentiu.tu...@freescale.com: rebased on latest kernel,
>  use define instead of hardcoded value]
> Signed-off-by: Laurentiu Tudor 
> ---
> Needs this patch: https://patchwork.ozlabs.org/patch/521752/
> 
>  arch/powerpc/include/asm/disassemble.h |  5 +
>  arch/powerpc/kvm/e500_emulate.c| 11 +++
>  2 files changed, 16 insertions(+)

KVM patches should be sent to k...@vger.kernel.org in addition to kvm-
p...@vger.kernel.org.

> @@ -165,6 +167,15 @@ int kvmppc_core_emulate_op_e500(struct kvm_run *run, 
> struct kvm_vcpu *vcpu,
>   emulated = kvmppc_e500_emul_tlbivax(vcpu, ea);
>   break;
>  
> + case XOP_MFTMR:
> + /* Expose one thread per vcpu */
> + if (get_tmrn(inst) == TMRN_TMCFG0)
> + kvmppc_set_gpr(vcpu, rt,
> +1 | (1 << 
> TMRN_TMCFG0_NATHRD_SHIFT));
> + else
> + emulated = EMULATE_FAIL;
> + break;

Line length.  Please move the implementation into its own function like all 
the others.

-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] powerpc/e6500: add TMCFG0 register definition

[Scott Wood]

On Wed, 2015-09-23 at 18:06 +0300, Laurentiu Tudor wrote:
> The register is not currently used in the base kernel
> but will be in a forthcoming kvm patch.
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> ---
>  arch/powerpc/include/asm/reg_booke.h | 6 ++
>  1 file changed, 6 insertions(+)

Acked-by: Scott Wood <scottw...@freescale.com>

-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 15/19] KVM: PPC: e500: fix handling local_sid_lookup result

[Scott Wood]

On Thu, 2015-09-24 at 16:00 +0200, Andrzej Hajda wrote:
> The function can return negative value.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046107
> 
> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
> ---
> Hi,
> 
> To avoid problems with too many mail recipients I have sent whole
> patchset only to LKML. Anyway patches have no dependencies.
> 
> Regards
> Andrzej
> ---
>  arch/powerpc/kvm/e500.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Scott Wood <scottw...@freescale.com>

-Scott

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

Re: [PATCH 15/19] KVM: PPC: e500: fix handling local_sid_lookup result

[Scott Wood]

On Thu, 2015-09-24 at 16:00 +0200, Andrzej Hajda wrote:
> The function can return negative value.
> 
> The problem has been detected using proposed semantic patch
> scripts/coccinelle/tests/assign_signed_to_unsigned.cocci [1].
> 
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046107
> 
> Signed-off-by: Andrzej Hajda <a.ha...@samsung.com>
> ---
> Hi,
> 
> To avoid problems with too many mail recipients I have sent whole
> patchset only to LKML. Anyway patches have no dependencies.
> 
> Regards
> Andrzej
> ---
>  arch/powerpc/kvm/e500.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Scott Wood <scottw...@freescale.com>

-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

[Scott Wood]

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 
> 

Re: [PATCH] powerpc/e500: move qemu machine spec together with the rest

[Scott Wood]

On Mon, 2015-09-14 at 16:17 +0300, Laurentiu Tudor wrote:
> On 09/10/2015 02:01 AM, Scott Wood wrote:
> > On Fri, 2015-09-04 at 15:46 +0300, Laurentiu Tudor wrote:
> > > This way we get rid of an entire file with mostly
> > > duplicated code plus a Kconfig option that you always
> > > had to take care to check it in order for kvm to work.
> > > 
> > > Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> > > ---
> > >  arch/powerpc/platforms/85xx/Kconfig   | 15 -
> > >  arch/powerpc/platforms/85xx/Makefile  |  1 -
> > >  arch/powerpc/platforms/85xx/corenet_generic.c |  1 +
> > >  arch/powerpc/platforms/85xx/qemu_e500.c   | 85 
> > > 
> > 
> > 
> > qemu_e500 is not only for corenet chips.  
> 
> That's too bad. :-(
> I remember discussions on dropping the e500v2 support at some point in time?
> 
> > We can add it to the defconfig (in fact I've been meaning to do so).
> 
> Or maybe just drop de KConfig option and
> wrap the file in an #ifdef CONFIG_KVM or something along these lines?
>  
> > > -static void __init qemu_e500_setup_arch(void)
> > > -{
> > > - ppc_md.progress("qemu_e500_setup_arch()", 0);
> > > -
> > > - fsl_pci_assign_primary();
> > > - swiotlb_detect_4g();
> > 
> > Where is fsl_pci_assign_primary() in corenet_generic.c?  
> 
> This commit claims it's not needed and drops it:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7d4d595dad30328bc6153e235d90f54c918fc127

That commit has nothing to do with QEMU.

> > At one point this 
> > was needed for QEMU's PCI implementation -- have you tested QEMU PCI 
> > without 
> > it?
> 
> Well, somehow i've (embarrassingly) messed up my initial tests.
> I've retested after seeing your comment and indeed this breaks pci under 
> qemu.
> Adding to the confusion, the commit above made me think that the removal 
> was safe.
> Why pci qemu doesn't work without the call to fsl_pci_assign_primary() is
> an interesting subject.

IIRC it has to do with QEMU not liking a BAR set to zero.

-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] powerpc/e500: move qemu machine spec together with the rest

[Scott Wood]

On Mon, 2015-09-14 at 16:14 +0200, Alexander Graf wrote:
> > Am 14.09.2015 um 15:17 schrieb Laurentiu Tudor <b10...@freescale.com>:
> > 
> > > On 09/10/2015 02:01 AM, Scott Wood wrote:
> > > > On Fri, 2015-09-04 at 15:46 +0300, Laurentiu Tudor wrote:
> > > > This way we get rid of an entire file with mostly
> > > > duplicated code plus a Kconfig option that you always
> > > > had to take care to check it in order for kvm to work.
> > > > 
> > > > Signed-off-by: Laurentiu Tudor <laurentiu.tu...@freescale.com>
> > > > ---
> > > > arch/powerpc/platforms/85xx/Kconfig   | 15 -
> > > > arch/powerpc/platforms/85xx/Makefile  |  1 -
> > > > arch/powerpc/platforms/85xx/corenet_generic.c |  1 +
> > > > arch/powerpc/platforms/85xx/qemu_e500.c   | 85 ---
> > > > -
> > > 
> > > 
> > > qemu_e500 is not only for corenet chips.  
> > 
> > That's too bad. :-(
> > I remember discussions on dropping the e500v2 support at some point in 
> > time?
> > 
> > > We can add it to the defconfig (in fact I've been meaning to do so).
> > 
> > Or maybe just drop de KConfig option and
> > wrap the file in an #ifdef CONFIG_KVM or something along these lines?
> 
> CONFIG_KVM is for host support though. This is for the guest kernel.

CONFIG_QEMU_E500 can also be used with TCG -- it's not KVM-specific.

-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] powerpc/e500: move qemu machine spec together with the rest

[Scott Wood]

On Fri, 2015-09-04 at 15:46 +0300, Laurentiu Tudor wrote:
> This way we get rid of an entire file with mostly
> duplicated code plus a Kconfig option that you always
> had to take care to check it in order for kvm to work.
> 
> Signed-off-by: Laurentiu Tudor 
> ---
>  arch/powerpc/platforms/85xx/Kconfig   | 15 -
>  arch/powerpc/platforms/85xx/Makefile  |  1 -
>  arch/powerpc/platforms/85xx/corenet_generic.c |  1 +
>  arch/powerpc/platforms/85xx/qemu_e500.c   | 85 


qemu_e500 is not only for corenet chips.  We can add it to the defconfig (in 
fact I've been meaning to do so).

> -static void __init qemu_e500_setup_arch(void)
> -{
> - ppc_md.progress("qemu_e500_setup_arch()", 0);
> -
> - fsl_pci_assign_primary();
> - swiotlb_detect_4g();

Where is fsl_pci_assign_primary() in corenet_generic.c?  At one point this 
was needed for QEMU's PCI implementation -- have you tested QEMU PCI without 
it?

-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 v5 1/2] perf,kvm/ppc: Add kvm_perf.h for powerpc

[Scott Wood]

[Added KVM lists and a couple relevant people]

On Fri, 2015-07-31 at 14:25 +0530, Hemant Kumar wrote:
 On 07/30/2015 03:52 AM, Scott Wood wrote:
  On Wed, 2015-07-29 at 16:07 +0530, Hemant Kumar wrote:
   Hi Scott,
   
   On 07/17/2015 01:40 AM, Scott Wood wrote:
On Thu, 2015-07-16 at 21:18 +0530, Hemant Kumar wrote:
 To analyze the exit events with perf, we need kvm_perf.h to be 
 added in
 the arch/powerpc directory, where the kvm tracepoints needed to 
 trace
 the KVM exit events are defined.
 
 This patch adds kvm_perf_book3s.h to indicate that the 
 tracepoints are
 book3s specific. Generic kvm_perf.h then can just include
 kvm_perf_book3s.h.
 
 Signed-off-by: Hemant Kumar hem...@linux.vnet.ibm.com
 ---
 Changes:
 - Not exporting the exit reasons compared to previous patchset
 (suggested
 by Paul)
 
arch/powerpc/include/uapi/asm/kvm_perf.h|  6 ++
arch/powerpc/include/uapi/asm/kvm_perf_book3s.h | 14 
 ++
2 files changed, 20 insertions(+)
create mode 100644 arch/powerpc/include/uapi/asm/kvm_perf.h
create mode 100644 
 arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 
 diff --git a/arch/powerpc/include/uapi/asm/kvm_perf.h
 b/arch/powerpc/include/uapi/asm/kvm_perf.h
 new file mode 100644
 index 000..5ed2ff3
 --- /dev/null
 +++ b/arch/powerpc/include/uapi/asm/kvm_perf.h
 @@ -0,0 +1,6 @@
 +#ifndef _ASM_POWERPC_KVM_PERF_H
 +#define _ASM_POWERPC_KVM_PERF_H
 +
 +#include asm/kvm_perf_book3s.h
 +
 +#endif
 diff --git a/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 b/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 new file mode 100644
 index 000..8c8d8c2
 --- /dev/null
 +++ b/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 @@ -0,0 +1,14 @@
 +#ifndef _ASM_POWERPC_KVM_PERF_BOOK3S_H
 +#define _ASM_POWERPC_KVM_PERF_BOOK3S_H
 +
 +#include asm/kvm.h
 +
 +#define DECODE_STR_LEN 20
 +
 +#define VCPU_ID vcpu_id
 +
 +#define KVM_ENTRY_TRACE kvm_hv:kvm_guest_enter
 +#define KVM_EXIT_TRACE kvm_hv:kvm_guest_exit
 +#define KVM_EXIT_REASON trap
 +
 +#endif /* _ASM_POWERPC_KVM_PERF_BOOK3S_H */
Again, why is book3s stuff being presented via uapi as generic
asm/kvm_perf.h with generic symbol names?

-Scott
   Ok.
   
   We can change the KVM_ENTRY_TRACE macro to something like
   KVM_BOOK3S_ENTRY_TRACE and likewise for KVM_EXIT_TRACE
   and KVM_EXIT_REASON
  What about DECODE_STR_LEN and VCPU_ID?
 
 DECODE_STR_LEN can be common, we can give a big enough size to it, if
 we need to.
 And, VCPU_ID depends on the field in the tracepoint payload data which is
 specific to that tracepoint. This field is used to maintain the per vcpu 
 record
 and this field gives us the vcpu id. So, yeah, I guess, since, I can't 
 find any
 such field as vcpu_id in the kvm_exit tracepoint for book3e, we have to
 make this specific to book3s.

Or maybe we could add kvm_guest_enter/kvm_guest_leave, with vcpu_id, to 
book3e... though the kvm-hv would be a problem for book3s-pr, if anyone cares 
about this feature there.

I'm not sure why the strings are present both in the UAPI header, as well as 
in kvm_events_tp[] in kvm-stat.c.

  Where is this API documented?
  
 and then, to resolve the issue of generic
   macro names in the userspace side, we can handle it using __weak
   modifier.
  Does userspace get built differently for book3s versus book3e?  For now 
  it'd
  
  be fine for userspace to check for book3s and not use the feature if it's
  
  book3e.  If and when book3e gains this feature, then userspace can be 
  changed.
 
 Well, I couldn't find any way to build user space differently for book3s and
 book3e.
 
 How about keeping this as it is after modifying the tracepoint macro names
 to book3s specific in the uapi? And as and when booke decides to implement
 this feature, a runtime check for event availability can be added then, 
 IMHO.
 
 What do you think?

What does userspace use, at runtime, to determine if this feature is present 
and whether the book3s symbols should be used?

Deferring the implementation of book3e support is fine, but from a uapi 
perspective it should be discoverable at runtime whether the feature exposed 
by asm/kvm_perf_book3s.h is available.  Otherwise, if it is implemented (or 
even if it isn't), you have the potential for user confusion if an older perf 
tool is used.  This sort of discovery is done all the time in the KVM APIs 
themselves.

FWIW, on x86 cpu_isa_init() uses cpuid to select an exit table.  Using PVR 
seems like it would be a bit cumbersome though compared to the kernel 
directly exporting which subarch it is (and it wouldn't help with HV versus 
PR).

   What would you suggest?
  Another option would be to explain this interface so that we can figure 
  out
  if book3e would even

Re: [PATCH v5 1/2] perf,kvm/ppc: Add kvm_perf.h for powerpc

[Scott Wood]

[Added KVM lists and a couple relevant people]

On Fri, 2015-07-31 at 14:25 +0530, Hemant Kumar wrote:
 On 07/30/2015 03:52 AM, Scott Wood wrote:
  On Wed, 2015-07-29 at 16:07 +0530, Hemant Kumar wrote:
   Hi Scott,
   
   On 07/17/2015 01:40 AM, Scott Wood wrote:
On Thu, 2015-07-16 at 21:18 +0530, Hemant Kumar wrote:
 To analyze the exit events with perf, we need kvm_perf.h to be 
 added in
 the arch/powerpc directory, where the kvm tracepoints needed to 
 trace
 the KVM exit events are defined.
 
 This patch adds kvm_perf_book3s.h to indicate that the 
 tracepoints are
 book3s specific. Generic kvm_perf.h then can just include
 kvm_perf_book3s.h.
 
 Signed-off-by: Hemant Kumar hem...@linux.vnet.ibm.com
 ---
 Changes:
 - Not exporting the exit reasons compared to previous patchset
 (suggested
 by Paul)
 
arch/powerpc/include/uapi/asm/kvm_perf.h|  6 ++
arch/powerpc/include/uapi/asm/kvm_perf_book3s.h | 14 
 ++
2 files changed, 20 insertions(+)
create mode 100644 arch/powerpc/include/uapi/asm/kvm_perf.h
create mode 100644 
 arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 
 diff --git a/arch/powerpc/include/uapi/asm/kvm_perf.h
 b/arch/powerpc/include/uapi/asm/kvm_perf.h
 new file mode 100644
 index 000..5ed2ff3
 --- /dev/null
 +++ b/arch/powerpc/include/uapi/asm/kvm_perf.h
 @@ -0,0 +1,6 @@
 +#ifndef _ASM_POWERPC_KVM_PERF_H
 +#define _ASM_POWERPC_KVM_PERF_H
 +
 +#include asm/kvm_perf_book3s.h
 +
 +#endif
 diff --git a/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 b/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 new file mode 100644
 index 000..8c8d8c2
 --- /dev/null
 +++ b/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
 @@ -0,0 +1,14 @@
 +#ifndef _ASM_POWERPC_KVM_PERF_BOOK3S_H
 +#define _ASM_POWERPC_KVM_PERF_BOOK3S_H
 +
 +#include asm/kvm.h
 +
 +#define DECODE_STR_LEN 20
 +
 +#define VCPU_ID vcpu_id
 +
 +#define KVM_ENTRY_TRACE kvm_hv:kvm_guest_enter
 +#define KVM_EXIT_TRACE kvm_hv:kvm_guest_exit
 +#define KVM_EXIT_REASON trap
 +
 +#endif /* _ASM_POWERPC_KVM_PERF_BOOK3S_H */
Again, why is book3s stuff being presented via uapi as generic
asm/kvm_perf.h with generic symbol names?

-Scott
   Ok.
   
   We can change the KVM_ENTRY_TRACE macro to something like
   KVM_BOOK3S_ENTRY_TRACE and likewise for KVM_EXIT_TRACE
   and KVM_EXIT_REASON
  What about DECODE_STR_LEN and VCPU_ID?
 
 DECODE_STR_LEN can be common, we can give a big enough size to it, if
 we need to.
 And, VCPU_ID depends on the field in the tracepoint payload data which is
 specific to that tracepoint. This field is used to maintain the per vcpu 
 record
 and this field gives us the vcpu id. So, yeah, I guess, since, I can't 
 find any
 such field as vcpu_id in the kvm_exit tracepoint for book3e, we have to
 make this specific to book3s.

Or maybe we could add kvm_guest_enter/kvm_guest_leave, with vcpu_id, to 
book3e... though the kvm-hv would be a problem for book3s-pr, if anyone cares 
about this feature there.

I'm not sure why the strings are present both in the UAPI header, as well as 
in kvm_events_tp[] in kvm-stat.c.

  Where is this API documented?
  
 and then, to resolve the issue of generic
   macro names in the userspace side, we can handle it using __weak
   modifier.
  Does userspace get built differently for book3s versus book3e?  For now 
  it'd
  
  be fine for userspace to check for book3s and not use the feature if it's
  
  book3e.  If and when book3e gains this feature, then userspace can be 
  changed.
 
 Well, I couldn't find any way to build user space differently for book3s and
 book3e.
 
 How about keeping this as it is after modifying the tracepoint macro names
 to book3s specific in the uapi? And as and when booke decides to implement
 this feature, a runtime check for event availability can be added then, 
 IMHO.
 
 What do you think?

What does userspace use, at runtime, to determine if this feature is present 
and whether the book3s symbols should be used?

Deferring the implementation of book3e support is fine, but from a uapi 
perspective it should be discoverable at runtime whether the feature exposed 
by asm/kvm_perf_book3s.h is available.  Otherwise, if it is implemented (or 
even if it isn't), you have the potential for user confusion if an older perf 
tool is used.  This sort of discovery is done all the time in the KVM APIs 
themselves.

FWIW, on x86 cpu_isa_init() uses cpuid to select an exit table.  Using PVR 
seems like it would be a bit cumbersome though compared to the kernel 
directly exporting which subarch it is (and it wouldn't help with HV versus 
PR).

   What would you suggest?
  Another option would be to explain this interface so that we can figure 
  out
  if book3e would even

Re: [RFC PATCH 6/6] powerpc/kvm: change the condition of identifying hugetlb vm

[Scott Wood]

On Tue, 2015-07-07 at 16:05 +0800, wenwei tao wrote:
 Hi Scott
 
 I understand what you said.
 
 I will use the function 'is_vm_hugetlb_page()' to hide the bit
 combinations according to your comments in the next version of patch
 set.
 
 But for the situation like below, there isn't an obvious structure
 'vma', using 'is_vm_hugetlb_page()' maybe costly or even not possible.
 void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
 unsigned long end, unsigned long vmflag)
 {
 ...
 
 if (end == TLB_FLUSH_ALL || tlb_flushall_shift == -1
 || vmflag  VM_HUGETLB) {
 local_flush_tlb();
 goto flush_all;
 }
 ...
 }

Add a function that operates on the flags directly, then.

-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: [RFC PATCH 6/6] powerpc/kvm: change the condition of identifying hugetlb vm

[Scott Wood]

On Tue, 2015-07-07 at 16:05 +0800, wenwei tao wrote:
 Hi Scott
 
 I understand what you said.
 
 I will use the function 'is_vm_hugetlb_page()' to hide the bit
 combinations according to your comments in the next version of patch
 set.
 
 But for the situation like below, there isn't an obvious structure
 'vma', using 'is_vm_hugetlb_page()' maybe costly or even not possible.
 void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
 unsigned long end, unsigned long vmflag)
 {
 ...
 
 if (end == TLB_FLUSH_ALL || tlb_flushall_shift == -1
 || vmflag  VM_HUGETLB) {
 local_flush_tlb();
 goto flush_all;
 }
 ...
 }

Add a function that operates on the flags directly, then.

-Scott

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

Re: [RFC PATCH 6/6] powerpc/kvm: change the condition of identifying hugetlb vm

[Scott Wood]

On Fri, 2015-07-03 at 16:47 +0800, wenwei tao wrote:
 Hi Scott
 
 Thank you for your comments.
 
 Kernel already has that function: is_vm_hugetlb_page() , but the
 original code didn't use it,
 in order to keep the coding style of the original code, I didn't use it 
 either.

 For the sentence like: vma-vm_flags  VM_HUGETLB , hiding it behind
 'is_vm_hugetlb_page()' is ok,
 but the sentence like: vma-vm_flags 
 (VM_LOCKED|VM_HUGETLB|VM_PFNMAP) appears in the patch 2/6,
 is it better to hide the bit combinations behind the
 is_vm_hugetlb_page() ?  In my patch I just replaced it with
 vma-vm_flags  (VM_LOCKED|VM_PFNMAP) ||  (vma-vm_flags 
 (VM_HUGETLB|VM_MERGEABLE)) == VM_HUGETLB.

If you're going to do non-obvious things with the flags, it should be done in 
one place rather than throughout the code.  Why would you do the above and 
not vma-vm_flags  (VM_LOCKED | VM_PFNMAP) || is_vm_hugetlb_page(vma)?

-Scott

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

Re: [RFC PATCH 6/6] powerpc/kvm: change the condition of identifying hugetlb vm

[Scott Wood]

On Fri, 2015-07-03 at 16:47 +0800, wenwei tao wrote:
 Hi Scott
 
 Thank you for your comments.
 
 Kernel already has that function: is_vm_hugetlb_page() , but the
 original code didn't use it,
 in order to keep the coding style of the original code, I didn't use it 
 either.

 For the sentence like: vma-vm_flags  VM_HUGETLB , hiding it behind
 'is_vm_hugetlb_page()' is ok,
 but the sentence like: vma-vm_flags 
 (VM_LOCKED|VM_HUGETLB|VM_PFNMAP) appears in the patch 2/6,
 is it better to hide the bit combinations behind the
 is_vm_hugetlb_page() ?  In my patch I just replaced it with
 vma-vm_flags  (VM_LOCKED|VM_PFNMAP) ||  (vma-vm_flags 
 (VM_HUGETLB|VM_MERGEABLE)) == VM_HUGETLB.

If you're going to do non-obvious things with the flags, it should be done in 
one place rather than throughout the code.  Why would you do the above and 
not vma-vm_flags  (VM_LOCKED | VM_PFNMAP) || is_vm_hugetlb_page(vma)?

-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: [RFC PATCH 6/6] powerpc/kvm: change the condition of identifying hugetlb vm

[Scott Wood]

On Wed, 2015-06-10 at 14:27 +0800, Wenwei Tao wrote:
 Hugetlb VMAs are not mergeable, that means a VMA couldn't have VM_HUGETLB 
 and
 VM_MERGEABLE been set in the same time. So we use VM_HUGETLB to indicate new
 mergeable VMAs. Because of that a VMA which has VM_HUGETLB been set is a 
 hugetlb
 VMA only if it doesn't have VM_MERGEABLE been set in the same time.

Eww.

If you must overload such bit combinations, please hide it behind a 
vm_is_hugetlb() function.

-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: [RFC PATCH 6/6] powerpc/kvm: change the condition of identifying hugetlb vm

[Scott Wood]

On Wed, 2015-06-10 at 14:27 +0800, Wenwei Tao wrote:
 Hugetlb VMAs are not mergeable, that means a VMA couldn't have VM_HUGETLB 
 and
 VM_MERGEABLE been set in the same time. So we use VM_HUGETLB to indicate new
 mergeable VMAs. Because of that a VMA which has VM_HUGETLB been set is a 
 hugetlb
 VMA only if it doesn't have VM_MERGEABLE been set in the same time.

Eww.

If you must overload such bit combinations, please hide it behind a 
vm_is_hugetlb() function.

-Scott

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

Re: [PATCH][RFC] KVM: PPC: fix suspicious use of conditional operator

[Scott Wood]

On Fri, 2015-05-22 at 17:46 +0300, Laurentiu Tudor wrote:
 This was signaled by a static code analysis tool.
 
 Signed-off-by: Laurentiu Tudor laurentiu.tu...@freescale.com
 ---
  arch/powerpc/kvm/e500_mmu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/arch/powerpc/kvm/e500_mmu.c b/arch/powerpc/kvm/e500_mmu.c
 index 50860e9..29911a0 100644
 --- a/arch/powerpc/kvm/e500_mmu.c
 +++ b/arch/powerpc/kvm/e500_mmu.c
 @@ -377,7 +377,7 @@ int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, gva_t 
 ea)
   | MAS0_NV(vcpu_e500-gtlb_nv[tlbsel]);
   vcpu-arch.shared-mas1 =
 (vcpu-arch.shared-mas6  MAS6_SPID0)
 - | (vcpu-arch.shared-mas6  (MAS6_SAS ? MAS1_TS : 0))
 + | ((vcpu-arch.shared-mas6  MAS6_SAS) ? MAS1_TS : 0)
   | (vcpu-arch.shared-mas4  MAS4_TSIZED(~0));
   vcpu-arch.shared-mas2 = MAS2_EPN;
   vcpu-arch.shared-mas2 |= vcpu-arch.shared-mas4 

Reviewed-by: Scott Wood scottw...@freescale.com

Please send as non-RFC.

-Scott


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

Re: [PATCH][RFC] KVM: PPC: fix suspicious use of conditional operator

[Scott Wood]

On Fri, 2015-05-22 at 17:46 +0300, Laurentiu Tudor wrote:
 This was signaled by a static code analysis tool.
 
 Signed-off-by: Laurentiu Tudor laurentiu.tu...@freescale.com
 ---
  arch/powerpc/kvm/e500_mmu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/arch/powerpc/kvm/e500_mmu.c b/arch/powerpc/kvm/e500_mmu.c
 index 50860e9..29911a0 100644
 --- a/arch/powerpc/kvm/e500_mmu.c
 +++ b/arch/powerpc/kvm/e500_mmu.c
 @@ -377,7 +377,7 @@ int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, gva_t 
 ea)
   | MAS0_NV(vcpu_e500-gtlb_nv[tlbsel]);
   vcpu-arch.shared-mas1 =
 (vcpu-arch.shared-mas6  MAS6_SPID0)
 - | (vcpu-arch.shared-mas6  (MAS6_SAS ? MAS1_TS : 0))
 + | ((vcpu-arch.shared-mas6  MAS6_SAS) ? MAS1_TS : 0)
   | (vcpu-arch.shared-mas4  MAS4_TSIZED(~0));
   vcpu-arch.shared-mas2 = MAS2_EPN;
   vcpu-arch.shared-mas2 |= vcpu-arch.shared-mas4 

Reviewed-by: Scott Wood scottw...@freescale.com

Please send as non-RFC.

-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] KVM: PPC: check for lookup_linux_ptep() returning NULL

[Scott Wood]

On Thu, 2015-05-21 at 16:26 +0300, Laurentiu Tudor wrote:
 If passed a larger page size lookup_linux_ptep()
 may fail, so add a check for that and bail out
 if that's the case.
 This was found with the help of a static
 code analysis tool.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 Signed-off-by: Laurentiu Tudor laurentiu.tu...@freescale.com
 Cc: Scott Wood scottw...@freescale.com
 ---
 based on https://github.com/agraf/linux-2.6.git kvm-ppc-next
 
  arch/powerpc/kvm/e500_mmu_host.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Scott Wood scottw...@freescale.com

-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] KVM: PPC: check for lookup_linux_ptep() returning NULL

[Scott Wood]

On Thu, 2015-05-21 at 16:26 +0300, Laurentiu Tudor wrote:
 If passed a larger page size lookup_linux_ptep()
 may fail, so add a check for that and bail out
 if that's the case.
 This was found with the help of a static
 code analysis tool.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 Signed-off-by: Laurentiu Tudor laurentiu.tu...@freescale.com
 Cc: Scott Wood scottw...@freescale.com
 ---
 based on https://github.com/agraf/linux-2.6.git kvm-ppc-next
 
  arch/powerpc/kvm/e500_mmu_host.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Scott Wood scottw...@freescale.com

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/1] KVM: PPC: Book3S: correct width in XER handling

[Scott Wood]

On Wed, 2015-05-20 at 15:26 +1000, Sam Bobroff wrote:
 In 64 bit kernels, the Fixed Point Exception Register (XER) is a 64
 bit field (e.g. in kvm_regs and kvm_vcpu_arch) and in most places it is
 accessed as such.
 
 This patch corrects places where it is accessed as a 32 bit field by a
 64 bit kernel.  In some cases this is via a 32 bit load or store
 instruction which, depending on endianness, will cause either the
 lower or upper 32 bits to be missed.  In another case it is cast as a
 u32, causing the upper 32 bits to be cleared.
 
 This patch corrects those places by extending the access methods to
 64 bits.
 
 Signed-off-by: Sam Bobroff sam.bobr...@au1.ibm.com
 ---
 
  arch/powerpc/include/asm/kvm_book3s.h   |4 ++--
  arch/powerpc/kvm/book3s_hv_rmhandlers.S |6 +++---
  arch/powerpc/kvm/book3s_segment.S   |4 ++--
  3 files changed, 7 insertions(+), 7 deletions(-)

It's nominally a 64-bit register, but the upper 32 bits are reserved in
ISA 2.06.  Do newer ISAs or certain implementations define things in the
upper 32 bits, or is this just about the asm accesses being wrong on
big-endian?

Also, I see the same issue in the booke code (CCing Mihai).

-Scott

 diff --git a/arch/powerpc/include/asm/kvm_book3s.h 
 b/arch/powerpc/include/asm/kvm_book3s.h
 index b91e74a..05a875a 100644
 --- a/arch/powerpc/include/asm/kvm_book3s.h
 +++ b/arch/powerpc/include/asm/kvm_book3s.h
 @@ -225,12 +225,12 @@ static inline u32 kvmppc_get_cr(struct kvm_vcpu *vcpu)
   return vcpu-arch.cr;
  }
  
 -static inline void kvmppc_set_xer(struct kvm_vcpu *vcpu, u32 val)
 +static inline void kvmppc_set_xer(struct kvm_vcpu *vcpu, ulong val)
  {
   vcpu-arch.xer = val;
  }
  
 -static inline u32 kvmppc_get_xer(struct kvm_vcpu *vcpu)
 +static inline ulong kvmppc_get_xer(struct kvm_vcpu *vcpu)
  {
   return vcpu-arch.xer;
  }
 diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S 
 b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
 index 4d70df2..d75be59 100644
 --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
 +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
 @@ -870,7 +870,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
   blt hdec_soon
  
   ld  r6, VCPU_CTR(r4)
 - lwz r7, VCPU_XER(r4)
 + ld  r7, VCPU_XER(r4)
  
   mtctr   r6
   mtxer   r7
 @@ -1103,7 +1103,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
   mfctr   r3
   mfxer   r4
   std r3, VCPU_CTR(r9)
 - stw r4, VCPU_XER(r9)
 + std r4, VCPU_XER(r9)
  
   /* If this is a page table miss then see if it's theirs or ours */
   cmpwi   r12, BOOK3S_INTERRUPT_H_DATA_STORAGE
 @@ -1675,7 +1675,7 @@ kvmppc_hdsi:
   bl  kvmppc_msr_interrupt
  fast_interrupt_c_return:
  6:   ld  r7, VCPU_CTR(r9)
 - lwz r8, VCPU_XER(r9)
 + ld  r8, VCPU_XER(r9)
   mtctr   r7
   mtxer   r8
   mr  r4, r9
 diff --git a/arch/powerpc/kvm/book3s_segment.S 
 b/arch/powerpc/kvm/book3s_segment.S
 index acee37c..ca8f174 100644
 --- a/arch/powerpc/kvm/book3s_segment.S
 +++ b/arch/powerpc/kvm/book3s_segment.S
 @@ -123,7 +123,7 @@ no_dcbz32_on:
   PPC_LL  r8, SVCPU_CTR(r3)
   PPC_LL  r9, SVCPU_LR(r3)
   lwz r10, SVCPU_CR(r3)
 - lwz r11, SVCPU_XER(r3)
 + PPC_LL  r11, SVCPU_XER(r3)
  
   mtctr   r8
   mtlrr9
 @@ -237,7 +237,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
   mfctr   r8
   mflrr9
  
 - stw r5, SVCPU_XER(r13)
 + PPC_STL r5, SVCPU_XER(r13)
   PPC_STL r6, SVCPU_FAULT_DAR(r13)
   stw r7, SVCPU_FAULT_DSISR(r13)
   PPC_STL r8, SVCPU_CTR(r13)


--
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: [RFC PATCH] KVM: introduce kvm_check_device

[Scott Wood]

On Wed, 2015-01-07 at 10:55 +, Andre Przywara wrote:
 Hi Scott,
 
 thanks for looking at the patch.
 
 On 06/01/15 20:52, Scott Wood wrote:
  Out of curiosity, why do you need to test it from inside the kernel but
  outside kvm_main.c?
 
 I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c.
 The problem is that while KVM_CREATE_DEVICE works fine with checking the
 availability of the requested device, KVM_CREATE_IRQCHIP does not - and
 the latter is handled in the arch specific parts of the code. At the
 moment the GIC_V2 is the only IRQ chip, so it's all or nothing right
 now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not
 always being available, so KVM_CREATE_IRQCHIP may fail although there is
 an in-kernel IRQ chip available.
 Instead of hacking something up I thought it would be cleaner to use the
 existing framework and just map KVM_CREATE_IRQCHIP to
 KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2).

In that case you'd need the full create_device functionality from
arch/arm/kvm, not just testing whether a device type is present, right?

-Scott


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

Re: [RFC PATCH] KVM: introduce kvm_check_device

[Scott Wood]

On Wed, 2015-01-07 at 18:11 +, Andre Przywara wrote:
 On 07/01/15 17:45, Scott Wood wrote:
  On Wed, 2015-01-07 at 10:55 +, Andre Przywara wrote:
  Hi Scott,
 
  thanks for looking at the patch.
 
  On 06/01/15 20:52, Scott Wood wrote:
  Out of curiosity, why do you need to test it from inside the kernel but
  outside kvm_main.c?
 
  I need it from arch/arm/kvm/arm.c or alternatively virt/kvm/arm/vgic.c.
  The problem is that while KVM_CREATE_DEVICE works fine with checking the
  availability of the requested device, KVM_CREATE_IRQCHIP does not - and
  the latter is handled in the arch specific parts of the code. At the
  moment the GIC_V2 is the only IRQ chip, so it's all or nothing right
  now. But very soon there will be a GIC_V3, with GIC_V2 compatibility not
  always being available, so KVM_CREATE_IRQCHIP may fail although there is
  an in-kernel IRQ chip available.
  Instead of hacking something up I thought it would be cleaner to use the
  existing framework and just map KVM_CREATE_IRQCHIP to
  KVM_CREATE_DEVICE(KVM_DEV_TYPE_ARM_VGIC_V2).
  
  In that case you'd need the full create_device functionality from
  arch/arm/kvm, not just testing whether a device type is present, right?
 
 Well, not really. On KVM_CREATE_IRQCHIP kvm_vgic_create is called, which
 is also what the KVM device .create function for GIC_V2 does.
 So yes, we don't fully use the KVM device framework, but just use the
 same functionality.
 Not sure if it would make sense to use more of the KVM device framework,
 as currently there is no issue with the current approach and I just need
 to know whether the GIC_V2 has been registered.

If you're communicating privately with the vgic code to create the
irqchip, can't you also communicate privately to see if that
functionality is available?  I'm not opposed to this patch, but it seems
odd to use the device framework just for testing whether some other
interface is available.

-Scott

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

Re: [RFC PATCH] KVM: introduce kvm_check_device

[Scott Wood]

On Tue, 2015-01-06 at 16:12 +, Andre Przywara wrote:
 While we can easily register and unregister KVM devices, there is
 currently no easy way of checking whether a device has been
 registered.
 Introduce kvm_check_device() for that purpose and use it in two
 existing functions. Also change the return code for an invalid
 type number from ENOSPC to EINVAL.
 This function will be later used by another patch set to check
 whether a KVM_CREATE_IRQCHIP ioctl is valid.

You're checking whether a device type has been registered, not the
device itself -- so could you make it kvm_check_device_type()?

 Signed-off-by: Andre Przywara andre.przyw...@arm.com
 ---
 Hi,
 
 can people comment whether there is an easier way to detect KVM
 device registration _outside_ of virt/kvm/kvm_main.c? Using the
 KVM_CREATE_DEVICE_TEST flag sounds like a fit, but
 kvm_ioctl_create_device() isn't exported.

Out of curiosity, why do you need to test it from inside the kernel but
outside kvm_main.c?

-Scott


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

Re: [PATCH] KVM: PPC: Convert openpic lock to raw_spinlock

[Scott Wood]

On Fri, 2014-09-12 at 09:12 -0500, Purcareata Bogdan-B43198 wrote:
  -Original Message-
  From: Wood Scott-B07421
  Sent: Thursday, September 11, 2014 9:19 PM
  To: Purcareata Bogdan-B43198
  Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org
  Subject: Re: [PATCH] KVM: PPC: Convert openpic lock to raw_spinlock
  
  On Thu, 2014-09-11 at 15:25 -0400, Bogdan Purcareata wrote:
   This patch enables running intensive I/O workloads, e.g. netperf, in a 
   guest
   deployed on a RT host. No change for !RT kernels.
  
   The openpic spinlock becomes a sleeping mutex on a RT system. This no 
   longer
   guarantees that EPR is atomic with exception delivery. The guest VCPU 
   thread
   fails due to a BUG_ON(preemptible()) when running netperf.
  
   In order to make the kvmppc_mpic_set_epr() call safe on RT from non-atomic
   context, convert the openpic lock to a raw_spinlock. A similar approach 
   can
   be seen for x86 platforms in the following commit [1].
  
   Here are some comparative cyclitest measurements run inside a high 
   priority
  RT
   guest run on a RT host. The guest has 1 VCPU and the test has been run for
  15
   minutes. The guest runs ~750 hackbench processes as background stress.
  
  Does hackbench involve triggering interrupts that would go through the
  MPIC?  You may want to try an I/O-heavy benchmark to stress the MPIC
  code (the more interrupt sources are active at once, the better).
 
 Before this patch, running netperf/iperf in the guest always resulted
 in hitting the afore-mentioned BUG_ON, when the host was RT. This is
 why I can't provide comparative cyclitest measurements before and after
 the patch, with heavy I/O stress. Since I had no problem running
 hackbench before, I'm assuming it doesn't involve interrupts passing
 through the MPIC. The measurements were posted just to show that the
 patch doesn't mess up anything somewhere else.

I know you can't provide before/after, but it would be nice to see what
the after numbers are with heavy MPIC activity.

  Also try a guest with many vcpus.
 
 AFAIK, without the MSI affinity patches [1], all vfio interrupts will
 go to core 0 in the guest. In this case, I guess there won't be
 contention induced latencies due to multiple VCPUs expecting to have
 their interrupts delivered. Am I getting it wrong?

It's not about contention, but about loops in the MPIC code that iterate
over the entire set of vcpus.

-Scott

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

Re: [PATCH] KVM: PPC: Convert openpic lock to raw_spinlock

[Scott Wood]

On Fri, 2014-09-12 at 09:12 -0500, Purcareata Bogdan-B43198 wrote:
  -Original Message-
  From: Wood Scott-B07421
  Sent: Thursday, September 11, 2014 9:19 PM
  To: Purcareata Bogdan-B43198
  Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org
  Subject: Re: [PATCH] KVM: PPC: Convert openpic lock to raw_spinlock
  
  On Thu, 2014-09-11 at 15:25 -0400, Bogdan Purcareata wrote:
   This patch enables running intensive I/O workloads, e.g. netperf, in a 
   guest
   deployed on a RT host. No change for !RT kernels.
  
   The openpic spinlock becomes a sleeping mutex on a RT system. This no 
   longer
   guarantees that EPR is atomic with exception delivery. The guest VCPU 
   thread
   fails due to a BUG_ON(preemptible()) when running netperf.
  
   In order to make the kvmppc_mpic_set_epr() call safe on RT from non-atomic
   context, convert the openpic lock to a raw_spinlock. A similar approach 
   can
   be seen for x86 platforms in the following commit [1].
  
   Here are some comparative cyclitest measurements run inside a high 
   priority
  RT
   guest run on a RT host. The guest has 1 VCPU and the test has been run for
  15
   minutes. The guest runs ~750 hackbench processes as background stress.
  
  Does hackbench involve triggering interrupts that would go through the
  MPIC?  You may want to try an I/O-heavy benchmark to stress the MPIC
  code (the more interrupt sources are active at once, the better).
 
 Before this patch, running netperf/iperf in the guest always resulted
 in hitting the afore-mentioned BUG_ON, when the host was RT. This is
 why I can't provide comparative cyclitest measurements before and after
 the patch, with heavy I/O stress. Since I had no problem running
 hackbench before, I'm assuming it doesn't involve interrupts passing
 through the MPIC. The measurements were posted just to show that the
 patch doesn't mess up anything somewhere else.

I know you can't provide before/after, but it would be nice to see what
the after numbers are with heavy MPIC activity.

  Also try a guest with many vcpus.
 
 AFAIK, without the MSI affinity patches [1], all vfio interrupts will
 go to core 0 in the guest. In this case, I guess there won't be
 contention induced latencies due to multiple VCPUs expecting to have
 their interrupts delivered. Am I getting it wrong?

It's not about contention, but about loops in the MPIC code that iterate
over the entire set of vcpus.

-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] KVM: PPC: Convert openpic lock to raw_spinlock

[Scott Wood]

On Thu, 2014-09-11 at 15:25 -0400, Bogdan Purcareata wrote:
 This patch enables running intensive I/O workloads, e.g. netperf, in a guest
 deployed on a RT host. No change for !RT kernels.
 
 The openpic spinlock becomes a sleeping mutex on a RT system. This no longer
 guarantees that EPR is atomic with exception delivery. The guest VCPU thread
 fails due to a BUG_ON(preemptible()) when running netperf.
 
 In order to make the kvmppc_mpic_set_epr() call safe on RT from non-atomic
 context, convert the openpic lock to a raw_spinlock. A similar approach can
 be seen for x86 platforms in the following commit [1].
 
 Here are some comparative cyclitest measurements run inside a high priority RT
 guest run on a RT host. The guest has 1 VCPU and the test has been run for 15
 minutes. The guest runs ~750 hackbench processes as background stress.

Does hackbench involve triggering interrupts that would go through the
MPIC?  You may want to try an I/O-heavy benchmark to stress the MPIC
code (the more interrupt sources are active at once, the better).
Also try a guest with many vcpus.

-Scott


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

Re: [PATCH] KVM: PPC: Convert openpic lock to raw_spinlock

[Scott Wood]

On Thu, 2014-09-11 at 15:25 -0400, Bogdan Purcareata wrote:
 This patch enables running intensive I/O workloads, e.g. netperf, in a guest
 deployed on a RT host. No change for !RT kernels.
 
 The openpic spinlock becomes a sleeping mutex on a RT system. This no longer
 guarantees that EPR is atomic with exception delivery. The guest VCPU thread
 fails due to a BUG_ON(preemptible()) when running netperf.
 
 In order to make the kvmppc_mpic_set_epr() call safe on RT from non-atomic
 context, convert the openpic lock to a raw_spinlock. A similar approach can
 be seen for x86 platforms in the following commit [1].
 
 Here are some comparative cyclitest measurements run inside a high priority RT
 guest run on a RT host. The guest has 1 VCPU and the test has been run for 15
 minutes. The guest runs ~750 hackbench processes as background stress.

Does hackbench involve triggering interrupts that would go through the
MPIC?  You may want to try an I/O-heavy benchmark to stress the MPIC
code (the more interrupt sources are active at once, the better).
Also try a guest with many vcpus.

-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 v4] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-08-27 at 13:23 +0200, Alexander Graf wrote:
 
 On 13.08.14 11:09, Bharat Bhushan wrote:
  This patch emulates debug registers and debug exception
  to support guest using debug resource. This enables running
  gdb/kgdb etc in guest.
  
  On BOOKE architecture we cannot share debug resources between QEMU and
  guest because:
  When QEMU is using debug resources then debug exception must
  be always enabled. To achieve this we set MSR_DE and also set
  MSRP_DEP so guest cannot change MSR_DE.
  
  When emulating debug resource for guest we want guest
  to control MSR_DE (enable/disable debug interrupt on need).
  
  So above mentioned two configuration cannot be supported
  at the same time. So the result is that we cannot share
  debug resources between QEMU and Guest on BOOKE architecture.
  
  In the current design QEMU gets priority over guest, this means that if
  QEMU is using debug resources then guest cannot use them and if guest is
  using debug resource then QEMU can overwrite them.
  
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 
 Scott, could you please recheck whether you're ok with it now? :)

I'm OK with it.

-Scott


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

Re: [PATCH v4] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-08-27 at 13:23 +0200, Alexander Graf wrote:
 
 On 13.08.14 11:09, Bharat Bhushan wrote:
  This patch emulates debug registers and debug exception
  to support guest using debug resource. This enables running
  gdb/kgdb etc in guest.
  
  On BOOKE architecture we cannot share debug resources between QEMU and
  guest because:
  When QEMU is using debug resources then debug exception must
  be always enabled. To achieve this we set MSR_DE and also set
  MSRP_DEP so guest cannot change MSR_DE.
  
  When emulating debug resource for guest we want guest
  to control MSR_DE (enable/disable debug interrupt on need).
  
  So above mentioned two configuration cannot be supported
  at the same time. So the result is that we cannot share
  debug resources between QEMU and Guest on BOOKE architecture.
  
  In the current design QEMU gets priority over guest, this means that if
  QEMU is using debug resources then guest cannot use them and if guest is
  using debug resource then QEMU can overwrite them.
  
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 
 Scott, could you please recheck whether you're ok with it now? :)

I'm OK with it.

-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 v2 1/2] powerpc/booke: Restrict SPE exception handlers to e200/e500 cores

[Scott Wood]

On Wed, 2014-08-20 at 16:09 +0300, Mihai Caraman wrote:
 SPE exception handlers are now defined for 32-bit e500mc cores even though
 SPE unit is not present and CONFIG_SPE is undefined.
 
 Restrict SPE exception handlers to e200/e500 cores adding CONFIG_SPE_POSSIBLE
 and consequently guard __stup_ivors and __setup_cpu functions.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 Cc: Scott Wood scottw...@freescale.com
 Cc: Alexander Graf ag...@suse.de
 ---
 v2:
  - use CONFIG_PPC_E500MC without CONFIG_E500
  - use elif defined()
 
  arch/powerpc/kernel/cpu_setup_fsl_booke.S | 12 +++-
  arch/powerpc/kernel/cputable.c|  5 +
  arch/powerpc/kernel/head_fsl_booke.S  | 18 +-
  arch/powerpc/platforms/Kconfig.cputype|  6 +-
  4 files changed, 34 insertions(+), 7 deletions(-)

Acked-by: Scott Wood scottw...@freescale.com

-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 v2 2/2] powerpc/booke: Revert SPE/AltiVec common defines for interrupt numbers

[Scott Wood]

On Wed, 2014-08-20 at 16:09 +0300, Mihai Caraman wrote:
 Book3E specification defines shared interrupt numbers for SPE and AltiVec
 units. Still SPE is present in e200/e500v2 cores while AltiVec is present in
 e6500 core. So we can currently decide at compile-time which unit to support
 exclusively. As Alexander Graf suggested, this will improve code readability
 especially in KVM.
 
 Use distinct defines to identify SPE/AltiVec interrupt numbers, reverting
 c58ce397 and 6b310fc5 patches that added common defines.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 Cc: Scott Wood scottw...@freescale.com
 Cc: Alexander Graf ag...@suse.de
 ---
  arch/powerpc/kernel/exceptions-64e.S | 4 ++--
  arch/powerpc/kernel/head_fsl_booke.S | 8 
  2 files changed, 6 insertions(+), 6 deletions(-)

Acked-by: Scott Wood scottw...@freescale.com

-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 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Tue, 2014-08-12 at 02:36 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, August 12, 2014 5:30 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and
  exception
  
  On Wed, 2014-08-06 at 12:08 +0530, Bharat Bhushan wrote:
   @@ -1249,6 +1284,7 @@ int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
 setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
 (unsigned long)vcpu);
  
   + kvmppc_clear_dbsr();
 return 0;
  
  This could use a comment for why we're doing this.  Also, I'm a bit uneasy 
  about
  clearing the whole DBSR here, where we haven't yet switched the debug 
  registers
  to guest context.
 
 I think we wanted MRR to not cause debug event to guest, So should we only 
 clear MRR ?
 
  It shouldn't actually matter except for deferred debug
  exceptions which are not actually useful (in fact e6500 removed support for
  them),
 
 Exactly, that's why I was clearing complete DBSR. Probably we can have a 
 comment
  Do not let previously set debug events visible to guest. As deferred debug 
 events
   are not supported, so it is ok to clear complete DBSR.
  

This would be affecting host debugging of the host, not guest debugging
of the guest.  Still I don't think it's a huge deal, but clearing only
MRR would be cleaner.

-Scott


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

Re: [PATCH 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Tue, 2014-08-12 at 02:36 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, August 12, 2014 5:30 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and
  exception
  
  On Wed, 2014-08-06 at 12:08 +0530, Bharat Bhushan wrote:
   @@ -1249,6 +1284,7 @@ int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
 setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
 (unsigned long)vcpu);
  
   + kvmppc_clear_dbsr();
 return 0;
  
  This could use a comment for why we're doing this.  Also, I'm a bit uneasy 
  about
  clearing the whole DBSR here, where we haven't yet switched the debug 
  registers
  to guest context.
 
 I think we wanted MRR to not cause debug event to guest, So should we only 
 clear MRR ?
 
  It shouldn't actually matter except for deferred debug
  exceptions which are not actually useful (in fact e6500 removed support for
  them),
 
 Exactly, that's why I was clearing complete DBSR. Probably we can have a 
 comment
  Do not let previously set debug events visible to guest. As deferred debug 
 events
   are not supported, so it is ok to clear complete DBSR.
  

This would be affecting host debugging of the host, not guest debugging
of the guest.  Still I don't think it's a huge deal, but clearing only
MRR would be cleaner.

-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] KVM: PPC: e500mc: Add support for single threaded vcpus on e6500 core

[Scott Wood]

On Wed, 2014-08-06 at 19:33 +0300, Mihai Caraman wrote:
 @@ -390,19 +400,30 @@ static void kvmppc_core_vcpu_free_e500mc(struct 
 kvm_vcpu *vcpu)
  
  static int kvmppc_core_init_vm_e500mc(struct kvm *kvm)
  {
 - int lpid;
 + int i, lpid;
  
 - lpid = kvmppc_alloc_lpid();
 - if (lpid  0)
 - return lpid;
 + /* The lpid pool supports only 2 entries now */
 + if (threads_per_core  2)
 + return -ENOMEM;
 +
 + /* Each VM allocates one LPID per HW thread index */
 + for (i = 0; i  threads_per_core; i++) {
 + lpid = kvmppc_alloc_lpid();
 + if (lpid  0)
 + return lpid;
 +
 + kvm-arch.lpid_pool[i] = lpid;
 + }

Wouldn't it be simpler to halve the size of the lpid pool that the
allocator sees, and just OR in the high bit based on the low bit of the
cpu number?

-Scott


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

Re: [PATCH] KVM: PPC: e500mc: Add support for single threaded vcpus on e6500 core

[Scott Wood]

On Tue, 2014-08-12 at 01:53 +0200, Alexander Graf wrote:
 
  Am 12.08.2014 um 01:36 schrieb Scott Wood scottw...@freescale.com:
  
  On Wed, 2014-08-06 at 19:33 +0300, Mihai Caraman wrote:
  @@ -390,19 +400,30 @@ static void kvmppc_core_vcpu_free_e500mc(struct 
  kvm_vcpu *vcpu)
  
  static int kvmppc_core_init_vm_e500mc(struct kvm *kvm)
  {
  -int lpid;
  +int i, lpid;
  
  -lpid = kvmppc_alloc_lpid();
  -if (lpid  0)
  -return lpid;
  +/* The lpid pool supports only 2 entries now */
  +if (threads_per_core  2)
  +return -ENOMEM;
  +
  +/* Each VM allocates one LPID per HW thread index */
  +for (i = 0; i  threads_per_core; i++) {
  +lpid = kvmppc_alloc_lpid();
  +if (lpid  0)
  +return lpid;
  +
  +kvm-arch.lpid_pool[i] = lpid;
  +}
  
  Wouldn't it be simpler to halve the size of the lpid pool that the
  allocator sees, and just OR in the high bit based on the low bit of the
  cpu number?
 
 Heh, I wrote the same and then removed the section from my reply again. It 
 wouldn't really make that much of a difference if you think it through 
 completely.
 
 But yes, it certainly would be quite a bit more natural. I'm ok either way.

It's not a huge difference, but it would at least get rid of some of the
ifdeffing in the headers.  It'd also be nicer when debugging to have the
LPIDs correlated.

-Scott


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

Re: [PATCH 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-08-06 at 12:08 +0530, Bharat Bhushan wrote:
 @@ -1249,6 +1284,7 @@ int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
   (unsigned long)vcpu);
  
 + kvmppc_clear_dbsr();
   return 0;

This could use a comment for why we're doing this.  Also, I'm a bit
uneasy about clearing the whole DBSR here, where we haven't yet switched
the debug registers to guest context.  It shouldn't actually matter
except for deferred debug exceptions which are not actually useful (in
fact e6500 removed support for them), but still...

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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] powerpc/booke: Restrict SPE exception handlers to e200/e500 cores

[Scott Wood]

On Wed, 2014-08-06 at 11:39 +0300, Mihai Caraman wrote:
 SPE exception handlers are now defined for 32-bit e500mc cores even though
 SPE unit is not present and CONFIG_SPE is undefined.
 
 Restrict SPE exception handlers to e200/e500 cores adding CONFIG_SPE_POSSIBLE
 and consequently guard __stup_ivors and __setup_cpu functions.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 ---
  arch/powerpc/kernel/cpu_setup_fsl_booke.S | 12 +++-
  arch/powerpc/kernel/cputable.c|  5 +
  arch/powerpc/kernel/head_fsl_booke.S  | 18 +-
  arch/powerpc/platforms/Kconfig.cputype|  6 +-
  4 files changed, 34 insertions(+), 7 deletions(-)
 
 diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S 
 b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
 index 4f1393d..44bb2c9 100644
 --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
 +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
 @@ -91,6 +91,7 @@ _GLOBAL(setup_altivec_idle)
  
   blr
  
 +#if defined(CONFIG_E500)  defined(CONFIG_PPC_E500MC)

When would you ever have CONFIG_PPC_E500MC without CONFIG_E500?

 diff --git a/arch/powerpc/kernel/head_fsl_booke.S 
 b/arch/powerpc/kernel/head_fsl_booke.S
 index b497188..4f8930f 100644
 --- a/arch/powerpc/kernel/head_fsl_booke.S
 +++ b/arch/powerpc/kernel/head_fsl_booke.S
 @@ -613,6 +613,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
   mfspr   r10, SPRN_SPRG_RSCRATCH0
   b   InstructionStorage
  
 +/* Define SPE handlers for e200 and e500v2 */
  #ifdef CONFIG_SPE
   /* SPE Unavailable */
   START_EXCEPTION(SPEUnavailable)
 @@ -622,10 +623,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
   b   fast_exception_return
  1:   addir3,r1,STACK_FRAME_OVERHEAD
   EXC_XFER_EE_LITE(0x2010, KernelSPE)
 -#else
 +#elif CONFIG_SPE_POSSIBLE

#elif defined(CONFIG_SPE_POSSIBLE)

Likewise elsewhere

-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] KVM: PPC: e500mc: Add support for single threaded vcpus on e6500 core

[Scott Wood]

On Wed, 2014-08-06 at 19:33 +0300, Mihai Caraman wrote:
 @@ -390,19 +400,30 @@ static void kvmppc_core_vcpu_free_e500mc(struct 
 kvm_vcpu *vcpu)
  
  static int kvmppc_core_init_vm_e500mc(struct kvm *kvm)
  {
 - int lpid;
 + int i, lpid;
  
 - lpid = kvmppc_alloc_lpid();
 - if (lpid  0)
 - return lpid;
 + /* The lpid pool supports only 2 entries now */
 + if (threads_per_core  2)
 + return -ENOMEM;
 +
 + /* Each VM allocates one LPID per HW thread index */
 + for (i = 0; i  threads_per_core; i++) {
 + lpid = kvmppc_alloc_lpid();
 + if (lpid  0)
 + return lpid;
 +
 + kvm-arch.lpid_pool[i] = lpid;
 + }

Wouldn't it be simpler to halve the size of the lpid pool that the
allocator sees, and just OR in the high bit based on the low bit of the
cpu number?

-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] KVM: PPC: e500mc: Add support for single threaded vcpus on e6500 core

[Scott Wood]

On Tue, 2014-08-12 at 01:53 +0200, Alexander Graf wrote:
 
  Am 12.08.2014 um 01:36 schrieb Scott Wood scottw...@freescale.com:
  
  On Wed, 2014-08-06 at 19:33 +0300, Mihai Caraman wrote:
  @@ -390,19 +400,30 @@ static void kvmppc_core_vcpu_free_e500mc(struct 
  kvm_vcpu *vcpu)
  
  static int kvmppc_core_init_vm_e500mc(struct kvm *kvm)
  {
  -int lpid;
  +int i, lpid;
  
  -lpid = kvmppc_alloc_lpid();
  -if (lpid  0)
  -return lpid;
  +/* The lpid pool supports only 2 entries now */
  +if (threads_per_core  2)
  +return -ENOMEM;
  +
  +/* Each VM allocates one LPID per HW thread index */
  +for (i = 0; i  threads_per_core; i++) {
  +lpid = kvmppc_alloc_lpid();
  +if (lpid  0)
  +return lpid;
  +
  +kvm-arch.lpid_pool[i] = lpid;
  +}
  
  Wouldn't it be simpler to halve the size of the lpid pool that the
  allocator sees, and just OR in the high bit based on the low bit of the
  cpu number?
 
 Heh, I wrote the same and then removed the section from my reply again. It 
 wouldn't really make that much of a difference if you think it through 
 completely.
 
 But yes, it certainly would be quite a bit more natural. I'm ok either way.

It's not a huge difference, but it would at least get rid of some of the
ifdeffing in the headers.  It'd also be nicer when debugging to have the
LPIDs correlated.

-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 7/7 v3] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-08-06 at 12:08 +0530, Bharat Bhushan wrote:
 @@ -1249,6 +1284,7 @@ int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
   (unsigned long)vcpu);
  
 + kvmppc_clear_dbsr();
   return 0;

This could use a comment for why we're doing this.  Also, I'm a bit
uneasy about clearing the whole DBSR here, where we haven't yet switched
the debug registers to guest context.  It shouldn't actually matter
except for deferred debug exceptions which are not actually useful (in
fact e6500 removed support for them), but still...

-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 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-08-04 at 22:41 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, August 05, 2014 4:23 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and
  exception
  
  On Mon, 2014-08-04 at 13:32 +0530, Bharat Bhushan wrote:
   @@ -735,7 +745,27 @@ static int kvmppc_handle_debug(struct kvm_run *run,
  struct kvm_vcpu *vcpu)
 struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
 u32 dbsr = vcpu-arch.dbsr;
  
   - /* Clear guest dbsr (vcpu-arch.dbsr).
   + if (vcpu-guest_debug == 0) {
   + /*
   +  * Debug resources belong to Guest.
   +  * Imprecise debug event are not injected
   +  */
   + if (dbsr  DBSR_IDE)
   + return RESUME_GUEST;
  
  This is incorrect.  DBSR_IDE shouldn't *cause* an injection, but it 
  shouldn't
  inhibit it either.
 
 Will this work ?
   If ((dbsr  DBSR_IDE)  !(dbsr  ~DBSR_IDE))
   Return RESUME_GUEST; 

I suppose it could, but it would be cleaner to just change dbsr to
(dbsr  ~DBSR_IDE) in the next if-statement (maybe factoring out each
 term of that if-statement to variables to make it more readable).

   @@ -828,6 +858,8 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu
  *vcpu,
 case BOOKE_INTERRUPT_DEBUG:
 /* Save DBSR before preemption is enabled */
 vcpu-arch.dbsr = mfspr(SPRN_DBSR);
   + /* MASK out DBSR_MRR */
   + vcpu-arch.dbsr = ~DBSR_MRR;
 kvmppc_clear_dbsr();
 break;
 }
  
  DBSR[MRR] can only be set once per host system reset.  There's no need to 
  filter
  it out here; just make sure the host clears it at some point before this 
  point.
 
 Can you please suggest where ? somewhere in KVM initialization ?

Sure, KVM init works given that there's no real reason for non-KVM code
to care.

  The MRR value doesn't currently survive past kvmppc_clear_dbsr(), so this 
  isn't
  helping to preserve it for the host's benefit...
  
   @@ -1858,6 +1890,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
   kvm_vcpu *vcpu,
  
 if (!(dbg-control  KVM_GUESTDBG_ENABLE)) {
 vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
   + vcpu-arch.dbg_reg.dbcr0 = 0;
  
  Again, it's not clear why we need shadow debug registers here.  Just in 
  case we
  implement something that can't be implemented isn't a good reason to keep
  complexity around.
 
 One reason was that setting EDM in guest visible register, For this we
 need shadow_reg is used to save/restore state in h/w register (which
 does not have DBCR0_EDM) but debug_reg have DBCR0_EDM.

If that's the only reason, then I'd get rid of the shadow and just OR in
DCBR0_EDM when reading the register, if vcpu-guest_debug is nonzero.

-Scott


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

Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-08-04 at 22:41 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, August 05, 2014 4:23 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and
  exception
  
  On Mon, 2014-08-04 at 13:32 +0530, Bharat Bhushan wrote:
   @@ -735,7 +745,27 @@ static int kvmppc_handle_debug(struct kvm_run *run,
  struct kvm_vcpu *vcpu)
 struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
 u32 dbsr = vcpu-arch.dbsr;
  
   - /* Clear guest dbsr (vcpu-arch.dbsr).
   + if (vcpu-guest_debug == 0) {
   + /*
   +  * Debug resources belong to Guest.
   +  * Imprecise debug event are not injected
   +  */
   + if (dbsr  DBSR_IDE)
   + return RESUME_GUEST;
  
  This is incorrect.  DBSR_IDE shouldn't *cause* an injection, but it 
  shouldn't
  inhibit it either.
 
 Will this work ?
   If ((dbsr  DBSR_IDE)  !(dbsr  ~DBSR_IDE))
   Return RESUME_GUEST; 

I suppose it could, but it would be cleaner to just change dbsr to
(dbsr  ~DBSR_IDE) in the next if-statement (maybe factoring out each
 term of that if-statement to variables to make it more readable).

   @@ -828,6 +858,8 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu
  *vcpu,
 case BOOKE_INTERRUPT_DEBUG:
 /* Save DBSR before preemption is enabled */
 vcpu-arch.dbsr = mfspr(SPRN_DBSR);
   + /* MASK out DBSR_MRR */
   + vcpu-arch.dbsr = ~DBSR_MRR;
 kvmppc_clear_dbsr();
 break;
 }
  
  DBSR[MRR] can only be set once per host system reset.  There's no need to 
  filter
  it out here; just make sure the host clears it at some point before this 
  point.
 
 Can you please suggest where ? somewhere in KVM initialization ?

Sure, KVM init works given that there's no real reason for non-KVM code
to care.

  The MRR value doesn't currently survive past kvmppc_clear_dbsr(), so this 
  isn't
  helping to preserve it for the host's benefit...
  
   @@ -1858,6 +1890,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
   kvm_vcpu *vcpu,
  
 if (!(dbg-control  KVM_GUESTDBG_ENABLE)) {
 vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
   + vcpu-arch.dbg_reg.dbcr0 = 0;
  
  Again, it's not clear why we need shadow debug registers here.  Just in 
  case we
  implement something that can't be implemented isn't a good reason to keep
  complexity around.
 
 One reason was that setting EDM in guest visible register, For this we
 need shadow_reg is used to save/restore state in h/w register (which
 does not have DBCR0_EDM) but debug_reg have DBCR0_EDM.

If that's the only reason, then I'd get rid of the shadow and just OR in
DCBR0_EDM when reading the register, if vcpu-guest_debug is nonzero.

-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 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit KVM_EXIT_DEBUG

[Scott Wood]

On Mon, 2014-08-04 at 13:22 +0530, Bharat Bhushan wrote:
 Dbsr is not visible to userspace and we do not think any need to
 expose this to userspace because:
   Userspace cannot inject debug interrupt to guest (as this
   does not know guest ability to handle debug interrupt), so
   userspace will always clear DBSR.
   Now if userspace has to always clear DBSR in KVM_EXIT_DEBUG
   handling then clearing dbsr in kernel looks simple as this
   avoid doing SET_SREGS/set_one_reg() to clear DBSR
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke.c | 11 +++
  1 file changed, 11 insertions(+)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index 322da7d..5c2e26a 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -735,6 +735,17 @@ static int kvmppc_handle_debug(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
   u32 dbsr = vcpu-arch.dbsr;
  
 + /* Clear guest dbsr (vcpu-arch.dbsr).
 +  * dbsr is not visible to userspace and we do not think any
 +  * need to expose this to userspace because:
 +  * Userspace cannot inject debug interrupt to guest (as this does
 +  * not know guest ability to handle debug interrupt), so userspace
 +  * will always clear DBSR.
 +  * Now if userspace has to always clear DBSR in KVM_EXIT_DEBUG
 +  * handling then clearing here looks simple as this
 +  * avoid doing SET_SREGS/set_one_reg() to clear DBSR
 +  */
 + vcpu-arch.dbsr = 0;
   run-debug.arch.status = 0;
   run-debug.arch.address = vcpu-arch.pc;
  

I think the changelog is adequate -- I don't think we need to be so
verbose in the code itself.  The question was just whether this was a
userspace-visible change, and it isn't.

FWIW, I think dbsr should be visible to userspace in general (regardless
of whether it's cleared here), because all guest registers should be
visible to userspace.  I may be debugging a guest through means that
don't require owning debug resources, such as stopping and inspecting a
guest that has hung or crashed.

-Scott


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

Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-08-04 at 13:32 +0530, Bharat Bhushan wrote:
 @@ -735,7 +745,27 @@ static int kvmppc_handle_debug(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
   u32 dbsr = vcpu-arch.dbsr;
  
 - /* Clear guest dbsr (vcpu-arch.dbsr).
 + if (vcpu-guest_debug == 0) {
 + /*
 +  * Debug resources belong to Guest.
 +  * Imprecise debug event are not injected
 +  */
 + if (dbsr  DBSR_IDE)
 + return RESUME_GUEST;

This is incorrect.  DBSR_IDE shouldn't *cause* an injection, but it
shouldn't inhibit it either.

 @@ -828,6 +858,8 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu 
 *vcpu,
   case BOOKE_INTERRUPT_DEBUG:
   /* Save DBSR before preemption is enabled */
   vcpu-arch.dbsr = mfspr(SPRN_DBSR);
 + /* MASK out DBSR_MRR */
 + vcpu-arch.dbsr = ~DBSR_MRR;
   kvmppc_clear_dbsr();
   break;
   }

DBSR[MRR] can only be set once per host system reset.  There's no need
to filter it out here; just make sure the host clears it at some point
before this point.  The MRR value doesn't currently survive past
kvmppc_clear_dbsr(), so this isn't helping to preserve it for the host's
benefit...

 @@ -1858,6 +1890,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu 
 *vcpu,
  
   if (!(dbg-control  KVM_GUESTDBG_ENABLE)) {
   vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
 + vcpu-arch.dbg_reg.dbcr0 = 0;

Again, it's not clear why we need shadow debug registers here.  Just in
case we implement something that can't be implemented isn't a good
reason to keep complexity around.

-Scott


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

Re: [PATCH 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit KVM_EXIT_DEBUG

[Scott Wood]

On Mon, 2014-08-04 at 22:33 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, August 05, 2014 4:17 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit
  KVM_EXIT_DEBUG
  
  FWIW, I think dbsr should be visible to userspace in general (regardless of
  whether it's cleared here), because all guest registers should be visible to
  userspace.  I may be debugging a guest through means that don't require 
  owning
  debug resources, such as stopping and inspecting a guest that has hung or
  crashed.
 
 Do you mean that we should also add a one-reg interface for DBSR ?

Yes.

-Scott


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

Re: [PATCH 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit KVM_EXIT_DEBUG

[Scott Wood]

On Mon, 2014-08-04 at 13:22 +0530, Bharat Bhushan wrote:
 Dbsr is not visible to userspace and we do not think any need to
 expose this to userspace because:
   Userspace cannot inject debug interrupt to guest (as this
   does not know guest ability to handle debug interrupt), so
   userspace will always clear DBSR.
   Now if userspace has to always clear DBSR in KVM_EXIT_DEBUG
   handling then clearing dbsr in kernel looks simple as this
   avoid doing SET_SREGS/set_one_reg() to clear DBSR
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke.c | 11 +++
  1 file changed, 11 insertions(+)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index 322da7d..5c2e26a 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -735,6 +735,17 @@ static int kvmppc_handle_debug(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
   u32 dbsr = vcpu-arch.dbsr;
  
 + /* Clear guest dbsr (vcpu-arch.dbsr).
 +  * dbsr is not visible to userspace and we do not think any
 +  * need to expose this to userspace because:
 +  * Userspace cannot inject debug interrupt to guest (as this does
 +  * not know guest ability to handle debug interrupt), so userspace
 +  * will always clear DBSR.
 +  * Now if userspace has to always clear DBSR in KVM_EXIT_DEBUG
 +  * handling then clearing here looks simple as this
 +  * avoid doing SET_SREGS/set_one_reg() to clear DBSR
 +  */
 + vcpu-arch.dbsr = 0;
   run-debug.arch.status = 0;
   run-debug.arch.address = vcpu-arch.pc;
  

I think the changelog is adequate -- I don't think we need to be so
verbose in the code itself.  The question was just whether this was a
userspace-visible change, and it isn't.

FWIW, I think dbsr should be visible to userspace in general (regardless
of whether it's cleared here), because all guest registers should be
visible to userspace.  I may be debugging a guest through means that
don't require owning debug resources, such as stopping and inspecting a
guest that has hung or crashed.

-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 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-08-04 at 13:32 +0530, Bharat Bhushan wrote:
 @@ -735,7 +745,27 @@ static int kvmppc_handle_debug(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
   u32 dbsr = vcpu-arch.dbsr;
  
 - /* Clear guest dbsr (vcpu-arch.dbsr).
 + if (vcpu-guest_debug == 0) {
 + /*
 +  * Debug resources belong to Guest.
 +  * Imprecise debug event are not injected
 +  */
 + if (dbsr  DBSR_IDE)
 + return RESUME_GUEST;

This is incorrect.  DBSR_IDE shouldn't *cause* an injection, but it
shouldn't inhibit it either.

 @@ -828,6 +858,8 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu 
 *vcpu,
   case BOOKE_INTERRUPT_DEBUG:
   /* Save DBSR before preemption is enabled */
   vcpu-arch.dbsr = mfspr(SPRN_DBSR);
 + /* MASK out DBSR_MRR */
 + vcpu-arch.dbsr = ~DBSR_MRR;
   kvmppc_clear_dbsr();
   break;
   }

DBSR[MRR] can only be set once per host system reset.  There's no need
to filter it out here; just make sure the host clears it at some point
before this point.  The MRR value doesn't currently survive past
kvmppc_clear_dbsr(), so this isn't helping to preserve it for the host's
benefit...

 @@ -1858,6 +1890,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu 
 *vcpu,
  
   if (!(dbg-control  KVM_GUESTDBG_ENABLE)) {
   vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
 + vcpu-arch.dbg_reg.dbcr0 = 0;

Again, it's not clear why we need shadow debug registers here.  Just in
case we implement something that can't be implemented isn't a good
reason to keep complexity around.

-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 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit KVM_EXIT_DEBUG

[Scott Wood]

On Mon, 2014-08-04 at 22:33 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, August 05, 2014 4:17 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 4/5] KVM: PPC: BOOKE: Clear guest dbsr in userspace exit
  KVM_EXIT_DEBUG
  
  FWIW, I think dbsr should be visible to userspace in general (regardless of
  whether it's cleared here), because all guest registers should be visible to
  userspace.  I may be debugging a guest through means that don't require 
  owning
  debug resources, such as stopping and inspecting a guest that has hung or
  crashed.
 
 Do you mean that we should also add a one-reg interface for DBSR ?

Yes.

-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 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Fri, 2014-08-01 at 04:34 -0500, Bhushan Bharat-R65777 wrote:
 on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set.
 
 case SPRN_DBSR:
 
 vcpu-arch.dbsr = ~spr_val;
 if (!(vcpu-arch.dbsr  ~DBSR_IDE))
 kvmppc_core_dequeue_debug(vcpu);
 break;
 
 or
 vcpu-arch.dbsr = ~(spr_val | DBSR_IDE);
 if (!vcpu-arch.dbsr)
 kvmppc_core_dequeue_debug(vcpu);
 break;

The first option.  I see no reason to have KVM forcibly clear DBSR[IDE].

-Scott


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

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Fri, 2014-08-01 at 04:34 -0500, Bhushan Bharat-R65777 wrote:
 on dbsr write emulation, deque the debug interrupt even if DBSR_IDE is set.
 
 case SPRN_DBSR:
 
 vcpu-arch.dbsr = ~spr_val;
 if (!(vcpu-arch.dbsr  ~DBSR_IDE))
 kvmppc_core_dequeue_debug(vcpu);
 break;
 
 or
 vcpu-arch.dbsr = ~(spr_val | DBSR_IDE);
 if (!vcpu-arch.dbsr)
 kvmppc_core_dequeue_debug(vcpu);
 break;

The first option.  I see no reason to have KVM forcibly clear DBSR[IDE].

-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 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Thursday, July 31, 2014 8:18 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and 
  exception
  
  On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
  
-Original Message-
From: Wood Scott-B07421
Sent: Tuesday, July 29, 2014 3:58 AM
To: Bhushan Bharat-R65777
Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org;
Yoder Stuart-
B08248
Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
and exception
   
 Userspace might be interested in
the raw value,
  
   With the current design, If userspace is interested then it will not
   get the DBSR.
  
  Oh, because DBSR isn't currently implemented in sregs or one reg?
 
 That is one reason. Another is that if we give dbsr visibility to
 userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG.

Right -- since I didn't realize DBSR wasn't already exposed, I thought
userspace already had this responsibility.

  It looked like it was removing dbsr visibility and the requirement for 
  userspace
  to clear dbsr.  I guess the old way was that the value in
  vcpu-arch.dbsr didn't matter until the next debug exception, when it
  would be overwritten by the new SPRN_DBSR?
 
 But that means old dbsr will be visibility to userspace, which is even bad 
 than not visible, no?
 
 Also this can lead to old dbsr visible to guest once userspace releases
 debug resources, but this can be solved by clearing dbsr in
 kvm_arch_vcpu_ioctl_set_guest_debug() -  if (!(dbg-control 
 KVM_GUESTDBG_ENABLE)) { }.

I wasn't suggesting that you keep it that way, just clarifying my
understanding of the current code.


 
  
 + case SPRN_DBCR2:
 + /*
 +  * If userspace is debugging guest then guest
 +  * can not access debug registers.
 +  */
 + if (vcpu-guest_debug)
 + break;
 +
 + debug_inst = true;
 + vcpu-arch.dbg_reg.dbcr2 = spr_val;
 + vcpu-arch.shadow_dbg_reg.dbcr2 = spr_val;
   break;
   
In what circumstances can the architected and shadow registers differ?
  
   As of now they are same. But I think that if we want to implement other
  features like Freeze Timer (FT) then they can be different.
  
  I don't think we can possibly implement Freeze Timer.
 
 May be, but in my opinion we should keep this open.

We're not talking about API here -- the implementation should be kept
simple if there's no imminent need for shadow registers.

   I am not sure what we should in that case ?
  
   As we are currently emulating a subset of debug events (IAC, DAC, IC,
   BT and TIE --- DBCR0 emulation) then we should expose status of those
   events in guest dbsr and rest should be cleared ?
  
  I'm not saying they need to be exposed to the guest, but I don't see where 
  you
  filter out bits like these.
 
 I am trying to get what all bits should be filtered out, all bits
 except IACx, DACx, IC, BT and TIE (same as event set filtering done
 when setting DBCR0) ? 
 
 i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?

Bits like IRPT and RET don't really matter, as you shouldn't see them
happen.  Likewise MRR if you're sure you've cleared it since boot.  But
IDE could be set any time an asynchronous exception happens.  I don't
think you should filter it out, but instead make sure that it doesn't
cause an exception to be delivered.

-Scott


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

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Thu, 2014-07-31 at 01:15 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Thursday, July 31, 2014 8:18 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and 
  exception
  
  On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
  
-Original Message-
From: Wood Scott-B07421
Sent: Tuesday, July 29, 2014 3:58 AM
To: Bhushan Bharat-R65777
Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
Yoder Stuart-
B08248
Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers
and exception
   
 Userspace might be interested in
the raw value,
  
   With the current design, If userspace is interested then it will not
   get the DBSR.
  
  Oh, because DBSR isn't currently implemented in sregs or one reg?
 
 That is one reason. Another is that if we give dbsr visibility to
 userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG.

Right -- since I didn't realize DBSR wasn't already exposed, I thought
userspace already had this responsibility.

  It looked like it was removing dbsr visibility and the requirement for 
  userspace
  to clear dbsr.  I guess the old way was that the value in
  vcpu-arch.dbsr didn't matter until the next debug exception, when it
  would be overwritten by the new SPRN_DBSR?
 
 But that means old dbsr will be visibility to userspace, which is even bad 
 than not visible, no?
 
 Also this can lead to old dbsr visible to guest once userspace releases
 debug resources, but this can be solved by clearing dbsr in
 kvm_arch_vcpu_ioctl_set_guest_debug() -  if (!(dbg-control 
 KVM_GUESTDBG_ENABLE)) { }.

I wasn't suggesting that you keep it that way, just clarifying my
understanding of the current code.


 
  
 + case SPRN_DBCR2:
 + /*
 +  * If userspace is debugging guest then guest
 +  * can not access debug registers.
 +  */
 + if (vcpu-guest_debug)
 + break;
 +
 + debug_inst = true;
 + vcpu-arch.dbg_reg.dbcr2 = spr_val;
 + vcpu-arch.shadow_dbg_reg.dbcr2 = spr_val;
   break;
   
In what circumstances can the architected and shadow registers differ?
  
   As of now they are same. But I think that if we want to implement other
  features like Freeze Timer (FT) then they can be different.
  
  I don't think we can possibly implement Freeze Timer.
 
 May be, but in my opinion we should keep this open.

We're not talking about API here -- the implementation should be kept
simple if there's no imminent need for shadow registers.

   I am not sure what we should in that case ?
  
   As we are currently emulating a subset of debug events (IAC, DAC, IC,
   BT and TIE --- DBCR0 emulation) then we should expose status of those
   events in guest dbsr and rest should be cleared ?
  
  I'm not saying they need to be exposed to the guest, but I don't see where 
  you
  filter out bits like these.
 
 I am trying to get what all bits should be filtered out, all bits
 except IACx, DACx, IC, BT and TIE (same as event set filtering done
 when setting DBCR0) ? 
 
 i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?

Bits like IRPT and RET don't really matter, as you shouldn't see them
happen.  Likewise MRR if you're sure you've cleared it since boot.  But
IDE could be set any time an asynchronous exception happens.  I don't
think you should filter it out, but instead make sure that it doesn't
cause an exception to be delivered.

-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/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, July 29, 2014 3:22 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
  visible register
  
  On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
   This is not used and  even I do not remember why this was added in
   first place.
  
   Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
   ---
arch/powerpc/kvm/booke.c | 2 --
1 file changed, 2 deletions(-)
  
   diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
   ab62109..a5ee42c 100644
   --- a/arch/powerpc/kvm/booke.c
   +++ b/arch/powerpc/kvm/booke.c
   @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct 
   kvm_vcpu
  *vcpu,
 kvm_guest_protect_msr(vcpu, MSR_DE, true);
 vcpu-guest_debug = dbg-control;
 vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
   - /* Set DBCR0_EDM in guest visible DBCR0 register. */
   - vcpu-arch.dbg_reg.dbcr0 = DBCR0_EDM;
  
 if (vcpu-guest_debug  KVM_GUESTDBG_SINGLESTEP)
 vcpu-arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
  
  This was intended to let the guest know that the host owns the debug 
  resources,
  by analogy to what a JTAG debugger would do.
  
  The Power ISA has this Virtualized Implementation Note:
  
  It is the responsibility of the hypervisor to ensure that
  DBCR0[EDM] is consistent with usage of DEP.
 
 Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if MSRP_DEP is 
 clear then clear DBCR0_EDM, right?
 We need to implement above mentioned this.

We should probably clear EDM only when guest debug emulation is working
and enabled (i.e. not until at least patch 6/6).

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Wed, 2014-07-30 at 12:57 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Wednesday, July 30, 2014 11:18 PM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
  visible register
  
  On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
  
-Original Message-
From: Wood Scott-B07421
Sent: Tuesday, July 29, 2014 3:22 AM
To: Bhushan Bharat-R65777
Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org;
Yoder Stuart-
B08248
Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM
in guest visible register
   
On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
 This is not used and  even I do not remember why this was added in
 first place.

 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke.c | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index ab62109..a5ee42c 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -1804,8 +1804,6 @@ int
 kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
*vcpu,
   kvm_guest_protect_msr(vcpu, MSR_DE, true);
   vcpu-guest_debug = dbg-control;
   vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
 - /* Set DBCR0_EDM in guest visible DBCR0 register. */
 - vcpu-arch.dbg_reg.dbcr0 = DBCR0_EDM;

   if (vcpu-guest_debug  KVM_GUESTDBG_SINGLESTEP)
   vcpu-arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
   
This was intended to let the guest know that the host owns the debug
resources, by analogy to what a JTAG debugger would do.
   
The Power ISA has this Virtualized Implementation Note:
   
It is the responsibility of the hypervisor to ensure that
DBCR0[EDM] is consistent with usage of DEP.
  
   Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if 
   MSRP_DEP is
  clear then clear DBCR0_EDM, right?
   We need to implement above mentioned this.
  
  We should probably clear EDM only when guest debug emulation is working and
  enabled (i.e. not until at least patch 6/6).
 
 But if EDM is set then guest debug emulation will not start/allowed.

I don't mean after the guest tries to write to the registers -- I mean
after the code has been added to KVM to allow it to work.

-Scott


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

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, July 29, 2014 3:58 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-...@vger.kernel.org; kvm@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and 
  exception
  
   Userspace might be interested in
  the raw value,
 
 With the current design, If userspace is interested then it will not
 get the DBSR.

Oh, because DBSR isn't currently implemented in sregs or one reg?

  But why userspace will be interested?

Do you expose all of the hardware's debugging features in your
high-level interface?

  plus it's a change from the current API semantics.
 
 Can you please let us know how ?

It looked like it was removing dbsr visibility and the requirement for
userspace to clear dbsr.  I guess the old way was that the value in
vcpu-arch.dbsr didn't matter until the next debug exception, when it
would be overwritten by the new SPRN_DBSR?

   + case SPRN_DBCR2:
   + /*
   +  * If userspace is debugging guest then guest
   +  * can not access debug registers.
   +  */
   + if (vcpu-guest_debug)
   + break;
   +
   + debug_inst = true;
   + vcpu-arch.dbg_reg.dbcr2 = spr_val;
   + vcpu-arch.shadow_dbg_reg.dbcr2 = spr_val;
 break;
  
  In what circumstances can the architected and shadow registers differ?
 
 As of now they are same. But I think that if we want to implement other 
 features like Freeze Timer (FT) then they can be different.

I don't think we can possibly implement Freeze Timer.
 
 case SPRN_DBSR:
   + /*
   +  * If userspace is debugging guest then guest
   +  * can not access debug registers.
   +  */
   + if (vcpu-guest_debug)
   + break;
   +
 vcpu-arch.dbsr = ~spr_val;
   + if (vcpu-arch.dbsr == 0)
   + kvmppc_core_dequeue_debug(vcpu);
 break;
  
  Not all DBSR bits cause an exception, e.g. IDE and MRR.
 
 I am not sure what we should in that case ?

 As we are currently emulating a subset of debug events (IAC, DAC, IC,
 BT and TIE --- DBCR0 emulation) then we should expose status of those
 events in guest dbsr and rest should be cleared ?

I'm not saying they need to be exposed to the guest, but I don't see
where you filter out bits like these.

   @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu 
   *vcpu, int
  sprn, ulong spr_val)
 emulated = EMULATE_FAIL;
 }
  
   + if (debug_inst) {
   + switch_booke_debug_regs(vcpu-arch.shadow_dbg_reg);
   + current-thread.debug = vcpu-arch.shadow_dbg_reg;
   + }
  
  Could you explain what's going on with regard to copying the registers
  into current-thread.debug?  Why is it done after loading the registers
  into the hardware (is there a race if we get preempted in the middle)?
 
 Yes, and this was something I was not clear when writing this code.
 Should we have preempt disable-enable around this.

Can they be reordered instead?

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, July 29, 2014 3:22 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
  visible register
  
  On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
   This is not used and  even I do not remember why this was added in
   first place.
  
   Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
   ---
arch/powerpc/kvm/booke.c | 2 --
1 file changed, 2 deletions(-)
  
   diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
   ab62109..a5ee42c 100644
   --- a/arch/powerpc/kvm/booke.c
   +++ b/arch/powerpc/kvm/booke.c
   @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct 
   kvm_vcpu
  *vcpu,
 kvm_guest_protect_msr(vcpu, MSR_DE, true);
 vcpu-guest_debug = dbg-control;
 vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
   - /* Set DBCR0_EDM in guest visible DBCR0 register. */
   - vcpu-arch.dbg_reg.dbcr0 = DBCR0_EDM;
  
 if (vcpu-guest_debug  KVM_GUESTDBG_SINGLESTEP)
 vcpu-arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
  
  This was intended to let the guest know that the host owns the debug 
  resources,
  by analogy to what a JTAG debugger would do.
  
  The Power ISA has this Virtualized Implementation Note:
  
  It is the responsibility of the hypervisor to ensure that
  DBCR0[EDM] is consistent with usage of DEP.
 
 Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if MSRP_DEP is 
 clear then clear DBCR0_EDM, right?
 We need to implement above mentioned this.

We should probably clear EDM only when guest debug emulation is working
and enabled (i.e. not until at least patch 6/6).

-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/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Wed, 2014-07-30 at 12:57 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Wednesday, July 30, 2014 11:18 PM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest
  visible register
  
  On Wed, 2014-07-30 at 00:21 -0500, Bhushan Bharat-R65777 wrote:
  
-Original Message-
From: Wood Scott-B07421
Sent: Tuesday, July 29, 2014 3:22 AM
To: Bhushan Bharat-R65777
Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
Yoder Stuart-
B08248
Subject: Re: [PATCH 1/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM
in guest visible register
   
On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
 This is not used and  even I do not remember why this was added in
 first place.

 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke.c | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index ab62109..a5ee42c 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -1804,8 +1804,6 @@ int
 kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
*vcpu,
   kvm_guest_protect_msr(vcpu, MSR_DE, true);
   vcpu-guest_debug = dbg-control;
   vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
 - /* Set DBCR0_EDM in guest visible DBCR0 register. */
 - vcpu-arch.dbg_reg.dbcr0 = DBCR0_EDM;

   if (vcpu-guest_debug  KVM_GUESTDBG_SINGLESTEP)
   vcpu-arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;
   
This was intended to let the guest know that the host owns the debug
resources, by analogy to what a JTAG debugger would do.
   
The Power ISA has this Virtualized Implementation Note:
   
It is the responsibility of the hypervisor to ensure that
DBCR0[EDM] is consistent with usage of DEP.
  
   Ok, That means that if MSRP_DEP is set then set DBCR0_EDM  and if 
   MSRP_DEP is
  clear then clear DBCR0_EDM, right?
   We need to implement above mentioned this.
  
  We should probably clear EDM only when guest debug emulation is working and
  enabled (i.e. not until at least patch 6/6).
 
 But if EDM is set then guest debug emulation will not start/allowed.

I don't mean after the guest tries to write to the registers -- I mean
after the code has been added to KVM to allow it to work.

-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 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Tuesday, July 29, 2014 3:58 AM
  To: Bhushan Bharat-R65777
  Cc: ag...@suse.de; kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Yoder 
  Stuart-
  B08248
  Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and 
  exception
  
   Userspace might be interested in
  the raw value,
 
 With the current design, If userspace is interested then it will not
 get the DBSR.

Oh, because DBSR isn't currently implemented in sregs or one reg?

  But why userspace will be interested?

Do you expose all of the hardware's debugging features in your
high-level interface?

  plus it's a change from the current API semantics.
 
 Can you please let us know how ?

It looked like it was removing dbsr visibility and the requirement for
userspace to clear dbsr.  I guess the old way was that the value in
vcpu-arch.dbsr didn't matter until the next debug exception, when it
would be overwritten by the new SPRN_DBSR?

   + case SPRN_DBCR2:
   + /*
   +  * If userspace is debugging guest then guest
   +  * can not access debug registers.
   +  */
   + if (vcpu-guest_debug)
   + break;
   +
   + debug_inst = true;
   + vcpu-arch.dbg_reg.dbcr2 = spr_val;
   + vcpu-arch.shadow_dbg_reg.dbcr2 = spr_val;
 break;
  
  In what circumstances can the architected and shadow registers differ?
 
 As of now they are same. But I think that if we want to implement other 
 features like Freeze Timer (FT) then they can be different.

I don't think we can possibly implement Freeze Timer.
 
 case SPRN_DBSR:
   + /*
   +  * If userspace is debugging guest then guest
   +  * can not access debug registers.
   +  */
   + if (vcpu-guest_debug)
   + break;
   +
 vcpu-arch.dbsr = ~spr_val;
   + if (vcpu-arch.dbsr == 0)
   + kvmppc_core_dequeue_debug(vcpu);
 break;
  
  Not all DBSR bits cause an exception, e.g. IDE and MRR.
 
 I am not sure what we should in that case ?

 As we are currently emulating a subset of debug events (IAC, DAC, IC,
 BT and TIE --- DBCR0 emulation) then we should expose status of those
 events in guest dbsr and rest should be cleared ?

I'm not saying they need to be exposed to the guest, but I don't see
where you filter out bits like these.

   @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu 
   *vcpu, int
  sprn, ulong spr_val)
 emulated = EMULATE_FAIL;
 }
  
   + if (debug_inst) {
   + switch_booke_debug_regs(vcpu-arch.shadow_dbg_reg);
   + current-thread.debug = vcpu-arch.shadow_dbg_reg;
   + }
  
  Could you explain what's going on with regard to copying the registers
  into current-thread.debug?  Why is it done after loading the registers
  into the hardware (is there a race if we get preempted in the middle)?
 
 Yes, and this was something I was not clear when writing this code.
 Should we have preempt disable-enable around this.

Can they be reordered instead?

-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 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
 On 29.07.14 00:33, Scott Wood wrote:
  On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
  On 11.07.14 10:39, Bharat Bhushan wrote:
  This patch emulates debug registers and debug exception
  to support guest using debug resource. This enables running
  gdb/kgdb etc in guest.
 
  On BOOKE architecture we cannot share debug resources between QEMU and
  guest because:
When QEMU is using debug resources then debug exception must
be always enabled. To achieve this we set MSR_DE and also set
MSRP_DEP so guest cannot change MSR_DE.
 
When emulating debug resource for guest we want guest
to control MSR_DE (enable/disable debug interrupt on need).
 
So above mentioned two configuration cannot be supported
at the same time. So the result is that we cannot share
debug resources between QEMU and Guest on BOOKE architecture.
 
  In the current design QEMU gets priority over guest, this means that if
  QEMU is using debug resources then guest cannot use them and if guest is
  using debug resource then QEMU can overwrite them.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  Hi Alex,
 
  I thought of having some print in register emulation if QEMU
  is using debug resource, Also when QEMU overwrites guest written
  values but that looks excessive. If I uses some variable which
  get set when guest starts using debug registers and check in
  debug set ioctl then that look ugly. Looking for suggestions
  Whatever you do, have QEMU do the print, not the kernel.
  How would that be accomplished?  How would the kernel know to exit to
  QEMU, and how would the exit reason be conveyed?
 
 QEMU is the one forcefully enabling debug and overwriting guest debug 
 registers, so it also knows when it did overwrite valid ones.

QEMU knows when it overwrites the guest values, but it doesn't know if,
after enabling host debug, the guest tries to write to the debug
registers and it gets nopped.  If we keep the EDM setting, then we can
at least say the situation is no worse than with a JTAG.

-Scott


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

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Tue, 2014-07-29 at 16:06 +0200, Alexander Graf wrote:
 On 29.07.14 00:33, Scott Wood wrote:
  On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
  On 11.07.14 10:39, Bharat Bhushan wrote:
  This patch emulates debug registers and debug exception
  to support guest using debug resource. This enables running
  gdb/kgdb etc in guest.
 
  On BOOKE architecture we cannot share debug resources between QEMU and
  guest because:
When QEMU is using debug resources then debug exception must
be always enabled. To achieve this we set MSR_DE and also set
MSRP_DEP so guest cannot change MSR_DE.
 
When emulating debug resource for guest we want guest
to control MSR_DE (enable/disable debug interrupt on need).
 
So above mentioned two configuration cannot be supported
at the same time. So the result is that we cannot share
debug resources between QEMU and Guest on BOOKE architecture.
 
  In the current design QEMU gets priority over guest, this means that if
  QEMU is using debug resources then guest cannot use them and if guest is
  using debug resource then QEMU can overwrite them.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  Hi Alex,
 
  I thought of having some print in register emulation if QEMU
  is using debug resource, Also when QEMU overwrites guest written
  values but that looks excessive. If I uses some variable which
  get set when guest starts using debug registers and check in
  debug set ioctl then that look ugly. Looking for suggestions
  Whatever you do, have QEMU do the print, not the kernel.
  How would that be accomplished?  How would the kernel know to exit to
  QEMU, and how would the exit reason be conveyed?
 
 QEMU is the one forcefully enabling debug and overwriting guest debug 
 registers, so it also knows when it did overwrite valid ones.

QEMU knows when it overwrites the guest values, but it doesn't know if,
after enabling host debug, the guest tries to write to the debug
registers and it gets nopped.  If we keep the EDM setting, then we can
at least say the situation is no worse than with a JTAG.

-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/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
 This is not used and  even I do not remember why this was added
 in first place.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke.c | 2 --
  1 file changed, 2 deletions(-)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index ab62109..a5ee42c 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu 
 *vcpu,
   kvm_guest_protect_msr(vcpu, MSR_DE, true);
   vcpu-guest_debug = dbg-control;
   vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
 - /* Set DBCR0_EDM in guest visible DBCR0 register. */
 - vcpu-arch.dbg_reg.dbcr0 = DBCR0_EDM;
  
   if (vcpu-guest_debug  KVM_GUESTDBG_SINGLESTEP)
   vcpu-arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;

This was intended to let the guest know that the host owns the debug
resources, by analogy to what a JTAG debugger would do.

The Power ISA has this Virtualized Implementation Note:

It is the responsibility of the hypervisor to ensure that
DBCR0[EDM] is consistent with usage of DEP.

-Scott


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

Re: [PATCH 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug

[Scott Wood]

On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
 When userspace (QEMU) is using the debug resource to debug guest
 then we want MSR_DE to be always set. This patch adds missing
 MSR_DE setting in rfci instruction.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke_emulate.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)
 
 diff --git a/arch/powerpc/kvm/booke_emulate.c 
 b/arch/powerpc/kvm/booke_emulate.c
 index 27a4b28..80c51a2 100644
 --- a/arch/powerpc/kvm/booke_emulate.c
 +++ b/arch/powerpc/kvm/booke_emulate.c
 @@ -40,7 +40,11 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
  static void kvmppc_emul_rfci(struct kvm_vcpu *vcpu)
  {
   vcpu-arch.pc = vcpu-arch.csrr0;
 - kvmppc_set_msr(vcpu, vcpu-arch.csrr1);
 + /* Force MSR_DE when guest does not own debug facilities */
 + if (vcpu-guest_debug)
 + kvmppc_set_msr(vcpu, vcpu-arch.csrr1 | MSR_DE);
 + else
 + kvmppc_set_msr(vcpu, vcpu-arch.csrr1);
  }
  
  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,

It looks like this is already handled by kvmppc_vcpu_sync_debug(), which
is called by kvmppc_set_msr().

Plus, it should only be done for HV mode.

-Scott


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

Re: [PATCH 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE

[Scott Wood]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
 When userspace is debugging guest then MSR_DE is always set and
 MSRP_DEP is set so that guest cannot change MSR_DE.
 Guest debug resources are not yet emulated, So there seems no reason
 we should stop guest controlling MSR_DE.
 Also a followup patch will enable debug emulation and that requires
 guest to control MSR_DE.

Why does it matter whether we emulate debug resources?  We still don't
want the guest to be able to clear MSR[DE] and thus break host debug.

-Scott


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

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
 This patch emulates debug registers and debug exception
 to support guest using debug resource. This enables running
 gdb/kgdb etc in guest.
 
 On BOOKE architecture we cannot share debug resources between QEMU and
 guest because:
 When QEMU is using debug resources then debug exception must
 be always enabled. To achieve this we set MSR_DE and also set
 MSRP_DEP so guest cannot change MSR_DE.

 When emulating debug resource for guest we want guest
 to control MSR_DE (enable/disable debug interrupt on need).
 
 So above mentioned two configuration cannot be supported
 at the same time. So the result is that we cannot share
 debug resources between QEMU and Guest on BOOKE architecture.
 
 In the current design QEMU gets priority over guest, this means that if
 QEMU is using debug resources then guest cannot use them and if guest is
 using debug resource then QEMU can overwrite them.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 Hi Alex,
 
 I thought of having some print in register emulation if QEMU
 is using debug resource, Also when QEMU overwrites guest written
 values but that looks excessive. If I uses some variable which
 get set when guest starts using debug registers and check in
 debug set ioctl then that look ugly. Looking for suggestions
 
  arch/powerpc/include/asm/kvm_ppc.h |   3 +
  arch/powerpc/kvm/booke.c   |  27 +++
  arch/powerpc/kvm/booke_emulate.c   | 157 
 +
  3 files changed, 187 insertions(+)
 
 diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
 b/arch/powerpc/include/asm/kvm_ppc.h
 index e2fd5a1..f3f7611 100644
 --- a/arch/powerpc/include/asm/kvm_ppc.h
 +++ b/arch/powerpc/include/asm/kvm_ppc.h
 @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, 
 u32 *server,
  extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
  extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
  
 +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu);
 +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
 +
  union kvmppc_one_reg {
   u32 wval;
   u64 dval;
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index fadfe76..c2471ed 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu 
 *vcpu)
   clear_bit(BOOKE_IRQPRIO_WATCHDOG, vcpu-arch.pending_exceptions);
  }
  
 +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
 +{
 + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
 +}
 +
 +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
 +{
 + clear_bit(BOOKE_IRQPRIO_DEBUG, vcpu-arch.pending_exceptions);
 +}
 +
  static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 
 srr1)
  {
  #ifdef CONFIG_KVM_BOOKE_HV
 @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
   u32 dbsr = vcpu-arch.dbsr;
  
 + if (vcpu-guest_debug == 0) {
 + /* Debug resources belong to Guest */
 + if (dbsr  (vcpu-arch.shared-msr  MSR_DE))
 + kvmppc_core_queue_debug(vcpu);

Should also check for DBCR0[IDM].

 +
 + /* Inject a program interrupt if trap debug is not allowed */
 + if ((dbsr  DBSR_TIE)  !(vcpu-arch.shared-msr  MSR_DE))
 + kvmppc_core_queue_program(vcpu, ESR_PTR);
 +
 + return RESUME_GUEST;
 + }
 +
 + /*
 +  * Prepare for userspace exit as debug resources
 +  * are owned by userspace.
 +  */
 + vcpu-arch.dbsr = 0;
   run-debug.arch.status = 0;
   run-debug.arch.address = vcpu-arch.pc;

Why are you clearing vcpu-arch.dbsr?  Userspace might be interested in
the raw value, plus it's a change from the current API semantics.

Where is this run-debug.arch.foo stuff and KVM_EXIT_DEBUG documented?


 diff --git a/arch/powerpc/kvm/booke_emulate.c 
 b/arch/powerpc/kvm/booke_emulate.c
 index 2a20194..3d143fe 100644
 --- a/arch/powerpc/kvm/booke_emulate.c
 +++ b/arch/powerpc/kvm/booke_emulate.c
 @@ -139,6 +139,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct 
 kvm_vcpu *vcpu,
  int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong 
 spr_val)
  {
   int emulated = EMULATE_DONE;
 + bool debug_inst = false;
  
   switch (sprn) {
   case SPRN_DEAR:
 @@ -153,14 +154,137 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, 
 int sprn, ulong spr_val)
   case SPRN_CSRR1:
   vcpu-arch.csrr1 = spr_val;
   break;
 + case SPRN_DSRR0:
 + vcpu-arch.dsrr0 = spr_val;
 + break;
 + case SPRN_DSRR1:
 + vcpu-arch.dsrr1 = spr_val;
 + break;
 + case SPRN_IAC1:
 + /*
 +  * If userspace is debugging guest then 

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
 On 11.07.14 10:39, Bharat Bhushan wrote:
  This patch emulates debug registers and debug exception
  to support guest using debug resource. This enables running
  gdb/kgdb etc in guest.
 
  On BOOKE architecture we cannot share debug resources between QEMU and
  guest because:
   When QEMU is using debug resources then debug exception must
   be always enabled. To achieve this we set MSR_DE and also set
   MSRP_DEP so guest cannot change MSR_DE.
 
   When emulating debug resource for guest we want guest
   to control MSR_DE (enable/disable debug interrupt on need).
 
   So above mentioned two configuration cannot be supported
   at the same time. So the result is that we cannot share
   debug resources between QEMU and Guest on BOOKE architecture.
 
  In the current design QEMU gets priority over guest, this means that if
  QEMU is using debug resources then guest cannot use them and if guest is
  using debug resource then QEMU can overwrite them.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  Hi Alex,
 
  I thought of having some print in register emulation if QEMU
  is using debug resource, Also when QEMU overwrites guest written
  values but that looks excessive. If I uses some variable which
  get set when guest starts using debug registers and check in
  debug set ioctl then that look ugly. Looking for suggestions
 
 Whatever you do, have QEMU do the print, not the kernel.

How would that be accomplished?  How would the kernel know to exit to
QEMU, and how would the exit reason be conveyed?

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Mon, 2014-07-28 at 03:54 -0500, Caraman Mihai Claudiu-B02008 wrote:
  -Original Message-
  From: Wood Scott-B07421
  Sent: Saturday, July 26, 2014 3:11 AM
  To: Caraman Mihai Claudiu-B02008
  Cc: Alexander Graf; kvm-...@vger.kernel.org; kvm@vger.kernel.org;
  linuxppc-...@lists.ozlabs.org
  Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
  SPE/FP/AltiVec int numbers
  
  On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
   Scott, Alex's request to define SPE handlers only for e500v2 implies
  changes
   in 32-bit FSL kernel to have exclusive configurations for e200/e500v2
  and
   e500mc/e5500. We would probably need something like this, what's your
  take on it?
  
  That is already a compile-time decision.
 
 Yes, but is not fully implemented.

We might be missing some kconfig language to prohibit e500v2 boards from
being enabled in an e500mc kernel, but if you try to do so it won't work
(except for obsolete hacks like running QEMU's mpc8544ds platform with
-cpu e500mc).

   diff --git a/arch/powerpc/kernel/head_fsl_booke.S
  b/arch/powerpc/kernel/head_fsl_booke.S
   index b497188..9d41015 100644
   --- a/arch/powerpc/kernel/head_fsl_booke.S
   +++ b/arch/powerpc/kernel/head_fsl_booke.S
   @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
   mfspr   r10, SPRN_SPRG_RSCRATCH0
   b   InstructionStorage
  
   +/* Define SPE handlers only for e500v2 and e200 */
   +#ifndef CONFIG_PPC_E500MC
#ifdef CONFIG_SPE
   /* SPE Unavailable */
   START_EXCEPTION(SPEUnavailable)
   @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
   EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
 unknown_exception, EXC_XFER_EE)
#endif /* CONFIG_SPE */
   +#endif
  
  This is redundant:
  
  config SPE
  bool SPE Support
  depends on E200 || (E500  !PPC_E500MC)
  default y
 
 I see what you mean, but it's not redundant.

OK, I didn't realize there was an #else that wasn't included in the
context.  It would have been nice if the comment at the end were
!CONFIG_SPE rather than CONFIG_SPE.

 Alex was looking to remove SPE
 handlers for e500mc+ and the proposal handled !SPE case. In the new
 light I find this variant more readable:
 
 +/* Define SPE handlers for e200 and e500v2 */
  #ifdef CONFIG_SPE
 /* SPE Unavailable */
 START_EXCEPTION(SPEUnavailable)
 @@ -622,11 +623,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
 b   fast_exception_return
  1: addir3,r1,STACK_FRAME_OVERHEAD
 EXC_XFER_EE_LITE(0x2010, KernelSPE)
 -#else
 +#elif defined(CONFIG_E200) || \
 +  (defined(CONFIG_E500)  !defined(CONFIG_PPC_E500MC))
 EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
   unknown_exception, EXC_XFER_EE)
  #endif /* CONFIG_SPE */

Yes, or better define a CONFIG_SPE_POSSIBLE so that the list only has to
exist in one place, and the intent is clearer.
 
   diff --git a/arch/powerpc/kernel/cputable.c
  b/arch/powerpc/kernel/cputable.c
   index c1faade..3ab65c2 100644
   --- a/arch/powerpc/kernel/cputable.c
   +++ b/arch/powerpc/kernel/cputable.c
   @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
#endif /* CONFIG_PPC32 */
#ifdef CONFIG_E500
#ifdef CONFIG_PPC32
   +#ifndef CONFIG_PPC_E500MC
   {   /* e500 */
   .pvr_mask   = 0x,
   .pvr_value  = 0x8020,
   @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
   .machine_check  = machine_check_e500,
   .platform   = ppc8548,
   },
   +#endif /* !CONFIG_PPC_E500MC */
   {   /* e500mc */
   .pvr_mask   = 0x,
   .pvr_value  = 0x8023,
  
  
  This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
  e500mc gets included in !PPC_E500MC?
 
 Right, my main purpose was to get rid of __setup_e500_ivors on PPC_E500MC
 which refers SPEUnavailable. I will add an #else to exclude e500mc.
 
 The generic E500 PPC default cpu advertises PPC_FEATURE_HAS_SPE_COMP.
 Do we want to excluded it if PPC_E500MC? Is this cpu useful without 
 cpu_setup()
 and if so why don't we also have a default for 64-bit?

I don't think that default will do anything useful.

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6] KVM: PPC: BOOKE: No need to set DBCR0_EDM in guest visible register

[Scott Wood]

On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
 This is not used and  even I do not remember why this was added
 in first place.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke.c | 2 --
  1 file changed, 2 deletions(-)
 
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index ab62109..a5ee42c 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -1804,8 +1804,6 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu 
 *vcpu,
   kvm_guest_protect_msr(vcpu, MSR_DE, true);
   vcpu-guest_debug = dbg-control;
   vcpu-arch.shadow_dbg_reg.dbcr0 = 0;
 - /* Set DBCR0_EDM in guest visible DBCR0 register. */
 - vcpu-arch.dbg_reg.dbcr0 = DBCR0_EDM;
  
   if (vcpu-guest_debug  KVM_GUESTDBG_SINGLESTEP)
   vcpu-arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC;

This was intended to let the guest know that the host owns the debug
resources, by analogy to what a JTAG debugger would do.

The Power ISA has this Virtualized Implementation Note:

It is the responsibility of the hypervisor to ensure that
DBCR0[EDM] is consistent with usage of DEP.

-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 2/6] KVM: PPC: BOOKE: Force MSR_DE in rfci if guest is under debug

[Scott Wood]

On Fri, 2014-07-11 at 14:08 +0530, Bharat Bhushan wrote:
 When userspace (QEMU) is using the debug resource to debug guest
 then we want MSR_DE to be always set. This patch adds missing
 MSR_DE setting in rfci instruction.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
  arch/powerpc/kvm/booke_emulate.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)
 
 diff --git a/arch/powerpc/kvm/booke_emulate.c 
 b/arch/powerpc/kvm/booke_emulate.c
 index 27a4b28..80c51a2 100644
 --- a/arch/powerpc/kvm/booke_emulate.c
 +++ b/arch/powerpc/kvm/booke_emulate.c
 @@ -40,7 +40,11 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
  static void kvmppc_emul_rfci(struct kvm_vcpu *vcpu)
  {
   vcpu-arch.pc = vcpu-arch.csrr0;
 - kvmppc_set_msr(vcpu, vcpu-arch.csrr1);
 + /* Force MSR_DE when guest does not own debug facilities */
 + if (vcpu-guest_debug)
 + kvmppc_set_msr(vcpu, vcpu-arch.csrr1 | MSR_DE);
 + else
 + kvmppc_set_msr(vcpu, vcpu-arch.csrr1);
  }
  
  int kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,

It looks like this is already handled by kvmppc_vcpu_sync_debug(), which
is called by kvmppc_set_msr().

Plus, it should only be done for HV mode.

-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 5/6] KVM: PPC: BOOKE: Allow guest to change MSR_DE

[Scott Wood]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
 When userspace is debugging guest then MSR_DE is always set and
 MSRP_DEP is set so that guest cannot change MSR_DE.
 Guest debug resources are not yet emulated, So there seems no reason
 we should stop guest controlling MSR_DE.
 Also a followup patch will enable debug emulation and that requires
 guest to control MSR_DE.

Why does it matter whether we emulate debug resources?  We still don't
want the guest to be able to clear MSR[DE] and thus break host debug.

-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 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
 This patch emulates debug registers and debug exception
 to support guest using debug resource. This enables running
 gdb/kgdb etc in guest.
 
 On BOOKE architecture we cannot share debug resources between QEMU and
 guest because:
 When QEMU is using debug resources then debug exception must
 be always enabled. To achieve this we set MSR_DE and also set
 MSRP_DEP so guest cannot change MSR_DE.

 When emulating debug resource for guest we want guest
 to control MSR_DE (enable/disable debug interrupt on need).
 
 So above mentioned two configuration cannot be supported
 at the same time. So the result is that we cannot share
 debug resources between QEMU and Guest on BOOKE architecture.
 
 In the current design QEMU gets priority over guest, this means that if
 QEMU is using debug resources then guest cannot use them and if guest is
 using debug resource then QEMU can overwrite them.
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 Hi Alex,
 
 I thought of having some print in register emulation if QEMU
 is using debug resource, Also when QEMU overwrites guest written
 values but that looks excessive. If I uses some variable which
 get set when guest starts using debug registers and check in
 debug set ioctl then that look ugly. Looking for suggestions
 
  arch/powerpc/include/asm/kvm_ppc.h |   3 +
  arch/powerpc/kvm/booke.c   |  27 +++
  arch/powerpc/kvm/booke_emulate.c   | 157 
 +
  3 files changed, 187 insertions(+)
 
 diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
 b/arch/powerpc/include/asm/kvm_ppc.h
 index e2fd5a1..f3f7611 100644
 --- a/arch/powerpc/include/asm/kvm_ppc.h
 +++ b/arch/powerpc/include/asm/kvm_ppc.h
 @@ -173,6 +173,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq, 
 u32 *server,
  extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq);
  extern int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
  
 +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu);
 +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
 +
  union kvmppc_one_reg {
   u32 wval;
   u64 dval;
 diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
 index fadfe76..c2471ed 100644
 --- a/arch/powerpc/kvm/booke.c
 +++ b/arch/powerpc/kvm/booke.c
 @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu 
 *vcpu)
   clear_bit(BOOKE_IRQPRIO_WATCHDOG, vcpu-arch.pending_exceptions);
  }
  
 +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu)
 +{
 + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG);
 +}
 +
 +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu)
 +{
 + clear_bit(BOOKE_IRQPRIO_DEBUG, vcpu-arch.pending_exceptions);
 +}
 +
  static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 
 srr1)
  {
  #ifdef CONFIG_KVM_BOOKE_HV
 @@ -783,6 +793,23 @@ static int kvmppc_handle_debug(struct kvm_run *run, 
 struct kvm_vcpu *vcpu)
   struct debug_reg *dbg_reg = (vcpu-arch.shadow_dbg_reg);
   u32 dbsr = vcpu-arch.dbsr;
  
 + if (vcpu-guest_debug == 0) {
 + /* Debug resources belong to Guest */
 + if (dbsr  (vcpu-arch.shared-msr  MSR_DE))
 + kvmppc_core_queue_debug(vcpu);

Should also check for DBCR0[IDM].

 +
 + /* Inject a program interrupt if trap debug is not allowed */
 + if ((dbsr  DBSR_TIE)  !(vcpu-arch.shared-msr  MSR_DE))
 + kvmppc_core_queue_program(vcpu, ESR_PTR);
 +
 + return RESUME_GUEST;
 + }
 +
 + /*
 +  * Prepare for userspace exit as debug resources
 +  * are owned by userspace.
 +  */
 + vcpu-arch.dbsr = 0;
   run-debug.arch.status = 0;
   run-debug.arch.address = vcpu-arch.pc;

Why are you clearing vcpu-arch.dbsr?  Userspace might be interested in
the raw value, plus it's a change from the current API semantics.

Where is this run-debug.arch.foo stuff and KVM_EXIT_DEBUG documented?


 diff --git a/arch/powerpc/kvm/booke_emulate.c 
 b/arch/powerpc/kvm/booke_emulate.c
 index 2a20194..3d143fe 100644
 --- a/arch/powerpc/kvm/booke_emulate.c
 +++ b/arch/powerpc/kvm/booke_emulate.c
 @@ -139,6 +139,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct 
 kvm_vcpu *vcpu,
  int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn, ulong 
 spr_val)
  {
   int emulated = EMULATE_DONE;
 + bool debug_inst = false;
  
   switch (sprn) {
   case SPRN_DEAR:
 @@ -153,14 +154,137 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, 
 int sprn, ulong spr_val)
   case SPRN_CSRR1:
   vcpu-arch.csrr1 = spr_val;
   break;
 + case SPRN_DSRR0:
 + vcpu-arch.dsrr0 = spr_val;
 + break;
 + case SPRN_DSRR1:
 + vcpu-arch.dsrr1 = spr_val;
 + break;
 + case SPRN_IAC1:
 + /*
 +  * If userspace is debugging guest then 

Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood]

On Mon, 2014-07-28 at 16:04 +0200, Alexander Graf wrote:
 On 11.07.14 10:39, Bharat Bhushan wrote:
  This patch emulates debug registers and debug exception
  to support guest using debug resource. This enables running
  gdb/kgdb etc in guest.
 
  On BOOKE architecture we cannot share debug resources between QEMU and
  guest because:
   When QEMU is using debug resources then debug exception must
   be always enabled. To achieve this we set MSR_DE and also set
   MSRP_DEP so guest cannot change MSR_DE.
 
   When emulating debug resource for guest we want guest
   to control MSR_DE (enable/disable debug interrupt on need).
 
   So above mentioned two configuration cannot be supported
   at the same time. So the result is that we cannot share
   debug resources between QEMU and Guest on BOOKE architecture.
 
  In the current design QEMU gets priority over guest, this means that if
  QEMU is using debug resources then guest cannot use them and if guest is
  using debug resource then QEMU can overwrite them.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  Hi Alex,
 
  I thought of having some print in register emulation if QEMU
  is using debug resource, Also when QEMU overwrites guest written
  values but that looks excessive. If I uses some variable which
  get set when guest starts using debug registers and check in
  debug set ioctl then that look ugly. Looking for suggestions
 
 Whatever you do, have QEMU do the print, not the kernel.

How would that be accomplished?  How would the kernel know to exit to
QEMU, and how would the exit reason be conveyed?

-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/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Mon, 2014-07-28 at 03:54 -0500, Caraman Mihai Claudiu-B02008 wrote:
  -Original Message-
  From: Wood Scott-B07421
  Sent: Saturday, July 26, 2014 3:11 AM
  To: Caraman Mihai Claudiu-B02008
  Cc: Alexander Graf; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
  linuxppc-...@lists.ozlabs.org
  Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
  SPE/FP/AltiVec int numbers
  
  On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
   Scott, Alex's request to define SPE handlers only for e500v2 implies
  changes
   in 32-bit FSL kernel to have exclusive configurations for e200/e500v2
  and
   e500mc/e5500. We would probably need something like this, what's your
  take on it?
  
  That is already a compile-time decision.
 
 Yes, but is not fully implemented.

We might be missing some kconfig language to prohibit e500v2 boards from
being enabled in an e500mc kernel, but if you try to do so it won't work
(except for obsolete hacks like running QEMU's mpc8544ds platform with
-cpu e500mc).

   diff --git a/arch/powerpc/kernel/head_fsl_booke.S
  b/arch/powerpc/kernel/head_fsl_booke.S
   index b497188..9d41015 100644
   --- a/arch/powerpc/kernel/head_fsl_booke.S
   +++ b/arch/powerpc/kernel/head_fsl_booke.S
   @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
   mfspr   r10, SPRN_SPRG_RSCRATCH0
   b   InstructionStorage
  
   +/* Define SPE handlers only for e500v2 and e200 */
   +#ifndef CONFIG_PPC_E500MC
#ifdef CONFIG_SPE
   /* SPE Unavailable */
   START_EXCEPTION(SPEUnavailable)
   @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
   EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
 unknown_exception, EXC_XFER_EE)
#endif /* CONFIG_SPE */
   +#endif
  
  This is redundant:
  
  config SPE
  bool SPE Support
  depends on E200 || (E500  !PPC_E500MC)
  default y
 
 I see what you mean, but it's not redundant.

OK, I didn't realize there was an #else that wasn't included in the
context.  It would have been nice if the comment at the end were
!CONFIG_SPE rather than CONFIG_SPE.

 Alex was looking to remove SPE
 handlers for e500mc+ and the proposal handled !SPE case. In the new
 light I find this variant more readable:
 
 +/* Define SPE handlers for e200 and e500v2 */
  #ifdef CONFIG_SPE
 /* SPE Unavailable */
 START_EXCEPTION(SPEUnavailable)
 @@ -622,11 +623,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
 b   fast_exception_return
  1: addir3,r1,STACK_FRAME_OVERHEAD
 EXC_XFER_EE_LITE(0x2010, KernelSPE)
 -#else
 +#elif defined(CONFIG_E200) || \
 +  (defined(CONFIG_E500)  !defined(CONFIG_PPC_E500MC))
 EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
   unknown_exception, EXC_XFER_EE)
  #endif /* CONFIG_SPE */

Yes, or better define a CONFIG_SPE_POSSIBLE so that the list only has to
exist in one place, and the intent is clearer.
 
   diff --git a/arch/powerpc/kernel/cputable.c
  b/arch/powerpc/kernel/cputable.c
   index c1faade..3ab65c2 100644
   --- a/arch/powerpc/kernel/cputable.c
   +++ b/arch/powerpc/kernel/cputable.c
   @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
#endif /* CONFIG_PPC32 */
#ifdef CONFIG_E500
#ifdef CONFIG_PPC32
   +#ifndef CONFIG_PPC_E500MC
   {   /* e500 */
   .pvr_mask   = 0x,
   .pvr_value  = 0x8020,
   @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
   .machine_check  = machine_check_e500,
   .platform   = ppc8548,
   },
   +#endif /* !CONFIG_PPC_E500MC */
   {   /* e500mc */
   .pvr_mask   = 0x,
   .pvr_value  = 0x8023,
  
  
  This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
  e500mc gets included in !PPC_E500MC?
 
 Right, my main purpose was to get rid of __setup_e500_ivors on PPC_E500MC
 which refers SPEUnavailable. I will add an #else to exclude e500mc.
 
 The generic E500 PPC default cpu advertises PPC_FEATURE_HAS_SPE_COMP.
 Do we want to excluded it if PPC_E500MC? Is this cpu useful without 
 cpu_setup()
 and if so why don't we also have a default for 64-bit?

I don't think that default will do anything useful.

-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/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
 Scott, Alex's request to define SPE handlers only for e500v2 implies changes
 in 32-bit FSL kernel to have exclusive configurations for e200/e500v2 and
 e500mc/e5500. We would probably need something like this, what's your take on 
 it?

That is already a compile-time decision.

 diff --git a/arch/powerpc/kernel/head_fsl_booke.S 
 b/arch/powerpc/kernel/head_fsl_booke.S
 index b497188..9d41015 100644
 --- a/arch/powerpc/kernel/head_fsl_booke.S
 +++ b/arch/powerpc/kernel/head_fsl_booke.S
 @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
 mfspr   r10, SPRN_SPRG_RSCRATCH0
 b   InstructionStorage
  
 +/* Define SPE handlers only for e500v2 and e200 */
 +#ifndef CONFIG_PPC_E500MC
  #ifdef CONFIG_SPE
 /* SPE Unavailable */
 START_EXCEPTION(SPEUnavailable)
 @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
 EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
   unknown_exception, EXC_XFER_EE)
  #endif /* CONFIG_SPE */
 +#endif

This is redundant:

config SPE
bool SPE Support
depends on E200 || (E500  !PPC_E500MC)
default y 

 diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
 index c1faade..3ab65c2 100644
 --- a/arch/powerpc/kernel/cputable.c
 +++ b/arch/powerpc/kernel/cputable.c
 @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
  #endif /* CONFIG_PPC32 */
  #ifdef CONFIG_E500
  #ifdef CONFIG_PPC32
 +#ifndef CONFIG_PPC_E500MC
 {   /* e500 */
 .pvr_mask   = 0x,
 .pvr_value  = 0x8020,
 @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
 .machine_check  = machine_check_e500,
 .platform   = ppc8548,
 },
 +#endif /* !CONFIG_PPC_E500MC */
 {   /* e500mc */
 .pvr_mask   = 0x,
 .pvr_value  = 0x8023,
 

This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
e500mc gets included in !PPC_E500MC?

-Scott


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

[PATCH] powerpc/e6500: Work around erratum A-008139

[Scott Wood]

Erratum A-008139 can cause duplicate TLB entries if an indirect
entry is overwritten using tlbwe while the other thread is using it to
do a lookup.  Work around this by using tlbilx to invalidate prior
to overwriting.

To avoid the need to save another register to hold MAS1 during the
workaround code, TID clearing has been moved from tlb_miss_kernel_e6500
until after the SMT section.

Signed-off-by: Scott Wood scottw...@freescale.com
---
 arch/powerpc/mm/tlb_low_64e.S | 68 +++
 1 file changed, 56 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 57c4d66..89bf95b 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -299,7 +299,9 @@ itlb_miss_fault_bolted:
  * r10 = crap (free to use)
  */
 tlb_miss_common_e6500:
-BEGIN_FTR_SECTION
+   crmove  cr2*4+2,cr0*4+2 /* cr2.eq != 0 if kernel address */
+
+BEGIN_FTR_SECTION  /* CPU_FTR_SMT */
/*
 * Search if we already have an indirect entry for that virtual
 * address, and if we do, bail out.
@@ -324,17 +326,62 @@ BEGIN_FTR_SECTION
b   1b
.previous
 
+   /*
+* Erratum A-008139 says that we can't use tlbwe to change
+* an indirect entry in any way (including replacing or
+* invalidating) if the other thread could be in the process
+* of a lookup.  The workaround is to invalidate the entry
+* with tlbilx before overwriting.
+*/
+
+   lbz r15,TCD_ESEL_NEXT(r11)
+   rlwinm  r10,r15,16,0xff
+   orisr10,r10,MAS0_TLBSEL(1)@h
+   mtspr   SPRN_MAS0,r10
+   isync
+   tlbre
mfspr   r15,SPRN_MAS1
-   mfspr   r10,SPRN_MAS2
+   andis.  r15,r15,MAS1_VALID@h
+   beq 5f
+
+BEGIN_FTR_SECTION_NESTED(532)
+   mfspr   r10,SPRN_MAS8
+   rlwinm  r10,r10,0,0x8fff  /* tgs,tlpid - sgs,slpid */
+   mtspr   SPRN_MAS5,r10
+END_FTR_SECTION_NESTED(CPU_FTR_EMB_HV,CPU_FTR_EMB_HV,532)
 
-   tlbsx   0,r16
-   mtspr   SPRN_MAS2,r10
mfspr   r10,SPRN_MAS1
-   mtspr   SPRN_MAS1,r15
+   rlwinm  r15,r10,0,0x3fff  /* tid - spid */
+   rlwimi  r15,r10,20,0x0003 /* ind,ts - sind,sas */
+   mfspr   r10,SPRN_MAS6
+   mtspr   SPRN_MAS6,r15
+
+   mfspr   r15,SPRN_MAS2
+   isync
+   tlbilxva 0,r15
+   isync
+
+   mtspr   SPRN_MAS6,r10
 
-   andis.  r10,r10,MAS1_VALID@h
+5:
+BEGIN_FTR_SECTION_NESTED(532)
+   li  r10,0
+   mtspr   SPRN_MAS8,r10
+   mtspr   SPRN_MAS5,r10
+END_FTR_SECTION_NESTED(CPU_FTR_EMB_HV,CPU_FTR_EMB_HV,532)
+
+   tlbsx   0,r16
+   mfspr   r10,SPRN_MAS1
+   andis.  r15,r10,MAS1_VALID@h
bne tlb_miss_done_e6500
-END_FTR_SECTION_IFSET(CPU_FTR_SMT)
+FTR_SECTION_ELSE
+   mfspr   r10,SPRN_MAS1
+ALT_FTR_SECTION_END_IFSET(CPU_FTR_SMT)
+
+   orisr10,r10,MAS1_VALID@h
+   beq cr2,4f
+   rlwinm  r10,r10,0,16,1  /* Clear TID */
+4: mtspr   SPRN_MAS1,r10
 
/* Now, we need to walk the page tables. First check if we are in
 * range.
@@ -410,12 +457,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_SMT)
rfi
 
 tlb_miss_kernel_e6500:
-   mfspr   r10,SPRN_MAS1
ld  r14,PACA_KERNELPGD(r13)
-   cmpldi  cr0,r15,8   /* Check for vmalloc region */
-   rlwinm  r10,r10,0,16,1  /* Clear TID */
-   mtspr   SPRN_MAS1,r10
-   beq+tlb_miss_common_e6500
+   cmpldi  cr1,r15,8   /* Check for vmalloc region */
+   beq+cr1,tlb_miss_common_e6500
 
 tlb_miss_fault_e6500:
tlb_unlock_e6500
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe kvm 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/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
 Scott, Alex's request to define SPE handlers only for e500v2 implies changes
 in 32-bit FSL kernel to have exclusive configurations for e200/e500v2 and
 e500mc/e5500. We would probably need something like this, what's your take on 
 it?

That is already a compile-time decision.

 diff --git a/arch/powerpc/kernel/head_fsl_booke.S 
 b/arch/powerpc/kernel/head_fsl_booke.S
 index b497188..9d41015 100644
 --- a/arch/powerpc/kernel/head_fsl_booke.S
 +++ b/arch/powerpc/kernel/head_fsl_booke.S
 @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
 mfspr   r10, SPRN_SPRG_RSCRATCH0
 b   InstructionStorage
  
 +/* Define SPE handlers only for e500v2 and e200 */
 +#ifndef CONFIG_PPC_E500MC
  #ifdef CONFIG_SPE
 /* SPE Unavailable */
 START_EXCEPTION(SPEUnavailable)
 @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
 EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
   unknown_exception, EXC_XFER_EE)
  #endif /* CONFIG_SPE */
 +#endif

This is redundant:

config SPE
bool SPE Support
depends on E200 || (E500  !PPC_E500MC)
default y 

 diff --git a/arch/powerpc/kernel/cputable.c b/arch/powerpc/kernel/cputable.c
 index c1faade..3ab65c2 100644
 --- a/arch/powerpc/kernel/cputable.c
 +++ b/arch/powerpc/kernel/cputable.c
 @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
  #endif /* CONFIG_PPC32 */
  #ifdef CONFIG_E500
  #ifdef CONFIG_PPC32
 +#ifndef CONFIG_PPC_E500MC
 {   /* e500 */
 .pvr_mask   = 0x,
 .pvr_value  = 0x8020,
 @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
 .machine_check  = machine_check_e500,
 .platform   = ppc8548,
 },
 +#endif /* !CONFIG_PPC_E500MC */
 {   /* e500mc */
 .pvr_mask   = 0x,
 .pvr_value  = 0x8023,
 

This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
e500mc gets included in !PPC_E500MC?

-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

[PATCH] powerpc/e6500: Work around erratum A-008139

[Scott Wood]

Erratum A-008139 can cause duplicate TLB entries if an indirect
entry is overwritten using tlbwe while the other thread is using it to
do a lookup.  Work around this by using tlbilx to invalidate prior
to overwriting.

To avoid the need to save another register to hold MAS1 during the
workaround code, TID clearing has been moved from tlb_miss_kernel_e6500
until after the SMT section.

Signed-off-by: Scott Wood scottw...@freescale.com
---
 arch/powerpc/mm/tlb_low_64e.S | 68 +++
 1 file changed, 56 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 57c4d66..89bf95b 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -299,7 +299,9 @@ itlb_miss_fault_bolted:
  * r10 = crap (free to use)
  */
 tlb_miss_common_e6500:
-BEGIN_FTR_SECTION
+   crmove  cr2*4+2,cr0*4+2 /* cr2.eq != 0 if kernel address */
+
+BEGIN_FTR_SECTION  /* CPU_FTR_SMT */
/*
 * Search if we already have an indirect entry for that virtual
 * address, and if we do, bail out.
@@ -324,17 +326,62 @@ BEGIN_FTR_SECTION
b   1b
.previous
 
+   /*
+* Erratum A-008139 says that we can't use tlbwe to change
+* an indirect entry in any way (including replacing or
+* invalidating) if the other thread could be in the process
+* of a lookup.  The workaround is to invalidate the entry
+* with tlbilx before overwriting.
+*/
+
+   lbz r15,TCD_ESEL_NEXT(r11)
+   rlwinm  r10,r15,16,0xff
+   orisr10,r10,MAS0_TLBSEL(1)@h
+   mtspr   SPRN_MAS0,r10
+   isync
+   tlbre
mfspr   r15,SPRN_MAS1
-   mfspr   r10,SPRN_MAS2
+   andis.  r15,r15,MAS1_VALID@h
+   beq 5f
+
+BEGIN_FTR_SECTION_NESTED(532)
+   mfspr   r10,SPRN_MAS8
+   rlwinm  r10,r10,0,0x8fff  /* tgs,tlpid - sgs,slpid */
+   mtspr   SPRN_MAS5,r10
+END_FTR_SECTION_NESTED(CPU_FTR_EMB_HV,CPU_FTR_EMB_HV,532)
 
-   tlbsx   0,r16
-   mtspr   SPRN_MAS2,r10
mfspr   r10,SPRN_MAS1
-   mtspr   SPRN_MAS1,r15
+   rlwinm  r15,r10,0,0x3fff  /* tid - spid */
+   rlwimi  r15,r10,20,0x0003 /* ind,ts - sind,sas */
+   mfspr   r10,SPRN_MAS6
+   mtspr   SPRN_MAS6,r15
+
+   mfspr   r15,SPRN_MAS2
+   isync
+   tlbilxva 0,r15
+   isync
+
+   mtspr   SPRN_MAS6,r10
 
-   andis.  r10,r10,MAS1_VALID@h
+5:
+BEGIN_FTR_SECTION_NESTED(532)
+   li  r10,0
+   mtspr   SPRN_MAS8,r10
+   mtspr   SPRN_MAS5,r10
+END_FTR_SECTION_NESTED(CPU_FTR_EMB_HV,CPU_FTR_EMB_HV,532)
+
+   tlbsx   0,r16
+   mfspr   r10,SPRN_MAS1
+   andis.  r15,r10,MAS1_VALID@h
bne tlb_miss_done_e6500
-END_FTR_SECTION_IFSET(CPU_FTR_SMT)
+FTR_SECTION_ELSE
+   mfspr   r10,SPRN_MAS1
+ALT_FTR_SECTION_END_IFSET(CPU_FTR_SMT)
+
+   orisr10,r10,MAS1_VALID@h
+   beq cr2,4f
+   rlwinm  r10,r10,0,16,1  /* Clear TID */
+4: mtspr   SPRN_MAS1,r10
 
/* Now, we need to walk the page tables. First check if we are in
 * range.
@@ -410,12 +457,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_SMT)
rfi
 
 tlb_miss_kernel_e6500:
-   mfspr   r10,SPRN_MAS1
ld  r14,PACA_KERNELPGD(r13)
-   cmpldi  cr0,r15,8   /* Check for vmalloc region */
-   rlwinm  r10,r10,0,16,1  /* Clear TID */
-   mtspr   SPRN_MAS1,r10
-   beq+tlb_miss_common_e6500
+   cmpldi  cr1,r15,8   /* Check for vmalloc region */
+   beq+cr1,tlb_miss_common_e6500
 
 tlb_miss_fault_e6500:
tlb_unlock_e6500
-- 
1.9.1

--
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] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
 On 17.07.14 18:27, Alexander Graf wrote:
 
  On 17.07.14 18:24, bharat.bhus...@freescale.com wrote:
 
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 17, 2014 9:41 PM
  To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org
  Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
  Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
 
 
  On 16.07.14 08:02, Bharat Bhushan wrote:
  SPRG3 is guest accessible and SPRG3 can be clobbered by host or
  another guest, So this need to be restored when loading guest state.

SPRG3 is not guest writeable.  We should be doing this so that guest
reads of SPRG3 through the alternative read-only SPR work, not because
SPRG3 can be clobbered by host or another guest.

 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
 arch/powerpc/kvm/booke_interrupts.S | 2 ++
 1 file changed, 2 insertions(+)
 
  diff --git a/arch/powerpc/kvm/booke_interrupts.S
  b/arch/powerpc/kvm/booke_interrupts.S
  index 2c6deb5ef..0d3403f 100644
  --- a/arch/powerpc/kvm/booke_interrupts.S
  +++ b/arch/powerpc/kvm/booke_interrupts.S
  @@ -459,6 +459,8 @@ lightweight_exit:
  * written directly to the shared area, so we
  * need to reload them here with the guest's values.
  */
  +PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
  +mtsprSPRN_SPRG3, r3
  We also need to restore it when resuming the host, no?
  I do not think host expect some meaningful value when returning from 
  guest, same true for SPRG4-7.
  So there seems no reason to save host values and restore them.

Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
Alex points out.

  Hmm - arch/powerpc/include/asm/reg.h says:
 
   * All 32-bit:
   *  - SPRG3 current thread_info pointer
   *(virtual on BookE, physical on others)
 
  but I can indeed find no trace of usage anywhere. This at least needs 
  to go into the patch description.
 
 Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so 
 incredibly important that I have no idea how we could possibly run 
 without switching the host value back in very early. And even then our 
 interrupt handlers wouldn't work anymore.
 
 This is more complicated :).

To make this work we need to avoid SPRG3 as well, or at least avoid
using it for something needed prior to DO_KVM.

We also need to update the documentation in reg.h to reflect the fact
that we don't use SPRG4-7 anymore on e500.

-Scott


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

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
 On 18.07.14 02:28, Scott Wood wrote:
  On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
  On 17.07.14 18:27, Alexander Graf wrote:
  On 17.07.14 18:24, bharat.bhus...@freescale.com wrote:
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 17, 2014 9:41 PM
  To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org
  Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
  Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
 
 
  On 16.07.14 08:02, Bharat Bhushan wrote:
  SPRG3 is guest accessible and SPRG3 can be clobbered by host or
  another guest, So this need to be restored when loading guest state.
  SPRG3 is not guest writeable.  We should be doing this so that guest
  reads of SPRG3 through the alternative read-only SPR work, not because
  SPRG3 can be clobbered by host or another guest.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  arch/powerpc/kvm/booke_interrupts.S | 2 ++
  1 file changed, 2 insertions(+)
 
  diff --git a/arch/powerpc/kvm/booke_interrupts.S
  b/arch/powerpc/kvm/booke_interrupts.S
  index 2c6deb5ef..0d3403f 100644
  --- a/arch/powerpc/kvm/booke_interrupts.S
  +++ b/arch/powerpc/kvm/booke_interrupts.S
  @@ -459,6 +459,8 @@ lightweight_exit:
   * written directly to the shared area, so we
   * need to reload them here with the guest's values.
   */
  +PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
  +mtsprSPRN_SPRG3, r3
  We also need to restore it when resuming the host, no?
  I do not think host expect some meaningful value when returning from
  guest, same true for SPRG4-7.
  So there seems no reason to save host values and restore them.
  Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
  Alex points out.
 
  Hmm - arch/powerpc/include/asm/reg.h says:
 
* All 32-bit:
*  - SPRG3 current thread_info pointer
*(virtual on BookE, physical on others)
 
  but I can indeed find no trace of usage anywhere. This at least needs
  to go into the patch description.
  Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
  incredibly important that I have no idea how we could possibly run
  without switching the host value back in very early. And even then our
  interrupt handlers wouldn't work anymore.
 
  This is more complicated :).
  To make this work we need to avoid SPRG3 as well, or at least avoid
  using it for something needed prior to DO_KVM.
 
  We also need to update the documentation in reg.h to reflect the fact
  that we don't use SPRG4-7 anymore on e500.
 
 I would personally prefer if we claim SPRG3R as unsupported on e500v2 
 until we find someone who actually uses it. There's a good chance we'd 
 start jumping through a lot of hoops and reduce overall performance for 
 no real-world gain today.

The same problem applies to e500mc.

-Scott


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

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
 On 18.07.14 02:36, Scott Wood wrote:
  On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
  On 18.07.14 02:28, Scott Wood wrote:
  On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
  On 17.07.14 18:27, Alexander Graf wrote:
  On 17.07.14 18:24, bharat.bhus...@freescale.com wrote:
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 17, 2014 9:41 PM
  To: Bhushan Bharat-R65777; kvm-...@vger.kernel.org
  Cc: kvm@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
  Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering 
  guest
 
 
  On 16.07.14 08:02, Bharat Bhushan wrote:
  SPRG3 is guest accessible and SPRG3 can be clobbered by host or
  another guest, So this need to be restored when loading guest state.
  SPRG3 is not guest writeable.  We should be doing this so that guest
  reads of SPRG3 through the alternative read-only SPR work, not because
  SPRG3 can be clobbered by host or another guest.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
   arch/powerpc/kvm/booke_interrupts.S | 2 ++
   1 file changed, 2 insertions(+)
 
  diff --git a/arch/powerpc/kvm/booke_interrupts.S
  b/arch/powerpc/kvm/booke_interrupts.S
  index 2c6deb5ef..0d3403f 100644
  --- a/arch/powerpc/kvm/booke_interrupts.S
  +++ b/arch/powerpc/kvm/booke_interrupts.S
  @@ -459,6 +459,8 @@ lightweight_exit:
* written directly to the shared area, so we
* need to reload them here with the guest's values.
*/
  +PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
  +mtsprSPRN_SPRG3, r3
  We also need to restore it when resuming the host, no?
  I do not think host expect some meaningful value when returning from
  guest, same true for SPRG4-7.
  So there seems no reason to save host values and restore them.
  Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
  Alex points out.
 
  Hmm - arch/powerpc/include/asm/reg.h says:
 
 * All 32-bit:
 *  - SPRG3 current thread_info pointer
 *(virtual on BookE, physical on others)
 
  but I can indeed find no trace of usage anywhere. This at least needs
  to go into the patch description.
  Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
  incredibly important that I have no idea how we could possibly run
  without switching the host value back in very early. And even then our
  interrupt handlers wouldn't work anymore.
 
  This is more complicated :).
  To make this work we need to avoid SPRG3 as well, or at least avoid
  using it for something needed prior to DO_KVM.
 
  We also need to update the documentation in reg.h to reflect the fact
  that we don't use SPRG4-7 anymore on e500.
  I would personally prefer if we claim SPRG3R as unsupported on e500v2
  until we find someone who actually uses it. There's a good chance we'd
  start jumping through a lot of hoops and reduce overall performance for
  no real-world gain today.
  The same problem applies to e500mc.
 
 There we have SPRN_GSPRG3, no?

Oh, right.

Since it's only a problem for PR-mode, it can be fixed without needing
to avoid SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only
need to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
ffe129ecd79779221fdb03305049ec8b5a8beb0f).

And if we decide it's not worthwhile and don't revert that commit, we
should at least remove the comment that Under KVM, the host SPRG1 is
used to point to the current VCPU data structure...

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6 v2] kvm: ppc: bookehv: Added wrapper macros for shadow registers

[Scott Wood]

On Thu, 2014-07-17 at 17:01 +0530, Bharat Bhushan wrote:
 There are shadow registers like, GSPRG[0-3], GSRR0, GSRR1 etc on
 BOOKE-HV and these shadow registers are guest accessible.
 So these shadow registers needs to be updated on BOOKE-HV.
 This patch adds new macro for get/set helper of shadow register .
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 v1-v2
  - Fix compilation for book3s (separate macro etc)
 
  arch/powerpc/include/asm/kvm_ppc.h | 44 
 +++---
  1 file changed, 36 insertions(+), 8 deletions(-)
 
 diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
 b/arch/powerpc/include/asm/kvm_ppc.h
 index f3f7611..7646994 100644
 --- a/arch/powerpc/include/asm/kvm_ppc.h
 +++ b/arch/powerpc/include/asm/kvm_ppc.h
 @@ -475,8 +475,20 @@ static inline bool kvmppc_shared_big_endian(struct 
 kvm_vcpu *vcpu)
  #endif
  }
  
 +#define SPRNG_WRAPPER_GET(reg, e500hv_spr)   \
 +static inline ulong kvmppc_get_##reg(struct kvm_vcpu *vcpu)  \
 +{\
 + return mfspr(e500hv_spr);   \
 +}\
 +
 +#define SPRNG_WRAPPER_SET(reg, e500hv_spr)   \
 +static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, ulong val)
 \
 +{\
 + mtspr(e500hv_spr, val); \
 +}\

Why e500hv rather than bookehv?

-Scott


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

Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
 On 18.07.14 02:28, Scott Wood wrote:
  On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
  On 17.07.14 18:27, Alexander Graf wrote:
  On 17.07.14 18:24, bharat.bhus...@freescale.com wrote:
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 17, 2014 9:41 PM
  To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
  Cc: k...@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
  Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering guest
 
 
  On 16.07.14 08:02, Bharat Bhushan wrote:
  SPRG3 is guest accessible and SPRG3 can be clobbered by host or
  another guest, So this need to be restored when loading guest state.
  SPRG3 is not guest writeable.  We should be doing this so that guest
  reads of SPRG3 through the alternative read-only SPR work, not because
  SPRG3 can be clobbered by host or another guest.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
  arch/powerpc/kvm/booke_interrupts.S | 2 ++
  1 file changed, 2 insertions(+)
 
  diff --git a/arch/powerpc/kvm/booke_interrupts.S
  b/arch/powerpc/kvm/booke_interrupts.S
  index 2c6deb5ef..0d3403f 100644
  --- a/arch/powerpc/kvm/booke_interrupts.S
  +++ b/arch/powerpc/kvm/booke_interrupts.S
  @@ -459,6 +459,8 @@ lightweight_exit:
   * written directly to the shared area, so we
   * need to reload them here with the guest's values.
   */
  +PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
  +mtsprSPRN_SPRG3, r3
  We also need to restore it when resuming the host, no?
  I do not think host expect some meaningful value when returning from
  guest, same true for SPRG4-7.
  So there seems no reason to save host values and restore them.
  Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
  Alex points out.
 
  Hmm - arch/powerpc/include/asm/reg.h says:
 
* All 32-bit:
*  - SPRG3 current thread_info pointer
*(virtual on BookE, physical on others)
 
  but I can indeed find no trace of usage anywhere. This at least needs
  to go into the patch description.
  Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
  incredibly important that I have no idea how we could possibly run
  without switching the host value back in very early. And even then our
  interrupt handlers wouldn't work anymore.
 
  This is more complicated :).
  To make this work we need to avoid SPRG3 as well, or at least avoid
  using it for something needed prior to DO_KVM.
 
  We also need to update the documentation in reg.h to reflect the fact
  that we don't use SPRG4-7 anymore on e500.
 
 I would personally prefer if we claim SPRG3R as unsupported on e500v2 
 until we find someone who actually uses it. There's a good chance we'd 
 start jumping through a lot of hoops and reduce overall performance for 
 no real-world gain today.

The same problem applies to e500mc.

-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] kvm: ppc: booke: Restore SPRG3 when entering guest

[Scott Wood]

On Fri, 2014-07-18 at 02:37 +0200, Alexander Graf wrote:
 On 18.07.14 02:36, Scott Wood wrote:
  On Fri, 2014-07-18 at 02:33 +0200, Alexander Graf wrote:
  On 18.07.14 02:28, Scott Wood wrote:
  On Thu, 2014-07-17 at 18:29 +0200, Alexander Graf wrote:
  On 17.07.14 18:27, Alexander Graf wrote:
  On 17.07.14 18:24, bharat.bhus...@freescale.com wrote:
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 17, 2014 9:41 PM
  To: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org
  Cc: k...@vger.kernel.org; Wood Scott-B07421; Yoder Stuart-B08248
  Subject: Re: [PATCH] kvm: ppc: booke: Restore SPRG3 when entering 
  guest
 
 
  On 16.07.14 08:02, Bharat Bhushan wrote:
  SPRG3 is guest accessible and SPRG3 can be clobbered by host or
  another guest, So this need to be restored when loading guest state.
  SPRG3 is not guest writeable.  We should be doing this so that guest
  reads of SPRG3 through the alternative read-only SPR work, not because
  SPRG3 can be clobbered by host or another guest.
 
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  ---
   arch/powerpc/kvm/booke_interrupts.S | 2 ++
   1 file changed, 2 insertions(+)
 
  diff --git a/arch/powerpc/kvm/booke_interrupts.S
  b/arch/powerpc/kvm/booke_interrupts.S
  index 2c6deb5ef..0d3403f 100644
  --- a/arch/powerpc/kvm/booke_interrupts.S
  +++ b/arch/powerpc/kvm/booke_interrupts.S
  @@ -459,6 +459,8 @@ lightweight_exit:
* written directly to the shared area, so we
* need to reload them here with the guest's values.
*/
  +PPC_LD(r3, VCPU_SHARED_SPRG3, r5)
  +mtsprSPRN_SPRG3, r3
  We also need to restore it when resuming the host, no?
  I do not think host expect some meaningful value when returning from
  guest, same true for SPRG4-7.
  So there seems no reason to save host values and restore them.
  Linux no longer uses SPRG4-7 for itself.  That is not true of SPRG3, as
  Alex points out.
 
  Hmm - arch/powerpc/include/asm/reg.h says:
 
 * All 32-bit:
 *  - SPRG3 current thread_info pointer
 *(virtual on BookE, physical on others)
 
  but I can indeed find no trace of usage anywhere. This at least needs
  to go into the patch description.
  Bah - it obviously is used. It's SPRN_SPRG_THREAD. And it's so
  incredibly important that I have no idea how we could possibly run
  without switching the host value back in very early. And even then our
  interrupt handlers wouldn't work anymore.
 
  This is more complicated :).
  To make this work we need to avoid SPRG3 as well, or at least avoid
  using it for something needed prior to DO_KVM.
 
  We also need to update the documentation in reg.h to reflect the fact
  that we don't use SPRG4-7 anymore on e500.
  I would personally prefer if we claim SPRG3R as unsupported on e500v2
  until we find someone who actually uses it. There's a good chance we'd
  start jumping through a lot of hoops and reduce overall performance for
  no real-world gain today.
  The same problem applies to e500mc.
 
 There we have SPRN_GSPRG3, no?

Oh, right.

Since it's only a problem for PR-mode, it can be fixed without needing
to avoid SPRG3 entirely, since PR-mode doesn't use DO_KVM.  We'd only
need to avoid using SPRG_THREAD in __KVM_HANDLER (i.e. revert commit
ffe129ecd79779221fdb03305049ec8b5a8beb0f).

And if we decide it's not worthwhile and don't revert that commit, we
should at least remove the comment that Under KVM, the host SPRG1 is
used to point to the current VCPU data structure...

-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/6 v2] kvm: ppc: bookehv: Added wrapper macros for shadow registers

[Scott Wood]

On Thu, 2014-07-17 at 17:01 +0530, Bharat Bhushan wrote:
 There are shadow registers like, GSPRG[0-3], GSRR0, GSRR1 etc on
 BOOKE-HV and these shadow registers are guest accessible.
 So these shadow registers needs to be updated on BOOKE-HV.
 This patch adds new macro for get/set helper of shadow register .
 
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 ---
 v1-v2
  - Fix compilation for book3s (separate macro etc)
 
  arch/powerpc/include/asm/kvm_ppc.h | 44 
 +++---
  1 file changed, 36 insertions(+), 8 deletions(-)
 
 diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
 b/arch/powerpc/include/asm/kvm_ppc.h
 index f3f7611..7646994 100644
 --- a/arch/powerpc/include/asm/kvm_ppc.h
 +++ b/arch/powerpc/include/asm/kvm_ppc.h
 @@ -475,8 +475,20 @@ static inline bool kvmppc_shared_big_endian(struct 
 kvm_vcpu *vcpu)
  #endif
  }
  
 +#define SPRNG_WRAPPER_GET(reg, e500hv_spr)   \
 +static inline ulong kvmppc_get_##reg(struct kvm_vcpu *vcpu)  \
 +{\
 + return mfspr(e500hv_spr);   \
 +}\
 +
 +#define SPRNG_WRAPPER_SET(reg, e500hv_spr)   \
 +static inline void kvmppc_set_##reg(struct kvm_vcpu *vcpu, ulong val)
 \
 +{\
 + mtspr(e500hv_spr, val); \
 +}\

Why e500hv rather than bookehv?

-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: [RFC PATCH 2/4] KVM: PPC: Book3E: Handle LRAT error exception

[Scott Wood]

On Fri, 2014-07-04 at 10:15 +0200, Alexander Graf wrote:
 On 03.07.14 16:45, Mihai Caraman wrote:
  diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
  index a192975..ab1077f 100644
  --- a/arch/powerpc/kvm/booke.c
  +++ b/arch/powerpc/kvm/booke.c
  @@ -1286,6 +1286,46 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
  kvm_vcpu *vcpu,
  break;
  }

  +#ifdef CONFIG_KVM_BOOKE_HV
  +   case BOOKE_INTERRUPT_LRAT_ERROR:
  +   {
  +   gfn_t gfn;
  +
  +   /*
  +* Guest TLB management instructions (EPCR.DGTMI == 0) is not
  +* supported for now
  +*/
  +   if (!(vcpu-arch.fault_esr  ESR_PT)) {
  +   WARN(1, %s: Guest TLB management instructions not 
  supported!\n, __func__);
 
 Wouldn't this allow a guest to flood the host's kernel log?

It shouldn't be possible for this to happen, since the host will never
set EPCR[DGTMI] -- but yes, it should be WARN_ONCE or ratelimited.

  +{
  +   int this, next;
  +
  +   this = local_paca-tcd.lrat_next;
  +   next = (this + 1) % local_paca-tcd.lrat_max;
 
 Can we assume that lrat_max is always a power of 2? IIRC modulo 
 functions with variables can be quite expensive. So if we can instead do
 
next = (this + 1)  local_paca-tcd.lrat_mask;
 
 we should be faster and not rely on division helpers.

Architecturally we can't assume that, though it's true on the only
existing implementation.

Why not do something similar to what is done for tlb1:

unsigned int sesel = vcpu_e500-host_tlb1_nv++;

if (unlikely(vcpu_e500-host_tlb1_nv = tlb1_max_shadow_size()))
vcpu_e500-host_tlb1_nv = 0;

...and while we're at it, use local_paca-tcd for tlb1 as well (except
on 32-bit).

Also, please use get_paca() rather than local_paca so that the
preemption-disabled check is retained.

  +void write_host_lrate(int tsize, gfn_t gfn, unsigned long pfn, uint32_t 
  lpid,
  + int valid, int lrat_entry)
  +{
  +   struct kvm_book3e_206_tlb_entry stlbe;
  +   int esel = lrat_entry;
  +   unsigned long flags;
  +
  +   stlbe.mas1 = (valid ? MAS1_VALID : 0) | MAS1_TSIZE(tsize);
  +   stlbe.mas2 = ((u64)gfn  PAGE_SHIFT);
  +   stlbe.mas7_3 = ((u64)pfn  PAGE_SHIFT);
  +   stlbe.mas8 = MAS8_TGS | lpid;
  +
  +   local_irq_save(flags);
  +   /* book3e_tlb_lock(); */
 
 Hm?

Indeed.

  +
  +   if (esel == -1)
  +   esel = lrat_next();
  +   __write_host_tlbe(stlbe, MAS0_ATSEL | MAS0_ESEL(esel));

Where do you call this function with lrat_entry != -1?  Why rename it to
esel at function entry?

  +   down_read(current-mm-mmap_sem);
  +   vma = find_vma(current-mm, hva);
  +   if (vma  (hva = vma-vm_start)) {
  +   psize = vma_kernel_pagesize(vma);
  +   } else {
  +   pr_err_ratelimited(%s: couldn't find virtual memory address 
  for gfn %lx!\n, __func__, (long)gfn);

While output strings should not be linewrapped, the arguments that come
after the long string should be.

-Scott


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

Re: [RFC PATCH 3/4] KVM: PPC: e500: TLB emulation for IND entries

[Scott Wood]

On Thu, 2014-07-03 at 17:45 +0300, Mihai Caraman wrote:
 Handle indirect entries (IND) in TLB emulation code. Translation size of IND
 entries differ from the size of referred Page Tables (Linux guests now use IND
 of 2MB for 4KB PTs) and this require careful tweak of the existing logic.
 
 TLB search emulation requires additional search in HW TLB0 (since these 
 entries
 are directly added by HTW) and found entries shoud be presented to the guest 
 with
 RPN changed from PFN to GFN. There might be more GFNs pointing to the same 
 PFN so
 the only way to get the corresponding GFN is to search it in guest's PTE. If 
 IND
 entry for the corresponding PT is not available just invalidate guest's ea and
 report a tlbsx miss. This patch only implements the invalidation and let a 
 TODO
 note for searching HW TLB0.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 ---
  arch/powerpc/include/asm/mmu-book3e.h |  2 +
  arch/powerpc/kvm/e500.h   | 81 
 ---
  arch/powerpc/kvm/e500_mmu.c   | 78 +++--
  arch/powerpc/kvm/e500_mmu_host.c  | 31 --
  arch/powerpc/kvm/e500mc.c | 53 +--
  5 files changed, 211 insertions(+), 34 deletions(-)

Please look at erratum A-008139.  A patch to work around it for the main
Linux tablewalk code is forthcoming.  You need to make sure to never
overwrite an indirect entry with tlbwe -- always use tlbilx.

 @@ -286,6 +336,22 @@ void kvmppc_e500_tlbil_one(struct kvmppc_vcpu_e500 
 *vcpu_e500,
  void kvmppc_e500_tlbil_all(struct kvmppc_vcpu_e500 *vcpu_e500);
  
  #ifdef CONFIG_KVM_BOOKE_HV
 +void inval_tlb_on_host(struct kvm_vcpu *vcpu, int type, int pid);
 +
 +void inval_ea_on_host(struct kvm_vcpu *vcpu, gva_t ea, int pid, int sas,
 +   int sind);
 +#else
 +/* TLB is fully virtualized */
 +static inline void inval_tlb_on_host(struct kvm_vcpu *vcpu,
 +  int type, int pid)
 +{}
 +
 +static inline void inval_ea_on_host(struct kvm_vcpu *vcpu, gva_t ea, int pid,
 + int sas, int sind)
 +{}
 +#endif

Document exactly what these do, and explain why it's conceptually a
separate API from kvmppc_e500_tlbil_all/one(), and why TLB is fully
virtualized explains why we never need to do this on non-HV.

 @@ -290,21 +298,32 @@ static void tlbilx_all(struct kvmppc_vcpu_e500 
 *vcpu_e500, int tlbsel,
   kvmppc_e500_gtlbe_invalidate(vcpu_e500, tlbsel, esel);
   }
   }
 +
 + /* Invalidate enties added by HTW */
 + if (has_feature(vcpu_e500-vcpu, VCPU_FTR_MMU_V2)  (!sind))

Unnecessary parens.

 @@ -368,6 +388,23 @@ int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, gva_t 
 ea)
   } else {
   int victim;
  
 + if (has_feature(vcpu, VCPU_FTR_MMU_V2) 
 + get_cur_sind(vcpu) != 1) {
 + /*

Never align the continuation line of an if or loop with the indentation
of the if/loop body.

get_cur_sind(vcpu) == 0 would be more natural, and probably slightly
faster.

 +  * TLB0 entries are not cached in KVM being written
 +  * directly by HTW. TLB0 entry found in HW TLB0 needs
 +  * to be presented to the guest with RPN changed from
 +  * PFN to GFN. There might be more GFNs pointing to the
 +  * same PFN so the only way to get the corresponding GFN
 +  * is to search it in guest's PTE. If IND entry for the
 +  * corresponding PT is not available just invalidate
 +  * guest's ea and report a tlbsx miss.
 +  *
 +  * TODO: search ea in HW TLB0
 +  */
 + inval_ea_on_host(vcpu, ea, pid, as, 0);

What if the guest is in a loop where it does an access followed by a
tlbsx, looping back if the tlbsx reports no entry (e.g. an exception
handler that wants to emulate retrying the faulting instruction on a
tlbsx miss)?  The architecture allows hypervisors to invalidate at
arbitrary times, but we should avoid doing this in ways that can block
forward progress.

How likely is it really that we have multiple GFNs per PFN?  How bad is
it really if we return an arbitrary matching GFN in such a case?

 @@ -516,12 +527,17 @@ static inline int kvmppc_e500_shadow_map(struct 
 kvmppc_vcpu_e500 *vcpu_e500,
  
   tsize = (gtlbe-mas1  MAS1_TSIZE_MASK) 
   MAS1_TSIZE_SHIFT;
 + if (ind)
 + tsize -= BOOK3E_PAGESZ_4K + 7;
  
   /*
* e500 doesn't implement the lowest tsize bit,
* or 1K pages.
*/
 - tsize = max(BOOK3E_PAGESZ_4K, tsize  ~1);
 + if 

Re: [RFC PATCH 2/4] KVM: PPC: Book3E: Handle LRAT error exception

[Scott Wood]

On Fri, 2014-07-04 at 10:15 +0200, Alexander Graf wrote:
 On 03.07.14 16:45, Mihai Caraman wrote:
  diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
  index a192975..ab1077f 100644
  --- a/arch/powerpc/kvm/booke.c
  +++ b/arch/powerpc/kvm/booke.c
  @@ -1286,6 +1286,46 @@ int kvmppc_handle_exit(struct kvm_run *run, struct 
  kvm_vcpu *vcpu,
  break;
  }

  +#ifdef CONFIG_KVM_BOOKE_HV
  +   case BOOKE_INTERRUPT_LRAT_ERROR:
  +   {
  +   gfn_t gfn;
  +
  +   /*
  +* Guest TLB management instructions (EPCR.DGTMI == 0) is not
  +* supported for now
  +*/
  +   if (!(vcpu-arch.fault_esr  ESR_PT)) {
  +   WARN(1, %s: Guest TLB management instructions not 
  supported!\n, __func__);
 
 Wouldn't this allow a guest to flood the host's kernel log?

It shouldn't be possible for this to happen, since the host will never
set EPCR[DGTMI] -- but yes, it should be WARN_ONCE or ratelimited.

  +{
  +   int this, next;
  +
  +   this = local_paca-tcd.lrat_next;
  +   next = (this + 1) % local_paca-tcd.lrat_max;
 
 Can we assume that lrat_max is always a power of 2? IIRC modulo 
 functions with variables can be quite expensive. So if we can instead do
 
next = (this + 1)  local_paca-tcd.lrat_mask;
 
 we should be faster and not rely on division helpers.

Architecturally we can't assume that, though it's true on the only
existing implementation.

Why not do something similar to what is done for tlb1:

unsigned int sesel = vcpu_e500-host_tlb1_nv++;

if (unlikely(vcpu_e500-host_tlb1_nv = tlb1_max_shadow_size()))
vcpu_e500-host_tlb1_nv = 0;

...and while we're at it, use local_paca-tcd for tlb1 as well (except
on 32-bit).

Also, please use get_paca() rather than local_paca so that the
preemption-disabled check is retained.

  +void write_host_lrate(int tsize, gfn_t gfn, unsigned long pfn, uint32_t 
  lpid,
  + int valid, int lrat_entry)
  +{
  +   struct kvm_book3e_206_tlb_entry stlbe;
  +   int esel = lrat_entry;
  +   unsigned long flags;
  +
  +   stlbe.mas1 = (valid ? MAS1_VALID : 0) | MAS1_TSIZE(tsize);
  +   stlbe.mas2 = ((u64)gfn  PAGE_SHIFT);
  +   stlbe.mas7_3 = ((u64)pfn  PAGE_SHIFT);
  +   stlbe.mas8 = MAS8_TGS | lpid;
  +
  +   local_irq_save(flags);
  +   /* book3e_tlb_lock(); */
 
 Hm?

Indeed.

  +
  +   if (esel == -1)
  +   esel = lrat_next();
  +   __write_host_tlbe(stlbe, MAS0_ATSEL | MAS0_ESEL(esel));

Where do you call this function with lrat_entry != -1?  Why rename it to
esel at function entry?

  +   down_read(current-mm-mmap_sem);
  +   vma = find_vma(current-mm, hva);
  +   if (vma  (hva = vma-vm_start)) {
  +   psize = vma_kernel_pagesize(vma);
  +   } else {
  +   pr_err_ratelimited(%s: couldn't find virtual memory address 
  for gfn %lx!\n, __func__, (long)gfn);

While output strings should not be linewrapped, the arguments that come
after the long string should be.

-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: [RFC PATCH 3/4] KVM: PPC: e500: TLB emulation for IND entries

[Scott Wood]

On Thu, 2014-07-03 at 17:45 +0300, Mihai Caraman wrote:
 Handle indirect entries (IND) in TLB emulation code. Translation size of IND
 entries differ from the size of referred Page Tables (Linux guests now use IND
 of 2MB for 4KB PTs) and this require careful tweak of the existing logic.
 
 TLB search emulation requires additional search in HW TLB0 (since these 
 entries
 are directly added by HTW) and found entries shoud be presented to the guest 
 with
 RPN changed from PFN to GFN. There might be more GFNs pointing to the same 
 PFN so
 the only way to get the corresponding GFN is to search it in guest's PTE. If 
 IND
 entry for the corresponding PT is not available just invalidate guest's ea and
 report a tlbsx miss. This patch only implements the invalidation and let a 
 TODO
 note for searching HW TLB0.
 
 Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
 ---
  arch/powerpc/include/asm/mmu-book3e.h |  2 +
  arch/powerpc/kvm/e500.h   | 81 
 ---
  arch/powerpc/kvm/e500_mmu.c   | 78 +++--
  arch/powerpc/kvm/e500_mmu_host.c  | 31 --
  arch/powerpc/kvm/e500mc.c | 53 +--
  5 files changed, 211 insertions(+), 34 deletions(-)

Please look at erratum A-008139.  A patch to work around it for the main
Linux tablewalk code is forthcoming.  You need to make sure to never
overwrite an indirect entry with tlbwe -- always use tlbilx.

 @@ -286,6 +336,22 @@ void kvmppc_e500_tlbil_one(struct kvmppc_vcpu_e500 
 *vcpu_e500,
  void kvmppc_e500_tlbil_all(struct kvmppc_vcpu_e500 *vcpu_e500);
  
  #ifdef CONFIG_KVM_BOOKE_HV
 +void inval_tlb_on_host(struct kvm_vcpu *vcpu, int type, int pid);
 +
 +void inval_ea_on_host(struct kvm_vcpu *vcpu, gva_t ea, int pid, int sas,
 +   int sind);
 +#else
 +/* TLB is fully virtualized */
 +static inline void inval_tlb_on_host(struct kvm_vcpu *vcpu,
 +  int type, int pid)
 +{}
 +
 +static inline void inval_ea_on_host(struct kvm_vcpu *vcpu, gva_t ea, int pid,
 + int sas, int sind)
 +{}
 +#endif

Document exactly what these do, and explain why it's conceptually a
separate API from kvmppc_e500_tlbil_all/one(), and why TLB is fully
virtualized explains why we never need to do this on non-HV.

 @@ -290,21 +298,32 @@ static void tlbilx_all(struct kvmppc_vcpu_e500 
 *vcpu_e500, int tlbsel,
   kvmppc_e500_gtlbe_invalidate(vcpu_e500, tlbsel, esel);
   }
   }
 +
 + /* Invalidate enties added by HTW */
 + if (has_feature(vcpu_e500-vcpu, VCPU_FTR_MMU_V2)  (!sind))

Unnecessary parens.

 @@ -368,6 +388,23 @@ int kvmppc_e500_emul_tlbsx(struct kvm_vcpu *vcpu, gva_t 
 ea)
   } else {
   int victim;
  
 + if (has_feature(vcpu, VCPU_FTR_MMU_V2) 
 + get_cur_sind(vcpu) != 1) {
 + /*

Never align the continuation line of an if or loop with the indentation
of the if/loop body.

get_cur_sind(vcpu) == 0 would be more natural, and probably slightly
faster.

 +  * TLB0 entries are not cached in KVM being written
 +  * directly by HTW. TLB0 entry found in HW TLB0 needs
 +  * to be presented to the guest with RPN changed from
 +  * PFN to GFN. There might be more GFNs pointing to the
 +  * same PFN so the only way to get the corresponding GFN
 +  * is to search it in guest's PTE. If IND entry for the
 +  * corresponding PT is not available just invalidate
 +  * guest's ea and report a tlbsx miss.
 +  *
 +  * TODO: search ea in HW TLB0
 +  */
 + inval_ea_on_host(vcpu, ea, pid, as, 0);

What if the guest is in a loop where it does an access followed by a
tlbsx, looping back if the tlbsx reports no entry (e.g. an exception
handler that wants to emulate retrying the faulting instruction on a
tlbsx miss)?  The architecture allows hypervisors to invalidate at
arbitrary times, but we should avoid doing this in ways that can block
forward progress.

How likely is it really that we have multiple GFNs per PFN?  How bad is
it really if we return an arbitrary matching GFN in such a case?

 @@ -516,12 +527,17 @@ static inline int kvmppc_e500_shadow_map(struct 
 kvmppc_vcpu_e500 *vcpu_e500,
  
   tsize = (gtlbe-mas1  MAS1_TSIZE_MASK) 
   MAS1_TSIZE_SHIFT;
 + if (ind)
 + tsize -= BOOK3E_PAGESZ_4K + 7;
  
   /*
* e500 doesn't implement the lowest tsize bit,
* or 1K pages.
*/
 - tsize = max(BOOK3E_PAGESZ_4K, tsize  ~1);
 + if 

Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 03, 2014 3:21 PM
  To: Caraman Mihai Claudiu-B02008; kvm-...@vger.kernel.org
  Cc: kvm@vger.kernel.org; linuxppc-...@lists.ozlabs.org
  Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
  SPE/FP/AltiVec int numbers
  
  
  On 30.06.14 17:34, Mihai Caraman wrote:
   Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
   which share the same interrupt numbers.
  
   Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
   ---
   v2:
 - remove outdated definitions
  
 arch/powerpc/include/asm/kvm_asm.h|  8 
 arch/powerpc/kvm/booke.c  | 17 +
 arch/powerpc/kvm/booke.h  |  4 ++--
 arch/powerpc/kvm/booke_interrupts.S   |  9 +
 arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
 arch/powerpc/kvm/e500.c   | 10 ++
 arch/powerpc/kvm/e500_emulate.c   | 10 ++
 7 files changed, 30 insertions(+), 32 deletions(-)
  
   diff --git a/arch/powerpc/include/asm/kvm_asm.h
  b/arch/powerpc/include/asm/kvm_asm.h
   index 9601741..c94fd33 100644
   --- a/arch/powerpc/include/asm/kvm_asm.h
   +++ b/arch/powerpc/include/asm/kvm_asm.h
   @@ -56,14 +56,6 @@
 /* E500 */
 #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
 #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
   -/*
   - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
  defines
   - */
   -#define BOOKE_INTERRUPT_SPE_UNAVAIL
  BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
   -#define BOOKE_INTERRUPT_SPE_FP_DATA
  BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
   -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
  BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
   -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
   - BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
  
  I think I'd prefer to keep them separate.
 
 What is the reason from changing your mind from ver 1? Do you want to have
 different defines with same values (we specifically mapped them to the
 hardware interrupt numbers). We already upstreamed the necessary changes
 in the kernel. Scott, please share your opinion here.

I don't like hiding the fact that they're the same number, which could
lead to wrong code in the absence of ifdefs that strictly mutually
exclude SPE and Altivec code -- there was an instance of this with
MSR_VEC versus MSR_SPE in a previous patchset.
 
 #define BOOKE_INTERRUPT_SPE_FP_ROUND 34
 #define BOOKE_INTERRUPT_PERFORMANCE_MONITOR 35
 #define BOOKE_INTERRUPT_DOORBELL 36
   diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
   index ab62109..3c86d9b 100644
   --- a/arch/powerpc/kvm/booke.c
   +++ b/arch/powerpc/kvm/booke.c
   @@ -388,8 +388,8 @@ static int kvmppc_booke_irqprio_deliver(struct
  kvm_vcpu *vcpu,
 case BOOKE_IRQPRIO_ITLB_MISS:
 case BOOKE_IRQPRIO_SYSCALL:
 case BOOKE_IRQPRIO_FP_UNAVAIL:
   - case BOOKE_IRQPRIO_SPE_UNAVAIL:
   - case BOOKE_IRQPRIO_SPE_FP_DATA:
   + case BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL:
   + case BOOKE_IRQPRIO_SPE_FP_DATA_ALTIVEC_ASSIST:
  
  #ifdef CONFIG_KVM_E500V2
 case ...SPE:
  #else
 case ..ALTIVEC:
  #endif

I don't think that's an improvement.

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Thu, 2014-07-03 at 17:15 -0500, Scott Wood wrote:
 On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
   -Original Message-
   From: Alexander Graf [mailto:ag...@suse.de]
   Sent: Thursday, July 03, 2014 3:21 PM
   To: Caraman Mihai Claudiu-B02008; kvm-...@vger.kernel.org
   Cc: kvm@vger.kernel.org; linuxppc-...@lists.ozlabs.org
   Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
   SPE/FP/AltiVec int numbers
   
   
   On 30.06.14 17:34, Mihai Caraman wrote:
Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
which share the same interrupt numbers.
   
Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
---
v2:
  - remove outdated definitions
   
  arch/powerpc/include/asm/kvm_asm.h|  8 
  arch/powerpc/kvm/booke.c  | 17 +
  arch/powerpc/kvm/booke.h  |  4 ++--
  arch/powerpc/kvm/booke_interrupts.S   |  9 +
  arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
  arch/powerpc/kvm/e500.c   | 10 ++
  arch/powerpc/kvm/e500_emulate.c   | 10 ++
  7 files changed, 30 insertions(+), 32 deletions(-)
   
diff --git a/arch/powerpc/include/asm/kvm_asm.h
   b/arch/powerpc/include/asm/kvm_asm.h
index 9601741..c94fd33 100644
--- a/arch/powerpc/include/asm/kvm_asm.h
+++ b/arch/powerpc/include/asm/kvm_asm.h
@@ -56,14 +56,6 @@
  /* E500 */
  #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
  #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
-/*
- * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
   defines
- */
-#define BOOKE_INTERRUPT_SPE_UNAVAIL
   BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
-#define BOOKE_INTERRUPT_SPE_FP_DATA
   BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
-#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
   BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
-#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
-   
BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
   
   I think I'd prefer to keep them separate.
  
  What is the reason from changing your mind from ver 1? Do you want to have
  different defines with same values (we specifically mapped them to the
  hardware interrupt numbers). We already upstreamed the necessary changes
  in the kernel. Scott, please share your opinion here.
 
 I don't like hiding the fact that they're the same number, which could
 lead to wrong code in the absence of ifdefs that strictly mutually
 exclude SPE and Altivec code -- there was an instance of this with
 MSR_VEC versus MSR_SPE in a previous patchset. 

That said, if you want to enforce that mutual exclusion in a way that is
clear, I won't object too loudly -- but the code does look pretty
similar between the two (as well as between the two IVORs).

-Scott


--
To unsubscribe from this list: send the line unsubscribe kvm 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/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers

[Scott Wood]

On Fri, 2014-07-04 at 00:35 +0200, Alexander Graf wrote:
 On 04.07.14 00:31, Scott Wood wrote:
  On Thu, 2014-07-03 at 17:15 -0500, Scott Wood wrote:
  On Thu, 2014-07-03 at 10:25 -0500, Caraman Mihai Claudiu-B02008 wrote:
  -Original Message-
  From: Alexander Graf [mailto:ag...@suse.de]
  Sent: Thursday, July 03, 2014 3:21 PM
  To: Caraman Mihai Claudiu-B02008; kvm-...@vger.kernel.org
  Cc: kvm@vger.kernel.org; linuxppc-...@lists.ozlabs.org
  Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
  SPE/FP/AltiVec int numbers
 
 
  On 30.06.14 17:34, Mihai Caraman wrote:
  Use common BOOKE_IRQPRIO and BOOKE_INTERRUPT defines for SPE/FP/AltiVec
  which share the same interrupt numbers.
 
  Signed-off-by: Mihai Caraman mihai.cara...@freescale.com
  ---
  v2:
 - remove outdated definitions
 
 arch/powerpc/include/asm/kvm_asm.h|  8 
 arch/powerpc/kvm/booke.c  | 17 +
 arch/powerpc/kvm/booke.h  |  4 ++--
 arch/powerpc/kvm/booke_interrupts.S   |  9 +
 arch/powerpc/kvm/bookehv_interrupts.S |  4 ++--
 arch/powerpc/kvm/e500.c   | 10 ++
 arch/powerpc/kvm/e500_emulate.c   | 10 ++
 7 files changed, 30 insertions(+), 32 deletions(-)
 
  diff --git a/arch/powerpc/include/asm/kvm_asm.h
  b/arch/powerpc/include/asm/kvm_asm.h
  index 9601741..c94fd33 100644
  --- a/arch/powerpc/include/asm/kvm_asm.h
  +++ b/arch/powerpc/include/asm/kvm_asm.h
  @@ -56,14 +56,6 @@
 /* E500 */
 #define BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL 32
 #define BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST 33
  -/*
  - * TODO: Unify 32-bit and 64-bit kernel exception handlers to use same
  defines
  - */
  -#define BOOKE_INTERRUPT_SPE_UNAVAIL
  BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
  -#define BOOKE_INTERRUPT_SPE_FP_DATA
  BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
  -#define BOOKE_INTERRUPT_ALTIVEC_UNAVAIL
  BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL
  -#define BOOKE_INTERRUPT_ALTIVEC_ASSIST \
  -   
  BOOKE_INTERRUPT_SPE_FP_DATA_ALTIVEC_ASSIST
  I think I'd prefer to keep them separate.
  What is the reason from changing your mind from ver 1? Do you want to have
  different defines with same values (we specifically mapped them to the
  hardware interrupt numbers). We already upstreamed the necessary changes
  in the kernel. Scott, please share your opinion here.
  I don't like hiding the fact that they're the same number, which could
  lead to wrong code in the absence of ifdefs that strictly mutually
  exclude SPE and Altivec code -- there was an instance of this with
  MSR_VEC versus MSR_SPE in a previous patchset.
  That said, if you want to enforce that mutual exclusion in a way that is
  clear, I won't object too loudly -- but the code does look pretty
  similar between the two (as well as between the two IVORs).
 
 Yes, I want to make sure we have 2 separate code paths for SPE and 
 Altivec. No code sharing at all unless it's very generically possible.
 
 Also, which code does look pretty similar? The fact that we deflect 
 interrupts back into the guest? That's mostly boilerplate.

There's also the injection of a program check (or exiting to userspace)
when CONFIG_SPE/ALTIVEC is missing.  Not a big deal, but maybe it could
be factored into a helper function.  I like minimizing boilerplate.

-Scott


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