lib/pem: fix SDP queue mapping #2
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The firmware previously mapped the maximum number of SDP queues to each pseudo device, which limited the number of queues available per SDP VF. Firmware has been fixed to map only the required number of queues to each pseudo device, allowing SDP VFs to be configured with desired number of queues. Patch also fixes miscellaneous address range checks in register read & write routines. Fixes: e734ded ("lib/vfio: use pseudo RVU SDP devices") Signed-off-by: Vamsi Attunuru <vattunuru@marvell.com>
Reviewer's GuideThis PR secures SDP register access by consolidating offset validation into a helper and changing the read API to return status codes, and fixes SDP queue mapping by using the correct VF count mask and calculating PF mailbox write offsets based on actual VF IDs with conditional writes. Sequence Diagram: sdp_reg_read Operation with Offset ValidationsequenceDiagram
participant Caller
participant sdp_reg_read as "sdp_reg_read()"
participant sdp_offset_valid as "sdp_offset_valid()"
participant BAR2_Memory as "BAR2 Memory"
Caller->>sdp_reg_read: Call sdp_reg_read(sdp_pdev, offset, &val)
sdp_reg_read->>sdp_offset_valid: Check sdp_offset_valid(sdp_pdev, offset)
sdp_offset_valid-->>sdp_reg_read: isValid
alt offset is valid
sdp_reg_read->>BAR2_Memory: Read value from sdp_pdev->mem[BAR2].addr + offset
BAR2_Memory-->>sdp_reg_read: value
sdp_reg_read-->>Caller: Return 0 (success), *val = value
else offset is invalid
sdp_reg_read-->>Caller: Return -ENOMEM (error)
end
Sequence Diagram: sdp_reg_write Operation with Offset ValidationsequenceDiagram
participant Caller
participant sdp_reg_write as "sdp_reg_write()"
participant sdp_offset_valid as "sdp_offset_valid()"
participant BAR2_Memory as "BAR2 Memory"
Caller->>sdp_reg_write: Call sdp_reg_write(sdp_pdev, offset, val)
sdp_reg_write->>sdp_offset_valid: Check sdp_offset_valid(sdp_pdev, offset)
sdp_offset_valid-->>sdp_reg_write: isValid
alt offset is valid
sdp_reg_write->>BAR2_Memory: Write val to sdp_pdev->mem[BAR2].addr + offset
BAR2_Memory-->>sdp_reg_write: (ack)
sdp_reg_write-->>Caller: Return 0 (success)
else offset is invalid
sdp_reg_write-->>Caller: Return -ENOMEM (error)
end
Sequence Diagram: sdp_init Corrected SDP Mailbox ConfigurationsequenceDiagram
participant sdp_init as "sdp_init()"
participant DeviceRegisters as "Device Registers"
sdp_init->>DeviceRegisters: sdp_reg_read(sdp_pdev, SDP_EPFX_RINFO(0), ®_val)
DeviceRegisters-->>sdp_init: reg_val
sdp_init->>sdp_init: num_vfs = (reg_val >> SDP_EPFX_RINFO_NVVF_SHIFT) & 0x7f // Corrected VF count extraction
sdp_init->>sdp_init: rpvf = (reg_val >> SDP_EPFX_RINFO_RPVF_SHIFT) & 0xf
sdp_init->>sdp_init: vfid = num_vfs >> 1
alt vfid is non-zero
sdp_init->>sdp_init: Calculate info for mailbox
sdp_init->>sdp_init: offset = SDP_PF_MBOX_DATA(vfid * rpvf) // New offset calculation
sdp_init->>DeviceRegisters: sdp_reg_write(sdp_pdev, offset, info) // Conditional PF mailbox write
end
Class Diagram: Updated SDP Utility Functions in sdp.cclassDiagram
class dao_vfio_device {
+mem: void[]
%% Struct holding memory region info like BAR2 addr and len
}
class SdpFunctions {
<<Module: sdp.c>>
+sdp_offset_valid(sdp_pdev: dao_vfio_device*, offset: uint64_t): int
+sdp_reg_write(sdp_pdev: dao_vfio_device*, offset: uint64_t, val: uint64_t): int
+sdp_reg_read(sdp_pdev: dao_vfio_device*, offset: uint64_t, val_ptr: uint64_t*): int
+sdp_reg_addr(sdp_pdev: dao_vfio_device*, offset: uint64_t): uint64_t*
+sdp_init(sdp_pdev: dao_vfio_device*): int
}
SdpFunctions ..> dao_vfio_device : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @anoobj - I've reviewed your changes - here's some feedback:
- Propagate and check the return value of sdp_reg_read in callers (e.g., sdp_init and dao_pem_host_interrupt_setup) instead of ignoring errors.
- Add null-pointer validation for sdp_pdev in sdp_reg_write and sdp_reg_addr similar to the checks in sdp_reg_read to prevent potential dereferences of an invalid device.
- Double-check that using SDP_PF_MBOX_DATA(vfid * rpvf) produces the correct mailbox offset, as multiplying vfid by rpvf may not align with the intended mailbox index spacing.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| static inline int sdp_offset_valid(struct dao_vfio_device *sdp_pdev, uint64_t offset) | ||
| { | ||
| return offset < sdp_pdev->mem[DAO_VFIO_DEV_BAR2].len; |
There was a problem hiding this comment.
issue (bug_risk): Offset validation doesn't account for full 8-byte access width
Validate that offset + sizeof(uint64_t) <= len to prevent out-of-bounds access when performing 8-byte operations.
Comment on lines
+28
to
29
| if (!sdp_offset_valid(sdp_pdev, offset)) | ||
| return -ENOMEM; |
There was a problem hiding this comment.
suggestion: Use a more appropriate error code than -ENOMEM for invalid offsets
Consider returning -EINVAL or -ERANGE instead to better indicate an invalid offset.
Suggested change
| if (!sdp_offset_valid(sdp_pdev, offset)) | |
| return -ENOMEM; | |
| if (!sdp_offset_valid(sdp_pdev, offset)) | |
| return -EINVAL; |
|
|
||
| if (sdp_pdev->prime) { | ||
| reg_val = sdp_reg_read(sdp_pdev, SDP_EPFX_RINFO(0)); | ||
| sdp_reg_read(sdp_pdev, SDP_EPFX_RINFO(0), ®_val); |
There was a problem hiding this comment.
issue (bug_risk): Return code of sdp_reg_read is ignored in initialization
Please check the return value of sdp_reg_read and handle errors to prevent use of an uninitialized reg_val.
Comment on lines
+48
to
51
| if (!sdp_offset_valid(sdp_pdev, offset)) | ||
| return NULL; | ||
|
|
||
| return (uint64_t *)(sdp_pdev->mem[DAO_VFIO_DEV_BAR2].addr + offset); |
There was a problem hiding this comment.
suggestion (bug_risk): Potential unaligned pointer cast for register address
Enforce alignment by checking that offset is a multiple of sizeof(uint64_t), or use memcpy for unaligned access.
Suggested change
| if (!sdp_offset_valid(sdp_pdev, offset)) | |
| return NULL; | |
| return (uint64_t *)(sdp_pdev->mem[DAO_VFIO_DEV_BAR2].addr + offset); | |
| if (!sdp_offset_valid(sdp_pdev, offset)) | |
| return NULL; | |
| if (offset % sizeof(uint64_t) != 0) | |
| return NULL; | |
| return (uint64_t *)(sdp_pdev->mem[DAO_VFIO_DEV_BAR2].addr + offset); |
| #define SDP0_PCIE_DEV_NAME "0002:18:00.0" | ||
| #define SDP1_PCIE_DEV_NAME "0002:19:00.0" | ||
|
|
||
| static inline int sdp_offset_valid(struct dao_vfio_device *sdp_pdev, uint64_t offset) |
There was a problem hiding this comment.
issue (bug_risk): sdp_offset_valid dereferences sdp_pdev without verifying it's non-null
Please add a null check for sdp_pdev or document that it must not be null.
There was a problem hiding this comment.
Hey @anoobj - I've reviewed your changes - here's some feedback:
- Most call sites ignore the return value of sdp_reg_read, so please add error checks before using the output to avoid using uninitialized data on failure.
- The sdp_offset_valid helper only checks the start offset—consider validating offset + sizeof(uint64_t) against the BAR length to prevent partial out-of-bounds accesses.
- The new PF mailbox write uses
SDP_PF_MBOX_DATA(vfid * rpvf)—verify this offset computation against the hardware spec, since the original fixed 32-byte stride may indicate a constant step size.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The firmware previously mapped the maximum number of SDP queues to each pseudo device, which limited the number of queues available per SDP VF.
Firmware has been fixed to map only the required number of queues to each pseudo device, allowing SDP VFs to be configured with desired number of queues.
Patch also fixes miscellaneous address range checks in register read & write routines.
Fixes: e734ded ("lib/vfio: use pseudo RVU SDP devices")
Summary by Sourcery
Enable dynamic queue mapping for SDP VFs and strengthen register access routines with proper boundary validation, corrected bitfield extraction, and updated read/write interfaces
New Features:
Bug Fixes:
Enhancements: