UCT/IB: add CoCo control object allocation#11586
Conversation
Add configure checks for the rdma-core and kernel APIs used by the CoCo control-object path. The checks cover CC_DMA_BOUNCE detection, parent-domain attributes, CQ parent-domain support, dma-heap headers and DEVX dmabuf UMEM registration so the new code is compiled only when the headers provide the required interfaces.
Detect devices that report IBV_DEVICE_CC_DMA_BOUNCE during MD open and allocate a CoCo parent domain for UCX-owned RDMA control objects. Keep the existing PD as the default path and expose helpers that select the control PD only when CoCo bounce DMA is active.
Create verbs QPs, CQs and SRQs through the CoCo control PD when the device requires bounce DMA. Use the extended CQ creation path with parent-domain attributes in CoCo mode and make the RC verbs capability probe follow the same allocation rules.
Pass CoCo allocation attributes to mlx5 parent domains and allocate DEVX UMEM-backed control buffers from system_cc_shared when CC_DMA_BOUNCE is reported. Route mlx5 tag-matching control QPs through the control PD and reject mlx5 transports or modes that are not made CoCo control-object safe in this milestone.
Keep DEVX doorbell record bookkeeping outside memory exposed to an untrusted CoCo device. Register only the doorbell record page with DEVX, while keeping UCX metadata in private memory.
|
🤖 Starting review — findings will be posted here when done. |
| if (status != UCS_OK) { | ||
| goto err_cleanup_device; | ||
| } | ||
| } |
There was a problem hiding this comment.
blocker: md->coco_pd is leaked on the uct_ib_md_probe_coco_control_cq failure path. uct_ib_md_create_coco_pd allocates md->coco_pd, but if the subsequent probe returns non-OK we jump to err_cleanup_device:, which does not call ibv_dealloc_pd(md->coco_pd). The MD destructor (uct_ib_md_free) only runs when the MD open succeeds, so the parent domain is leaked here. Add a dedicated error label that deallocates coco_pd before falling through to err_cleanup_device.
| return uct_ib_device_is_cc_dma_bounce(&md->dev); | ||
| } | ||
|
|
||
| static ucs_status_t uct_ib_md_create_coco_pd(uct_ib_md_t *md) |
There was a problem hiding this comment.
uct_ib_md_create_coco_pd / uct_ib_md_probe_coco_control_cq log ucs_error for every failure, including UCS_ERR_UNSUPPORTED. uct_ib_component_md_open iterates providers and treats UCS_ERR_UNSUPPORTED as "try next" — every device without CoCo support will print loud errors on a normal probe. Log at ucs_debug for the unsupported branches (or accept a silent flag), similar to uct_ib_mlx5_devx_check_event_channel.
| return UCS_ERR_UNSUPPORTED; | ||
| } | ||
|
|
||
| errno = saved_errno; |
There was a problem hiding this comment.
minor: saved_errno = errno; errno = saved_errno; immediately following each other is a no-op. Drop the redundant reassignment and keep only the one before uct_ib_check_memlock_limit_msg.
| UCS_LOG_LEVEL_ERROR; | ||
| const char *alloc_name = (name != NULL) ? name : "unknown"; | ||
| #if HAVE_LINUX_DMA_HEAP_H && HAVE_DECL_MLX5DV_DEVX_UMEM_REG_EX && \ | ||
| HAVE_DECL_MLX5DV_UMEM_MASK_DMABUF && HAVE_STRUCT_MLX5DV_DEVX_UMEM_IN |
There was a problem hiding this comment.
The dmabuf-backed path in uct_ib_mlx5_md_buf_alloc_shared skips the madvise(MADV_DONTFORK) that the regular path performs when md->super.fork_init is set. If a CoCo MD is opened with fork-init, the mmap'd dmabuf region is not marked DONTFORK. Is that intentional (dmabuf mappings handled differently on fork)? Worth a comment if so.
| mem->dmabuf_fd = UCT_IB_MLX5_INVALID_DMABUF_FD; | ||
| mem->is_dmabuf = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
minor: uct_ib_mlx5_devx_umem_reset(mem) is invoked at function entry and then again on every error return, including the first one before any mem field has been touched. Drop the duplicate resets and rely on the entry reset where nothing was set yet.
| ucs_free(page); | ||
| } | ||
|
|
||
| static ucs_mpool_ops_t uct_ib_mlx5_dbrec_ops = { |
There was a problem hiding this comment.
The new layout splits each dbrec chunk into (a) ucs_calloc'd chunk metadata (uct_ib_mlx5_dbrec_page_t + ucs_mpool_chunk_t + dbrec elements) and (b) a separate page-aligned umem-registered buffer of num_dbrecs * 2 * sizeof(uint32_t) for doorbells. This is unrelated to the CoCo feature (it is taken on every DEVX MD). Please move it to a dedicated refactor PR with a behavioural test (leak path, fork_init interaction, dbrec offset / mem_id consistency across put/get reuse) instead of the compile-time std::is_pointer assertion currently added in test/gtest/uct/ib/test_ib_md.cc.
There was a problem hiding this comment.
The dbrec refactor was kept in this PR because it is part of the CoCo control-object story: the NIC-visible doorbell record memory must be separated from UCX-private metadata before it can be safely backed by the shared/control allocation path. As a standalone non-CoCo refactor this change has no independent motivation, while splitting it out would make this PR incomplete.
The test concern was addressed: replaced the compile-time pointer check with behavioral coverage in test_devx.dbrec for db pointer/offset consistency, mem_id, metadata isolation, and put/get reuse.
| #endif | ||
|
|
||
| class test_ib_md : public test_md | ||
| { |
There was a problem hiding this comment.
This test only asserts that decltype(((uct_ib_mlx5_dbrec_t*)nullptr)->db) is a pointer — that's a compile-time tautology, not behavioural coverage. There is no runtime test for the new dbrec layout, the new shared dmabuf umem allocator, or the CoCo probe/PD lifecycle. Per REVIEW.md "Test Expectations", please add gtest coverage for the dbrec refactor (offsets, mem_id, reuse after put/get) and either CI or an explicit note about hardware coverage for the CoCo path.
|
|
||
| if (uct_ib_md_is_cc_dma_bounce(&md->super)) { | ||
| ucs_error("%s: %s is not CoCo control-object safe in this milestone", | ||
| uct_ib_device_name(&md->super.dev), "dc_mlx5"); |
There was a problem hiding this comment.
This "not CoCo control-object safe in this milestone" gate is duplicated verbatim (same error string and uct_ib_md_is_cc_dma_bounce(&md->super) check) in ud_mlx5.c and rc_mlx5_iface.c. Consider folding it into a single helper, e.g. uct_ib_md_check_cc_dma_bounce_supported(md, transport_name), so future milestones flip support in one place.
| [], [], [[#include <infiniband/mlx5dv.h>]]) | ||
| AC_CHECK_MEMBERS([struct mlx5dv_cq.cq_uar], | ||
| [], [], [[#include <infiniband/mlx5dv.h>]]) | ||
| AC_CHECK_DECLS([MLX5DV_OBJ_AH], [has_get_av=yes], |
There was a problem hiding this comment.
This PR adds several new configure decls (IBV_DEVICE_CC_DMA_BOUNCE, IBV_PARENT_DOMAIN_INIT_ATTR_ALLOW_CC_UNPROTECTED_ALLOC, IBV_CQ_INIT_ATTR_MASK_PD, ibv_alloc_parent_domain, linux/dma-heap.h, mlx5dv_devx_umem_reg_ex, mlx5dv_devx_umem_in, ibv_cq_init_attr_ex.parent_domain) but no CI job exercises either path. Per REVIEW.md "Configure/build options", please add a job in buildlib/tools/builds.sh / contrib/test_jenkins.sh that at least exercises the disabled-path fallbacks, or note in the PR description which existing job covers it.
There was a problem hiding this comment.
The PR was validated in a non-CoCo environment with configure/build and targeted gtests.
Current/default CI can exercise compilation/configure fallback paths for these declarations, but it cannot fully validate the live CoCo path because that requires a CoCo VM plus a CC-DMA-bounce-capable device. So full runtime coverage is blocked by environment/hardware availability, not by the test logic itself.
|
The PR mixes a non-CoCo refactor (dbrec layout: inline |
Fix CoCo MD error cleanup, quiet expected unsupported probes, share the cc-dma-bounce transport gate, and add behavioral DEVX dbrec coverage. Also apply fork handling to the dmabuf-backed shared control-object mapping path.
72f60d3 to
5888481
Compare
2 participants
What?
Why?
UCX must avoid exposing private guest control metadata to an untrusted device in CoCo RDMA mode. This PR adds the base detection and control-object allocation support required before enabling the DEVX hardening pieces.
How?