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

[Roman Bogorodskiy]

  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?

>          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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Roman Bogorodskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160827/8787ab62/attachment-0001.sig>