Re: [Xen-devel] [PATCH RFC] x86/HVM: meet xentrace's expectations on emulation event data

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 09 August 2018 09:02
> To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>
> Subject: [PATCH RFC] x86/HVM: meet xentrace's expectations on emulation
> event data
> 
> According to the logic in hvm_mmio_assist_process(), 64 bits of data are
> expected with 64-bit addresses, and 32 bits of data with 32-bit ones. I
> don't think this is very reasonable, but I'm also not going to touch the
> consumer side, the more that it is anyway not very helpful for the code
> here to only ever supply 32 bits of data (despite the field being 64
> bits wide, and having been even in the 32-bit days of Xen).

I suspect the data field was 64 bits so it could hold addresses, and no-one 
thought about 64 bits of immediate data (which of course you can only get with 
MMIO) whereas you can have 64-bit addresses even with PIO. Anyway the change 
LGTM...

> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
> RFC: Untested; solely based on the observation of "(no data)" in a trace
>      where it was entirely unclear why no data would have been available.
>      I just so happened that the guest had more than 4Gb of memory, and
>      hence addresses were not representable as 32-bit values.
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -30,7 +30,7 @@ struct hvmemul_cache
>  static void hvmtrace_io_assist(const ioreq_t *p)
>  {
>      unsigned int size, event;
> -    unsigned char buffer[12];
> +    unsigned char buffer[16];
> 
>      if ( likely(!tb_init_done) )
>          return;
> @@ -47,8 +47,11 @@ static void hvmtrace_io_assist(const ior
> 
>      if ( !p->data_is_ptr )
>      {
> -        *(uint32_t *)&buffer[size] = p->data;
> -        size += 4;
> +        if ( size == 4 )
> +            *(uint32_t *)&buffer[size] = p->data;
> +        else
> +            *(uint64_t *)&buffer[size] = p->data;
> +        size *= 2;
>      }
> 
>      trace_var(event, 0/*!cycles*/, size, buffer);
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel