Patchwork [2/2] fix virtio_blk serial pci config breakage

login
register
mail settings
Submitter john cooper
Date Sept. 29, 2009, 6:10 a.m.
Message ID <4AC1A4C7.1090809@redhat.com>
Download mbox | patch
Permalink /patch/34411/
State Superseded
Headers show

Comments

john cooper - Sept. 29, 2009, 6:10 a.m.
Change virtblk_identify() to pull ATA identify
data from the bar #5 map vs. the pci config
area.  Add minimal support for bar mapping external
to virtio/virtio_pci.c.

Signed-off-by: john cooper <john.cooper@redhat.com>
---
Michael S. Tsirkin - Sept. 29, 2009, 6:57 a.m.
On Tue, Sep 29, 2009 at 02:10:15AM -0400, john cooper wrote:
> Change virtblk_identify() to pull ATA identify
> data from the bar #5 map vs. the pci config
> area.  Add minimal support for bar mapping external
> to virtio/virtio_pci.c.
> 
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index aa1a3d5..a1687f3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -8,6 +8,8 @@
>  
>  #define PART_BITS 4
>  
> +#define VBLK_IDENTIFY_BAR 5	/* PCI BAR #5 maps identify/config area */
> +

This constant is PCI specific.

>  static int major, index;
>  
>  struct virtio_blk
> @@ -25,6 +27,9 @@ struct virtio_blk
>  
>  	mempool_t *pool;
>  
> +	/* maintains mapping of pci bar #5 unique to virtio_blk */

This puts PCI specific stuff in virtio_blk. It would be better to
hide this in transport-specific part.

> +	void __iomem *identify_ioaddr;
> +
>  	/* What host tells us, plus 2 for header & tailer. */
>  	unsigned int sg_elems;
>  
> @@ -171,24 +176,30 @@ static void do_virtblk_request(struct request_queue *q)
>  		vblk->vq->vq_ops->kick(vblk->vq);
>  }
>  
> -/* return ATA identify data
> +/* return ATA identify data if supported by virtio_blk device
>   */
>  static int virtblk_identify(struct gendisk *disk, void *argp)
>  {
>  	struct virtio_blk *vblk = disk->private_data;
>  	void *opaque;
> -	int err = -ENOMEM;
> +	int err, i;
> +	char *p;
>  
> +	err = 0;
>  	opaque = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
> -	if (!opaque)
> +	if (!opaque) {
> +		err = -ENOMEM;
>  		goto out;
> +	}
>  
> -	err = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_IDENTIFY,
> -		offsetof(struct virtio_blk_config, identify), opaque,
> -		VIRTIO_BLK_ID_BYTES);
> -
> -	if (err)
> +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_IDENTIFY) ||
> +	    !vblk->identify_ioaddr) {
> +		err = -ENOENT;
>  		goto out_kfree;
> +	}
> +
> +	for (p = opaque, i = 0; i < VIRTIO_BLK_ID_BYTES; ++i)
> +		*p++ = ioread8(vblk->identify_ioaddr + i);
>  
>  	if (copy_to_user(argp, opaque, VIRTIO_BLK_ID_BYTES))
>  		err = -EFAULT;
> @@ -383,6 +394,9 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	if (!err)
>  		blk_queue_logical_block_size(vblk->disk->queue, blk_size);
>  
> +	vblk->identify_ioaddr = vdev->config->map ?
> +		vdev->config->map(vdev, VBLK_IDENTIFY_BAR, 0) : NULL;
> +
>  	add_disk(vblk->disk);
>  	return 0;
>  
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 248e00e..abc6963 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -561,6 +561,13 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  	return err;
>  }
>  
> +/* translate struct virtio_device to struct pci_dev, setup map for bar
> + */
> +void __iomem *vp_map(struct virtio_device *vdev, int bar, unsigned long maxlen)
> +{
> +        return pci_iomap(to_vp_device(vdev)->pci_dev, bar, maxlen);
> +}
> +

I think we need a feature bit to figure out whether
device supports this feature.

