|WARNING| [RFC] bus/vmbus: make driver interface internal

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/170863

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-08

# DPDK Patch Review

## Summary
This patch makes the vmbus bus API internal by renaming headers and marking all functions with `__rte_internal`. The changes are largely mechanical and follow the deprecation plan, but there are several issues that need to be addressed.

---

## Errors

### 1. Inconsistent symbol export annotation placement
**File: `drivers/bus/vmbus/linux/vmbus_bus.c`, `vmbus_channel.c`**

The `RTE_EXPORT_INTERNAL_SYMBOL()` macro is placed **before** the function definition, but per DPDK convention, symbol export macros must be placed on the line **immediately before** the function definition in the `.c` file, matching the pattern shown in the API guidelines.

Current pattern:
```c
RTE_EXPORT_INTERNAL_SYMBOL(rte_vmbus_map_device)
int
rte_vmbus_map_device(struct rte_vmbus_device *dev)
```

This appears correct based on the function format rules (return type on its own line), but verify this matches the actual `RTE_EXPORT_INTERNAL_SYMBOL` macro definition. If the macro expects to be immediately adjacent to the return type line, the placement is wrong.

**Suggested fix:** Verify the macro definition and ensure the placement matches DPDK conventions. If uncertain, follow the pattern used in existing internal APIs elsewhere in DPDK.

---

### 2. Missing export annotation added to undocumented function
**File: `drivers/bus/vmbus/vmbus_channel.c:462`**

The function `rte_vmbus_chan_dump()` gains its first export annotation:
```c
+RTE_EXPORT_INTERNAL_SYMBOL(rte_vmbus_chan_dump)
void rte_vmbus_chan_dump(FILE *f, const struct vmbus_channel *chan)
```

The commit message states: *"rte_vmbus_chan_dump() had no export annotation at all, so it was declared in the public header but never present in the shared library. It is now exported as internal like the rest."*

**Problem:** This is a **new symbol export** being added. Adding a symbol to the version map is an ABI change. The function was not previously exported from the shared library, so applications could not have called it. Making it available now (even as internal) is technically an ABI addition.

For an internal-only API change, this is likely acceptable, but it should be called out in the release notes under "ABI Changes" or explained why this doesn't constitute an ABI break.

**Suggested fix:** Add a note in the release notes explicitly mentioning that `rte_vmbus_chan_dump()` is now exported as an internal symbol (was declared but not previously exported).

---

## Warnings

### 1. Release notes entry could be clearer about ABI compatibility
**File: `doc/guides/rel_notes/release_26_11.rst:238`**

The release notes state:
```rst
* bus/vmbus: Made the vmbus bus API internal.
  The ``rte_bus_vmbus.h`` and ``rte_vmbus_reg.h`` headers are no longer installed;
  their contents moved to the driver only headers
  ``vmbus_api.h`` and ``vmbus_reg.h``.
  All ``rte_vmbus_*`` functions are now internal to DPDK.
```

**Issue:** This doesn't explicitly state whether this is an ABI break for applications (it is--applications can no longer link against these symbols). While the deprecation notice warned about this, the release notes should be clearer.

**Suggested fix:** Add a sentence clarifying the impact:
```rst
Applications that previously used these functions will fail to link;
these APIs were only usable by DPDK internal code and the netvsc PMD.
```

---

### 2. No verification that netvsc builds successfully
**Commit message:**
> I don't have access to Hyper-V or Azure to test this.
> Was generated by LLM based on the intention in the deprecation note.
> Please test on proper environment before merging.

While honest, this means the patch is **untested** on its target platform. The claim that "netvsc needs no change" is based on static analysis, not actual compilation.

**Suggested action:** Before merging, someone with a Hyper-V/Azure environment must verify that:
- The code compiles without errors
- netvsc can still access all the renamed headers through `bus_vmbus_driver.h`
- Functional tests pass (basic device probe, channel open/close, data transfer)

---

### 3. Header include path dependency
**File: `drivers/bus/vmbus/bus_vmbus_driver.h:9`**

The change from `#include <rte_bus_vmbus.h>` to `#include <vmbus_api.h>` relies on the build system adding `drivers/bus/vmbus/` to the include path for consumers (netvsc).

**Verification needed:** Confirm that the meson build dependency mechanism automatically adds the bus driver directory to the include path for dependent drivers. This should work via the `deps` variable in `drivers/net/netvsc/meson.build`, but it's worth explicitly verifying.

**Suggested fix:** Add a comment in the commit message or release notes that drivers depending on vmbus will automatically have the correct include paths via the meson driver dependency mechanism.

---

### 4. Deprecated API removal timing
**File: `doc/guides/rel_notes/deprecation.rst`**

The deprecation notice is removed:
```diff
-* bus/vmbus: Starting DPDK 25.11, all the vmbus API defined in
-  ``drivers/bus/vmbus/rte_bus_vmbus.h`` will become internal to DPDK.
-  Those API functions are used internally by DPDK core and netvsc PMD.
```

The patch targets release **26.11** (per the release notes file name), which is **two major releases** after the 25.11 deprecation notice. This is the correct timing per DPDK policy (deprecation notice - wait one release - remove in next).

However, the release notes say "Starting DPDK 25.11" which could be interpreted as "will happen in 25.11" rather than "announced for removal after 25.11."

**Clarification:** This is not an error, but the wording in the original deprecation notice was ambiguous. The current patch timing is correct (25.11 announcement - 26.03 skip - 26.11 removal).

---

## Info

### 1. Header guard naming convention
**File: `drivers/bus/vmbus/vmbus_api.h`**

The header guard changes from `_VMBUS_H_` to `VMBUS_API_H`, dropping the leading underscore and trailing underscore.

**Rationale:** Identifiers starting with underscore are reserved in C. The new name is better, but the old name (`_VMBUS_H_`) violates the standard. This is a **good cleanup**.

---

### 2. Consistent symbol export pattern
All functions in the bus are systematically converted from `RTE_EXPORT_SYMBOL()` to `RTE_EXPORT_INTERNAL_SYMBOL()`, and all declarations gain `__rte_internal`. This is the correct pattern for making a public API internal.

---

### 3. Driver SDK header list is clean
**File: `drivers/bus/vmbus/meson.build:9`**

The `headers` variable is removed entirely, and both `vmbus_api.h` and `vmbus_reg.h` are moved to `driver_sdk_headers`. This correctly prevents their installation for applications while keeping them available for drivers.

```python
driver_sdk_headers = files(
        'bus_vmbus_driver.h',
        'vmbus_api.h',
        'vmbus_reg.h',
)
```

This is the correct pattern for driver-only headers.

---

## Final Recommendations

**Before merging:**
1. **Test compilation** of netvsc driver with these changes on actual Hyper-V/Azure environment
2. **Clarify** the `rte_vmbus_chan_dump()` symbol addition in release notes (was never exported before)
3. **Verify** symbol export macro placement matches DPDK conventions (return type on line before function body is unusual)
4. **Add** explicit statement in release notes about ABI break impact (applications cannot link to these symbols anymore)

**The patch is mostly correct** and follows the deprecation plan. The primary risk is that it's **untested** on the target platform. The symbol export syntax should be double-checked against other internal API conversions in DPDK.