* [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte
@ 2014-10-20 14:28 Aneesh Kumar K.V
2014-10-20 14:28 ` [PATCH 2/3] KVM: PPC: BOOK3S: HV: Use unlock variant with memory barrier Aneesh Kumar K.V
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Aneesh Kumar K.V @ 2014-10-20 14:28 UTC (permalink / raw)
To: agraf, benh, paulus; +Cc: linuxppc-dev, kvm, kvm-ppc, Aneesh Kumar K.V
This patch adds helper routine for lock and unlock hpte and use
the same for rest of the code. We don't change any locking rules in this
patch. In the next patch we switch some of the unlock usage to use
the api with barrier and also document the usage without barriers.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/kvm_book3s_64.h | 14 ++++++++++++++
arch/powerpc/kvm/book3s_64_mmu_hv.c | 25 ++++++++++---------------
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 27 ++++++++++-----------------
3 files changed, 34 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index 0aa817933e6a..ec9fb6085843 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -86,6 +86,20 @@ static inline long try_lock_hpte(__be64 *hpte, unsigned long bits)
return old == 0;
}
+static inline void unlock_hpte(__be64 *hpte, unsigned long hpte_v)
+{
+ hpte_v &= ~HPTE_V_HVLOCK;
+ asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
+ hpte[0] = cpu_to_be64(hpte_v);
+}
+
+/* Without barrier */
+static inline void __unlock_hpte(__be64 *hpte, unsigned long hpte_v)
+{
+ hpte_v &= ~HPTE_V_HVLOCK;
+ hpte[0] = cpu_to_be64(hpte_v);
+}
+
static inline int __hpte_actual_psize(unsigned int lp, int psize)
{
int i, shift;
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index cebb86bc4a37..5ea4b2b6a157 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -475,9 +475,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
v = be64_to_cpu(hptep[0]) & ~HPTE_V_HVLOCK;
gr = kvm->arch.revmap[index].guest_rpte;
- /* Unlock the HPTE */
- asm volatile("lwsync" : : : "memory");
- hptep[0] = cpu_to_be64(v);
+ unlock_hpte(hptep, v);
preempt_enable();
gpte->eaddr = eaddr;
@@ -606,8 +604,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
hpte[0] = be64_to_cpu(hptep[0]) & ~HPTE_V_HVLOCK;
hpte[1] = be64_to_cpu(hptep[1]);
hpte[2] = r = rev->guest_rpte;
- asm volatile("lwsync" : : : "memory");
- hptep[0] = cpu_to_be64(hpte[0]);
+ unlock_hpte(hptep, hpte[0]);
preempt_enable();
if (hpte[0] != vcpu->arch.pgfault_hpte[0] ||
@@ -758,7 +755,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
hptep[1] = cpu_to_be64(r);
eieio();
- hptep[0] = cpu_to_be64(hpte[0]);
+ __unlock_hpte(hptep, hpte[0]);
asm volatile("ptesync" : : : "memory");
preempt_enable();
if (page && hpte_is_writable(r))
@@ -777,7 +774,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
return ret;
out_unlock:
- hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
preempt_enable();
goto out_put;
}
@@ -907,7 +904,7 @@ static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp,
}
}
unlock_rmap(rmapp);
- hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
}
return 0;
}
@@ -995,7 +992,7 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp,
}
ret = 1;
}
- hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
} while ((i = j) != head);
unlock_rmap(rmapp);
@@ -1118,8 +1115,7 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)
/* Now check and modify the HPTE */
if (!(hptep[0] & cpu_to_be64(HPTE_V_VALID))) {
- /* unlock and continue */
- hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
continue;
}
/* need to make it temporarily absent so C is stable */
@@ -1139,9 +1135,9 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)
npages_dirty = n;
eieio();
}
- v &= ~(HPTE_V_ABSENT | HPTE_V_HVLOCK);
+ v &= ~HPTE_V_ABSENT;
v |= HPTE_V_VALID;
- hptep[0] = cpu_to_be64(v);
+ __unlock_hpte(hptep, v);
} while ((i = j) != head);
unlock_rmap(rmapp);
@@ -1379,8 +1375,7 @@ static long record_hpte(unsigned long flags, __be64 *hptp,
r &= ~HPTE_GR_MODIFIED;
revp->guest_rpte = r;
}
- asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
- hptp[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ unlock_hpte(hptp, be64_to_cpu(hptp[0]));
preempt_enable();
if (!(valid == want_valid && (first_pass || dirty)))
ok = 0;
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 084ad54c73cd..769a5d4c0430 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -154,12 +154,6 @@ static pte_t lookup_linux_pte_and_update(pgd_t *pgdir, unsigned long hva,
return kvmppc_read_update_linux_pte(ptep, writing, hugepage_shift);
}
-static inline void unlock_hpte(__be64 *hpte, unsigned long hpte_v)
-{
- asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
- hpte[0] = cpu_to_be64(hpte_v);
-}
-
long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
long pte_index, unsigned long pteh, unsigned long ptel,
pgd_t *pgdir, bool realmode, unsigned long *pte_idx_ret)
@@ -295,10 +289,10 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
u64 pte;
while (!try_lock_hpte(hpte, HPTE_V_HVLOCK))
cpu_relax();
- pte = be64_to_cpu(*hpte);
+ pte = be64_to_cpu(hpte[0]);
if (!(pte & (HPTE_V_VALID | HPTE_V_ABSENT)))
break;
- *hpte &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hpte, pte);
hpte += 2;
}
if (i == 8)
@@ -314,9 +308,9 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
while (!try_lock_hpte(hpte, HPTE_V_HVLOCK))
cpu_relax();
- pte = be64_to_cpu(*hpte);
+ pte = be64_to_cpu(hpte[0]);
if (pte & (HPTE_V_VALID | HPTE_V_ABSENT)) {
- *hpte &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hpte, pte);
return H_PTEG_FULL;
}
}
@@ -356,7 +350,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
/* Write the first HPTE dword, unlocking the HPTE and making it valid */
eieio();
- hpte[0] = cpu_to_be64(pteh);
+ __unlock_hpte(hpte, pteh);
asm volatile("ptesync" : : : "memory");
*pte_idx_ret = pte_index;
@@ -487,7 +481,7 @@ long kvmppc_do_h_remove(struct kvm *kvm, unsigned long flags,
if ((pte & (HPTE_V_ABSENT | HPTE_V_VALID)) == 0 ||
((flags & H_AVPN) && (pte & ~0x7fUL) != avpn) ||
((flags & H_ANDCOND) && (pte & avpn) != 0)) {
- hpte[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hpte, pte);
return H_NOT_FOUND;
}
@@ -623,7 +617,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
be64_to_cpu(hp[0]), be64_to_cpu(hp[1]));
rcbits = rev->guest_rpte & (HPTE_R_R|HPTE_R_C);
args[j] |= rcbits << (56 - 5);
- hp[0] = 0;
+ __unlock_hpte(hp, 0);
}
}
@@ -649,7 +643,7 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
pte = be64_to_cpu(hpte[0]);
if ((pte & (HPTE_V_ABSENT | HPTE_V_VALID)) == 0 ||
((flags & H_AVPN) && (pte & ~0x7fUL) != avpn)) {
- hpte[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
+ __unlock_hpte(hpte, pte);
return H_NOT_FOUND;
}
@@ -700,7 +694,7 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
}
hpte[1] = cpu_to_be64(r);
eieio();
- hpte[0] = cpu_to_be64(v & ~HPTE_V_HVLOCK);
+ __unlock_hpte(hpte, v);
asm volatile("ptesync" : : : "memory");
return H_SUCCESS;
}
@@ -841,8 +835,7 @@ long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
/* Return with the HPTE still locked */
return (hash << 3) + (i >> 1);
- /* Unlock and move on */
- hpte[i] = cpu_to_be64(v);
+ __unlock_hpte(&hpte[i], v);
}
if (val & HPTE_V_SECONDARY)
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] KVM: PPC: BOOK3S: HV: Use unlock variant with memory barrier
2014-10-20 14:28 [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
@ 2014-10-20 14:28 ` Aneesh Kumar K.V
2014-10-20 14:29 ` [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability Aneesh Kumar K.V
2015-01-13 5:09 ` [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
2 siblings, 0 replies; 5+ messages in thread
From: Aneesh Kumar K.V @ 2014-10-20 14:28 UTC (permalink / raw)
To: agraf, benh, paulus; +Cc: linuxppc-dev, kvm, kvm-ppc, Aneesh Kumar K.V
We switch to unlock variant with memory barriers in the error path
and also in code path where we had implicit dependency on previous
functions calling lwsync/ptesync. In most of the cases we don't really
need an explicit barrier, but using the variant make sure we don't make
mistakes later with code movements. We also document why a
non-barrier variant is ok in performance critical path.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 10 +++++-----
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 15 ++++++++++-----
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 5ea4b2b6a157..c97690ffb5f6 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -774,7 +774,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
return ret;
out_unlock:
- __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
+ unlock_hpte(hptep, be64_to_cpu(hptep[0]));
preempt_enable();
goto out_put;
}
@@ -903,8 +903,8 @@ static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp,
note_hpte_modification(kvm, &rev[i]);
}
}
+ unlock_hpte(hptep, be64_to_cpu(hptep[0]));
unlock_rmap(rmapp);
- __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
}
return 0;
}
@@ -992,7 +992,7 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp,
}
ret = 1;
}
- __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
+ unlock_hpte(hptep, be64_to_cpu(hptep[0]));
} while ((i = j) != head);
unlock_rmap(rmapp);
@@ -1115,7 +1115,7 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)
/* Now check and modify the HPTE */
if (!(hptep[0] & cpu_to_be64(HPTE_V_VALID))) {
- __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
+ unlock_hpte(hptep, be64_to_cpu(hptep[0]));
continue;
}
/* need to make it temporarily absent so C is stable */
@@ -1137,7 +1137,7 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)
}
v &= ~HPTE_V_ABSENT;
v |= HPTE_V_VALID;
- __unlock_hpte(hptep, v);
+ unlock_hpte(hptep, v);
} while ((i = j) != head);
unlock_rmap(rmapp);
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 769a5d4c0430..78e689b066f1 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -292,6 +292,9 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
pte = be64_to_cpu(hpte[0]);
if (!(pte & (HPTE_V_VALID | HPTE_V_ABSENT)))
break;
+ /*
+ * Data dependency will avoid re-ordering
+ */
__unlock_hpte(hpte, pte);
hpte += 2;
}
@@ -310,7 +313,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
cpu_relax();
pte = be64_to_cpu(hpte[0]);
if (pte & (HPTE_V_VALID | HPTE_V_ABSENT)) {
- __unlock_hpte(hpte, pte);
+ unlock_hpte(hpte, pte);
return H_PTEG_FULL;
}
}
@@ -481,7 +484,7 @@ long kvmppc_do_h_remove(struct kvm *kvm, unsigned long flags,
if ((pte & (HPTE_V_ABSENT | HPTE_V_VALID)) == 0 ||
((flags & H_AVPN) && (pte & ~0x7fUL) != avpn) ||
((flags & H_ANDCOND) && (pte & avpn) != 0)) {
- __unlock_hpte(hpte, pte);
+ unlock_hpte(hpte, pte);
return H_NOT_FOUND;
}
@@ -617,7 +620,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
be64_to_cpu(hp[0]), be64_to_cpu(hp[1]));
rcbits = rev->guest_rpte & (HPTE_R_R|HPTE_R_C);
args[j] |= rcbits << (56 - 5);
- __unlock_hpte(hp, 0);
+ unlock_hpte(hp, 0);
}
}
@@ -643,7 +646,7 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
pte = be64_to_cpu(hpte[0]);
if ((pte & (HPTE_V_ABSENT | HPTE_V_VALID)) == 0 ||
((flags & H_AVPN) && (pte & ~0x7fUL) != avpn)) {
- __unlock_hpte(hpte, pte);
+ unlock_hpte(hpte, pte);
return H_NOT_FOUND;
}
@@ -834,7 +837,9 @@ long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
hpte_base_page_size(v, r) == (1ul << pshift))
/* Return with the HPTE still locked */
return (hash << 3) + (i >> 1);
-
+ /*
+ * Data dependency should avoid re-ordering
+ */
__unlock_hpte(&hpte[i], v);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability
2014-10-20 14:28 [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
2014-10-20 14:28 ` [PATCH 2/3] KVM: PPC: BOOK3S: HV: Use unlock variant with memory barrier Aneesh Kumar K.V
@ 2014-10-20 14:29 ` Aneesh Kumar K.V
2014-11-27 0:45 ` Paul Mackerras
2015-01-13 5:09 ` [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
2 siblings, 1 reply; 5+ messages in thread
From: Aneesh Kumar K.V @ 2014-10-20 14:29 UTC (permalink / raw)
To: agraf, benh, paulus; +Cc: linuxppc-dev, kvm, kvm-ppc, Aneesh Kumar K.V
Minor cleanup
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 78e689b066f1..2922f8d127ff 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -523,7 +523,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
unsigned long *args = &vcpu->arch.gpr[4];
__be64 *hp, *hptes[4];
unsigned long tlbrb[4];
- long int i, j, k, n, found, indexes[4];
+ long int i, j, k, collected_hpte, found, indexes[4];
unsigned long flags, req, pte_index, rcbits;
int global;
long int ret = H_SUCCESS;
@@ -532,7 +532,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
global = global_invalidates(kvm, 0);
for (i = 0; i < 4 && ret == H_SUCCESS; ) {
- n = 0;
+ collected_hpte = 0;
for (; i < 4; ++i) {
j = i * 2;
pte_index = args[j];
@@ -554,7 +554,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
hp = (__be64 *) (kvm->arch.hpt_virt + (pte_index << 4));
/* to avoid deadlock, don't spin except for first */
if (!try_lock_hpte(hp, HPTE_V_HVLOCK)) {
- if (n)
+ if (collected_hpte)
break;
while (!try_lock_hpte(hp, HPTE_V_HVLOCK))
cpu_relax();
@@ -596,22 +596,23 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
/* leave it locked */
hp[0] &= ~cpu_to_be64(HPTE_V_VALID);
- tlbrb[n] = compute_tlbie_rb(be64_to_cpu(hp[0]),
- be64_to_cpu(hp[1]), pte_index);
- indexes[n] = j;
- hptes[n] = hp;
- revs[n] = rev;
- ++n;
+ tlbrb[collected_hpte] = compute_tlbie_rb(be64_to_cpu(hp[0]),
+ be64_to_cpu(hp[1]),
+ pte_index);
+ indexes[collected_hpte] = j;
+ hptes[collected_hpte] = hp;
+ revs[collected_hpte] = rev;
+ ++collected_hpte;
}
- if (!n)
+ if (!collected_hpte)
break;
/* Now that we've collected a batch, do the tlbies */
- do_tlbies(kvm, tlbrb, n, global, true);
+ do_tlbies(kvm, tlbrb, collected_hpte, global, true);
/* Read PTE low words after tlbie to get final R/C values */
- for (k = 0; k < n; ++k) {
+ for (k = 0; k < collected_hpte; ++k) {
j = indexes[k];
pte_index = args[j] & ((1ul << 56) - 1);
hp = hptes[k];
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability
2014-10-20 14:29 ` [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability Aneesh Kumar K.V
@ 2014-11-27 0:45 ` Paul Mackerras
0 siblings, 0 replies; 5+ messages in thread
From: Paul Mackerras @ 2014-11-27 0:45 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev, agraf, kvm-ppc, kvm
On Mon, Oct 20, 2014 at 07:59:00PM +0530, Aneesh Kumar K.V wrote:
> Minor cleanup
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 78e689b066f1..2922f8d127ff 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -523,7 +523,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
> unsigned long *args = &vcpu->arch.gpr[4];
> __be64 *hp, *hptes[4];
> unsigned long tlbrb[4];
> - long int i, j, k, n, found, indexes[4];
> + long int i, j, k, collected_hpte, found, indexes[4];
Hmmm... I don't find it more readable, because "collected_hpte" sounds
like it contains a HPTE value. Also I don't like using a long name
for something that is just a temporary value inside a function, and
"n" is a suitable name for a temporary variable counting the number of
things we have. I would prefer just adding a comment like this:
- n = 0;
+ n = 0; /* # values collected in tlbrb[], indexes[] etc. */
Paul.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte
2014-10-20 14:28 [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
2014-10-20 14:28 ` [PATCH 2/3] KVM: PPC: BOOK3S: HV: Use unlock variant with memory barrier Aneesh Kumar K.V
2014-10-20 14:29 ` [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability Aneesh Kumar K.V
@ 2015-01-13 5:09 ` Aneesh Kumar K.V
2 siblings, 0 replies; 5+ messages in thread
From: Aneesh Kumar K.V @ 2015-01-13 5:09 UTC (permalink / raw)
To: agraf, benh, paulus; +Cc: linuxppc-dev, kvm, kvm-ppc
Hi,
Any update on this patch. We could drop patch 3. Any feedback on 1 and 2
?.
-aneesh
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> This patch adds helper routine for lock and unlock hpte and use
> the same for rest of the code. We don't change any locking rules in this
> patch. In the next patch we switch some of the unlock usage to use
> the api with barrier and also document the usage without barriers.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_book3s_64.h | 14 ++++++++++++++
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 25 ++++++++++---------------
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 27 ++++++++++-----------------
> 3 files changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 0aa817933e6a..ec9fb6085843 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -86,6 +86,20 @@ static inline long try_lock_hpte(__be64 *hpte, unsigned long bits)
> return old == 0;
> }
>
> +static inline void unlock_hpte(__be64 *hpte, unsigned long hpte_v)
> +{
> + hpte_v &= ~HPTE_V_HVLOCK;
> + asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
> + hpte[0] = cpu_to_be64(hpte_v);
> +}
> +
> +/* Without barrier */
> +static inline void __unlock_hpte(__be64 *hpte, unsigned long hpte_v)
> +{
> + hpte_v &= ~HPTE_V_HVLOCK;
> + hpte[0] = cpu_to_be64(hpte_v);
> +}
> +
> static inline int __hpte_actual_psize(unsigned int lp, int psize)
> {
> int i, shift;
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index cebb86bc4a37..5ea4b2b6a157 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -475,9 +475,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
> v = be64_to_cpu(hptep[0]) & ~HPTE_V_HVLOCK;
> gr = kvm->arch.revmap[index].guest_rpte;
>
> - /* Unlock the HPTE */
> - asm volatile("lwsync" : : : "memory");
> - hptep[0] = cpu_to_be64(v);
> + unlock_hpte(hptep, v);
> preempt_enable();
>
> gpte->eaddr = eaddr;
> @@ -606,8 +604,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
> hpte[0] = be64_to_cpu(hptep[0]) & ~HPTE_V_HVLOCK;
> hpte[1] = be64_to_cpu(hptep[1]);
> hpte[2] = r = rev->guest_rpte;
> - asm volatile("lwsync" : : : "memory");
> - hptep[0] = cpu_to_be64(hpte[0]);
> + unlock_hpte(hptep, hpte[0]);
> preempt_enable();
>
> if (hpte[0] != vcpu->arch.pgfault_hpte[0] ||
> @@ -758,7 +755,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
>
> hptep[1] = cpu_to_be64(r);
> eieio();
> - hptep[0] = cpu_to_be64(hpte[0]);
> + __unlock_hpte(hptep, hpte[0]);
> asm volatile("ptesync" : : : "memory");
> preempt_enable();
> if (page && hpte_is_writable(r))
> @@ -777,7 +774,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
> return ret;
>
> out_unlock:
> - hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
> preempt_enable();
> goto out_put;
> }
> @@ -907,7 +904,7 @@ static int kvm_unmap_rmapp(struct kvm *kvm, unsigned long *rmapp,
> }
> }
> unlock_rmap(rmapp);
> - hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
> }
> return 0;
> }
> @@ -995,7 +992,7 @@ static int kvm_age_rmapp(struct kvm *kvm, unsigned long *rmapp,
> }
> ret = 1;
> }
> - hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
> } while ((i = j) != head);
>
> unlock_rmap(rmapp);
> @@ -1118,8 +1115,7 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)
>
> /* Now check and modify the HPTE */
> if (!(hptep[0] & cpu_to_be64(HPTE_V_VALID))) {
> - /* unlock and continue */
> - hptep[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hptep, be64_to_cpu(hptep[0]));
> continue;
> }
> /* need to make it temporarily absent so C is stable */
> @@ -1139,9 +1135,9 @@ static int kvm_test_clear_dirty_npages(struct kvm *kvm, unsigned long *rmapp)
> npages_dirty = n;
> eieio();
> }
> - v &= ~(HPTE_V_ABSENT | HPTE_V_HVLOCK);
> + v &= ~HPTE_V_ABSENT;
> v |= HPTE_V_VALID;
> - hptep[0] = cpu_to_be64(v);
> + __unlock_hpte(hptep, v);
> } while ((i = j) != head);
>
> unlock_rmap(rmapp);
> @@ -1379,8 +1375,7 @@ static long record_hpte(unsigned long flags, __be64 *hptp,
> r &= ~HPTE_GR_MODIFIED;
> revp->guest_rpte = r;
> }
> - asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
> - hptp[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + unlock_hpte(hptp, be64_to_cpu(hptp[0]));
> preempt_enable();
> if (!(valid == want_valid && (first_pass || dirty)))
> ok = 0;
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index 084ad54c73cd..769a5d4c0430 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -154,12 +154,6 @@ static pte_t lookup_linux_pte_and_update(pgd_t *pgdir, unsigned long hva,
> return kvmppc_read_update_linux_pte(ptep, writing, hugepage_shift);
> }
>
> -static inline void unlock_hpte(__be64 *hpte, unsigned long hpte_v)
> -{
> - asm volatile(PPC_RELEASE_BARRIER "" : : : "memory");
> - hpte[0] = cpu_to_be64(hpte_v);
> -}
> -
> long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> long pte_index, unsigned long pteh, unsigned long ptel,
> pgd_t *pgdir, bool realmode, unsigned long *pte_idx_ret)
> @@ -295,10 +289,10 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> u64 pte;
> while (!try_lock_hpte(hpte, HPTE_V_HVLOCK))
> cpu_relax();
> - pte = be64_to_cpu(*hpte);
> + pte = be64_to_cpu(hpte[0]);
> if (!(pte & (HPTE_V_VALID | HPTE_V_ABSENT)))
> break;
> - *hpte &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hpte, pte);
> hpte += 2;
> }
> if (i == 8)
> @@ -314,9 +308,9 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
>
> while (!try_lock_hpte(hpte, HPTE_V_HVLOCK))
> cpu_relax();
> - pte = be64_to_cpu(*hpte);
> + pte = be64_to_cpu(hpte[0]);
> if (pte & (HPTE_V_VALID | HPTE_V_ABSENT)) {
> - *hpte &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hpte, pte);
> return H_PTEG_FULL;
> }
> }
> @@ -356,7 +350,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
>
> /* Write the first HPTE dword, unlocking the HPTE and making it valid */
> eieio();
> - hpte[0] = cpu_to_be64(pteh);
> + __unlock_hpte(hpte, pteh);
> asm volatile("ptesync" : : : "memory");
>
> *pte_idx_ret = pte_index;
> @@ -487,7 +481,7 @@ long kvmppc_do_h_remove(struct kvm *kvm, unsigned long flags,
> if ((pte & (HPTE_V_ABSENT | HPTE_V_VALID)) == 0 ||
> ((flags & H_AVPN) && (pte & ~0x7fUL) != avpn) ||
> ((flags & H_ANDCOND) && (pte & avpn) != 0)) {
> - hpte[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hpte, pte);
> return H_NOT_FOUND;
> }
>
> @@ -623,7 +617,7 @@ long kvmppc_h_bulk_remove(struct kvm_vcpu *vcpu)
> be64_to_cpu(hp[0]), be64_to_cpu(hp[1]));
> rcbits = rev->guest_rpte & (HPTE_R_R|HPTE_R_C);
> args[j] |= rcbits << (56 - 5);
> - hp[0] = 0;
> + __unlock_hpte(hp, 0);
> }
> }
>
> @@ -649,7 +643,7 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
> pte = be64_to_cpu(hpte[0]);
> if ((pte & (HPTE_V_ABSENT | HPTE_V_VALID)) == 0 ||
> ((flags & H_AVPN) && (pte & ~0x7fUL) != avpn)) {
> - hpte[0] &= ~cpu_to_be64(HPTE_V_HVLOCK);
> + __unlock_hpte(hpte, pte);
> return H_NOT_FOUND;
> }
>
> @@ -700,7 +694,7 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
> }
> hpte[1] = cpu_to_be64(r);
> eieio();
> - hpte[0] = cpu_to_be64(v & ~HPTE_V_HVLOCK);
> + __unlock_hpte(hpte, v);
> asm volatile("ptesync" : : : "memory");
> return H_SUCCESS;
> }
> @@ -841,8 +835,7 @@ long kvmppc_hv_find_lock_hpte(struct kvm *kvm, gva_t eaddr, unsigned long slb_v,
> /* Return with the HPTE still locked */
> return (hash << 3) + (i >> 1);
>
> - /* Unlock and move on */
> - hpte[i] = cpu_to_be64(v);
> + __unlock_hpte(&hpte[i], v);
> }
>
> if (val & HPTE_V_SECONDARY)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-01-13 5:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-20 14:28 [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
2014-10-20 14:28 ` [PATCH 2/3] KVM: PPC: BOOK3S: HV: Use unlock variant with memory barrier Aneesh Kumar K.V
2014-10-20 14:29 ` [PATCH 3/3] KVM: PPC: BOOK3S: HV: Rename variable for better readability Aneesh Kumar K.V
2014-11-27 0:45 ` Paul Mackerras
2015-01-13 5:09 ` [PATCH 1/3] KVM: PPC: BOOK3S: HV: Add helpers for lock/unlock hpte Aneesh Kumar K.V
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).