Re: [PATCH 04/11] powerpc/kvm: Don't store values derivable from HPT order

2016-12-18 Thread David Gibson 
On Fri, Dec 16, 2016 at 11:39:26AM +0100, Thomas Huth wrote:
> On 15.12.2016 06:53, David Gibson wrote:
> > Currently the kvm_hpt_info structure stores the hashed page table's order,
> > and also the number of HPTEs it contains and a mask for its size.  The
> > last two can be easily derived from the order, so remove them and just
> > calculate them as necessary with a couple of helper inlines.
> > 
> > Signed-off-by: David Gibson 
> > ---
> [...]
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> > b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index b5799d1..fe88132 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -83,15 +83,11 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
> >  
> > kvm->arch.hpt.virt = hpt;
> > kvm->arch.hpt.order = order;
> > -   /* HPTEs are 2**4 bytes long */
> > -   kvm->arch.hpt.npte = 1ul << (order - 4);
> > -   /* 128 (2**7) bytes in each HPTEG */
> > -   kvm->arch.hpt.mask = (1ul << (order - 7)) - 1;
> >  
> > atomic64_set(>arch.mmio_update, 0);
> >  
> > /* Allocate reverse map array */
> > -   rev = vmalloc(sizeof(struct revmap_entry) * kvm->arch.hpt.npte);
> > +   rev = vmalloc(sizeof(struct revmap_entry) * 
> > kvmppc_hpt_npte(>arch.hpt));
> > if (!rev) {
> > pr_err("kvmppc_alloc_hpt: Couldn't alloc reverse map array\n");
> > goto out_freehpt;
> > @@ -194,8 +190,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
> > kvm_memory_slot *memslot,
> > if (npages > 1ul << (40 - porder))
> > npages = 1ul << (40 - porder);
> > /* Can't use more than 1 HPTE per HPTEG */
> > -   if (npages > kvm->arch.hpt.mask + 1)
> > -   npages = kvm->arch.hpt.mask + 1;
> > +   if (npages > kvmppc_hpt_mask(>arch.hpt) + 1)
> > +   npages = kvmppc_hpt_mask(>arch.hpt) + 1;
> >  
> > hp0 = HPTE_V_1TB_SEG | (VRMA_VSID << (40 - 16)) |
> > HPTE_V_BOLTED | hpte0_pgsize_encoding(psize);
> > @@ -205,7 +201,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
> > kvm_memory_slot *memslot,
> > for (i = 0; i < npages; ++i) {
> > addr = i << porder;
> > /* can't use hpt_hash since va > 64 bits */
> > -   hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25))) & 
> > kvm->arch.hpt.mask;
> > +   hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25)))
> > +   & kvmppc_hpt_mask(>arch.hpt);
> > /*
> >  * We assume that the hash table is empty and no
> >  * vcpus are using it at this stage.  Since we create
> 
> kvmppc_hpt_mask() is now called three times in kvmppc_map_vrma() ... you
> could use a local variable to store the value so that the calculation
> has only be done once here.

Well, I was assuming that inlining plus gcc's common subexpression
elimination would deal with that.

> 
> > @@ -1306,7 +1303,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> > __user *buf,
> >  
> > /* Skip uninteresting entries, i.e. clean on not-first pass */
> > if (!first_pass) {
> > -   while (i < kvm->arch.hpt.npte &&
> > +   while (i < kvmppc_hpt_npte(>arch.hpt) &&
> >!hpte_dirty(revp, hptp)) {
> > ++i;
> > hptp += 2;
> > @@ -1316,7 +1313,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> > __user *buf,
> > hdr.index = i;
> >  
> > /* Grab a series of valid entries */
> > -   while (i < kvm->arch.hpt.npte &&
> > +   while (i < kvmppc_hpt_npte(>arch.hpt) &&
> >hdr.n_valid < 0x &&
> >nb + HPTE_SIZE < count &&
> >record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
> > @@ -1332,7 +1329,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> > __user *buf,
> > ++revp;
> > }
> > /* Now skip invalid entries while we can */
> > -   while (i < kvm->arch.hpt.npte &&
> > +   while (i < kvmppc_hpt_npte(>arch.hpt) &&
> >hdr.n_invalid < 0x &&
> >record_hpte(flags, hptp, hpte, revp, 0, first_pass)) {
> > /* found an invalid entry */
> 
> Dito, you could use a local variable to store the value from
> kvmppc_hpt_npte()
> 
> Anyway, apart from these nits, patch looks fine to me, so:
> 
> Reviewed-by: Thomas Huth 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 04/11] powerpc/kvm: Don't store values derivable from HPT order

2016-12-16 Thread Thomas Huth 
On 15.12.2016 06:53, David Gibson wrote:
> Currently the kvm_hpt_info structure stores the hashed page table's order,
> and also the number of HPTEs it contains and a mask for its size.  The
> last two can be easily derived from the order, so remove them and just
> calculate them as necessary with a couple of helper inlines.
> 
> Signed-off-by: David Gibson 
> ---
[...]
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index b5799d1..fe88132 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -83,15 +83,11 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
>  
>   kvm->arch.hpt.virt = hpt;
>   kvm->arch.hpt.order = order;
> - /* HPTEs are 2**4 bytes long */
> - kvm->arch.hpt.npte = 1ul << (order - 4);
> - /* 128 (2**7) bytes in each HPTEG */
> - kvm->arch.hpt.mask = (1ul << (order - 7)) - 1;
>  
>   atomic64_set(>arch.mmio_update, 0);
>  
>   /* Allocate reverse map array */
> - rev = vmalloc(sizeof(struct revmap_entry) * kvm->arch.hpt.npte);
> + rev = vmalloc(sizeof(struct revmap_entry) * 
> kvmppc_hpt_npte(>arch.hpt));
>   if (!rev) {
>   pr_err("kvmppc_alloc_hpt: Couldn't alloc reverse map array\n");
>   goto out_freehpt;
> @@ -194,8 +190,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
> kvm_memory_slot *memslot,
>   if (npages > 1ul << (40 - porder))
>   npages = 1ul << (40 - porder);
>   /* Can't use more than 1 HPTE per HPTEG */
> - if (npages > kvm->arch.hpt.mask + 1)
> - npages = kvm->arch.hpt.mask + 1;
> + if (npages > kvmppc_hpt_mask(>arch.hpt) + 1)
> + npages = kvmppc_hpt_mask(>arch.hpt) + 1;
>  
>   hp0 = HPTE_V_1TB_SEG | (VRMA_VSID << (40 - 16)) |
>   HPTE_V_BOLTED | hpte0_pgsize_encoding(psize);
> @@ -205,7 +201,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
> kvm_memory_slot *memslot,
>   for (i = 0; i < npages; ++i) {
>   addr = i << porder;
>   /* can't use hpt_hash since va > 64 bits */
> - hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25))) & 
> kvm->arch.hpt.mask;
> + hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25)))
> + & kvmppc_hpt_mask(>arch.hpt);
>   /*
>* We assume that the hash table is empty and no
>* vcpus are using it at this stage.  Since we create

kvmppc_hpt_mask() is now called three times in kvmppc_map_vrma() ... you
could use a local variable to store the value so that the calculation
has only be done once here.

> @@ -1306,7 +1303,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> __user *buf,
>  
>   /* Skip uninteresting entries, i.e. clean on not-first pass */
>   if (!first_pass) {
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(>arch.hpt) &&
>  !hpte_dirty(revp, hptp)) {
>   ++i;
>   hptp += 2;
> @@ -1316,7 +1313,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> __user *buf,
>   hdr.index = i;
>  
>   /* Grab a series of valid entries */
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(>arch.hpt) &&
>  hdr.n_valid < 0x &&
>  nb + HPTE_SIZE < count &&
>  record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
> @@ -1332,7 +1329,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
> __user *buf,
>   ++revp;
>   }
>   /* Now skip invalid entries while we can */
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(>arch.hpt) &&
>  hdr.n_invalid < 0x &&
>  record_hpte(flags, hptp, hpte, revp, 0, first_pass)) {
>   /* found an invalid entry */

Dito, you could use a local variable to store the value from
kvmppc_hpt_npte()

Anyway, apart from these nits, patch looks fine to me, so:

Reviewed-by: Thomas Huth

[PATCH 04/11] powerpc/kvm: Don't store values derivable from HPT order

2016-12-14 Thread David Gibson 
Currently the kvm_hpt_info structure stores the hashed page table's order,
and also the number of HPTEs it contains and a mask for its size.  The
last two can be easily derived from the order, so remove them and just
calculate them as necessary with a couple of helper inlines.

Signed-off-by: David Gibson 
---
 arch/powerpc/include/asm/kvm_book3s_64.h | 12 
 arch/powerpc/include/asm/kvm_host.h  |  2 --
 arch/powerpc/kvm/book3s_64_mmu_hv.c  | 28 +---
 arch/powerpc/kvm/book3s_hv_rm_mmu.c  | 18 +-
 4 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h 
b/arch/powerpc/include/asm/kvm_book3s_64.h
index 8482921..8810327 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -350,6 +350,18 @@ extern void kvmppc_mmu_debugfs_init(struct kvm *kvm);
 
 extern void kvmhv_rm_send_ipi(int cpu);
 
+static inline unsigned long kvmppc_hpt_npte(struct kvm_hpt_info *hpt)
+{
+   /* HPTEs are 2**4 bytes long */
+   return 1UL << (hpt->order - 4);
+}
+
+static inline unsigned long kvmppc_hpt_mask(struct kvm_hpt_info *hpt)
+{
+   /* 128 (2**7) bytes in each HPTEG */
+   return (1UL << (hpt->order - 7)) - 1;
+}
+
 #endif /* CONFIG_KVM_BOOK3S_HV_POSSIBLE */
 
 #endif /* __ASM_KVM_BOOK3S_64_H__ */
diff --git a/arch/powerpc/include/asm/kvm_host.h 
b/arch/powerpc/include/asm/kvm_host.h
index 2673271..3900f63 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -244,8 +244,6 @@ struct kvm_arch_memory_slot {
 struct kvm_hpt_info {
unsigned long virt;
struct revmap_entry *rev;
-   unsigned long npte;
-   unsigned long mask;
u32 order;
int cma;
 };
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index b5799d1..fe88132 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -83,15 +83,11 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
 
kvm->arch.hpt.virt = hpt;
kvm->arch.hpt.order = order;
-   /* HPTEs are 2**4 bytes long */
-   kvm->arch.hpt.npte = 1ul << (order - 4);
-   /* 128 (2**7) bytes in each HPTEG */
-   kvm->arch.hpt.mask = (1ul << (order - 7)) - 1;
 
atomic64_set(>arch.mmio_update, 0);
 
/* Allocate reverse map array */
-   rev = vmalloc(sizeof(struct revmap_entry) * kvm->arch.hpt.npte);
+   rev = vmalloc(sizeof(struct revmap_entry) * 
kvmppc_hpt_npte(>arch.hpt));
if (!rev) {
pr_err("kvmppc_alloc_hpt: Couldn't alloc reverse map array\n");
goto out_freehpt;
@@ -194,8 +190,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
kvm_memory_slot *memslot,
if (npages > 1ul << (40 - porder))
npages = 1ul << (40 - porder);
/* Can't use more than 1 HPTE per HPTEG */
-   if (npages > kvm->arch.hpt.mask + 1)
-   npages = kvm->arch.hpt.mask + 1;
+   if (npages > kvmppc_hpt_mask(>arch.hpt) + 1)
+   npages = kvmppc_hpt_mask(>arch.hpt) + 1;
 
hp0 = HPTE_V_1TB_SEG | (VRMA_VSID << (40 - 16)) |
HPTE_V_BOLTED | hpte0_pgsize_encoding(psize);
@@ -205,7 +201,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct 
kvm_memory_slot *memslot,
for (i = 0; i < npages; ++i) {
addr = i << porder;
/* can't use hpt_hash since va > 64 bits */
-   hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25))) & 
kvm->arch.hpt.mask;
+   hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25)))
+   & kvmppc_hpt_mask(>arch.hpt);
/*
 * We assume that the hash table is empty and no
 * vcpus are using it at this stage.  Since we create
@@ -1306,7 +1303,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
__user *buf,
 
/* Skip uninteresting entries, i.e. clean on not-first pass */
if (!first_pass) {
-   while (i < kvm->arch.hpt.npte &&
+   while (i < kvmppc_hpt_npte(>arch.hpt) &&
   !hpte_dirty(revp, hptp)) {
++i;
hptp += 2;
@@ -1316,7 +1313,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
__user *buf,
hdr.index = i;
 
/* Grab a series of valid entries */
-   while (i < kvm->arch.hpt.npte &&
+   while (i < kvmppc_hpt_npte(>arch.hpt) &&
   hdr.n_valid < 0x &&
   nb + HPTE_SIZE < count &&
   record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
@@ -1332,7 +1329,7 @@ static ssize_t kvm_htab_read(struct file *file, char 
__user *buf,
++revp;
}
/* Now skip invalid