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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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
>