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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



  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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]