Various of SR-IOV fixes and cleanup
cover Various of SR-IOV fixes and cleanup
This patch set is meant to address several issues found in recent patches currently in the pci/virtualization branch.
The first two patches address possible issues in initialization. Specifically the fact that offset and stride cannot be read while numvfs is 0, and that the BARs need to be reset if totalVFs has increased because the BIOS likely allocated the given resources under the assumption of a smaller number of VFs being available.
The last 3 address a number of issues in sriov_enable. They started as just a patch to address the offset and stride issues, however there were some other issues found related to pcibios_enable_sriov that I decided to address as well, and then there was some minor cleanup in addition to all of that.
---
Alexander Duyck (5):
iov: Update virtfn_max_buses to validate offset and stride
iov: Reset resources to 0 if totalVFs increases after enabling ARI
iov: Fix sriov_enable exception handling path
iov: Variable and loop cleanup for sriov_disable and sriov_enable
iov: Update sriov_enable to correctly handle offset and stride
drivers/pci/iov.c | 123 ++++++++++++++++++++++++++++------------------------- 1 file changed, 65 insertions(+), 58 deletions(-)
--
PATCH 1/5 iov: Update virtfn_max_buses to validate offset and stride
This patch pulls the validation of offset and stride into virtfn_max_buses. The general idea is to validate offset and stride for each possible value of numvfs in addition to still determining the maximum bus value for the VFs.
I also reversed the loop as the most likely maximum will be when numvfs is set to total_VFs. In addition this makes it so that we loop down to a value of 0 for numvfs which should be the resting state for the register.
--- drivers/pci/iov.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-)
drivers/pci/iov.c +24 -18
@@ -54,24 +54,33 @@ static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn)* The PF consumes one bus number. NumVFs, First VF Offset, and VF Stride * determine how many additional bus numbers will be consumed by VFs. * -* Iterate over all valid NumVFs and calculate the maximum number of bus -* numbers that could ever be required. +* Iterate over all valid NumVFs, validate offset and stride, and calculate +* the maximum number of bus numbers that could ever be required. */ -static inline u8 virtfn_max_buses (struct pci_dev *dev) +static int virtfn_max_buses (struct pci_dev *dev) {struct pci_sriov *iov = dev->sriov; -int nr_virtfn; -u8 max =0 ; +int nr_virtfn = iov->total_VFs;int busnr; -for (nr_virtfn =1 ; nr_virtfn <= iov->total_VFs; nr_virtfn++) { -pci_iov_set_numvfs (dev, nr_virtfn); +pci_iov_set_numvfs (dev, nr_virtfn); + +while (nr_virtfn--) { +if (!iov->offset || !iov->stride) + goto err; + busnr =pci_iov_virtfn_bus (dev, nr_virtfn -1 ); -if (busnr > max) - max = busnr; +if (busnr > iov->max_VF_buses) + iov->max_VF_buses = busnr; + +pci_iov_set_numvfs (dev, nr_virtfn); } -return max; +return 0 ; +err: +pci_write_config_word (dev, iov->pos +PCI_SRIOV_NUM_VF ,0 ); + +return -EIO ; }static struct pci_bus *virtfn_add_bus (struct pci_bus *bus,int busnr)
@@ -467,22 +476,19 @@ found: dev->sriov = iov; dev->is_physfn =1 ; - iov->max_VF_buses =virtfn_max_buses (dev); -pci_iov_set_numvfs (dev,0 ); -if (!iov->offset || (total >1 && !iov->stride)) { - rc = -EIO ; - goto failed; - } -return 0 ; + rc =virtfn_max_buses (dev); +if (!rc) +return 0 ; + dev->sriov =NULL ; + dev->is_physfn =0 ; failed:for (i =0 ; i <PCI_SRIOV_NUM_BARS ; i++) { res = &dev->resource[i +PCI_IOV_RESOURCES ]; res->flags =0 ; } - dev->sriov =NULL ;kfree (iov);return rc; }
Re: [PATCH 1/5] iov: Update virtfn_max_buses to validate offset and stride
Hi Alex,
Thanks a lot for cleaning this up. I think this is a great improvement over what I did.
Quoted 11 lines · Alexander Duyck
This patch pulls the validation of offset and stride into
virtfn_max_buses. The general idea is to validate offset and stride for each possible value of numvfs in addition to still determining the maximum bus value for the VFs.I also reversed the loop as the most likely maximum will be when numvfs is set to
total_VFs. In addition this makes it so that we loop down to a value of 0 for numvfs which should be the resting state for the register.Fixes 8e20e89658f2 ("PCI: Set SR-IOV NumVFs to zero after enumeration")Signed-off-by Alexander Duyck <[email protected]>
I'd like to squash this together with my patch instead of having fixes on top of fixes. What do you think of the following? (This applies on top of 70675e0b6a1a ("PCI: Don't try to restore VF BARs")).
commit c20e11b572c5d4e4f01c86580a133122fbd13cfa
Author: Alexander Duyck <[email protected]> Date: Wed Oct 28 10:54:32 2015 -0500
PCI: Set SR-IOV NumVFs to zero after enumeration
The enumeration path should leave NumVFs set to zero. But after 4449f079722c ("PCI: Calculate maximum number of buses required for VFs"), we call virtfn_max_buses() in the enumeration path, which changes NumVFs. This NumVFs change is visible via lspci and sysfs until a driver enables SR-IOV.
Iterate from TotalVFs down to zero so NumVFs is zero when we're finished computing the maximum number of buses. Validate offset and stride in the loop, so we can test it at every possible NumVFs setting. Rename virtfn_max_buses() to compute_max_vf_buses() to hint that it does have a side effect of updating iov->max_VF_buses.
drivers/pci/iov.c +26 -19
@@ -54,24 +54,33 @@ static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn)* The PF consumes one bus number. NumVFs, First VF Offset, and VF Stride * determine how many additional bus numbers will be consumed by VFs. * -* Iterate over all valid NumVFs and calculate the maximum number of bus -* numbers that could ever be required. +* Iterate over all valid NumVFs, validate offset and stride, and calculate +* the maximum number of bus numbers that could ever be required. */ -static inline u8 virtfn_max_buses (struct pci_dev *dev) +static int compute_max_vf_buses (struct pci_dev *dev) {struct pci_sriov *iov = dev->sriov; -int nr_virtfn; -u8 max =0 ; +int nr_virtfn = iov->total_VFs;int busnr; -for (nr_virtfn =1 ; nr_virtfn <= iov->total_VFs; nr_virtfn++) { -pci_iov_set_numvfs (dev, nr_virtfn); +pci_iov_set_numvfs (dev, nr_virtfn); + +while (nr_virtfn--) { +if (!iov->offset || !iov->stride) + goto err; + busnr =pci_iov_virtfn_bus (dev, nr_virtfn -1 ); -if (busnr > max) - max = busnr; +if (busnr > iov->max_VF_buses) + iov->max_VF_buses = busnr; + +pci_iov_set_numvfs (dev, nr_virtfn); } -return max; +return 0 ; + +err: +pci_iov_set_numvfs (dev,0 ); +return -EIO ; }static struct pci_bus *virtfn_add_bus (struct pci_bus *bus,int busnr)
@@ -384,7 +393,7 @@ static int sriov_init(struct pci_dev *dev, int pos)int rc;int nres;u32 pgsz; -u16 ctrl, total, offset, stride; +u16 ctrl, total;struct pci_sriov *iov;struct resource *res;struct pci_dev *pdev;
@@ -414,11 +423,6 @@ static int sriov_init(struct pci_dev *dev, int pos) found:pci_write_config_word (dev, pos +PCI_SRIOV_CTRL , ctrl); -pci_write_config_word (dev, pos +PCI_SRIOV_NUM_VF ,0 ); -pci_read_config_word (dev, pos +PCI_SRIOV_VF_OFFSET , &offset); -pci_read_config_word (dev, pos +PCI_SRIOV_VF_STRIDE , &stride); -if (!offset || (total >1 && !stride)) -return -EIO ;pci_read_config_dword (dev, pos +PCI_SRIOV_SUP_PGSIZE , &pgsz); i =PAGE_SHIFT >12 ?PAGE_SHIFT -12 :0 ;
@@ -456,8 +460,6 @@ found: iov->nres = nres; iov->ctrl = ctrl; iov->total_VFs = total; - iov->offset = offset; - iov->stride = stride; iov->pgsz = pgsz; iov->self = dev;pci_read_config_dword (dev, pos +PCI_SRIOV_CAP , &iov->cap);
@@ -474,10 +476,15 @@ found: dev->sriov = iov; dev->is_physfn =1 ; - iov->max_VF_buses =virtfn_max_buses (dev); + rc =compute_max_vf_buses (dev); +if (rc) + goto fail_max_buses;return 0 ; +fail_max_buses: + dev->sriov =NULL ; + dev->is_physfn =0 ; failed:for (i =0 ; i <PCI_SRIOV_NUM_BARS ; i++) { res = &dev->resource[i +PCI_IOV_RESOURCES ];
Alexander Duyck
Acked-byRe: [PATCH 1/5] iov: Update virtfn_max_buses to validate offset and stride
Quoted 41 lines · Bjorn Helgaas
Hi Alex,
Thanks a lot for cleaning this up. I think this is a great improvement over what I did.
Quoted 11 lines · Alexander Duyck
This patch pulls the validation of offset and stride into
virtfn_max_buses. The general idea is to validate offset and stride for each possible value of numvfs in addition to still determining the maximum bus value for the VFs.I also reversed the loop as the most likely maximum will be when numvfs is set to
total_VFs. In addition this makes it so that we loop down to a value of 0 for numvfs which should be the resting state for the register.Fixes 8e20e89658f2 ("PCI: Set SR-IOV NumVFs to zero after enumeration")Signed-off-by Alexander Duyck <[email protected]>I'd like to squash this together with my patch instead of having fixes on top of fixes. What do you think of the following? (This applies on top of
70675e0b6a1a("PCI: Don't try to restore VF BARs")).commit
c20e11b572c5d4e4f01c86580a133122fbd13cfaAuthor: Alexander Duyck <[email protected]> Date: Wed Oct 28 10:54:32 2015 -0500PCI: Set SR-IOV NumVFs to zero after enumeration
The enumeration path should leave NumVFs set to zero. But after
4449f079722c("PCI: Calculate maximum number of buses required for VFs"), we callvirtfn_max_buses()in the enumeration path, which changes NumVFs. This NumVFs change is visible via lspci and sysfs until a driver enables SR-IOV.Iterate from TotalVFs down to zero so NumVFs is zero when we're finished computing the maximum number of buses. Validate offset and stride in the loop, so we can test it at every possible NumVFs setting. Rename
virtfn_max_buses()tocompute_max_vf_buses()to hint that it does have a side effect of updatingiov->max_VF_buses.
Quoted 6 lines
[bhelgaas: changelog, rename, reverse sense of error path]Fixes 4449f079722c ("PCI: Calculate maximum number of buses required for VFs")Based-on-patch-by Ethan Zhao <[email protected]>Signed-off-by Alexander Duyck <[email protected]>Signed-off-by Bjorn Helgaas <[email protected]>
This looks fine to me.
Re: [PATCH 1/5] iov: Update virtfn_max_buses to validate offset and stride
In reply to Bjorn HelgaasQuoted 66 lines · Bjorn Helgaas
Hi Alex,
Thanks a lot for cleaning this up. I think this is a great improvement over what I did.
Quoted 11 lines · Alexander Duyck
This patch pulls the validation of offset and stride into
virtfn_max_buses. The general idea is to validate offset and stride for each possible value of numvfs in addition to still determining the maximum bus value for the VFs.I also reversed the loop as the most likely maximum will be when numvfs is set to
total_VFs. In addition this makes it so that we loop down to a value of 0 for numvfs which should be the resting state for the register.Fixes 8e20e89658f2 ("PCI: Set SR-IOV NumVFs to zero after enumeration")Signed-off-by Alexander Duyck <[email protected]>I'd like to squash this together with my patch instead of having fixes on top of fixes. What do you think of the following? (This applies on top of
70675e0b6a1a("PCI: Don't try to restore VF BARs")).commit
c20e11b572c5d4e4f01c86580a133122fbd13cfaAuthor: Alexander Duyck <[email protected]> Date: Wed Oct 28 10:54:32 2015 -0500PCI: Set SR-IOV NumVFs to zero after enumeration
The enumeration path should leave NumVFs set to zero. But after
4449f079722c("PCI: Calculate maximum number of buses required for VFs"), we callvirtfn_max_buses()in the enumeration path, which changes NumVFs. This NumVFs change is visible via lspci and sysfs until a driver enables SR-IOV.Iterate from TotalVFs down to zero so NumVFs is zero when we're finished computing the maximum number of buses. Validate offset and stride in the loop, so we can test it at every possible NumVFs setting. Rename
virtfn_max_buses()tocompute_max_vf_buses()to hint that it does have a side effect of updatingiov->max_VF_buses.[bhelgaas: changelog, rename, reverse sense of error path]Fixes 4449f079722c ("PCI: Calculate maximum number of buses required for VFs")Based-on-patch-by Ethan Zhao <[email protected]>Signed-off-by Alexander Duyck <[email protected]>Signed-off-by Bjorn Helgaas <[email protected]>drivers/pci/iov.c +3
-5@@ -54,24 +54,33 @@ static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn)* The PF consumes one bus number. NumVFs, First VF Offset, and VF Stride * determine how many additional bus numbers will be consumed by VFs. * -* Iterate over all valid NumVFs and calculate the maximum number of bus -* numbers that could ever be required. +* Iterate over all valid NumVFs, validate offset and stride, and calculate +* the maximum number of bus numbers that could ever be required. */ -static inline u8 virtfn_max_buses (struct pci_dev *dev) +static int compute_max_vf_buses (struct pci_dev *dev) {struct pci_sriov *iov = dev->sriov; -int nr_virtfn; -u8 max =0 ;
drivers/pci/iov.c static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn) · line 70
Quoted 10 lines
drivers/pci/iov.c +6
-2+int nr_virtfn = iov->total_VFs;int busnr; -for (nr_virtfn =1 ; nr_virtfn <= iov->total_VFs; nr_virtfn++) { -pci_iov_set_numvfs (dev, nr_virtfn); +pci_iov_set_numvfs (dev, nr_virtfn); + +while (nr_virtfn--) { +if (!iov->offset || !iov->stride) + goto err;
I think we have a minor problem here. In sriov_enable(), we return an error if "nr_virtfn > 1 && !iov->stride", so it's legal for stride to be zero if NumVF is 1. Here we don't allow that. Sec 3.3.10 says:
Note: VF Stride is unused if NumVFs is 0 or 1. If NumVFs is greater than 1, VF Stride must not be zero."
So I think we should allow "stride == 0" here when NumVFs is 1.
drivers/pci/iov.c static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn) · line 72
Quoted 2 lines
drivers/pci/iov.c +1
-0+ busnr =pci_iov_virtfn_bus (dev, nr_virtfn -1 );
I think this loop management is slightly wrong: I don't think we ever compute busnr for the highest VF because we always decrement nr_virtfn after calling pci_iov_set_numvfs(), and then we subtract one again. E.g., if Total VFs is 8, the VFs are numbered VF0..VF7, and we have this, which doesn't check VF7:
nr_virtfn = iov->total_VFs # nr_virtfn == 8
pci_iov_set_numvfs(..., nr_virtfn) # passes 8 (correct)
while (nr_virtfn--) {
# nr_virtfn == 7 in loop body
pci_iov_virtfn_bus(..., nr_virtfn - 1) # passes 6 (wrong)Quoted 5 lines
drivers/pci/iov.c +3
-2-if (busnr > max) - max = busnr; +if (busnr > iov->max_VF_buses) + iov->max_VF_buses = busnr; +
drivers/pci/iov.c static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn) · line 84
Quoted 10 lines
drivers/pci/iov.c +6
-1+pci_iov_set_numvfs (dev, nr_virtfn); } -return max; +return 0 ; + +err: +pci_iov_set_numvfs (dev,0 ); +return -EIO ; }
Here's my new proposal:
static int compute_max_vf_buses(struct pci_dev *dev)
{
struct pci_sriov *iov = dev->sriov;
int nr_virtfn, busnr, rc = 0; for (nr_virtfn = iov->total_VFs; nr_virtfn; nr_virtfn--) {
pci_iov_set_numvfs(dev, nr_virtfn);
if (!iov->offset || (nr_virtfn > 1 && !iov->stride)) {
rc = -EIO;
goto out;
}
busnr = pci_iov_virtfn_bus(dev, nr_virtfn - 1);
if (busnr > iov->max_VF_buses)
iov->max_VF_buses = busnr;
} out:
pci_iov_set_numvfs(dev, 0);
return rc;
}Re: [PATCH 1/5] iov: Update virtfn_max_buses to validate offset and stride
Quoted 82 lines · Bjorn Helgaas
Quoted 66 lines · Bjorn Helgaas
Hi Alex,
Thanks a lot for cleaning this up. I think this is a great improvement over what I did.
Quoted 11 lines · Alexander Duyck
This patch pulls the validation of offset and stride into
virtfn_max_buses. The general idea is to validate offset and stride for each possible value of numvfs in addition to still determining the maximum bus value for the VFs.I also reversed the loop as the most likely maximum will be when numvfs is set to
total_VFs. In addition this makes it so that we loop down to a value of 0 for numvfs which should be the resting state for the register.Fixes 8e20e89658f2 ("PCI: Set SR-IOV NumVFs to zero after enumeration")Signed-off-by Alexander Duyck <[email protected]>I'd like to squash this together with my patch instead of having fixes on top of fixes. What do you think of the following? (This applies on top of
70675e0b6a1a("PCI: Don't try to restore VF BARs")).commit
c20e11b572c5d4e4f01c86580a133122fbd13cfaAuthor: Alexander Duyck <[email protected]> Date: Wed Oct 28 10:54:32 2015 -0500PCI: Set SR-IOV NumVFs to zero after enumeration
The enumeration path should leave NumVFs set to zero. But after
4449f079722c("PCI: Calculate maximum number of buses required for VFs"), we callvirtfn_max_buses()in the enumeration path, which changes NumVFs. This NumVFs change is visible via lspci and sysfs until a driver enables SR-IOV.Iterate from TotalVFs down to zero so NumVFs is zero when we're finished computing the maximum number of buses. Validate offset and stride in the loop, so we can test it at every possible NumVFs setting. Rename
virtfn_max_buses()tocompute_max_vf_buses()to hint that it does have a side effect of updatingiov->max_VF_buses.[bhelgaas: changelog, rename, reverse sense of error path]Fixes 4449f079722c ("PCI: Calculate maximum number of buses required for VFs")Based-on-patch-by Ethan Zhao <[email protected]>Signed-off-by Alexander Duyck <[email protected]>Signed-off-by Bjorn Helgaas <[email protected]>drivers/pci/iov.c +3
-5@@ -54,24 +54,33 @@ static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn)* The PF consumes one bus number. NumVFs, First VF Offset, and VF Stride * determine how many additional bus numbers will be consumed by VFs. * -* Iterate over all valid NumVFs and calculate the maximum number of bus -* numbers that could ever be required. +* Iterate over all valid NumVFs, validate offset and stride, and calculate +* the maximum number of bus numbers that could ever be required. */ -static inline u8 virtfn_max_buses (struct pci_dev *dev) +static int compute_max_vf_buses (struct pci_dev *dev) {struct pci_sriov *iov = dev->sriov; -int nr_virtfn; -u8 max =0 ;drivers/pci/iov.c static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn) · line 70
Quoted 10 lines
drivers/pci/iov.c +6
-2+int nr_virtfn = iov->total_VFs;int busnr; -for (nr_virtfn =1 ; nr_virtfn <= iov->total_VFs; nr_virtfn++) { -pci_iov_set_numvfs (dev, nr_virtfn); +pci_iov_set_numvfs (dev, nr_virtfn); + +while (nr_virtfn--) { +if (!iov->offset || !iov->stride) + goto err;I think we have a minor problem here. In
sriov_enable(), we return an error if "nr_virtfn> 1 && !iov->stride", so it's legal for stride to be zero if NumVF is 1. Here we don't allow that. Sec 3.3.10 says:
Quoted 4 lines
Note: VF Stride is unused if NumVFs is 0 or 1. If NumVFs is greater than 1, VF Stride must not be zero."
So I think we should allow "stride == 0" here when NumVFs is 1.
Right, we shouldn't be testing it if NumVFs is 1 or less.
Quoted 9 lines
drivers/pci/iov.c static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn) · line 72
Quoted 2 lines
drivers/pci/iov.c +1
-0+ busnr =pci_iov_virtfn_bus (dev, nr_virtfn -1 );I think this loop management is slightly wrong: I don't think we ever compute busnr for the highest VF because we always decrement
nr_virtfnafter callingpci_iov_set_numvfs(), and then we subtract one again. E.g., if Total VFs is 8, the VFs are numbered VF0..VF7, and we have this, which doesn't check VF7:
Quoted 6 lines
nr_virtfn = iov->total_VFs # nr_virtfn == 8
pci_iov_set_numvfs(..., nr_virtfn) # passes 8 (correct)
while (nr_virtfn--) {
# nr_virtfn == 7 in loop body
pci_iov_virtfn_bus(..., nr_virtfn - 1) # passes 6 (wrong)Yeah, that was supposed to just be nr_virtfn.
Quoted 35 lines
Quoted 5 lines
drivers/pci/iov.c +3
-2-if (busnr > max) - max = busnr; +if (busnr > iov->max_VF_buses) + iov->max_VF_buses = busnr; +drivers/pci/iov.c static inline void pci_iov_set_numvfs(struct pci_dev *dev, int nr_virtfn) · line 84
Quoted 10 lines
drivers/pci/iov.c +6
-1+pci_iov_set_numvfs (dev, nr_virtfn); } -return max; +return 0 ; + +err: +pci_iov_set_numvfs (dev,0 ); +return -EIO ; }Here's my new proposal:
static int compute_max_vf_buses(struct pci_dev *dev) { struct pci_sriov *iov = dev->sriov; int nr_virtfn, busnr, rc = 0;for (nr_virtfn = iov->total_VFs; nr_virtfn; nr_virtfn--) { pci_iov_set_numvfs(dev, nr_virtfn); if (!iov->offset || (nr_virtfn > 1 && !iov->stride)) { rc = -EIO; goto out; } busnr = pci_iov_virtfn_bus(dev, nr_virtfn - 1); if (busnr > iov->max_VF_buses) iov->max_VF_buses = busnr; }
Quoted 5 lines
out:
pci_iov_set_numvfs(dev, 0);
return rc;
}This looks good to me. In theory you could save yourself a pair of MMIO reads at the end of the loop by just writing numvfs without the offset and stride read, but this should work.
- Alex
Re: [PATCH 1/5] iov: Update virtfn_max_buses to validate offset and stride
Quoted 28 lines · Alexander Duyck
Quoted 21 lines · Bjorn Helgaas
On Wed, Oct 28, 2015 at 11:32:16AM -0500, Bjorn Helgaas wrote:
Here's my new proposal:
static int compute_max_vf_buses(struct pci_dev *dev) { struct pci_sriov *iov = dev->sriov; int nr_virtfn, busnr, rc = 0;for (nr_virtfn = iov->total_VFs; nr_virtfn; nr_virtfn--) { pci_iov_set_numvfs(dev, nr_virtfn); if (!iov->offset || (nr_virtfn > 1 && !iov->stride)) { rc = -EIO; goto out; } busnr = pci_iov_virtfn_bus(dev, nr_virtfn - 1); if (busnr > iov->max_VF_buses) iov->max_VF_buses = busnr; }Quoted 5 lines
out: pci_iov_set_numvfs(dev, 0); return rc; }
Quoted 3 lines
This looks good to me. In theory you could save yourself a pair of MMIO reads at the end of the loop by just writing numvfs without the offset and stride read, but this should work.
True. It's called once per device at boot-time, so it's not a hot path, and I think it's worth two unnecessary MMIO reads to make it easier to
analyze: there's exactly one place that updates PCI_SRIOV_NUM_VF, and that place always updates our caches of PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE.
Bjorn
PATCH 2/5 iov: Reset resources to 0 if totalVFs increases after enabling ARI
In reply to Alexander DuyckThis patch forces us to reallocate VF BARs if the totalVFs value has increased after enabling ARI. This normally shouldn't occur, however I have seen some non-spec devices that shift between 7 and some value greater than 7 based on the ARI value and we want to avoid triggering any issues with such devices.
--- drivers/pci/iov.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/pci/iov.c +10 -1
@@ -393,7 +393,7 @@ static int sriov_init(struct pci_dev *dev, int pos)int rc;int nres;u32 pgsz; -u16 ctrl, total; +u16 ctrl, total, orig_total;struct pci_sriov *iov;struct resource *res;struct pci_dev *pdev;
@@ -402,6 +402,7 @@ static int sriov_init(struct pci_dev *dev, int pos) pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT)return -ENODEV ; +pci_read_config_word (dev, pos +PCI_SRIOV_TOTAL_VF , &orig_total);pci_read_config_word (dev, pos +PCI_SRIOV_CTRL , &ctrl);if (ctrl &PCI_SRIOV_CTRL_VFE ) {pci_write_config_word (dev, pos +PCI_SRIOV_CTRL ,0 );
@@ -450,6 +451,14 @@ found: } iov->barsz[i] =resource_size (res); res->end = res->start +resource_size (res) * total -1 ; + +/* force reallocation of BARs if total VFs increased */ +if (orig_total < total) { + res->flags |=IORESOURCE_UNSET ; + res->end -= res->start; + res->start =0 ; + } +dev_info (&dev->dev,"VF(n) BAR%d space: %pR (contains BAR%d for %d VFs)\n" , i, res, i, total); i += bar64;
Re: [PATCH 2/5] iov: Reset resources to 0 if totalVFs increases after enabling ARI
Hi Alex,
Quoted 5 lines · Alexander Duyck
This patch forces us to reallocate VF BARs if the totalVFs value has increased after enabling ARI. This normally shouldn't occur, however I have seen some non-spec devices that shift between 7 and some value greater than 7 based on the ARI value and we want to avoid triggering any issues with such devices.
Can you include specifics about the devices? The value "7" is pretty specific, so if we're going to include that level of detail, we should have the actual device info to go with it.
I guess the problem is:
- Device supports 7 TotalVFs with ARI disabled, >7 with ARI enabled
- Firmware leaves ARI disabled in SRIOV_CTRL
- Firmware computes size based on 7 VFs
- Firmware allocates space and programs BARs for 7 VFs
- Linux enables ARI, reads >7 TotalVFs
- Linux computes size based on >7 VFs
- Increased size may overlap other resources
Right?
Quoted 28 lines
Fixes 3aa71da412fe ("PCI: Enable SR-IOV ARI Capable Hierarchy before reading TotalVFs")Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/pci/iov.c +2
-1@@ -393,7 +393,7 @@ static int sriov_init(struct pci_dev *dev, int pos)int rc;int nres;u32 pgsz; -u16 ctrl, total; +u16 ctrl, total, orig_total;struct pci_sriov *iov;struct resource *res;struct pci_dev *pdev;@@ -402,6 +402,7 @@ static int sriov_init(struct pci_dev *dev, int pos) pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT)return -ENODEV ; +pci_read_config_word (dev, pos +PCI_SRIOV_TOTAL_VF , &orig_total);pci_read_config_word (dev, pos +PCI_SRIOV_CTRL , &ctrl);if (ctrl &PCI_SRIOV_CTRL_VFE ) {pci_write_config_word (dev, pos +PCI_SRIOV_CTRL ,0 );
drivers/pci/iov.c found: · line 460
Quoted 10 lines
drivers/pci/iov.c +7
-0@@ -450,6 +451,14 @@ found: } iov->barsz[i] =resource_size (res); res->end = res->start +resource_size (res) * total -1 ; + +/* force reallocation of BARs if total VFs increased */ +if (orig_total < total) { + res->flags |=IORESOURCE_UNSET ; + res->end -= res->start; + res->start =0 ; + }
Two thoughts here:
1) Even if the required space increased, it's possible that firmware placed the BAR somewhere where the extra space is available. In that case, this forces reallocation unnecessarily.
2) This *feels* like something the PCI core should be doing anyway, even without any help here. Shouldn't we fail in pci_claim_resource() and set IORESOURCE_UNSET there?
OK, and a third: re-reading TotalVF is (I think) completely unnecessary per spec, so if we're going to do it we should probably have a one-line comment about why the code is doing something that appears unnecessary, and really should have a concrete example device in the changelog.
Quoted 10 lines
drivers/pci/iov.c +1
-1+dev_info (&dev->dev,"VF(n) BAR%d space: %pR (contains BAR%d for %d VFs)\n" , i, res, i, total); i += bar64; --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/5] iov: Reset resources to 0 if totalVFs increases after enabling ARI
Quoted 9 lines · Bjorn Helgaas
Hi Alex,
Quoted 5 lines · Alexander Duyck
This patch forces us to reallocate VF BARs if the totalVFs value has increased after enabling ARI. This normally shouldn't occur, however I have seen some non-spec devices that shift between 7 and some value greater than 7 based on the ARI value and we want to avoid triggering any issues with such devices.
Quoted 3 lines
Can you include specifics about the devices? The value "7" is pretty specific, so if we're going to include that level of detail, we should have the actual device info to go with it.
I referenced 7 as that is the largest number of VFs a single function can support assuming a single function without ARI and without the ability to handle Type 1 configuration requests. The Intel fm10k driver has logic in it that does a check for ARI and if it is supported it reports via sysfs a totalVFs of 64, otherwise it limits the totalVFs reported to 7. However, I don't believe it exposes the limitation via the configuration space.
Quoted 11 lines
I guess the problem is:
- Device supports 7 TotalVFs with ARI disabled, >7 with ARI enabled
- Firmware leaves ARI disabled inSRIOV_CTRL
- Firmware computes size based on 7 VFs
- Firmware allocates space and programs BARs for 7 VFs
- Linux enables ARI, reads >7 TotalVFs
- Linux computes size based on >7 VFs
- Increased size may overlap other resourcesRight?
Right. More than likely what will happen is that you will see overlap of the device on itself if it has multiple base address registers assigned to the VFs.
Quoted 41 lines
Quoted 28 lines
Fixes 3aa71da412fe ("PCI: Enable SR-IOV ARI Capable Hierarchy before reading TotalVFs")Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/pci/iov.c +2
-1@@ -393,7 +393,7 @@ static int sriov_init(struct pci_dev *dev, int pos)int rc;int nres;u32 pgsz; -u16 ctrl, total; +u16 ctrl, total, orig_total;struct pci_sriov *iov;struct resource *res;struct pci_dev *pdev;@@ -402,6 +402,7 @@ static int sriov_init(struct pci_dev *dev, int pos) pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT)return -ENODEV ; +pci_read_config_word (dev, pos +PCI_SRIOV_TOTAL_VF , &orig_total);pci_read_config_word (dev, pos +PCI_SRIOV_CTRL , &ctrl);if (ctrl &PCI_SRIOV_CTRL_VFE ) {pci_write_config_word (dev, pos +PCI_SRIOV_CTRL ,0 );drivers/pci/iov.c found: · line 460
Quoted 10 lines
drivers/pci/iov.c +7
-0@@ -450,6 +451,14 @@ found: } iov->barsz[i] =resource_size (res); res->end = res->start +resource_size (res) * total -1 ; + +/* force reallocation of BARs if total VFs increased */ +if (orig_total < total) { + res->flags |=IORESOURCE_UNSET ; + res->end -= res->start; + res->start =0 ; + }Two thoughts here:
Quoted 3 lines
1) Even if the required space increased, it's possible that firmware placed the BAR somewhere where the extra space is available. In that case, this forces reallocation unnecessarily.
I'd say it is possible, but not likely. From past experience I have seen BIOSes do some very dumb things when it comes to SR-IOV, assuming they even support it.
In addition many of the VF devices out there support more than one base address register per function. The Intel NICs for example have one for device registers and one for MSI-X registers. And most BIOSes usually pack one right after the other from what I have seen. So while there may be more space there what usually happens is that the MSI-X region will have to be relocated in order to make room for expanding the other base address register.
My last bit on all this is that VFs are meant to be assigned into guests. I would argue that for the sake of security we are much better off invalidating the VF base address registers and forcing a reallocation if there is even a risk of the VF base address register space overlapping with some other piece of host memory. We don't want to risk possibly exposing any bits of the host that we didn't intend on.
Quoted 9 lines
2) This *feels* like something the PCI core should be doing anyway, even without any help here. Shouldn't we fail in
pci_claim_resource()and setIORESOURCE_UNSETthere?OK, and a third: re-reading TotalVF is (I think) completely unnecessary per spec, so if we're going to do it we should probably have a one-line comment about why the code is doing something that appears unnecessary, and really should have a concrete example device in the changelog.
I'm perfectly fine with us adding a comment about this being needed for out-of-spec devices on BIOSes that may not fully support SR-IOV. However, I wasn't the one that had the device that needed the workaround for totalVFs and so I cannot provide a concrete example myself.
I've added Ben to the Cc as he was the author for the original patch that introduced this possible issue. Perhaps he can provide us with more details on the hardware that needs this.
- Alex
Re: [PATCH 2/5] iov: Reset resources to 0 if totalVFs increases after enabling ARI
Quoted 14 lines · Alexander Duyck
Quoted 9 lines · Bjorn Helgaas
Hi Alex,
Quoted 5 lines · Alexander Duyck
This patch forces us to reallocate VF BARs if the totalVFs value has increased after enabling ARI. This normally shouldn't occur, however I have seen some non-spec devices that shift between 7 and some value greater than 7 based on the ARI value and we want to avoid triggering any issues with such devices.
Quoted 3 lines
Can you include specifics about the devices? The value "7" is pretty specific, so if we're going to include that level of detail, we should have the actual device info to go with it.
Quoted 7 lines
I referenced 7 as that is the largest number of VFs a single function can support assuming a single function without ARI and without the ability to handle Type 1 configuration requests. The Intel fm10k driver has logic in it that does a check for ARI and if it is supported it reports via sysfs a totalVFs of 64, otherwise it limits the totalVFs reported to 7. However, I don't believe it exposes the limitation via the configuration space.
Ah, OK, that makes sense.
Quoted 73 lines
Quoted 11 lines
I guess the problem is:
- Device supports 7 TotalVFs with ARI disabled, >7 with ARI enabled
- Firmware leaves ARI disabled inSRIOV_CTRL
- Firmware computes size based on 7 VFs
- Firmware allocates space and programs BARs for 7 VFs
- Linux enables ARI, reads >7 TotalVFs
- Linux computes size based on >7 VFs
- Increased size may overlap other resourcesRight?
Right. More than likely what will happen is that you will see overlap of the device on itself if it has multiple base address registers assigned to the VFs.
Quoted 41 lines
Quoted 28 lines
Fixes 3aa71da412fe ("PCI: Enable SR-IOV ARI Capable Hierarchy before reading TotalVFs")Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/pci/iov.c +2
-1@@ -393,7 +393,7 @@ static int sriov_init(struct pci_dev *dev, int pos)int rc;int nres;u32 pgsz; -u16 ctrl, total; +u16 ctrl, total, orig_total;struct pci_sriov *iov;struct resource *res;struct pci_dev *pdev;@@ -402,6 +402,7 @@ static int sriov_init(struct pci_dev *dev, int pos) pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT)return -ENODEV ; +pci_read_config_word (dev, pos +PCI_SRIOV_TOTAL_VF , &orig_total);pci_read_config_word (dev, pos +PCI_SRIOV_CTRL , &ctrl);if (ctrl &PCI_SRIOV_CTRL_VFE ) {pci_write_config_word (dev, pos +PCI_SRIOV_CTRL ,0 );drivers/pci/iov.c found: · line 460
Quoted 10 lines
drivers/pci/iov.c +7
-0@@ -450,6 +451,14 @@ found: } iov->barsz[i] =resource_size (res); res->end = res->start +resource_size (res) * total -1 ; + +/* force reallocation of BARs if total VFs increased */ +if (orig_total < total) { + res->flags |=IORESOURCE_UNSET ; + res->end -= res->start; + res->start =0 ; + }Two thoughts here:
Quoted 3 lines
1) Even if the required space increased, it's possible that firmware placed the BAR somewhere where the extra space is available. In that case, this forces reallocation unnecessarily.
I'd say it is possible, but not likely. From past experience I have seen BIOSes do some very dumb things when it comes to SR-IOV, assuming they even support it.
In addition many of the VF devices out there support more than one base address register per function. The Intel NICs for example have one for device registers and one for MSI-X registers. And most BIOSes usually pack one right after the other from what I have seen. So while there may be more space there what usually happens is that the MSI-X region will have to be relocated in order to make room for expanding the other base address register.
Quoted 7 lines
My last bit on all this is that VFs are meant to be assigned into guests. I would argue that for the sake of security we are much better off invalidating the VF base address registers and forcing a reallocation if there is even a risk of the VF base address register space overlapping with some other piece of host memory. We don't want to risk possibly exposing any bits of the host that we didn't intend on.
Agreed, not likely for several reasons.
Quoted 3 lines
Quoted 3 lines
2) This *feels* like something the PCI core should be doing anyway, even without any help here. Shouldn't we fail in
pci_claim_resource()and setIORESOURCE_UNSETthere?
This is really the core of my question -- what problem does this patch solve? I'm trying to figure out if delaying the read of TotalVFs until after we set ARI Capable Hierarchy is sufficient, and if it's not sufficient, *why* not?
Quoted 21 lines
Quoted 5 lines
OK, and a third: re-reading TotalVF is (I think) completely unnecessary per spec, so if we're going to do it we should probably have a one-line comment about why the code is doing something that appears unnecessary, and really should have a concrete example device in the changelog.
I'm perfectly fine with us adding a comment about this being needed for out-of-spec devices on BIOSes that may not fully support SR-IOV. However, I wasn't the one that had the device that needed the workaround for totalVFs and so I cannot provide a concrete example myself.
I've added Ben to the Cc as he was the author for the original patch that introduced this possible issue. Perhaps he can provide us with more details on the hardware that needs this.
- Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] iov: Reset resources to 0 if totalVFs increases after enabling ARI
Quoted 108 lines · Bjorn Helgaas
Quoted 14 lines · Alexander Duyck
Quoted 9 lines · Bjorn Helgaas
Hi Alex,
Quoted 5 lines · Alexander Duyck
This patch forces us to reallocate VF BARs if the totalVFs value has increased after enabling ARI. This normally shouldn't occur, however I have seen some non-spec devices that shift between 7 and some value greater than 7 based on the ARI value and we want to avoid triggering any issues with such devices.
Quoted 3 lines
Can you include specifics about the devices? The value "7" is pretty specific, so if we're going to include that level of detail, we should have the actual device info to go with it.
Quoted 7 lines
I referenced 7 as that is the largest number of VFs a single function can support assuming a single function without ARI and without the ability to handle Type 1 configuration requests. The Intel fm10k driver has logic in it that does a check for ARI and if it is supported it reports via sysfs a totalVFs of 64, otherwise it limits the totalVFs reported to 7. However, I don't believe it exposes the limitation via the configuration space.
Ah, OK, that makes sense.
Quoted 73 lines
Quoted 11 lines
I guess the problem is:
- Device supports 7 TotalVFs with ARI disabled, >7 with ARI enabled
- Firmware leaves ARI disabled inSRIOV_CTRL
- Firmware computes size based on 7 VFs
- Firmware allocates space and programs BARs for 7 VFs
- Linux enables ARI, reads >7 TotalVFs
- Linux computes size based on >7 VFs
- Increased size may overlap other resourcesRight?
Right. More than likely what will happen is that you will see overlap of the device on itself if it has multiple base address registers assigned to the VFs.
Quoted 41 lines
Quoted 28 lines
Fixes 3aa71da412fe ("PCI: Enable SR-IOV ARI Capable Hierarchy before reading TotalVFs")Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/pci/iov.c +2
-1@@ -393,7 +393,7 @@ static int sriov_init(struct pci_dev *dev, int pos)int rc;int nres;u32 pgsz; -u16 ctrl, total; +u16 ctrl, total, orig_total;struct pci_sriov *iov;struct resource *res;struct pci_dev *pdev;@@ -402,6 +402,7 @@ static int sriov_init(struct pci_dev *dev, int pos) pci_pcie_type(dev) != PCI_EXP_TYPE_ENDPOINT)return -ENODEV ; +pci_read_config_word (dev, pos +PCI_SRIOV_TOTAL_VF , &orig_total);pci_read_config_word (dev, pos +PCI_SRIOV_CTRL , &ctrl);if (ctrl &PCI_SRIOV_CTRL_VFE ) {pci_write_config_word (dev, pos +PCI_SRIOV_CTRL ,0 );drivers/pci/iov.c found: · line 460
Quoted 10 lines
drivers/pci/iov.c +7
-0@@ -450,6 +451,14 @@ found: } iov->barsz[i] =resource_size (res); res->end = res->start +resource_size (res) * total -1 ; + +/* force reallocation of BARs if total VFs increased */ +if (orig_total < total) { + res->flags |=IORESOURCE_UNSET ; + res->end -= res->start; + res->start =0 ; + }Two thoughts here:
Quoted 3 lines
1) Even if the required space increased, it's possible that firmware placed the BAR somewhere where the extra space is available. In that case, this forces reallocation unnecessarily.
I'd say it is possible, but not likely. From past experience I have seen BIOSes do some very dumb things when it comes to SR-IOV, assuming they even support it.
In addition many of the VF devices out there support more than one base address register per function. The Intel NICs for example have one for device registers and one for MSI-X registers. And most BIOSes usually pack one right after the other from what I have seen. So while there may be more space there what usually happens is that the MSI-X region will have to be relocated in order to make room for expanding the other base address register.
Quoted 7 lines
My last bit on all this is that VFs are meant to be assigned into guests. I would argue that for the sake of security we are much better off invalidating the VF base address registers and forcing a reallocation if there is even a risk of the VF base address register space overlapping with some other piece of host memory. We don't want to risk possibly exposing any bits of the host that we didn't intend on.
Agreed, not likely for several reasons.
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
2) This *feels* like something the PCI core should be doing anyway, even without any help here. Shouldn't we fail in
pci_claim_resource()and setIORESOURCE_UNSETthere?This is really the core of my question -- what problem does this patch solve? I'm trying to figure out if delaying the read of TotalVFs until after we set ARI Capable Hierarchy is sufficient, and if it's not sufficient, *why* not?
I suppose you have a point. As long as the PCI core is taking care of any overlaps in pci_claim_resource that is probably good enough.
- Alex
PATCH 3/5 iov: Fix sriov_enable exception handling path
In reply to Alexander DuyckQuoted 1 line
From what I can tell there were several errors in the
sriov_enable
exception handling path. Below is a brief list of what I believe I am fixing:
1. If pcibios_enable_sriov failed, we returned without disabling SR-IOV on the device.
2. If virtfn_add failed we didn't call pcibios_disable_sriov to undo pcibios_enable_sriov.
3. We were resetting numvfs to 0 before a second had passed for the VFs to quiesce.
4. Minor coding style issues for white space and for assignment in conditional check.
Beyond addressing these 4 issues there were also 2 other minor issues in that retval was a redundant variable with rc, and j wasn't actually needed as we could simply reverse the loop we were running when setting up i. As such I have updated the code to address those two items.
--- drivers/pci/iov.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
drivers/pci/iov.c +17 -14
@@ -231,13 +231,18 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)int __weak pcibios_sriov_enable (struct pci_dev *pdev,u16 num_vfs) { -return 0 ; +return 0 ; +} + +int __weak pcibios_sriov_disable (struct pci_dev *pdev) +{ +return 0 ; }static int sriov_enable (struct pci_dev *dev,int nr_virtfn) {int rc; -int i, j; +int i;int nres;u16 offset, stride, initial;struct resource *res;
@@ -245,7 +250,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)struct pci_sriov *iov = dev->sriov;int bars =0 ;int bus; -int retval;if (!nr_virtfn)return 0 ;
@@ -322,10 +326,11 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (nr_virtfn < initial) initial = nr_virtfn; -if ((retval =pcibios_sriov_enable (dev, initial))) { + rc =pcibios_sriov_enable (dev, initial); +if (rc) {dev_err (&dev->dev,"failure %d from pcibios_sriov_enable()\n" , - retval); -return retval; + rc); + goto err_pcibios; }for (i =0 ; i < initial; i++) {
@@ -340,25 +345,23 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)return 0 ; failed: -for (j =0 ; j < i; j++) -virtfn_remove (dev, j,0 ); +while (i--) +virtfn_remove (dev, i,0 ); +pcibios_sriov_disable (dev); +err_pcibios: iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE |PCI_SRIOV_CTRL_MSE );pci_cfg_access_lock (dev);pci_write_config_word (dev, iov->pos +PCI_SRIOV_CTRL , iov->ctrl); -pci_iov_set_numvfs (dev,0 );ssleep (1 );pci_cfg_access_unlock (dev);if (iov->link != dev->devfn)sysfs_remove_link (&dev->dev.kobj,"dep_link" ); -return rc; -} +pci_iov_set_numvfs (dev,0 ); -int __weak pcibios_sriov_disable (struct pci_dev *pdev) -{ -return 0 ; +return rc; }static void sriov_disable (struct pci_dev *dev)
Re: [PATCH 3/5] iov: Fix sriov_enable exception handling path
Hi Alex,
Thanks, this definitely clears up some problems. Two minor questions below.
Quoted 85 lines · Alexander Duyck
Quoted 1 line
From what I can tell there were several errors in the
sriov_enableexception handling path. Below is a brief list of what I believe I am fixing:
1. If
pcibios_enable_sriovfailed, we returned without disabling SR-IOV on the device.
2. Ifvirtfn_addfailed we didn't callpcibios_disable_sriovto undopcibios_enable_sriov.
3. We were resetting numvfs to 0 before a second had passed for the VFs to quiesce.
4. Minor coding style issues for white space and for assignment in conditional check.Beyond addressing these 4 issues there were also 2 other minor issues in that retval was a redundant variable with rc, and j wasn't actually needed as we could simply reverse the loop we were running when setting up i. As such I have updated the code to address those two items.
Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)drivers/pci/iov.c +15
-8@@ -231,13 +231,18 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)int __weak pcibios_sriov_enable (struct pci_dev *pdev,u16 num_vfs) { -return 0 ; +return 0 ; +} + +int __weak pcibios_sriov_disable (struct pci_dev *pdev) +{ +return 0 ; }static int sriov_enable (struct pci_dev *dev,int nr_virtfn) {int rc; -int i, j; +int i;int nres;u16 offset, stride, initial;struct resource *res;@@ -245,7 +250,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)struct pci_sriov *iov = dev->sriov;int bars =0 ;int bus; -int retval;if (!nr_virtfn)return 0 ;@@ -322,10 +326,11 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (nr_virtfn < initial) initial = nr_virtfn; -if ((retval =pcibios_sriov_enable (dev, initial))) { + rc =pcibios_sriov_enable (dev, initial); +if (rc) {dev_err (&dev->dev,"failure %d from pcibios_sriov_enable()\n" , - retval); -return retval; + rc); + goto err_pcibios; }for (i =0 ; i < initial; i++) {@@ -340,25 +345,23 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)return 0 ; failed: -for (j =0 ; j < i; j++) -virtfn_remove (dev, j,0 ); +while (i--) +virtfn_remove (dev, i,0 ); +pcibios_sriov_disable (dev); +err_pcibios: iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE |PCI_SRIOV_CTRL_MSE );pci_cfg_access_lock (dev);pci_write_config_word (dev, iov->pos +PCI_SRIOV_CTRL , iov->ctrl);
drivers/pci/iov.c static int sriov_enable(struct pci_dev *dev, int nr_virtfn) · line 362
Quoted 10 lines
drivers/pci/iov.c +1
-3-pci_iov_set_numvfs (dev,0 );ssleep (1 );pci_cfg_access_unlock (dev);if (iov->link != dev->devfn)sysfs_remove_link (&dev->dev.kobj,"dep_link" ); -return rc; -} +pci_iov_set_numvfs (dev,0 );
Do you have a spec pointer for the 1 sec delay before clearing NumVFs?
Does we need to clear NumVFs while holding the cfg access lock?
Quoted 13 lines
drivers/pci/iov.c +1
-4-int __weak pcibios_sriov_disable (struct pci_dev *pdev) -{ -return 0 ; +return rc; }static void sriov_disable (struct pci_dev *dev) --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] iov: Fix sriov_enable exception handling path
Quoted 95 lines · Bjorn Helgaas
Hi Alex,
Thanks, this definitely clears up some problems. Two minor questions below.
Quoted 89 lines · Alexander Duyck
Quoted 1 line
From what I can tell there were several errors in the
sriov_enableexception handling path. Below is a brief list of what I believe I am fixing:
1. If
pcibios_enable_sriovfailed, we returned without disabling SR-IOV on the device.
2. Ifvirtfn_addfailed we didn't callpcibios_disable_sriovto undopcibios_enable_sriov.
3. We were resetting numvfs to 0 before a second had passed for the VFs to quiesce.
4. Minor coding style issues for white space and for assignment in conditional check.Beyond addressing these 4 issues there were also 2 other minor issues in that retval was a redundant variable with rc, and j wasn't actually needed as we could simply reverse the loop we were running when setting up i. As such I have updated the code to address those two items.
Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)drivers/pci/iov.c +15
-9@@ -231,13 +231,18 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)int __weak pcibios_sriov_enable (struct pci_dev *pdev,u16 num_vfs) { -return 0 ; +return 0 ; +} + +int __weak pcibios_sriov_disable (struct pci_dev *pdev) +{ +return 0 ; }static int sriov_enable (struct pci_dev *dev,int nr_virtfn) {int rc; -int i, j; +int i;int nres;u16 offset, stride, initial;struct resource *res;@@ -245,7 +250,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)struct pci_sriov *iov = dev->sriov;int bars =0 ;int bus; -int retval;if (!nr_virtfn)return 0 ;@@ -322,10 +326,11 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (nr_virtfn < initial) initial = nr_virtfn; -if ((retval =pcibios_sriov_enable (dev, initial))) { + rc =pcibios_sriov_enable (dev, initial); +if (rc) {dev_err (&dev->dev,"failure %d from pcibios_sriov_enable()\n" , - retval); -return retval; + rc); + goto err_pcibios; }for (i =0 ; i < initial; i++) {@@ -340,25 +345,23 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)return 0 ; failed: -for (j =0 ; j < i; j++) -virtfn_remove (dev, j,0 ); +while (i--) +virtfn_remove (dev, i,0 ); +pcibios_sriov_disable (dev); +err_pcibios: iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE |PCI_SRIOV_CTRL_MSE );pci_cfg_access_lock (dev);pci_write_config_word (dev, iov->pos +PCI_SRIOV_CTRL , iov->ctrl); -pci_iov_set_numvfs (dev,0 );ssleep (1 );pci_cfg_access_unlock (dev);
Quoted 8 lines
drivers/pci/iov.c static int sriov_enable(struct pci_dev *dev, int nr_virtfn) · line 362
Quoted 6 lines
drivers/pci/iov.c +1
-2if (iov->link != dev->devfn)sysfs_remove_link (&dev->dev.kobj,"dep_link" ); -return rc; -} +pci_iov_set_numvfs (dev,0 );Do you have a spec pointer for the 1 sec delay before clearing NumVFs?
The text from the SR-IOV spec v1.1 in relation to clearing VF enable reads:
If software Clears VF Enable, software must allow 1.0 s second after VF Enable is Cleared before
reading any field in the SR-IOV Extended Capability or the VF Migration State Array (see
Section 3.3.15.1).
I'm assuming the same would apply to writing to the region after VFE has been cleared.
Quoted 1 line
Does we need to clear NumVFs while holding the cfg access lock?
I don't think so.
Earlier in the function pci_iov_set_numvfs was getting set before without taking the lock. I think the lock is being used to enforce the required grace period on configuration space access following setting or clearing the VFE bit. The code as it is now matches what we have in sriov_disable so I suspect it likely works this way as that path has likely seen much more validation than the exception handling path for sriov_enable has.
Re: [PATCH 3/5] iov: Fix sriov_enable exception handling path
Quoted 107 lines · Alex Duyck
Quoted 95 lines · Bjorn Helgaas
Hi Alex,
Thanks, this definitely clears up some problems. Two minor questions below.
Quoted 89 lines · Alexander Duyck
Quoted 1 line
From what I can tell there were several errors in the
sriov_enableexception handling path. Below is a brief list of what I believe I am fixing:
1. If
pcibios_enable_sriovfailed, we returned without disabling SR-IOV on the device.
2. Ifvirtfn_addfailed we didn't callpcibios_disable_sriovto undopcibios_enable_sriov.
3. We were resetting numvfs to 0 before a second had passed for the VFs to quiesce.
4. Minor coding style issues for white space and for assignment in conditional check.Beyond addressing these 4 issues there were also 2 other minor issues in that retval was a redundant variable with rc, and j wasn't actually needed as we could simply reverse the loop we were running when setting up i. As such I have updated the code to address those two items.
Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)drivers/pci/iov.c +15
-9@@ -231,13 +231,18 @@ static void virtfn_remove(struct pci_dev *dev, int id, int reset)int __weak pcibios_sriov_enable (struct pci_dev *pdev,u16 num_vfs) { -return 0 ; +return 0 ; +} + +int __weak pcibios_sriov_disable (struct pci_dev *pdev) +{ +return 0 ; }static int sriov_enable (struct pci_dev *dev,int nr_virtfn) {int rc; -int i, j; +int i;int nres;u16 offset, stride, initial;struct resource *res;@@ -245,7 +250,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)struct pci_sriov *iov = dev->sriov;int bars =0 ;int bus; -int retval;if (!nr_virtfn)return 0 ;@@ -322,10 +326,11 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (nr_virtfn < initial) initial = nr_virtfn; -if ((retval =pcibios_sriov_enable (dev, initial))) { + rc =pcibios_sriov_enable (dev, initial); +if (rc) {dev_err (&dev->dev,"failure %d from pcibios_sriov_enable()\n" , - retval); -return retval; + rc); + goto err_pcibios; }for (i =0 ; i < initial; i++) {@@ -340,25 +345,23 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)return 0 ; failed: -for (j =0 ; j < i; j++) -virtfn_remove (dev, j,0 ); +while (i--) +virtfn_remove (dev, i,0 ); +pcibios_sriov_disable (dev); +err_pcibios: iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE |PCI_SRIOV_CTRL_MSE );pci_cfg_access_lock (dev);pci_write_config_word (dev, iov->pos +PCI_SRIOV_CTRL , iov->ctrl); -pci_iov_set_numvfs (dev,0 );ssleep (1 );pci_cfg_access_unlock (dev);Quoted 8 lines
drivers/pci/iov.c static int sriov_enable(struct pci_dev *dev, int nr_virtfn) · line 362
Quoted 6 lines
drivers/pci/iov.c +1
-2if (iov->link != dev->devfn)sysfs_remove_link (&dev->dev.kobj,"dep_link" ); -return rc; -} +pci_iov_set_numvfs (dev,0 );Do you have a spec pointer for the 1 sec delay before clearing NumVFs?
The text from the SR-IOV spec v1.1 in relation to clearing VF enable reads:
Quoted 8 lines
If software Clears VF Enable, software must allow 1.0 s second after VF Enable is Cleared before
reading any field in the SR-IOV Extended Capability or the VF Migration State Array (see
Section 3.3.15.1).I'm assuming the same would apply to writing to the region after VFE has been cleared.
Yep, thanks. Sec 3.3.3.1 clearly says we have to wait 1.0s after clearing VF Enable before reading anything in the capability. And pci_iov_set_numvfs() *does* read PCI_SRIOV_VF_OFFSET and PCI_SRIOV_VF_STRIDE.
Quoted 11 lines
Quoted 1 line
Does we need to clear NumVFs while holding the cfg access lock?
I don't think so.
Earlier in the function
pci_iov_set_numvfswas getting set before without taking the lock. I think the lock is being used to enforce the required grace period on configuration space access following setting or clearing the VFE bit. The code as it is now matches what we have insriov_disableso I suspect it likely works this way as that path has likely seen much more validation than the exception handling path forsriov_enablehas.
Right. I think the important part is that we hold the lock during the ssleep(1).
Slightly different problem: I'm a little worried about the places in sriov_enable() and sriov_restore_state() where we set VF Enable and msleep for 100ms. Sec. 3.3.3.1 requires the 100ms before we issue config requests to the VFs, the msleep satisfies that.
But 3.3.3.1 goes on to say the new VFs can return CRS status for up to 1.0s, and they can silently drop Memory Requests for up to 1.0s. I don't think the VF add path checks for CRS status: it doesn't call pci_bus_read_dev_vendor_id(). So I'm not sure we're quite covered here.
Bjorn
PATCH 4/5 iov: Variable and loop cleanup for sriov_disable and sriov_enable
In reply to Alexander DuyckThis patch is just a minor cleanup to go through and group all of the variables into one declaration instead of a long string of single declarations for each int. It also changes the direction for a couple loops as we are able to loop with less code this way as testing against 0 can be done as a part of the decrement operation.
--- drivers/pci/iov.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
drivers/pci/iov.c +4 -9
@@ -241,15 +241,11 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)static int sriov_enable (struct pci_dev *dev,int nr_virtfn) { -int rc; -int i; -int nres;u16 offset, stride, initial;struct resource *res;struct pci_dev *pdev;struct pci_sriov *iov = dev->sriov; -int bars =0 ; -int bus; +int rc, i, nres, bars, bus;if (!nr_virtfn)return 0 ;
@@ -271,8 +267,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (!offset || (nr_virtfn >1 && !stride))return -EIO ; - nres =0 ; -for (i =0 ; i <PCI_SRIOV_NUM_BARS ; i++) { +for (nres =0 , bars =0 , i =PCI_SRIOV_NUM_BARS ; i--;) { bars |= (1 << (i +PCI_IOV_RESOURCES )); res = &dev->resource[i +PCI_IOV_RESOURCES ];if (res->parent)
@@ -366,13 +361,13 @@ err_pcibios:static void sriov_disable (struct pci_dev *dev) { -int i;struct pci_sriov *iov = dev->sriov; +int i = iov->num_VFs;if (!iov->num_VFs)return ; -for (i =0 ; i < iov->num_VFs; i++) +while (i--)virtfn_remove (dev, i,0 );pcibios_sriov_disable (dev);
Re: [PATCH 4/5] iov: Variable and loop cleanup for sriov_disable and sriov_enable
Hi Alex,
Quoted 19 lines · Alexander Duyck
This patch is just a minor cleanup to go through and group all of the variables into one declaration instead of a long string of single declarations for each int. It also changes the direction for a couple loops as we are able to loop with less code this way as testing against 0 can be done as a part of the decrement operation.
Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)drivers/pci/iov.c +0
-0@@ -241,15 +241,11 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)static int sriov_enable (struct pci_dev *dev,int nr_virtfn) {
drivers/pci/iov.c int __weak pcibios_sriov_disable(struct pci_dev *pdev) · line 248
Quoted 10 lines
drivers/pci/iov.c +1
-5-int rc; -int i; -int nres;u16 offset, stride, initial;struct resource *res;struct pci_dev *pdev;struct pci_sriov *iov = dev->sriov; -int bars =0 ; -int bus; +int rc, i, nres, bars, bus;
I don't have a strong opinion on combining the declarations to one line, and I would apply it if you wanted to do the same for the whole file at once, in a patch by itself.
drivers/pci/iov.c static int sriov_enable(struct pci_dev *dev, int nr_virtfn) · line 270
Quoted 9 lines
drivers/pci/iov.c +1
-2if (!nr_virtfn)return 0 ;@@ -271,8 +267,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (!offset || (nr_virtfn >1 && !stride))return -EIO ; - nres =0 ; -for (i =0 ; i <PCI_SRIOV_NUM_BARS ; i++) { +for (nres =0 , bars =0 , i =PCI_SRIOV_NUM_BARS ; i--;) {
But I don't agree that this is easier to read. I suppose it could be a tiny bit more efficient, but I think the benefit to the reader of the usual "for (i = 0; i < limit; i++)" loop is larger.
Quoted 7 lines
drivers/pci/iov.c +0
-0bars |= (1 << (i +PCI_IOV_RESOURCES )); res = &dev->resource[i +PCI_IOV_RESOURCES ];if (res->parent)@@ -366,13 +361,13 @@ err_pcibios:static void sriov_disable (struct pci_dev *dev) {
drivers/pci/iov.c err_pcibios: · line 371
Quoted 10 lines
drivers/pci/iov.c +2
-2-int i;struct pci_sriov *iov = dev->sriov; +int i = iov->num_VFs;if (!iov->num_VFs)return ; -for (i =0 ; i < iov->num_VFs; i++) +while (i--)virtfn_remove (dev, i,0 );
I do like the change to remove devices in the reverse order as we added them. But I'm really partial to the way a "for" loop keeps all the loop control in one spot. So I would apply a patch that made it look like this:
for (i = iov->num_VFs - 1; i >= 0; i--)
virtfn_remove(dev, i, 0);Quoted 6 lines
drivers/pci/iov.c +0
-1pcibios_sriov_disable (dev); --To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] iov: Variable and loop cleanup for sriov_disable and sriov_enable
Quoted 20 lines · Bjorn Helgaas
Hi Alex,
Quoted 17 lines · Alexander Duyck
This patch is just a minor cleanup to go through and group all of the variables into one declaration instead of a long string of single declarations for each int. It also changes the direction for a couple loops as we are able to loop with less code this way as testing against 0 can be done as a part of the decrement operation.
Signed-off-by Alexander Duyck <[email protected]>--- drivers/pci/iov.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)drivers/pci/iov.c +0
-0@@ -241,15 +241,11 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
Quoted 15 lines
drivers/pci/iov.c int __weak pcibios_sriov_disable(struct pci_dev *pdev) · line 248
Quoted 12 lines
drivers/pci/iov.c +1
-5static int sriov_enable (struct pci_dev *dev,int nr_virtfn) { -int rc; -int i; -int nres;u16 offset, stride, initial;struct resource *res;struct pci_dev *pdev;struct pci_sriov *iov = dev->sriov; -int bars =0 ; -int bus; +int rc, i, nres, bars, bus;I don't have a strong opinion on combining the declarations to one line, and I would apply it if you wanted to do the same for the whole file at once, in a patch by itself.
Maybe I will work on that tonight. It doesn't look like it would be much work.
Quoted 7 lines
Quoted 7 lines
drivers/pci/iov.c +0
-0if (!nr_virtfn)return 0 ;@@ -271,8 +267,7 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)if (!offset || (nr_virtfn >1 && !stride))return -EIO ;
Quoted 6 lines
drivers/pci/iov.c static int sriov_enable(struct pci_dev *dev, int nr_virtfn) · line 270
Quoted 3 lines
drivers/pci/iov.c +1
-2- nres =0 ; -for (i =0 ; i <PCI_SRIOV_NUM_BARS ; i++) { +for (nres =0 , bars =0 , i =PCI_SRIOV_NUM_BARS ; i--;) {But I don't agree that this is easier to read. I suppose it could be a tiny bit more efficient, but I think the benefit to the reader of the usual "for (i = 0; i < limit; i++)" loop is larger.
I agree with you. Pulling nres and bars into the loop was probably a bad idea on my part.
As far as reordering the loops that is just a bad habit I have kind of developed from doing driver performance tuning. Running the loop backwards you are able to combine the test and decrement so it saves a few instructions since compare against 0 or signed is usually built in for free with the decrement instructions. For something like this it really isn't needed.
Quoted 22 lines
Quoted 7 lines
drivers/pci/iov.c +0
-0bars |= (1 << (i +PCI_IOV_RESOURCES )); res = &dev->resource[i +PCI_IOV_RESOURCES ];if (res->parent)@@ -366,13 +361,13 @@ err_pcibios:static void sriov_disable (struct pci_dev *dev) {drivers/pci/iov.c err_pcibios: · line 371
Quoted 10 lines
drivers/pci/iov.c +2
-2-int i;struct pci_sriov *iov = dev->sriov; +int i = iov->num_VFs;if (!iov->num_VFs)return ; -for (i =0 ; i < iov->num_VFs; i++) +while (i--)virtfn_remove (dev, i,0 );I do like the change to remove devices in the reverse order as we added them. But I'm really partial to the way a "for" loop keeps all the loop control in one spot. So I would apply a patch that made it look like this:
Quoted 3 lines
for (i = iov->num_VFs - 1; i >= 0; i--)
virtfn_remove(dev, i, 0);Yeah, this was a section I had gone back and forth on. I originally had it doing a '!i' check at the start instead of '!iov->num_VFs'. I think that was why I pulled it out like that. I started to undo parts of it for readability sake, but I probably should have undone the move.
PATCH 5/5 iov: Update sriov_enable to correctly handle offset and stride
In reply to Alexander DuyckAs per the SR-IOV spec the values for offset and stride are undefined until ARI and numVFs have been set. As such we should not be reading them until the values have been assigned to the hardware. In addition if numVFs is 0 the values for offset and stride are defined as unused.
This change corrects a spot where we were reading the offset and stride even though we had left numVFs set to 0.
--- drivers/pci/iov.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/pci/iov.c +11 -17
@@ -241,11 +241,11 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)static int sriov_enable (struct pci_dev *dev,int nr_virtfn) { -u16 offset, stride, initial;struct resource *res;struct pci_dev *pdev;struct pci_sriov *iov = dev->sriov;int rc, i, nres, bars, bus; +u16 initial;if (!nr_virtfn)return 0 ;
@@ -262,11 +262,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) (!(iov->cap & PCI_SRIOV_CAP_VFM) && (nr_virtfn > initial)))return -EINVAL ; -pci_read_config_word (dev, iov->pos +PCI_SRIOV_VF_OFFSET , &offset); -pci_read_config_word (dev, iov->pos +PCI_SRIOV_VF_STRIDE , &stride); -if (!offset || (nr_virtfn >1 && !stride)) -return -EIO ; -for (nres =0 , bars =0 , i =PCI_SRIOV_NUM_BARS ; i--;) { bars |= (1 << (i +PCI_IOV_RESOURCES )); res = &dev->resource[i +PCI_IOV_RESOURCES ];
@@ -278,16 +273,6 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)return -ENOMEM ; } - iov->offset = offset; - iov->stride = stride; - - bus =pci_iov_virtfn_bus (dev, nr_virtfn -1 ); -if (bus > dev->bus->busn_res.end) { -dev_err (&dev->dev,"can't enable %d VFs (bus %02x out of range of %pR)\n" , - nr_virtfn, bus, &dev->bus->busn_res); -return -ENOMEM ; - } -if (pci_enable_resources (dev, bars)) {dev_err (&dev->dev,"SR-IOV: IOV BARS not allocated\n" );return -ENOMEM ;
@@ -311,6 +296,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn) }pci_iov_set_numvfs (dev, nr_virtfn); + + bus =pci_iov_virtfn_bus (dev, nr_virtfn -1 ); +if (bus > dev->bus->busn_res.end) { +dev_err (&dev->dev,"can't enable %d VFs (bus %02x out of range of %pR)\n" , + nr_virtfn, bus, &dev->bus->busn_res); + rc = -ENOMEM ; + goto err_bus; + } + iov->ctrl |=PCI_SRIOV_CTRL_VFE |PCI_SRIOV_CTRL_MSE ;pci_cfg_access_lock (dev);pci_write_config_word (dev, iov->pos +PCI_SRIOV_CTRL , iov->ctrl);
@@ -350,7 +344,7 @@ err_pcibios:pci_write_config_word (dev, iov->pos +PCI_SRIOV_CTRL , iov->ctrl);ssleep (1 );pci_cfg_access_unlock (dev); - +err_bus:if (iov->link != dev->devfn)sysfs_remove_link (&dev->dev.kobj,"dep_link" );