Laine Stump wrote:
> 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.
Yes, that's a bug, though bhyve didn't have a notion of disk addressing
within controller, e.g. until recently it had only:
-s $slot,virtio-blk,/my/image
Where $slot is:
slot pcislot[:function] bus:pcislot:function
So a similar scheme was used in the driver. Now bhyve supports a more
flexible disk management like this:
-s 1:0,ahci,hd:/images/disk.1,hd:/images/disk.2,\
hd:/images/disk.3,hd:/images/disk.4,\
hd:/images/disk.5,hd:/images/disk.6,\
hd:/images/disk.7,hd:/images/disk.8,\
cd:/images/install.iso \
So it's a good time to re-do that in the bhyve driver in a proper way.
> 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>
They look like controllers with disks inside the guest. E.g.:
<disk type='file' device='cdrom'>
<driver name='file' type='raw'/>
<source file='/home/novel/FreeBSD-11.0-CURRENT-amd64-20160217-r295683-disc1.iso'/>
<backingStore/>
<target dev='hdc' bus='sata'/>
<readonly/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
</disk>
Looks like this in the guest:
ahci0@pci0:0:3:0: class=0x010601 card=0x00000000 chip=0x28218086 rev=0x00 hdr=0x00
vendor = 'Intel Corporation'
device = '82801HR/HO/HH (ICH8R/DO/DH) 6 port SATA Controller [AHCI mode]'
class = mass storage
subclass = SATA
And the actual cdrom device there:
root@fbsdvm01:~ # dmesg | grep -E "(ahci0|cd0)"
ahci0: <Intel ICH8 AHCI SATA controller> mem 0xc0002000-0xc00023ff irq 17 at device 3.0 on pci0
ahci0: AHCI v1.30 with 6 6Gbps ports, Port Multiplier not supported
ahcich0: <AHCI channel> at channel 0 on ahci0
cd0 at ahcich0 bus 0 scbus0 target 0 lun 0
cd0: <BHYVE BHYVE DVD-ROM 001> Removable CD-ROM SCSI device
cd0: Serial Number BHYVE-13E5-4C93-29EB
cd0: 600.000MB/s transfers (SATA 3.x, UDMA6, ATAPI 12bytes, PIO 8192bytes)
cd0: 811MB (415600 2048 byte sectors)
root@fbsdvm01:~ #
>
> 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).
Yes, hopefully we'll do that before the October release.
In the meantime I'll submit a fix to get back to 'buggy-but-working' approach
for the next release.
> >> 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@xxxxxxxxxx
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> > Roman Bogorodskiy
Roman Bogorodskiy
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list