diff mbox series

[2/6] PCI/VPD: Remove struct pci_vpd_ops

Message ID b2532a41-df8b-860f-461f-d5c066c819d0@gmail.com (mailing list archive)
State Accepted
Delegated to: Bjorn Helgaas
Headers show
Series PCI/VPD: Further improvements | expand

Commit Message

Heiner Kallweit Aug. 8, 2021, 5:20 p.m. UTC 
Code can be significantly simplified by removing struct pci_vpd_ops and
avoiding the indirect calls.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/pci/vpd.c | 90 ++++++++++++++++++-----------------------------
 1 file changed, 34 insertions(+), 56 deletions(-)

Comments

Bjorn Helgaas Aug. 11, 2021, 10 p.m. UTC | #1 
[+cc Mark, Alex D, Jesse, Alex W]

On Sun, Aug 08, 2021 at 07:20:05PM +0200, Heiner Kallweit wrote:
> Code can be significantly simplified by removing struct pci_vpd_ops and
> avoiding the indirect calls.

I really like this patch.

Nothing to do with *this* patch, but I have a little heartburn about
the "access somebody else's VPD" approach.

I think the beginning of this was Mark's post [1].  IIUC, there are
Intel multifunction NICs that share some VPD hardware between
functions, and concurrent accesses to VPD of different functions
doesn't work correctly.

I'm pretty sure this is a defect per spec, because PCIe r5.0, sec
7.9.19 doesn't say anything about having to treat VPD on
multi-function devices specially.

The current solution is for all Intel multi-function NICs to redirect
VPD access to function 0.  That single-threads VPD access across all
the functions because we hold function 0's vpd->lock mutex while
reading or writing.

But I think we still have the problem that this implicit sharing of
function 0's VPD opens a channel between functions: if functions are
assigned to different VMs, the VMs can communicate by reading and
writing VPD.

So I wonder if we should just disallow VPD access for these NICs
except on function 0.  There was a little bit of discussion in that
direction at [2].

