Message ID | 20210512140813.112884-3-bruno.larsen@eldorado.org.br |
---|---|
State | New |
Headers | show |
Series | target/ppc: add support to disable-tcg | expand |
On 5/12/21 9:08 AM, Bruno Larsen (billionai) wrote: > Moved this function that is required in !TCG cases into a > common code file > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > +void ppc_store_sdr1(CPUPPCState *env, target_ulong value) > +{ > + PowerPCCPU *cpu = env_archcpu(env); > + qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value); > + assert(!cpu->vhyp); > +#if defined(TARGET_PPC64) > + if (mmu_is_64bit(env->mmu_model)) { > + target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE; > + target_ulong htabsize = value & SDR_64_HTABSIZE; > + > + if (value & ~sdr_mask) { > + error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1", > + value & ~sdr_mask); As a separate cleanup patch, this should not use error_report but qemu_log(LOG_GUEST_ERROR, ...). r~
On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote: > Moved this function that is required in !TCG cases into a > common code file The reasons it's needed by !TCG are kind of bogus, related to weirdness in the way KVM PR works. But it's fair not to care about that right now, so, applied to ppc-for-6.1. > > Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> > --- > target/ppc/cpu.c | 29 +++++++++++++++++++++++++++++ > target/ppc/mmu_helper.c | 26 -------------------------- > 2 files changed, 29 insertions(+), 26 deletions(-) > > diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c > index cb794e9f4f..0ab7ac1af1 100644 > --- a/target/ppc/cpu.c > +++ b/target/ppc/cpu.c > @@ -20,7 +20,10 @@ > #include "qemu/osdep.h" > #include "cpu.h" > #include "cpu-models.h" > +#include "cpu-qom.h" > +#include "exec/log.h" > #include "fpu/softfloat-helpers.h" > +#include "mmu-hash64.h" > > target_ulong cpu_read_xer(CPUPPCState *env) > { > @@ -61,3 +64,29 @@ uint32_t ppc_get_vscr(CPUPPCState *env) > uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0; > return env->vscr | (sat << VSCR_SAT); > } > + > +void ppc_store_sdr1(CPUPPCState *env, target_ulong value) > +{ > + PowerPCCPU *cpu = env_archcpu(env); > + qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value); > + assert(!cpu->vhyp); > +#if defined(TARGET_PPC64) > + if (mmu_is_64bit(env->mmu_model)) { > + target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE; > + target_ulong htabsize = value & SDR_64_HTABSIZE; > + > + if (value & ~sdr_mask) { > + error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1", > + value & ~sdr_mask); > + value &= sdr_mask; > + } > + if (htabsize > 28) { > + error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1", > + htabsize); > + return; > + } > + } > +#endif /* defined(TARGET_PPC64) */ > + /* FIXME: Should check for valid HTABMASK values in 32-bit case */ > + env->spr[SPR_SDR1] = value; > +} > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c > index ca88658cba..06e1ebdcbc 100644 > --- a/target/ppc/mmu_helper.c > +++ b/target/ppc/mmu_helper.c > @@ -2085,32 +2085,6 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr) > > /*****************************************************************************/ > /* Special registers manipulation */ > -void ppc_store_sdr1(CPUPPCState *env, target_ulong value) > -{ > - PowerPCCPU *cpu = env_archcpu(env); > - qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value); > - assert(!cpu->vhyp); > -#if defined(TARGET_PPC64) > - if (mmu_is_64bit(env->mmu_model)) { > - target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE; > - target_ulong htabsize = value & SDR_64_HTABSIZE; > - > - if (value & ~sdr_mask) { > - error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1", > - value & ~sdr_mask); > - value &= sdr_mask; > - } > - if (htabsize > 28) { > - error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1", > - htabsize); > - return; > - } > - } > -#endif /* defined(TARGET_PPC64) */ > - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ > - env->spr[SPR_SDR1] = value; > -} > - > #if defined(TARGET_PPC64) > void ppc_store_ptcr(CPUPPCState *env, target_ulong value) > {
On 13/05/2021 00:50, David Gibson wrote: > On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote: >> Moved this function that is required in !TCG cases into a >> common code file > The reasons it's needed by !TCG are kind of bogus, related to > weirdness in the way KVM PR works. But it's fair not to care about > that right now, so, applied to ppc-for-6.1. Now that the future is here, I was looking into why might the reasons be bogus. From what I can see, what should be happening is just storing what was retrieved by the kvm ioctl, right? Am I missing something? -- Bruno Piazera Larsen Instituto de Pesquisas ELDORADO <https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station> Departamento Computação Embarcada Analista de Software Trainee Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On Mon, May 31, 2021 at 03:17:47PM -0300, Bruno Piazera Larsen wrote: > > On 13/05/2021 00:50, David Gibson wrote: > > On Wed, May 12, 2021 at 11:08:04AM -0300, Bruno Larsen (billionai) wrote: > > > Moved this function that is required in !TCG cases into a > > > common code file > > The reasons it's needed by !TCG are kind of bogus, related to > > weirdness in the way KVM PR works. But it's fair not to care about > > that right now, so, applied to ppc-for-6.1. > Now that the future is here, I was looking into why might the reasons be > bogus. From what I can see, what should be happening is just storing what > was retrieved by the kvm ioctl, right? Am I missing something? Actually, I was mixing this up with something else. We invoke ppc_store_sdr1() in several places from !TCG code. From explicitly KVM code in kvmppc_book_get_sregs() - since we more or less trust KVM we could in theory just store directly to the value. However we also use it in common code in machine.c in the loadvm path. I don't want to bypass ppc_store_sdr1() there so that we have a common place for all SDR1 updates, for sanity checking and any secondary alterations we need. So having this in common code is the right thing to do. (In case you care, the "bogus reason" thing I was thinking of in connection to SDR1 handling is actually the special encode_hpt_for_kvm_pr() case in kvmppc_but_books_sregs. That's there because KVM PR requires us to inform us of the *qemu userspace* location of the hash table for PAPR guests via the SDR1 value, which doesn't really make sense)
diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c index cb794e9f4f..0ab7ac1af1 100644 --- a/target/ppc/cpu.c +++ b/target/ppc/cpu.c @@ -20,7 +20,10 @@ #include "qemu/osdep.h" #include "cpu.h" #include "cpu-models.h" +#include "cpu-qom.h" +#include "exec/log.h" #include "fpu/softfloat-helpers.h" +#include "mmu-hash64.h" target_ulong cpu_read_xer(CPUPPCState *env) { @@ -61,3 +64,29 @@ uint32_t ppc_get_vscr(CPUPPCState *env) uint32_t sat = (env->vscr_sat.u64[0] | env->vscr_sat.u64[1]) != 0; return env->vscr | (sat << VSCR_SAT); } + +void ppc_store_sdr1(CPUPPCState *env, target_ulong value) +{ + PowerPCCPU *cpu = env_archcpu(env); + qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value); + assert(!cpu->vhyp); +#if defined(TARGET_PPC64) + if (mmu_is_64bit(env->mmu_model)) { + target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE; + target_ulong htabsize = value & SDR_64_HTABSIZE; + + if (value & ~sdr_mask) { + error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1", + value & ~sdr_mask); + value &= sdr_mask; + } + if (htabsize > 28) { + error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1", + htabsize); + return; + } + } +#endif /* defined(TARGET_PPC64) */ + /* FIXME: Should check for valid HTABMASK values in 32-bit case */ + env->spr[SPR_SDR1] = value; +} diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c index ca88658cba..06e1ebdcbc 100644 --- a/target/ppc/mmu_helper.c +++ b/target/ppc/mmu_helper.c @@ -2085,32 +2085,6 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr) /*****************************************************************************/ /* Special registers manipulation */ -void ppc_store_sdr1(CPUPPCState *env, target_ulong value) -{ - PowerPCCPU *cpu = env_archcpu(env); - qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value); - assert(!cpu->vhyp); -#if defined(TARGET_PPC64) - if (mmu_is_64bit(env->mmu_model)) { - target_ulong sdr_mask = SDR_64_HTABORG | SDR_64_HTABSIZE; - target_ulong htabsize = value & SDR_64_HTABSIZE; - - if (value & ~sdr_mask) { - error_report("Invalid bits 0x"TARGET_FMT_lx" set in SDR1", - value & ~sdr_mask); - value &= sdr_mask; - } - if (htabsize > 28) { - error_report("Invalid HTABSIZE 0x" TARGET_FMT_lx" stored in SDR1", - htabsize); - return; - } - } -#endif /* defined(TARGET_PPC64) */ - /* FIXME: Should check for valid HTABMASK values in 32-bit case */ - env->spr[SPR_SDR1] = value; -} - #if defined(TARGET_PPC64) void ppc_store_ptcr(CPUPPCState *env, target_ulong value) {
Moved this function that is required in !TCG cases into a common code file Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br> --- target/ppc/cpu.c | 29 +++++++++++++++++++++++++++++ target/ppc/mmu_helper.c | 26 -------------------------- 2 files changed, 29 insertions(+), 26 deletions(-)