SCSI driver for VMware's virtual HBA.

drivers/scsi/pvscsi.c new file +4 -0

+static int pvscsi_setup_msg_workqueue(struct pvscsi_adapter *adapter)
+{
+	char name[32];
+

drivers/scsi/pvscsi.c new file +50 -0

+	adapter->workqueue = create_singlethread_workqueue(name);
+	if (!adapter->workqueue) {
+		printk(KERN_ERR "pvscsi: failed to create work queue\n");
+		return 0;
+	}
+	INIT_WORK(&adapter->work, pvscsi_msg_workqueue_handler);
+
+	return 1;
+}
+
+static irqreturn_t pvscsi_isr(int irq, void *devp)
+{
+	struct pvscsi_adapter *adapter = devp;
+	int handled;
+
+	if (adapter->use_msi || adapter->use_msix)
+		handled = true;
+	else {
+		u32 val = pvscsi_read_intr_status(adapter);
+		handled = (val & PVSCSI_INTR_ALL_SUPPORTED) != 0;
+		if (handled)
+			pvscsi_write_intr_status(devp, val);
+	}
+
+	if (handled) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&adapter->hw_lock, flags);
+
+		pvscsi_process_completion_ring(adapter);
+		if (adapter->use_msg && pvscsi_msg_pending(adapter))
+			queue_work(adapter->workqueue, &adapter->work);
+
+		spin_unlock_irqrestore(&adapter->hw_lock, flags);
+	}
+
+	return IRQ_RETVAL(handled);
+}
+
+static void pvscsi_free_sgls(const struct pvscsi_adapter *adapter)
+{
+	struct pvscsi_ctx *ctx = adapter->cmd_map;
+	unsigned i;
+
+	for (i = 0; i < adapter->req_depth; ++i, ++ctx)
+		pci_free_consistent(adapter->dev, PAGE_SIZE, ctx->sgl,
+				    ctx->sglPA);
+}
+
+static int pvscsi_setup_msix(const struct pvscsi_adapter *adapter, int

*irq) +{

drivers/scsi/pvscsi.c new file +5 -0

+#ifdef CONFIG_PCI_MSI
+	struct msix_entry entry = { 0, PVSCSI_VECTOR_COMPLETION };
+	int ret;
+
+	ret = pci_enable_msix(adapter->dev, &entry, 1);

drivers/scsi/pvscsi.c new file +8 -0

+static void pvscsi_shutdown_intr(struct pvscsi_adapter *adapter)
+{
+	if (adapter->irq) {
+		free_irq(adapter->irq, adapter);
+		adapter->irq = 0;
+	}
+#ifdef CONFIG_PCI_MSI
+	if (adapter->use_msi) {

drivers/scsi/pvscsi.c new file +14 -0

+	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0 &&
+	    pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
+		printk(KERN_INFO "pvscsi: using 64bit dma\n");
+	} else if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) == 0 &&
+		   pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) == 0) {
+		printk(KERN_INFO "pvscsi: using 32bit dma\n");
+	} else {
+		printk(KERN_ERR "pvscsi: failed to set DMA mask\n");
+		goto out_disable_device;
+	}
+
+	pvscsi_template.can_queue =
+		min(PVSCSI_MAX_NUM_PAGES_REQ_RING, pvscsi_ring_pages) *
+		PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;

drivers/scsi/pvscsi.c new file +14 -0

+	if (pci_request_regions(pdev, "pvscsi")) {
+		printk(KERN_ERR "pvscsi: pci memory selection failed\n");
+		goto out_free_host;
+	}
+
+	base = 0;
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		if ((pci_resource_flags(pdev, i) & PCI_BASE_ADDRESS_SPACE_IO))
+			continue;
+
+		if (pci_resource_len(pdev, i) <
+					PVSCSI_MEM_SPACE_NUM_PAGES * PAGE_SIZE)
+			continue;
+

drivers/scsi/pvscsi.c new file +16 -0

+	if (!adapter->mmioBase) {
+		printk(KERN_ERR "pvscsi: can't ioremap 0x%lx\n", base);
+		goto out_release_resources;
+	}
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, host);
+
+	ll_adapter_reset(adapter);
+
+	adapter->use_msg = pvscsi_setup_msg_workqueue(adapter);
+
+	error = pvscsi_allocate_rings(adapter);
+	if (error) {
+		printk(KERN_ERR "pvscsi: unable to allocate ring memory\n");
+		goto out_release_resources;

Quoted 4 lines · Alok Kataria

Greetings to all,

Please find below a patch which adds support for the VMware paravirtualized SCSI device (PVSCSI).

drivers/scsi/pvscsi.c line 140

Quoted 4 lines

drivers/scsi/pvscsi.c new file +4 -0

+static const struct pci_device_id pvscsi_pci_tbl[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_VMWARE, PCI_DEVICE_ID_VMWARE_PVSCSI) },
+	{ 0 }
+};

This can be written shorter as PCI_VDEVICE(VMWARE, 0x07C0). Putting the device id into the header doesn't get any benefit I see, it just makes harder to collect the pieces together.

It's used only here AFAICT so just write it down
here and be done with it. The vendor id might be better places in include/linux/pci_ids.h.

drivers/scsi/pvscsi.c line 165

Quoted 9 lines

(quoted diff) +2 -0

+static struct pvscsi_ctx *
+pvscsi_acquire_context(struct pvscsi_adapter *adapter, struct scsi_cmnd

*cmd) +{

drivers/scsi/pvscsi.c new file +6 -0

+	struct pvscsi_ctx *ctx;
+
+	if (list_empty(&adapter->cmd_pool))
+		return NULL;
+
+	ctx = list_entry(adapter->cmd_pool.next, struct pvscsi_ctx, list);

list_first_entry(&adapter->cmd_pool, struct pvscsi_ctx, list);

drivers/scsi/pvscsi.c line 187

Quoted 10 lines

drivers/scsi/pvscsi.c new file +9 -0

+/*
+ * Map a pvscsi_ctx struct to a context ID field value; we map to a simple
+ * non-zero integer.
+ */
+static u64 pvscsi_map_context(const struct pvscsi_adapter *adapter,
+			      const struct pvscsi_ctx *ctx)
+{
+	return ctx - adapter->cmd_map + 1;
+}

Is this guaranteed to always be !=0? Maybe add a BUG_ON or WARN_ON here? And if it is guaranteed please add a short comment to say /how/ this works, as just from the first look this is at least suspicious.

drivers/scsi/pvscsi.c line 239

Quoted 8 lines

drivers/scsi/pvscsi.c new file +7 -0

+static void pvscsi_write_cmd_desc(const struct pvscsi_adapter *adapter,
+				  u32 cmd, const void *desc, size_t len)
+{
+	const u32 *ptr = desc;
+	unsigned i;
+
+	len /= sizeof(u32);

How about sizeof(*ptr)? This would just remove the "magic" knowledge about the size.

Quoted 8 lines

drivers/scsi/pvscsi.c new file +8 -0

+static int pvscsi_setup_msg_workqueue(struct pvscsi_adapter *adapter)
+{
+	char name[32];
+
+	if (!pvscsi_use_msg)
+		return 0;
+
+	pvscsi_reg_write(adapter, PVSCSI_REG_OFFSET_COMMAND,

drivers/scsi/pvscsi.c line 1009

Quoted 6 lines

drivers/scsi/pvscsi.c new file +6 -0

+			 PVSCSI_CMD_SETUP_MSG_RING);
+
+	if (pvscsi_reg_read(adapter, PVSCSI_REG_OFFSET_COMMAND_STATUS) == -1)
+		return 0;
+
+	snprintf(name, sizeof name, "pvscsi_wq_%u", adapter->host->host_no);

sizeof(name)

Quoted 63 lines

drivers/scsi/pvscsi.c new file +50 -0

+	adapter->workqueue = create_singlethread_workqueue(name);
+	if (!adapter->workqueue) {
+		printk(KERN_ERR "pvscsi: failed to create work queue\n");
+		return 0;
+	}
+	INIT_WORK(&adapter->work, pvscsi_msg_workqueue_handler);
+
+	return 1;
+}
+
+static irqreturn_t pvscsi_isr(int irq, void *devp)
+{
+	struct pvscsi_adapter *adapter = devp;
+	int handled;
+
+	if (adapter->use_msi || adapter->use_msix)
+		handled = true;
+	else {
+		u32 val = pvscsi_read_intr_status(adapter);
+		handled = (val & PVSCSI_INTR_ALL_SUPPORTED) != 0;
+		if (handled)
+			pvscsi_write_intr_status(devp, val);
+	}
+
+	if (handled) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&adapter->hw_lock, flags);
+
+		pvscsi_process_completion_ring(adapter);
+		if (adapter->use_msg && pvscsi_msg_pending(adapter))
+			queue_work(adapter->workqueue, &adapter->work);
+
+		spin_unlock_irqrestore(&adapter->hw_lock, flags);
+	}
+
+	return IRQ_RETVAL(handled);
+}
+
+static void pvscsi_free_sgls(const struct pvscsi_adapter *adapter)
+{
+	struct pvscsi_ctx *ctx = adapter->cmd_map;
+	unsigned i;
+
+	for (i = 0; i < adapter->req_depth; ++i, ++ctx)
+		pci_free_consistent(adapter->dev, PAGE_SIZE, ctx->sgl,
+				    ctx->sglPA);
+}
+
+static int pvscsi_setup_msix(const struct pvscsi_adapter *adapter, int

*irq) +{

drivers/scsi/pvscsi.c new file +11 -0

+#ifdef CONFIG_PCI_MSI
+	struct msix_entry entry = { 0, PVSCSI_VECTOR_COMPLETION };
+	int ret;
+
+	ret = pci_enable_msix(adapter->dev, &entry, 1);
+	if (ret)
+		return ret;
+
+	*irq = entry.vector;
+
+	return 0;

drivers/scsi/pvscsi.c line 1076

Quoted 4 lines

drivers/scsi/pvscsi.c new file +4 -0

+#else
+	return -1;
+#endif
+}

