Re: [Qemu-devel] [PATCH 12/12] trace: [all] Add "guest_vmem" event

[Richard Henderson]

On 02/04/2014 12:01 PM, Lluís Vilanova wrote:
> Richard Henderson writes:
> 
>> On 01/31/2014 08:10 AM, Lluís Vilanova wrote:
>>> +#define ldub(p)    ({ trace_guest_vmem(p, 1, 0); ldub_raw(p);    })
> 
>> Are you sure you want to log these here?  Uses of these macros are
>> not restricted to the guest.  Therefore you could wind up with e.g.
>> PCI device accesses being attributed to the target cpu.
> 
> These defines are only enabled in user-level mode.
> 
> But I wrote them really long ago, and I just realized they are not
> up-to-date. The changes should also cover the 'cpu_*_data' and 'cpu_*_kernel'
> variants. These macros are mostly used in helpers (e.g., helper_boundl).

Yes, I know.  But they're also used in non-cpu contexts such as __get_user
(i.e. kernel accesses) or virtio.c (i.e. device accesses).

>> These are going to result in double-logging the same access with
> 
>>> +#define tcg_gen_qemu_ld_i32(val, addr, idx, memop)      \
>>> +    do {                                                \
>>> +        uint8_t _memop_size = _tcg_memop_size(memop);   \
>>> +        trace_guest_vmem_tcg(addr, _memop_size, 0);     \
>>> +        tcg_gen_qemu_ld_i32(val, addr, idx, memop);     \
>>> +    } while (0)
> 
>> ... these.
> 
> I don't see how 'tcg_gen_qemu_ld_i32' gets to call 'cpu_ldl_data'

tcg_gen_qemu_ld_i32 triggers some inline code, with an out of line fallback in
softmmu-template.h.

You're logging both on the main path (before qemu_ld_i32 opcode) and in the
fallback path.  It would be one thing if you logged something different in the
fallback path, but you're not.  You're using the exact same logging routine.

> What do you mean by non-target accesses? Accesses not directly "encoded" in 
> the
> semantics of a guest instruction? If so, I did this in purpose (like the 
> helper
> example on the top).

No, I mean accesses initiated by a device.  Such things are rare, I admit,
since most devices use physical addresses not virtual.  But e.g. old Sparc
hardware used a single mmu to handle both cpu and bus accesses.

> The 'trace_guest_vmem_tcg' function just calls a helper generator function in
> "helper.h", which cannot be included in "tcg.c". Another possibility is to 
> just
> forget about using "helper.h", and instead "manually" generate the call to the
> helper; but using macros seems to me it's easier to maintain.

You simply need to move the declarations somewhere else.  See e.g.
tcg-runtime.h for a set of helpers shared across all translators.


r~