Copilot commented on code in PR #12981:
URL: https://github.com/apache/cloudstack/pull/12981#discussion_r3050223477
##########
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/LibvirtGpuDefTest.java:
##########
@@ -115,6 +115,110 @@ public void testGpuDef_withComplexPciAddress() {
assertTrue(gpuXml.contains("</hostdev>"));
}
+ @Test
+ public void testGpuDef_withFullPciAddressDomainZero() {
+ LibvirtGpuDef gpuDef = new LibvirtGpuDef();
+ VgpuTypesInfo pciGpuInfo = new VgpuTypesInfo(
+ GpuDevice.DeviceType.PCI,
+ "passthrough",
+ "passthrough",
+ "0000:00:02.0",
+ "10de",
+ "NVIDIA Corporation",
+ "1b38",
+ "Tesla T4"
+ );
+ gpuDef.defGpu(pciGpuInfo);
+
+ String gpuXml = gpuDef.toString();
+
+ assertTrue(gpuXml.contains("<address domain='0x0000' bus='0x00'
slot='0x02' function='0x0'/>"));
+ }
+
+ @Test
+ public void testGpuDef_withFullPciAddressNonZeroDomain() {
+ LibvirtGpuDef gpuDef = new LibvirtGpuDef();
+ VgpuTypesInfo pciGpuInfo = new VgpuTypesInfo(
+ GpuDevice.DeviceType.PCI,
+ "passthrough",
+ "passthrough",
+ "0001:65:00.0",
+ "10de",
+ "NVIDIA Corporation",
+ "1b38",
+ "Tesla T4"
+ );
+ gpuDef.defGpu(pciGpuInfo);
+
+ String gpuXml = gpuDef.toString();
+
+ assertTrue(gpuXml.contains("<address domain='0x0001' bus='0x65'
slot='0x00' function='0x0'/>"));
+ }
+
+ @Test
+ public void testGpuDef_withFullPciAddressWideDomain() {
+ LibvirtGpuDef gpuDef = new LibvirtGpuDef();
+ VgpuTypesInfo pciGpuInfo = new VgpuTypesInfo(
+ GpuDevice.DeviceType.PCI,
+ "passthrough",
+ "passthrough",
+ "10000:af:00.1",
+ "10de",
+ "NVIDIA Corporation",
+ "1b38",
+ "Tesla T4"
+ );
+ gpuDef.defGpu(pciGpuInfo);
+
+ String gpuXml = gpuDef.toString();
+
+ assertTrue(gpuXml.contains("<address domain='0x10000' bus='0xaf'
slot='0x00' function='0x1'/>"));
+ }
+
+ @Test
Review Comment:
The test case `testGpuDef_withFullPciAddressWideDomain` uses `10000:af:00.1`
as a PCI address. PCI domains (segments) are 16-bit and typically represented
as 4 hex digits (`0000`-`ffff`); a 5-hex-digit domain is not a realistic input
from `lspci`/sysfs and may mislead future readers about supported formats. If
the intent is to cover nvidia-smi’s 8-hex-digit domain, consider using an
address like `00000001:af:00.1` (and asserting the normalized output) or
dropping this test.
```suggestion
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtGpuDef.java:
##########
@@ -79,17 +79,27 @@ private void generatePciXml(StringBuilder gpuBuilder) {
gpuBuilder.append(" <driver name='vfio'/>\n");
gpuBuilder.append(" <source>\n");
- // Parse the bus address (e.g., 00:02.0) into domain, bus, slot,
function
+ // Parse the bus address into domain, bus, slot, function. Two input
formats are accepted:
+ // - "dddd:bb:ss.f" full PCI address with domain (e.g. 0000:00:02.0)
+ // - "bb:ss.f" legacy short BDF; domain defaults to 0000
String domain = "0x0000";
String bus = "0x00";
String slot = "0x00";
String function = "0x0";
if (busAddress != null && !busAddress.isEmpty()) {
String[] parts = busAddress.split(":");
- if (parts.length > 1) {
+ String slotFunction = null;
+ if (parts.length == 3) {
+ domain = "0x" + parts[0];
+ bus = "0x" + parts[1];
+ slotFunction = parts[2];
Review Comment:
`generatePciXml` now accepts `domain:bus:slot.func`, but it passes the raw
string segments directly into the libvirt XML without normalizing/padding. This
can yield non-canonical values like `domain='0x00000000'` (from nvidia-smi’s
8-digit domain) or unpadded/uppercase bus/slot, which is inconsistent with
other libvirt XML generation in this codebase (typically `%04x/%02x`). Consider
parsing the segments as hex integers and formatting them with fixed widths
(domain 4 hex digits, bus/slot 2, function 1) before appending to XML.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
