| Message ID | 8def783b-404c-3452-196d-3f3fd4d72c9e@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4ba50e7c423c29639878c00573288869aa627068 |
| Headers | show |
| Series | xen-pciback: a fix and a workaround | expand |
On 5/18/21 12:13 PM, Jan Beulich wrote: > > @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc > > /* > * Keep multi-function devices together on the virtual PCI bus, except > - * virtual functions. > + * that we want to keep virtual functions at func 0 on their own. They > + * aren't multi-function devices and hence their presence at func 0 > + * may cause guests to not scan the other functions. So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)? -boris
On 20.05.2021 02:36, Boris Ostrovsky wrote: > > On 5/18/21 12:13 PM, Jan Beulich wrote: >> >> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc >> >> /* >> * Keep multi-function devices together on the virtual PCI bus, except >> - * virtual functions. >> + * that we want to keep virtual functions at func 0 on their own. They >> + * aren't multi-function devices and hence their presence at func 0 >> + * may cause guests to not scan the other functions. > > > So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)? I'm not sure I understand the question: Whether to look at higher numbered slots is a function of slot 0's multi-function bit alone, aiui. IOW if slot 1 is being looked at in the first place, slots 2-7 should also be looked at. Jan
On 5/20/21 3:43 AM, Jan Beulich wrote: > On 20.05.2021 02:36, Boris Ostrovsky wrote: >> On 5/18/21 12:13 PM, Jan Beulich wrote: >>> >>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc >>> >>> /* >>> * Keep multi-function devices together on the virtual PCI bus, except >>> - * virtual functions. >>> + * that we want to keep virtual functions at func 0 on their own. They >>> + * aren't multi-function devices and hence their presence at func 0 >>> + * may cause guests to not scan the other functions. >> >> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)? > I'm not sure I understand the question: Whether to look at higher numbered > slots is a function of slot 0's multi-function bit alone, aiui. IOW if > slot 1 is being looked at in the first place, slots 2-7 should also be > looked at. Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered. My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered". -boris
On 20.05.2021 16:38, Boris Ostrovsky wrote: > > On 5/20/21 3:43 AM, Jan Beulich wrote: >> On 20.05.2021 02:36, Boris Ostrovsky wrote: >>> On 5/18/21 12:13 PM, Jan Beulich wrote: >>>> >>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc >>>> >>>> /* >>>> * Keep multi-function devices together on the virtual PCI bus, except >>>> - * virtual functions. >>>> + * that we want to keep virtual functions at func 0 on their own. They >>>> + * aren't multi-function devices and hence their presence at func 0 >>>> + * may cause guests to not scan the other functions. >>> >>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)? >> I'm not sure I understand the question: Whether to look at higher numbered >> slots is a function of slot 0's multi-function bit alone, aiui. IOW if >> slot 1 is being looked at in the first place, slots 2-7 should also be >> looked at. > > > Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered. > > > My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered". The common scanning logic is to look at slot 0 first. If that's populated, other slots get looked at only if slot 0 has the multi-function bit set. If slot 0 is not populated, nothing is known about the other slots, and hence they need to be scanned. Jan
On 20.05.2021 16:44, Jan Beulich wrote: > On 20.05.2021 16:38, Boris Ostrovsky wrote: >> >> On 5/20/21 3:43 AM, Jan Beulich wrote: >>> On 20.05.2021 02:36, Boris Ostrovsky wrote: >>>> On 5/18/21 12:13 PM, Jan Beulich wrote: >>>>> >>>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc >>>>> >>>>> /* >>>>> * Keep multi-function devices together on the virtual PCI bus, except >>>>> - * virtual functions. >>>>> + * that we want to keep virtual functions at func 0 on their own. They >>>>> + * aren't multi-function devices and hence their presence at func 0 >>>>> + * may cause guests to not scan the other functions. >>>> >>>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)? >>> I'm not sure I understand the question: Whether to look at higher numbered >>> slots is a function of slot 0's multi-function bit alone, aiui. IOW if >>> slot 1 is being looked at in the first place, slots 2-7 should also be >>> looked at. >> >> >> Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered. >> >> >> My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered". > > The common scanning logic is to look at slot 0 first. If that's populated, > other slots get looked at only if slot 0 has the multi-function bit set. > If slot 0 is not populated, nothing is known about the other slots, and > hence they need to be scanned. In particular Linux'es next_fn() ends with /* dev may be NULL for non-contiguous multifunction devices */ if (!dev || dev->multifunction) return (fn + 1) % 8; return 0; Jan
On 5/20/21 10:46 AM, Jan Beulich wrote: > On 20.05.2021 16:44, Jan Beulich wrote: >> On 20.05.2021 16:38, Boris Ostrovsky wrote: >>> On 5/20/21 3:43 AM, Jan Beulich wrote: >>>> On 20.05.2021 02:36, Boris Ostrovsky wrote: >>>>> On 5/18/21 12:13 PM, Jan Beulich wrote: >>>>>> >>>>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc >>>>>> >>>>>> /* >>>>>> * Keep multi-function devices together on the virtual PCI bus, except >>>>>> - * virtual functions. >>>>>> + * that we want to keep virtual functions at func 0 on their own. They >>>>>> + * aren't multi-function devices and hence their presence at func 0 >>>>>> + * may cause guests to not scan the other functions. >>>>> So your reading of the original commit is that whatever the issue it was, only function zero was causing the problem? In other words, you are not concerned that pci_scan_slot() may now look at function 1 and skip all higher-numbered function (assuming the problem is still there)? >>>> I'm not sure I understand the question: Whether to look at higher numbered >>>> slots is a function of slot 0's multi-function bit alone, aiui. IOW if >>>> slot 1 is being looked at in the first place, slots 2-7 should also be >>>> looked at. >>> >>> Wasn't the original patch describing a problem strictly as one for single-function devices, so the multi-function bit is not set? I.e. if all VFs (which are single-function devices) are placed in the same slot then pci_scan_slot() would only look at function 0 and ignore anything higher-numbered. >>> >>> >>> My question is whether it would "only look at function 0 and ignore anything higher-numbered" or "only look at the lowest-numbered function and ignore anything higher-numbered". >> The common scanning logic is to look at slot 0 first. If that's populated, >> other slots get looked at only if slot 0 has the multi-function bit set. >> If slot 0 is not populated, nothing is known about the other slots, and >> hence they need to be scanned. > In particular Linux'es next_fn() ends with > > /* dev may be NULL for non-contiguous multifunction devices */ > if (!dev || dev->multifunction) > return (fn + 1) % 8; > > return 0; Ah yes. Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
--- a/drivers/xen/xen-pciback/vpci.c +++ b/drivers/xen/xen-pciback/vpci.c @@ -70,7 +70,7 @@ static int __xen_pcibk_add_pci_dev(struc struct pci_dev *dev, int devid, publish_pci_dev_cb publish_cb) { - int err = 0, slot, func = -1; + int err = 0, slot, func = PCI_FUNC(dev->devfn); struct pci_dev_entry *t, *dev_entry; struct vpci_dev_data *vpci_dev = pdev->pci_dev_data; @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc /* * Keep multi-function devices together on the virtual PCI bus, except - * virtual functions. + * that we want to keep virtual functions at func 0 on their own. They + * aren't multi-function devices and hence their presence at func 0 + * may cause guests to not scan the other functions. */ - if (!dev->is_virtfn) { + if (!dev->is_virtfn || func) { for (slot = 0; slot < PCI_SLOT_MAX; slot++) { if (list_empty(&vpci_dev->dev_list[slot])) continue; t = list_entry(list_first(&vpci_dev->dev_list[slot]), struct pci_dev_entry, list); + if (t->dev->is_virtfn && !PCI_FUNC(t->dev->devfn)) + continue; if (match_slot(dev, t->dev)) { dev_info(&dev->dev, "vpci: assign to virtual slot %d func %d\n", - slot, PCI_FUNC(dev->devfn)); + slot, func); list_add_tail(&dev_entry->list, &vpci_dev->dev_list[slot]); - func = PCI_FUNC(dev->devfn); goto unlock; } } @@ -123,7 +126,6 @@ static int __xen_pcibk_add_pci_dev(struc slot); list_add_tail(&dev_entry->list, &vpci_dev->dev_list[slot]); - func = dev->is_virtfn ? 0 : PCI_FUNC(dev->devfn); goto unlock; } }
The commit referenced below was incomplete: It merely affected what would get written to the vdev-<N> xenstore node. The guest would still find the function at the original function number as long as __xen_pcibk_get_pci_dev() wouldn't be in sync. The same goes for AER wrt __xen_pcibk_get_pcifront_dev(). Undo overriding the function to zero and instead make sure that VFs at function zero remain alone in their slot. This has the added benefit of improving overall capacity, considering that there's only a total of 32 slots available right now (PCI segment and bus can both only ever be zero at present). Fixes: 8a5248fe10b1 ("xen PV passthru: assign SR-IOV virtual functions to separate virtual slots") Signed-off-by: Jan Beulich <jbeulich@suse.com> Cc: stable@vger.kernel.org --- Like the original change this has the effect of changing where devices would appear in the guest, when there are multiple of them. I don't see an immediate problem with this, but if there is we may need to reduce the effect of the change. Taking into account, besides the described breakage, how xen-pcifront's pcifront_scan_bus() works, I also wonder what problem it was in the first place that needed fixing. It may therefore also be worth to consider simply reverting the original change.