[PATCH PCIBACK]

[PATCH PCIBACK]

[Konrad Rzeszutek Wilk]

Hey Jeremy,

Attached are six patches for the xen/dom0/pciback branch. They remove
the old xenbus_walk functionality and replace it with a registration
(and unregistration plus a find function) mechanism.

Please ignore the previous posting of the patches
(http://lists.xensource.com/archives/html/xen-devel/2009-12/msg00422.html).

Thank you.
Konrad

[PATCH 1/6] Revert "In xen_create_msi_irq, extract the domain id of the MSI device."

[Konrad Rzeszutek Wilk]

This reverts commit 661692690f8cf5a5ceccec1f4d221ec238197448.

Conflicts:

	drivers/xen/events.c

I did not remove the domid attribute from the structs as they
are going to be used in the subsequent patches.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |   44 --------------------------------------------
 1 files changed, 0 insertions(+), 44 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index be24a25..2ede0c8 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -45,7 +45,6 @@
 #include <xen/interface/xen.h>
 #include <xen/interface/event_channel.h>
 
-#include <xen/xenbus.h>
 #include "../pci/msi.h"
 
 /*
@@ -613,43 +612,6 @@ out:
 	spin_unlock(&irq_mapping_update_lock);
 	return rc;
 }
-#define XS_PCI_SEARCH "/local/domain/0/backend/pci/"
-#define XS_PCI_SEARCH_LEN 28
-
-static int get_domid_for_dev(char *nodename, char *path, char *val, void *data)
-{
-	struct pci_dev *dev = (struct pci_dev *)data;
-
-	/* We are looking for:
-	 * CB inv[/local/domain/0/backend/pci/3/0/dev-1]=0000:00:14.5 */
-
-	if (!dev || !pci_name(dev))
-		return -ENODEV;
-
-	if (strncmp(nodename, XS_PCI_SEARCH, XS_PCI_SEARCH_LEN) == 0)
-	{
-		if (strncmp(path, "dev", 3) == 0) {
-			char *name = NULL;
-			unsigned int domid;
-
-			/* The val contains: 0000:00:14.5 */
-			if (strcmp(val, pci_name(dev)) != 0)
-				return 0;
-
-			/* From here on, extract the Domain ID */
- 			name = strstr(nodename, "pci/");
-			if (!name)
-				return 0;
-
-			if (sscanf(name, "pci/%u", &domid) != 1)
-				return 0;
-
-			dev_dbg(&dev->dev,"Assigned to domain %u.\n", domid);
-			return domid;
-		}
-	}
-	return 0;
-}
 
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 {
@@ -660,12 +622,6 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	int pos;
 	u32 table_offset, bir;
 
-	/* Walk the XenStore keys to see if the device is assigned to any
-	 * domain. If so, extract the domain id. */
-	domid = rc = xenbus_walk( "/local/domain/0", get_domid_for_dev, dev);
-	if (rc < 0)
-		domid = DOMID_SELF;
-
 	memset(&map_irq, 0, sizeof(map_irq));
 	map_irq.domid = domid;
 	map_irq.type = MAP_PIRQ_TYPE_MSI;
-- 
1.6.2.5

[PATCH 2/6] Provide a mechanism to register domain owner of a PCI device.

[Konrad Rzeszutek Wilk]

. and also to find the domain owner based on the PCI device and
to unregister a domain owner of a PCI device.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/xen/pci.h |   16 +++++++++
 arch/x86/xen/pci.c             |   73 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index cb84abe..f4312d5 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -7,6 +7,11 @@ int xen_create_msi_irq(struct pci_dev *dev,
 			struct msi_desc *msidesc,
 			int type);
 int xen_destroy_irq(int irq);
+
+int find_device_owner(struct pci_dev *dev);
+int register_device_owner(struct pci_dev *dev, domid_t domain);
+int unregister_device_owner(struct pci_dev *dev);
+
 #else
 static inline int xen_register_gsi(u32 gsi, int triggering, int polarity)
 {
@@ -23,6 +28,17 @@ static inline int xen_destroy_irq(int irq)
 {
 	return -1;
 }
+
+static inline int find_device_owner(struct pci_dev *dev) { return -1; }
+static inline int register_device_owner(struct pci_dev *dev, domid_t domain)
+{
+ 	return -1;
+}
+static inline int unregister_device_owner(struct pci_dev *dev)
+{
+ 	return -1;
+}
+
 #endif
 
 #if defined(CONFIG_PCI_MSI) && defined(CONFIG_XEN_DOM0_PCI)
diff --git a/arch/x86/xen/pci.c b/arch/x86/xen/pci.c
index 44d91ad..4fa734c 100644
--- a/arch/x86/xen/pci.c
+++ b/arch/x86/xen/pci.c
@@ -2,6 +2,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/msi.h>
+#include <linux/slab.h>
 
 #include <asm/mpspec.h>
 #include <asm/io_apic.h>
@@ -109,3 +110,75 @@ error:
 	return ret;
 }
 #endif
+
+struct device_owner {
+	domid_t domain;
+	struct pci_dev *dev;
+	struct list_head list;
+};
+
+static DEFINE_SPINLOCK(dev_domain_list_spinlock);
+static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
+
+struct device_owner *find_device(struct pci_dev *dev)
+{
+
+	struct device_owner *owner;
+
+	list_for_each_entry(owner, &dev_domain_list, list) {
+		if (owner->dev == dev)
+			return owner;
+	}
+	return NULL;
+}
+
+int find_device_owner(struct pci_dev *dev)
+{
+
+	struct device_owner *owner = NULL;
+
+	owner = find_device(dev);
+	if (!owner)
+		return -ENODEV;
+
+	return owner->domain;
+}
+EXPORT_SYMBOL(find_device_owner);
+
+int register_device_owner(struct pci_dev *dev, domid_t domain)
+{
+	struct device_owner *owner;
+	unsigned long flags;
+
+	if (find_device(dev))
+		return -EEXIST;
+
+	owner = kzalloc(sizeof(struct device_owner), GFP_KERNEL);
+	owner->domain = domain;
+	owner->dev = dev;
+
+	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
+	list_add_tail(&owner->list, &dev_domain_list);
+	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(register_device_owner);
+
+int unregister_device_owner(struct pci_dev *dev)
+{
+	struct device_owner *owner = NULL;
+	unsigned long flags;
+
+	owner = find_device(dev);
+	if (!owner)
+		return -ENODEV;
+
+	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
+	list_del(&owner->list);
+	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
+
+	kfree(owner);
+	return 0;
+}
+EXPORT_SYMBOL(unregister_device_owner);
-- 
1.6.2.5

[PATCH 3/6] [events] Check if the PCI device is owner by a domain different than DOMID_SELF.

[Konrad Rzeszutek Wilk]

We check if there is a domain owner for the PCI device. In case of failure
(meaning no domain has registered for this device) we make
DOMID_SELF the owner.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 2ede0c8..18a1643 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -39,6 +39,7 @@
 #include <asm/sync_bitops.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
+#include <asm/xen/pci.h>
 
 #include <xen/xen-ops.h>
 #include <xen/events.h>
@@ -618,10 +619,14 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	int irq = 0;
 	struct physdev_map_pirq map_irq;
 	int rc;
-	domid_t domid = DOMID_SELF;
+	domid_t domid;
 	int pos;
 	u32 table_offset, bir;
 
+	domid = rc = find_device_owner(dev);
+	if (rc < 0)
+		domid = DOMID_SELF;
+	
 	memset(&map_irq, 0, sizeof(map_irq));
 	map_irq.domid = domid;
 	map_irq.type = MAP_PIRQ_TYPE_MSI;
-- 
1.6.2.5

[PATCH 4/6] [pciback] Register the owner (domain) of the PCI device.

[Konrad Rzeszutek Wilk]

When the front-end and back-end start negotiating we register
the domain that will use the PCI device. Furthermore during shutdown
of guest or unbinding of the PCI device (and unloading of module)
from pciback we unregister the domain owner.
 Please enter the commit message for your changes. Lines starting

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pciback/pci_stub.c |    3 +++
 drivers/xen/pciback/xenbus.c   |   12 ++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
index c65c7c1..00018c1 100644
--- a/drivers/xen/pciback/pci_stub.c
+++ b/drivers/xen/pciback/pci_stub.c
@@ -14,6 +14,7 @@
 #include <linux/wait.h>
 #include <asm/atomic.h>
 #include <xen/events.h>
+#include <asm/xen/pci.h>
 #include "pciback.h"
 #include "conf_space.h"
 #include "conf_space_quirks.h"
@@ -87,6 +88,8 @@ static void pcistub_device_release(struct kref *kref)
 
 	dev_dbg(&psdev->dev->dev, "pcistub_device_release\n");
 
+	unregister_device_owner(psdev->dev);
+
 	/* Clean-up the device */
 	pciback_reset_device(psdev->dev);
 	pciback_config_free_dyn_fields(psdev->dev);
diff --git a/drivers/xen/pciback/xenbus.c b/drivers/xen/pciback/xenbus.c
index efec585..e33a688 100644
--- a/drivers/xen/pciback/xenbus.c
+++ b/drivers/xen/pciback/xenbus.c
@@ -10,6 +10,7 @@
 #include <linux/workqueue.h>
 #include <xen/xenbus.h>
 #include <xen/events.h>
+#include <asm/xen/pci.h>
 #include <linux/workqueue.h>
 #include "pciback.h"
 
@@ -221,6 +222,14 @@ static int pciback_export_device(struct pciback_device *pdev,
 	if (err)
 		goto out;
 
+	dev_info(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
+	if (register_device_owner(dev, pdev->xdev->otherend_id) != 0) {
+		dev_err(&dev->dev, "device has been assigned to another " \
+			"domain! Over-writting the ownership, but beware.\n");
+		unregister_device_owner(dev);
+		register_device_owner(dev, pdev->xdev->otherend_id);
+	}
+
 	/* TODO: It'd be nice to export a bridge and have all of its children
 	 * get exported with it. This may be best done in xend (which will
 	 * have to calculate resource usage anyway) but we probably want to
@@ -251,6 +260,9 @@ static int pciback_remove_device(struct pciback_device *pdev,
 		goto out;
 	}
 
+	dev_dbg(&dev->dev, "unregistering for %d\n", pdev->xdev->otherend_id);
+	unregister_device_owner(dev);
+
 	pciback_release_pci_dev(pdev, dev);
 
 out:
-- 
1.6.2.5

[PATCH 5/6] [pciback] Remove the vestiges of CONFIG_PCI_GUESTDEV.

[Konrad Rzeszutek Wilk]

The code 'pci_is_guestdev' is not yet back-ported. Will revisit
this once that has been completed.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pciback/pci_stub.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
index 00018c1..e9e8e68 100644
--- a/drivers/xen/pciback/pci_stub.c
+++ b/drivers/xen/pciback/pci_stub.c
@@ -430,16 +430,6 @@ static int __devinit pcistub_probe(struct pci_dev *dev,
 
 		dev_info(&dev->dev, "seizing device\n");
 		err = pcistub_seize(dev);
-#ifdef CONFIG_PCI_GUESTDEV
-	} else if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
-		if (!pci_is_guestdev(dev)) {
-			err = -ENODEV;
-			goto out;
-		}
-
-		dev_info(&dev->dev, "seizing device\n");
-		err = pcistub_seize(dev);
-#endif /* CONFIG_PCI_GUESTDEV */
 	} else
 		/* Didn't find the device */
 		err = -ENODEV;
-- 
1.6.2.5

[PATCH 6/6] [pciback] Remove deprecated routine to find domain owner of PCI device.

[Konrad Rzeszutek Wilk]

In linux-2.6.18.hg tree the mechanism to find the domain owner was
for the MSI driver (msi-xen.c) to call in this function to retrieve
the domain number. This is not the way anymore.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pciback/pci_stub.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
index e9e8e68..a94d350 100644
--- a/drivers/xen/pciback/pci_stub.c
+++ b/drivers/xen/pciback/pci_stub.c
@@ -1157,22 +1157,6 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf)
 
 DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show, permissive_add);
 
-#ifdef CONFIG_PCI_MSI
-
-int pciback_get_owner(struct pci_dev *dev)
-{
-	struct pcistub_device *psdev;
-
-	psdev = pcistub_device_find(pci_domain_nr(dev->bus), dev->bus->number,
-			PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
-
-	if (!psdev || !psdev->pdev)
-		return -1;
-
-	return psdev->pdev->xdev->otherend_id;
-}
-#endif
-
 static void pcistub_exit(void)
 {
 	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
-- 
1.6.2.5

Re: [PATCH 2/6] Provide a mechanism to register domain owner of a PCI device.

[Jeremy Fitzhardinge]

On 12/09/09 12:51, Konrad Rzeszutek Wilk wrote:
> . and also to find the domain owner based on the PCI device and
> to unregister a domain owner of a PCI device.
>
> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> ---
>   arch/x86/include/asm/xen/pci.h |   16 +++++++++
>   arch/x86/xen/pci.c             |   73 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 89 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
> index cb84abe..f4312d5 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -7,6 +7,11 @@ int xen_create_msi_irq(struct pci_dev *dev,
>   			struct msi_desc *msidesc,
>   			int type);
>   int xen_destroy_irq(int irq);
> +
> +int find_device_owner(struct pci_dev *dev);
> +int register_device_owner(struct pci_dev *dev, domid_t domain);
> +int unregister_device_owner(struct pci_dev *dev);
>    

Could you give these less generic names?

> +
>   #else
>   static inline int xen_register_gsi(u32 gsi, int triggering, int polarity)
>   {
> @@ -23,6 +28,17 @@ static inline int xen_destroy_irq(int irq)
>   {
>   	return -1;
>   }
> +
> +static inline int find_device_owner(struct pci_dev *dev) { return -1; }
> +static inline int register_device_owner(struct pci_dev *dev, domid_t domain)
> +{
> + 	return -1;
> +}
> +static inline int unregister_device_owner(struct pci_dev *dev)
> +{
> + 	return -1;
> +}
>    

Make the formatting consistent here.

> +
>   #endif
>
>   #if defined(CONFIG_PCI_MSI)&&  defined(CONFIG_XEN_DOM0_PCI)
> diff --git a/arch/x86/xen/pci.c b/arch/x86/xen/pci.c
> index 44d91ad..4fa734c 100644
> --- a/arch/x86/xen/pci.c
> +++ b/arch/x86/xen/pci.c
> @@ -2,6 +2,7 @@
>   #include<linux/acpi.h>
>   #include<linux/pci.h>
>   #include<linux/msi.h>
> +#include<linux/slab.h>
>
>   #include<asm/mpspec.h>
>   #include<asm/io_apic.h>
> @@ -109,3 +110,75 @@ error:
>   	return ret;
>   }
>   #endif
> +
> +struct device_owner {
>    

Again, a bit generic (the name should at least convey we're talking 
about owner domains).


> +	domid_t domain;
> +	struct pci_dev *dev;
> +	struct list_head list;
> +};
> +
> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> +
> +struct device_owner *find_device(struct pci_dev *dev)
> +{
> +
> +	struct device_owner *owner;
>    
Stray blank line.
> +
> +	list_for_each_entry(owner,&dev_domain_list, list) {
> +		if (owner->dev == dev)
> +			return owner;
> +	}
> +	return NULL;
> +}
> +
> +int find_device_owner(struct pci_dev *dev)
> +{
> +
> +	struct device_owner *owner = NULL;
>    
Again.
> +
> +	owner = find_device(dev);
> +	if (!owner)
> +		return -ENODEV;
> +
> +	return owner->domain;
> +}
> +EXPORT_SYMBOL(find_device_owner);
> +
> +int register_device_owner(struct pci_dev *dev, domid_t domain)
> +{
> +	struct device_owner *owner;
> +	unsigned long flags;
> +
> +	if (find_device(dev))
>    
Should this be under the lock?
> +		return -EEXIST;
> +
> +	owner = kzalloc(sizeof(struct device_owner), GFP_KERNEL);
>    
Check for NULL.
> +	owner->domain = domain;
> +	owner->dev = dev;
> +
> +	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
> +	list_add_tail(&owner->list,&dev_domain_list);
> +	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(register_device_owner);
> +
> +int unregister_device_owner(struct pci_dev *dev)
> +{
> +	struct device_owner *owner = NULL;
> +	unsigned long flags;
> +
> +	owner = find_device(dev);
>    
Shouldn't this be under the lock too?
> +	if (!owner)
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
> +	list_del(&owner->list);
> +	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
> +
> +	kfree(owner);
> +	return 0;
> +}
> +EXPORT_SYMBOL(unregister_device_owner);
>    

Thanks,
     J

Re: [PATCH 4/6] [pciback] Register the owner (domain) of the PCI device.

[Jeremy Fitzhardinge]

On 12/09/09 12:51, Konrad Rzeszutek Wilk wrote:
> When the front-end and back-end start negotiating we register
> the domain that will use the PCI device. Furthermore during shutdown
> of guest or unbinding of the PCI device (and unloading of module)
> from pciback we unregister the domain owner.
>   Please enter the commit message for your changes. Lines starting
>    
Looks like a stray.

> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> ---
>   drivers/xen/pciback/pci_stub.c |    3 +++
>   drivers/xen/pciback/xenbus.c   |   12 ++++++++++++
>   2 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
> index c65c7c1..00018c1 100644
> --- a/drivers/xen/pciback/pci_stub.c
> +++ b/drivers/xen/pciback/pci_stub.c
> @@ -14,6 +14,7 @@
>   #include<linux/wait.h>
>   #include<asm/atomic.h>
>   #include<xen/events.h>
> +#include<asm/xen/pci.h>
>   #include "pciback.h"
>   #include "conf_space.h"
>   #include "conf_space_quirks.h"
> @@ -87,6 +88,8 @@ static void pcistub_device_release(struct kref *kref)
>
>   	dev_dbg(&psdev->dev->dev, "pcistub_device_release\n");
>
> +	unregister_device_owner(psdev->dev);
> +
>   	/* Clean-up the device */
>   	pciback_reset_device(psdev->dev);
>   	pciback_config_free_dyn_fields(psdev->dev);
> diff --git a/drivers/xen/pciback/xenbus.c b/drivers/xen/pciback/xenbus.c
> index efec585..e33a688 100644
> --- a/drivers/xen/pciback/xenbus.c
> +++ b/drivers/xen/pciback/xenbus.c
> @@ -10,6 +10,7 @@
>   #include<linux/workqueue.h>
>   #include<xen/xenbus.h>
>   #include<xen/events.h>
> +#include<asm/xen/pci.h>
>   #include<linux/workqueue.h>
>   #include "pciback.h"
>
> @@ -221,6 +222,14 @@ static int pciback_export_device(struct pciback_device *pdev,
>   	if (err)
>   		goto out;
>
> +	dev_info(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
>    
_dbg, to match unregistering message?

> +	if (register_device_owner(dev, pdev->xdev->otherend_id) != 0) {
> +		dev_err(&dev->dev, "device has been assigned to another " \
> +			"domain! Over-writting the ownership, but beware.\n");
> +		unregister_device_owner(dev);
> +		register_device_owner(dev, pdev->xdev->otherend_id);
> +	}
> +
>   	/* TODO: It'd be nice to export a bridge and have all of its children
>   	 * get exported with it. This may be best done in xend (which will
>   	 * have to calculate resource usage anyway) but we probably want to
> @@ -251,6 +260,9 @@ static int pciback_remove_device(struct pciback_device *pdev,
>   		goto out;
>   	}
>
> +	dev_dbg(&dev->dev, "unregistering for %d\n", pdev->xdev->otherend_id);
> +	unregister_device_owner(dev);
> +
>   	pciback_release_pci_dev(pdev, dev);
>
>   out:
>    

Re: [PATCH PCIBACK]

[Jeremy Fitzhardinge]

On 12/09/09 12:51, Konrad Rzeszutek Wilk wrote:
> Hey Jeremy,
>
> Attached are six patches for the xen/dom0/pciback branch. They remove
> the old xenbus_walk functionality and replace it with a registration
> (and unregistration plus a find function) mechanism.
>    

Thanks, it looks good overall.  I have few comments though (replies to 
patches).

     J

[PATCH PCIBACK]

[Konrad Rzeszutek Wilk]

On Wed, Dec 09, 2009 at 01:14:06PM -0800, Jeremy Fitzhardinge wrote:
> On 12/09/09 12:51, Konrad Rzeszutek Wilk wrote:
>> Hey Jeremy,
>>
>> Attached are six patches for the xen/dom0/pciback branch. They remove
>> the old xenbus_walk functionality and replace it with a registration
>> (and unregistration plus a find function) mechanism.
>>    
>
> Thanks, it looks good overall.  I have few comments though (replies to  
> patches).

Thanks for the review. Attached are the redone patches.

[PATCH 1/6] Revert "In xen_create_msi_irq, extract the domain id of the MSI device."

[Konrad Rzeszutek Wilk]

This reverts commit 661692690f8cf5a5ceccec1f4d221ec238197448.

Conflicts:

	drivers/xen/events.c

I did not remove the domid attribute from the structs as they
are going to be used in the subsequent patches.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |   44 --------------------------------------------
 1 files changed, 0 insertions(+), 44 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index be24a25..2ede0c8 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -45,7 +45,6 @@
 #include <xen/interface/xen.h>
 #include <xen/interface/event_channel.h>
 
-#include <xen/xenbus.h>
 #include "../pci/msi.h"
 
 /*
@@ -613,43 +612,6 @@ out:
 	spin_unlock(&irq_mapping_update_lock);
 	return rc;
 }
-#define XS_PCI_SEARCH "/local/domain/0/backend/pci/"
-#define XS_PCI_SEARCH_LEN 28
-
-static int get_domid_for_dev(char *nodename, char *path, char *val, void *data)
-{
-	struct pci_dev *dev = (struct pci_dev *)data;
-
-	/* We are looking for:
-	 * CB inv[/local/domain/0/backend/pci/3/0/dev-1]=0000:00:14.5 */
-
-	if (!dev || !pci_name(dev))
-		return -ENODEV;
-
-	if (strncmp(nodename, XS_PCI_SEARCH, XS_PCI_SEARCH_LEN) == 0)
-	{
-		if (strncmp(path, "dev", 3) == 0) {
-			char *name = NULL;
-			unsigned int domid;
-
-			/* The val contains: 0000:00:14.5 */
-			if (strcmp(val, pci_name(dev)) != 0)
-				return 0;
-
-			/* From here on, extract the Domain ID */
- 			name = strstr(nodename, "pci/");
-			if (!name)
-				return 0;
-
-			if (sscanf(name, "pci/%u", &domid) != 1)
-				return 0;
-
-			dev_dbg(&dev->dev,"Assigned to domain %u.\n", domid);
-			return domid;
-		}
-	}
-	return 0;
-}
 
 int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 {
@@ -660,12 +622,6 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	int pos;
 	u32 table_offset, bir;
 
-	/* Walk the XenStore keys to see if the device is assigned to any
-	 * domain. If so, extract the domain id. */
-	domid = rc = xenbus_walk( "/local/domain/0", get_domid_for_dev, dev);
-	if (rc < 0)
-		domid = DOMID_SELF;
-
 	memset(&map_irq, 0, sizeof(map_irq));
 	map_irq.domid = domid;
 	map_irq.type = MAP_PIRQ_TYPE_MSI;
-- 
1.6.2.2

[PATCH 2/6] Provide a mechanism to register domain owner of a PCI device.

[Konrad Rzeszutek Wilk]

. and also to find the domain owner based on the PCI device and
to unregister a domain owner of a PCI device.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/xen/pci.h |   18 +++++++++
 arch/x86/xen/pci.c             |   81 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index cb84abe..d335c90 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -7,6 +7,11 @@ int xen_create_msi_irq(struct pci_dev *dev,
 			struct msi_desc *msidesc,
 			int type);
 int xen_destroy_irq(int irq);
+
+int xen_find_device_domain_owner(struct pci_dev *dev);
+int xen_register_device_domain_owner(struct pci_dev *dev, domid_t domain);
+int xen_unregister_device_domain_owner(struct pci_dev *dev);
+
 #else
 static inline int xen_register_gsi(u32 gsi, int triggering, int polarity)
 {
@@ -23,6 +28,19 @@ static inline int xen_destroy_irq(int irq)
 {
 	return -1;
 }
+static inline int xen_find_device_domain_owner(struct pci_dev *dev)
+{
+	return -1;
+}
+static inline int xen_register_device_domain_owner(struct pci_dev *dev,
+						   domid_t domain)
+{
+ 	return -1;
+}
+static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
+{
+ 	return -1;
+}
 #endif
 
 #if defined(CONFIG_PCI_MSI) && defined(CONFIG_XEN_DOM0_PCI)
diff --git a/arch/x86/xen/pci.c b/arch/x86/xen/pci.c
index 44d91ad..6948d20 100644
--- a/arch/x86/xen/pci.c
+++ b/arch/x86/xen/pci.c
@@ -2,6 +2,7 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/msi.h>
+#include <linux/slab.h>
 
 #include <asm/mpspec.h>
 #include <asm/io_apic.h>
@@ -109,3 +110,83 @@ error:
 	return ret;
 }
 #endif
+
+struct xen_device_domain_owner {
+	domid_t domain;
+	struct pci_dev *dev;
+	struct list_head list;
+};
+
+static DEFINE_SPINLOCK(dev_domain_list_spinlock);
+static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
+
+static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
+{
+	struct xen_device_domain_owner *owner;
+
+	list_for_each_entry(owner, &dev_domain_list, list) {
+		if (owner->dev == dev)
+			return owner;
+	}
+	return NULL;
+}
+
+int xen_find_device_domain_owner(struct pci_dev *dev)
+{
+	struct xen_device_domain_owner *owner;
+	unsigned long flags;
+	int domain = -ENODEV;
+
+	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
+	owner = find_device(dev);
+	if (owner)
+		domain = owner->domain;
+	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
+	return domain;
+}
+EXPORT_SYMBOL(xen_find_device_domain_owner);
+
+int xen_register_device_domain_owner(struct pci_dev *dev, domid_t domain)
+{
+	struct xen_device_domain_owner *owner;
+	unsigned long flags;
+	int rc = 0;
+
+	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
+	if (find_device(dev)) {
+		rc = -EEXIST;
+		goto out;
+	}
+	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
+	if (!owner) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	owner->domain = domain;
+	owner->dev = dev;
+	list_add_tail(&owner->list, &dev_domain_list);
+out:
+	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
+	return rc;
+}
+EXPORT_SYMBOL(xen_register_device_domain_owner);
+
+int xen_unregister_device_domain_owner(struct pci_dev *dev)
+{
+	struct xen_device_domain_owner *owner;
+	unsigned long flags;
+	int rc = 0;
+
+	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
+	owner = find_device(dev);
+	if (!owner) {
+		rc = -ENODEV;
+		goto out;
+	}
+	list_del(&owner->list);
+	kfree(owner);
+out:
+	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
+	return rc;
+}
+EXPORT_SYMBOL(xen_unregister_device_domain_owner);
-- 
1.6.2.2

[PATCH 3/6] [events] Check if the PCI device is owner by a domain different than DOMID_SELF.

[Konrad Rzeszutek Wilk]

We check if there is a domain owner for the PCI device. In case of failure
(meaning no domain has registered for this device) we make
DOMID_SELF the owner.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/events.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 2ede0c8..ac066ce 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -39,6 +39,7 @@
 #include <asm/sync_bitops.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
+#include <asm/xen/pci.h>
 
 #include <xen/xen-ops.h>
 #include <xen/events.h>
@@ -618,10 +619,14 @@ int xen_create_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int type)
 	int irq = 0;
 	struct physdev_map_pirq map_irq;
 	int rc;
-	domid_t domid = DOMID_SELF;
+	domid_t domid;
 	int pos;
 	u32 table_offset, bir;
 
+	domid = rc = xen_find_device_domain_owner(dev);
+	if (rc < 0)
+		domid = DOMID_SELF;
+	
 	memset(&map_irq, 0, sizeof(map_irq));
 	map_irq.domid = domid;
 	map_irq.type = MAP_PIRQ_TYPE_MSI;
-- 
1.6.2.2

[PATCH 4/6] [pciback] Register the owner (domain) of the PCI device.

[Konrad Rzeszutek Wilk]

When the front-end and back-end start negotiating we register
the domain that will use the PCI device. Furthermore during shutdown
of guest or unbinding of the PCI device (and unloading of module)
from pciback we unregister the domain owner.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pciback/pci_stub.c |    3 +++
 drivers/xen/pciback/xenbus.c   |   13 +++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
index c65c7c1..9ee947f 100644
--- a/drivers/xen/pciback/pci_stub.c
+++ b/drivers/xen/pciback/pci_stub.c
@@ -14,6 +14,7 @@
 #include <linux/wait.h>
 #include <asm/atomic.h>
 #include <xen/events.h>
+#include <asm/xen/pci.h>
 #include "pciback.h"
 #include "conf_space.h"
 #include "conf_space_quirks.h"
@@ -87,6 +88,8 @@ static void pcistub_device_release(struct kref *kref)
 
 	dev_dbg(&psdev->dev->dev, "pcistub_device_release\n");
 
+	xen_unregister_device_domain_owner(psdev->dev);
+
 	/* Clean-up the device */
 	pciback_reset_device(psdev->dev);
 	pciback_config_free_dyn_fields(psdev->dev);
diff --git a/drivers/xen/pciback/xenbus.c b/drivers/xen/pciback/xenbus.c
index efec585..17c26bf 100644
--- a/drivers/xen/pciback/xenbus.c
+++ b/drivers/xen/pciback/xenbus.c
@@ -10,6 +10,7 @@
 #include <linux/workqueue.h>
 #include <xen/xenbus.h>
 #include <xen/events.h>
+#include <asm/xen/pci.h>
 #include <linux/workqueue.h>
 #include "pciback.h"
 
@@ -221,6 +222,15 @@ static int pciback_export_device(struct pciback_device *pdev,
 	if (err)
 		goto out;
 
+	dev_dbg(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
+	if (xen_register_device_domain_owner(dev,
+					     pdev->xdev->otherend_id) != 0) {
+		dev_err(&dev->dev, "device has been assigned to another " \
+			"domain! Over-writting the ownership, but beware.\n");
+		xen_unregister_device_domain_owner(dev);
+		xen_register_device_domain_owner(dev, pdev->xdev->otherend_id);
+	}
+
 	/* TODO: It'd be nice to export a bridge and have all of its children
 	 * get exported with it. This may be best done in xend (which will
 	 * have to calculate resource usage anyway) but we probably want to
@@ -251,6 +261,9 @@ static int pciback_remove_device(struct pciback_device *pdev,
 		goto out;
 	}
 
+	dev_dbg(&dev->dev, "unregistering for %d\n", pdev->xdev->otherend_id);
+	xen_unregister_device_domain_owner(dev);
+
 	pciback_release_pci_dev(pdev, dev);
 
 out:
-- 
1.6.2.2

[PATCH 5/6] [pciback] Remove the vestiges of CONFIG_PCI_GUESTDEV.

[Konrad Rzeszutek Wilk]

The code 'pci_is_guestdev' is not yet back-ported. Will revisit
this once that has been completed.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pciback/pci_stub.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
index 9ee947f..7d33999 100644
--- a/drivers/xen/pciback/pci_stub.c
+++ b/drivers/xen/pciback/pci_stub.c
@@ -430,16 +430,6 @@ static int __devinit pcistub_probe(struct pci_dev *dev,
 
 		dev_info(&dev->dev, "seizing device\n");
 		err = pcistub_seize(dev);
-#ifdef CONFIG_PCI_GUESTDEV
-	} else if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
-		if (!pci_is_guestdev(dev)) {
-			err = -ENODEV;
-			goto out;
-		}
-
-		dev_info(&dev->dev, "seizing device\n");
-		err = pcistub_seize(dev);
-#endif /* CONFIG_PCI_GUESTDEV */
 	} else
 		/* Didn't find the device */
 		err = -ENODEV;
-- 
1.6.2.2

[PATCH 6/6] [pciback] Remove deprecated routine to find domain owner of PCI device.

[Konrad Rzeszutek Wilk]

In linux-2.6.18.hg tree the mechanism to find the domain owner was
for the MSI driver (msi-xen.c) to call in this function to retrieve
the domain number. This is not the way anymore.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/pciback/pci_stub.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/drivers/xen/pciback/pci_stub.c b/drivers/xen/pciback/pci_stub.c
index 7d33999..ec67baa 100644
--- a/drivers/xen/pciback/pci_stub.c
+++ b/drivers/xen/pciback/pci_stub.c
@@ -1157,22 +1157,6 @@ static ssize_t permissive_show(struct device_driver *drv, char *buf)
 
 DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show, permissive_add);
 
-#ifdef CONFIG_PCI_MSI
-
-int pciback_get_owner(struct pci_dev *dev)
-{
-	struct pcistub_device *psdev;
-
-	psdev = pcistub_device_find(pci_domain_nr(dev->bus), dev->bus->number,
-			PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
-
-	if (!psdev || !psdev->pdev)
-		return -1;
-
-	return psdev->pdev->xdev->otherend_id;
-}
-#endif
-
 static void pcistub_exit(void)
 {
 	driver_remove_file(&pciback_pci_driver.driver, &driver_attr_new_slot);
-- 
1.6.2.2

Re: [PATCH 2/6] Provide a mechanism to register domain owner of a PCI device.

[Jeremy Fitzhardinge]

On 12/09/09 14:43, Konrad Rzeszutek Wilk wrote:
> . and also to find the domain owner based on the PCI device and
> to unregister a domain owner of a PCI device.
>    
Could you fix this up a bit?

> +int xen_register_device_domain_owner(struct pci_dev *dev, domid_t domain)
> +{
> +	struct xen_device_domain_owner *owner;
> +	unsigned long flags;
> +	int rc = 0;
> +
> +	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
>    
Are you expecting interrupt routines could also hold the lock?

> +	if (find_device(dev)) {
> +		rc = -EEXIST;
> +		goto out;
> +	}
> +	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
>    

This can block, so you can't do it while holding the lock.  Its probably 
best to allocate it first on the assumption that it will be used (it 
seems probable that duplicate registrations are at least unlikely, and 
possibly a bug).

Thanks,
     J

[PATCH PCIBACK]

[Konrad Rzeszutek Wilk]

Please apply the following two patches to the origin/xen/dom0/pciback
branch.

 [PATCH 1/2] [xen/pciback] Fix compiler warning in pci_stub.c.
 [PATCH 2/2] [xen/pciback]  Fix compile warning in vpci.c