[PATCH] virtio-pci: alloc only resources actually used.

[PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

Move resource allocation from common code to legacy and modern code.
Only request resources actually used, i.e. bar0 in legacy mode and
the bar(s) specified by capabilities in modern mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/virtio_pci_common.c |  7 -------
 drivers/virtio/virtio_pci_common.h |  2 ++
 drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
 drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index eba1b7a..327a121 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	if (rc)
 		goto err_enable_device;
 
-	rc = pci_request_regions(pci_dev, "virtio-pci");
-	if (rc)
-		goto err_request_regions;
-
 	if (force_legacy) {
 		rc = virtio_pci_legacy_probe(vp_dev);
 		/* Also try modern mode if we can't map BAR0 (no IO space). */
@@ -543,8 +539,6 @@ err_register:
 	else
 	     virtio_pci_modern_remove(vp_dev);
 err_probe:
-	pci_release_regions(pci_dev);
-err_request_regions:
 	pci_disable_device(pci_dev);
 err_enable_device:
 	kfree(vp_dev);
@@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 	else
 		virtio_pci_modern_remove(vp_dev);
 
-	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 28ee4e5..b976d96 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -75,6 +75,8 @@ struct virtio_pci_device {
 	/* Multiply queue_notify_off by this value. (non-legacy mode). */
 	u32 notify_offset_multiplier;
 
+	int modern_bars;
+
 	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 256a527..48bc979 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 {
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
+	int rc;
 
 	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
@@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 		return -ENODEV;
 	}
 
+	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
 	if (!vp_dev->ioaddr)
-		return -ENOMEM;
+		goto err_iomap;
 
 	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
 
@@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->del_vq = del_vq;
 
 	return 0;
+
+err_iomap:
+	pci_release_region(pci_dev, 0);
+	return rc;
 }
 
 void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
@@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_region(pci_dev, 0);
 }
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index e88e099..122ad7a 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
  * Returns offset of the capability, or 0.
  */
 static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
-					     u32 ioresource_types)
+					     u32 ioresource_types, int *bars)
 {
 	int pos;
 
@@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
 
 		if (type == cfg_type) {
 			if (pci_resource_len(dev, bar) &&
-			    pci_resource_flags(dev, bar) & ioresource_types)
+			    pci_resource_flags(dev, bar) & ioresource_types) {
+				*bars = (1 << bar);
 				return pos;
+			}
 		}
 	}
 	return 0;
@@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* check for a common config: if not, use legacy mode (bar 0). */
 	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!common) {
 		dev_info(&pci_dev->dev,
 			 "virtio_pci: leaving for legacy driver\n");
@@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* If common is there, these should be too... */
 	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
-					 IORESOURCE_IO | IORESOURCE_MEM);
+					 IORESOURCE_IO | IORESOURCE_MEM,
+					 &vp_dev->modern_bars);
 	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!isr || !notify) {
 		dev_err(&pci_dev->dev,
 			"virtio_pci: missing capabilities %i/%i/%i\n",
@@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	 * device-specific configuration.
 	 */
 	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
+
+	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
+					   "virtio-pci-modern");
+	if (err)
+		return err;
 
 	err = -EINVAL;
 	vp_dev->common = map_capability(pci_dev, common,
@@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 		pci_iounmap(pci_dev, vp_dev->notify_base);
 	pci_iounmap(pci_dev, vp_dev->isr);
 	pci_iounmap(pci_dev, vp_dev->common);
+	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
 }
-- 
1.8.3.1

[PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

Move resource allocation from common code to legacy and modern code.
Only request resources actually used, i.e. bar0 in legacy mode and
the bar(s) specified by capabilities in modern mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/virtio_pci_common.c |  7 -------
 drivers/virtio/virtio_pci_common.h |  2 ++
 drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
 drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index eba1b7a..327a121 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	if (rc)
 		goto err_enable_device;
 
-	rc = pci_request_regions(pci_dev, "virtio-pci");
-	if (rc)
-		goto err_request_regions;
-
 	if (force_legacy) {
 		rc = virtio_pci_legacy_probe(vp_dev);
 		/* Also try modern mode if we can't map BAR0 (no IO space). */
@@ -543,8 +539,6 @@ err_register:
 	else
 	     virtio_pci_modern_remove(vp_dev);
 err_probe:
-	pci_release_regions(pci_dev);
-err_request_regions:
 	pci_disable_device(pci_dev);
 err_enable_device:
 	kfree(vp_dev);
@@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 	else
 		virtio_pci_modern_remove(vp_dev);
 
-	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 28ee4e5..b976d96 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -75,6 +75,8 @@ struct virtio_pci_device {
 	/* Multiply queue_notify_off by this value. (non-legacy mode). */
 	u32 notify_offset_multiplier;
 
+	int modern_bars;
+
 	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 256a527..48bc979 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 {
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
+	int rc;
 
 	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
@@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 		return -ENODEV;
 	}
 
+	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
 	if (!vp_dev->ioaddr)
-		return -ENOMEM;
+		goto err_iomap;
 
 	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
 
@@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->del_vq = del_vq;
 
 	return 0;
+
+err_iomap:
+	pci_release_region(pci_dev, 0);
+	return rc;
 }
 
 void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
@@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_region(pci_dev, 0);
 }
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index e88e099..122ad7a 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
  * Returns offset of the capability, or 0.
  */
 static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
-					     u32 ioresource_types)
+					     u32 ioresource_types, int *bars)
 {
 	int pos;
 
@@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
 
 		if (type == cfg_type) {
 			if (pci_resource_len(dev, bar) &&
-			    pci_resource_flags(dev, bar) & ioresource_types)
+			    pci_resource_flags(dev, bar) & ioresource_types) {
+				*bars = (1 << bar);
 				return pos;
+			}
 		}
 	}
 	return 0;
