Bug 1350391

I could never quite understand why qemu's "multifunction" option was exposed to libvirt config at all, since it must always be set for function 0 of a multifunction device, and never set anywhere else at any time (as far as I know), it would make sense to automatically set multifunction=on for function 0 anytime there were other functions used on the same slot. I *think* it also makes sense to forbid having a device attached to a non-0 function on a slot that doesn't have a multifunction=1 device in function 0. But before making a patch to do those things, I want a sanity check from Marcel that there is *absolutely no valid case* for any of the following:

1) setting multifunction=on for any non-0 function
2) leaving multifunction=off on function 0 when there are multiple active 
   functions in the slot..
3) leaving function 0 empty when there are other functions in use on the slot

(In reply to Laine Stump from comment #2)
> I could never quite understand why qemu's "multifunction" option was exposed
> to libvirt config at all, since it must always be set for function 0 of a
> multifunction device, and never set anywhere else at any time (as far as I
> know), it would make sense to automatically set multifunction=on for
> function 0 anytime there were other functions used on the same slot. I
> *think* it also makes sense to forbid having a device attached to a non-0
> function on a slot that doesn't have a multifunction=1 device in function 0.
> But before making a patch to do those things, I want a sanity check from
> Marcel that there is *absolutely no valid case* for any of the following:
> 

Hi Laine,

> 1) setting multifunction=on for any non-0 function

Indeed, is not needed, multifuncion flag is per device
PCI spec: 6.2.1
"Header Type: This byte identifies the layout of the second part of the     
              predefined header (beginning at byte 10h in Configuration Space)    
              and alsowhether or not the device contains multiple functions."

> 2) leaving multifunction=off on function 0 when there are multiple active 
>    functions in the slot..

Definitely not allowed. Multifunction is a *device* flag, however in QEMU
we don't have such a modelling. For QEMU function 0 represents the device.

> 3) leaving function 0 empty when there are other functions in use on the slot

Definitely not allowed. The same answer as for 2)

Thanks,
Marcel

*** Bug 1445634 has been marked as a duplicate of this bug. ***

This was fixed upstream in libvirt 3.0.0, so it is already in the RHEL7.4 libvirt build:

commit 8f4008713a49873b4bf354321f94b3fdeee14231
Author: Laine Stump <laine>
Date:   Tue Jan 10 00:20:11 2017 -0500

    qemu: use virDomainPCIAddressSetAllMulti() to set multi when needed
    
    If there are multiple devices assigned to the different functions of a
    single PCI slot, they will not work properly if the device at function
    0 doesn't have its "multi" attribute turned on, so it makes sense for
    libvirt to turn it on during PCI address assignment. Setting multi
    then assures that the new setting is stored in the config (so it will
    be used next time the domain is started), preventing any potential
    problems in the case that a future change in the configuration
    eliminates the devices on all non-0 functions (multi will still be set
    for function 0 even though it is the only function in use on the slot,
    which has no useful purpose, but also doesn't cause any problems).
    
    (NB: If we were to instead just decide on the setting for
    multifunction at runtime, a later removal of the non-0 functions of a
    slot would result in a silent change in the guest ABI for the
    remaining device on function 0 (although it may seem like an
    inconsequential guest ABI change, it *is* a guest ABI change to turn
    off the multi bit).)

still can reproduce the scenario 

version:
libvirt-3.2.0-7.el7.x86_64
qemu-kvm-rhev-2.9.0-7.el7.x86_64

steps:

1. start a vm with 2 virtio disks, function in pci address of vdb is non-zero. I did not enable multifunction on function 0 for slot '0x09' 

 <disk type='file' device='disk'>
      <driver name='qemu' type='qcow2'/>
      <source file='/var/lib/libvirt/images/RHEL-7.4-x86_64-latest.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw'/>
      <source file='/var/lib/libvirt/images/b.img'/>
      <target dev='vdb' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x2'/>
    </disk>

2.
[root@lmen1 ~]# virsh dumpxml aaa | grep 0x09
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x2'/>

3.check disks in guest, cannot find disk vdb
[root@lmen1 ~]# lsblk
NAME          MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
vda           252:0    0  10G  0 disk 
├─vda1        252:1    0   1G  0 part /boot
└─vda2        252:2    0   9G  0 part 
  ├─rhel-root 253:0    0   8G  0 lvm  /
  └─rhel-swap 253:1    0   1G  0 lvm  [SWAP]

Those two devices are on different slots.

As for function 0 of slot 9, there isn't any device using function 0 of slot 9.

The the multifunction attribute will be set automatically for the device on function 0 of slot "X" if another device is assigned to a non-0 function on that same slot "X". That isn't what your test does.

(try using slot='0x08' for both devices and see what happens to the address of the device on function 0)

(In reply to Laine Stump from comment #10)
> Those two devices are on different slots.
> 
> As for function 0 of slot 9, there isn't any device using function 0 of slot
> 9.
> 
> The the multifunction attribute will be set automatically for the device on
> function 0 of slot "X" if another device is assigned to a non-0 function on
> that same slot "X". That isn't what your test does.
> 
> (try using slot='0x08' for both devices and see what happens to the address
> of the device on function 0)

thanks for your reply!

1.the scenario I test in comment9 is the same as the bug description

In this scenario, there isn't any device using function 0 of slot 9,but there is one device using function 2 of slot 9,what is the expected result? is it expected that I can't find the device in the guest?



2.I tried the scenario you mentioned in comment10,yes,the result of this scenario is as you said.

1)start a guest with the xml:
<disk type='file' device='disk'>
      <driver name='qemu' type='qcow2'/>
      <source file='/var/lib/libvirt/images/RHEL-7.4-x86_64-latest.qcow2'/>
      <target dev='vda' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0'/>
    </disk>
    <disk type='file' device='disk'>
      <driver name='qemu' type='raw'/>
      <source file='/var/lib/libvirt/images/test1.img'/>
      <target dev='vdb' bus='virtio'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x2'/>
    </disk>

[root@lmen1 ~]# virsh destroy test;virsh start test
Domain test destroyed

Domain test started

2)check the multifunction='on'
[root@lmen1 ~]# virsh dumpxml test | grep 0x09
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x0' multifunction='on'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x09' function='0x2'/>

