10:05 p.m.
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