@@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* check for a common config: if not, use legacy mode (bar 0). */
 	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!common) {
 		dev_info(&pci_dev->dev,
 			 "virtio_pci: leaving for legacy driver\n");
@@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* If common is there, these should be too... */
 	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
-					 IORESOURCE_IO | IORESOURCE_MEM);
+					 IORESOURCE_IO | IORESOURCE_MEM,
+					 &vp_dev->modern_bars);
 	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!isr || !notify) {
 		dev_err(&pci_dev->dev,
 			"virtio_pci: missing capabilities %i/%i/%i\n",
@@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	 * device-specific configuration.
 	 */
 	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
+
+	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
+					   "virtio-pci-modern");
+	if (err)
+		return err;
 
 	err = -EINVAL;
 	vp_dev->common = map_capability(pci_dev, common,
@@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 		pci_iounmap(pci_dev, vp_dev->notify_base);
 	pci_iounmap(pci_dev, vp_dev->isr);
 	pci_iounmap(pci_dev, vp_dev->common);
+	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
 }
-- 
1.8.3.1

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Tue, Jun 23, 2015 at 03:54:48PM +0200, Gerd Hoffmann wrote:
> Move resource allocation from common code to legacy and modern code.
> Only request resources actually used, i.e. bar0 in legacy mode and
> the bar(s) specified by capabilities in modern mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Looks nice but I think I see a minor bug, see below.

> ---
>  drivers/virtio/virtio_pci_common.c |  7 -------
>  drivers/virtio/virtio_pci_common.h |  2 ++
>  drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
>  drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
>  4 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index eba1b7a..327a121 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (rc)
>  		goto err_enable_device;
>  
> -	rc = pci_request_regions(pci_dev, "virtio-pci");
> -	if (rc)
> -		goto err_request_regions;
> -
>  	if (force_legacy) {
>  		rc = virtio_pci_legacy_probe(vp_dev);
>  		/* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -543,8 +539,6 @@ err_register:
>  	else
>  	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
> -	pci_release_regions(pci_dev);
> -err_request_regions:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
>  	kfree(vp_dev);
> @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	else
>  		virtio_pci_modern_remove(vp_dev);
>  
> -	pci_release_regions(pci_dev);
>  	pci_disable_device(pci_dev);
>  }
>  
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 28ee4e5..b976d96 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -75,6 +75,8 @@ struct virtio_pci_device {
>  	/* Multiply queue_notify_off by this value. (non-legacy mode). */
>  	u32 notify_offset_multiplier;
>  
> +	int modern_bars;
> +
>  	/* Legacy only field */
>  	/* the IO mapping for the PCI config space */
>  	void __iomem *ioaddr;
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 256a527..48bc979 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  {
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
> +	int rc;
>  
>  	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
>  	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  		return -ENODEV;
>  	}
>  
> +	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
>  	if (!vp_dev->ioaddr)
> -		return -ENOMEM;
> +		goto err_iomap;
>  
>  	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
>  
> @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->del_vq = del_vq;
>  
>  	return 0;
> +
> +err_iomap:
> +	pci_release_region(pci_dev, 0);
> +	return rc;
>  }
>  
>  void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
>  
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> +	pci_release_region(pci_dev, 0);
>  }
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index e88e099..122ad7a 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>   * Returns offset of the capability, or 0.
>   */
>  static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> -					     u32 ioresource_types)
> +					     u32 ioresource_types, int *bars)
>  {
>  	int pos;
>  
> @@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
>  
>  		if (type == cfg_type) {
>  			if (pci_resource_len(dev, bar) &&
> -			    pci_resource_flags(dev, bar) & ioresource_types)
> +			    pci_resource_flags(dev, bar) & ioresource_types) {
> +				*bars = (1 << bar);

Was this supposed to set a bit?
At most one bit will be set.

>  				return pos;
> +			}
>  		}
>  	}
>  	return 0;
> @@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* check for a common config: if not, use legacy mode (bar 0). */
>  	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!common) {
>  		dev_info(&pci_dev->dev,
>  			 "virtio_pci: leaving for legacy driver\n");
> @@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* If common is there, these should be too... */
>  	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
> -					 IORESOURCE_IO | IORESOURCE_MEM);
> +					 IORESOURCE_IO | IORESOURCE_MEM,
> +					 &vp_dev->modern_bars);
>  	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!isr || !notify) {
>  		dev_err(&pci_dev->dev,
>  			"virtio_pci: missing capabilities %i/%i/%i\n",
> @@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	 * device-specific configuration.
>  	 */
>  	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
> +
> +	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
> +					   "virtio-pci-modern");
> +	if (err)
> +		return err;
>  
>  	err = -EINVAL;
>  	vp_dev->common = map_capability(pci_dev, common,
> @@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
>  		pci_iounmap(pci_dev, vp_dev->notify_base);
>  	pci_iounmap(pci_dev, vp_dev->isr);
>  	pci_iounmap(pci_dev, vp_dev->common);
> +	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
>  }
> -- 
> 1.8.3.1

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Tue, Jun 23, 2015 at 03:54:48PM +0200, Gerd Hoffmann wrote:
> Move resource allocation from common code to legacy and modern code.
> Only request resources actually used, i.e. bar0 in legacy mode and
> the bar(s) specified by capabilities in modern mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Looks nice but I think I see a minor bug, see below.

> ---
>  drivers/virtio/virtio_pci_common.c |  7 -------
>  drivers/virtio/virtio_pci_common.h |  2 ++
>  drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
>  drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
>  4 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index eba1b7a..327a121 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (rc)
>  		goto err_enable_device;
>  
> -	rc = pci_request_regions(pci_dev, "virtio-pci");
> -	if (rc)
> -		goto err_request_regions;
> -
>  	if (force_legacy) {
>  		rc = virtio_pci_legacy_probe(vp_dev);
>  		/* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -543,8 +539,6 @@ err_register:
>  	else
>  	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
> -	pci_release_regions(pci_dev);
> -err_request_regions:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
>  	kfree(vp_dev);
> @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	else
>  		virtio_pci_modern_remove(vp_dev);
>  
> -	pci_release_regions(pci_dev);
>  	pci_disable_device(pci_dev);
>  }
>  
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 28ee4e5..b976d96 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -75,6 +75,8 @@ struct virtio_pci_device {
>  	/* Multiply queue_notify_off by this value. (non-legacy mode). */
>  	u32 notify_offset_multiplier;
>  
> +	int modern_bars;
> +
>  	/* Legacy only field */
>  	/* the IO mapping for the PCI config space */
>  	void __iomem *ioaddr;
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 256a527..48bc979 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  {
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
> +	int rc;
>  
>  	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
>  	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  		return -ENODEV;
>  	}
>  
> +	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
>  	if (!vp_dev->ioaddr)
> -		return -ENOMEM;
> +		goto err_iomap;
>  
>  	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
>  
> @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->del_vq = del_vq;
>  
>  	return 0;
> +
> +err_iomap:
> +	pci_release_region(pci_dev, 0);
> +	return rc;
>  }
>  
>  void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
>  
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> +	pci_release_region(pci_dev, 0);
>  }
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index e88e099..122ad7a 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>   * Returns offset of the capability, or 0.
>   */
>  static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> -					     u32 ioresource_types)
> +					     u32 ioresource_types, int *bars)
>  {
>  	int pos;
>  
> @@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
>  
>  		if (type == cfg_type) {
>  			if (pci_resource_len(dev, bar) &&
> -			    pci_resource_flags(dev, bar) & ioresource_types)
> +			    pci_resource_flags(dev, bar) & ioresource_types) {
> +				*bars = (1 << bar);

Was this supposed to set a bit?
At most one bit will be set.

>  				return pos;
> +			}
>  		}
>  	}
>  	return 0;
> @@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* check for a common config: if not, use legacy mode (bar 0). */
>  	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!common) {
>  		dev_info(&pci_dev->dev,
>  			 "virtio_pci: leaving for legacy driver\n");
> @@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* If common is there, these should be too... */
>  	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
> -					 IORESOURCE_IO | IORESOURCE_MEM);
> +					 IORESOURCE_IO | IORESOURCE_MEM,
> +					 &vp_dev->modern_bars);
>  	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!isr || !notify) {
>  		dev_err(&pci_dev->dev,
>  			"virtio_pci: missing capabilities %i/%i/%i\n",
> @@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	 * device-specific configuration.
>  	 */
>  	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
> +
> +	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
> +					   "virtio-pci-modern");
> +	if (err)
> +		return err;
>  
>  	err = -EINVAL;
>  	vp_dev->common = map_capability(pci_dev, common,
> @@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
>  		pci_iounmap(pci_dev, vp_dev->notify_base);
>  	pci_iounmap(pci_dev, vp_dev->isr);
>  	pci_iounmap(pci_dev, vp_dev->common);
> +	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
>  }
> -- 
> 1.8.3.1

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Wed, Jun 24, 2015 at 07:54:15AM +0200, Gerd Hoffmann wrote:
> Move resource allocation from common code to legacy and modern code.
> Only request resources actually used, i.e. bar0 in legacy mode and
> the bar(s) specified by capabilities in modern mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

In the future, please version patches in subject, and provide
changelog after ---.
Otherwise looks good, thanks!

>  drivers/virtio/virtio_pci_common.c |  7 -------
>  drivers/virtio/virtio_pci_common.h |  2 ++
>  drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
>  drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
>  4 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index eba1b7a..327a121 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (rc)
>  		goto err_enable_device;
>  
> -	rc = pci_request_regions(pci_dev, "virtio-pci");
> -	if (rc)
> -		goto err_request_regions;
> -
>  	if (force_legacy) {
>  		rc = virtio_pci_legacy_probe(vp_dev);
>  		/* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -543,8 +539,6 @@ err_register:
>  	else
>  	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
> -	pci_release_regions(pci_dev);
> -err_request_regions:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
>  	kfree(vp_dev);
> @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	else
>  		virtio_pci_modern_remove(vp_dev);
>  
> -	pci_release_regions(pci_dev);
>  	pci_disable_device(pci_dev);
>  }
>  
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 28ee4e5..b976d96 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -75,6 +75,8 @@ struct virtio_pci_device {
>  	/* Multiply queue_notify_off by this value. (non-legacy mode). */
>  	u32 notify_offset_multiplier;
>  
> +	int modern_bars;
> +
>  	/* Legacy only field */
>  	/* the IO mapping for the PCI config space */
>  	void __iomem *ioaddr;
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 256a527..48bc979 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  {
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
> +	int rc;
>  
>  	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
>  	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  		return -ENODEV;
>  	}
>  
> +	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
>  	if (!vp_dev->ioaddr)
> -		return -ENOMEM;
> +		goto err_iomap;
>  
>  	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
>  
> @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->del_vq = del_vq;
>  
>  	return 0;
> +
> +err_iomap:
> +	pci_release_region(pci_dev, 0);
> +	return rc;
>  }
>  
>  void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
>  
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> +	pci_release_region(pci_dev, 0);
>  }
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index e88e099..8e5cf19 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>   * Returns offset of the capability, or 0.
>   */
>  static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> -					     u32 ioresource_types)
> +					     u32 ioresource_types, int *bars)
>  {
>  	int pos;
>  
> @@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
>  
>  		if (type == cfg_type) {
>  			if (pci_resource_len(dev, bar) &&
> -			    pci_resource_flags(dev, bar) & ioresource_types)
> +			    pci_resource_flags(dev, bar) & ioresource_types) {
> +				*bars |= (1 << bar);
>  				return pos;
> +			}
>  		}
>  	}
>  	return 0;
> @@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* check for a common config: if not, use legacy mode (bar 0). */
>  	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!common) {
>  		dev_info(&pci_dev->dev,
>  			 "virtio_pci: leaving for legacy driver\n");
> @@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* If common is there, these should be too... */
>  	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
> -					 IORESOURCE_IO | IORESOURCE_MEM);
> +					 IORESOURCE_IO | IORESOURCE_MEM,
> +					 &vp_dev->modern_bars);
>  	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!isr || !notify) {
>  		dev_err(&pci_dev->dev,
>  			"virtio_pci: missing capabilities %i/%i/%i\n",
> @@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	 * device-specific configuration.
>  	 */
>  	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
> +
> +	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
> +					   "virtio-pci-modern");
> +	if (err)
> +		return err;
>  
>  	err = -EINVAL;
>  	vp_dev->common = map_capability(pci_dev, common,
> @@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
>  		pci_iounmap(pci_dev, vp_dev->notify_base);
>  	pci_iounmap(pci_dev, vp_dev->isr);
>  	pci_iounmap(pci_dev, vp_dev->common);
> +	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
>  }
> -- 
> 1.8.3.1

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Wed, Jun 24, 2015 at 07:54:15AM +0200, Gerd Hoffmann wrote:
> Move resource allocation from common code to legacy and modern code.
> Only request resources actually used, i.e. bar0 in legacy mode and
> the bar(s) specified by capabilities in modern mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

In the future, please version patches in subject, and provide
changelog after ---.
Otherwise looks good, thanks!

>  drivers/virtio/virtio_pci_common.c |  7 -------
>  drivers/virtio/virtio_pci_common.h |  2 ++
>  drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
>  drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
>  4 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index eba1b7a..327a121 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (rc)
>  		goto err_enable_device;
>  
> -	rc = pci_request_regions(pci_dev, "virtio-pci");
> -	if (rc)
> -		goto err_request_regions;
> -
>  	if (force_legacy) {
>  		rc = virtio_pci_legacy_probe(vp_dev);
>  		/* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -543,8 +539,6 @@ err_register:
>  	else
>  	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
> -	pci_release_regions(pci_dev);
> -err_request_regions:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
>  	kfree(vp_dev);
> @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	else
>  		virtio_pci_modern_remove(vp_dev);
>  
> -	pci_release_regions(pci_dev);
>  	pci_disable_device(pci_dev);
>  }
>  
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 28ee4e5..b976d96 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -75,6 +75,8 @@ struct virtio_pci_device {
>  	/* Multiply queue_notify_off by this value. (non-legacy mode). */
>  	u32 notify_offset_multiplier;
>  
> +	int modern_bars;
> +
>  	/* Legacy only field */
>  	/* the IO mapping for the PCI config space */
>  	void __iomem *ioaddr;
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 256a527..48bc979 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  {
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
> +	int rc;
>  
>  	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
>  	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  		return -ENODEV;
>  	}
>  
> +	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
>  	if (!vp_dev->ioaddr)
> -		return -ENOMEM;
> +		goto err_iomap;
>  
>  	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
>  
> @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->del_vq = del_vq;
>  
>  	return 0;
> +
> +err_iomap:
> +	pci_release_region(pci_dev, 0);
> +	return rc;
>  }
>  
>  void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
>  
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> +	pci_release_region(pci_dev, 0);
>  }
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index e88e099..8e5cf19 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>   * Returns offset of the capability, or 0.
>   */
>  static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
> -					     u32 ioresource_types)
> +					     u32 ioresource_types, int *bars)
>  {
>  	int pos;
>  
> @@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
>  
>  		if (type == cfg_type) {
>  			if (pci_resource_len(dev, bar) &&
> -			    pci_resource_flags(dev, bar) & ioresource_types)
> +			    pci_resource_flags(dev, bar) & ioresource_types) {
> +				*bars |= (1 << bar);
>  				return pos;
> +			}
>  		}
>  	}
>  	return 0;
> @@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* check for a common config: if not, use legacy mode (bar 0). */
>  	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!common) {
>  		dev_info(&pci_dev->dev,
>  			 "virtio_pci: leaving for legacy driver\n");
> @@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  
>  	/* If common is there, these should be too... */
>  	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
> -					 IORESOURCE_IO | IORESOURCE_MEM);
> +					 IORESOURCE_IO | IORESOURCE_MEM,
> +					 &vp_dev->modern_bars);
>  	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
>  	if (!isr || !notify) {
>  		dev_err(&pci_dev->dev,
>  			"virtio_pci: missing capabilities %i/%i/%i\n",
> @@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	 * device-specific configuration.
>  	 */
>  	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
> -					    IORESOURCE_IO | IORESOURCE_MEM);
> +					    IORESOURCE_IO | IORESOURCE_MEM,
> +					    &vp_dev->modern_bars);
> +
> +	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
> +					   "virtio-pci-modern");
> +	if (err)
> +		return err;
>  
>  	err = -EINVAL;
>  	vp_dev->common = map_capability(pci_dev, common,
> @@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
>  		pci_iounmap(pci_dev, vp_dev->notify_base);
>  	pci_iounmap(pci_dev, vp_dev->isr);
>  	pci_iounmap(pci_dev, vp_dev->common);
> +	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
>  }
> -- 
> 1.8.3.1

[PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

Move resource allocation from common code to legacy and modern code.
Only request resources actually used, i.e. bar0 in legacy mode and
the bar(s) specified by capabilities in modern mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/virtio_pci_common.c |  7 -------
 drivers/virtio/virtio_pci_common.h |  2 ++
 drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
 drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index eba1b7a..327a121 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	if (rc)
 		goto err_enable_device;
 
-	rc = pci_request_regions(pci_dev, "virtio-pci");
-	if (rc)
-		goto err_request_regions;
-
 	if (force_legacy) {
 		rc = virtio_pci_legacy_probe(vp_dev);
 		/* Also try modern mode if we can't map BAR0 (no IO space). */
@@ -543,8 +539,6 @@ err_register:
 	else
 	     virtio_pci_modern_remove(vp_dev);
 err_probe:
-	pci_release_regions(pci_dev);
-err_request_regions:
 	pci_disable_device(pci_dev);
 err_enable_device:
 	kfree(vp_dev);
@@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 	else
 		virtio_pci_modern_remove(vp_dev);
 
-	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 28ee4e5..b976d96 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -75,6 +75,8 @@ struct virtio_pci_device {
 	/* Multiply queue_notify_off by this value. (non-legacy mode). */
 	u32 notify_offset_multiplier;
 
+	int modern_bars;
+
 	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 256a527..48bc979 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 {
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
+	int rc;
 
 	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
@@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 		return -ENODEV;
 	}
 
+	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
 	if (!vp_dev->ioaddr)
-		return -ENOMEM;
+		goto err_iomap;
 
 	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
 
@@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->del_vq = del_vq;
 
 	return 0;
+
+err_iomap:
+	pci_release_region(pci_dev, 0);
+	return rc;
 }
 
 void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
@@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_region(pci_dev, 0);
 }
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index e88e099..8e5cf19 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
  * Returns offset of the capability, or 0.
  */
 static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
-					     u32 ioresource_types)
+					     u32 ioresource_types, int *bars)
 {
 	int pos;
 
@@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
 
 		if (type == cfg_type) {
 			if (pci_resource_len(dev, bar) &&
-			    pci_resource_flags(dev, bar) & ioresource_types)
+			    pci_resource_flags(dev, bar) & ioresource_types) {
+				*bars |= (1 << bar);
 				return pos;
+			}
 		}
 	}
 	return 0;
