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

On 08/27/2016 11:42 AM, Roman Bogorodskiy wrote: > Laine Stump wrote: > >> Rather than only assigning a PCI address when no address is given at >> all, also do it when the config says that the address type is 'pci', >> but it gives no address. >> --- >> src/bhyve/bhyve_device.c | 10 ++++------ >> 1 file changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/src/bhyve/bhyve_device.c b/src/bhyve/bhyve_device.c >> index 3eb2956..8373a5f 100644 >> --- a/src/bhyve/bhyve_device.c >> +++ b/src/bhyve/bhyve_device.c >> @@ -98,7 +98,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, >> goto error; >> >> for (i = 0; i < def->nnets; i++) { >> - if (def->nets[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) >> + if (!virDeviceInfoPCIAddressWanted(&def->nets[i]->info)) >> continue; >> if (virDomainPCIAddressReserveNextSlot(addrs, >> &def->nets[i]->info, >> @@ -107,8 +107,7 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, >> } >> >> for (i = 0; i < def->ndisks; i++) { >> - if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && >> - def->disks[i]->info.addr.pci.slot != 0) >> + if (!virDeviceInfoPCIAddressWanted(&def->disks[i]->info)) >> continue; > 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? Because bus='sata', the address type is set to "drive" during the device post-parse for disk devices. And when the address type is 'drive', it's a bug to assign a PCI address to it. Using the shortcut of describing a drive plus a controller in a single device by misusing the address type may have seemed expedient, but it's incorrect and needs to be fixed. And the longer you wait to fix it, the worse the consequences will be.

On the other hand, it's been broken (but working) for a long time already, whereas it's been *non-working* for a shorter time, so it makes sense to temporarily restore the broken-but-working behavior for this release, then start working on a permanent solution. Your proposed change is technically not correct, though.You really should only be calling virPCIDeviceAddressIsEmpty() if the address type='pci'. If I understand correctly, you currently give *all* disk devices a PCI address, so what you really want is this: if(def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && !virPCIDeviceAddressIsEmpty(&def->disks[i]->info.addr.pci)) continue; This way you will always assign a PCI address to every disk, unless it already has an address assigned. I'm okay with ACKing this as a temporary fix until you can fix it correctly (anyway, really the final word is up to you, since you're the bhyve maintainer :-) So, beyond the temporary fix, how do these disks appear inside the guest? Are they connected to a SATA controller and using the guest OS' sata driver? Or are they connected directly to the PCI bus and using some other driver (similar to virtio-blk-pci)? The XML should reflect what the emulated hardware really looks like, whereas right now what you end up with after the addressing is done, is this: <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <address type='pci' domain='0x0000' bus='0x00' slot='0xNN' function='0x0'/> <readonly/> </disk>

I think the first thing you need to start doing (after this release) is to separate that into: <controller type='sata' index='${I}'> <address type='pci' domain='0x0000' bus='0x00' slot='0xNN' function='0x0'/> </controller> <disk type='file' device='cdrom'> <driver name='file' type='raw'/> <source file='/home/test/foobar.iso'/> <target dev='hdc' bus='sata'/> <address type='drive' controller='${I}' bus='0' target='0' unit='0'/> <readonly/> </disk> Even if the version of bhyve only supports a single disk per sata controller, you should still do this. Later, when support is added for multiple disks on the same SATA controller, you can allow unit to increment for each disk on a particular controller (0-5). Follow the rules in the VIR_DOMAIN_DISK_BUS_SATA case of virDomainDiskDefAssignAddress - target and bus are always 0, controller is the same as the "index" attribute of the desired controller, and unit is 0-5 (6 disks per controller).

>> if (virDomainPCIAddressReserveNextSlot(addrs, >> &def->disks[i]->info, >> @@ -118,9 +117,8 @@ bhyveAssignDevicePCISlots(virDomainDefPtr def, >> >> for (i = 0; i < def->ncontrollers; i++) { >> if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { >> - if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) >> - continue; >> - if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) >> + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT || >> + !virDeviceInfoPCIAddressWanted(&def->controllers[i]->info)) >> continue; >> >> if (virDomainPCIAddressReserveNextSlot(addrs, >> -- >> 2.5.5 >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list > Roman Bogorodskiy