[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCHv2 2/3] target-ppc: Add helpers for up
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCHv2 2/3] target-ppc: Add helpers for updating a CPU's SDR1 and external HPT |
Date: |
Thu, 24 Mar 2016 09:41:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
On 24/03/2016 06:35, David Gibson wrote:
> On Tue, Mar 22, 2016 at 05:33:45PM +0100, Laurent Vivier wrote:
>> Hi David,
>>
>> using kvm-unit-tests, I've found a side effect of your patches: the MSR
>> is cleared (and perhaps some others).
>>
>> I was trying to test my patch on top of QEMU master:
>>
>> "ppc64: set MSR_SF bit"
>> http://patchwork.ozlabs.org/patch/598198/
>>
>> and it was not working anymore.
>>
>> By bisecting, I've found this commit.
>>
>> I think "cpu_synchronize_state()" in "ppc_hash64_set_external_hpt()"
>> restores the MSR from KVM whereas the one from QEMU has not been saved,
>> because cpu_synchronize_all_post_reset() is called later.
>>
>> So it is cleared.
>>
>> You can test this by applying the MSR_SF patch and using the "emulator"
>> test of kvm-unit-tests (the "emulator: 64bit" test case)
>
> Ugh, you're right of course. But, I'm having a bit of trouble
> figuring out how to fix it propertly.
Perhaps you can just remove the cpu_synchronize_state()?
As this is in the reset phase (spapr_cpu_reset()), I think the content
of the QEMU side registers are correct, and they will be synchronized at
the end of the reset phase.
You can see in spapr_cpu_reset(), SPR_HIOR is updated without prior
sync. I think spapr_cpu_reset() is called by qemu_device_reset() in
qemu_system_reset(), which is always followed by a
cpu_synchronize_all_post_reset(), which will call
do_kvm_cpu_synchronize_post_reset() (kvm_arch_put_registers(cpu,
KVM_PUT_RESET_STATE)) at the end.
Laurent
>> On 07/03/2016 03:26, 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 external
>>> HPT (one not in the guest memory space) some derived variables need to be
>>> updated.
>>>
>>> Currently the logic for this is (partially) duplicated in ppc_store_sdr1()
>>> and in spapr_cpu_reset(). In future we're going to need it in some other
>>> places, so make some common helpers for this update.
>>>
>>> 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.
>>>
>>> Currently there aren't situations where updating SDR1 at runtime in KVM
>>> matters, but there are going to be in future.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>> hw/ppc/spapr.c | 13 ++-----------
>>> target-ppc/kvm.c | 2 +-
>>> target-ppc/kvm_ppc.h | 6 ++++++
>>> target-ppc/mmu-hash64.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>> target-ppc/mmu-hash64.h | 6 ++++++
>>> target-ppc/mmu_helper.c | 13 ++++++-------
>>> 6 files changed, 64 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 64c4acc..72a018b 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -1196,17 +1196,8 @@ static void spapr_cpu_reset(void *opaque)
>>>
>>> env->spr[SPR_HIOR] = 0;
>>>
>>> - env->external_htab = (uint8_t *)spapr->htab;
>>> - env->htab_base = -1;
>>> - /*
>>> - * htab_mask is the mask used to normalize hash value to PTEG index.
>>> - * 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 = (1ULL << (spapr->htab_shift - 7)) - 1;
>>> - env->spr[SPR_SDR1] = (target_ulong)(uintptr_t)spapr->htab |
>>> - (spapr->htab_shift - 18);
>>> + ppc_hash64_set_external_hpt(cpu, spapr->htab, spapr->htab_shift,
>>> + &error_fatal);
>>> }
>>>
>>> static void spapr_create_nvram(sPAPRMachineState *spapr)
>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>> index 8a762e8..380dff6 100644
>>> --- a/target-ppc/kvm.c
>>> +++ b/target-ppc/kvm.c
>>> @@ -867,7 +867,7 @@ static int kvm_put_vpa(CPUState *cs)
>>> }
>>> #endif /* TARGET_PPC64 */
>>>
>>> -static int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>>> +int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>>> {
>>> CPUPPCState *env = &cpu->env;
>>> struct kvm_sregs sregs;
>>> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
>>> index fd64c44..fc79312 100644
>>> --- a/target-ppc/kvm_ppc.h
>>> +++ b/target-ppc/kvm_ppc.h
>>> @@ -55,6 +55,7 @@ void kvmppc_hash64_write_pte(CPUPPCState *env,
>>> target_ulong pte_index,
>>> target_ulong pte0, target_ulong pte1);
>>> bool kvmppc_has_cap_fixup_hcalls(void);
>>> int kvmppc_enable_hwrng(void);
>>> +int kvmppc_put_books_sregs(PowerPCCPU *cpu);
>>>
>>> #else
>>>
>>> @@ -246,6 +247,11 @@ static inline int kvmppc_enable_hwrng(void)
>>> {
>>> return -1;
>>> }
>>> +
>>> +static inline int kvmppc_put_books_sregs(PowerPCCPU *cpu)
>>> +{
>>> + abort();
>>> +}
>>> #endif
>>>
>>> #ifndef CONFIG_KVM
>>> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
>>> index 9c58fbf..7b5200b 100644
>>> --- a/target-ppc/mmu-hash64.c
>>> +++ b/target-ppc/mmu-hash64.c
>>> @@ -258,6 +258,49 @@ target_ulong helper_load_slb_vsid(CPUPPCState *env,
>>> target_ulong rb)
>>> /*
>>> * 64-bit hash table MMU handling
>>> */
>>> +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
>>> + Error **errp)
>>> +{
>>> + CPUPPCState *env = &cpu->env;
>>> + target_ulong htabsize = value & SDR_64_HTABSIZE;
>>> +
>>> + env->spr[SPR_SDR1] = value;
>>> + if (htabsize > 28) {
>>> + error_setg(errp,
>>> + "Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1",
>>> + htabsize);
>>> + htabsize = 28;
>>> + }
>>> + env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
>>> + env->htab_base = value & SDR_64_HTABORG;
>>> +}
>>> +
>>> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
>>> + Error **errp)
>>> +{
>>> + CPUPPCState *env = &cpu->env;
>>> + Error *local_err = NULL;
>>> +
>>> + cpu_synchronize_state(CPU(cpu));
>>> +
>>> + env->external_htab = hpt;
>>> + ppc_hash64_set_sdr1(cpu, (target_ulong)(uintptr_t)hpt | (shift - 18),
>>> + &local_err);
>>> + if (local_err) {
>>> + error_propagate(errp, local_err);
>>> + return;
>>> + }
>>> +
>>> + /* Not strictly necessary, but makes it clearer that an external
>>> + * htab is in use when debugging */
>>> + env->htab_base = -1;
>>> +
>>> + if (kvm_enabled()) {
>>> + if (kvmppc_put_books_sregs(cpu) < 0) {
>>> + error_setg(errp, "Unable to update SDR1 in KVM");
>>> + }
>>> + }
>>> +}
>>>
>>> static int ppc_hash64_pte_prot(PowerPCCPU *cpu,
>>> ppc_slb_t *slb, ppc_hash_pte64_t pte)
>>> diff --git a/target-ppc/mmu-hash64.h b/target-ppc/mmu-hash64.h
>>> index e7d9925..138cd7e 100644
>>> --- a/target-ppc/mmu-hash64.h
>>> +++ b/target-ppc/mmu-hash64.h
>>> @@ -92,6 +92,12 @@ unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU
>>> *cpu,
>>>
>>>
>>> extern bool kvmppc_kern_htab;
>>> +
>>> +void ppc_hash64_set_sdr1(PowerPCCPU *cpu, target_ulong value,
>>> + Error **errp);
>>> +void ppc_hash64_set_external_hpt(PowerPCCPU *cpu, void *hpt, int shift,
>>> + Error **errp);
>>> +
>>> uint64_t ppc_hash64_start_access(PowerPCCPU *cpu, target_ulong pte_index);
>>> void ppc_hash64_stop_access(uint64_t token);
>>>
>>> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
>>> index e5ec8d6..fcb2cc5 100644
>>> --- a/target-ppc/mmu_helper.c
>>> +++ b/target-ppc/mmu_helper.c
>>> @@ -2005,15 +2005,14 @@ void ppc_store_sdr1(CPUPPCState *env, target_ulong
>>> value)
>>> env->spr[SPR_SDR1] = value;
>>> #if defined(TARGET_PPC64)
>>> if (env->mmu_model & POWERPC_MMU_64) {
>>> - target_ulong htabsize = value & SDR_64_HTABSIZE;
>>> + PowerPCCPU *cpu = ppc_env_get_cpu(env);
>>> + Error *local_err = NULL;
>>>
>>> - if (htabsize > 28) {
>>> - fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
>>> - " stored in SDR1\n", htabsize);
>>> - htabsize = 28;
>>> + ppc_hash64_set_sdr1(cpu, value, &local_err);
>>> + if (local_err) {
>>> + error_report_err(local_err);
>>> + error_free(local_err);
>>> }
>>> - env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
>>> - env->htab_base = value & SDR_64_HTABORG;
>>> } else
>>> #endif /* defined(TARGET_PPC64) */
>>> {
>>>
>>
>
[Qemu-ppc] [PATCHv2 3/3] target-ppc: Eliminate kvmppc_kern_htab global, David Gibson, 2016/03/06
[Qemu-ppc] [PATCHv2 1/3] target-ppc: Split out SREGS get/put functions, David Gibson, 2016/03/06