@@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* check for a common config: if not, use legacy mode (bar 0). */
 	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!common) {
 		dev_info(&pci_dev->dev,
 			 "virtio_pci: leaving for legacy driver\n");
@@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* If common is there, these should be too... */
 	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
-					 IORESOURCE_IO | IORESOURCE_MEM);
+					 IORESOURCE_IO | IORESOURCE_MEM,
+					 &vp_dev->modern_bars);
 	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!isr || !notify) {
 		dev_err(&pci_dev->dev,
 			"virtio_pci: missing capabilities %i/%i/%i\n",
@@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	 * device-specific configuration.
 	 */
 	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
+
+	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
+					   "virtio-pci-modern");
+	if (err)
+		return err;
 
 	err = -EINVAL;
 	vp_dev->common = map_capability(pci_dev, common,
@@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 		pci_iounmap(pci_dev, vp_dev->notify_base);
 	pci_iounmap(pci_dev, vp_dev->isr);
 	pci_iounmap(pci_dev, vp_dev->common);
+	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
 }
-- 
1.8.3.1

[PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

Move resource allocation from common code to legacy and modern code.
Only request resources actually used, i.e. bar0 in legacy mode and
the bar(s) specified by capabilities in modern mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/virtio_pci_common.c |  7 -------
 drivers/virtio/virtio_pci_common.h |  2 ++
 drivers/virtio/virtio_pci_legacy.c | 13 ++++++++++++-
 drivers/virtio/virtio_pci_modern.c | 24 ++++++++++++++++++------
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index eba1b7a..327a121 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	if (rc)
 		goto err_enable_device;
 
-	rc = pci_request_regions(pci_dev, "virtio-pci");
-	if (rc)
-		goto err_request_regions;
-
 	if (force_legacy) {
 		rc = virtio_pci_legacy_probe(vp_dev);
 		/* Also try modern mode if we can't map BAR0 (no IO space). */
@@ -543,8 +539,6 @@ err_register:
 	else
 	     virtio_pci_modern_remove(vp_dev);
 err_probe:
-	pci_release_regions(pci_dev);
-err_request_regions:
 	pci_disable_device(pci_dev);
 err_enable_device:
 	kfree(vp_dev);
@@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 	else
 		virtio_pci_modern_remove(vp_dev);
 
-	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 28ee4e5..b976d96 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -75,6 +75,8 @@ struct virtio_pci_device {
 	/* Multiply queue_notify_off by this value. (non-legacy mode). */
 	u32 notify_offset_multiplier;
 
+	int modern_bars;
+
 	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 256a527..48bc979 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 {
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
+	int rc;
 
 	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
@@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 		return -ENODEV;
 	}
 
+	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
 	if (!vp_dev->ioaddr)
-		return -ENOMEM;
+		goto err_iomap;
 
 	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
 
@@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->del_vq = del_vq;
 
 	return 0;
+
+err_iomap:
+	pci_release_region(pci_dev, 0);
+	return rc;
 }
 
 void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
@@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_region(pci_dev, 0);
 }
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index e88e099..8e5cf19 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -499,7 +499,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
  * Returns offset of the capability, or 0.
  */
 static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
-					     u32 ioresource_types)
+					     u32 ioresource_types, int *bars)
 {
 	int pos;
 
@@ -520,8 +520,10 @@ static inline int virtio_pci_find_capability(struct pci_dev *dev, u8 cfg_type,
 
 		if (type == cfg_type) {
 			if (pci_resource_len(dev, bar) &&
-			    pci_resource_flags(dev, bar) & ioresource_types)
+			    pci_resource_flags(dev, bar) & ioresource_types) {
+				*bars |= (1 << bar);
 				return pos;
+			}
 		}
 	}
 	return 0;
