[12/12] trace: [all] Add "guest_vmem" event

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 include/exec/cpu-all.h        |   58 +++++++++++++++++++++--------------------
 include/exec/exec-all.h       |    3 ++
 include/exec/softmmu_header.h |   17 ++++++++++++
 tcg/tcg-op.h                  |    8 ++++++
 tcg/tcg.c                     |    1 +
 trace-events                  |   15 +++++++++++
 trace/tcg-op-internal.h       |   55 +++++++++++++++++++++++++++++++++++++++
 7 files changed, 129 insertions(+), 28 deletions(-)
 create mode 100644 trace/tcg-op-internal.h

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.

> --- a/include/exec/softmmu_header.h
> +++ b/include/exec/softmmu_header.h
> @@ -25,6 +25,11 @@
>   * You should have received a copy of the GNU Lesser General Public
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
> +
> +#if !defined(TRACE_TCG_CODE_ACCESSOR)
> +#include "trace.h"
> +#endif
> +
>  #if DATA_SIZE == 8
>  #define SUFFIX q
>  #define USUFFIX q
> @@ -88,6 +93,10 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
>      target_ulong addr;
>      int mmu_idx;
>  
> +#if !defined(TRACE_TCG_CODE_ACCESSOR)
> +    trace_guest_vmem(ptr, DATA_SIZE, 0);
> +#endif

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.

Of course, those softmmu functions are also used by the system emulators in the
same way the ldub macro above is used for userland emulation.  So again you
have non-target accesses being attributed to the target.

Also, doing this action with macros, here, seems truly backward.  Why not
simply modify the real tcg_gen_qemu_ld_i32 in tcg.c?


r~

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).


>> --- a/include/exec/softmmu_header.h
>> +++ b/include/exec/softmmu_header.h
>> @@ -25,6 +25,11 @@
>> * You should have received a copy of the GNU Lesser General Public
>> * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> */
>> +
>> +#if !defined(TRACE_TCG_CODE_ACCESSOR)
>> +#include "trace.h"
>> +#endif
>> +
>> #if DATA_SIZE == 8
>> #define SUFFIX q
>> #define USUFFIX q
>> @@ -88,6 +93,10 @@ glue(glue(cpu_ld, USUFFIX), MEMSUFFIX)(CPUArchState *env, target_ulong ptr)
>> target_ulong addr;
>> int mmu_idx;
>> 
>> +#if !defined(TRACE_TCG_CODE_ACCESSOR)
>> +    trace_guest_vmem(ptr, DATA_SIZE, 0);
>> +#endif

> 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' (for
example). I also did this long ago, so maybe it changed, but a quick look at the
code shows that in softmmu-mode, a TLB miss performs a slow path access with the
functions in "softmmu_template.h", not "softmmu_exec.h" (which includes
"softmmu_header.h").

Maybe you're referring to some case I've missed.


> Of course, those softmmu functions are also used by the system emulators in the
> same way the ldub macro above is used for userland emulation.  So again you
> have non-target accesses being attributed to the target.

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).


> Also, doing this action with macros, here, seems truly backward.  Why not
> simply modify the real tcg_gen_qemu_ld_i32 in tcg.c?

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.


Thanks,
  Lluis

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~

Richard Henderson writes:

> 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).

The macro "__get_user" accesses memory using physical addresses, so it is
not traced.

Grepping at "virtio.c" shows that only "ld*_phys" and "ld*_p" are used (which
are not traced). Similarly happens for stores.

I looked at refereces for ld/st macros (without the "_raw" and "_p" suffixes),
and no non-target code came up (I'm including accesses - directly or indirectly
- initiated from helper functions as target code).

Am I missing something?


>>> 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.

Aha, but I'm only tracing functions in "softmmu_header.h" (e.g.,
"cpu_ldub_kernel"). I've looked at the result of preprocessing
"softmmu_template.h" (from "target-i386/mem_helper.c"), and the functions there
(e.g., "helper_ret_ldub_mmu") perform translation themselves and then access
physical memory (e.g., "ldub_p"). AFAIK, I did not add any tracing to that path.


>> 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.

Hmmm, I'll have to take a look at that. Do you have any function or file for the
Sparc case from the top of your head? Admittedly, the whole memory access flow
is quite convoluted in QEMU, specially due to the big number of macros
involved. I guess whether something could be done to simplify all this a little
bit.


>> 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.

Nice. I'll move them there.


Thanks,
  Lluis