>  static struct virtio_config_ops virtio_pci_config_ops = {
>  	.get		= vp_get,
>  	.set		= vp_set,
> @@ -571,6 +578,7 @@ static struct virtio_config_ops virtio_pci_config_ops = {
>  	.del_vqs	= vp_del_vqs,
>  	.get_features	= vp_get_features,
>  	.finalize_features = vp_finalize_features,
> +	.map		= vp_map, 
>  };
>  
>  static void virtio_pci_release_dev(struct device *_d)
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index e547e3c..b9b3c62 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -94,6 +94,8 @@ struct virtio_config_ops {
>  	void (*del_vqs)(struct virtio_device *);
>  	u32 (*get_features)(struct virtio_device *vdev);
>  	void (*finalize_features)(struct virtio_device *vdev);
> +	void __iomem *(*map)(struct virtio_device *vdev, int bar,
> +				    unsigned long maxlen);


Don't we ever unmap?

>  };
>  
>  /* If driver didn't advertise the feature, it will never appear. */
> 
> 
> -- 
> john.cooper@redhat.com
Rusty Russell - Sept. 29, 2009, 5:14 p.m.
On Tue, 29 Sep 2009 03:40:15 pm john cooper wrote:
> Change virtblk_identify() to pull ATA identify
> data from the bar #5 map vs. the pci config
> area.  Add minimal support for bar mapping external
> to virtio/virtio_pci.c.

Repeat after me: "virtio is not PCI".

See my alternate series...
Rusty.

Patch

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index aa1a3d5..a1687f3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,6 +8,8 @@ 
 
 #define PART_BITS 4
 
+#define VBLK_IDENTIFY_BAR 5	/* PCI BAR #5 maps identify/config area */
+
 static int major, index;
 
 struct virtio_blk
@@ -25,6 +27,9 @@  struct virtio_blk
 
 	mempool_t *pool;
 
+	/* maintains mapping of pci bar #5 unique to virtio_blk */
+	void __iomem *identify_ioaddr;
+
 	/* What host tells us, plus 2 for header & tailer. */
 	unsigned int sg_elems;
 
@@ -171,24 +176,30 @@  static void do_virtblk_request(struct request_queue *q)
 		vblk->vq->vq_ops->kick(vblk->vq);
 }
 
-/* return ATA identify data
+/* return ATA identify data if supported by virtio_blk device
  */
 static int virtblk_identify(struct gendisk *disk, void *argp)
 {
 	struct virtio_blk *vblk = disk->private_data;
 	void *opaque;
-	int err = -ENOMEM;
+	int err, i;
+	char *p;
 
+	err = 0;
 	opaque = kmalloc(VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
-	if (!opaque)
+	if (!opaque) {
+		err = -ENOMEM;
 		goto out;
+	}
 
-	err = virtio_config_buf(vblk->vdev, VIRTIO_BLK_F_IDENTIFY,
-		offsetof(struct virtio_blk_config, identify), opaque,
-		VIRTIO_BLK_ID_BYTES);
-
-	if (err)
+	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_IDENTIFY) ||
+	    !vblk->identify_ioaddr) {
+		err = -ENOENT;
 		goto out_kfree;
+	}
+
+	for (p = opaque, i = 0; i < VIRTIO_BLK_ID_BYTES; ++i)
+		*p++ = ioread8(vblk->identify_ioaddr + i);
 
 	if (copy_to_user(argp, opaque, VIRTIO_BLK_ID_BYTES))
 		err = -EFAULT;
@@ -383,6 +394,9 @@  static int __devinit virtblk_probe(struct virtio_device *vdev)
 	if (!err)
 		blk_queue_logical_block_size(vblk->disk->queue, blk_size);
 
+	vblk->identify_ioaddr = vdev->config->map ?
+		vdev->config->map(vdev, VBLK_IDENTIFY_BAR, 0) : NULL;
+
 	add_disk(vblk->disk);
 	return 0;
 
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 248e00e..abc6963 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -561,6 +561,13 @@  static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 	return err;
 }
 
+/* translate struct virtio_device to struct pci_dev, setup map for bar
+ */
+void __iomem *vp_map(struct virtio_device *vdev, int bar, unsigned long maxlen)
+{
+        return pci_iomap(to_vp_device(vdev)->pci_dev, bar, maxlen);
+}
+
 static struct virtio_config_ops virtio_pci_config_ops = {
 	.get		= vp_get,
 	.set		= vp_set,
@@ -571,6 +578,7 @@  static struct virtio_config_ops virtio_pci_config_ops = {
 	.del_vqs	= vp_del_vqs,
 	.get_features	= vp_get_features,
 	.finalize_features = vp_finalize_features,
+	.map		= vp_map, 
 };
 
 static void virtio_pci_release_dev(struct device *_d)
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index e547e3c..b9b3c62 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -94,6 +94,8 @@  struct virtio_config_ops {
 	void (*del_vqs)(struct virtio_device *);
 	u32 (*get_features)(struct virtio_device *vdev);
 	void (*finalize_features)(struct virtio_device *vdev);
+	void __iomem *(*map)(struct virtio_device *vdev, int bar,
+				    unsigned long maxlen);
 };
 
 /* If driver didn't advertise the feature, it will never appear. */