Re: [libvirt] [PATCH 4/6] bhyve: auto-assign addresses when <address type='pci'/> is specified

Laine Stump wrote: I just noticed that this change breaks address allocation for disks in some cases. E.g. for the following piece virDeviceInfoPCIAddressWanted() returns false because it expects info.type to be either VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE or VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, but we have VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE in this case. <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <readonly/> </disk> Therefore address is not allocated, it stays 0:0 that's reserved for the hostbridge and therefore VM fails to start. Adding <address type='pci'/> fixes that, but that's not very obvious thing for users. Generally, it makes sense, but not in the bhyve driver currently, because it's been using a scheme where each disk resides on its own controller, so we didn't have to bother with disk addressing. Not so long ago a possibility to have multiple disk on a single controller was introduced to bhyve: https://svnweb.freebsd.org/base?view=revision&revision=302459 (thanks to Fabian for the link!) and we'd need to implement proper disk addressing for it. However, for the upcoming release I'm wondering if we should go with a simple solution/workaround similar to this one: diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c index 8373a5f..f662012 100644 --- a/src/bhyve/bhyve_device.c +++ b/src/bhyve/bhyve_device.c @@ -107,7 +107,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, } for (i = 0; i < def->ndisks; i++) { - if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) + if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue; if (virDomainPCIAddressReserveNextSlot(addrs, &def->disks[i]->info, Thoughts? Roman Bogorodskiy