From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abmW9-0007rn-T3 for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:59:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abmW6-0000TH-LF for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:59:29 -0500 References: <1457069753-13123-1-git-send-email-david@gibson.dropbear.id.au> <1457069753-13123-2-git-send-email-david@gibson.dropbear.id.au> From: Thomas Huth Message-ID: <56D95C75.3050009@redhat.com> Date: Fri, 4 Mar 2016 10:59:17 +0100 MIME-Version: 1.0 In-Reply-To: <1457069753-13123-2-git-send-email-david@gibson.dropbear.id.au> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , gkurz@linux.vnet.ibm.com, aik@ozlabs.ru Cc: lvivier@redhat.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, mdroth@linux.vnet.ibm.com On 04.03.2016 06:35, David Gibson wrote: > When a Power cpu with 64-bit hash MMU has it's hash page table (HPT) > pointer updated by a write to the SDR1 register we need to update some > derived variables. Likewise, when the cpu is configured for an externa= l > HPT (one not in the guest memory space) some derived variables need to = be > updated. >=20 > Currently the logic for this is (partially) duplicated in ppc_store_sdr= 1() > and in spapr_cpu_reset(). In future we're going to need it in some oth= er > places, so make some common helpers for this update. >=20 > In addition the new ppc_hash64_set_external_hpt() helper also updates > SDR1 in KVM - it's not updated by the normal runtime KVM <-> qemu CPU > synchronization. In a sense this belongs logically in the > ppc_hash64_set_sdr1() helper, but that is called from > kvm_arch_get_registers() so can't itself call cpu_synchronize_state() > without infinite recursion. In practice this doesn't matter because > the only other caller is TCG specific. >=20 > Currently there aren't situations where updating SDR1 at runtime in KVM > matters, but there are going to be in future. >=20 > Signed-off-by: David Gibson > --- > hw/ppc/spapr.c | 13 ++----------- > target-ppc/kvm.c | 15 +++++++++++++++ > target-ppc/kvm_ppc.h | 6 ++++++ > target-ppc/mmu-hash64.c | 42 +++++++++++++++++++++++++++++++++++++++++= + > target-ppc/mmu-hash64.h | 6 ++++++ > target-ppc/mmu_helper.c | 13 ++++++------- > 6 files changed, 77 insertions(+), 18 deletions(-) >=20 > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index e9d4abf..a88e3af 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1196,17 +1196,8 @@ static void spapr_cpu_reset(void *opaque) > =20 > env->spr[SPR_HIOR] =3D 0; > =20 > - env->external_htab =3D (uint8_t *)spapr->htab; > - env->htab_base =3D -1; > - /* > - * htab_mask is the mask used to normalize hash value to PTEG inde= x. > - * htab_shift is log2 of hash table size. > - * We have 8 hpte per group, and each hpte is 16 bytes. > - * ie have 128 bytes per hpte entry. > - */ > - env->htab_mask =3D (1ULL << (spapr->htab_shift - 7)) - 1; > - env->spr[SPR_SDR1] =3D (target_ulong)(uintptr_t)spapr->htab | > - (spapr->htab_shift - 18); > + ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift, > + &error_fatal); > } > =20 > static void spapr_create_nvram(sPAPRMachineState *spapr) > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index d67c169..99b3231 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -2537,3 +2537,18 @@ int kvmppc_enable_hwrng(void) > =20 > return kvmppc_enable_hcall(kvm_state, H_RANDOM); > } > + > +int kvmppc_update_sdr1(PowerPCCPU *cpu) > +{ > + CPUState *cs =3D CPU(cpu); > + > + if (!kvm_enabled()) { > + return 0; /* nothing to do */ > + } Since you're updating more than SDR1 below ... Could you maybe add a "assert(cs->kvm_vcpu_dirty)" here, just to make sure that nobody accidentally ever calls this function without synchronizing the registers first? > + /* The normal KVM_PUT_RUNTIME_STATE doesn't include SDR1, which is > + * why we need an explicit update for it. KVM_PUT_RESET_STATE is > + * overkill, but this is a pretty rare operation, so it's simpler > + * than writing a special purpose updater */ > + return kvm_arch_put_registers(cs, KVM_PUT_RESET_STATE); > +} That indeed looks like a big overkill ... what about extracting the block from kvm_arch_put_registers() which sets the the "struct kvm_sregs" via KVM_SET_SREGS into a separate function, and then call that function here instead? kvm_arch_put_registers() is IMHO way too big anyway, so separating some code into external functions there would improve readability there, too. Apart from that, the patch looks right to me. Thomas