3)check the disk in the guest

# fdisk -l

Disk /dev/vda: 10.7 GB, 10737418240 bytes, 20971520 sectors
Units = sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes
Disk label type: dos
Disk identifier: 0x000ab07b

   Device Boot      Start         End      Blocks   Id  System
/dev/vda1   *        2048     2099199     1048576   83  Linux
/dev/vda2         2099200    20971519     9436160   8e  Linux LVM

Disk /dev/vdb: 1073 MB, 1073741824 bytes, 2097152 sectors
Units = sectors of 1 * 512 = 512 bytes
Sector size (logical/physical): 512 bytes / 512 bytes
I/O size (minimum/optimal): 512 bytes / 512 bytes

disk can be read/wrote

Okay, I see now that although the Summary of the BZ only mentioned that libvirt should check that multifunction='on' for function 0 when multiple functions of a slot are used, the description and ensuing comments also point out that it should be an error if a slot has nothing in function 0, but devices int any other non-0 functions (I even participated in that discussion, but had forgotten it and didnt read closely when I came back through the BZ several months later).

At this point in the release cycle, I don't think it's advisable to add a patch that results in classifying some configs as incorrect (it could end up causing a previously working config to fail). But since the patch that's already in does fix the problem given in the Summary, I think we should set this BZ to VERIFIED, and open a new BZ to request that any use of a non-0 function on a slot should be an error unless function 0 of that slot is also used.

(In reply to Laine Stump from comment #12)
> Okay, I see now that although the Summary of the BZ only mentioned that
> libvirt should check that multifunction='on' for function 0 when multiple
> functions of a slot are used, the description and ensuing comments also
> point out that it should be an error if a slot has nothing in function 0,
> but devices int any other non-0 functions (I even participated in that
> discussion, but had forgotten it and didnt read closely when I came back
> through the BZ several months later).
> 
> At this point in the release cycle, I don't think it's advisable to add a
> patch that results in classifying some configs as incorrect (it could end up
> causing a previously working config to fail). But since the patch that's
> already in does fix the problem given in the Summary, I think we should set
> this BZ to VERIFIED, and open a new BZ to request that any use of a non-0
> function on a slot should be an error unless function 0 of that slot is also
> used.

ok,mark this bug as verified

and file a new bug to track this issue:
Bug 1463892 - any use of a non-0 function on a slot should be an error unless function 0 of that slot is also used

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2017:1846

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2017:1846