You don't need those #ifdef's here. If CONFIG_PCI_MSI is not defined pci_enable_msix() is a static inline that always returns -1 (see include/linux/pci.h).

Quoted 13 lines

drivers/scsi/pvscsi.c new file +12 -0

+static void pvscsi_shutdown_intr(struct pvscsi_adapter *adapter)
+{
+	if (adapter->irq) {
+		free_irq(adapter->irq, adapter);
+		adapter->irq = 0;
+	}
+#ifdef CONFIG_PCI_MSI
+	if (adapter->use_msi) {
+		pci_disable_msi(adapter->dev);
+		adapter->use_msi = 0;
+	}
+

drivers/scsi/pvscsi.c line 1095

Quoted 6 lines

drivers/scsi/pvscsi.c new file +6 -0

+	if (adapter->use_msix) {
+		pci_disable_msix(adapter->dev);
+		adapter->use_msix = 0;
+	}
+#endif
+}

Those can go away then too, I think.

Quoted 12 lines

drivers/scsi/pvscsi.c new file +11 -0

+static int __devinit pvscsi_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
+	struct pvscsi_adapter *adapter;
+	struct Scsi_Host *host;
+	unsigned long base, i;
+	int error;
+
+	error = -ENODEV;
+
+	if (pci_enable_device(pdev))

Review

Quoted 1 line

(quoted diff) +1 -0

+		return error;

As always I suggest having a look on devres (see Documentation/driver- model/devres.txt) which could simplify your error handling here and your release function a lot. You only need to make sure it doesn't hurt if all the PCI resources are freed after the scsi ones as you would end up cleaning the scsi ones by hand and afterwards devres would throw all it handles (which will probably be most of your PCI stuff) away itself.

Quoted 14 lines

drivers/scsi/pvscsi.c new file +14 -0

+	if (pci_set_dma_mask(pdev, DMA_BIT_MASK(64)) == 0 &&
+	    pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)) == 0) {
+		printk(KERN_INFO "pvscsi: using 64bit dma\n");
+	} else if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) == 0 &&
+		   pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) == 0) {
+		printk(KERN_INFO "pvscsi: using 32bit dma\n");
+	} else {
+		printk(KERN_ERR "pvscsi: failed to set DMA mask\n");
+		goto out_disable_device;
+	}
+
+	pvscsi_template.can_queue =
+		min(PVSCSI_MAX_NUM_PAGES_REQ_RING, pvscsi_ring_pages) *
+		PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;

drivers/scsi/pvscsi.c line 1209

Quoted 10 lines

drivers/scsi/pvscsi.c new file +10 -0

+	pvscsi_template.cmd_per_lun =
+		min(pvscsi_template.can_queue, pvscsi_cmd_per_lun);
+	host = scsi_host_alloc(&pvscsi_template, sizeof(struct pvscsi_adapter));
+	if (!host) {
+		printk(KERN_ERR "pvscsi: failed to allocate host\n");
+		goto out_disable_device;
+	}
+
+	adapter = shost_priv(host);
+	memset(adapter, 0, sizeof *adapter);

sizeof(*adapter)

drivers/scsi/pvscsi.c line 1219

Quoted 10 lines

drivers/scsi/pvscsi.c new file +10 -0

+	adapter->dev  = pdev;
+	adapter->host = host;
+
+	spin_lock_init(&adapter->hw_lock);
+
+	host->max_channel = 0;
+	host->max_id      = 16;
+	host->max_lun     = 1;
+
+	pci_read_config_byte(pdev, PCI_CLASS_REVISION, &adapter->rev);

That's in pdev->revision anyway, isn't it?

Quoted 20 lines

drivers/scsi/pvscsi.c new file +19 -0

+	if (pci_request_regions(pdev, "pvscsi")) {
+		printk(KERN_ERR "pvscsi: pci memory selection failed\n");
+		goto out_free_host;
+	}
+
+	base = 0;
+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		if ((pci_resource_flags(pdev, i) & PCI_BASE_ADDRESS_SPACE_IO))
+			continue;
+
+		if (pci_resource_len(pdev, i) <
+					PVSCSI_MEM_SPACE_NUM_PAGES * PAGE_SIZE)
+			continue;
+
+		base = pci_resource_start(pdev, i);
+		break;
+	}
+
+	if (base == 0) {

drivers/scsi/pvscsi.c line 1244

Quoted 5 lines

drivers/scsi/pvscsi.c new file +5 -0

+		printk(KERN_ERR "pvscsi: adapter has no suitable MMIO region\n");
+		goto out_release_resources;
+	}
+
+	adapter->mmioBase = ioremap(base, PVSCSI_MEM_SPACE_SIZE);

You are mapping this here with a (probably) different size than the one you checked above.

Also pci_iomap() could simplify that as you don't have to get the base address and only need to tell it the bar number.

Quoted 21 lines

drivers/scsi/pvscsi.c new file +20 -0

+	if (!adapter->mmioBase) {
+		printk(KERN_ERR "pvscsi: can't ioremap 0x%lx\n", base);
+		goto out_release_resources;
+	}
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, host);
+
+	ll_adapter_reset(adapter);
+
+	adapter->use_msg = pvscsi_setup_msg_workqueue(adapter);
+
+	error = pvscsi_allocate_rings(adapter);
+	if (error) {
+		printk(KERN_ERR "pvscsi: unable to allocate ring memory\n");
+		goto out_release_resources;
+	}
+
+	/*
+	 * From this point on we should reset the adapter if anything goes

drivers/scsi/pvscsi.c line 1270

Quoted 6 lines

drivers/scsi/pvscsi.c new file +6 -0

+	 * wrong.
+	 */
+	pvscsi_setup_all_rings(adapter);
+
+	adapter->cmd_map = kmalloc(adapter->req_depth *
+				   sizeof(struct pvscsi_ctx), GFP_KERNEL);

kcalloc(), this will do overflow checking and also clear the memory for you.

drivers/scsi/pvscsi.c line 1320

Quoted 5 lines

(quoted diff) +1 -0

+	printk(KERN_INFO "VMware PVSCSI rev %d on bus:%u slot:%u func:%u host

#%u\n", + adapter->rev, pdev->bus->number, PCI_SLOT(pdev->devfn),

drivers/scsi/pvscsi.c new file +1 -0

+	       PCI_FUNC(pdev->devfn), host->host_no);

dev_info(&pdev->dev, ..), this should also give you the PCI domain/bus/slot/function information for free.

drivers/scsi/pvscsi.c line 1390

Quoted 4 lines

drivers/scsi/pvscsi.c new file +4 -0

