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

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

[Jan Beulich]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 09 August 2018 09:02
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; George Dunlap <George.Dunlap@citrix.com>
> 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@suse.com>

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

> ---
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping: [PATCH RFC] x86/HVM: meet xentrace's expectations on emulation event data

[Jan Beulich]

>>> On 09.08.18 at 11:09, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 09 August 2018 09:02
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>; George Dunlap <George.Dunlap@citrix.com>
>> 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@suse.com>
> 
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Thanks.

George, strictly speaking I could put this in with just this, but
considering ...

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

... this I'd prefer to have your consent.

Jan

>> --- 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Andrew Cooper]

On 09/08/18 09:01, Jan Beulich wrote:
> 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).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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.

Unfortunately, I don't think this helps much.

From what I can tell, xentrace_format doesn't understand the TRC_64_FLAG
versions of these events at all, and xenalyze uses:

    union {
        struct {
            unsigned int gpa;
            unsigned int data;
        } x32;
        struct {
            unsigned long long gpa;
            unsigned int data;
        } x64;
    } *r = (typeof(r))h->d;

to pull the data back out.  AFAICT, this matches what Xen is currently
writing, and is equally wrong.

~Andrew

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

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

[Jan Beulich]

>>> On 29.08.18 at 16:22, <andrew.cooper3@citrix.com> wrote:
> On 09/08/18 09:01, Jan Beulich wrote:
>> 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).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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.
> 
> Unfortunately, I don't think this helps much.
> 
> From what I can tell, xentrace_format doesn't understand the TRC_64_FLAG
> versions of these events at all, and xenalyze uses:
> 
>     union {
>         struct {
>             unsigned int gpa;
>             unsigned int data;
>         } x32;
>         struct {
>             unsigned long long gpa;
>             unsigned int data;
>         } x64;
>     } *r = (typeof(r))h->d;
> 
> to pull the data back out.  AFAICT, this matches what Xen is currently
> writing, and is equally wrong.

Are you sure? You've not quoted the relevant other half:

    union {
        unsigned event;
        struct {
            unsigned minor:8,
                x64:1,
                write:2;
        };
    } mevt = { .event = ri->event };

with the check then being

    if(mevt.x64) {

If what consumer and producer matched in (good or bad) behavior,
I'd expect "(no data)" to not appear here, but it was definitely
observed (on an older Xen version) by Olaf.

Jan



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