Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>,
	Richard Henderson <richard.henderson@linaro.org>
Cc: "QEMU Developers" <qemu-devel@nongnu.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	kvm-devel <kvm@vger.kernel.org>, "Thomas Huth" <thuth@redhat.com>,
	qemu-arm <qemu-arm@nongnu.org>, "Fam Zheng" <fam@euphon.net>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH v3 03/19] target/arm: Restrict DC-CVAP instruction to TCG accel
Date: Fri, 17 Apr 2020 15:49:37 +0200	[thread overview]
Message-ID: <5d9606c9-f812-f629-e03f-d72ddbce05ee@redhat.com> (raw)
In-Reply-To: <CAFEAcA8K-njh=TyjS_4deD4wTjhqnc=t6SQB1DbKgWWS5rixSQ@mail.gmail.com>

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
> 