+static int __init pvscsi_init(void)
+{
+	printk(KERN_DEBUG "%s - version %s\n",
+	       PVSCSI_LINUX_DRIVER_DESC, PVSCSI_DRIVER_VERSION_STRING);

pr_debug()

HTH & HAND

Eike

Hi Eike,

Quoted 11 lines

Quoted 11 lines

drivers/scsi/pvscsi.c new file +11 -0

+static int __devinit pvscsi_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
+	struct pvscsi_adapter *adapter;
+	struct Scsi_Host *host;
+	unsigned long base, i;
+	int error;
+
+	error = -ENODEV;
+
+	if (pci_enable_device(pdev))

Quoted 8 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		return error;

As always I suggest having a look on devres (see Documentation/driver- model/devres.txt) which could simplify your error handling here and your release function a lot. You only need to make sure it doesn't hurt if all the PCI resources are freed after the scsi ones as you would end up cleaning the scsi ones by hand and afterwards devres would throw all it handles (which will probably be most of your PCI stuff) away itself.

I took a quick look, but would prefer not to change this right now. Will do this as a incremental change later. Hope that is fine.

Quoted 4 lines

Quoted 4 lines

drivers/scsi/pvscsi.c new file +4 -0

+	adapter->dev  = pdev;
+	adapter->host = host;
+
+	spin_lock_init(&adapter->hw_lock);

Quoted 8 lines

drivers/scsi/pvscsi.c line 1219

Quoted 6 lines

drivers/scsi/pvscsi.c new file +6 -0

+
+	host->max_channel = 0;
+	host->max_id      = 16;
+	host->max_lun     = 1;
+
+	pci_read_config_byte(pdev, PCI_CLASS_REVISION, &adapter->rev);

That's in pdev->revision anyway, isn't it?

Yep, though, its needed in pvscsi_info, so will keep this in adapter too.

Quoted 22 lines · Alok Kataria

Hi Eike,

Quoted 11 lines

Quoted 11 lines

drivers/scsi/pvscsi.c new file +11 -0

+static int __devinit pvscsi_probe(struct pci_dev *pdev,
+				  const struct pci_device_id *id)
+{
+	struct pvscsi_adapter *adapter;
+	struct Scsi_Host *host;
+	unsigned long base, i;
+	int error;
+
+	error = -ENODEV;
+
+	if (pci_enable_device(pdev))

Quoted 8 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		return error;

As always I suggest having a look on devres (see Documentation/driver- model/devres.txt) which could simplify your error handling here and your release function a lot. You only need to make sure it doesn't hurt if all the PCI resources are freed after the scsi ones as you would end up cleaning the scsi ones by hand and afterwards devres would throw all it handles (which will probably be most of your PCI stuff) away itself.

Quoted 2 lines

I took a quick look, but would prefer not to change this right now. Will do this as a incremental change later. Hope that is fine.

Just a suggestion ;)

Quoted 13 lines

Quoted 12 lines

drivers/scsi/pvscsi.c line 1219

Quoted 10 lines

drivers/scsi/pvscsi.c new file +10 -0

+	adapter->dev  = pdev;
+	adapter->host = host;
+
+	spin_lock_init(&adapter->hw_lock);
+
+	host->max_channel = 0;
+	host->max_id      = 16;
+	host->max_lun     = 1;
+
+	pci_read_config_byte(pdev, PCI_CLASS_REVISION, &adapter->rev);

That's in pdev->revision anyway, isn't it?

Quoted 2 lines

Yep, though, its needed in pvscsi_info, so will keep this in adapter too.

Yes, but you don't need to do pci_read_config_byte() but simply copying that over from pdev->revision.

Eike

+static int __devinit pvscsi_allocate_rings(struct pvscsi_adapter *adapter)

drivers/scsi/pvscsi.c new file +33 -0

+{
+	adapter->rings_state = pci_alloc_consistent(adapter->dev, PAGE_SIZE,
+						    &adapter->ringStatePA);
+	if (!adapter->rings_state)
+		return -ENOMEM;
+
+	adapter->req_pages = min(PVSCSI_MAX_NUM_PAGES_REQ_RING,
+				 pvscsi_ring_pages);
+	adapter->req_depth = adapter->req_pages
+					* PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;
+	adapter->req_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->req_pages * PAGE_SIZE,
+						 &adapter->reqRingPA);
+	if (!adapter->req_ring)
+		return -ENOMEM;
+
+	adapter->cmp_pages = min(PVSCSI_MAX_NUM_PAGES_CMP_RING,
+				 pvscsi_ring_pages);
+	adapter->cmp_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->cmp_pages * PAGE_SIZE,
+						 &adapter->cmpRingPA);
+	if (!adapter->cmp_ring)
+		return -ENOMEM;
+
+	BUG_ON(adapter->ringStatePA & ~PAGE_MASK);
+	BUG_ON(adapter->reqRingPA   & ~PAGE_MASK);
+	BUG_ON(adapter->cmpRingPA   & ~PAGE_MASK);
+
+	if (!adapter->use_msg)
+		return 0;
+
+	adapter->msg_pages = min(PVSCSI_MAX_NUM_PAGES_MSG_RING,
+				 pvscsi_msg_ring_pages);

Alok,

Quoted 40 lines

+static int __devinit pvscsi_allocate_rings(struct pvscsi_adapter *adapter)

drivers/scsi/pvscsi.c new file +38 -0

+{
+	adapter->rings_state = pci_alloc_consistent(adapter->dev, PAGE_SIZE,
+						    &adapter->ringStatePA);
+	if (!adapter->rings_state)
+		return -ENOMEM;
+
+	adapter->req_pages = min(PVSCSI_MAX_NUM_PAGES_REQ_RING,
+				 pvscsi_ring_pages);
+	adapter->req_depth = adapter->req_pages
+					* PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;
+	adapter->req_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->req_pages * PAGE_SIZE,
+						 &adapter->reqRingPA);
+	if (!adapter->req_ring)
+		return -ENOMEM;
+
+	adapter->cmp_pages = min(PVSCSI_MAX_NUM_PAGES_CMP_RING,
+				 pvscsi_ring_pages);
+	adapter->cmp_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->cmp_pages * PAGE_SIZE,
+						 &adapter->cmpRingPA);
+	if (!adapter->cmp_ring)
+		return -ENOMEM;
+
+	BUG_ON(adapter->ringStatePA & ~PAGE_MASK);
+	BUG_ON(adapter->reqRingPA   & ~PAGE_MASK);
+	BUG_ON(adapter->cmpRingPA   & ~PAGE_MASK);
+
+	if (!adapter->use_msg)
+		return 0;
+
+	adapter->msg_pages = min(PVSCSI_MAX_NUM_PAGES_MSG_RING,
+				 pvscsi_msg_ring_pages);
+	adapter->msg_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->msg_pages * PAGE_SIZE,
+						 &adapter->msgRingPA);
+	if (!adapter->msg_ring)
+		return -ENOMEM;

drivers/scsi/pvscsi.c line 433

Quoted 5 lines

drivers/scsi/pvscsi.c new file +5 -0

+	BUG_ON(adapter->msgRingPA & ~PAGE_MASK);
+
+	return 0;
+}
+

I understand the emulation etc. But I see that this function isn't allocating multiple rings right? Didn't see any performance benefits?

Chetan

-----Original Message-----
From: Alok Kataria [mailto:[email protected]]
Sent: Friday, August 28, 2009 6:31 PM
To: Loke,Chetan
Cc: [email protected]; [email protected]; Dmitry Torokhov; [email protected]; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH] SCSI driver for VMware's virtual HBA.

Hi Chetan,

Quoted 20 lines

Alok,

drivers/scsi/pvscsi.c line 411

Quoted 11 lines

+static int __devinit pvscsi_allocate_rings(struct pvscsi_adapter *adapter)

drivers/scsi/pvscsi.c new file +9 -0

+{
+	adapter->rings_state = pci_alloc_consistent(adapter->dev, PAGE_SIZE,
+						    &adapter->ringStatePA);
+	adapter->req_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->req_pages * PAGE_SIZE,
+						 &adapter->reqRingPA);
+	adapter->cmp_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->cmp_pages * PAGE_SIZE,
+						 &adapter->cmpRingPA);

I understand the emulation etc. But I see that this function isn't allocating multiple rings right? Didn't see any performance benefits?

The function is allocating multiple rings, actually 2 of them for the IO path, request and completion rings (req_ring, cmp_ring).

Let me know if you were asking something else.

Quoted 23 lines · On Fri, 2009-08-28 at 14:18 -0700, [email protected] wrote:

Quoted 20 lines

Alok,

drivers/scsi/pvscsi.c line 411

Quoted 11 lines

+static int __devinit pvscsi_allocate_rings(struct pvscsi_adapter *adapter)

drivers/scsi/pvscsi.c new file +9 -0

+{
+	adapter->rings_state = pci_alloc_consistent(adapter->dev, PAGE_SIZE,
+						    &adapter->ringStatePA);
+	adapter->req_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->req_pages * PAGE_SIZE,
+						 &adapter->reqRingPA);
+	adapter->cmp_ring = pci_alloc_consistent(adapter->dev,
+						 adapter->cmp_pages * PAGE_SIZE,
+						 &adapter->cmpRingPA);

I understand the emulation etc. But I see that this function isn't allocating multiple rings right? Didn't see any performance benefits?

Quoted 5 lines

The function is allocating multiple rings, actually 2 of them for the IO path, request and completion rings (req_ring, cmp_ring).

Let me know if you were asking something else.

I was using 'Ring' loosely.
So, Ring == [req_path,cmpl_path]
Example - R0[req-path,cmpl-path]

But the code doesn't allocate R0,R1,...,RN.

Existing code -

      --------
	Ring-0
	--------
	  Req 
	   +
	   |

+
Cmpl

Why not multiple rings as shown below -

      --------	--------
	Ring-0	Ring-M
	--------	--------
	  Req 	  Req
	   +		   +
	   |		   |

+ +
Cmpl Cmpl

Creating multiple rings @ the guest level buys nothing in terms of performance?

This is also related to how the hypervisor will schedule the world-interrupts. If 'a' pv-driver has multiple interrupt lines enabled then will the interrupts/ISR's get routed on different PCPU's for a vmworld? Or is there a limitation?

Chetan

VMware PVSCSI driver - v2.

Changelog (v2-v1)
- use PCI_VDEVICE instead of PCI_DEVICE.
- use list_first_entry
- use parenthesis for every sizeof usage
- get rid of all #ifdef for CONFIG_PCI_MSI
- use PVSCSI_MEM_SPACE_SIZE while checking for MMIO resource len.
- use kcalloc instead of kmalloc.
- replaced a couple of scmd_printk usage with dev_dbg
- use dev_info and pr_info at couple of places.
- add a comment to pvscsi_map_context
- Add entry in MAINTAINERS.

Patch applies on top of 2.6.31-rc8.
--

SCSI driver for VMware's virtual HBA.

From: Alok N Kataria <[email protected]>

This is a driver for VMware's paravirtualized SCSI device, which should improve disk performance for guests running under control of VMware hypervisors that support such devices.

Signed-off-by Alok N Kataria <[email protected]>