@@ -617,7 +619,8 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* check for a common config: if not, use legacy mode (bar 0). */
 	common = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_COMMON_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!common) {
 		dev_info(&pci_dev->dev,
 			 "virtio_pci: leaving for legacy driver\n");
@@ -626,9 +629,11 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 
 	/* If common is there, these should be too... */
 	isr = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_ISR_CFG,
-					 IORESOURCE_IO | IORESOURCE_MEM);
+					 IORESOURCE_IO | IORESOURCE_MEM,
+					 &vp_dev->modern_bars);
 	notify = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_NOTIFY_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
 	if (!isr || !notify) {
 		dev_err(&pci_dev->dev,
 			"virtio_pci: missing capabilities %i/%i/%i\n",
@@ -640,7 +645,13 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	 * device-specific configuration.
 	 */
 	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
-					    IORESOURCE_IO | IORESOURCE_MEM);
+					    IORESOURCE_IO | IORESOURCE_MEM,
+					    &vp_dev->modern_bars);
+
+	err = pci_request_selected_regions(pci_dev, vp_dev->modern_bars,
+					   "virtio-pci-modern");
+	if (err)
+		return err;
 
 	err = -EINVAL;
 	vp_dev->common = map_capability(pci_dev, common,
@@ -727,4 +738,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 		pci_iounmap(pci_dev, vp_dev->notify_base);
 	pci_iounmap(pci_dev, vp_dev->isr);
 	pci_iounmap(pci_dev, vp_dev->common);
+	pci_release_selected_regions(pci_dev, vp_dev->modern_bars);
 }
-- 
1.8.3.1

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Thu, Jun 18, 2015 at 10:14:40AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +static struct resource *request_capability(struct pci_dev *dev, int off,
> > > +					   const char *name)
> > > +{
> > > +	u8 bar;
> > > +	u32 offset, length;
> > > +
> > > +	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> > > +						 bar),
> > > +			     &bar);
> > > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> > > +			     &offset);
> > > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> > > +			      &length);
> > > +
> > > +	return request_mem_region(pci_resource_start(dev, bar) + offset,
> > > +				  length, name);
> > > +}
> > > +
> > 
> > For device config, this might request too much. The spec says:
> > 	The drivers SHOULD only map part of configuration structure large enough
> > 	for device operation.
> 
> We don't map it here though.  We just reserve what belongs to virtio
> according to the capabilities.
> 
> > I think you should limit this to PAGE_SIZE like we do for map_capability.
> 
> notify is much larger than PAGE_SIZE.

Yes - I mean for the device config.

> > >  	err = -EINVAL;
> > >  	vp_dev->common = map_capability(pci_dev, common,
> > >  					sizeof(struct virtio_pci_common_cfg), 4,
> > 
> > map_capability has a bunch of checks in place to validate the capability
> > structure. With request_capability called earlier, they are now done too
> > late.
> 
> Hmm, lets move the checks to find_capability then?
> 
> cheers,
>   Gerd
> 

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Thu, Jun 18, 2015 at 10:14:40AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > +static struct resource *request_capability(struct pci_dev *dev, int off,
> > > +					   const char *name)
> > > +{
> > > +	u8 bar;
> > > +	u32 offset, length;
> > > +
> > > +	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> > > +						 bar),
> > > +			     &bar);
> > > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> > > +			     &offset);
> > > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> > > +			      &length);
> > > +
> > > +	return request_mem_region(pci_resource_start(dev, bar) + offset,
> > > +				  length, name);
> > > +}
> > > +
> > 
> > For device config, this might request too much. The spec says:
> > 	The drivers SHOULD only map part of configuration structure large enough
> > 	for device operation.
> 
> We don't map it here though.  We just reserve what belongs to virtio
> according to the capabilities.
> 
> > I think you should limit this to PAGE_SIZE like we do for map_capability.
> 
> notify is much larger than PAGE_SIZE.

Yes - I mean for the device config.

> > >  	err = -EINVAL;
> > >  	vp_dev->common = map_capability(pci_dev, common,
> > >  					sizeof(struct virtio_pci_common_cfg), 4,
> > 
> > map_capability has a bunch of checks in place to validate the capability
> > structure. With request_capability called earlier, they are now done too
> > late.
> 
> Hmm, lets move the checks to find_capability then?
> 
> cheers,
>   Gerd
> 

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

  Hi,

> > +static struct resource *request_capability(struct pci_dev *dev, int off,
> > +					   const char *name)
> > +{
> > +	u8 bar;
> > +	u32 offset, length;
> > +
> > +	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> > +						 bar),
> > +			     &bar);
> > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> > +			     &offset);
> > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> > +			      &length);
> > +
> > +	return request_mem_region(pci_resource_start(dev, bar) + offset,
> > +				  length, name);
> > +}
> > +
> 
> For device config, this might request too much. The spec says:
> 	The drivers SHOULD only map part of configuration structure large enough
> 	for device operation.

We don't map it here though.  We just reserve what belongs to virtio
according to the capabilities.

> I think you should limit this to PAGE_SIZE like we do for map_capability.

notify is much larger than PAGE_SIZE.

> >  	err = -EINVAL;
> >  	vp_dev->common = map_capability(pci_dev, common,
> >  					sizeof(struct virtio_pci_common_cfg), 4,
> 
> map_capability has a bunch of checks in place to validate the capability
> structure. With request_capability called earlier, they are now done too
> late.

Hmm, lets move the checks to find_capability then?

cheers,
  Gerd

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

  Hi,

> > +static struct resource *request_capability(struct pci_dev *dev, int off,
> > +					   const char *name)
> > +{
> > +	u8 bar;
> > +	u32 offset, length;
> > +
> > +	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> > +						 bar),
> > +			     &bar);
> > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> > +			     &offset);
> > +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> > +			      &length);
> > +
> > +	return request_mem_region(pci_resource_start(dev, bar) + offset,
> > +				  length, name);
> > +}
> > +
> 
> For device config, this might request too much. The spec says:
> 	The drivers SHOULD only map part of configuration structure large enough
> 	for device operation.

We don't map it here though.  We just reserve what belongs to virtio
according to the capabilities.

> I think you should limit this to PAGE_SIZE like we do for map_capability.

notify is much larger than PAGE_SIZE.

> >  	err = -EINVAL;
> >  	vp_dev->common = map_capability(pci_dev, common,
> >  					sizeof(struct virtio_pci_common_cfg), 4,
> 
> map_capability has a bunch of checks in place to validate the capability
> structure. With request_capability called earlier, they are now done too
> late.

Hmm, lets move the checks to find_capability then?

cheers,
  Gerd

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Tue, Jun 16, 2015 at 03:57:52PM +0200, Gerd Hoffmann wrote:
> Move resource allocation from common code to legacy and modern code.
> Only request resources actually used, i.e. bar0 in legacy mode and
> the regions specified by capabilities in modern mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/virtio/virtio_pci_common.c |  7 -----
>  drivers/virtio/virtio_pci_common.h |  5 ++++
>  drivers/virtio/virtio_pci_legacy.c | 13 +++++++++-
>  drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++++++++++++++++-
>  4 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index eba1b7a..327a121 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (rc)
>  		goto err_enable_device;
>  
> -	rc = pci_request_regions(pci_dev, "virtio-pci");
> -	if (rc)
> -		goto err_request_regions;
> -
>  	if (force_legacy) {
>  		rc = virtio_pci_legacy_probe(vp_dev);
>  		/* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -543,8 +539,6 @@ err_register:
>  	else
>  	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
> -	pci_release_regions(pci_dev);
> -err_request_regions:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
>  	kfree(vp_dev);
> @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	else
>  		virtio_pci_modern_remove(vp_dev);
>  
> -	pci_release_regions(pci_dev);
>  	pci_disable_device(pci_dev);
>  }
>  
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 28ee4e5..9e744b8 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -75,6 +75,11 @@ struct virtio_pci_device {
>  	/* Multiply queue_notify_off by this value. (non-legacy mode). */
>  	u32 notify_offset_multiplier;
>  
> +	struct resource *res_common;
> +	struct resource *res_isr;
> +	struct resource *res_device;
> +	struct resource *res_notify;
> +
>  	/* Legacy only field */
>  	/* the IO mapping for the PCI config space */
>  	void __iomem *ioaddr;
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 256a527..48bc979 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  {
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
> +	int rc;
>  
>  	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
>  	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  		return -ENODEV;
>  	}
>  
> +	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
>  	if (!vp_dev->ioaddr)
> -		return -ENOMEM;
> +		goto err_iomap;
>  
>  	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
>  
> @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->del_vq = del_vq;
>  
>  	return 0;
> +
> +err_iomap:
> +	pci_release_region(pci_dev, 0);
> +	return rc;
>  }
>  
>  void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
>  
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> +	pci_release_region(pci_dev, 0);
>  }
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index e88e099..b4bd92b 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -64,6 +64,24 @@ static void vp_iowrite64_twopart(u64 val,
>  	vp_iowrite32(val >> 32, hi);
>  }
>  
> +static struct resource *request_capability(struct pci_dev *dev, int off,
> +					   const char *name)
> +{
> +	u8 bar;
> +	u32 offset, length;
> +
> +	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> +						 bar),
> +			     &bar);
> +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> +			     &offset);
> +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> +			      &length);
> +
> +	return request_mem_region(pci_resource_start(dev, bar) + offset,
> +				  length, name);
> +}
> +

