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 <[email protected]>
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
