Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
On Fri, 17 Apr 2020 at 15:19, Philippe Mathieu-Daudé 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
Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
On 4/17/20 3:54 PM, Peter Maydell wrote:
On Fri, 17 Apr 2020 at 14:49, Philippe Mathieu-Daudé wrote:
On 3/16/20 9:11 PM, Peter Maydell wrote:
On Mon, 16 Mar 2020 at 19:36, Richard Henderson
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
Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
On Fri, 17 Apr 2020 at 14:49, Philippe Mathieu-Daudé wrote: > > On 3/16/20 9:11 PM, Peter Maydell wrote: > > On Mon, 16 Mar 2020 at 19:36, Richard Henderson > > 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
Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
On 3/16/20 9:11 PM, Peter Maydell wrote:
On Mon, 16 Mar 2020 at 19:36, Richard Henderson
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
Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
On Mon, 16 Mar 2020 at 19:36, Richard Henderson 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
Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
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é
> ---
> 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);
>