For device config, this might request too much. The spec says:
	The drivers SHOULD only map part of configuration structure large enough
	for device operation.
I think you should limit this to PAGE_SIZE like we do for map_capability.

>  static void __iomem *map_capability(struct pci_dev *dev, int off,
>  				    size_t minlen,
>  				    u32 align,
> @@ -131,10 +149,12 @@ static void __iomem *map_capability(struct pci_dev *dev, int off,
>  	}
>  
>  	p = pci_iomap_range(dev, bar, offset, length);
> -	if (!p)
> +	if (!p) {
>  		dev_err(&dev->dev,
>  			"virtio_pci: unable to map virtio %u@%u on bar %i\n",
>  			length, offset, bar);
> +		return NULL;
> +	}
>  	return p;
>  }
>  
> @@ -642,6 +662,21 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
>  					    IORESOURCE_IO | IORESOURCE_MEM);
>  
> +	vp_dev->res_common = request_capability(pci_dev, common,
> +						"virtio-pci-common");
> +	vp_dev->res_isr = request_capability(pci_dev, isr,
> +						"virtio-pci-isr");
> +	vp_dev->res_notify = request_capability(pci_dev, notify,
> +						"virtio-pci-notify");
> +	if (device) {
> +		vp_dev->res_device = request_capability(pci_dev, device,
> +							"virtio-pci-device");
> +	}
> +	err = -EINVAL;
> +	if (!vp_dev->res_common || !vp_dev->res_isr || !vp_dev->res_notify ||
> +	    (device && !vp_dev->res_device))
> +		goto err_reserve;
> +
>  	err = -EINVAL;
>  	vp_dev->common = map_capability(pci_dev, common,
>  					sizeof(struct virtio_pci_common_cfg), 4,

map_capability has a bunch of checks in place to validate the capability
structure. With request_capability called earlier, they are now done too
late.

> @@ -714,6 +749,15 @@ err_map_notify:
>  err_map_isr:
>  	pci_iounmap(pci_dev, vp_dev->common);
>  err_map_common:
> +err_reserve:
> +	if (vp_dev->res_common)
> +		release_resource(vp_dev->res_common);
> +	if (vp_dev->res_isr)
> +		release_resource(vp_dev->res_isr);
> +	if (vp_dev->res_notify)
> +		release_resource(vp_dev->res_notify);
> +	if (vp_dev->res_device)
> +		release_resource(vp_dev->res_device);
>  	return err;
>  }
>  
> @@ -727,4 +771,10 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
>  		pci_iounmap(pci_dev, vp_dev->notify_base);
>  	pci_iounmap(pci_dev, vp_dev->isr);
>  	pci_iounmap(pci_dev, vp_dev->common);
> +
> +	release_resource(vp_dev->res_common);
> +	release_resource(vp_dev->res_isr);
> +	release_resource(vp_dev->res_notify);
> +	if (vp_dev->res_device)
> +		release_resource(vp_dev->res_device);
>  }
> -- 
> 1.8.3.1

Re: [PATCH] virtio-pci: alloc only resources actually used.

[Michael S. Tsirkin]

On Tue, Jun 16, 2015 at 03:57:52PM +0200, Gerd Hoffmann wrote:
> Move resource allocation from common code to legacy and modern code.
> Only request resources actually used, i.e. bar0 in legacy mode and
> the regions specified by capabilities in modern mode.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/virtio/virtio_pci_common.c |  7 -----
>  drivers/virtio/virtio_pci_common.h |  5 ++++
>  drivers/virtio/virtio_pci_legacy.c | 13 +++++++++-
>  drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++++++++++++++++-
>  4 files changed, 68 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index eba1b7a..327a121 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	if (rc)
>  		goto err_enable_device;
>  
> -	rc = pci_request_regions(pci_dev, "virtio-pci");
> -	if (rc)
> -		goto err_request_regions;
> -
>  	if (force_legacy) {
>  		rc = virtio_pci_legacy_probe(vp_dev);
>  		/* Also try modern mode if we can't map BAR0 (no IO space). */
> @@ -543,8 +539,6 @@ err_register:
>  	else
>  	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
> -	pci_release_regions(pci_dev);
> -err_request_regions:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
>  	kfree(vp_dev);
> @@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>  	else
>  		virtio_pci_modern_remove(vp_dev);
>  
> -	pci_release_regions(pci_dev);
>  	pci_disable_device(pci_dev);
>  }
>  
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index 28ee4e5..9e744b8 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -75,6 +75,11 @@ struct virtio_pci_device {
>  	/* Multiply queue_notify_off by this value. (non-legacy mode). */
>  	u32 notify_offset_multiplier;
>  
> +	struct resource *res_common;
> +	struct resource *res_isr;
> +	struct resource *res_device;
> +	struct resource *res_notify;
> +
>  	/* Legacy only field */
>  	/* the IO mapping for the PCI config space */
>  	void __iomem *ioaddr;
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 256a527..48bc979 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>  int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  {
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
> +	int rc;
>  
>  	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
>  	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
> @@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  		return -ENODEV;
>  	}
>  
> +	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
> +	if (rc)
> +		return rc;
> +
> +	rc = -ENOMEM;
>  	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
>  	if (!vp_dev->ioaddr)
> -		return -ENOMEM;
> +		goto err_iomap;
>  
>  	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
>  
> @@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->del_vq = del_vq;
>  
>  	return 0;
> +
> +err_iomap:
> +	pci_release_region(pci_dev, 0);
> +	return rc;
>  }
>  
>  void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
> @@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
>  	struct pci_dev *pci_dev = vp_dev->pci_dev;
>  
>  	pci_iounmap(pci_dev, vp_dev->ioaddr);
> +	pci_release_region(pci_dev, 0);
>  }
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index e88e099..b4bd92b 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -64,6 +64,24 @@ static void vp_iowrite64_twopart(u64 val,
>  	vp_iowrite32(val >> 32, hi);
>  }
>  
> +static struct resource *request_capability(struct pci_dev *dev, int off,
> +					   const char *name)
> +{
> +	u8 bar;
> +	u32 offset, length;
> +
> +	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
> +						 bar),
> +			     &bar);
> +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
> +			     &offset);
> +	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
> +			      &length);
> +
> +	return request_mem_region(pci_resource_start(dev, bar) + offset,
> +				  length, name);
> +}
> +

