thr3ads.net - Xen devel - [Xen-devel] [PATCH 4 of 6] xen pci platform device driver [Apr 2010]

If this information is useful, please help other people find it:
Share via:

Stefano Stabellini

2010-Apr-22 15:16 UTC

[Xen-devel] [PATCH 4 of 6] xen pci platform device driver

Hi all,
this patch adds the xen pci platform device driver that is responsible
for initializing the grant table and xenbus in PV on HVM mode.
Few changes to xenbus and grant table are necessary to allow the delayed
initialization in HVM mode.
Finally grant table needs few additional modifications to work in HVM
mode.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>


---
 drivers/xen/Kconfig                  |   10 ++
 drivers/xen/Makefile                 |    1 +
 drivers/xen/events.c                 |    2 +-
 drivers/xen/grant-table.c            |   66 +++++++++-
 drivers/xen/platform-pci.c           |  227 ++++++++++++++++++++++++++++++++++
 drivers/xen/xenbus/xenbus_probe.c    |   20 ++-
 include/xen/grant_table.h            |    1 +
 include/xen/interface/grant_table.h  |    1 +
 include/xen/interface/platform_pci.h |   45 +++++++
 include/xen/platform_pci.h           |   40 ++++++
 include/xen/xenbus.h                 |    1 +
 11 files changed, 400 insertions(+), 14 deletions(-)
 create mode 100644 drivers/xen/platform-pci.c
 create mode 100644 include/xen/interface/platform_pci.h
 create mode 100644 include/xen/platform_pci.h

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index d6d6f3e..5f9f7da 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -184,3 +184,13 @@ config ACPI_PROCESSOR_XEN
 	   tristate
 	   depends on XEN_DOM0 && ACPI_PROCESSOR && CPU_FREQ
 	   default y
+
+config XEN_PLATFORM_PCI
+	tristate "xen platform pci device driver"
+	depends on XEN
+	select XEN_XENBUS_FRONTEND
+	help
+	  Driver for the Xen Pci Platform device: it is responsible for
+	  initializing xenbus and grant_table when running in a Xen HVM
+	  domain. As a consequence this driver is required to run any Xen PV
+	  frontend on Xen HVM.
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 23bc06e..5a63c02 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_XEN_SYS_HYPERVISOR)	+= sys-hypervisor.o
 obj-$(CONFIG_XEN_S3)           += acpi.o
 obj-$(CONFIG_ACPI_PROCESSOR_XEN) += acpi_processor.o
 obj-$(CONFIG_ACPI_HOTPLUG_MEMORY)  += xen_acpi_memhotplug.o
+obj-$(CONFIG_XEN_PLATFORM_PCI)		+= platform-pci.o
 
 xen-evtchn-y				:= evtchn.o
 xen-gntdev-y				:= gntdev.o
diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 50a360b..5f758c9 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1120,7 +1120,7 @@ static int rebind_irq_to_cpu(unsigned irq, unsigned tcpu)
 	struct evtchn_bind_vcpu bind_vcpu;
 	int evtchn = evtchn_from_irq(irq);
 
-	if (!VALID_EVTCHN(evtchn))
+	if (!VALID_EVTCHN(evtchn) || xen_hvm_domain())
 		return -1;
 
 	/* Send future instances of this interrupt to other vcpu. */
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 76fe621..238de46 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -41,11 +41,14 @@
 #include <xen/interface/xen.h>
 #include <xen/page.h>
 #include <xen/grant_table.h>
+#include <xen/platform_pci.h>
 #include <asm/xen/hypercall.h>
 
 #include <asm/pgtable.h>
 #include <asm/sync_bitops.h>
 
+#include <xen/interface/memory.h>
+#include <linux/io.h>
 
 /* External tools reserve first few grant table entries. */
 #define NR_RESERVED_ENTRIES 8
@@ -58,6 +61,7 @@ static unsigned int boot_max_nr_grant_frames;
 static int gnttab_free_count;
 static grant_ref_t gnttab_free_head;
 static DEFINE_SPINLOCK(gnttab_list_lock);
+static unsigned long hvm_pv_resume_frames;
 
 static struct grant_entry *shared;
 
@@ -448,6 +452,26 @@ static int gnttab_map(unsigned int start_idx, unsigned int
end_idx)
 	unsigned int nr_gframes = end_idx + 1;
 	int rc;
 
+	if (xen_hvm_domain()) {
+		struct xen_add_to_physmap xatp;
+		unsigned int i = end_idx;
+		rc = 0;
+		/*
+		 * Loop backwards, so that the first hypercall has the largest
+		 * index, ensuring that the table will grow only once.
+		 */
+		do {
+			xatp.domid = DOMID_SELF;
+			xatp.idx = i;
+			xatp.space = XENMAPSPACE_grant_table;
+			xatp.gpfn = (hvm_pv_resume_frames >> PAGE_SHIFT) + i;
+			if ((rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp)) != 0)
+				break;
+		} while (i-- > start_idx);
+
+		return rc;
+	}
+
 	frames = kmalloc(nr_gframes * sizeof(unsigned long), GFP_ATOMIC);
 	if (!frames)
 		return -ENOMEM;
@@ -580,9 +604,28 @@ EXPORT_SYMBOL_GPL(gnttab_reset_grant_page);
 
 int gnttab_resume(void)
 {
-	if (max_nr_grant_frames() < nr_grant_frames)
+	unsigned int max_nr_gframes;
+
+	max_nr_gframes = max_nr_grant_frames();
+	if (max_nr_gframes < nr_grant_frames)
 		return -ENOSYS;
-	return gnttab_map(0, nr_grant_frames - 1);
+
+	if (xen_pv_domain())
+		return gnttab_map(0, nr_grant_frames - 1);
+
+	if (!hvm_pv_resume_frames) {
+		hvm_pv_resume_frames = alloc_xen_mmio(PAGE_SIZE * max_nr_gframes);
+		shared = ioremap(hvm_pv_resume_frames, PAGE_SIZE * max_nr_gframes);
+		if (shared == NULL) {
+			printk(KERN_WARNING
+					"Fail to ioremap gnttab share frames\n");
+			return -ENOMEM;
+		}
+	}
+
+	gnttab_map(0, nr_grant_frames - 1);
+
+	return 0;
 }
 
 int gnttab_suspend(void)
