| Message ID | 20200316160634.3386-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support disabling TCG on ARM (part 2) | expand |
On 3/16/20 9:06 AM, Philippe Mathieu-Daudé wrote: > Under KVM the 'Data or unified Cache line Clean by VA to PoP' > instruction will trap. > > Fixes: 0d57b4999 ("Add support for DC CVAP & DC CVADP ins") > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > target/arm/helper.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index b61ee73d18..924deffd65 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -6777,7 +6777,7 @@ static const ARMCPRegInfo rndr_reginfo[] = { > REGINFO_SENTINEL > }; > > -#ifndef CONFIG_USER_ONLY > +#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG) > static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque, > uint64_t value) > { > @@ -6820,9 +6820,9 @@ static const ARMCPRegInfo dcpodp_reg[] = { > .accessfn = aa64_cacheop_poc_access, .writefn = dccvap_writefn }, > REGINFO_SENTINEL > }; > -#endif /*CONFIG_USER_ONLY*/ > +#endif /* !CONFIG_USER_ONLY && CONFIG_TCG */ I'm not 100% sure how the system regs function under kvm. If they are not used at all, then we should avoid them all en masse an not piecemeal like this. If they are used for something, then we should keep them registered and change the writefn like so: #ifdef CONFIG_TCG /* existing stuff */ #else /* Handled by hardware accelerator. */ g_assert_not_reached(); #endif r~ > > -#endif > +#endif /* TARGET_AARCH64 */ > > static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo *ri, > bool isread) > @@ -7929,7 +7929,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) > if (cpu_isar_feature(aa64_rndr, cpu)) { > define_arm_cp_regs(cpu, rndr_reginfo); > } > -#ifndef CONFIG_USER_ONLY > +#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG) > /* Data Cache clean instructions up to PoP */ > if (cpu_isar_feature(aa64_dcpop, cpu)) { > define_one_arm_cp_reg(cpu, dcpop_reg); > @@ -7938,8 +7938,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) > define_one_arm_cp_reg(cpu, dcpodp_reg); > } > } > -#endif /*CONFIG_USER_ONLY*/ > -#endif > +#endif /* !CONFIG_USER_ONLY && CONFIG_TCG */ > +#endif /* TARGET_AARCH64 */ > > if (cpu_isar_feature(any_predinv, cpu)) { > define_arm_cp_regs(cpu, predinv_reginfo); >
On Mon, 16 Mar 2020 at 19:36, Richard Henderson <richard.henderson@linaro.org> wrote: > I'm not 100% sure how the system regs function under kvm. > > If they are not used at all, then we should avoid them all en masse an not > piecemeal like this. > > If they are used for something, then we should keep them registered and change > the writefn like so: > > #ifdef CONFIG_TCG > /* existing stuff */ > #else > /* Handled by hardware accelerator. */ > g_assert_not_reached(); > #endif (1) for those registers where we need to know the value within QEMU code (notably anything involved in VA-to-PA translation, as this is used by gdbstub accesses, etc, but sometimes we want other register values too): the sysreg struct is what lets us map from the KVM register to the field in the CPU struct when we do a sync of data to/from the kernel. (2) for other registers, the sync lets us make the register visible as an r/o register in the gdbstub. (this is not very important, but it's nice) (3) Either way, the sync works via the raw_read/raw_write accessors (this is a big part of what they're for), which are supposed to just stuff the data into/out of the underlying CPU struct field. (But watch out because we fall back to using the non-raw read/writefn if there's no raw version provided for a particular register.) If a regdef is marked as NO_RAW then it means there is no raw access and we don't sync the value. (4) I think that in KVM mode we won't deliberately do non-raw accesses, and a quick grep through of the places that do 'readfn' accesses supports that. thanks -- PMM
On 3/16/20 9:11 PM, Peter Maydell wrote: > On Mon, 16 Mar 2020 at 19:36, Richard Henderson > <richard.henderson@linaro.org> wrote: >> I'm not 100% sure how the system regs function under kvm. >> >> If they are not used at all, then we should avoid them all en masse an not >> piecemeal like this. >> >> If they are used for something, then we should keep them registered and change >> the writefn like so: >> >> #ifdef CONFIG_TCG >> /* existing stuff */ >> #else >> /* Handled by hardware accelerator. */ >> g_assert_not_reached(); >> #endif I ended with that patch because dccvap_writefn() calls probe_read() which is an inlined call to probe_access(), which itself is only defined when using TCG. So with KVM either linking fails or I get: target/arm/helper.c: In function ‘dccvap_writefn’: target/arm/helper.c:6898:13: error: implicit declaration of function ‘probe_read’; haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC()); ^~~~~~~~~~ I'll use your suggestion which works for me: -- >8 -- --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -330,8 +330,20 @@ static inline void tlb_flush_by_mmuidx_all_cpus_synced(CPUState *cpu, { } #endif + +#ifdef CONFIG_TCG void *probe_access(CPUArchState *env, target_ulong addr, int size, MMUAccessType access_type, int mmu_idx, uintptr_t retaddr); +#else +static inline void *probe_access(CPUArchState *env, + target_ulong addr, int size, + MMUAccessType access_type, + int mmu_idx, uintptr_t retaddr) +{ + /* Handled by hardware accelerator. */ + g_assert_not_reached(); +} +#endif /* CONFIG_TCG */ static inline void *probe_write(CPUArchState *env, target_ulong addr, int size, int mmu_idx, uintptr_t retaddr) --- > > (1) for those registers where we need to know the value within > QEMU code (notably anything involved in VA-to-PA translation, > as this is used by gdbstub accesses, etc, but sometimes we > want other register values too): the sysreg struct is > what lets us map from the KVM register to the field in the > CPU struct when we do a sync of data to/from the kernel. > > (2) for other registers, the sync lets us make the register > visible as an r/o register in the gdbstub. (this is not > very important, but it's nice) > > (3) Either way, the sync works via the raw_read/raw_write > accessors (this is a big part of what they're for), which are > supposed to just stuff the data into/out of the underlying > CPU struct field. (But watch out because we fall back to > using the non-raw read/writefn if there's no raw version > provided for a particular register.) If a regdef is marked > as NO_RAW then it means there is no raw access and we don't > sync the value. > > (4) I think that in KVM mode we won't deliberately do > non-raw accesses, and a quick grep through of the places > that do 'readfn' accesses supports that. > > thanks > -- PMM >
On Fri, 17 Apr 2020 at 14:49, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 3/16/20 9:11 PM, Peter Maydell wrote: > > On Mon, 16 Mar 2020 at 19:36, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> I'm not 100% sure how the system regs function under kvm. > >> > >> If they are not used at all, then we should avoid them all en masse an not > >> piecemeal like this. > >> > >> If they are used for something, then we should keep them registered and change > >> the writefn like so: > >> > >> #ifdef CONFIG_TCG > >> /* existing stuff */ > >> #else > >> /* Handled by hardware accelerator. */ > >> g_assert_not_reached(); > >> #endif > > I ended with that patch because dccvap_writefn() calls probe_read() > which is an inlined call to probe_access(), which itself is only defined > when using TCG. So with KVM either linking fails or I get: > > target/arm/helper.c: In function ‘dccvap_writefn’: > target/arm/helper.c:6898:13: error: implicit declaration of function > ‘probe_read’; > haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC()); > ^~~~~~~~~~ IN this particular case, DC CVAP is really a system insn rather than a 'register'; our register struct for it is marked up as ARM_CP_NO_RAW, which means we'll effectively ignore it when running KVM (it will not be migrated, have its state synced against the kernel, or be visible in gdb). If dccvap_writefn() ever gets called somehow that's a bug, so having it end up with an assert is the right thing. > I'll use your suggestion which works for me: Your suggested patch isn't quite the same as RTH's suggestion, because it puts the assert inside a stub probe_read() implementation rather than having the ifdef at the level of the writefn body. I have no opinion on whether one or the other of these is preferable. thanks -- PMM
On 4/17/20 3:54 PM, Peter Maydell wrote: > On Fri, 17 Apr 2020 at 14:49, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> On 3/16/20 9:11 PM, Peter Maydell wrote: >>> On Mon, 16 Mar 2020 at 19:36, Richard Henderson >>> <richard.henderson@linaro.org> wrote: >>>> I'm not 100% sure how the system regs function under kvm. >>>> >>>> If they are not used at all, then we should avoid them all en masse an not >>>> piecemeal like this. >>>> >>>> If they are used for something, then we should keep them registered and change >>>> the writefn like so: >>>> >>>> #ifdef CONFIG_TCG >>>> /* existing stuff */ >>>> #else >>>> /* Handled by hardware accelerator. */ >>>> g_assert_not_reached(); >>>> #endif >> >> I ended with that patch because dccvap_writefn() calls probe_read() >> which is an inlined call to probe_access(), which itself is only defined >> when using TCG. So with KVM either linking fails or I get: >> >> target/arm/helper.c: In function ‘dccvap_writefn’: >> target/arm/helper.c:6898:13: error: implicit declaration of function >> ‘probe_read’; >> haddr = probe_read(env, vaddr, dline_size, mem_idx, GETPC()); >> ^~~~~~~~~~ > > IN this particular case, DC CVAP is really a system insn rather > than a 'register'; our register struct for it is marked up as > ARM_CP_NO_RAW, which means we'll effectively ignore it when > running KVM (it will not be migrated, have its state synced > against the kernel, or be visible in gdb). If dccvap_writefn() > ever gets called somehow that's a bug, so having it end up > with an assert is the right thing. > >> I'll use your suggestion which works for me: > > Your suggested patch isn't quite the same as RTH's suggestion, > because it puts the assert inside a stub probe_read() > implementation rather than having the ifdef at the level > of the writefn body. I have no opinion on whether one or > the other of these is preferable. I'll let Richard modify the writefn() bodies if required, as he understand what they do :) Btw since we have this rule: obj-$(call lnot,$(CONFIG_TCG)) += tcg-stub.o I'll use the following patch which is less intrusive: -- >8 -- index 677191a69c..e4bbf997aa 100644 --- a/accel/stubs/tcg-stub.c +++ b/accel/stubs/tcg-stub.c @@ -22,3 +22,10 @@ void tb_flush(CPUState *cpu) void tlb_set_dirty(CPUState *cpu, target_ulong vaddr) { } + +void *probe_access(CPUArchState *env, target_ulong addr, int size, + MMUAccessType access_type, int mmu_idx, uintptr_t retaddr) +{ + /* Handled by hardware accelerator. */ + g_assert_not_reached(); +} --- > > thanks > -- PMM >
On Fri, 17 Apr 2020 at 15:19, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 4/17/20 3:54 PM, Peter Maydell wrote: > > Your suggested patch isn't quite the same as RTH's suggestion, > > because it puts the assert inside a stub probe_read() > > implementation rather than having the ifdef at the level > > of the writefn body. I have no opinion on whether one or > > the other of these is preferable. > > I'll let Richard modify the writefn() bodies if required, as he > understand what they do :) RTH is suggesting static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque, uint64_t value) { #ifdef CONFIG_TCG entire current body of function goes here #else g_assert_not_reached(); #endif } If we take that approach then the stub probe_read() would be pointless, so we should do one or the other, not both. > Btw since we have this rule: > > obj-$(call lnot,$(CONFIG_TCG)) += tcg-stub.o > > I'll use the following patch which is less intrusive: This is temptingly less ifdeffery, certainly -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index b61ee73d18..924deffd65 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -6777,7 +6777,7 @@ static const ARMCPRegInfo rndr_reginfo[] = { REGINFO_SENTINEL }; -#ifndef CONFIG_USER_ONLY +#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG) static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *opaque, uint64_t value) { @@ -6820,9 +6820,9 @@ static const ARMCPRegInfo dcpodp_reg[] = { .accessfn = aa64_cacheop_poc_access, .writefn = dccvap_writefn }, REGINFO_SENTINEL }; -#endif /*CONFIG_USER_ONLY*/ +#endif /* !CONFIG_USER_ONLY && CONFIG_TCG */ -#endif +#endif /* TARGET_AARCH64 */ static CPAccessResult access_predinv(CPUARMState *env, const ARMCPRegInfo *ri, bool isread) @@ -7929,7 +7929,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) if (cpu_isar_feature(aa64_rndr, cpu)) { define_arm_cp_regs(cpu, rndr_reginfo); } -#ifndef CONFIG_USER_ONLY +#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG) /* Data Cache clean instructions up to PoP */ if (cpu_isar_feature(aa64_dcpop, cpu)) { define_one_arm_cp_reg(cpu, dcpop_reg); @@ -7938,8 +7938,8 @@ void register_cp_regs_for_features(ARMCPU *cpu) define_one_arm_cp_reg(cpu, dcpodp_reg); } } -#endif /*CONFIG_USER_ONLY*/ -#endif +#endif /* !CONFIG_USER_ONLY && CONFIG_TCG */ +#endif /* TARGET_AARCH64 */ if (cpu_isar_feature(any_predinv, cpu)) { define_arm_cp_regs(cpu, predinv_reginfo);
Under KVM the 'Data or unified Cache line Clean by VA to PoP' instruction will trap. Fixes: 0d57b4999 ("Add support for DC CVAP & DC CVADP ins") Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- target/arm/helper.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)