For device config, this might request too much. The spec says:
	The drivers SHOULD only map part of configuration structure large enough
	for device operation.
I think you should limit this to PAGE_SIZE like we do for map_capability.

>  static void __iomem *map_capability(struct pci_dev *dev, int off,
>  				    size_t minlen,
>  				    u32 align,
> @@ -131,10 +149,12 @@ static void __iomem *map_capability(struct pci_dev *dev, int off,
>  	}
>  
>  	p = pci_iomap_range(dev, bar, offset, length);
> -	if (!p)
> +	if (!p) {
>  		dev_err(&dev->dev,
>  			"virtio_pci: unable to map virtio %u@%u on bar %i\n",
>  			length, offset, bar);
> +		return NULL;
> +	}
>  	return p;
>  }
>  
> @@ -642,6 +662,21 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
>  					    IORESOURCE_IO | IORESOURCE_MEM);
>  
> +	vp_dev->res_common = request_capability(pci_dev, common,
> +						"virtio-pci-common");
> +	vp_dev->res_isr = request_capability(pci_dev, isr,
> +						"virtio-pci-isr");
> +	vp_dev->res_notify = request_capability(pci_dev, notify,
> +						"virtio-pci-notify");
> +	if (device) {
> +		vp_dev->res_device = request_capability(pci_dev, device,
> +							"virtio-pci-device");
> +	}
> +	err = -EINVAL;
> +	if (!vp_dev->res_common || !vp_dev->res_isr || !vp_dev->res_notify ||
> +	    (device && !vp_dev->res_device))
> +		goto err_reserve;
> +
>  	err = -EINVAL;
>  	vp_dev->common = map_capability(pci_dev, common,
>  					sizeof(struct virtio_pci_common_cfg), 4,

map_capability has a bunch of checks in place to validate the capability
structure. With request_capability called earlier, they are now done too
late.

> @@ -714,6 +749,15 @@ err_map_notify:
>  err_map_isr:
>  	pci_iounmap(pci_dev, vp_dev->common);
>  err_map_common:
> +err_reserve:
> +	if (vp_dev->res_common)
> +		release_resource(vp_dev->res_common);
> +	if (vp_dev->res_isr)
> +		release_resource(vp_dev->res_isr);
> +	if (vp_dev->res_notify)
> +		release_resource(vp_dev->res_notify);
> +	if (vp_dev->res_device)
> +		release_resource(vp_dev->res_device);
>  	return err;
>  }
>  
> @@ -727,4 +771,10 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
>  		pci_iounmap(pci_dev, vp_dev->notify_base);
>  	pci_iounmap(pci_dev, vp_dev->isr);
>  	pci_iounmap(pci_dev, vp_dev->common);
> +
> +	release_resource(vp_dev->res_common);
> +	release_resource(vp_dev->res_isr);
> +	release_resource(vp_dev->res_notify);
> +	if (vp_dev->res_device)
> +		release_resource(vp_dev->res_device);
>  }
> -- 
> 1.8.3.1

[PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

Move resource allocation from common code to legacy and modern code.
Only request resources actually used, i.e. bar0 in legacy mode and
the regions specified by capabilities in modern mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/virtio_pci_common.c |  7 -----
 drivers/virtio/virtio_pci_common.h |  5 ++++
 drivers/virtio/virtio_pci_legacy.c | 13 +++++++++-
 drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index eba1b7a..327a121 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	if (rc)
 		goto err_enable_device;
 
-	rc = pci_request_regions(pci_dev, "virtio-pci");
-	if (rc)
-		goto err_request_regions;
-
 	if (force_legacy) {
 		rc = virtio_pci_legacy_probe(vp_dev);
 		/* Also try modern mode if we can't map BAR0 (no IO space). */
@@ -543,8 +539,6 @@ err_register:
 	else
 	     virtio_pci_modern_remove(vp_dev);
 err_probe:
-	pci_release_regions(pci_dev);
-err_request_regions:
 	pci_disable_device(pci_dev);
 err_enable_device:
 	kfree(vp_dev);
@@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 	else
 		virtio_pci_modern_remove(vp_dev);
 
-	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 28ee4e5..9e744b8 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -75,6 +75,11 @@ struct virtio_pci_device {
 	/* Multiply queue_notify_off by this value. (non-legacy mode). */
 	u32 notify_offset_multiplier;
 
+	struct resource *res_common;
+	struct resource *res_isr;
+	struct resource *res_device;
+	struct resource *res_notify;
+
 	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 256a527..48bc979 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 {
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
+	int rc;
 
 	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
@@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 		return -ENODEV;
 	}
 
+	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
 	if (!vp_dev->ioaddr)
-		return -ENOMEM;
+		goto err_iomap;
 
 	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
 
@@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->del_vq = del_vq;
 
 	return 0;
+
+err_iomap:
+	pci_release_region(pci_dev, 0);
+	return rc;
 }
 
 void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
@@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_region(pci_dev, 0);
 }
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index e88e099..b4bd92b 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -64,6 +64,24 @@ static void vp_iowrite64_twopart(u64 val,
 	vp_iowrite32(val >> 32, hi);
 }
 
+static struct resource *request_capability(struct pci_dev *dev, int off,
+					   const char *name)
+{
+	u8 bar;
+	u32 offset, length;
+
+	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
+						 bar),
+			     &bar);
+	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
+			     &offset);
+	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
+			      &length);
+
+	return request_mem_region(pci_resource_start(dev, bar) + offset,
+				  length, name);
+}
+
 static void __iomem *map_capability(struct pci_dev *dev, int off,
 				    size_t minlen,
 				    u32 align,
@@ -131,10 +149,12 @@ static void __iomem *map_capability(struct pci_dev *dev, int off,
 	}
 
 	p = pci_iomap_range(dev, bar, offset, length);
-	if (!p)
+	if (!p) {
 		dev_err(&dev->dev,
 			"virtio_pci: unable to map virtio %u@%u on bar %i\n",
 			length, offset, bar);
+		return NULL;
+	}
 	return p;
 }
 
@@ -642,6 +662,21 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
 					    IORESOURCE_IO | IORESOURCE_MEM);
 
+	vp_dev->res_common = request_capability(pci_dev, common,
+						"virtio-pci-common");
+	vp_dev->res_isr = request_capability(pci_dev, isr,
+						"virtio-pci-isr");
+	vp_dev->res_notify = request_capability(pci_dev, notify,
+						"virtio-pci-notify");
+	if (device) {
+		vp_dev->res_device = request_capability(pci_dev, device,
+							"virtio-pci-device");
+	}
+	err = -EINVAL;
+	if (!vp_dev->res_common || !vp_dev->res_isr || !vp_dev->res_notify ||
+	    (device && !vp_dev->res_device))
+		goto err_reserve;
+
 	err = -EINVAL;
 	vp_dev->common = map_capability(pci_dev, common,
 					sizeof(struct virtio_pci_common_cfg), 4,
@@ -714,6 +749,15 @@ err_map_notify:
 err_map_isr:
 	pci_iounmap(pci_dev, vp_dev->common);
 err_map_common:
+err_reserve:
+	if (vp_dev->res_common)
+		release_resource(vp_dev->res_common);
+	if (vp_dev->res_isr)
+		release_resource(vp_dev->res_isr);
+	if (vp_dev->res_notify)
+		release_resource(vp_dev->res_notify);
+	if (vp_dev->res_device)
+		release_resource(vp_dev->res_device);
 	return err;
 }
 
