Skip to contentSkip to conversation

gpu: nova-core: firmware: fwsec: use zeroed() to zero-pad buffers

PATCH Superseded Thorsten Blum 3 participants 5 messages Oct 6 – Oct 7

Subsystems (4)
DRM drivers DRM Rust nova-core (Rust) nova-drm (Rust)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Thorsten Blum

author
#0 lore raw

PATCH 1/1 gpu: nova-core: firmware: fwsec: use zeroed() to zero-pad buffers

Use KVec::zeroed() to allocate the zero-padded microcode buffer and copy the bootloader code into it. Also use KVVec::zeroed() to initialize the firmware object's leading zero padding.

Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/gpu/nova-core/firmware/fwsec/bootloader.rs | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/gpu/nova-core/firmware/fwsec/bootloader.rs +3 -6
index ec4d92317a93..37dcd4bf4711 100644
@@ -137,10 +137,8 @@ pub(crate) fn new(
                 .align_up(Alignment::new::<{ falcon::MEM_BLOCK_ALIGNMENT }>())
                 .ok_or(EINVAL)?;
 
-            let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
-            ucode.extend_from_slice(code, GFP_KERNEL)?;
-            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
-
+            let mut ucode = KVec::zeroed(aligned_code_size, GFP_KERNEL)?;
+            ucode[..code.len()].copy_from_slice(code);
             ucode
         };
 

@@ -162,8 +160,7 @@ pub(crate) fn new(
                     .ok_or(EOVERFLOW)?
             };
 
-            let mut firmware_obj = KVVec::new();
-            firmware_obj.extend_with(align_padding, 0u8, GFP_KERNEL)?;
+            let mut firmware_obj = KVVec::zeroed(align_padding, GFP_KERNEL)?;
             firmware_obj.extend_from_slice(firmware.ucode.0.as_slice(), GFP_KERNEL)?;
 
             (
maintainer
#1 lore raw

drivers/gpu/nova-core/firmware/fwsec/bootloader.rs pub(crate) fn new( · line 141

Quoted 6 lines · Thorsten Blum
drivers/gpu/nova-core/firmware/fwsec/bootloader.rs +2 -4
-            let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
-            ucode.extend_from_slice(code, GFP_KERNEL)?;
-            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
-
+            let mut ucode = KVec::zeroed(aligned_code_size, GFP_KERNEL)?;
+            ucode[..code.len()].copy_from_slice(code);

I kinda dislike that this is now a potential panic, but it does better express the intent.

Can you please add a corresponding // PANIC comment?

Thanks,
Danilo

maintainer
#2 lore raw
Quoted 10 lines · Danilo Krummrich

drivers/gpu/nova-core/firmware/fwsec/bootloader.rs pub(crate) fn new( · line 141

Quoted 6 lines · Thorsten Blum
drivers/gpu/nova-core/firmware/fwsec/bootloader.rs +2 -4
-            let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
-            ucode.extend_from_slice(code, GFP_KERNEL)?;
-            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
-
+            let mut ucode = KVec::zeroed(aligned_code_size, GFP_KERNEL)?;
+            ucode[..code.len()].copy_from_slice(code);

I kinda dislike that this is now a potential panic, but it does better express the intent.

IMHO the original code expresses the intent just as well, and avoids writing the area covered by code twice, on top of not introducing a potential panic site.

maintainer
#3 lore raw
Quoted 9 lines · Alexandre Courbot
Quoted 8 lines · Danilo Krummrich
Quoted 6 lines · Thorsten Blum
drivers/gpu/nova-core/firmware/fwsec/bootloader.rs +2 -4
-            let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
-            ucode.extend_from_slice(code, GFP_KERNEL)?;
-            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
-
+            let mut ucode = KVec::zeroed(aligned_code_size, GFP_KERNEL)?;
+            ucode[..code.len()].copy_from_slice(code);
Quoted 6 lines
Quoted 2 lines

I kinda dislike that this is now a potential panic, but it does better express the intent.

IMHO the original code expresses the intent just as well, and avoids writing the area covered by code twice, on top of not introducing a potential panic site.

I'm referring to the fact that the original code expresses "I may allocate" three times, while the intent is that the allocation is exactly aligned_code_size.

This is much more clear with the new code, if we really want to avoid a potential panic here, we could use get_mut(), but the ergonomics isn't great.

That said, I wouldn't mind adding something like Vec::extend_within_capacity(), which fails if there's not enough capacity.

Thanks,
Danilo

maintainer
#4 lore raw
Quoted 21 lines · Danilo Krummrich
Quoted 9 lines · Alexandre Courbot
Quoted 8 lines · Danilo Krummrich
Quoted 6 lines · Thorsten Blum
drivers/gpu/nova-core/firmware/fwsec/bootloader.rs +2 -4
-            let mut ucode = KVec::with_capacity(aligned_code_size, GFP_KERNEL)?;
-            ucode.extend_from_slice(code, GFP_KERNEL)?;
-            ucode.resize(aligned_code_size, 0, GFP_KERNEL)?;
-
+            let mut ucode = KVec::zeroed(aligned_code_size, GFP_KERNEL)?;
+            ucode[..code.len()].copy_from_slice(code);
Quoted 6 lines
Quoted 2 lines

I kinda dislike that this is now a potential panic, but it does better express the intent.

IMHO the original code expresses the intent just as well, and avoids writing the area covered by code twice, on top of not introducing a potential panic site.

I'm referring to the fact that the original code expresses "I may allocate" three times, while the intent is that the allocation is exactly aligned_code_size.

Quoted 5 lines

This is much more clear with the new code, if we really want to avoid a potential panic here, we could use get_mut(), but the ergonomics isn't great.

That said, I wouldn't mind adding something like Vec::extend_within_capacity(), which fails if there's not enough capacity.

You know how I feel about panic sites in the code, even if they appear to be untaken today. :)

Another thing we could do is replace KVec::with_capacity with KVec::new, removing one potential allocation site. The added padding (which in practice is likely to be 0-sized per the comment in the code) is unlikely to trigger a resize anyway.