On Tue, Feb 21, 2017 at 01:52:11PM +1100, David Gibson wrote: > ppc_hash64_store_hpte() is used to update HPTEs in the hashed page table > (HPT) for 64-bit machines. This is used when the (emulated) CPU needs to > update the referenced (R) or changed (C) bits in the HPTE. > > Some time ago this was converted to take an HPTE index, instead of a > raw offset to the HPTE within the HPT (similar functions for 32-bit still > take an offset). In the process a serious bug was introduced: we're > still using the index parameter as though it was an offset, failing to > multiply by the size of an HPTE, so it will update bits in the wrong part > of the HPT. This can corrupt the guests's HPT, causing crashes or data > loss. > > AFAICT the reason we haven't noticed this error earlier is that for 64-bit > machines we've been testing almost exclusively with Linux guests. Linux > on ppc does not make use of the hardware R & C bits, so this writeback > will never be triggered. It also occurs only on TCG, not KVM, guests. > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
Self NACK. Sorry. In my panic, I managed to miss the multiply a few lines above. There isn't actually a bug here, just some confusing variable naming. > --- > target/ppc/mmu-hash64.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > index bb78fb5..dc3b5f7 100644 > --- a/target/ppc/mmu-hash64.c > +++ b/target/ppc/mmu-hash64.c > @@ -894,12 +894,15 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu, > > pte_index *= HASH_PTE_SIZE_64; > if (env->external_htab) { > - stq_p(env->external_htab + pte_index, pte0); > - stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64 / 2, pte1); > + stq_p(env->external_htab + pte_index * HASH_PTE_SIZE_64, pte0); > + stq_p(env->external_htab + pte_index * HASH_PTE_SIZE_64 > + + HASH_PTE_SIZE_64 / 2, pte1); > } else { > - stq_phys(CPU(cpu)->as, env->htab_base + pte_index, pte0); > stq_phys(CPU(cpu)->as, > - env->htab_base + pte_index + HASH_PTE_SIZE_64 / 2, pte1); > + env->htab_base + pte_index * HASH_PTE_SIZE_64, pte0); > + stq_phys(CPU(cpu)->as, > + env->htab_base + pte_index * HASH_PTE_SIZE_64 > + + HASH_PTE_SIZE_64 / 2, pte1); > } > } > -- 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