---

 MAINTAINERS           |    7 
 drivers/scsi/Kconfig  |    8 
 drivers/scsi/Makefile |    1 
 drivers/scsi/pvscsi.c | 1391 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/pvscsi.h |  395 ++++++++++++++
 5 files changed, 1802 insertions(+), 0 deletions(-)
 create mode 100644 drivers/scsi/pvscsi.c
 create mode 100644 drivers/scsi/pvscsi.h

MAINTAINERS +7 -0

index 60299a9..952fe93 100644
@@ -5491,6 +5491,13 @@ S:	Maintained
 F:	drivers/vlynq/vlynq.c
 F:	include/linux/vlynq.h
 
+VMware PVSCSI driver
+M:	Alok Kataria <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/scsi/pvscsi.c
+F:	drivers/scsi/pvscsi.h
+
 VOLTAGE AND CURRENT REGULATOR FRAMEWORK
 M:	Liam Girdwood <[email protected]>
 M:	Mark Brown <[email protected]>

drivers/scsi/Kconfig +0 -0

index 9c23122..8a47981 100644

drivers/scsi/Kconfig +5 -0

+	help
+	  This driver supports VMware's para virtualized SCSI HBA.
+	  To compile this driver as a module, choose M here: the
+	  module will be called pvscsi.
+
 config LIBFC
 	tristate "LibFC module"
 	select SCSI_FC_ATTRS

drivers/scsi/Makefile +1 -0

index 25429ea..27fd013 100644
@@ -130,6 +130,7 @@ obj-$(CONFIG_SCSI_MVSAS)	+= mvsas/
 obj-$(CONFIG_PS3_ROM)		+= ps3rom.o
 obj-$(CONFIG_SCSI_CXGB3_ISCSI)	+= libiscsi.o libiscsi_tcp.o cxgb3i/
 obj-$(CONFIG_SCSI_BNX2_ISCSI)	+= libiscsi.o bnx2i/
+obj-$(CONFIG_VMWARE_PVSCSI)	+= pvscsi.o
 
 obj-$(CONFIG_ARM)		+= arm/
 

drivers/scsi/pvscsi.c new file +0 -0

new file mode 100644
index 0000000..fc85a3a

drivers/scsi/pvscsi.c new file +4 -0

