[PATCH] x86/shadow: don't leave trace record field uninitialized

[PATCH] x86/shadow: don't leave trace record field uninitialized

[Jan Beulich]

The emulation_count field is set only conditionally right now. Convert
all field setting to an initializer, thus guaranteeing that field to be
set to 0 (default initialized) when GUEST_PAGING_LEVELS != 3.

While there also drop the "event" local variable, thus eliminating an
instance of the being phased out u32 type.

Coverity ID: 1598430
Fixes: 9a86ac1aa3d2 ("xentrace 5/7: Additional tracing for the shadow code")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2093,20 +2093,18 @@ static inline void trace_shadow_emulate(
             guest_l1e_t gl1e, write_val;
             guest_va_t va;
             uint32_t flags:29, emulation_count:3;
-        } d;
-        u32 event;
-
-        event = TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS-2)<<8);
-
-        d.gl1e = gl1e;
-        d.write_val.l1 = this_cpu(trace_emulate_write_val);
-        d.va = va;
+        } d = {
+            .gl1e = gl1e,
+            .write_val.l1 = this_cpu(trace_emulate_write_val),
+            .va = va,
 #if GUEST_PAGING_LEVELS == 3
-        d.emulation_count = this_cpu(trace_extra_emulation_count);
+            .emulation_count = this_cpu(trace_extra_emulation_count),
 #endif
-        d.flags = this_cpu(trace_shadow_path_flags);
+            .flags = this_cpu(trace_shadow_path_flags),
+        };
 
-        trace(event, sizeof(d), &d);
+        trace(TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS - 2) << 8),
+              sizeof(d), &d);
     }
 }
 #endif /* CONFIG_HVM */

Re: [PATCH] x86/shadow: don't leave trace record field uninitialized

[Roger Pau Monné]

On Wed, May 22, 2024 at 12:17:30PM +0200, Jan Beulich wrote:
> The emulation_count field is set only conditionally right now. Convert
> all field setting to an initializer, thus guaranteeing that field to be
> set to 0 (default initialized) when GUEST_PAGING_LEVELS != 3.
> 
> While there also drop the "event" local variable, thus eliminating an
> instance of the being phased out u32 type.
> 
> Coverity ID: 1598430
> Fixes: 9a86ac1aa3d2 ("xentrace 5/7: Additional tracing for the shadow code")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] x86/shadow: don't leave trace record field uninitialized

[Andrew Cooper]

On 22/05/2024 11:17 am, Jan Beulich wrote:
> The emulation_count field is set only conditionally right now. Convert
> all field setting to an initializer, thus guaranteeing that field to be
> set to 0 (default initialized) when GUEST_PAGING_LEVELS != 3.
>
> While there also drop the "event" local variable, thus eliminating an
> instance of the being phased out u32 type.
>
> Coverity ID: 1598430
> Fixes: 9a86ac1aa3d2 ("xentrace 5/7: Additional tracing for the shadow code")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This is an improvement, but there's a related mess right next to it.

I think this would be a whole lot better with a couple of tweaks, if
you're willing to wait a little for me to try.

~Andrew

Re: [PATCH] x86/shadow: don't leave trace record field uninitialized

[Oleksii K.]

On Wed, 2024-05-22 at 12:17 +0200, Jan Beulich wrote:
> The emulation_count field is set only conditionally right now.
> Convert
> all field setting to an initializer, thus guaranteeing that field to
> be
> set to 0 (default initialized) when GUEST_PAGING_LEVELS != 3.
> 
> While there also drop the "event" local variable, thus eliminating an
> instance of the being phased out u32 type.
> 
> Coverity ID: 1598430
> Fixes: 9a86ac1aa3d2 ("xentrace 5/7: Additional tracing for the shadow
> code")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Oleksii Kurochko <oleksii.kurochko.com>

~ Oleksii
> 
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -2093,20 +2093,18 @@ static inline void trace_shadow_emulate(
>              guest_l1e_t gl1e, write_val;
>              guest_va_t va;
>              uint32_t flags:29, emulation_count:3;
> -        } d;
> -        u32 event;
> -
> -        event = TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS-2)<<8);
> -
> -        d.gl1e = gl1e;
> -        d.write_val.l1 = this_cpu(trace_emulate_write_val);
> -        d.va = va;
> +        } d = {
> +            .gl1e = gl1e,
> +            .write_val.l1 = this_cpu(trace_emulate_write_val),
> +            .va = va,
>  #if GUEST_PAGING_LEVELS == 3
> -        d.emulation_count = this_cpu(trace_extra_emulation_count);
> +            .emulation_count =
> this_cpu(trace_extra_emulation_count),
>  #endif
> -        d.flags = this_cpu(trace_shadow_path_flags);
> +            .flags = this_cpu(trace_shadow_path_flags),
> +        };
>  
> -        trace(event, sizeof(d), &d);
> +        trace(TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS - 2) << 8),
> +              sizeof(d), &d);
>      }
>  }
>  #endif /* CONFIG_HVM */