diff mbox series

[v2,12/12] x86/trace: Clean up trace handling

Message ID 20210920172529.24932-13-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series xen/trace: Fix leakage of uninitialised stack into the tracebuffer | expand

Commit Message

Andrew Cooper Sept. 20, 2021, 5:25 p.m. UTC
Use more appropriate types.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * New
---
 xen/arch/x86/irq.c             |   4 +-
 xen/arch/x86/mm/p2m-pt.c       |   6 +-
 xen/arch/x86/mm/shadow/multi.c |   2 +-
 xen/arch/x86/pv/trace.c        | 159 +++++++++++++++++++----------------------
 4 files changed, 78 insertions(+), 93 deletions(-)

Comments

Jan Beulich Sept. 21, 2021, 4:08 p.m. UTC | #1
On 20.09.2021 19:25, Andrew Cooper wrote:
> Use more appropriate types.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a suggestion:

> @@ -48,30 +45,28 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
>  
>      if ( is_pv_32bit_vcpu(current) )
>      {
> -        struct __packed {
> -            u32 eip, addr, error_code;
> -        } d;
> -
> -        d.eip = eip;
> -        d.addr = addr;
> -        d.error_code = error_code;
> +        struct {
> +            uint32_t eip, addr, error_code;
> +        } d = {
> +            .eip = eip,
> +            .addr = addr,
> +            .error_code = error_code,
> +        };
>  
>          __trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
>      }
>      else
>      {
>          struct __packed {
> -            unsigned long eip, addr;
> -            u32 error_code;
> -        } d;
> -        unsigned event;
> -
> -        d.eip = eip;
> -        d.addr = addr;
> -        d.error_code = error_code;
> -        event = TRC_PV_PAGE_FAULT;
> -        event |= TRC_64_FLAG;
> -        __trace_var(event, 1, sizeof(d), &d);
> +            uint64_t eip, addr;

Like you've done in __trace_pv_trap() and __trace_ptwr_emulation(),
perhaps name the field "rip" here as well?

Jan
Andrew Cooper Sept. 21, 2021, 6:34 p.m. UTC | #2
On 21/09/2021 17:08, Jan Beulich wrote:
> On 20.09.2021 19:25, Andrew Cooper wrote:
>> Use more appropriate types.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> with a suggestion:
>
>> @@ -48,30 +45,28 @@ void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
>>  
>>      if ( is_pv_32bit_vcpu(current) )
>>      {
>> -        struct __packed {
>> -            u32 eip, addr, error_code;
>> -        } d;
>> -
>> -        d.eip = eip;
>> -        d.addr = addr;
>> -        d.error_code = error_code;
>> +        struct {
>> +            uint32_t eip, addr, error_code;
>> +        } d = {
>> +            .eip = eip,
>> +            .addr = addr,
>> +            .error_code = error_code,
>> +        };
>>  
>>          __trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
>>      }
>>      else
>>      {
>>          struct __packed {
>> -            unsigned long eip, addr;
>> -            u32 error_code;
>> -        } d;
>> -        unsigned event;
>> -
>> -        d.eip = eip;
>> -        d.addr = addr;
>> -        d.error_code = error_code;
>> -        event = TRC_PV_PAGE_FAULT;
>> -        event |= TRC_64_FLAG;
>> -        __trace_var(event, 1, sizeof(d), &d);
>> +            uint64_t eip, addr;
> Like you've done in __trace_pv_trap() and __trace_ptwr_emulation(),
> perhaps name the field "rip" here as well?

Fixed.  (Honestly - these all start to blur together given how large the
series is.)

~Andrew
diff mbox series

Patch

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index a1693f92dd92..67cbf6b979dc 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -130,8 +130,8 @@  static void _trace_irq_mask(uint32_t event, int irq, int vector,
                             const cpumask_t *mask)
 {
     struct {
-        unsigned int irq:16, vec:16;
-        unsigned int mask[6];
+        uint16_t irq, vec;
+        uint32_t mask[6];
     } d = {
        .irq = irq,
        .vec = vector,
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 5a0c0f5aceff..09c99d78aa40 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -609,9 +609,9 @@  p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
     if ( tb_init_done )
     {
         struct {
-            u64 gfn, mfn;
-            int p2mt;
-            int d:16,order:16;
+            uint64_t gfn, mfn;
+            uint32_t p2mt;
+            uint16_t d, order;
         } t;
 
         t.gfn = gfn;
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 8bb028c2e2fa..15265fc81dca 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2118,7 +2118,7 @@  static inline void trace_shadow_emulate(guest_l1e_t gl1e, unsigned long va)
                so put it first for alignment sake. */
             guest_l1e_t gl1e, write_val;
             guest_va_t va;
-            unsigned flags:29, emulation_count:3;
+            uint32_t flags:29, emulation_count:3;
         } d;
         u32 event;
 
diff --git a/xen/arch/x86/pv/trace.c b/xen/arch/x86/pv/trace.c
index 550c22765bae..a952fbc1eb0f 100644
--- a/xen/arch/x86/pv/trace.c
+++ b/xen/arch/x86/pv/trace.c
@@ -7,38 +7,35 @@  void __trace_pv_trap(int trapnr, unsigned long eip,
 {
     if ( is_pv_32bit_vcpu(current) )
     {
-        struct __packed {
-            unsigned eip:32,
-                trapnr:15,
-                use_error_code:1,
-                error_code:16;
-        } d;
-
-        d.eip = eip;
-        d.trapnr = trapnr;
-        d.error_code = error_code;
-        d.use_error_code=!!use_error_code;
+        struct {
+            uint32_t eip;
+            uint16_t trapnr:15;
+            bool use_error_code:1;
+            uint16_t error_code;
+        } d = {
+            .eip            = eip,
+            .trapnr         = trapnr,
+            .use_error_code = use_error_code,
+            .error_code     = error_code,
+        };
 
         __trace_var(TRC_PV_TRAP, 1, sizeof(d), &d);
     }
     else
     {
         struct __packed {
-            unsigned long eip;
-            unsigned trapnr:15,
-                use_error_code:1,
-                error_code:16;
-        } d;
-        unsigned event;
-
-        d.eip = eip;
-        d.trapnr = trapnr;
-        d.error_code = error_code;
-        d.use_error_code=!!use_error_code;
-
-        event = TRC_PV_TRAP;
-        event |= TRC_64_FLAG;
-        __trace_var(event, 1, sizeof(d), &d);
+            uint64_t rip;
+            uint16_t trapnr:15;
+            bool use_error_code:1;
+            uint16_t error_code;
+        } d = {
+            .rip            = eip,
+            .trapnr         = trapnr,
+            .use_error_code = use_error_code,
+            .error_code     = error_code,
+        };
+
+        __trace_var(TRC_PV_TRAP | TRC_64_FLAG, 1, sizeof(d), &d);
     }
 }
 
@@ -48,30 +45,28 @@  void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
 
     if ( is_pv_32bit_vcpu(current) )
     {
-        struct __packed {
-            u32 eip, addr, error_code;
-        } d;
-
-        d.eip = eip;
-        d.addr = addr;
-        d.error_code = error_code;
+        struct {
+            uint32_t eip, addr, error_code;
+        } d = {
+            .eip = eip,
+            .addr = addr,
+            .error_code = error_code,
+        };
 
         __trace_var(TRC_PV_PAGE_FAULT, 1, sizeof(d), &d);
     }
     else
     {
         struct __packed {
-            unsigned long eip, addr;
-            u32 error_code;
-        } d;
-        unsigned event;
-
-        d.eip = eip;
-        d.addr = addr;
-        d.error_code = error_code;
-        event = TRC_PV_PAGE_FAULT;
-        event |= TRC_64_FLAG;
-        __trace_var(event, 1, sizeof(d), &d);
+            uint64_t eip, addr;
+            uint32_t error_code;
+        } d = {
+            .eip = eip,
+            .addr = addr,
+            .error_code = error_code,
+        };
+
+        __trace_var(TRC_PV_PAGE_FAULT | TRC_64_FLAG, 1, sizeof(d), &d);
     }
 }
 
@@ -83,10 +78,7 @@  void __trace_trap_one_addr(unsigned event, unsigned long va)
         __trace_var(event, 1, sizeof(d), &d);
     }
     else
-    {
-        event |= TRC_64_FLAG;
-        __trace_var(event, 1, sizeof(va), &va);
-    }
+        __trace_var(event | TRC_64_FLAG, 1, sizeof(va), &va);
 }
 
 void __trace_trap_two_addr(unsigned event, unsigned long va1,
@@ -94,22 +86,25 @@  void __trace_trap_two_addr(unsigned event, unsigned long va1,
 {
     if ( is_pv_32bit_vcpu(current) )
     {
-        struct __packed {
-            u32 va1, va2;
-        } d;
-        d.va1=va1;
-        d.va2=va2;
+        struct {
+            uint32_t va1, va2;
+        } d = {
+            .va1 = va1,
+            .va2 = va2,
+        };
+
         __trace_var(event, 1, sizeof(d), &d);
     }
     else
     {
-        struct __packed {
-            unsigned long va1, va2;
-        } d;
-        d.va1=va1;
-        d.va2=va2;
-        event |= TRC_64_FLAG;
-        __trace_var(event, 1, sizeof(d), &d);
+        struct {
+            uint64_t va1, va2;
+        } d = {
+            .va1 = va1,
+            .va2 = va2,
+        };
+
+        __trace_var(event | TRC_64_FLAG, 1, sizeof(d), &d);
     }
 }
 
@@ -117,40 +112,30 @@  void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
 {
     unsigned long eip = guest_cpu_user_regs()->rip;
 
-    /* We have a couple of different modes to worry about:
-     * - 32-on-32: 32-bit pte, 32-bit virtual addresses
-     * - pae-on-pae, pae-on-64: 64-bit pte, 32-bit virtual addresses
-     * - 64-on-64: 64-bit pte, 64-bit virtual addresses
-     * pae-on-64 is the only one that requires extra code; in all other
-     * cases, "unsigned long" is the size of a guest virtual address.
-     */
-
     if ( is_pv_32bit_vcpu(current) )
     {
-        struct __packed {
-            l1_pgentry_t pte;
-            u32 addr, eip;
-        } d;
-        d.addr = addr;
-        d.eip = eip;
-        d.pte = npte;
+        struct {
+            uint64_t pte;
+            uint32_t addr, eip;
+        } d = {
+            .pte  = l1e_get_intpte(npte),
+            .addr = addr,
+            .eip  = eip,
+        };
 
         __trace_var(TRC_PV_PTWR_EMULATION_PAE, 1, sizeof(d), &d);
     }
     else
     {
         struct {
-            l1_pgentry_t pte;
-            unsigned long addr, eip;
-        } d;
-        unsigned event;
-
-        d.addr = addr;
-        d.eip = eip;
-        d.pte = npte;
-
-        event = TRC_PV_PTWR_EMULATION;
-        event |= TRC_64_FLAG;
-        __trace_var(event, 1/*tsc*/, sizeof(d), &d);
+            uint64_t pte;
+            uint64_t addr, rip;
+        } d = {
+            .pte  = l1e_get_intpte(npte),
+            .addr = addr,
+            .rip  = eip,
+        };
+
+        __trace_var(TRC_PV_PTWR_EMULATION | TRC_64_FLAG, 1, sizeof(d), &d);
     }
 }