+		BUG_ON(ctx->sglPA & ~PAGE_MASK);
+		if (!ctx->sgl) {
+			for (; i >= 0; --i, --ctx) {
+				pci_free_consistent(adapter->dev, PAGE_SIZE,

Quoted 1 line · Alok Kataria

VMware PVSCSI driver - v2.

OK, so the first thing that springs to mind is that we already have one of these things: the ibmvscsi ... is there no way we can share code between this and the other PV drivers?

Quoted 63 lines

Changelog (v2-v1)
- use PCI_VDEVICE instead of PCI_DEVICE.
- use list_first_entry
- use parenthesis for every sizeof usage
- get rid of all #ifdef for CONFIG_PCI_MSI
- use PVSCSI_MEM_SPACE_SIZE while checking for MMIO resource len.
- use kcalloc instead of kmalloc.
- replaced a couple of scmd_printk usage with dev_dbg
- use dev_info and pr_info at couple of places.
- add a comment to pvscsi_map_context
- Add entry in MAINTAINERS.

Patch applies on top of 2.6.31-rc8.
--

SCSI driver for VMware's virtual HBA.

From: Alok N Kataria <[email protected]>

This is a driver for VMware's paravirtualized SCSI device, which should improve disk performance for guests running under control of VMware hypervisors that support such devices.

Signed-off-by Alok N Kataria <[email protected]>

---

 MAINTAINERS           |    7 
 drivers/scsi/Kconfig  |    8 
 drivers/scsi/Makefile |    1 
 drivers/scsi/pvscsi.c | 1391 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/pvscsi.h |  395 ++++++++++++++
 5 files changed, 1802 insertions(+), 0 deletions(-)
 create mode 100644 drivers/scsi/pvscsi.c
 create mode 100644 drivers/scsi/pvscsi.h

MAINTAINERS +7 -0

index 60299a9..952fe93 100644
@@ -5491,6 +5491,13 @@ S:	Maintained
 F:	drivers/vlynq/vlynq.c
 F:	include/linux/vlynq.h
 
+VMware PVSCSI driver
+M:	Alok Kataria <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/scsi/pvscsi.c
+F:	drivers/scsi/pvscsi.h
+
 VOLTAGE AND CURRENT REGULATOR FRAMEWORK
 M:	Liam Girdwood <[email protected]>
 M:	Mark Brown <[email protected]>

drivers/scsi/Kconfig +0 -0

index 9c23122..8a47981 100644
@@ -620,6 +620,14 @@ config SCSI_FLASHPOINT
 	  substantial, so users of MultiMaster Host Adapters may not
 	  wish to include it.
 

drivers/scsi/Kconfig config SCSI_FLASHPOINT · line 625

Quoted 3 lines

drivers/scsi/Kconfig +3 -0

+config VMWARE_PVSCSI
+	tristate "VMware PVSCSI driver support"
+	depends on PCI && SCSI && X86

This is a PCI emulation driver ... it shouldn't be x86 only.

Quoted 26 lines

drivers/scsi/Kconfig +5 -0

+	help
+	  This driver supports VMware's para virtualized SCSI HBA.
+	  To compile this driver as a module, choose M here: the
+	  module will be called pvscsi.
+
 config LIBFC
 	tristate "LibFC module"
 	select SCSI_FC_ATTRS

drivers/scsi/Makefile +1 -0

index 25429ea..27fd013 100644
@@ -130,6 +130,7 @@ obj-$(CONFIG_SCSI_MVSAS)	+= mvsas/
 obj-$(CONFIG_PS3_ROM)		+= ps3rom.o
 obj-$(CONFIG_SCSI_CXGB3_ISCSI)	+= libiscsi.o libiscsi_tcp.o cxgb3i/
 obj-$(CONFIG_SCSI_BNX2_ISCSI)	+= libiscsi.o bnx2i/
+obj-$(CONFIG_VMWARE_PVSCSI)	+= pvscsi.o
 
 obj-$(CONFIG_ARM)		+= arm/
 

drivers/scsi/pvscsi.c new file +0 -0

new file mode 100644
index 0000000..fc85a3a

[...]

drivers/scsi/pvscsi.c line 1152

Quoted 11 lines

drivers/scsi/pvscsi.c new file +11 -0

+static int __devinit pvscsi_allocate_sg(struct pvscsi_adapter *adapter)
+{
+	struct pvscsi_ctx *ctx;
+	int i;
+
+	ctx = adapter->cmd_map;
+	BUILD_BUG_ON(sizeof(struct pvscsi_sg_list) > PAGE_SIZE);
+
+	for (i = 0; i < adapter->req_depth; ++i, ++ctx) {
+		ctx->sgl = pci_alloc_consistent(adapter->dev, PAGE_SIZE,
+						&ctx->sglPA);

Why do you need coherent memory for the sg list? Surely the use pattern of an SG list is that it follows a predefined ownership model between the driver and the virtual device, thus not really requiring coherent memory?

Quoted 4 lines

drivers/scsi/pvscsi.c new file +4 -0

+		BUG_ON(ctx->sglPA & ~PAGE_MASK);
+		if (!ctx->sgl) {
+			for (; i >= 0; --i, --ctx) {
+				pci_free_consistent(adapter->dev, PAGE_SIZE,

drivers/scsi/pvscsi.c line 1166

Quoted 10 lines

drivers/scsi/pvscsi.c new file +10 -0

+						    ctx->sgl, ctx->sglPA);
+				ctx->sgl = NULL;
+				ctx->sglPA = 0;
+			}
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}

James

Hi James,

Thanks for your comments.

Quoted 3 lines · James Bottomley

Quoted 1 line · Alok Kataria

VMware PVSCSI driver - v2.

Quoted 3 lines

OK, so the first thing that springs to mind is that we already have one of these things: the ibmvscsi ... is there no way we can share code between this and the other PV drivers?

I took a quick look at the ibmvscsi driver, and there are lot of differences between the two, mainly the ABI that is shared between the hypervisor and driver differ.

Also the ibmvscsi driver seems to offer a
lot of other features as well, like the SRP.

The pvscsi driver is a simple SCSI adapter driver and is basically no different than any other SCSI driver written for a particular HBA.

Quoted 8 lines · Alok Kataria

Hi James,

Thanks for your comments.

Quoted 3 lines · James Bottomley

Quoted 1 line · Alok Kataria

VMware PVSCSI driver - v2.

Quoted 7 lines

Quoted 3 lines

OK, so the first thing that springs to mind is that we already have one of these things: the ibmvscsi ... is there no way we can share code between this and the other PV drivers?

I took a quick look at the ibmvscsi driver, and there are lot of differences between the two, mainly the ABI that is shared between the hypervisor and driver differ.

Well, that's pretty abstractable, surely?  However, there is an interesting question of what the best hypervisor interface actually is.

Quoted 2 lines

 Also the ibmvscsi driver seems to offer a
lot of other features as well, like the SRP.

SRP is the protocol transfer abstraction.  It's just a way of packaging up SCSI commands for transfer over a DMA protocol (OK, so it was envisaged that the DMA protocol would be RDMA, but a hypervisor interface is also a DMA protocol).

Quoted 2 lines

The pvscsi driver is a simple SCSI adapter driver and is basically no different than any other SCSI driver written for a particular HBA.

Well, it is really ... hopefully all the hypervisor interfaces won't decide to be completely incompatible, so there's a good chance of code sharing between them.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Hi James,

Quoted 28 lines · James Bottomley

Quoted 8 lines · Alok Kataria

Hi James,

Thanks for your comments.

Quoted 3 lines · James Bottomley

Quoted 1 line · Alok Kataria

VMware PVSCSI driver - v2.

Quoted 7 lines

Quoted 3 lines

OK, so the first thing that springs to mind is that we already have one of these things: the ibmvscsi ... is there no way we can share code between this and the other PV drivers?

I took a quick look at the ibmvscsi driver, and there are lot of differences between the two, mainly the ABI that is shared between the hypervisor and driver differ.

Well, that's pretty abstractable, surely? ?However, there is an interesting question of what the best hypervisor interface actually is.

Quoted 2 lines

?Also the ibmvscsi driver seems to offer a
lot of other features as well, like the SRP.

SRP is the protocol transfer abstraction. ?It's just a way of packaging up SCSI commands for transfer over a DMA protocol (OK, so it was envisaged that the DMA protocol would be RDMA, but a hypervisor interface is also a DMA protocol).

Quoted 6 lines

Quoted 2 lines

The pvscsi driver is a simple SCSI adapter driver and is basically no different than any other SCSI driver written for a particular HBA.

Well, it is really ... hopefully all the hypervisor interfaces won't decide to be completely incompatible, so there's a good chance of code sharing between them.

Well, going by that theory every SCSI driver can share some code with any other SCSI driver, In short we can come up with an low level SCSI driver interface which every SCSI driver can make use of, to reduce code duplicity. We don't do that, and the reason being, that its a device and we shouldn't expect every device to share common features or design decisions.

Umm, we do share code between SCSI drivers. We have the ULDs (sg, sd, sr, etc), we have the midlayer, and we have the transport classes (spi, iscsi, fc, etc).

Quoted 5 lines · Matthew Wilcox

Umm, we do share code between SCSI drivers. We have the ULDs (sg, sd, sr, etc), we have the midlayer, and we have the transport classes (spi, iscsi, fc, etc).

This makes a tons of sense. a scsi_transport_paravirt can have lots of common nubs. I bet administrators would love such common management facility.

Boaz

Quoted 6 lines

Quoted 5 lines

drivers/scsi/pvscsi.c new file +0 -0

new file mode 100644
index 0000000..fc85a3a

[...]

Quoted 16 lines

drivers/scsi/pvscsi.c line 1152

Quoted 11 lines

drivers/scsi/pvscsi.c new file +11 -0

+static int __devinit pvscsi_allocate_sg(struct pvscsi_adapter *adapter)
+{
+	struct pvscsi_ctx *ctx;
+	int i;
+
+	ctx = adapter->cmd_map;
+	BUILD_BUG_ON(sizeof(struct pvscsi_sg_list) > PAGE_SIZE);
+
+	for (i = 0; i < adapter->req_depth; ++i, ++ctx) {
+		ctx->sgl = pci_alloc_consistent(adapter->dev, PAGE_SIZE,
+						&ctx->sglPA);

Why do you need coherent memory for the sg list? Surely the use pattern of an SG list is that it follows a predefined ownership model between the driver and the virtual device, thus not really requiring coherent memory?

The SG list needs to be accessed by the hypervisor too, and we need the physical address to access it from the hypervisor. We do that using the sglPA field right now, which the pci_.. interface returns. Do you think something else should be used for that purpose ?

Quoted 1 line · Alok Kataria

VMware PVSCSI driver - v2.

OK, so the first thing that springs to mind is that we already have one of these things: the ibmvscsi ... is there no way we can share code between this and the other PV drivers?

Quoted 2 lines · James Bottomley

On Mon, 2009-08-31 at 10:28 -0700, Alok Kataria wrote:

Quoted 5 lines

Quoted 1 line

VMware PVSCSI driver - v2.

OK, so the first thing that springs to mind is that we already have one of these things: the ibmvscsi ... is there no way we can share code between this and the other PV drivers?

Good question. But shouldn't the ibmvscsi driver be refactored before considering sharing ibmvscsi code with other paravirtualized drivers ?

A quote from the ibmvscsi.c source code:

* TODO: This is currently pretty tied to the IBM i/pSeries hypervisor
* interfaces. It would be really nice to abstract this above an RDMA
* layer.

Splitting the ibmvscsi.c driver in an SRP initiator and an RDMA driver would make the following possible:
- Reuse the existing SRP initiator (ib_srp). Currently there are two SRP initiators present in the Linux kernel -- one that uses the RDMA verbs API (ib_srp) and one that only works with IBM's i/pSeries hypervisor (ibmvscsi).
- Reuse the ib_ipoib kernel module to provide an IP stack on top of the new RDMA driver instead of having to maintain a separate network driver for this hardware (ibmveth).

More information about the architecture the ibmvscsi and the ibmveth drivers have been developed for can be found in the following paper:
D. Boutcher and D. Engebretsen, Linux Virtualization on IBM POWER5 Systems, Proceedings of the Linux Symposium, Vol. 1, July 2004, pp. 113-120 (http://www.kernel.org/doc/mirror/ols2004v1.pdf).

- Reuse the existing SRP initiator (ib_srp). Currently there are two SRP initiators present in the Linux kernel -- one that uses the RDMA verbs API (ib_srp) and one that only works with IBM's i/pSeries hypervisor (ibmvscsi).

- Reuse the ib_ipoib kernel module to provide an IP stack on top of the new RDMA driver instead of having to maintain a separate network driver for this hardware (ibmveth).

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.

Quoted 4 lines · Roland Dreier

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.

I really disagree. This kind of virtualised drivers are pretty much communication protocols, and not hardware. As such, why design a new one? If there's an infelicity in the ibmvscsi protocol, it makes sense to design a new one. But being different for the sake of being different is just a way to generate a huge amount of make-work.

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines · Roland Dreier

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.

Quoted 6 lines

I really disagree. This kind of virtualised drivers are pretty much communication protocols, and not hardware. As such, why design a new one? If there's an infelicity in the ibmvscsi protocol, it makes sense to design a new one. But being different for the sake of being different is just a way to generate a huge amount of make-work.

The same thing can be said about pretty much anything. We don't have single SCSI, network, etc driver handling every devices in their respective class, I don't see why it would be different here. A hypervisor presents the same interface to the guest OS (whether it is Linux, Solaris or another OS) much like a piece of silicone does and it may very well be different form other hypervisors.

Quoted 14 lines · Dmitry Torokhov

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines · Roland Dreier

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.

Quoted 6 lines

I really disagree. This kind of virtualised drivers are pretty much communication protocols, and not hardware. As such, why design a new one? If there's an infelicity in the ibmvscsi protocol, it makes sense to design a new one. But being different for the sake of being different is just a way to generate a huge amount of make-work.

Quoted 6 lines

The same thing can be said about pretty much anything. We don't have single SCSI, network, etc driver handling every devices in their respective class, I don't see why it would be different here. A hypervisor presents the same interface to the guest OS (whether it is Linux, Solaris or another OS) much like a piece of silicone does and it may very well be different form other hypervisors.

Nobody said you had to have the exact same driver for every hypervisor. What people are suggesting is that we look at commonalities in the interfaces both from a control plane point of view (transport class) and from a code sharing point of view (libscsivirt). However, all the hypervisor interfaces I've seen are basically DMA rings ... they really do seem to be very similar across hypervisors, so it does seem there could be a lot of shared commonality. I'm not going to insist on RDMA emulation, but perhaps you lot should agree on what a guest to hypervisor DMA interface looks like.

James

Quoted 22 lines · James Bottomley

Quoted 14 lines · Dmitry Torokhov

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines · Roland Dreier

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.

Quoted 6 lines

I really disagree. This kind of virtualised drivers are pretty much communication protocols, and not hardware. As such, why design a new one? If there's an infelicity in the ibmvscsi protocol, it makes sense to design a new one. But being different for the sake of being different is just a way to generate a huge amount of make-work.

Quoted 6 lines

The same thing can be said about pretty much anything. We don't have single SCSI, network, etc driver handling every devices in their respective class, I don't see why it would be different here. A hypervisor presents the same interface to the guest OS (whether it is Linux, Solaris or another OS) much like a piece of silicone does and it may very well be different form other hypervisors.

Quoted 9 lines

Nobody said you had to have the exact same driver for every hypervisor. What people are suggesting is that we look at commonalities in the interfaces both from a control plane point of view (transport class) and from a code sharing point of view (libscsivirt). However, all the hypervisor interfaces I've seen are basically DMA rings ... they really do seem to be very similar across hypervisors, so it does seem there could be a lot of shared commonality. I'm not going to insist on RDMA emulation, but perhaps you lot should agree on what a guest to hypervisor DMA interface looks like.

Which is this other hypervisor driver that you are talking about, ibmvscsi is using RDMA emulation and I don't think you mean that.

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

Quoted 33 lines · Alok Kataria

Quoted 31 lines · James Bottomley

Quoted 14 lines · Dmitry Torokhov

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines · Roland Dreier

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.

Quoted 6 lines

I really disagree. This kind of virtualised drivers are pretty much communication protocols, and not hardware. As such, why design a new one? If there's an infelicity in the ibmvscsi protocol, it makes sense to design a new one. But being different for the sake of being different is just a way to generate a huge amount of make-work.

Quoted 6 lines

The same thing can be said about pretty much anything. We don't have single SCSI, network, etc driver handling every devices in their respective class, I don't see why it would be different here. A hypervisor presents the same interface to the guest OS (whether it is Linux, Solaris or another OS) much like a piece of silicone does and it may very well be different form other hypervisors.

Nobody said you had to have the exact same driver for every hypervisor. What people are suggesting is that we look at commonalities in the interfaces both from a control plane point of view (transport class) and from a code sharing point of view (libscsivirt). However, all the hypervisor interfaces I've seen are basically DMA rings ... they really do seem to be very similar across hypervisors, so it does seem there could be a lot of shared commonality. I'm not going to insist on RDMA emulation, but perhaps you lot should agree on what a guest to hypervisor DMA interface looks like.

Quoted 2 lines

Which is this other hypervisor driver that you are talking about, ibmvscsi is using RDMA emulation and I don't think you mean that.

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

The benefit to users would be a common control plane and interface from the transport class, plus common code means more testers regardless of virtualisation technology chosen.

James

Quoted 2 lines

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

I don't see the sg_ring abstraction that you are talking about. Can you please give me some pointers.

Also regarding Xen and KVM I think they are using the xenbus/vbus interface, which is quite different than what we do here.

Quoted 5 lines

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

Quoted 2 lines

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

And not just that, different HV-vendors can have different features, like say XYZ can come up tomorrow and implement the multiple rings interface so the feature set doesn't remain common and we will have less code to share in the not so distant future.

Quoted 5 lines · Alok Kataria

Quoted 2 lines

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

I don't see the sg_ring abstraction that you are talking about. Can you please give me some pointers.

it's in drivers/lguest ... apparently it's vring now and the code is in driver/virtio

Quoted 2 lines

Also regarding Xen and KVM I think they are using the xenbus/vbus interface, which is quite different than what we do here.

Not sure about Xen ... KVM uses virtio above.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

Quoted 5 lines

And not just that, different HV-vendors can have different features, like say XYZ can come up tomorrow and implement the multiple rings interface so the feature set doesn't remain common and we will have less code to share in the not so distant future.

Multiple rings is really just a multiqueue abstraction. That's fine, but it needs a standard multiqueue control plane.

The desire to one up the competition by adding a new whiz bang feature to which you code a special interface is very common in the storage industry. The counter pressure is that consumers really like these things standardised. That's what the transport class abstraction is all about.

We also seem to be off on a tangent about hypervisor interfaces. I'm actually more interested in the utility of an SRP abstraction or at least something SAM based. It seems that in your driver you don't quite do the task management functions as SAM requests, but do them over your own protocol abstractions.

James

Quoted 20 lines · James Bottomley

Quoted 5 lines · Alok Kataria

Quoted 2 lines

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

I don't see the sg_ring abstraction that you are talking about. Can you please give me some pointers.

it's in drivers/lguest ... apparently it's vring now and the code is in driver/virtio

Quoted 2 lines

Also regarding Xen and KVM I think they are using the xenbus/vbus interface, which is quite different than what we do here.

Not sure about Xen ... KVM uses virtio above.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

I don't see how, the rest of the code comprises of IO/MMIO space & ring processing which is very different in each of the implementations. What is left is the setup and initialization code which obviously depends on the implementation of the driver data structures.

Quoted 14 lines

Quoted 4 lines

And not just that, different HV-vendors can have different features, like say XYZ can come up tomorrow and implement the multiple rings interface so the feature set doesn't remain common and we will have less code to share in the not so distant future.

Multiple rings is really just a multiqueue abstraction. That's fine, but it needs a standard multiqueue control plane.

The desire to one up the competition by adding a new whiz bang feature to which you code a special interface is very common in the storage industry. The counter pressure is that consumers really like these things standardised. That's what the transport class abstraction is all about.

Quoted 5 lines

We also seem to be off on a tangent about hypervisor interfaces. I'm actually more interested in the utility of an SRP abstraction or at least something SAM based. It seems that in your driver you don't quite do the task management functions as SAM requests, but do them over your own protocol abstractions.

Okay, I think I need to take a step back here and understand what actually are you asking for.

1. What do you mean by the "transport class abstraction" ? Do you mean that the way we communicate with the hypervisor needs to be standardized ?

2. Are you saying that we should use the virtio ring mechanism to handle our request and completion rings ?

We can not do that. Our backend expects that each slot on the ring is
in a particular format. Where as vring expects that each slot on the vring is in the vring_desc format.

3. Also, the way we communicate with the hypervisor backend is that the driver writes to our device IO registers in a particular format. The format that we follow is to first write the command on the COMMAND_REGISTER and then write a stream of data words in the DATA_REGISTER, which is a normal device interface.
The reason I make this point is to highlight we are not making any hypercalls instead we communicate with the hypervisor by writing to IO/Memory mapped regions. So from that perspective the driver has no knowledge that its is talking to a software backend (aka device emulation) instead it is very similar to how a driver talks to a silicon device. The backend expects things in a certain way and we cannot really change that interface ( i.e. the ABI shared between Device driver and Device Emulation).

So sharing code with vring or virtio is not something that works well with our backend. The VMware PVSCSI driver is simply a virtual HBA and shouldn't be looked at any differently.

Is their anything else that you are asking us to standardize ?

Quoted 24 lines · Alok Kataria

Quoted 22 lines · James Bottomley

Quoted 5 lines · Alok Kataria

Quoted 2 lines

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

I don't see the sg_ring abstraction that you are talking about. Can you please give me some pointers.

it's in drivers/lguest ... apparently it's vring now and the code is in driver/virtio

Quoted 2 lines

Also regarding Xen and KVM I think they are using the xenbus/vbus interface, which is quite different than what we do here.

Not sure about Xen ... KVM uses virtio above.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

Quoted 4 lines

I don't see how, the rest of the code comprises of IO/MMIO space & ring processing which is very different in each of the implementations. What is left is the setup and initialization code which obviously depends on the implementation of the driver data structures.

Are there benchmarks comparing the two approaches?

Quoted 21 lines

Quoted 15 lines

Quoted 5 lines

And not just that, different HV-vendors can have different features, like say XYZ can come up tomorrow and implement the multiple rings interface so the feature set doesn't remain common and we will have less code to share in the not so distant future.

Multiple rings is really just a multiqueue abstraction. That's fine, but it needs a standard multiqueue control plane.

The desire to one up the competition by adding a new whiz bang feature to which you code a special interface is very common in the storage industry. The counter pressure is that consumers really like these things standardised. That's what the transport class abstraction is all about.

Quoted 5 lines

We also seem to be off on a tangent about hypervisor interfaces. I'm actually more interested in the utility of an SRP abstraction or at least something SAM based. It seems that in your driver you don't quite do the task management functions as SAM requests, but do them over your own protocol abstractions.

Quoted 6 lines

Okay, I think I need to take a step back here and understand what actually are you asking for.

1. What do you mean by the "transport class abstraction" ? Do you mean that the way we communicate with the hypervisor needs to be standardized ?

Not really. Transport classes are designed to share code and provide a uniform control plane when the underlying implementation is different.

Quoted 2 lines

2. Are you saying that we should use the virtio ring mechanism to handle our request and completion rings ?

That's an interesting question. Virtio is currently the standard linux guest<=>hypervisor communication mechanism, but if you have comparative benchmarks showing that virtual hardware emulation is faster, it doesn't need to remain so.

Quoted 4 lines

We can not do that. Our backend expects that each slot on the ring is
in a particular format. Where as vring expects that each slot on the vring is in the vring_desc format.

Your backend is a software server, surely?

Quoted 14 lines

3. Also, the way we communicate with the hypervisor backend is that the driver writes to our device IO registers in a particular format. The format that we follow is to first write the command on the COMMAND_REGISTER and then write a stream of data words in the DATA_REGISTER, which is a normal device interface.
The reason I make this point is to highlight we are not making any hypercalls instead we communicate with the hypervisor by writing to IO/Memory mapped regions. So from that perspective the driver has no knowledge that its is talking to a software backend (aka device emulation) instead it is very similar to how a driver talks to a silicon device. The backend expects things in a certain way and we cannot really change that interface ( i.e. the ABI shared between Device driver and Device Emulation).

Quoted 5 lines

So sharing code with vring or virtio is not something that works well with our backend. The VMware PVSCSI driver is simply a virtual HBA and shouldn't be looked at any differently.

Is their anything else that you are asking us to standardize ?

I'm not really asking you to standardise anything (yet). I was more probing for why you hadn't included any of the SCSI control plane interfaces and what lead you do produce a different design from the current patterns in virtual I/O. I think what I'm hearing is "Because we didn't look at how modern SCSI drivers are constructed" and "Because we didn't look at how virtual I/O is currently done in Linux". That's OK (it's depressingly familiar in drivers),

but now we get to figure out
what, if anything, makes sense from a SCSI control plane to a hypervisor interface and whether this approach to hypervisor interfaces is better or worse than virtio.

James

Quoted 25 lines · James Bottomley

Quoted 24 lines · Alok Kataria

Quoted 22 lines · James Bottomley

Quoted 5 lines · Alok Kataria

Quoted 2 lines

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

I don't see the sg_ring abstraction that you are talking about. Can you please give me some pointers.

it's in drivers/lguest ... apparently it's vring now and the code is in driver/virtio

Quoted 2 lines

Also regarding Xen and KVM I think they are using the xenbus/vbus interface, which is quite different than what we do here.

Not sure about Xen ... KVM uses virtio above.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

Quoted 6 lines

Quoted 4 lines

I don't see how, the rest of the code comprises of IO/MMIO space & ring processing which is very different in each of the implementations. What is left is the setup and initialization code which obviously depends on the implementation of the driver data structures.

Are there benchmarks comparing the two approaches?

Benchmarks comparing what ?

Quoted 34 lines

Quoted 21 lines

Quoted 15 lines

Quoted 5 lines

And not just that, different HV-vendors can have different features, like say XYZ can come up tomorrow and implement the multiple rings interface so the feature set doesn't remain common and we will have less code to share in the not so distant future.

Multiple rings is really just a multiqueue abstraction. That's fine, but it needs a standard multiqueue control plane.

The desire to one up the competition by adding a new whiz bang feature to which you code a special interface is very common in the storage industry. The counter pressure is that consumers really like these things standardised. That's what the transport class abstraction is all about.

Quoted 5 lines

We also seem to be off on a tangent about hypervisor interfaces. I'm actually more interested in the utility of an SRP abstraction or at least something SAM based. It seems that in your driver you don't quite do the task management functions as SAM requests, but do them over your own protocol abstractions.

Quoted 6 lines

Okay, I think I need to take a step back here and understand what actually are you asking for.

1. What do you mean by the "transport class abstraction" ? Do you mean that the way we communicate with the hypervisor needs to be standardized ?

Not really. Transport classes are designed to share code and provide a uniform control plane when the underlying implementation is different.

Quoted 2 lines

2. Are you saying that we should use the virtio ring mechanism to handle our request and completion rings ?

Quoted 4 lines

That's an interesting question. Virtio is currently the standard linux guest<=>hypervisor communication mechanism, but if you have comparative benchmarks showing that virtual hardware emulation is faster, it doesn't need to remain so.

It is a standard that KVM and lguest are using. I don't think it needs any benchamrks to show if a particular approach is faster or not.

VMware has supported paravirtualized devices in backend for more than an year now (may be more, don't quote me on this), and the backend is common across different guest OS's. Virtual hardware emulation helps us give a common interface to different GOS's, whereas virtio binds this heavily to Linux usage. And please note that the backend implementation for our virtual device was done before virtio was integrated in mainline.

Also, from your statements above it seems that you think we are proposing to change the standard communication mechanism (between guest & hypervisor) for Linux. For the record that's not the case, the standard that the Linux based VM's are using does not need to be changed. This pvscsi driver is used for a new SCSI HBA, how does it matter if this SCSI HBA is actually a virtual HBA and implemented by the hypervisor in software.

Quoted 6 lines

Quoted 4 lines

We can not do that. Our backend expects that each slot on the ring is
in a particular format. Where as vring expects that each slot on the vring is in the vring_desc format.

Your backend is a software server, surely?

Yes it is, but the backend is as good as written in stone, as it is being supported by our various products which are out in the market. The pvscsi driver that I proposed for mainlining has also been in existence for some time now and was being used/tested heavily. Earlier we used to distribute it as part of our open-vm-tools project, and it is now that we are proposing to integrate it with mainline.

So if you are hinting that since the backend is software, it can be changed the answer is no. The reason being, their are existing implementations that have that device support and we still want newer guests to make use of that backend implementation.

Quoted 20 lines

Quoted 14 lines

3. Also, the way we communicate with the hypervisor backend is that the driver writes to our device IO registers in a particular format. The format that we follow is to first write the command on the COMMAND_REGISTER and then write a stream of data words in the DATA_REGISTER, which is a normal device interface.
The reason I make this point is to highlight we are not making any hypercalls instead we communicate with the hypervisor by writing to IO/Memory mapped regions. So from that perspective the driver has no knowledge that its is talking to a software backend (aka device emulation) instead it is very similar to how a driver talks to a silicon device. The backend expects things in a certain way and we cannot really change that interface ( i.e. the ABI shared between Device driver and Device Emulation).

Quoted 5 lines

So sharing code with vring or virtio is not something that works well with our backend. The VMware PVSCSI driver is simply a virtual HBA and shouldn't be looked at any differently.

Is their anything else that you are asking us to standardize ?

Quoted 7 lines

I'm not really asking you to standardise anything (yet). I was more probing for why you hadn't included any of the SCSI control plane interfaces and what lead you do produce a different design from the current patterns in virtual I/O. I think what I'm hearing is "Because we didn't look at how modern SCSI drivers are constructed" and "Because we didn't look at how virtual I/O is currently done in Linux". That's OK (it's depressingly familiar in drivers),

I am sorry that's not the case, the reason we have different design as I have mentioned above is because we want a generic mechanism which works for all/most of the GOS's out their and doesn't need to be specific to Linux.

Quoted 4 lines

but now we get to figure out
what, if anything, makes sense from a SCSI control plane to a hypervisor interface and whether this approach to hypervisor interfaces is better or worse than virtio.

I guess these points are answered above. Let me know if their is still something amiss.

Thanks,
Alok

Quoted 4 lines

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines

Quoted 8 lines

I'm not really asking you to standardise anything (yet). I was more probing for why you hadn't included any of the SCSI control plane interfaces and what lead you do produce a different design from the current patterns in virtual I/O. I think what I'm hearing is "Because we didn't look at how modern SCSI drivers are constructed" and "Because we didn't look at how virtual I/O is currently done in Linux". That's OK (it's depressingly familiar in drivers),

Quoted 4 lines

I am sorry that's not the case, the reason we have different design as I have mentioned above is because we want a generic mechanism which works for all/most of the GOS's out their and doesn't need to be specific to Linux.

Slightly confused now ... you're saying you did look at the transport class and virtio? But you chose not to do a virtio like interface (for reasons which I'm still not clear on) ...

Quoted 8 lines · James Bottomley

Quoted 8 lines

Quoted 8 lines

I'm not really asking you to standardise anything (yet). I was more probing for why you hadn't included any of the SCSI control plane interfaces and what lead you do produce a different design from the current patterns in virtual I/O. I think what I'm hearing is "Because we didn't look at how modern SCSI drivers are constructed" and "Because we didn't look at how virtual I/O is currently done in Linux". That's OK (it's depressingly familiar in drivers),

Quoted 9 lines

Quoted 5 lines

I am sorry that's not the case, the reason we have different design as I have mentioned above is because we want a generic mechanism which works for all/most of the GOS's out their and doesn't need to be specific to Linux.

Slightly confused now ... you're saying you did look at the transport class and virtio? But you chose not to do a virtio like interface (for reasons which I'm still not clear on) ...

Virtio is Linux-specific and is not available on older kernels which our hypervisor/PVSCSI combination does support. Even if we were to use virtio-like schema in the hypervisor code we would have to re-implement much of the virtio code for kernels earlier than those shipped in '07 and do the same for other operating systems for no apparent benefit. The PCI device abstraction is self-contained and works well on Windows, Linux and other guest operating systems and so it was chosen.

Quoted 19 lines · Dmitry Torokhov

Quoted 8 lines · James Bottomley

Quoted 8 lines

Quoted 8 lines

I'm not really asking you to standardise anything (yet). I was more probing for why you hadn't included any of the SCSI control plane interfaces and what lead you do produce a different design from the current patterns in virtual I/O. I think what I'm hearing is "Because we didn't look at how modern SCSI drivers are constructed" and "Because we didn't look at how virtual I/O is currently done in Linux". That's OK (it's depressingly familiar in drivers),

Quoted 9 lines

Quoted 5 lines

I am sorry that's not the case, the reason we have different design as I have mentioned above is because we want a generic mechanism which works for all/most of the GOS's out their and doesn't need to be specific to Linux.

Slightly confused now ... you're saying you did look at the transport class and virtio? But you chose not to do a virtio like interface (for reasons which I'm still not clear on) ...

Quoted 9 lines

Virtio is Linux-specific and is not available on older kernels which our hypervisor/PVSCSI combination does support. Even if we were to use virtio-like schema in the hypervisor code we would have to re-implement much of the virtio code for kernels earlier than those shipped in '07 and do the same for other operating systems for no apparent benefit. The PCI device abstraction is self-contained and works well on Windows, Linux and other guest operating systems and so it was chosen.

Several arguments have a history of never winning when you try to get a new bit of code in linux.

Number one in the bad justifications is that your design is good because it avoids being "linux specific" closely followed by needing to backport :-)

Quoted 26 lines · Alok Kataria

Quoted 25 lines · James Bottomley

Quoted 24 lines · Alok Kataria

Quoted 22 lines · James Bottomley

Quoted 5 lines · Alok Kataria

Quoted 2 lines

lguest uses the sg_ring abstraction. Xen and KVM were certainly looking at this too.

I don't see the sg_ring abstraction that you are talking about. Can you please give me some pointers.

it's in drivers/lguest ... apparently it's vring now and the code is in driver/virtio

Quoted 2 lines

Also regarding Xen and KVM I think they are using the xenbus/vbus interface, which is quite different than what we do here.

Not sure about Xen ... KVM uses virtio above.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

And anyways how large is the DMA code that we are worrying about here ? Only about 300-400 LOC ? I don't think we might want to over-design for such small gains.

So even if you have different DMA code, the remaining thousand or so lines would be in common. That's a worthwhile improvement.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

I don't see how, the rest of the code comprises of IO/MMIO space & ring processing which is very different in each of the implementations. What is left is the setup and initialization code which obviously depends on the implementation of the driver data structures.

Are there benchmarks comparing the two approaches?

Benchmarks comparing what ?

Your approach versus virtio.

Quoted 34 lines

Quoted 34 lines

Quoted 21 lines

Quoted 15 lines

Quoted 5 lines

And not just that, different HV-vendors can have different features, like say XYZ can come up tomorrow and implement the multiple rings interface so the feature set doesn't remain common and we will have less code to share in the not so distant future.

Multiple rings is really just a multiqueue abstraction. That's fine, but it needs a standard multiqueue control plane.

The desire to one up the competition by adding a new whiz bang feature to which you code a special interface is very common in the storage industry. The counter pressure is that consumers really like these things standardised. That's what the transport class abstraction is all about.

Quoted 5 lines

We also seem to be off on a tangent about hypervisor interfaces. I'm actually more interested in the utility of an SRP abstraction or at least something SAM based. It seems that in your driver you don't quite do the task management functions as SAM requests, but do them over your own protocol abstractions.

Quoted 6 lines

Okay, I think I need to take a step back here and understand what actually are you asking for.

1. What do you mean by the "transport class abstraction" ? Do you mean that the way we communicate with the hypervisor needs to be standardized ?

Not really. Transport classes are designed to share code and provide a uniform control plane when the underlying implementation is different.

Quoted 2 lines

2. Are you saying that we should use the virtio ring mechanism to handle our request and completion rings ?

Quoted 7 lines

Quoted 4 lines

That's an interesting question. Virtio is currently the standard linux guest<=>hypervisor communication mechanism, but if you have comparative benchmarks showing that virtual hardware emulation is faster, it doesn't need to remain so.

It is a standard that KVM and lguest are using. I don't think it needs any benchamrks to show if a particular approach is faster or not.

It's a useful datapoint especially since the whole object of paravirtualised drivers is supposed to be speed vs full hardware emulation.

Quoted 7 lines

VMware has supported paravirtualized devices in backend for more than an year now (may be more, don't quote me on this), and the backend is common across different guest OS's. Virtual hardware emulation helps us give a common interface to different GOS's, whereas virtio binds this heavily to Linux usage. And please note that the backend implementation for our virtual device was done before virtio was integrated in mainline.

Virtio mainline integration dates from October 2007. The mailing list discussions obviously predate that by several months.

Quoted 55 lines

Also, from your statements above it seems that you think we are proposing to change the standard communication mechanism (between guest & hypervisor) for Linux. For the record that's not the case, the standard that the Linux based VM's are using does not need to be changed. This pvscsi driver is used for a new SCSI HBA, how does it matter if this SCSI HBA is actually a virtual HBA and implemented by the hypervisor in software.

Quoted 6 lines

Quoted 4 lines

We can not do that. Our backend expects that each slot on the ring is
in a particular format. Where as vring expects that each slot on the vring is in the vring_desc format.

Your backend is a software server, surely?

Yes it is, but the backend is as good as written in stone, as it is being supported by our various products which are out in the market. The pvscsi driver that I proposed for mainlining has also been in existence for some time now and was being used/tested heavily. Earlier we used to distribute it as part of our open-vm-tools project, and it is now that we are proposing to integrate it with mainline.

So if you are hinting that since the backend is software, it can be changed the answer is no. The reason being, their are existing implementations that have that device support and we still want newer guests to make use of that backend implementation.

Quoted 27 lines

Quoted 14 lines

3. Also, the way we communicate with the hypervisor backend is that the driver writes to our device IO registers in a particular format. The format that we follow is to first write the command on the COMMAND_REGISTER and then write a stream of data words in the DATA_REGISTER, which is a normal device interface.
The reason I make this point is to highlight we are not making any hypercalls instead we communicate with the hypervisor by writing to IO/Memory mapped regions. So from that perspective the driver has no knowledge that its is talking to a software backend (aka device emulation) instead it is very similar to how a driver talks to a silicon device. The backend expects things in a certain way and we cannot really change that interface ( i.e. the ABI shared between Device driver and Device Emulation).

Quoted 5 lines

So sharing code with vring or virtio is not something that works well with our backend. The VMware PVSCSI driver is simply a virtual HBA and shouldn't be looked at any differently.

Is their anything else that you are asking us to standardize ?

I'm not really asking you to standardise anything (yet). I was more probing for why you hadn't included any of the SCSI control plane interfaces and what lead you do produce a different design from the current patterns in virtual I/O. I think what I'm hearing is "Because we didn't look at how modern SCSI drivers are constructed" and "Because we didn't look at how virtual I/O is currently done in Linux". That's OK (it's depressingly familiar in drivers),

Quoted 4 lines

I am sorry that's not the case, the reason we have different design as I have mentioned above is because we want a generic mechanism which works for all/most of the GOS's out their and doesn't need to be specific to Linux.

Slightly confused now ... you're saying you did look at the transport class and virtio? But you chose not to do a virtio like interface (for reasons which I'm still not clear on) ...

I didn't manage to extract
anything about why no transport class from the foregoing.

James

Quoted 20 lines

Quoted 4 lines

but now we get to figure out
what, if anything, makes sense from a SCSI control plane to a hypervisor interface and whether this approach to hypervisor interfaces is better or worse than virtio.

I guess these points are answered above. Let me know if their is still something amiss.

Thanks,
Alok

Quoted 4 lines

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nobody said you had to have the exact same driver for every hypervisor. What people are suggesting is that we look at commonalities in the interfaces both from a control plane point of view (transport class) and from a code sharing point of view (libscsivirt). However, all the hypervisor interfaces I've seen are basically DMA rings ...

Quoted 5 lines

Nobody said you had to have the exact same driver for every hypervisor. What people are suggesting is that we look at commonalities in the interfaces both from a control plane point of view (transport class) and from a code sharing point of view (libscsivirt). However, all the hypervisor interfaces I've seen are basically DMA rings ...

I don't think that's anything special about hypervisors though -- pretty much all modern device interfaces are basically DMA rings, aren't they? I'm definitely in favor of common code to handle commonality but on the other hand I don't see what's so special about virtual devices vs. real HW devices. One the one side we have VMware's closed hypervisor code and on the other side we have vendor XYZ's closed RTL and firmware code.

Quoted 6 lines · Roland Dreier

Quoted 5 lines

Nobody said you had to have the exact same driver for every hypervisor. What people are suggesting is that we look at commonalities in the interfaces both from a control plane point of view (transport class) and from a code sharing point of view (libscsivirt). However, all the hypervisor interfaces I've seen are basically DMA rings ...

Quoted 6 lines

I don't think that's anything special about hypervisors though -- pretty much all modern device interfaces are basically DMA rings, aren't they? I'm definitely in favor of common code to handle commonality but on the other hand I don't see what's so special about virtual devices vs. real HW devices. One the one side we have VMware's closed hypervisor code and on the other side we have vendor XYZ's closed RTL and firmware code.

But the main difference between actual hardware and hypervisors is the fact that to set up a DMA transfer you have to poke registers on the card, set up a mailbox and manage queues of commands to the card. For a hypervisor, sending a DMA transaction is a hypercall.

Now for most physical drivers, take for example FCP ones, we have a common control plane interface (fc transport class), we're evolving a frame handling library (libfc) so all the drivers really have are specific codes to bit bang the hardware. Some of the libfc handling is actually done in intelligent offload firmware on the HBAs, so some will use more or less of the libfc handling (same is true for SAS and libsas). When there's no actual hardware to be bit banged, and no real firmware offload, it does make one wonder what would be left unique to the driver.

James

Also note that, the way all these things are implemented for each of the hypervisor devices will differ and getting every hv-vendor to agree on a common set of things is not very attractive proposition ( atleast, this I can say from my past experiences).

Quoted 4 lines · Alok Kataria

Also note that, the way all these things are implemented for each of the hypervisor devices will differ and getting every hv-vendor to agree on a common set of things is not very attractive proposition ( atleast, this I can say from my past experiences).

virtio is shared by three or four hypervisors depending on how you count it, with others under development.

 > - Reuse the ib_ipoib kernel module to provide an IP stack on top of > the new RDMA driver instead of having to maintain a separate network > driver for this hardware (ibmveth).

I don't think this really makes sense, because IPoIB is not really handling ethernet (it is a different L2 ethernet encapsulation), and I think the commonality with ibmveth is going to be minimal.

I'm not really sure we should be trying to force drivers to share just because they are paravirtualized -- if there is real commonality, then sure put it in common code, but different hypervisors are probably as different as different hardware.