Skip to contentSkip to conversation

gpu: nova-core: check for overflow to DMATRFBASE1

PATCH Applied Timur Tabi 3 participants 8 messages Jan 7 – Jan 23

Subsystems (4)
lore
Share & tools
lore

Timur Tabi

author
#0 lore raw

PATCH v2 1/2 gpu: nova-core: check for overflow to DMATRFBASE1

The NV_PFALCON_FALCON_DMATRFBASE/1 register pair supports DMA addresses up to 49 bits only, but the write to DMATRFBASE1 could exceed that. To mitigate, check first that the DMA address will fit.

Fixes 69f5cd67ce41 ("gpu: nova-core: add falcon register definitions and base code")

---

v2: Improved comment and moved check earlier in function

 drivers/gpu/nova-core/falcon.rs | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/gpu/nova-core/falcon.rs +6 -0
index 82c661aef594..6ae0490caffa 100644
@@ -472,6 +472,12 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
             return Err(EINVAL);
         }
 
+        // The DMATRFBASE/1 register pair only supports a 49-bit address.
+        if dma_start > kernel::dma::DmaMask::new::<49>().value() {
+            dev_err!(self.dev, "DMA address {:#x} exceeds 49 bits\n", dma_start);
+            return Err(ERANGE);
+        }
+
         // DMA transfers can only be done in units of 256 bytes. Compute how many such transfers we
         // need to perform.
         let num_transfers = load_offsets.len.div_ceil(DMA_LEN);

base-commit: 2d7b4a44fb768e1887e7e4cdd8b86817ccd9c3bf

Signature · 1 line
2.52.0
author
#1 lore raw

PATCH v2 2/2 gpu: nova-core: add missing newlines to several print strings

Although the dev_xx!() macro calls do not technically require terminating newlines for the format strings, they should be added anyway to maintain consistency, both within Rust code and with the C versions.

---
v2: added some more strings that were missed

 drivers/gpu/nova-core/falcon.rs           |  6 +++---
 drivers/gpu/nova-core/falcon/hal/ga102.rs |  4 ++--
 drivers/gpu/nova-core/fb.rs               |  2 +-
 drivers/gpu/nova-core/gpu.rs              |  2 +-
 drivers/gpu/nova-core/gsp/boot.rs         |  2 +-
 drivers/gpu/nova-core/gsp/cmdq.rs         |  2 +-
 drivers/gpu/nova-core/gsp/sequencer.rs    | 10 +++++-----
 drivers/gpu/nova-core/vbios.rs            |  2 +-
 8 files changed, 15 insertions(+), 15 deletions(-)