@@ -727,4 +771,10 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 		pci_iounmap(pci_dev, vp_dev->notify_base);
 	pci_iounmap(pci_dev, vp_dev->isr);
 	pci_iounmap(pci_dev, vp_dev->common);
+
+	release_resource(vp_dev->res_common);
+	release_resource(vp_dev->res_isr);
+	release_resource(vp_dev->res_notify);
+	if (vp_dev->res_device)
+		release_resource(vp_dev->res_device);
 }
-- 
1.8.3.1

[PATCH] virtio-pci: alloc only resources actually used.

[Gerd Hoffmann]

Move resource allocation from common code to legacy and modern code.
Only request resources actually used, i.e. bar0 in legacy mode and
the regions specified by capabilities in modern mode.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/virtio/virtio_pci_common.c |  7 -----
 drivers/virtio/virtio_pci_common.h |  5 ++++
 drivers/virtio/virtio_pci_legacy.c | 13 +++++++++-
 drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index eba1b7a..327a121 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -510,10 +510,6 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	if (rc)
 		goto err_enable_device;
 
-	rc = pci_request_regions(pci_dev, "virtio-pci");
-	if (rc)
-		goto err_request_regions;
-
 	if (force_legacy) {
 		rc = virtio_pci_legacy_probe(vp_dev);
 		/* Also try modern mode if we can't map BAR0 (no IO space). */
@@ -543,8 +539,6 @@ err_register:
 	else
 	     virtio_pci_modern_remove(vp_dev);
 err_probe:
-	pci_release_regions(pci_dev);
-err_request_regions:
 	pci_disable_device(pci_dev);
 err_enable_device:
 	kfree(vp_dev);
@@ -562,7 +556,6 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 	else
 		virtio_pci_modern_remove(vp_dev);
 
-	pci_release_regions(pci_dev);
 	pci_disable_device(pci_dev);
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 28ee4e5..9e744b8 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -75,6 +75,11 @@ struct virtio_pci_device {
 	/* Multiply queue_notify_off by this value. (non-legacy mode). */
 	u32 notify_offset_multiplier;
 
+	struct resource *res_common;
+	struct resource *res_isr;
+	struct resource *res_device;
+	struct resource *res_notify;
+
 	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 256a527..48bc979 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -215,6 +215,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 {
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
+	int rc;
 
 	/* We only own devices >= 0x1000 and <= 0x103f: leave the rest. */
 	if (pci_dev->device < 0x1000 || pci_dev->device > 0x103f)
@@ -226,9 +227,14 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 		return -ENODEV;
 	}
 
+	rc = pci_request_region(pci_dev, 0, "virtio-pci-legacy");
+	if (rc)
+		return rc;
+
+	rc = -ENOMEM;
 	vp_dev->ioaddr = pci_iomap(pci_dev, 0, 0);
 	if (!vp_dev->ioaddr)
-		return -ENOMEM;
+		goto err_iomap;
 
 	vp_dev->isr = vp_dev->ioaddr + VIRTIO_PCI_ISR;
 
@@ -246,6 +252,10 @@ int virtio_pci_legacy_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->del_vq = del_vq;
 
 	return 0;
+
+err_iomap:
+	pci_release_region(pci_dev, 0);
+	return rc;
 }
 
 void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
@@ -253,4 +263,5 @@ void virtio_pci_legacy_remove(struct virtio_pci_device *vp_dev)
 	struct pci_dev *pci_dev = vp_dev->pci_dev;
 
 	pci_iounmap(pci_dev, vp_dev->ioaddr);
+	pci_release_region(pci_dev, 0);
 }
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index e88e099..b4bd92b 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -64,6 +64,24 @@ static void vp_iowrite64_twopart(u64 val,
 	vp_iowrite32(val >> 32, hi);
 }
 
+static struct resource *request_capability(struct pci_dev *dev, int off,
+					   const char *name)
+{
+	u8 bar;
+	u32 offset, length;
+
+	pci_read_config_byte(dev, off + offsetof(struct virtio_pci_cap,
+						 bar),
+			     &bar);
+	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, offset),
+			     &offset);
+	pci_read_config_dword(dev, off + offsetof(struct virtio_pci_cap, length),
+			      &length);
+
+	return request_mem_region(pci_resource_start(dev, bar) + offset,
+				  length, name);
+}
+
 static void __iomem *map_capability(struct pci_dev *dev, int off,
 				    size_t minlen,
 				    u32 align,
@@ -131,10 +149,12 @@ static void __iomem *map_capability(struct pci_dev *dev, int off,
 	}
 
 	p = pci_iomap_range(dev, bar, offset, length);
-	if (!p)
+	if (!p) {
 		dev_err(&dev->dev,
 			"virtio_pci: unable to map virtio %u@%u on bar %i\n",
 			length, offset, bar);
+		return NULL;
+	}
 	return p;
 }
 
@@ -642,6 +662,21 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	device = virtio_pci_find_capability(pci_dev, VIRTIO_PCI_CAP_DEVICE_CFG,
 					    IORESOURCE_IO | IORESOURCE_MEM);
 
+	vp_dev->res_common = request_capability(pci_dev, common,
+						"virtio-pci-common");
+	vp_dev->res_isr = request_capability(pci_dev, isr,
+						"virtio-pci-isr");
+	vp_dev->res_notify = request_capability(pci_dev, notify,
+						"virtio-pci-notify");
+	if (device) {
+		vp_dev->res_device = request_capability(pci_dev, device,
+							"virtio-pci-device");
+	}
+	err = -EINVAL;
+	if (!vp_dev->res_common || !vp_dev->res_isr || !vp_dev->res_notify ||
+	    (device && !vp_dev->res_device))
+		goto err_reserve;
+
 	err = -EINVAL;
 	vp_dev->common = map_capability(pci_dev, common,
 					sizeof(struct virtio_pci_common_cfg), 4,
@@ -714,6 +749,15 @@ err_map_notify:
 err_map_isr:
 	pci_iounmap(pci_dev, vp_dev->common);
 err_map_common:
+err_reserve:
+	if (vp_dev->res_common)
+		release_resource(vp_dev->res_common);
+	if (vp_dev->res_isr)
+		release_resource(vp_dev->res_isr);
+	if (vp_dev->res_notify)
+		release_resource(vp_dev->res_notify);
+	if (vp_dev->res_device)
+		release_resource(vp_dev->res_device);
 	return err;
 }
 
@@ -727,4 +771,10 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 		pci_iounmap(pci_dev, vp_dev->notify_base);
 	pci_iounmap(pci_dev, vp_dev->isr);
 	pci_iounmap(pci_dev, vp_dev->common);
+
+	release_resource(vp_dev->res_common);
+	release_resource(vp_dev->res_isr);
+	release_resource(vp_dev->res_notify);
+	if (vp_dev->res_device)
+		release_resource(vp_dev->res_device);
 }
-- 
1.8.3.1