[1] https://lore.kernel.org/linux-pci/20150519000037.56109.68356.stgit@mdrustad-wks.jf.intel.com/
[2] https://lore.kernel.org/linux-pci/20150519160158.00002cd6@unknown/

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/vpd.c | 90 ++++++++++++++++++-----------------------------
>  1 file changed, 34 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index e87f299ee..6a0d617b2 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -13,13 +13,7 @@
>  
>  /* VPD access through PCI 2.2+ VPD capability */
>  
> -struct pci_vpd_ops {
> -	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
> -	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
> -};
> -
>  struct pci_vpd {
> -	const struct pci_vpd_ops *ops;
>  	struct mutex	lock;
>  	unsigned int	len;
>  	u8		cap;
> @@ -123,11 +117,16 @@ static int pci_vpd_wait(struct pci_dev *dev, bool set)
>  static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  			    void *arg)
>  {
> -	struct pci_vpd *vpd = dev->vpd;
> +	struct pci_vpd *vpd;
>  	int ret = 0;
>  	loff_t end = pos + count;
>  	u8 *buf = arg;
>  
> +	if (!dev || !dev->vpd)
> +		return -ENODEV;
> +
> +	vpd = dev->vpd;
> +
>  	if (pos < 0)
>  		return -EINVAL;
>  
> @@ -189,11 +188,16 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
>  			     const void *arg)
>  {
> -	struct pci_vpd *vpd = dev->vpd;
> +	struct pci_vpd *vpd;
>  	const u8 *buf = arg;
>  	loff_t end = pos + count;
>  	int ret = 0;
>  
> +	if (!dev || !dev->vpd)
> +		return -ENODEV;
> +
> +	vpd = dev->vpd;
> +
>  	if (pos < 0 || (pos & 3) || (count & 3))
>  		return -EINVAL;
>  
> @@ -238,44 +242,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
>  	return ret ? ret : count;
>  }
>  
> -static const struct pci_vpd_ops pci_vpd_ops = {
> -	.read = pci_vpd_read,
> -	.write = pci_vpd_write,
> -};
> -
> -static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count,
> -			       void *arg)
> -{
> -	struct pci_dev *tdev = pci_get_func0_dev(dev);
> -	ssize_t ret;
> -
> -	if (!tdev)
> -		return -ENODEV;
> -
> -	ret = pci_read_vpd(tdev, pos, count, arg);
> -	pci_dev_put(tdev);
> -	return ret;
> -}
> -
> -static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count,
> -				const void *arg)
> -{
> -	struct pci_dev *tdev = pci_get_func0_dev(dev);
> -	ssize_t ret;
> -
> -	if (!tdev)
> -		return -ENODEV;
> -
> -	ret = pci_write_vpd(tdev, pos, count, arg);
> -	pci_dev_put(tdev);
> -	return ret;
> -}
> -
> -static const struct pci_vpd_ops pci_vpd_f0_ops = {
> -	.read = pci_vpd_f0_read,
> -	.write = pci_vpd_f0_write,
> -};
> -
>  void pci_vpd_init(struct pci_dev *dev)
>  {
>  	struct pci_vpd *vpd;
> @@ -290,10 +256,6 @@ void pci_vpd_init(struct pci_dev *dev)
>  		return;
>  
>  	vpd->len = PCI_VPD_MAX_SIZE;
> -	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
> -		vpd->ops = &pci_vpd_f0_ops;
> -	else
> -		vpd->ops = &pci_vpd_ops;
>  	mutex_init(&vpd->lock);
>  	vpd->cap = cap;
>  	vpd->valid = 0;
> @@ -388,9 +350,17 @@ EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword);
>   */
>  ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf)
>  {
> -	if (!dev->vpd || !dev->vpd->ops)
> -		return -ENODEV;
> -	return dev->vpd->ops->read(dev, pos, count, buf);
> +	ssize_t ret;
> +
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> +		dev = pci_get_func0_dev(dev);
> +		ret = pci_vpd_read(dev, pos, count, buf);
> +		pci_dev_put(dev);
> +	} else {
> +		ret = pci_vpd_read(dev, pos, count, buf);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(pci_read_vpd);
>  
> @@ -403,9 +373,17 @@ EXPORT_SYMBOL(pci_read_vpd);
>   */
>  ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf)
>  {
> -	if (!dev->vpd || !dev->vpd->ops)
> -		return -ENODEV;
> -	return dev->vpd->ops->write(dev, pos, count, buf);
> +	ssize_t ret;
> +
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> +		dev = pci_get_func0_dev(dev);
> +		ret = pci_vpd_write(dev, pos, count, buf);
> +		pci_dev_put(dev);
> +	} else {
> +		ret = pci_vpd_write(dev, pos, count, buf);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(pci_write_vpd);
>  
> -- 
> 2.32.0
> 
>
Rustad, Mark D Aug. 11, 2021, 11:43 p.m. UTC | #2 
at 3:00 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> So I wonder if we should just disallow VPD access for these NICs
> except on function 0.  There was a little bit of discussion in that
> direction at [2].

If this is done, you'll have to be sure that any non-0 functions assigned  
to guests (which will appear as function 0 to the guest!) does not appear  
to have VPD and block access to those resources. That seems kind of messy,  
but should work. One hopes that no guest drivers are hard-wired to assume  
VPD presence based on the device type.

--
Mark Rustad (he/him), Ethernet Products Group, Intel Corporation
Alex Williamson Aug. 11, 2021, 11:58 p.m. UTC | #3 
On Wed, 11 Aug 2021 23:43:43 +0000
"Rustad, Mark D" <mark.d.rustad@intel.com> wrote:

> at 3:00 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > So I wonder if we should just disallow VPD access for these NICs
> > except on function 0.  There was a little bit of discussion in that
> > direction at [2].  
> 
> If this is done, you'll have to be sure that any non-0 functions assigned  
> to guests (which will appear as function 0 to the guest!) does not appear  
> to have VPD and block access to those resources. That seems kind of messy,  
> but should work. One hopes that no guest drivers are hard-wired to assume  
> VPD presence based on the device type.

A bit messy, yes.  But if we're saying VPD is not essential for
operation in the VM, I'd rather we mask it on all functions rather than
create scenarios where different identical functions produce different
userspace results.  Thanks,

Alex
diff mbox series

Patch

diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
index e87f299ee..6a0d617b2 100644
--- a/drivers/pci/vpd.c
+++ b/drivers/pci/vpd.c
@@ -13,13 +13,7 @@ 
 
 /* VPD access through PCI 2.2+ VPD capability */
 
-struct pci_vpd_ops {
-	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
-	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
-};
-
 struct pci_vpd {
-	const struct pci_vpd_ops *ops;
 	struct mutex	lock;
 	unsigned int	len;
 	u8		cap;
@@ -123,11 +117,16 @@  static int pci_vpd_wait(struct pci_dev *dev, bool set)
 static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 			    void *arg)
 {
-	struct pci_vpd *vpd = dev->vpd;
+	struct pci_vpd *vpd;
 	int ret = 0;
 	loff_t end = pos + count;
 	u8 *buf = arg;
 
+	if (!dev || !dev->vpd)
+		return -ENODEV;
+
+	vpd = dev->vpd;
+
 	if (pos < 0)
 		return -EINVAL;
 
@@ -189,11 +188,16 @@  static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
 static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 			     const void *arg)
 {
-	struct pci_vpd *vpd = dev->vpd;
+	struct pci_vpd *vpd;
 	const u8 *buf = arg;
 	loff_t end = pos + count;
 	int ret = 0;
 
+	if (!dev || !dev->vpd)
+		return -ENODEV;
+
+	vpd = dev->vpd;
+
 	if (pos < 0 || (pos & 3) || (count & 3))
 		return -EINVAL;
 
@@ -238,44 +242,6 @@  static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
 	return ret ? ret : count;
 }
 
-static const struct pci_vpd_ops pci_vpd_ops = {
-	.read = pci_vpd_read,
-	.write = pci_vpd_write,
-};
-
-static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count,
-			       void *arg)
-{
-	struct pci_dev *tdev = pci_get_func0_dev(dev);
-	ssize_t ret;
-
-	if (!tdev)
-		return -ENODEV;
-
-	ret = pci_read_vpd(tdev, pos, count, arg);
-	pci_dev_put(tdev);
-	return ret;
-}
-
-static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count,
-				const void *arg)
-{
-	struct pci_dev *tdev = pci_get_func0_dev(dev);
-	ssize_t ret;
-
-	if (!tdev)
-		return -ENODEV;
-
-	ret = pci_write_vpd(tdev, pos, count, arg);
-	pci_dev_put(tdev);
-	return ret;
-}
-
-static const struct pci_vpd_ops pci_vpd_f0_ops = {
-	.read = pci_vpd_f0_read,
-	.write = pci_vpd_f0_write,
-};
-
 void pci_vpd_init(struct pci_dev *dev)
 {
 	struct pci_vpd *vpd;
@@ -290,10 +256,6 @@  void pci_vpd_init(struct pci_dev *dev)
 		return;
 
 	vpd->len = PCI_VPD_MAX_SIZE;
-	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
-		vpd->ops = &pci_vpd_f0_ops;
-	else
-		vpd->ops = &pci_vpd_ops;
 	mutex_init(&vpd->lock);
 	vpd->cap = cap;
 	vpd->valid = 0;
@@ -388,9 +350,17 @@  EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword);
  */
 ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf)
 {
-	if (!dev->vpd || !dev->vpd->ops)
-		return -ENODEV;
-	return dev->vpd->ops->read(dev, pos, count, buf);
+	ssize_t ret;
+
+	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
+		dev = pci_get_func0_dev(dev);
+		ret = pci_vpd_read(dev, pos, count, buf);
+		pci_dev_put(dev);
+	} else {
+		ret = pci_vpd_read(dev, pos, count, buf);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(pci_read_vpd);
 
@@ -403,9 +373,17 @@  EXPORT_SYMBOL(pci_read_vpd);
  */
 ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf)
 {
-	if (!dev->vpd || !dev->vpd->ops)
-		return -ENODEV;
-	return dev->vpd->ops->write(dev, pos, count, buf);
+	ssize_t ret;
+
+	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
+		dev = pci_get_func0_dev(dev);
+		ret = pci_vpd_write(dev, pos, count, buf);
+		pci_dev_put(dev);
+	} else {
+		ret = pci_vpd_write(dev, pos, count, buf);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL(pci_write_vpd);