Re: [PATCH 11/23] KVM: PPC: Book3S PR: Allocate kvm_vcpu structs from kvm_vcpu_cache

2013-08-12 Thread Aneesh Kumar K.V 
Paul Mackerras pau...@samba.org writes:

 This makes PR KVM allocate its kvm_vcpu structs from the kvm_vcpu_cache
 rather than having them embedded in the kvmppc_vcpu_book3s struct,
 which is allocated with vzalloc.  The reason is to reduce the
 differences between PR and HV KVM in order to make is easier to have
 them coexist in one kernel binary.

 With this, the kvm_vcpu struct has a pointer to the kvmppc_vcpu_book3s
 struct.  The pointer to the kvmppc_book3s_shadow_vcpu struct has moved
 from the kvmppc_vcpu_book3s struct to the kvm_vcpu struct.

 Signed-off-by: Paul Mackerras pau...@samba.org
 ---
  arch/powerpc/include/asm/kvm_book3s.h|  4 +---
  arch/powerpc/include/asm/kvm_book3s_32.h |  2 +-
  arch/powerpc/include/asm/kvm_host.h  |  5 +
  arch/powerpc/kvm/book3s_32_mmu.c |  8 
  arch/powerpc/kvm/book3s_64_mmu.c | 11 +--
  arch/powerpc/kvm/book3s_pr.c | 29 ++---
  6 files changed, 34 insertions(+), 25 deletions(-)

 diff --git a/arch/powerpc/include/asm/kvm_book3s.h 
 b/arch/powerpc/include/asm/kvm_book3s.h
 index 322b539..1b32f6c 100644
 --- a/arch/powerpc/include/asm/kvm_book3s.h
 +++ b/arch/powerpc/include/asm/kvm_book3s.h
 @@ -70,8 +70,6 @@ struct hpte_cache {
  };
  
  struct kvmppc_vcpu_book3s {
 - struct kvm_vcpu vcpu;
 - struct kvmppc_book3s_shadow_vcpu *shadow_vcpu;
   struct kvmppc_sid_map sid_map[SID_MAP_NUM];
   struct {
   u64 esid;
 @@ -192,7 +190,7 @@ extern int kvmppc_h_pr(struct kvm_vcpu *vcpu, unsigned 
 long cmd);
  
  static inline struct kvmppc_vcpu_book3s *to_book3s(struct kvm_vcpu *vcpu)
  {
 - return container_of(vcpu, struct kvmppc_vcpu_book3s, vcpu);
 + return vcpu-arch.book3s;
  }
  
  extern void kvm_return_point(void);
 diff --git a/arch/powerpc/include/asm/kvm_book3s_32.h 
 b/arch/powerpc/include/asm/kvm_book3s_32.h
 index ce0ef6c..c720e0b 100644
 --- a/arch/powerpc/include/asm/kvm_book3s_32.h
 +++ b/arch/powerpc/include/asm/kvm_book3s_32.h
 @@ -22,7 +22,7 @@
  
  static inline struct kvmppc_book3s_shadow_vcpu *svcpu_get(struct kvm_vcpu 
 *vcpu)
  {
 - return to_book3s(vcpu)-shadow_vcpu;
 + return vcpu-arch.shadow_vcpu;
  }
  
  static inline void svcpu_put(struct kvmppc_book3s_shadow_vcpu *svcpu)
 diff --git a/arch/powerpc/include/asm/kvm_host.h 
 b/arch/powerpc/include/asm/kvm_host.h
 index c37207f..4d83972 100644
 --- a/arch/powerpc/include/asm/kvm_host.h
 +++ b/arch/powerpc/include/asm/kvm_host.h
 @@ -91,6 +91,9 @@ struct lppaca;
  struct slb_shadow;
  struct dtl_entry;
  
 +struct kvmppc_vcpu_book3s;
 +struct kvmppc_book3s_shadow_vcpu;
 +
  struct kvm_vm_stat {
   u32 remote_tlb_flush;
  };
 @@ -409,6 +412,8 @@ struct kvm_vcpu_arch {
   int slb_max;/* 1 + index of last valid entry in slb[] */
   int slb_nr; /* total number of entries in SLB */
   struct kvmppc_mmu mmu;
 + struct kvmppc_vcpu_book3s *book3s;
 + struct kvmppc_book3s_shadow_vcpu *shadow_vcpu;
  #endif

can the *shadow_vcpu be within  #ifdef CONFIG_PPC_BOOK3S_32 ? Rest of
the code access the variable under  #ifdef CONFIG_PPC_BOOK3S_32



-aneesh

--
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 v3] kvm: powerpc: use caching attributes as per linux pte

2013-08-12 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, August 10, 2013 6:35 AM
 To: Bhushan Bharat-R65777
 Cc: b...@kernel.crashing.org; ag...@suse.de; pau...@samba.org;
 k...@vger.kernel.org; kvm-ppc@vger.kernel.org; linuxppc-...@lists.ozlabs.org;
 Bhushan Bharat-R65777
 Subject: Re: [PATCH 6/6 v3] kvm: powerpc: use caching attributes as per linux
 pte
 
 On Tue, 2013-08-06 at 17:01 +0530, Bharat Bhushan wrote:
  @@ -449,7 +446,16 @@ static inline int kvmppc_e500_shadow_map(struct
 kvmppc_vcpu_e500 *vcpu_e500,
  gvaddr = ~((tsize_pages  PAGE_SHIFT) - 1);
  }
 
  -   kvmppc_e500_ref_setup(ref, gtlbe, pfn);
  +   pgdir = vcpu_e500-vcpu.arch.pgdir;
  +   ptep = lookup_linux_pte(pgdir, hva, tsize_pages);
  +   if (pte_present(*ptep)) {
  +   wimg = (pte_val(*ptep)  PTE_WIMGE_SHIFT)  MAS2_WIMGE_MASK;
  +   } else {
  +   printk(KERN_ERR pte not present: gfn %lx, pfn %lx\n,
  +   (long)gfn, pfn);
  +   return -EINVAL;
 
 Don't let the guest spam the host kernel console by repeatedly accessing bad
 mappings (even if it requires host userspace to assist by pointing a memslot 
 at
 a bad hva).  This should at most be printk_ratelimited(), and probably just
 pr_debug().  It should also have __func__ context.

Very good point, I will make this printk_ratelimited() in this patch. And 
convert this and other error prints to pr_debug() when we will send machine 
check on error in this flow.

 
 Also, I don't see the return value getting checked (the immediate callers 
 check
 it and propogate the error, but kvmppc_mmu_map() doesn't).
 We want to send a machine check to the guest if this happens (or possibly exit
 to userspace since it indicates a bad memslot, not just a guest bug).  We 
 don't
 want to just silently retry over and over.

I completely agree with you, but this was something already missing (error 
return by this function is nothing new added in this patch), So I would like to 
take that separately.

 
 Otherwise, this series looks good to me.

Thank you. :)
-Bharat

 
 -Scott
 

N�r��yb�X��ǧv�^�)޺{.n�+jir)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