@@ -609,15 +652,12 @@ static int gnttab_expand(unsigned int req_entries)
 	return rc;
 }
 
-static int __devinit gnttab_init(void)
+int gnttab_init(void)
 {
 	int i;
 	unsigned int max_nr_glist_frames, nr_glist_frames;
 	unsigned int nr_init_grefs;
 
-	if (!xen_domain())
-		return -ENODEV;
-
 	nr_grant_frames = 1;
 	boot_max_nr_grant_frames = __max_nr_grant_frames();
 
@@ -661,4 +701,16 @@ static int __devinit gnttab_init(void)
 	return -ENOMEM;
 }
 
-core_initcall(gnttab_init);
+static int __devinit __gnttab_init(void)
+{
+	/* Delay grant-table initialization in the PV on HVM case */
+	if (xen_hvm_domain())
+		return 0;
+
+	if (!xen_pv_domain())
+		return -ENODEV;
+
+	return gnttab_init();
+}
+
+core_initcall(__gnttab_init);
diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
new file mode 100644
index 0000000..cf91d7e
--- /dev/null
+++ b/drivers/xen/platform-pci.c
@@ -0,0 +1,227 @@
+/******************************************************************************
+ * platform-pci.c
+ * 
+ * Xen platform PCI device driver
+ * Copyright (c) 2005, Intel Corporation.
+ * Copyright (c) 2007, XenSource Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/interrupt.h>
+
+#include <asm/io.h>
+
+#include <xen/grant_table.h>
+#include <xen/platform_pci.h>
+#include <xen/interface/platform_pci.h>
+#include <xen/xenbus.h>
+#include <xen/events.h>
+#include <xen/hvm.h>
+
+#define DRV_NAME    "xen-platform-pci"
+
+MODULE_AUTHOR("ssmith@xensource.com and
stefano.stabellini@eu.citrix.com");
+MODULE_DESCRIPTION("Xen platform PCI device");
+MODULE_LICENSE("GPL");
+
+static unsigned long platform_mmio;
+static unsigned long platform_mmio_alloc;
+static unsigned long platform_mmiolen;
+
+unsigned long alloc_xen_mmio(unsigned long len)
+{
+	unsigned long addr;
+
+	addr = platform_mmio + platform_mmio_alloc;
+	platform_mmio_alloc += len;
+	BUG_ON(platform_mmio_alloc > platform_mmiolen);
+
+	return addr;
+}
+
+static uint64_t get_callback_via(struct pci_dev *pdev)
+{
+	u8 pin;
+	int irq;
+
+#ifdef __ia64__
+	for (irq = 0; irq < 16; irq++) {
+		if (isa_irq_to_vector(irq) == pdev->irq)
+			return irq; /* ISA IRQ */
+	}
+#else /* !__ia64__ */
+	irq = pdev->irq;
+	if (irq < 16)
+		return irq; /* ISA IRQ */
+#endif
+	pin = pdev->pin;
+
+	/* We don''t know the GSI. Specify the PCI INTx line instead. */
+	return (((uint64_t)0x01 << 56) | /* PCI INTx identifier */
+		((uint64_t)pci_domain_nr(pdev->bus) << 32) |
+		((uint64_t)pdev->bus->number << 16) |
+		((uint64_t)(pdev->devfn & 0xff) << 8) |
+		((uint64_t)(pin - 1) & 3));
+}
+
+static irqreturn_t do_hvm_evtchn_intr(int irq, void *dev_id)
+{
+	xen_hvm_evtchn_do_upcall(get_irq_regs());
+	return IRQ_HANDLED;
+}
+
+int xen_irq_init(struct pci_dev *pdev)
+{
+	__set_irq_handler(pdev->irq, handle_edge_irq, 0, NULL);
+	return request_irq(pdev->irq, do_hvm_evtchn_intr,
+			IRQF_SAMPLE_RANDOM | IRQF_DISABLED | IRQF_NOBALANCING |
+			IRQF_TRIGGER_RISING, "xen-platform-pci", pdev);
+}
+
+static int __devinit platform_pci_init(struct pci_dev *pdev,
+				       const struct pci_device_id *ent)
+{
+	int i, ret;
+	long ioaddr, iolen;
+	long mmio_addr, mmio_len;
+	uint64_t callback_via;
+
+	i = pci_enable_device(pdev);
+	if (i)
+		return i;
+
+	ioaddr = pci_resource_start(pdev, 0);
+	iolen = pci_resource_len(pdev, 0);
+
+	mmio_addr = pci_resource_start(pdev, 1);
+	mmio_len = pci_resource_len(pdev, 1);
+
+	if (mmio_addr == 0 || ioaddr == 0) {
+		printk(KERN_WARNING DRV_NAME ":no resources found\n");
+		return -ENOENT;
+	}
+
+	if (request_mem_region(mmio_addr, mmio_len, DRV_NAME) == NULL) {
+		printk(KERN_ERR ":MEM I/O resource 0x%lx @ 0x%lx busy\n",
+		       mmio_addr, mmio_len);
+		return -EBUSY;
+	}
+
+	if (request_region(ioaddr, iolen, DRV_NAME) == NULL) {
+		printk(KERN_ERR DRV_NAME ":I/O resource 0x%lx @ 0x%lx busy\n",
+		       iolen, ioaddr);
+		release_mem_region(mmio_addr, mmio_len);
+		return -EBUSY;
+	}
+
+	platform_mmio = mmio_addr;
+	platform_mmiolen = mmio_len;
+
+	if (!xen_have_vector_callback) {
+		xen_irq_init(pdev);
+		callback_via = get_callback_via(pdev);
+		if ((ret = xen_set_callback_via(callback_via)))
+			goto out;
+	}
+	if ((ret = gnttab_init()))
+		goto out;
+
+	if ((ret = xenbus_probe_init()))
+		goto out;
+
+ out:
+	if (ret) {
+		release_mem_region(mmio_addr, mmio_len);
+		release_region(ioaddr, iolen);
+	}
+
+	return ret;
+}
+
+#define XEN_PLATFORM_VENDOR_ID 0x5853
+#define XEN_PLATFORM_DEVICE_ID 0x0001
+static struct pci_device_id platform_pci_tbl[] __devinitdata = {
+	{XEN_PLATFORM_VENDOR_ID, XEN_PLATFORM_DEVICE_ID,
+	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
+	{0,}
+};
+
+MODULE_DEVICE_TABLE(pci, platform_pci_tbl);
+
+static struct pci_driver platform_driver = {
+	name:     DRV_NAME,
+	probe:    platform_pci_init,
+	id_table: platform_pci_tbl,
+};
+
+static int check_platform_magic(void)
+{
+	short magic;
+	char protocol, *err;
+
+	magic = inw(XEN_IOPORT_MAGIC);
+
+	if (magic != XEN_IOPORT_MAGIC_VAL) {
+		err = "unrecognised magic value";
+		goto no_dev;
+	}
+
+	protocol = inb(XEN_IOPORT_PROTOVER);
+
+	printk(KERN_DEBUG DRV_NAME "I/O protocol version %d\n", protocol);
+
+	switch (protocol) {
+	case 1:
+		outw(XEN_IOPORT_LINUX_PRODNUM, XEN_IOPORT_PRODNUM);
+		outl(XEN_IOPORT_LINUX_DRVVER, XEN_IOPORT_DRVVER);
+		if (inw(XEN_IOPORT_MAGIC) != XEN_IOPORT_MAGIC_VAL) {
+			printk(KERN_ERR DRV_NAME "blacklisted by host\n");
+			return -ENODEV;
+		}
+		break;
+	default:
+		err = "unknown I/O protocol version";
+		goto no_dev;
+	}
+
+	return 0;
+
+ no_dev:
+	printk(KERN_WARNING DRV_NAME  "failed backend handshake: %s\n",
err);
+	return 0;
+}
+
+static int __init platform_pci_module_init(void)
+{
+	int rc;
+
+	rc = check_platform_magic();
+	if (rc < 0)
+		return rc;
+
+	rc = pci_register_driver(&platform_driver);
+	if (rc) {
+		printk(KERN_INFO DRV_NAME
+		       ": No platform pci device model found\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+module_init(platform_pci_module_init);
diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
index 7d11957..f0e6574 100644
--- a/drivers/xen/xenbus/xenbus_probe.c
+++ b/drivers/xen/xenbus/xenbus_probe.c
@@ -668,17 +668,25 @@ void xenbus_probe(struct work_struct *unused)
 	blocking_notifier_call_chain(&xenstore_chain, 0, NULL);
 }
 
-static int __init xenbus_probe_init(void)
+static int __init __xenbus_probe_init(void)
+{
+	/* Delay initialization in the PV on HVM case */
+	if (xen_hvm_domain())
+		return 0;
+
+	if (!xen_pv_domain())
+		return -ENODEV;
+
+	return xenbus_probe_init();
+}
+
+int xenbus_probe_init(void)
 {
 	int err = 0;
 	unsigned long page = 0;
 
 	DPRINTK("");
 
-	err = -ENODEV;
-	if (!xen_domain())
-		return err;
-
 	/*
 	 * Domain0 doesn''t have a store_evtchn or store_mfn yet.
 	 */
@@ -752,6 +760,6 @@ static int __init xenbus_probe_init(void)
 	return err;
 }
 
-postcore_initcall(xenbus_probe_init);
+postcore_initcall(__xenbus_probe_init);
 
 MODULE_LICENSE("GPL");
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 9e54167..7f48be7 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -59,6 +59,7 @@ struct gnttab_free_callback {
 
 void gnttab_reset_grant_page(struct page *page);
 
+int gnttab_init(void);
 int gnttab_suspend(void);
 int gnttab_resume(void);
 
diff --git a/include/xen/interface/grant_table.h
b/include/xen/interface/grant_table.h
index 8211af8..c704fe5 100644
--- a/include/xen/interface/grant_table.h
+++ b/include/xen/interface/grant_table.h
@@ -28,6 +28,7 @@
 #ifndef __XEN_PUBLIC_GRANT_TABLE_H__
 #define __XEN_PUBLIC_GRANT_TABLE_H__
 
+#include <xen/interface/xen.h>
 
 /***********************************
  * GRANT TABLE REPRESENTATION
diff --git a/include/xen/interface/platform_pci.h
b/include/xen/interface/platform_pci.h
new file mode 100644
index 0000000..bc230cd
--- /dev/null
+++ b/include/xen/interface/platform_pci.h
@@ -0,0 +1,45 @@
+/******************************************************************************
+ * platform_pci.h
+ *
+ * Interface for granting foreign access to page frames, and receiving
+ * page-ownership transfers.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the
"Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef __XEN_PUBLIC_PLATFORM_PCI_H__
+#define __XEN_PUBLIC_PLATFORM_PCI_H__
+
+#define XEN_IOPORT_BASE 0x10
+
+#define XEN_IOPORT_PLATFLAGS	(XEN_IOPORT_BASE + 0) /* 1 byte access (R/W) */
+#define XEN_IOPORT_MAGIC	(XEN_IOPORT_BASE + 0) /* 2 byte access (R) */
+#define XEN_IOPORT_UNPLUG	(XEN_IOPORT_BASE + 0) /* 2 byte access (W) */
+#define XEN_IOPORT_DRVVER	(XEN_IOPORT_BASE + 0) /* 4 byte access (W) */
+
+#define XEN_IOPORT_SYSLOG	(XEN_IOPORT_BASE + 2) /* 1 byte access (W) */
+#define XEN_IOPORT_PROTOVER	(XEN_IOPORT_BASE + 2) /* 1 byte access (R) */
+#define XEN_IOPORT_PRODNUM	(XEN_IOPORT_BASE + 2) /* 2 byte access (W) */
+
+#define UNPLUG_ALL_IDE_DISKS 1
+#define UNPLUG_ALL_NICS 2
+#define UNPLUG_AUX_IDE_DISKS 4
+#define UNPLUG_ALL 7
+
+#endif /* __XEN_PUBLIC_PLATFORM_PCI_H__ */
diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
new file mode 100644
index 0000000..870e7a4
--- /dev/null
+++ b/include/xen/platform_pci.h
@@ -0,0 +1,40 @@
+/******************************************************************************
+ * platform-pci.h
+ * 
+ * Xen platform PCI device driver
+ * Copyright (c) 2004, Intel Corporation. <xiaofeng.ling@intel.com>
+ * Copyright (c) 2007, XenSource Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef _XEN_PLATFORM_PCI_H
+#define _XEN_PLATFORM_PCI_H
+
+#include <linux/version.h>
+
+#define XEN_IOPORT_MAGIC_VAL 0x49d2
+#define XEN_IOPORT_LINUX_PRODNUM 0xffff
+#define XEN_IOPORT_LINUX_DRVVER  ((LINUX_VERSION_CODE << 8) + 0x0)
+
+#ifdef CONFIG_XEN_PLATFORM_PCI
+unsigned long alloc_xen_mmio(unsigned long len);
+#else
+static inline unsigned long alloc_xen_mmio(unsigned long len)
+{
+	return ~0UL;
+}
+#endif
+
+#endif /* _XEN_PLATFORM_PCI_H */
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 542ca7c..a7d13ff 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -173,6 +173,7 @@ void unregister_xenbus_watch(struct xenbus_watch *watch);
 void xs_suspend(void);
 void xs_resume(void);
 void xs_suspend_cancel(void);
+int xenbus_probe_init(void);
 
 /* Used by xenbus_dev to borrow kernel''s store connection. */
 void *xenbus_dev_request_and_reply(struct xsd_sockmsg *msg);
-- 
1.5.4.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Konrad Rzeszutek Wilk

2010-Apr-22 21:02 UTC

head link

Re: [Xen-devel] [PATCH 4 of 6] xen pci platform device driver

On Thu, Apr 22, 2010 at 04:16:36PM +0100, Stefano Stabellini
wrote:> Hi all,
> this patch adds the xen pci platform device driver that is responsible
> for initializing the grant table and xenbus in PV on HVM mode.
> Few changes to xenbus and grant table are necessary to allow the delayed
> initialization in HVM mode.
> Finally grant table needs few additional modifications to work in HVM
> mode.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> 
> 
> ---
>  drivers/xen/Kconfig                  |   10 ++
>  drivers/xen/Makefile                 |    1 +
>  drivers/xen/events.c                 |    2 +-
>  drivers/xen/grant-table.c            |   66 +++++++++-
>  drivers/xen/platform-pci.c           |  227
++++++++++++++++++++++++++++++++++
>  drivers/xen/xenbus/xenbus_probe.c    |   20 ++-
>  include/xen/grant_table.h            |    1 +
>  include/xen/interface/grant_table.h  |    1 +
>  include/xen/interface/platform_pci.h |   45 +++++++
>  include/xen/platform_pci.h           |   40 ++++++
>  include/xen/xenbus.h                 |    1 +
>  11 files changed, 400 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/xen/platform-pci.c
>  create mode 100644 include/xen/interface/platform_pci.h
>  create mode 100644 include/xen/platform_pci.h
> 
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index d6d6f3e..5f9f7da 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -184,3 +184,13 @@ config ACPI_PROCESSOR_XEN
>  	   tristate
>  	   depends on XEN_DOM0 && ACPI_PROCESSOR && CPU_FREQ
>  	   default y
> +
> +config XEN_PLATFORM_PCI
> +	tristate "xen platform pci device driver"
> +	depends on XEN
> +	select XEN_XENBUS_FRONTEND
> +	help
> +	  Driver for the Xen Pci Platform device: it is responsible for
Pci -> PCI

.. snip ..> +	  initializing xenbus and grant_table when running in a Xen HVM
> +	  domain. As a consequence this driver is required to run any Xen PV
> +	  frontend on Xen HVM.
> @@ -448,6 +452,26 @@ static int gnttab_map(unsigned int start_idx, unsigned
int end_idx)
>  	unsigned int nr_gframes = end_idx + 1;
>  	int rc;
>  
> +	if (xen_hvm_domain()) {
> +		struct xen_add_to_physmap xatp;
> +		unsigned int i = end_idx;
> +		rc = 0;
> +		/*
> +		 * Loop backwards, so that the first hypercall has the largest
> +		 * index, ensuring that the table will grow only once.
> +		 */
> +		do {
> +			xatp.domid = DOMID_SELF;
> +			xatp.idx = i;
> +			xatp.space = XENMAPSPACE_grant_table;
> +			xatp.gpfn = (hvm_pv_resume_frames >> PAGE_SHIFT) + i;
> +			if ((rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp)) != 0)
> +				break;
> +		} while (i-- > start_idx);
> +So if the hypercall fails, should we print out a message ? Or is it OK
to return the rc?> +		return rc;
> +	}
> +
>  	frames = kmalloc(nr_gframes * sizeof(unsigned long), GFP_ATOMIC);
>  	if (!frames)
>  		return -ENOMEM;
... snip ..> +++ b/drivers/xen/platform-pci.c
> @@ -0,0 +1,227 @@
>
+/******************************************************************************
> + * platform-pci.c
> + * 
> + * Xen platform PCI device driver
> + * Copyright (c) 2005, Intel Corporation.
> + * Copyright (c) 2007, XenSource Inc.
No ''Copyright (c) 2010, Citrix''?
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
with
> + * this program; if not, write to the Free Software Foundation, Inc., 59
Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>
> +
> +#include <asm/io.h>
I think scripts/checkpatch.pl complains about this. You did
run script/checkpath.pl, right?

You might want also to put all of those includes in
an alphabetical order.
> +
> +#include <xen/grant_table.h>
> +#include <xen/platform_pci.h>
> +#include <xen/interface/platform_pci.h>
> +#include <xen/xenbus.h>
> +#include <xen/events.h>
> +#include <xen/hvm.h>
> +
> +#define DRV_NAME    "xen-platform-pci"
> +
> +MODULE_AUTHOR("ssmith@xensource.com and
stefano.stabellini@eu.citrix.com");
> +MODULE_DESCRIPTION("Xen platform PCI device");
> +MODULE_LICENSE("GPL");
> +
> +static unsigned long platform_mmio;
> +static unsigned long platform_mmio_alloc;
> +static unsigned long platform_mmiolen;
> +
> +unsigned long alloc_xen_mmio(unsigned long len)
> +{
> +	unsigned long addr;
> +
> +	addr = platform_mmio + platform_mmio_alloc;
> +	platform_mmio_alloc += len;
> +	BUG_ON(platform_mmio_alloc > platform_mmiolen);
> +
> +	return addr;
> +}
> +
> +static uint64_t get_callback_via(struct pci_dev *pdev)
> +{
> +	u8 pin;
> +	int irq;
> +
> +#ifdef __ia64__
> +	for (irq = 0; irq < 16; irq++) {
> +		if (isa_irq_to_vector(irq) == pdev->irq)
> +			return irq; /* ISA IRQ */
> +	}
Um, is this still neccessary? Have you tested this driver on IA64 with
this kernel?> +#else /* !__ia64__ */
> +	irq = pdev->irq;
> +	if (irq < 16)
> +		return irq; /* ISA IRQ */
> +#endif
> +	pin = pdev->pin;
> +
> +	/* We don''t know the GSI. Specify the PCI INTx line instead. */
> +	return (((uint64_t)0x01 << 56) | /* PCI INTx identifier */
> +		((uint64_t)pci_domain_nr(pdev->bus) << 32) |
> +		((uint64_t)pdev->bus->number << 16) |
> +		((uint64_t)(pdev->devfn & 0xff) << 8) |
> +		((uint64_t)(pin - 1) & 3));
> +}
> +
> +static irqreturn_t do_hvm_evtchn_intr(int irq, void *dev_id)
> +{
> +	xen_hvm_evtchn_do_upcall(get_irq_regs());
> +	return IRQ_HANDLED;
> +}
> +
> +int xen_irq_init(struct pci_dev *pdev)
The name sounds awkward. How about ''get_irq'' or
''allocate_irq''? > +{
> +	__set_irq_handler(pdev->irq, handle_edge_irq, 0, NULL);
Is that kosher?  I am not that familiar with how QEMU sets up the ACPI
DSDT to figure out what the device pin (which looking at the qemu source
is set to ''1'') corresponds to what IRQ.

Is it possible that with AMD-Vi or Intel VT-d you pass in a PCI device
that would have an interrupt line that would coincide with this device?
Or is the hvmloader + QEMU smart enought to take that under account
and stick this device on an edge interrupt line?

Perhaps doing something like:
	int flags =  IRQF_SAMPLE_RANDOM | IRQF_NOBALANCING;

	if (pdev->irq > 16)
		flags |= IRQF_SHARED;
	else {
		flags |= IRQF_TRIGGER_RISING | IRQF_DISABLED;
		/* Whack! */
		__set_irq_handler(pdev->irq, handle_edge_irq, 0, NULL);
	}
	return request_irq(pdev->irq, do_.., flags,"xen-pla..", pdev);
> +	return request_irq(pdev->irq, do_hvm_evtchn_intr,
> +			IRQF_SAMPLE_RANDOM | IRQF_DISABLED | IRQF_NOBALANCING |
> +			IRQF_TRIGGER_RISING, "xen-platform-pci", pdev);
> +}
> +
> +static int __devinit platform_pci_init(struct pci_dev *pdev,
> +				       const struct pci_device_id *ent)
> +{
> +	int i, ret;
> +	long ioaddr, iolen;
> +	long mmio_addr, mmio_len;
> +	uint64_t callback_via;
> +
> +	i = pci_enable_device(pdev);
> +	if (i)
> +		return i;
> +
> +	ioaddr = pci_resource_start(pdev, 0);
> +	iolen = pci_resource_len(pdev, 0);
> +
> +	mmio_addr = pci_resource_start(pdev, 1);
> +	mmio_len = pci_resource_len(pdev, 1);
> +
> +	if (mmio_addr == 0 || ioaddr == 0) {
> +		printk(KERN_WARNING DRV_NAME ":no resources found\n");Uhh, you can also use ''dev_warn'' - much nicer than thse
printks.

Shouldn''t you do pci_disable_device(..) ?
> +		return -ENOENT;
> +	}
> +
> +	if (request_mem_region(mmio_addr, mmio_len, DRV_NAME) == NULL) {
> +		printk(KERN_ERR ":MEM I/O resource 0x%lx @ 0x%lx busy\n",
> +		       mmio_addr, mmio_len);
Ditto> +		return -EBUSY;
> +	}
> +
> +	if (request_region(ioaddr, iolen, DRV_NAME) == NULL) {
> +		printk(KERN_ERR DRV_NAME ":I/O resource 0x%lx @ 0x%lx busy\n",
> +		       iolen, ioaddr);
> +		release_mem_region(mmio_addr, mmio_len);
> +		return -EBUSY;
Ditto.> +	}
> +
> +	platform_mmio = mmio_addr;
> +	platform_mmiolen = mmio_len;
> +
> +	if (!xen_have_vector_callback) {
> +		xen_irq_init(pdev);
Not checking the return value? What if it is -1?
> +		callback_via = get_callback_via(pdev);
> +		if ((ret = xen_set_callback_via(callback_via)))
What happens with the xen_have_vector_callback and 
x86_platform_ipi_callback which were set in the previous patch?
Will you receive a poke on do_hvm_pv_evtchn_intr and do_hvm_evtchn_intr?
> +			goto out;
> +	}
> +	if ((ret = gnttab_init()))
> +		goto out;
> +
> +	if ((ret = xenbus_probe_init()))
> +		goto out;
> +
> + out:
> +	if (ret) {
> +		release_mem_region(mmio_addr, mmio_len);
> +		release_region(ioaddr, iolen);
No pci_disable_device() ?> +	}
> +
> +	return ret;
> +}
> +
> +#define XEN_PLATFORM_VENDOR_ID 0x5853
> +#define XEN_PLATFORM_DEVICE_ID 0x0001
> +static struct pci_device_id platform_pci_tbl[] __devinitdata = {
> +	{XEN_PLATFORM_VENDOR_ID, XEN_PLATFORM_DEVICE_ID,
> +	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +	{0,}
> +};
> +
> +MODULE_DEVICE_TABLE(pci, platform_pci_tbl);
> +
> +static struct pci_driver platform_driver = {
> +	name:     DRV_NAME,
> +	probe:    platform_pci_init,
> +	id_table: platform_pci_tbl,
> +};
> +
> +static int check_platform_magic(void)
> +{
> +	short magic;
> +	char protocol, *err;
> +
> +	magic = inw(XEN_IOPORT_MAGIC);
> +
> +	if (magic != XEN_IOPORT_MAGIC_VAL) {
> +		err = "unrecognised magic value";
> +		goto no_dev;
> +	}
> +
> +	protocol = inb(XEN_IOPORT_PROTOVER);
> +
> +	printk(KERN_DEBUG DRV_NAME "I/O protocol version %d\n",
protocol);
dev_dbg
> +
> +	switch (protocol) {
> +	case 1:
> +		outw(XEN_IOPORT_LINUX_PRODNUM, XEN_IOPORT_PRODNUM);
> +		outl(XEN_IOPORT_LINUX_DRVVER, XEN_IOPORT_DRVVER);
> +		if (inw(XEN_IOPORT_MAGIC) != XEN_IOPORT_MAGIC_VAL) {
> +			printk(KERN_ERR DRV_NAME "blacklisted by host\n");
dev_err
> +			return -ENODEV;
> +		}
> +		break;
> +	default:
> +		err = "unknown I/O protocol version";
> +		goto no_dev;
> +	}
> +
> +	return 0;
> +
> + no_dev:
> +	printk(KERN_WARNING DRV_NAME  "failed backend handshake: %s\n",
err);dev_warn. Shouldn''t the rc be -ENODEV?
> +	return 0;
> +}
> +
> +static int __init platform_pci_module_init(void)
> +{
> +	int rc;
> +
> +	rc = check_platform_magic();
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = pci_register_driver(&platform_driver);
> +	if (rc) {
> +		printk(KERN_INFO DRV_NAME
> +		       ": No platform pci device model found\n");
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +module_init(platform_pci_module_init);
> diff --git a/drivers/xen/xenbus/xenbus_probe.c
b/drivers/xen/xenbus/xenbus_probe.c
> index 7d11957..f0e6574 100644
> --- a/drivers/xen/xenbus/xenbus_probe.c
> +++ b/drivers/xen/xenbus/xenbus_probe.c
> @@ -668,17 +668,25 @@ void xenbus_probe(struct work_struct *unused)
>  	blocking_notifier_call_chain(&xenstore_chain, 0, NULL);
>  }
>  
> -static int __init xenbus_probe_init(void)
> +static int __init __xenbus_probe_init(void)
> +{
> +	/* Delay initialization in the PV on HVM case */
> +	if (xen_hvm_domain())
> +		return 0;
> +
> +	if (!xen_pv_domain())
> +		return -ENODEV;
> +
> +	return xenbus_probe_init();
> +}
> +
> +int xenbus_probe_init(void)
>  {
>  	int err = 0;
>  	unsigned long page = 0;
>  
>  	DPRINTK("");
>  
> -	err = -ENODEV;
> -	if (!xen_domain())
> -		return err;
> -
>  	/*
>  	 * Domain0 doesn''t have a store_evtchn or store_mfn yet.
>  	 */
> @@ -752,6 +760,6 @@ static int __init xenbus_probe_init(void)
>  	return err;
>  }
>  
> -postcore_initcall(xenbus_probe_init);
> +postcore_initcall(__xenbus_probe_init);
>  
>  MODULE_LICENSE("GPL");
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 9e54167..7f48be7 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -59,6 +59,7 @@ struct gnttab_free_callback {
>  
>  void gnttab_reset_grant_page(struct page *page);
>  
> +int gnttab_init(void);
>  int gnttab_suspend(void);
>  int gnttab_resume(void);
>  
> diff --git a/include/xen/interface/grant_table.h
b/include/xen/interface/grant_table.h
> index 8211af8..c704fe5 100644
> --- a/include/xen/interface/grant_table.h
> +++ b/include/xen/interface/grant_table.h
> @@ -28,6 +28,7 @@
>  #ifndef __XEN_PUBLIC_GRANT_TABLE_H__
>  #define __XEN_PUBLIC_GRANT_TABLE_H__
>  
> +#include <xen/interface/xen.h>
>  
>  /***********************************
>   * GRANT TABLE REPRESENTATION
> diff --git a/include/xen/interface/platform_pci.h
b/include/xen/interface/platform_pci.h
> new file mode 100644
> index 0000000..bc230cd
> --- /dev/null
> +++ b/include/xen/interface/platform_pci.h
> @@ -0,0 +1,45 @@
>
+/******************************************************************************
> + * platform_pci.h
> + *
> + * Interface for granting foreign access to page frames, and receiving
> + * page-ownership transfers.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
copy
> + * of this software and associated documentation files (the
"Software"), to
> + * deal in the Software without restriction, including without limitation
the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense,
and/or
> + * sell copies of the Software, and to permit persons to whom the Software
is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included
in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef __XEN_PUBLIC_PLATFORM_PCI_H__
> +#define __XEN_PUBLIC_PLATFORM_PCI_H__
> +
> +#define XEN_IOPORT_BASE 0x10
> +
> +#define XEN_IOPORT_PLATFLAGS	(XEN_IOPORT_BASE + 0) /* 1 byte access (R/W)
*/
> +#define XEN_IOPORT_MAGIC	(XEN_IOPORT_BASE + 0) /* 2 byte access (R) */
> +#define XEN_IOPORT_UNPLUG	(XEN_IOPORT_BASE + 0) /* 2 byte access (W) */
> +#define XEN_IOPORT_DRVVER	(XEN_IOPORT_BASE + 0) /* 4 byte access (W) */
> +
> +#define XEN_IOPORT_SYSLOG	(XEN_IOPORT_BASE + 2) /* 1 byte access (W) */
> +#define XEN_IOPORT_PROTOVER	(XEN_IOPORT_BASE + 2) /* 1 byte access (R) */
> +#define XEN_IOPORT_PRODNUM	(XEN_IOPORT_BASE + 2) /* 2 byte access (W) */
> +
> +#define UNPLUG_ALL_IDE_DISKS 1
> +#define UNPLUG_ALL_NICS 2
> +#define UNPLUG_AUX_IDE_DISKS 4
> +#define UNPLUG_ALL 7
> +
> +#endif /* __XEN_PUBLIC_PLATFORM_PCI_H__ */
> diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
> new file mode 100644
> index 0000000..870e7a4
> --- /dev/null
> +++ b/include/xen/platform_pci.h
> @@ -0,0 +1,40 @@
>
+/******************************************************************************
> + * platform-pci.h
> + * 
> + * Xen platform PCI device driver
> + * Copyright (c) 2004, Intel Corporation. <xiaofeng.ling@intel.com>
> + * Copyright (c) 2007, XenSource Inc.
Copyring 2010 Citrix?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Stefano Stabellini

2010-Apr-23 15:33 UTC

head link

Re: [Xen-devel] [PATCH 4 of 6] xen pci platform device driver

Thanks for the very detailed and careful review!

On Thu, 22 Apr 2010, Konrad Rzeszutek Wilk wrote:> 
> Pci -> PCI
> 
Ok
> > +       initializing xenbus and grant_table when running in a Xen HVM
> .. snip ..
> > +       domain. As a consequence this driver is required to run any
Xen PV
> > +       frontend on Xen HVM.
> > @@ -448,6 +452,26 @@ static int gnttab_map(unsigned int start_idx,
unsigned int end_idx)
> >       unsigned int nr_gframes = end_idx + 1;
> >       int rc;
> >
> > +     if (xen_hvm_domain()) {
> > +             struct xen_add_to_physmap xatp;
> > +             unsigned int i = end_idx;
> > +             rc = 0;
> > +             /*
> > +              * Loop backwards, so that the first hypercall has the
largest
> > +              * index, ensuring that the table will grow only once.
> > +              */
> > +             do {
> > +                     xatp.domid = DOMID_SELF;
> > +                     xatp.idx = i;
> > +                     xatp.space = XENMAPSPACE_grant_table;
> > +                     xatp.gpfn = (hvm_pv_resume_frames >>
PAGE_SHIFT) + i;
> > +                     if ((rc =
HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp)) != 0)
> > +                             break;
> > +             } while (i-- > start_idx);
> > +
> So if the hypercall fails, should we print out a message ? Or is it OK
> to return the rc?
good catch, I actually added a printk there for debugging at some point
but I didn''t commit it.
> > +             return rc;
> > +     }
> > +
> >       frames = kmalloc(nr_gframes * sizeof(unsigned long),
GFP_ATOMIC);
> >       if (!frames)
> >               return -ENOMEM;
> 
> ... snip ..
> > +++ b/drivers/xen/platform-pci.c
> > @@ -0,0 +1,227 @@
> >
+/******************************************************************************
> > + * platform-pci.c
> > + *
> > + * Xen platform PCI device driver
> > + * Copyright (c) 2005, Intel Corporation.
> > + * Copyright (c) 2007, XenSource Inc.
> 
> No ''Copyright (c) 2010, Citrix''?
> 
I''ll add one
> > + *
> > + * This program is free software; you can redistribute it and/or
modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
along with
> > + * this program; if not, write to the Free Software Foundation, Inc.,
59 Temple
> > + * Place - Suite 330, Boston, MA 02111-1307 USA.
> > + *
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/interrupt.h>
> > +
> > +#include <asm/io.h>
> 
> I think scripts/checkpatch.pl complains about this. You did
> run script/checkpath.pl, right?
> 
> You might want also to put all of those includes in
> an alphabetical order.
> 
script/checkpatch.pl complains about a lot of things :)
I''ll fix.
> > +
> > +#include <xen/grant_table.h>
> > +#include <xen/platform_pci.h>
> > +#include <xen/interface/platform_pci.h>
> > +#include <xen/xenbus.h>
> > +#include <xen/events.h>
> > +#include <xen/hvm.h>
> > +
> > +#define DRV_NAME    "xen-platform-pci"
> > +
> > +MODULE_AUTHOR("ssmith@xensource.com and
stefano.stabellini@eu.citrix.com");
> > +MODULE_DESCRIPTION("Xen platform PCI device");
> > +MODULE_LICENSE("GPL");
> > +
> > +static unsigned long platform_mmio;
> > +static unsigned long platform_mmio_alloc;
> > +static unsigned long platform_mmiolen;
> > +
> > +unsigned long alloc_xen_mmio(unsigned long len)
> > +{
> > +     unsigned long addr;
> > +
> > +     addr = platform_mmio + platform_mmio_alloc;
> > +     platform_mmio_alloc += len;
> > +     BUG_ON(platform_mmio_alloc > platform_mmiolen);
> > +
> > +     return addr;
> > +}
> > +
> > +static uint64_t get_callback_via(struct pci_dev *pdev)
> > +{
> > +     u8 pin;
> > +     int irq;
> > +
> > +#ifdef __ia64__
> > +     for (irq = 0; irq < 16; irq++) {
> > +             if (isa_irq_to_vector(irq) == pdev->irq)
> > +                     return irq; /* ISA IRQ */
> > +     }
> 
> Um, is this still neccessary? Have you tested this driver on IA64 with
> this kernel?
No I didn''t and it would be difficult for me to.
However reading the code again it seems to me that the ifdef is
unnecessary here, so I''ll remove it.

> > +#else /* !__ia64__ */
> > +     irq = pdev->irq;
> > +     if (irq < 16)
> > +             return irq; /* ISA IRQ */
> > +#endif
> > +     pin = pdev->pin;
> > +
> > +     /* We don''t know the GSI. Specify the PCI INTx line
instead. */
> > +     return (((uint64_t)0x01 << 56) | /* PCI INTx identifier */
> > +             ((uint64_t)pci_domain_nr(pdev->bus) << 32) |
> > +             ((uint64_t)pdev->bus->number << 16) |
> > +             ((uint64_t)(pdev->devfn & 0xff) << 8) |
> > +             ((uint64_t)(pin - 1) & 3));
> > +}
> > +
> > +static irqreturn_t do_hvm_evtchn_intr(int irq, void *dev_id)
> > +{
> > +     xen_hvm_evtchn_do_upcall(get_irq_regs());
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +int xen_irq_init(struct pci_dev *pdev)
> 
> The name sounds awkward. How about ''get_irq'' or
''allocate_irq''?
Ok, I''ll go for xen_allocate_irq.
> > +{
> > +     __set_irq_handler(pdev->irq, handle_edge_irq, 0, NULL);
> 
> Is that kosher?  I am not that familiar with how QEMU sets up the ACPI
> DSDT to figure out what the device pin (which looking at the qemu source
> is set to ''1'') corresponds to what IRQ.
> 
> Is it possible that with AMD-Vi or Intel VT-d you pass in a PCI device
> that would have an interrupt line that would coincide with this device?
> Or is the hvmloader + QEMU smart enought to take that under account
> and stick this device on an edge interrupt line?
> 
> Perhaps doing something like:
>         int flags =  IRQF_SAMPLE_RANDOM | IRQF_NOBALANCING;
> 
>         if (pdev->irq > 16)
>                 flags |= IRQF_SHARED;
>         else {
>                 flags |= IRQF_TRIGGER_RISING | IRQF_DISABLED;
>                 /* Whack! */
>                 __set_irq_handler(pdev->irq, handle_edge_irq, 0, NULL);
>         }
>         return request_irq(pdev->irq, do_..,
flags,"xen-pla..", pdev);
> 
Yes I did that on purpose because it seems that it is the only thing
able to fix the multivcpu problem I was having: when receiving evtchns
from the pci platform device interrupt line (vector callback not
available), the guest boots fine with a single vcpu but it hangs at boot
with 2 or more vcpus. In particular it seems to deadlock in the xenbus
initialization code because of some missing interrupts/evtchns.

Switching to handle_edge_irq (even though the interrupt is supposed to
be level triggered, but it is emulated, so I guess is not important anyway)
fixes the issue for me. It is also interesting that using vector
callback the problem doesn''t occur.

In the default scenario linux uses handle_fasteoi_irq that acks the
interrupt only at the end, after the call to handle_IRQ_event, that in
the evtchn case can loop several times on the pending evtchns so can
take long to complete.
On the other hand handle_edge_irq acks the interrupt right away before
calling handle_IRQ_event, that is the same behaviour of the ipi handler.
Also evtchn handlers can re-enable interrupts and that would cause
handle_edge_irq to be called again if we receive another
interrupt/evtchn, and handle_edge_irq would again ack the interrupt
right away even if it is marked as IRQ_INPROGRESS while handle_fasteoi_irq
would just exit.

I guess I could add a comment before __set_irq_handler.

> > +     return request_irq(pdev->irq, do_hvm_evtchn_intr,
> > +                     IRQF_SAMPLE_RANDOM | IRQF_DISABLED |
IRQF_NOBALANCING |
> > +                     IRQF_TRIGGER_RISING,
"xen-platform-pci", pdev);
> > +}
> > +
> > +static int __devinit platform_pci_init(struct pci_dev *pdev,
> > +                                    const struct pci_device_id *ent)
> > +{
> > +     int i, ret;
> > +     long ioaddr, iolen;
> > +     long mmio_addr, mmio_len;
> > +     uint64_t callback_via;
> > +
> > +     i = pci_enable_device(pdev);
> > +     if (i)
> > +             return i;
> > +
> > +     ioaddr = pci_resource_start(pdev, 0);
> > +     iolen = pci_resource_len(pdev, 0);
> > +
> > +     mmio_addr = pci_resource_start(pdev, 1);
> > +     mmio_len = pci_resource_len(pdev, 1);
> > +
> > +     if (mmio_addr == 0 || ioaddr == 0) {
> > +             printk(KERN_WARNING DRV_NAME ":no resources
found\n");
> Uhh, you can also use ''dev_warn'' - much nicer than thse
printks.
> 
Ok
> Shouldn''t you do pci_disable_device(..) ?
> 
Yes I should
> > +     }
> > +
> > +     platform_mmio = mmio_addr;
> > +     platform_mmiolen = mmio_len;
> > +
> > +     if (!xen_have_vector_callback) {
> > +             xen_irq_init(pdev);
> 
> Not checking the return value? What if it is -1?
I''ll fix
> 
> > +             callback_via = get_callback_via(pdev);
> > +             if ((ret = xen_set_callback_via(callback_via)))
> 
> What happens with the xen_have_vector_callback and
> x86_platform_ipi_callback which were set in the previous patch?
> Will you receive a poke on do_hvm_pv_evtchn_intr and do_hvm_evtchn_intr?
> 
Yes, exactly.
If xen_have_vector_callback == 1 then I receive evtchns from
do_hvm_pv_evtchn_intr, otherwise from do_hvm_evtchn_intr.

> > + out:
> > +     if (ret) {
> > +             release_mem_region(mmio_addr, mmio_len);
> > +             release_region(ioaddr, iolen);
> No pci_disable_device() ?
I''ll fix
> > +     printk(KERN_DEBUG DRV_NAME "I/O protocol version
%d\n", protocol);
> 
> dev_dbg
> 
> > +
> > +     switch (protocol) {
> > +     case 1:
> > +             outw(XEN_IOPORT_LINUX_PRODNUM, XEN_IOPORT_PRODNUM);
> > +             outl(XEN_IOPORT_LINUX_DRVVER, XEN_IOPORT_DRVVER);
> > +             if (inw(XEN_IOPORT_MAGIC) != XEN_IOPORT_MAGIC_VAL) {
> > +                     printk(KERN_ERR DRV_NAME "blacklisted by
host\n");
> 
> dev_err
> 
> > + no_dev:
> > +     printk(KERN_WARNING DRV_NAME  "failed backend handshake:
%s\n", err);
> dev_warn.    
the problem here is that we are called by module_init, we don''t have a
struct pci_dev to use with dev_*
    
> Shouldn''t the rc be -ENODEV?
Yes, I''ll fix.

> > diff --git a/include/xen/platform_pci.h b/include/xen/platform_pci.h
> > new file mode 100644
> > index 0000000..870e7a4
> > --- /dev/null
> > +++ b/include/xen/platform_pci.h
> > @@ -0,0 +1,40 @@
> >
+/******************************************************************************
> > + * platform-pci.h
> > + *
> > + * Xen platform PCI device driver
> > + * Copyright (c) 2004, Intel Corporation.
<xiaofeng.ling@intel.com>
> > + * Copyright (c) 2007, XenSource Inc.
> 
> Copyring 2010 Citrix?
> 
I''ll add one

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Apr 2010 - [PATCH 4 of 6] xen pci platform device driver

[Xen-devel] [PATCH 4 of 6] xen pci platform device driver

Re: [Xen-devel] [PATCH 4 of 6] xen pci platform device driver

Re: [Xen-devel] [PATCH 4 of 6] xen pci platform device driver