drivers/gpu/nova-core/falcon.rs +3 -3
index 6ae0490caffa..4e1da0b9cf4f 100644
@@ -466,7 +466,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
         if dma_start % DmaAddress::from(DMA_LEN) > 0 {
             dev_err!(
                 self.dev,
-                "DMA transfer start addresses must be a multiple of {}",
+                "DMA transfer start addresses must be a multiple of {}\n",
                 DMA_LEN
             );
             return Err(EINVAL);
@@ -489,11 +489,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
             .and_then(|size| size.checked_add(load_offsets.src_start))
         {
             None => {
-                dev_err!(self.dev, "DMA transfer length overflow");
+                dev_err!(self.dev, "DMA transfer length overflow\n");
                 return Err(EOVERFLOW);
             }
             Some(upper_bound) if usize::from_safe_cast(upper_bound) > fw.size() => {
-                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object");
+                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object\n");
                 return Err(EINVAL);
             }
             Some(_) => (),
drivers/gpu/nova-core/falcon/hal/ga102.rs +2 -2
index 69a7a95cac16..0bdfe45a2d03 100644
@@ -52,7 +52,7 @@ fn signature_reg_fuse_version_ga102(
     let ucode_idx = match usize::from(ucode_id) {
         ucode_id @ 1..=regs::NV_FUSE_OPT_FPF_SIZE => ucode_id - 1,
         _ => {
-            dev_err!(dev, "invalid ucode id {:#x}", ucode_id);
+            dev_err!(dev, "invalid ucode id {:#x}\n", ucode_id);
             return Err(EINVAL);
         }
     };
@@ -66,7 +66,7 @@ fn signature_reg_fuse_version_ga102(
     } else if engine_id_mask & 0x0400 != 0 {
         regs::NV_FUSE_OPT_FPF_GSP_UCODE1_VERSION::read(bar, ucode_idx).data()
     } else {
-        dev_err!(dev, "unexpected engine_id_mask {:#x}", engine_id_mask);
+        dev_err!(dev, "unexpected engine_id_mask {:#x}\n", engine_id_mask);
         return Err(EINVAL);
     };
 
drivers/gpu/nova-core/fb.rs +1 -1
index 3c9cf151786c..c62abcaed547 100644
@@ -80,7 +80,7 @@ pub(crate) fn unregister(&self, bar: &Bar0) {
             let _ = hal.write_sysmem_flush_page(bar, 0).inspect_err(|e| {
                 dev_warn!(
                     &self.device,
-                    "failed to unregister sysmem flush page: {:?}",
+                    "failed to unregister sysmem flush page: {:?}\n",
                     e
                 )
             });
drivers/gpu/nova-core/gpu.rs +1 -1
index 50d76092fbdd..9b042ef1a308 100644
@@ -268,7 +268,7 @@ pub(crate) fn new<'a>(
             // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
             _: {
                 gfw::wait_gfw_boot_completion(bar)
-                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete"))?;
+                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete\n"))?;
             },
 
             sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
drivers/gpu/nova-core/gsp/boot.rs +1 -1
index a53d80620468..da21447be663 100644
@@ -82,7 +82,7 @@ fn run_fwsec_frts(
         if frts_status != 0 {
             dev_err!(
                 dev,
-                "FWSEC-FRTS returned with error code {:#x}",
+                "FWSEC-FRTS returned with error code {:#x}\n",
                 frts_status
             );
 
drivers/gpu/nova-core/gsp/cmdq.rs +1 -1
index 6f946d14868a..3c01fc6e6b6a 100644
@@ -615,7 +615,7 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
         {
             dev_err!(
                 self.dev,
-                "GSP RPC: receive: Call {} - bad checksum",
+                "GSP RPC: receive: Call {} - bad checksum\n",
                 header.sequence()
             );
             return Err(EIO);
drivers/gpu/nova-core/gsp/sequencer.rs +5 -5
index d78a30fbb70f..d6c489c39092 100644
@@ -121,7 +121,7 @@ pub(crate) fn new(data: &[u8], dev: &device::Device) -> Result<(Self, usize)> {
         };
 
         if data.len() < size {
-            dev_err!(dev, "Data is not enough for command");
+            dev_err!(dev, "Data is not enough for command\n");
             return Err(EINVAL);
         }
 
@@ -320,7 +320,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         cmd_result.map_or_else(
             |_err| {
-                dev_err!(self.dev, "Error parsing command at offset {}", offset);
+                dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
                 None
             },
             |(cmd, size)| {
@@ -382,7 +382,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
             dev: params.dev,
         };
 
-        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands");
+        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands\n");
 
         for cmd_result in sequencer.iter() {
             match cmd_result {
@@ -390,7 +390,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
                 Err(e) => {
                     dev_err!(
                         sequencer.dev,
-                        "Error running command at index {}",
+                        "Error running command at index {}\n",
                         sequencer.seq_info.cmd_index
                     );
                     return Err(e);
@@ -400,7 +400,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
 
         dev_dbg!(
             sequencer.dev,
-            "CPU Sequencer commands completed successfully"
+            "CPU Sequencer commands completed successfully\n"
         );
         Ok(())
     }
drivers/gpu/nova-core/vbios.rs +1 -1
index 7c26e4a2d61c..e4eae9385f47 100644
@@ -790,7 +790,7 @@ fn falcon_data_ptr(&self) -> Result<u32> {
         // read the 4 bytes at the offset specified in the token
         let offset = usize::from(token.data_offset);
         let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
-            dev_err!(self.base.dev, "Failed to convert data slice to array");
+            dev_err!(self.base.dev, "Failed to convert data slice to array\n");
             EINVAL
         })?;
 
Signature · 1 line
2.52.0
Reviewed-by on v2 2/2
#2 lore raw

Re: [PATCH v2 2/2] gpu: nova-core: add missing newlines to several print strings

Quoted 8 lines · Timur Tabi

Although the dev_xx!() macro calls do not technically require terminating newlines for the format strings, they should be added anyway to maintain consistency, both within Rust code and with the C versions.

---
v2: added some more strings that were missed

Quoted 9 lines
 drivers/gpu/nova-core/falcon.rs           |  6 +++---
 drivers/gpu/nova-core/falcon/hal/ga102.rs |  4 ++--
 drivers/gpu/nova-core/fb.rs               |  2 +-
 drivers/gpu/nova-core/gpu.rs              |  2 +-
 drivers/gpu/nova-core/gsp/boot.rs         |  2 +-
 drivers/gpu/nova-core/gsp/cmdq.rs         |  2 +-
 drivers/gpu/nova-core/gsp/sequencer.rs    | 10 +++++-----
 drivers/gpu/nova-core/vbios.rs            |  2 +-
 8 files changed, 15 insertions(+), 15 deletions(-)

OK, looks like you got them all:

Codebase WITH \n WITHOUT \n % with \n
-------------------------------------------------------------------
nova-core (dev_*!) 73 0 100%

thanks,

Signature · 1 line
John Hubbard
Quoted 164 lines
drivers/gpu/nova-core/falcon.rs +3 -3
index 6ae0490caffa..4e1da0b9cf4f 100644
@@ -466,7 +466,7 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
         if dma_start % DmaAddress::from(DMA_LEN) > 0 {
             dev_err!(
                 self.dev,
-                "DMA transfer start addresses must be a multiple of {}",
+                "DMA transfer start addresses must be a multiple of {}\n",
                 DMA_LEN
             );
             return Err(EINVAL);
@@ -489,11 +489,11 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
             .and_then(|size| size.checked_add(load_offsets.src_start))
         {
             None => {
-                dev_err!(self.dev, "DMA transfer length overflow");
+                dev_err!(self.dev, "DMA transfer length overflow\n");
                 return Err(EOVERFLOW);
             }
             Some(upper_bound) if usize::from_safe_cast(upper_bound) > fw.size() => {
-                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object");
+                dev_err!(self.dev, "DMA transfer goes beyond range of DMA object\n");
                 return Err(EINVAL);
             }
             Some(_) => (),
drivers/gpu/nova-core/falcon/hal/ga102.rs +2 -2
index 69a7a95cac16..0bdfe45a2d03 100644
@@ -52,7 +52,7 @@ fn signature_reg_fuse_version_ga102(
     let ucode_idx = match usize::from(ucode_id) {
         ucode_id @ 1..=regs::NV_FUSE_OPT_FPF_SIZE => ucode_id - 1,
         _ => {
-            dev_err!(dev, "invalid ucode id {:#x}", ucode_id);
+            dev_err!(dev, "invalid ucode id {:#x}\n", ucode_id);
             return Err(EINVAL);
         }
     };
@@ -66,7 +66,7 @@ fn signature_reg_fuse_version_ga102(
     } else if engine_id_mask & 0x0400 != 0 {
         regs::NV_FUSE_OPT_FPF_GSP_UCODE1_VERSION::read(bar, ucode_idx).data()
     } else {
-        dev_err!(dev, "unexpected engine_id_mask {:#x}", engine_id_mask);
+        dev_err!(dev, "unexpected engine_id_mask {:#x}\n", engine_id_mask);
         return Err(EINVAL);
     };
 
drivers/gpu/nova-core/fb.rs +1 -1
index 3c9cf151786c..c62abcaed547 100644
@@ -80,7 +80,7 @@ pub(crate) fn unregister(&self, bar: &Bar0) {
             let _ = hal.write_sysmem_flush_page(bar, 0).inspect_err(|e| {
                 dev_warn!(
                     &self.device,
-                    "failed to unregister sysmem flush page: {:?}",
+                    "failed to unregister sysmem flush page: {:?}\n",
                     e
                 )
             });
drivers/gpu/nova-core/gpu.rs +1 -1
index 50d76092fbdd..9b042ef1a308 100644
@@ -268,7 +268,7 @@ pub(crate) fn new<'a>(
             // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
             _: {
                 gfw::wait_gfw_boot_completion(bar)
-                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete"))?;
+                    .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete\n"))?;
             },
 
             sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
drivers/gpu/nova-core/gsp/boot.rs +1 -1
index a53d80620468..da21447be663 100644
@@ -82,7 +82,7 @@ fn run_fwsec_frts(
         if frts_status != 0 {
             dev_err!(
                 dev,
-                "FWSEC-FRTS returned with error code {:#x}",
+                "FWSEC-FRTS returned with error code {:#x}\n",
                 frts_status
             );
 
drivers/gpu/nova-core/gsp/cmdq.rs +1 -1
index 6f946d14868a..3c01fc6e6b6a 100644
@@ -615,7 +615,7 @@ fn wait_for_msg(&self, timeout: Delta) -> Result<GspMessage<'_>> {
         {
             dev_err!(
                 self.dev,
-                "GSP RPC: receive: Call {} - bad checksum",
+                "GSP RPC: receive: Call {} - bad checksum\n",
                 header.sequence()
             );
             return Err(EIO);
drivers/gpu/nova-core/gsp/sequencer.rs +5 -5
index d78a30fbb70f..d6c489c39092 100644
@@ -121,7 +121,7 @@ pub(crate) fn new(data: &[u8], dev: &device::Device) -> Result<(Self, usize)> {
         };
 
         if data.len() < size {
-            dev_err!(dev, "Data is not enough for command");
+            dev_err!(dev, "Data is not enough for command\n");
             return Err(EINVAL);
         }
 
@@ -320,7 +320,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         cmd_result.map_or_else(
             |_err| {
-                dev_err!(self.dev, "Error parsing command at offset {}", offset);
+                dev_err!(self.dev, "Error parsing command at offset {}\n", offset);
                 None
             },
             |(cmd, size)| {
@@ -382,7 +382,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
             dev: params.dev,
         };
 
-        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands");
+        dev_dbg!(sequencer.dev, "Running CPU Sequencer commands\n");
 
         for cmd_result in sequencer.iter() {
             match cmd_result {
@@ -390,7 +390,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
                 Err(e) => {
                     dev_err!(
                         sequencer.dev,
-                        "Error running command at index {}",
+                        "Error running command at index {}\n",
                         sequencer.seq_info.cmd_index
                     );
                     return Err(e);
@@ -400,7 +400,7 @@ pub(crate) fn run(cmdq: &mut Cmdq, params: GspSequencerParams<'a>) -> Result {
 
         dev_dbg!(
             sequencer.dev,
-            "CPU Sequencer commands completed successfully"
+            "CPU Sequencer commands completed successfully\n"
         );
         Ok(())
     }
drivers/gpu/nova-core/vbios.rs +1 -1
index 7c26e4a2d61c..e4eae9385f47 100644
@@ -790,7 +790,7 @@ fn falcon_data_ptr(&self) -> Result<u32> {
         // read the 4 bytes at the offset specified in the token
         let offset = usize::from(token.data_offset);
         let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
-            dev_err!(self.base.dev, "Failed to convert data slice to array");
+            dev_err!(self.base.dev, "Failed to convert data slice to array\n");
             EINVAL
         })?;
 
#3 lore raw

Re: [PATCH v2 2/2] gpu: nova-core: add missing newlines to several print strings

Quoted 8 lines · John Hubbard
Quoted 7 lines · Timur Tabi

Although the dev_xx!() macro calls do not technically require terminating newlines for the format strings, they should be added anyway to maintain consistency, both within Rust code and with the C versions.

---
v2: added some more strings that were missed

...

Quoted 2 lines

Hi Danilo,

Are you comfortable merging these two patches to drm-rust-next? I think they are ready to go at this point.

thanks,

Signature · 1 line
John Hubbard
maintainer
#4 lore raw

Re: [PATCH v2 2/2] gpu: nova-core: add missing newlines to several print strings

Quoted 2 lines · John Hubbard

Are you comfortable merging these two patches to drm-rust-next? I think they are ready to go at this point.

Yes, they are already on my apply list.

maintainer Applied
#5 lore raw

Re: [PATCH v2 2/2] gpu: nova-core: add missing newlines to several print strings

Quoted 5 lines · Danilo Krummrich
Quoted 2 lines · John Hubbard

Are you comfortable merging these two patches to drm-rust-next? I think they are ready to go at this point.

Yes, they are already on my apply list.

Applied to drm-rust-next, thanks!

author
#6 lore raw
In reply to Timur Tabi
Quoted 8 lines · Timur Tabi

The NV_PFALCON_FALCON_DMATRFBASE/1 register pair supports DMA addresses up to 49 bits only, but the write to DMATRFBASE1 could exceed that. To mitigate, check first that the DMA address will fit.

Fixes 69f5cd67ce41 ("gpu: nova-core: add falcon register definitions and base code")

Danilo, is it too late to make 6.20 with this patch?

maintainer
#7 lore raw
Quoted 11 lines · Timur Tabi
Quoted 8 lines · Timur Tabi

The NV_PFALCON_FALCON_DMATRFBASE/1 register pair supports DMA addresses up to 49 bits only, but the write to DMATRFBASE1 could exceed that. To mitigate, check first that the DMA address will fit.

Fixes 69f5cd67ce41 ("gpu: nova-core: add falcon register definitions and base code")

Danilo, is it too late to make 6.20 with this patch?

This patch is already in drm-rust-next.