[PATCH 0/3] save/restore PCI configuration space in pciback.

[PATCH 0/3] save/restore PCI configuration space in pciback.

[Yuji Shimada]

This series of patches enables saving/restoring PCI configuration
space in pciback.

Xend saves/restores PCI configuration space on current Xen. But it's
not better place to save/restore. Because devices should be configured
by device driver.
And Shimura-san's D-state problem will be resolved with this patch.
Because I modify the timing of saving/restoring configuration space
like below.

  When pciback is bound to devices.
   - Pciback saves configuration space.

  When pciback is unbound to devices.
   - Pciback restores configuration space.

  When guest OS boots or a device is hotadded.
   - Pciback restores configuration space.
   - Pciback changes state of backend device to Initialised/Reconfigured.
   - Xend waits for the transition to Initialised/Reconfigured.

  When guest OS shutdowns or a device is hotremoved.
   - Pciback restores configuration space.
   - Xend resets devices.
     * If D-state of the device is not D0, the state is changed to D0
       before resetting the device.
   - Xend deassigns devices.

  * Essentially, devices should be reset before configuration space is
    restored. But it needs big modifications. Applying these patches,
    configuration space is restored when guest OS boots, a device is
    hotadded or pciback is unbound. So it has no matter.

The following registers are saved/restored by pciback.

  Configuration Space
   - Base Address Register set
   - Cache-line size Register
   - Latency timer Register
   - Enable SERR Bit / Enable PERR Bit in Control Register
   - Device Control Register (PCI Express Capability)
   - Link Control Register (PCI Express Capability)
   - Device Control 2 Register (PCI Express Capability)
   - Link Control 2 Register (PCI Express Capability)

  AER
   - Uncorrectable Error Mask Register
   - Uncorrectable Error Severity Register
   - Correctable Error Mask Register
   - Advanced Error Capabilities and Control Register

Thanks,
--
Yuji Shimada

[PATCH 1/3] save/restore PCI configuration space in pciback.

[Yuji Shimada]

This patch enables saving/restoring PCI configuration space in
pciback.

The timing of saving/restoring configuration space is like below.

  When pciback is bound to devices.
   - Pciback saves configuration space.

  When pciback is unbound to devices.
   - Pciback restores configuration space.

  When guest OS boots or a device is hotadded.
   - Pciback restores configuration space.
   - Pciback changes state of backend device to Initialised/Reconfigured.
   - Xend waits for the transition to Initialised/Reconfigured.

  When guest OS shutdowns or a device is hotremoved.
   - Pciback restores configuration space.
   - Xend resets devices.
     * If D-state of the device is not D0, the state is changed to D0
       before resetting the device.
   - Xend deassigns devices.

  * Essentially, devices should be reset before configuration space is
    restored. But it needs big modifications. Applying these patches,
    configuration space is restored when guest OS boots, a device is
    hotadded or pciback is unbound. So it has no matter.

The following registers are saved/restored by pciback.

  Configuration Space
   - Base Address Register set
   - Cache-line size Register
   - Latency timer Register
   - Enable SERR Bit / Enable PERR Bit in Control Register
   - Device Control Register (PCI Express Capability)
   - Link Control Register (PCI Express Capability)
   - Device Control 2 Register (PCI Express Capability)
   - Link Control 2 Register (PCI Express Capability)

  AER
   - Uncorrectable Error Mask Register
   - Uncorrectable Error Severity Register
   - Correctable Error Mask Register
   - Advanced Error Capabilities and Control Register

Thanks,
--
Yuji Shimada


Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

diff -r 366c31f3ab4b drivers/xen/pciback/Makefile
--- a/drivers/xen/pciback/Makefile	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/Makefile	Tue Apr 21 16:48:25 2009 +0900
@@ -5,6 +5,7 @@
 	     conf_space_capability.o \
 	     conf_space_capability_vpd.o \
 	     conf_space_capability_pm.o \
+	     conf_space_capability_exp.o \
              conf_space_quirks.o
 pciback-$(CONFIG_PCI_MSI) += conf_space_capability_msi.o
 pciback-$(CONFIG_XEN_PCIDEV_BACKEND_VPCI) += vpci.o
diff -r 366c31f3ab4b drivers/xen/pciback/conf_space.c
--- a/drivers/xen/pciback/conf_space.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/conf_space.c	Tue Apr 21 16:48:25 2009 +0900
@@ -375,6 +375,18 @@
 	cfg_entry->field = field;
 	cfg_entry->base_offset = base_offset;
 
+	/* When the size of the registers is different, by a version of the PCI,
+	 * "is_need" function can prevent addition to the list of an unnecessary
+	 * register.
+	 */
+	if (field->is_need) {
+		err = field->is_need(dev, base_offset);
+		if (!err) {
+			kfree(cfg_entry);
+			goto out;
+		}
+	}
+
 	/* silently ignore duplicate fields */
 	err = pciback_field_is_dup(dev,OFFSET(cfg_entry));
 	if (err)
@@ -433,3 +445,69 @@
 {
 	return pciback_config_capability_init();
 }
+
+/* save AER one register */
+static int pciback_aer_save_one_register(struct pci_dev *dev, int offset)
+{
+	struct pciback_dev_data *dev_data = pci_get_drvdata(dev);
+	return pci_read_config_dword(dev, (dev_data->config.aer_base + offset),
+		(u32*)&dev_data->config.config_aer[offset]);
+}
+
+/* save AER registers */
+int pciback_aer_reg_save(struct pci_dev *dev)
+{
+	int err = 0;
+
+	/* after reset, following register values should be restored.
+	 * So, save them.
+	 */
+	err = pciback_aer_save_one_register(dev, PCI_ERR_UNCOR_MASK);
+	if (err)
+		goto out;
+	err = pciback_aer_save_one_register(dev, PCI_ERR_UNCOR_SEVER);
+	if (err)
+		goto out;
+	err = pciback_aer_save_one_register(dev, PCI_ERR_COR_MASK);
+	if (err)
+		goto out;
+	err = pciback_aer_save_one_register(dev, PCI_ERR_CAP);
+
+      out:
+	return err;
+}
+
+/* restore AER one register */
+static int pciback_aer_restore_one_register(struct pci_dev *dev, int offset)
+{
+	struct pciback_dev_data *dev_data = pci_get_drvdata(dev);
+	return pci_write_config_dword(dev,
+				 (dev_data->config.aer_base + offset),
+				  *((u32*)&dev_data->config.config[offset]));
+}
+
+/* restore AER registers */
+int pciback_aer_reg_restore(struct pci_dev *dev)
+{
+	int err = 0;
+
+	/* the following registers should be reconfigured to correct values
+	 * after reset. restore them.
+	 * other registers should not be reconfigured after reset
+	 * if there is no reason
+	 */
+	err = pciback_aer_restore_one_register(dev, PCI_ERR_UNCOR_MASK);
+	if (err)
+		goto out;
+	err = pciback_aer_restore_one_register(dev, PCI_ERR_UNCOR_SEVER);
+	if (err)
+		goto out;
+	err = pciback_aer_restore_one_register(dev, PCI_ERR_COR_MASK);
+	if (err)
+		goto out;
+	err = pciback_aer_restore_one_register(dev, PCI_ERR_CAP);
+
+      out:
+	return err;
+}
+
diff -r 366c31f3ab4b drivers/xen/pciback/conf_space.h
--- a/drivers/xen/pciback/conf_space.h	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/conf_space.h	Tue Apr 21 16:48:25 2009 +0900
@@ -27,6 +27,10 @@
 			       void *data);
 typedef int (*conf_byte_read) (struct pci_dev * dev, int offset, u8 * value,
 			       void *data);
+typedef int (*conf_dword_restore)(struct pci_dev *dev, int offset, u32 data);
+typedef int (*conf_word_restore)(struct pci_dev *dev, int offset, u16 data);
+typedef int (*conf_byte_restore)(struct pci_dev *dev, int offset, u8 data);
+typedef int (*conf_field_is_need)(struct pci_dev *dev, int offset);
 
 /* These are the fields within the configuration space which we
  * are interested in intercepting reads/writes to and changing their
@@ -44,16 +48,20 @@
 		struct {
 			conf_dword_write write;
 			conf_dword_read read;
+			conf_dword_restore restore;
 		} dw;
 		struct {
 			conf_word_write write;
 			conf_word_read read;
+			conf_word_restore restore;
 		} w;
 		struct {
 			conf_byte_write write;
 			conf_byte_read read;
+			conf_byte_restore restore;
 		} b;
 	} u;
+	conf_field_is_need is_need;
 	struct list_head list;
 };
 
@@ -123,4 +131,22 @@
 int pciback_config_header_add_fields(struct pci_dev *dev);
 int pciback_config_capability_add_fields(struct pci_dev *dev);
 
+static inline int pciback_restore_config_dword(struct pci_dev *dev, int offset,
+					      u32 data)
+{
+	return pci_write_config_dword(dev, offset, data);
+}
+
+static inline int pciback_restore_config_word(struct pci_dev *dev, int offset,
+					      u16 data)
+{
+	return pci_write_config_word(dev, offset, data);
+}
+
+static inline int pciback_restore_config_byte(struct pci_dev *dev, int offset,
+				       u8 data)
+{
+	return pci_write_config_byte(dev, offset, data);
+}
+
 #endif				/* __XEN_PCIBACK_CONF_SPACE_H__ */
diff -r 366c31f3ab4b drivers/xen/pciback/conf_space_capability.c
--- a/drivers/xen/pciback/conf_space_capability.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/conf_space_capability.c	Tue Apr 21 16:48:25 2009 +0900
@@ -59,11 +59,13 @@
 
 extern struct pciback_config_capability pciback_config_capability_vpd;
 extern struct pciback_config_capability pciback_config_capability_pm;
+extern struct pciback_config_capability pciback_config_capability_exp;
 
 int pciback_config_capability_init(void)
 {
 	register_capability(&pciback_config_capability_vpd);
 	register_capability(&pciback_config_capability_pm);
+	register_capability(&pciback_config_capability_exp);
 
 	return 0;
 }
diff -r 366c31f3ab4b drivers/xen/pciback/conf_space_capability_exp.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/drivers/xen/pciback/conf_space_capability_exp.c	Tue Apr 21 16:48:25 2009 +0900
@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) 2009, NEC Corporation.
+ *
+ * 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.
+ */
+/*
+ * PCI Backend -- Configuration overlay for PCI Express capability
+ */
+#include <linux/pci.h>
+#include "conf_space.h"
+#include "conf_space_capability.h"
+
+/* PCI express version 1.0 does not have the registers over offset 24H */
+static int pciback_exp_is_need_config2(struct pci_dev *dev, int base_offset)
+{
+	u16 cap;
+	pci_read_config_word(dev, base_offset + PCI_EXP_FLAGS, &cap);
+	if ((cap & PCI_EXP_FLAGS_VERS) == 1)
+		return 0;
+	else
+		return 1;
+}
+
+static const struct config_field caplist_exp[] = {
+	{
+		.offset      = PCI_EXP_DEVCTL,
+		.size        = 2,
+		.u.w.restore = pciback_restore_config_word,
+	},
+	{
+		.offset      = PCI_EXP_LNKCTL,
+		.size        = 2,
+		.u.w.restore = pciback_restore_config_word,
+	},
+	{
+		.offset      = PCI_EXP_DEVCTL2,
+		.size        = 2,
+		.u.w.restore = pciback_restore_config_word,
+		.is_need     = pciback_exp_is_need_config2,
+	},
+	{
+		.offset      = PCI_EXP_LNKCTL2,
+		.size        = 2,
+		.u.w.restore = pciback_restore_config_word,
+		.is_need     = pciback_exp_is_need_config2,
+	},
+	{}
+};
+
+struct pciback_config_capability pciback_config_capability_exp = {
+	.capability = PCI_CAP_ID_EXP,
+	.fields = caplist_exp,
+};
+
diff -r 366c31f3ab4b drivers/xen/pciback/conf_space_capability_pm.c
--- a/drivers/xen/pciback/conf_space_capability_pm.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/conf_space_capability_pm.c	Tue Apr 21 16:48:25 2009 +0900
@@ -94,6 +94,27 @@
 	return ERR_PTR(err);
 }
 
+/* restore Power Management Control/Status register */
+static int pm_ctrl_restore(struct pci_dev *dev, int offset, u16 data)
+{
+	int err;
+	u16 value;
+
+	err = pci_read_config_word(dev, offset, &value);
+	if (err)
+		goto out;
+
+	/* No need to restore, just clear PME Enable and PME Status bit
+	 * Note: register type of PME Status bit is RW1C, so clear by writing 1b
+	 */
+	value = (value & ~PCI_PM_CTRL_PME_ENABLE) | PCI_PM_CTRL_PME_STATUS;
+
+	err = pci_write_config_word(dev, offset, value);
+
+      out:
+	return err;
+}
+
 static const struct config_field caplist_pm[] = {
 	{
 		.offset     = PCI_PM_PMC,
@@ -106,6 +127,7 @@
 		.init       = pm_ctrl_init,
 		.u.w.read   = pciback_read_config_word,
 		.u.w.write  = pm_ctrl_write,
+		.u.w.restore = pm_ctrl_restore,
 	},
 	{
 		.offset     = PCI_PM_PPB_EXTENSIONS,
diff -r 366c31f3ab4b drivers/xen/pciback/conf_space_header.c
--- a/drivers/xen/pciback/conf_space_header.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/conf_space_header.c	Tue Apr 21 16:48:25 2009 +0900
@@ -210,12 +210,34 @@
 	return err;
 }
 
+#define CONF_COMMAND_MASK (PCI_COMMAND_PARITY | PCI_COMMAND_SERR)
+
+static int command_restore(struct pci_dev *dev, int offset, u16 data)
+{
+	int err;
+	u16 tmp_val;
+
+	err = pci_read_config_word(dev, offset, &tmp_val);
+	if (err)
+		goto out;
+
+	tmp_val &= ~CONF_COMMAND_MASK;
+	tmp_val |= (data & CONF_COMMAND_MASK);
+
+	err = pci_write_config_word(dev, offset, tmp_val);
+
+      out:
+	return err;
+}
+
+
 static const struct config_field header_common[] = {
 	{
 	 .offset    = PCI_COMMAND,
 	 .size      = 2,
 	 .u.w.read  = pciback_read_config_word,
 	 .u.w.write = command_write,
+	 .u.w.restore = command_restore,
 	},
 	{
 	 .offset    = PCI_INTERRUPT_LINE,
@@ -233,11 +255,13 @@
 	 .size      = 1,
 	 .u.b.read  = pciback_read_config_byte,
 	 .u.b.write = pciback_write_config_byte,
+	 .u.b.restore = pciback_restore_config_byte,
 	},
 	{
 	 .offset    = PCI_LATENCY_TIMER,
 	 .size      = 1,
 	 .u.b.read  = pciback_read_config_byte,
+	 .u.b.restore = pciback_restore_config_byte,
 	},
 	{
 	 .offset    = PCI_BIST,
@@ -257,6 +281,7 @@
 	 .release    = bar_release, 			\
 	 .u.dw.read  = bar_read, 			\
 	 .u.dw.write = bar_write, 			\
+	 .u.dw.restore = pciback_restore_config_dword,	\
 	 }
 
 #define CFG_FIELD_ROM(reg_offset) 			\
@@ -268,6 +293,7 @@
 	 .release    = bar_release, 			\
 	 .u.dw.read  = bar_read, 			\
 	 .u.dw.write = rom_write, 			\
+	 .u.dw.restore = pciback_restore_config_dword,	\
 	 }
 
 static const struct config_field header_0[] = {
diff -r 366c31f3ab4b drivers/xen/pciback/pci_stub.c
--- a/drivers/xen/pciback/pci_stub.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/pci_stub.c	Tue Apr 21 16:48:25 2009 +0900
@@ -135,6 +135,91 @@
 	return psdev;
 }
 
+int pcistub_save_config_space(struct pci_dev *dev)
+{
+	int err = 0;
+	int offset;
+	u8  value;
+	struct pciback_dev_data *dev_data = pci_get_drvdata(dev);
+
+	/* save configuration space */
+	for (offset = 0; offset < 256; offset++) {
+		err = pci_read_config_byte(dev, offset, &value);
+		if (err) {
+			dev_err(&dev->dev, 
+				"(%s) Read error in config space[%x]!\n",
+				__func__, offset);
+			goto out;
+		}
+		dev_data->config.config[offset] = value;
+	}
+	dev_dbg(&dev->dev, "Save real configuration space \n");
+
+	dev_data->config.aer_base = 
+			(u32)pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR);
+	if (!dev_data->config.aer_base)
+		goto out;
+
+	/* save advanced error reporting registers */
+	err = pciback_aer_reg_save(dev);
+	dev_dbg(&dev->dev, "Save advanced error reporting[%d]\n", err);
+
+      out:
+	return err;
+}
+
+int pcistub_restore_config_space(struct pci_dev *dev)
+{
+	int err = 0;
+	struct pciback_dev_data *dev_data = pci_get_drvdata(dev);
+	struct config_field_entry *cfg_entry;
+	const struct config_field *field;
+	int offset;
+
+	/* Restore configuration space */
+	list_for_each_entry(cfg_entry, &dev_data->config_fields, list) {
+		field = cfg_entry->field;
+		offset = OFFSET(cfg_entry);
+		switch (field->size) {
+		case 1:
+			if (!field->u.b.restore)
+				continue;
+			err = field->u.b.restore(dev,
+				offset, dev_data->config.config[offset]);
+			break;
+		case 2:
+			if (!field->u.w.restore)
+				continue;
+			err = field->u.w.restore(dev, offset,
+				*((u16 *)&dev_data->config.config[offset]));
+			break;
+		case 4:
+			if (!field->u.dw.restore)
+				continue;
+			err = field->u.dw.restore(dev, offset,
+				*((u32 *)&dev_data->config.config[offset]));
+			break;
+		}
+		if (err) {
+			dev_err(&dev->dev, 
+				"(%s) Restore error<%d> in config space"
+				"at offset 0x%x!\n", __func__, err, offset);
+			goto out;
+		}
+	}
+	dev_dbg(&dev->dev, "Restore base configuration space \n");
+
+	if (!dev_data->config.aer_base)
+		goto out;
+
+	/* restore advanced error reporting registers */
+	err = pciback_aer_reg_restore(dev);
+	dev_dbg(&dev->dev, "Restore advanced error reporting\n");
+
+      out:
+	return err;
+}
+
 static struct pci_dev *pcistub_device_get_pci_dev(struct pciback_device *pdev,
 						  struct pcistub_device *psdev)
 {
@@ -292,6 +377,10 @@
 	}
 	pci_set_drvdata(dev, dev_data);
 
+	/* Save configuration space */
+	dev_dbg(&dev->dev, "Save configuration space\n");
+	pcistub_save_config_space(dev);
+
 	dev_dbg(&dev->dev, "initializing config\n");
 
 	init_waitqueue_head(&aer_wait_queue);
diff -r 366c31f3ab4b drivers/xen/pciback/pciback.h
--- a/drivers/xen/pciback/pciback.h	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/pciback.h	Tue Apr 21 16:48:25 2009 +0900
@@ -44,10 +44,17 @@
 	struct work_struct op_work;
 };
 
+struct pciback_config {
+	u8  config[256];
+	u32 aer_base;
+	u8  config_aer[72];
+};
+
 struct pciback_dev_data {
 	struct list_head config_fields;
 	int permissive;
 	int warned_on_write;
+	struct pciback_config config;
 };
 
 /* Get/Put PCI Devices that are hidden from the PCI Backend Domain */
@@ -122,5 +129,12 @@
 extern int verbose_request;
 
 void test_and_schedule_op(struct pciback_device *pdev);
+
+extern int pciback_aer_reg_save(struct pci_dev *dev);
+extern int pciback_aer_reg_restore(struct pci_dev *dev);
+
+extern int pcistub_save_config_space(struct pci_dev *dev);
+extern int pcistub_restore_config_space(struct pci_dev *dev);
+
 #endif
 
diff -r 366c31f3ab4b drivers/xen/pciback/pciback_ops.c
--- a/drivers/xen/pciback/pciback_ops.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/pciback_ops.c	Tue Apr 21 16:48:25 2009 +0900
@@ -20,6 +20,9 @@
 {
 	u16 cmd;
 
+	/* restore configuration space */
+	pcistub_restore_config_space(dev);
+
 	/* Disable devices (but not bridges) */
 	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
 		pci_disable_device(dev);
diff -r 366c31f3ab4b drivers/xen/pciback/xenbus.c
--- a/drivers/xen/pciback/xenbus.c	Tue Apr 14 11:17:47 2009 +0100
+++ b/drivers/xen/pciback/xenbus.c	Tue Apr 21 16:48:25 2009 +0900
@@ -43,6 +43,30 @@
 	return pdev;
 }
 
+static int pciback_reinit_device(struct pciback_device *pdev,
+			       int domain, int bus, int slot, int func)
+{
+	int err = 0;
+	struct pci_dev *dev;
+
+	dev_dbg(&pdev->xdev->dev, "Reinitializing dom %x bus %x slot %x"
+		" func %x\n", domain, bus, slot, func);
+
+	dev = pciback_get_pci_dev(pdev, domain, bus, PCI_DEVFN(slot, func));
+	if (!dev) {
+		err = -EINVAL;
+		dev_dbg(&pdev->xdev->dev, "Couldn't locate PCI device "
+			"(%04x:%02x:%02x.%01x)! not owned by this domain\n",
+			domain, bus, slot, func);
+		goto out;
+	}
+
+	pciback_reset_device(dev);
+	
+      out:
+	return err;
+}
+
 static void pciback_disconnect(struct pciback_device *pdev)
 {
 	spin_lock(&pdev->dev_lock);
@@ -394,6 +418,15 @@
 			if (err)
 				goto out;
 
+			err = pciback_reinit_device(pdev, domain, bus, slot,
+						  func);
+			if (err) {
+				xenbus_dev_fatal(pdev->xdev, err,
+						 "Error reinitialize pci device"
+						 "configuration");
+				goto out;
+			}
+
 			/* Publish pci roots. */
 			err = pciback_publish_pci_roots(pdev, pciback_publish_pci_root);
 			if (err) {
@@ -575,6 +608,14 @@
 		if (err)
 			goto out;
 
+		err = pciback_reinit_device(pdev, domain, bus, slot, func);
+		if (err) {
+			xenbus_dev_fatal(pdev->xdev, err,
+					 "Error reinitialize pci device "
+					 "configuration");
+			goto out;
+		}
+
 		/* Switch substate of this device. */
 		l = snprintf(state_str, sizeof(state_str), "state-%d", i);
 		if (unlikely(l >= (sizeof(state_str) - 1))) {
@@ -627,6 +668,10 @@
 		pciback_setup_backend(pdev);
 		break;
 
+	case XenbusStateReconfiguring:
+		pciback_reconfigure(pdev);
+		break;
+
 	default:
 		break;
 	}

[PATCH 2/3] remove saving/restoring method in Xend.

[Yuji Shimada]

This patch removes Xend method which saves/restores PCI configuration
space.
And this patch modifies the timing of saving/restoring configuration
space like below.

  When pciback is bound to devices.
   - Pciback saves configuration space.

  When pciback is unbound to devices.
   - Pciback restores configuration space.

  When guest OS boots or a device is hotadded.
   - Pciback restores configuration space.
   - Pciback changes state of backend device to Initialised/Reconfigured.
   - Xend waits for the transition to Initialised/Reconfigured.

  When guest OS shutdowns or a device is hotremoved.
   - Pciback restores configuration space.
   - Xend resets devices.
     * If D-state of the device is not D0, the state is changed to D0
       before resetting the device.
   - Xend deassigns devices.

  * Essentially, devices should be reset before configuration space is
    restored. But it needs big modifications. Applying these patches,
    configuration space is restored when guest OS boots, a device is
    hotadded or pciback is unbound. So it has no matter.

Thanks,
--
Yuji Shimada


Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

diff -r 94ffd85005c5 tools/python/xen/util/pci.py
--- a/tools/python/xen/util/pci.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/util/pci.py	Tue Apr 21 16:51:20 2009 +0900
@@ -70,6 +70,7 @@
 PCI_PM_CTRL_NO_SOFT_RESET = 0x0008
 PCI_PM_CTRL_STATE_MASK = 0x0003
 PCI_D3hot = 3
+PCI_D2    = 2
 PCI_D0hot = 0
 
 VENDOR_INTEL  = 0x8086
@@ -209,33 +210,6 @@
     finally:
         lspci_info_lock.release()
 
-def save_pci_conf_space(devs_string):
-    pci_list = []
-    cfg_list = []
-    sysfs_mnt = find_sysfs_mnt()
-    for pci_str in devs_string:
-        pci_path = sysfs_mnt + SYSFS_PCI_DEVS_PATH + '/' + pci_str + \
-                SYSFS_PCI_DEV_CONFIG_PATH
-        fd = os.open(pci_path, os.O_RDONLY)
-        configs = []
-        for i in range(0, 256, 4):
-            configs = configs + [os.read(fd,4)]
-        os.close(fd)
-        pci_list = pci_list + [pci_path]
-        cfg_list = cfg_list + [configs]
-    return (pci_list, cfg_list)
-
-def restore_pci_conf_space(pci_cfg_list):
-    pci_list = pci_cfg_list[0]
-    cfg_list = pci_cfg_list[1]
-    for i in range(0, len(pci_list)):
-        pci_path = pci_list[i]
-        configs  = cfg_list[i]
-        fd = os.open(pci_path, os.O_WRONLY)
-        for dw in configs:
-            os.write(fd, dw)
-        os.close(fd) 
-
 def find_all_devices_owned_by_pciback():
     sysfs_mnt = find_sysfs_mnt()
     pciback_path = sysfs_mnt + SYSFS_PCIBACK_PATH
@@ -476,9 +450,6 @@
         return dev_list
 
     def do_secondary_bus_reset(self, target_bus, devs):
-        # Save the config spaces of all the devices behind the bus.
-        (pci_list, cfg_list) = save_pci_conf_space(devs)
-        
         #Do the Secondary Bus Reset
         sysfs_mnt = find_sysfs_mnt()
         parent_path = sysfs_mnt + SYSFS_PCI_DEVS_PATH + '/' + \
@@ -498,9 +469,6 @@
         time.sleep(0.100)
         os.close(fd)
 
-        # Restore the config spaces
-        restore_pci_conf_space((pci_list, cfg_list))
-        
     def do_Dstate_transition(self):
         pos = self.find_cap_offset(PCI_CAP_ID_PM)
         if pos == 0:
@@ -513,8 +481,6 @@
         if (pm_ctl & PCI_PM_CTRL_NO_SOFT_RESET) == 1:
             return False
 
-        (pci_list, cfg_list) = save_pci_conf_space([self.name])
-        
         # Enter D3hot
         pm_ctl &= ~PCI_PM_CTRL_STATE_MASK
         pm_ctl |= PCI_D3hot
@@ -527,9 +493,25 @@
         self.pci_conf_write32(pos + PCI_PM_CTRL, pm_ctl)
         time.sleep(0.010)
 
-        restore_pci_conf_space((pci_list, cfg_list))
         return True
 
+    def do_Dstate_reset(self):
+        pos = self.find_cap_offset(PCI_CAP_ID_PM)
+        if pos == 0:
+            return
+
+        pm_ctl = self.pci_conf_read32(pos + PCI_PM_CTRL)
+        cur_state = pm_ctl & PCI_PM_CTRL_STATE_MASK
+        if (cur_state) != 0:
+            # From not D0 to D0
+            pm_ctl &= ~PCI_PM_CTRL_STATE_MASK
+            pm_ctl |= PCI_D0hot
+            self.pci_conf_write32(pos + PCI_PM_CTRL, pm_ctl)
+            if cur_state == PCI_D3hot:
+                time.sleep(0.010)
+            if cur_state == PCI_D2:
+                time.sleep(0.0002)
+
     def do_vendor_specific_FLR_method(self):
         pos = self.find_cap_offset(PCI_CAP_ID_VENDOR_SPECIFIC_CAP)
         if pos == 0:
@@ -543,13 +525,9 @@
         if class_id != PCI_CLASS_ID_USB:
             return
 
-        (pci_list, cfg_list) = save_pci_conf_space([self.name])
-
         self.pci_conf_write8(pos + PCI_USB_FLRCTRL, 1)
         time.sleep(0.100)
 
-        restore_pci_conf_space((pci_list, cfg_list))
-
     def do_FLR_for_integrated_device(self):
         if not self.do_Dstate_transition():
             self.do_vendor_specific_FLR_method()
@@ -725,15 +703,16 @@
     def do_FLR(self):
         """ Perform FLR (Functional Level Reset) for the device.
         """
+        # Set power management state to D0
+        self.do_Dstate_reset()
+
         if self.dev_type == DEV_TYPE_PCIe_ENDPOINT:
             # If PCIe device supports FLR, we use it.
             if self.pcie_flr:
-                (pci_list, cfg_list) = save_pci_conf_space([self.name])
                 pos = self.find_cap_offset(PCI_CAP_ID_EXP)
                 self.pci_conf_write32(pos + PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_FLR)
                 # We must sleep at least 100ms for the completion of FLR
                 time.sleep(0.100)
-                restore_pci_conf_space((pci_list, cfg_list))
             else:
                 if self.bus == 0:
                     self.do_FLR_for_integrated_device()
@@ -749,12 +728,10 @@
         else:
             # For PCI device on host bus, we test "PCI Advanced Capabilities".
             if self.bus == 0 and self.pci_af_flr:
-                (pci_list, cfg_list) = save_pci_conf_space([self.name])
                 # We use Advanced Capability to do FLR.
                 pos = self.find_cap_offset(PCI_CAP_ID_AF)
                 self.pci_conf_write8(pos + PCI_AF_CTL, PCI_AF_CTL_FLR)
                 time.sleep(0.100)
-                restore_pci_conf_space((pci_list, cfg_list))
             else:
                 if self.bus == 0:
                     self.do_FLR_for_integrated_device()
diff -r 94ffd85005c5 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Apr 21 16:51:20 2009 +0900
@@ -764,6 +764,23 @@
         return self.getDeviceController(dev_type).sxpr(devid)
 
 
+    def pci_device_attach(self, dev_sxp):
+        """PCI device attachment.
+        
+        @param dev_sxp: device configuration
+        @type  dev_sxp: SXP object (parsed config)
+        """
+        pci_dev = sxp.children(dev_sxp, 'dev')[0]
+        dev_config = self.info.pci_convert_sxp_to_dict(dev_sxp)
+
+        vslot = self.hvm_pci_device_create(dev_config)
+        # Update vslot
+        dev['vslot'] = vslot
+        for n in sxp.children(pci_dev):
+            if(n[0] == 'vslot'):
+                n[1] = vslot
+
+
     def pci_device_configure(self, dev_sxp, devid = 0):
         """Configure an existing pci device.
         
@@ -794,15 +811,7 @@
                 
         # Do HVM specific processing
         if self.info.is_hvm():
-            if pci_state == 'Initialising':
-                # HVM PCI device attachment
-                vslot = self.hvm_pci_device_create(dev_config)
-                # Update vslot
-                dev['vslot'] = vslot
-                for n in sxp.children(pci_dev):
-                    if(n[0] == 'vslot'):
-                        n[1] = vslot
-            else:
+            if pci_state == 'Closing':
                 # HVM PCI device detachment
                 existing_dev_uuid = sxp.child_value(existing_dev_info, 'uuid')
                 existing_pci_conf = self.info['devices'][existing_dev_uuid][1]
@@ -829,15 +838,22 @@
         # If pci platform does not exist, create and exit.
         if existing_dev_info is None:
             self.device_create(dev_sxp)
+            if self.info.is_hvm():
+                if pci_state == 'Initialising':
+                    # HVM PCI device attachment
+                    self.pci_device_attach(dev_sxp)
             return True
 
         if self.domid is not None:
             # use DevController.reconfigureDevice to change device config
             dev_control = self.getDeviceController(dev_class)
             dev_uuid = dev_control.reconfigureDevice(devid, dev_config)
-            if not self.info.is_hvm():
-                # in PV case, wait until backend state becomes connected.
-                dev_control.waitForDevice_reconfigure(devid)
+            dev_control.waitForDevice_reconfigure(devid)
+
+            if self.info.is_hvm():
+                if pci_state == 'Initialising':
+                    # HVM PCI device attachment
+                    self.pci_device_attach(dev_sxp)
             num_devs = dev_control.cleanupDevice(devid)
 
             # update XendConfig with new device info
diff -r 94ffd85005c5 tools/python/xen/xend/server/DevConstants.py
--- a/tools/python/xen/xend/server/DevConstants.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/server/DevConstants.py	Tue Apr 21 16:51:20 2009 +0900
@@ -29,6 +29,8 @@
 Timeout      = 4
 Busy         = 5
 Disconnected = 6
+Initialised  = 7
+Reconfigured = 8
 
 xenbusState = {
     'Unknown'       : 0,
diff -r 94ffd85005c5 tools/python/xen/xend/server/pciif.py
--- a/tools/python/xen/xend/server/pciif.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/server/pciif.py	Tue Apr 21 16:51:20 2009 +0900
@@ -17,6 +17,7 @@
 #============================================================================
 
 
+from threading import Event
 import types
 import time
 
@@ -27,7 +28,7 @@
 from xen.xend.XendConstants import *
 
 from xen.xend.server.DevController import DevController
-from xen.xend.server.DevConstants import xenbusState
+from xen.xend.server.DevConstants import *
 
 import xen.lowlevel.xc
 
@@ -489,13 +490,16 @@
                     "bind your slot/device to the PCI backend using sysfs" \
                     )%(dev.name))
 
-        if not self.vm.info.is_hvm():
-            pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
-            bdf = xc.deassign_device(fe_domid, pci_str)
-            if bdf > 0:
-                raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
-                              % (bus, slot, func))
-            log.debug("pci: deassign device %x:%x.%x" % (bus, slot, func))
+        # Need to do FLR here before deassign device in order to terminate
+        # DMA transaction, etc
+        dev.do_FLR()
+
+        pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
+        bdf = xc.deassign_device(fe_domid, pci_str)
+        if bdf > 0:
+            raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
+                          % (bus, slot, func))
+        log.debug("pci: Deassign device %x:%x.%x" % (bus, slot, func))
 
         for (start, size) in dev.ioports:
             log.debug('pci: disabling ioport 0x%x/0x%x'%(start,size))
@@ -528,7 +532,6 @@
             if rc<0:
                 raise VmError(('pci: failed to configure irq on device '+
                             '%s - errno=%d')%(dev.name,rc))
-        dev.do_FLR()
 
     def cleanupDevice(self, devid):
         """ Detach I/O resources for device and cleanup xenstore nodes
@@ -603,8 +606,53 @@
         except:
             log.exception("Unwatching aerState failed.")
   
-    def waitForBackend(self,devid):
-        return (0, "ok - no hotplug")
+    def waitForBackend(self, devid):
+        return self.waitForBackend_reconfigure(devid)
 
     def migrate(self, config, network, dst, step, domName):
         raise XendError('Migration not permitted with assigned PCI device.')
+
+    def createDevice(self, config):
+        """@see DevController.createDevice"""
+        devid = DevController.createDevice(self, config)
+        if devid:
+            self.waitForDevice(devid)
+
+        return devid
+
+    def waitForBackend_reconfigure(self, devid):
+        frontpath = self.frontendPath(devid)
+        backpath = xstransact.Read(frontpath, "backend")
+        if backpath:
+            statusPath = backpath + '/' + "state"
+            ev = Event()
+            result = { 'status': Timeout }
+
+            xswatch(statusPath, xenbusStatusCallback, ev, result)
+
+            ev.wait(DEVICE_CREATE_TIMEOUT)
+
+            return (result['status'], None)
+        else:
+            return (Missing, None)
+
+
+def xenbusStatusCallback(statusPath, ev, result):
+    log.debug("xenbusStatusCallback %s.", statusPath)
+
+    status = xstransact.Read(statusPath)
+
+    if status == str(xenbusState['Connected']):
+        result['status'] = Connected
+    elif status == str(xenbusState['Initialised']):
+    	result['status'] = Initialised
+    elif status == str(xenbusState['Reconfigured']):
+    	result['status'] = Reconfigured
+    else:
+        return 1
+
+    log.debug("xenbusStatusCallback %d.", result['status'])
+
+    ev.set()
+    return 0
+

Re: [PATCH 2/3] remove saving/restoring method in Xend.

[Masaki Kanno]

Hi Yuji,

I have two comments.

1. Is the "if" statement necessary?
   I think that the "if" statement is not necessary.
   Because, if existing_dev_info is None, pci_state is "Initialising" 
   certainly.  Please see the line: 790-791 in XendDomainInfo.py.

pci_device_configure@XendDomainInfo.py
@@ -829,15 +838,22 @@
         # If pci platform does not exist, create and exit.
         if existing_dev_info is None:
             self.device_create(dev_sxp)
+            if self.info.is_hvm():
+                if pci_state == 'Initialising':   <--- here
+                    # HVM PCI device attachment
+                    self.pci_device_attach(dev_sxp)
             return True

pci_device_configure@XendDomainInfo.py
line:790-791
        if existing_dev_info is None and pci_state != 'Initialising':
            raise XendError("Cannot detach when pci platform does not exist")


2. Tab indents are included.

+def xenbusStatusCallback(statusPath, ev, result):
+    log.debug("xenbusStatusCallback %s.", statusPath)
+
+    status = xstransact.Read(statusPath)
+
+    if status == str(xenbusState['Connected']):
+        result['status'] = Connected
+    elif status == str(xenbusState['Initialised']):
+    	result['status'] = Initialised                 <---- here
+    elif status == str(xenbusState['Reconfigured']):
+    	result['status'] = Reconfigured                <---- here
+    else:
+        return 1

Best regards,
 Kan

Wed, 22 Apr 2009 11:20:21 +0900, Yuji Shimada wrote:

>This patch removes Xend method which saves/restores PCI configuration
>space.
>And this patch modifies the timing of saving/restoring configuration
>space like below.
>
>  When pciback is bound to devices.
>   - Pciback saves configuration space.
>
>  When pciback is unbound to devices.
>   - Pciback restores configuration space.
>
>  When guest OS boots or a device is hotadded.
>   - Pciback restores configuration space.
>   - Pciback changes state of backend device to Initialised/Reconfigured.
>   - Xend waits for the transition to Initialised/Reconfigured.
>
>  When guest OS shutdowns or a device is hotremoved.
>   - Pciback restores configuration space.
>   - Xend resets devices.
>     * If D-state of the device is not D0, the state is changed to D0
>       before resetting the device.
>   - Xend deassigns devices.
>
>  * Essentially, devices should be reset before configuration space is
>    restored. But it needs big modifications. Applying these patches,
>    configuration space is restored when guest OS boots, a device is
>    hotadded or pciback is unbound. So it has no matter.
>
>Thanks,
>--
>Yuji Shimada
>
>
>Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
>
>diff -r 94ffd85005c5 tools/python/xen/util/pci.py
>--- a/tools/python/xen/util/pci.py	Tue Apr 14 15:23:53 2009 +0100
>+++ b/tools/python/xen/util/pci.py	Tue Apr 21 16:51:20 2009 +0900
>@@ -70,6 +70,7 @@
> PCI_PM_CTRL_NO_SOFT_RESET = 0x0008
> PCI_PM_CTRL_STATE_MASK = 0x0003
> PCI_D3hot = 3
>+PCI_D2    = 2
> PCI_D0hot = 0
> 
> VENDOR_INTEL  = 0x8086
>@@ -209,33 +210,6 @@
>     finally:
>         lspci_info_lock.release()
> 
>-def save_pci_conf_space(devs_string):
>-    pci_list = []
>-    cfg_list = []
>-    sysfs_mnt = find_sysfs_mnt()
>-    for pci_str in devs_string:
>-        pci_path = sysfs_mnt + SYSFS_PCI_DEVS_PATH + '/' + pci_str + \
>-                SYSFS_PCI_DEV_CONFIG_PATH
>-        fd = os.open(pci_path, os.O_RDONLY)
>-        configs = []
>-        for i in range(0, 256, 4):
>-            configs = configs + [os.read(fd,4)]
>-        os.close(fd)
>-        pci_list = pci_list + [pci_path]
>-        cfg_list = cfg_list + [configs]
>-    return (pci_list, cfg_list)
>-
>-def restore_pci_conf_space(pci_cfg_list):
>-    pci_list = pci_cfg_list[0]
>-    cfg_list = pci_cfg_list[1]
>-    for i in range(0, len(pci_list)):
>-        pci_path = pci_list[i]
>-        configs  = cfg_list[i]
>-        fd = os.open(pci_path, os.O_WRONLY)
>-        for dw in configs:
>-            os.write(fd, dw)
>-        os.close(fd) 
>-
> def find_all_devices_owned_by_pciback():
>     sysfs_mnt = find_sysfs_mnt()
>     pciback_path = sysfs_mnt + SYSFS_PCIBACK_PATH
>@@ -476,9 +450,6 @@
>         return dev_list
> 
>     def do_secondary_bus_reset(self, target_bus, devs):
>-        # Save the config spaces of all the devices behind the bus.
>-        (pci_list, cfg_list) = save_pci_conf_space(devs)
>-        
>         #Do the Secondary Bus Reset
>         sysfs_mnt = find_sysfs_mnt()
>         parent_path = sysfs_mnt + SYSFS_PCI_DEVS_PATH + '/' + \
>@@ -498,9 +469,6 @@
>         time.sleep(0.100)
>         os.close(fd)
> 
>-        # Restore the config spaces
>-        restore_pci_conf_space((pci_list, cfg_list))
>-        
>     def do_Dstate_transition(self):
>         pos = self.find_cap_offset(PCI_CAP_ID_PM)
>         if pos == 0:
>@@ -513,8 +481,6 @@
>         if (pm_ctl & PCI_PM_CTRL_NO_SOFT_RESET) == 1:
>             return False
> 
>-        (pci_list, cfg_list) = save_pci_conf_space([self.name])
>-        
>         # Enter D3hot
>         pm_ctl &= ~PCI_PM_CTRL_STATE_MASK
>         pm_ctl |= PCI_D3hot
>@@ -527,9 +493,25 @@
>         self.pci_conf_write32(pos + PCI_PM_CTRL, pm_ctl)
>         time.sleep(0.010)
> 
>-        restore_pci_conf_space((pci_list, cfg_list))
>         return True
> 
>+    def do_Dstate_reset(self):
>+        pos = self.find_cap_offset(PCI_CAP_ID_PM)
>+        if pos == 0:
>+            return
>+
>+        pm_ctl = self.pci_conf_read32(pos + PCI_PM_CTRL)
>+        cur_state = pm_ctl & PCI_PM_CTRL_STATE_MASK
>+        if (cur_state) != 0:
>+            # From not D0 to D0
>+            pm_ctl &= ~PCI_PM_CTRL_STATE_MASK
>+            pm_ctl |= PCI_D0hot
>+            self.pci_conf_write32(pos + PCI_PM_CTRL, pm_ctl)
>+            if cur_state == PCI_D3hot:
>+                time.sleep(0.010)
>+            if cur_state == PCI_D2:
>+                time.sleep(0.0002)
>+
>     def do_vendor_specific_FLR_method(self):
>         pos = self.find_cap_offset(PCI_CAP_ID_VENDOR_SPECIFIC_CAP)
>         if pos == 0:
>@@ -543,13 +525,9 @@
>         if class_id != PCI_CLASS_ID_USB:
>             return
> 
>-        (pci_list, cfg_list) = save_pci_conf_space([self.name])
>-
>         self.pci_conf_write8(pos + PCI_USB_FLRCTRL, 1)
>         time.sleep(0.100)
> 
>-        restore_pci_conf_space((pci_list, cfg_list))
>-
>     def do_FLR_for_integrated_device(self):
>         if not self.do_Dstate_transition():
>             self.do_vendor_specific_FLR_method()
>@@ -725,15 +703,16 @@
>     def do_FLR(self):
>         """ Perform FLR (Functional Level Reset) for the device.
>         """
>+        # Set power management state to D0
>+        self.do_Dstate_reset()
>+
>         if self.dev_type == DEV_TYPE_PCIe_ENDPOINT:
>             # If PCIe device supports FLR, we use it.
>             if self.pcie_flr:
>-                (pci_list, cfg_list) = save_pci_conf_space([self.name])
>                 pos = self.find_cap_offset(PCI_CAP_ID_EXP)
>                 self.pci_conf_write32(pos + PCI_EXP_DEVCTL, 
>PCI_EXP_DEVCTL_FLR)
>                 # We must sleep at least 100ms for the completion of FLR
>                 time.sleep(0.100)
>-                restore_pci_conf_space((pci_list, cfg_list))
>             else:
>                 if self.bus == 0:
>                     self.do_FLR_for_integrated_device()
>@@ -749,12 +728,10 @@
>         else:
>             # For PCI device on host bus, we test "PCI Advanced 
>Capabilities".
>             if self.bus == 0 and self.pci_af_flr:
>-                (pci_list, cfg_list) = save_pci_conf_space([self.name])
>                 # We use Advanced Capability to do FLR.
>                 pos = self.find_cap_offset(PCI_CAP_ID_AF)
>                 self.pci_conf_write8(pos + PCI_AF_CTL, PCI_AF_CTL_FLR)
>                 time.sleep(0.100)
>-                restore_pci_conf_space((pci_list, cfg_list))
>             else:
>                 if self.bus == 0:
>                     self.do_FLR_for_integrated_device()
>diff -r 94ffd85005c5 tools/python/xen/xend/XendDomainInfo.py
>--- a/tools/python/xen/xend/XendDomainInfo.py	Tue Apr 14 15:23:53 2009 +0100
>+++ b/tools/python/xen/xend/XendDomainInfo.py	Tue Apr 21 16:51:20 2009 +0900
>@@ -764,6 +764,23 @@
>         return self.getDeviceController(dev_type).sxpr(devid)
> 
> 
>+    def pci_device_attach(self, dev_sxp):
>+        """PCI device attachment.
>+        
>+        @param dev_sxp: device configuration
>+        @type  dev_sxp: SXP object (parsed config)
>+        """
>+        pci_dev = sxp.children(dev_sxp, 'dev')[0]
>+        dev_config = self.info.pci_convert_sxp_to_dict(dev_sxp)
>+
>+        vslot = self.hvm_pci_device_create(dev_config)
>+        # Update vslot
>+        dev['vslot'] = vslot
>+        for n in sxp.children(pci_dev):
>+            if(n[0] == 'vslot'):
>+                n[1] = vslot
>+
>+
>     def pci_device_configure(self, dev_sxp, devid = 0):
>         """Configure an existing pci device.
>         
>@@ -794,15 +811,7 @@
>                 
>         # Do HVM specific processing
>         if self.info.is_hvm():
>-            if pci_state == 'Initialising':
>-                # HVM PCI device attachment
>-                vslot = self.hvm_pci_device_create(dev_config)
>-                # Update vslot
>-                dev['vslot'] = vslot
>-                for n in sxp.children(pci_dev):
>-                    if(n[0] == 'vslot'):
>-                        n[1] = vslot
>-            else:
>+            if pci_state == 'Closing':
>                 # HVM PCI device detachment
>                 existing_dev_uuid = sxp.child_value(existing_dev_info, 
>'uuid')
>                 existing_pci_conf = self.info['devices'][existing_dev_uuid
>][1]
>@@ -829,15 +838,22 @@
>         # If pci platform does not exist, create and exit.
>         if existing_dev_info is None:
>             self.device_create(dev_sxp)
>+            if self.info.is_hvm():
>+                if pci_state == 'Initialising':
>+                    # HVM PCI device attachment
>+                    self.pci_device_attach(dev_sxp)
>             return True
> 
>         if self.domid is not None:
>             # use DevController.reconfigureDevice to change device config
>             dev_control = self.getDeviceController(dev_class)
>             dev_uuid = dev_control.reconfigureDevice(devid, dev_config)
>-            if not self.info.is_hvm():
>-                # in PV case, wait until backend state becomes connected.
>-                dev_control.waitForDevice_reconfigure(devid)
>+            dev_control.waitForDevice_reconfigure(devid)
>+
>+            if self.info.is_hvm():
>+                if pci_state == 'Initialising':
>+                    # HVM PCI device attachment
>+                    self.pci_device_attach(dev_sxp)
>             num_devs = dev_control.cleanupDevice(devid)
> 
>             # update XendConfig with new device info
>diff -r 94ffd85005c5 tools/python/xen/xend/server/DevConstants.py
>--- a/tools/python/xen/xend/server/DevConstants.py	Tue Apr 14 15:23:53 2009
> +0100
>+++ b/tools/python/xen/xend/server/DevConstants.py	Tue Apr 21 16:51:20 2009
> +0900
>@@ -29,6 +29,8 @@
> Timeout      = 4
> Busy         = 5
> Disconnected = 6
>+Initialised  = 7
>+Reconfigured = 8
> 
> xenbusState = {
>     'Unknown'       : 0,
>diff -r 94ffd85005c5 tools/python/xen/xend/server/pciif.py
>--- a/tools/python/xen/xend/server/pciif.py	Tue Apr 14 15:23:53 2009 +0100
>+++ b/tools/python/xen/xend/server/pciif.py	Tue Apr 21 16:51:20 2009 +0900
>@@ -17,6 +17,7 @@
> #=========================================================================
>===
> 
> 
>+from threading import Event
> import types
> import time
> 
>@@ -27,7 +28,7 @@
> from xen.xend.XendConstants import *
> 
> from xen.xend.server.DevController import DevController
>-from xen.xend.server.DevConstants import xenbusState
>+from xen.xend.server.DevConstants import *
> 
> import xen.lowlevel.xc
> 
>@@ -489,13 +490,16 @@
>                     "bind your slot/device to the PCI backend using sysfs" \
>                     )%(dev.name))
> 
>-        if not self.vm.info.is_hvm():
>-            pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
>-            bdf = xc.deassign_device(fe_domid, pci_str)
>-            if bdf > 0:
>-                raise VmError("Failed to deassign device from IOMMU (%x:%x
>.%x)"
>-                              % (bus, slot, func))
>-            log.debug("pci: deassign device %x:%x.%x" % (bus, slot, func))
>+        # Need to do FLR here before deassign device in order to terminate
>+        # DMA transaction, etc
>+        dev.do_FLR()
>+
>+        pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
>+        bdf = xc.deassign_device(fe_domid, pci_str)
>+        if bdf > 0:
>+            raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
>+                          % (bus, slot, func))
>+        log.debug("pci: Deassign device %x:%x.%x" % (bus, slot, func))
> 
>         for (start, size) in dev.ioports:
>             log.debug('pci: disabling ioport 0x%x/0x%x'%(start,size))
>@@ -528,7 +532,6 @@
>             if rc<0:
>                 raise VmError(('pci: failed to configure irq on device '+
>                             '%s - errno=%d')%(dev.name,rc))
>-        dev.do_FLR()
> 
>     def cleanupDevice(self, devid):
>         """ Detach I/O resources for device and cleanup xenstore nodes
>@@ -603,8 +606,53 @@
>         except:
>             log.exception("Unwatching aerState failed.")
>   
>-    def waitForBackend(self,devid):
>-        return (0, "ok - no hotplug")
>+    def waitForBackend(self, devid):
>+        return self.waitForBackend_reconfigure(devid)
> 
>     def migrate(self, config, network, dst, step, domName):
>         raise XendError('Migration not permitted with assigned PCI device.')
>+
>+    def createDevice(self, config):
>+        """@see DevController.createDevice"""
>+        devid = DevController.createDevice(self, config)
>+        if devid:
>+            self.waitForDevice(devid)
>+
>+        return devid
>+
>+    def waitForBackend_reconfigure(self, devid):
>+        frontpath = self.frontendPath(devid)
>+        backpath = xstransact.Read(frontpath, "backend")
>+        if backpath:
>+            statusPath = backpath + '/' + "state"
>+            ev = Event()
>+            result = { 'status': Timeout }
>+
>+            xswatch(statusPath, xenbusStatusCallback, ev, result)
>+
>+            ev.wait(DEVICE_CREATE_TIMEOUT)
>+
>+            return (result['status'], None)
>+        else:
>+            return (Missing, None)
>+
>+
>+def xenbusStatusCallback(statusPath, ev, result):
>+    log.debug("xenbusStatusCallback %s.", statusPath)
>+
>+    status = xstransact.Read(statusPath)
>+
>+    if status == str(xenbusState['Connected']):
>+        result['status'] = Connected
>+    elif status == str(xenbusState['Initialised']):
>+    	result['status'] = Initialised
>+    elif status == str(xenbusState['Reconfigured']):
>+    	result['status'] = Reconfigured
>+    else:
>+        return 1
>+
>+    log.debug("xenbusStatusCallback %d.", result['status'])
>+
>+    ev.set()
>+    return 0
>+
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel

[PATCH 2/3 v2] remove saving/restoring method in Xend.

[Yuji Shimada]

Hi, Kanno-san

Thanks for your review. I fixed the issue that you pointed out.

This patch removes Xend method which saves/restores PCI configuration
space.
And this patch modifies the timing of saving/restoring configuration
space like below.

  When pciback is bound to devices.
   - Pciback saves configuration space.

  When pciback is unbound to devices.
   - Pciback restores configuration space.

  When guest OS boots or a device is hotadded.
   - Pciback restores configuration space.
   - Pciback changes state of backend device to Initialised/Reconfigured.
   - Xend waits for the transition to Initialised/Reconfigured.

  When guest OS shutdowns or a device is hotremoved.
   - Pciback restores configuration space.
   - Xend resets devices.
     * If D-state of the device is not D0, the state is changed to D0
       before resetting the device.
   - Xend deassigns devices.

  * Essentially, devices should be reset before configuration space is
    restored. But it needs big modifications. Applying these patches,
    configuration space is restored when guest OS boots, a device is
    hotadded or pciback is unbound. So it has no matter.

Thanks,
--
Yuji Shimada


Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

diff -r 94ffd85005c5 tools/python/xen/util/pci.py
--- a/tools/python/xen/util/pci.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/util/pci.py	Wed Apr 22 14:27:15 2009 +0900
@@ -70,6 +70,7 @@
 PCI_PM_CTRL_NO_SOFT_RESET = 0x0008
 PCI_PM_CTRL_STATE_MASK = 0x0003
 PCI_D3hot = 3
+PCI_D2    = 2
 PCI_D0hot = 0
 
 VENDOR_INTEL  = 0x8086
@@ -209,33 +210,6 @@
     finally:
         lspci_info_lock.release()
 
-def save_pci_conf_space(devs_string):
-    pci_list = []
-    cfg_list = []
-    sysfs_mnt = find_sysfs_mnt()
-    for pci_str in devs_string:
-        pci_path = sysfs_mnt + SYSFS_PCI_DEVS_PATH + '/' + pci_str + \
-                SYSFS_PCI_DEV_CONFIG_PATH
-        fd = os.open(pci_path, os.O_RDONLY)
-        configs = []
-        for i in range(0, 256, 4):
-            configs = configs + [os.read(fd,4)]
-        os.close(fd)
-        pci_list = pci_list + [pci_path]
-        cfg_list = cfg_list + [configs]
-    return (pci_list, cfg_list)
-
-def restore_pci_conf_space(pci_cfg_list):
-    pci_list = pci_cfg_list[0]
-    cfg_list = pci_cfg_list[1]
-    for i in range(0, len(pci_list)):
-        pci_path = pci_list[i]
-        configs  = cfg_list[i]
-        fd = os.open(pci_path, os.O_WRONLY)
-        for dw in configs:
-            os.write(fd, dw)
-        os.close(fd) 
-
 def find_all_devices_owned_by_pciback():
     sysfs_mnt = find_sysfs_mnt()
     pciback_path = sysfs_mnt + SYSFS_PCIBACK_PATH
@@ -476,9 +450,6 @@
         return dev_list
 
     def do_secondary_bus_reset(self, target_bus, devs):
-        # Save the config spaces of all the devices behind the bus.
-        (pci_list, cfg_list) = save_pci_conf_space(devs)
-        
         #Do the Secondary Bus Reset
         sysfs_mnt = find_sysfs_mnt()
         parent_path = sysfs_mnt + SYSFS_PCI_DEVS_PATH + '/' + \
@@ -498,9 +469,6 @@
         time.sleep(0.100)
         os.close(fd)
 
-        # Restore the config spaces
-        restore_pci_conf_space((pci_list, cfg_list))
-        
     def do_Dstate_transition(self):
         pos = self.find_cap_offset(PCI_CAP_ID_PM)
         if pos == 0:
@@ -513,8 +481,6 @@
         if (pm_ctl & PCI_PM_CTRL_NO_SOFT_RESET) == 1:
             return False
 
-        (pci_list, cfg_list) = save_pci_conf_space([self.name])
-        
         # Enter D3hot
         pm_ctl &= ~PCI_PM_CTRL_STATE_MASK
         pm_ctl |= PCI_D3hot
@@ -527,9 +493,25 @@
         self.pci_conf_write32(pos + PCI_PM_CTRL, pm_ctl)
         time.sleep(0.010)
 
-        restore_pci_conf_space((pci_list, cfg_list))
         return True
 
+    def do_Dstate_reset(self):
+        pos = self.find_cap_offset(PCI_CAP_ID_PM)
+        if pos == 0:
+            return
+
+        pm_ctl = self.pci_conf_read32(pos + PCI_PM_CTRL)
+        cur_state = pm_ctl & PCI_PM_CTRL_STATE_MASK
+        if (cur_state) != 0:
+            # From not D0 to D0
+            pm_ctl &= ~PCI_PM_CTRL_STATE_MASK
+            pm_ctl |= PCI_D0hot
+            self.pci_conf_write32(pos + PCI_PM_CTRL, pm_ctl)
+            if cur_state == PCI_D3hot:
+                time.sleep(0.010)
+            if cur_state == PCI_D2:
+                time.sleep(0.0002)
+
     def do_vendor_specific_FLR_method(self):
         pos = self.find_cap_offset(PCI_CAP_ID_VENDOR_SPECIFIC_CAP)
         if pos == 0:
@@ -543,13 +525,9 @@
         if class_id != PCI_CLASS_ID_USB:
             return
 
-        (pci_list, cfg_list) = save_pci_conf_space([self.name])
-
         self.pci_conf_write8(pos + PCI_USB_FLRCTRL, 1)
         time.sleep(0.100)
 
-        restore_pci_conf_space((pci_list, cfg_list))
-
     def do_FLR_for_integrated_device(self):
         if not self.do_Dstate_transition():
             self.do_vendor_specific_FLR_method()
@@ -725,15 +703,16 @@
     def do_FLR(self):
         """ Perform FLR (Functional Level Reset) for the device.
         """
+        # Set power management state to D0
+        self.do_Dstate_reset()
+
         if self.dev_type == DEV_TYPE_PCIe_ENDPOINT:
             # If PCIe device supports FLR, we use it.
             if self.pcie_flr:
-                (pci_list, cfg_list) = save_pci_conf_space([self.name])
                 pos = self.find_cap_offset(PCI_CAP_ID_EXP)
                 self.pci_conf_write32(pos + PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_FLR)
                 # We must sleep at least 100ms for the completion of FLR
                 time.sleep(0.100)
-                restore_pci_conf_space((pci_list, cfg_list))
             else:
                 if self.bus == 0:
                     self.do_FLR_for_integrated_device()
@@ -749,12 +728,10 @@
         else:
             # For PCI device on host bus, we test "PCI Advanced Capabilities".
             if self.bus == 0 and self.pci_af_flr:
-                (pci_list, cfg_list) = save_pci_conf_space([self.name])
                 # We use Advanced Capability to do FLR.
                 pos = self.find_cap_offset(PCI_CAP_ID_AF)
                 self.pci_conf_write8(pos + PCI_AF_CTL, PCI_AF_CTL_FLR)
                 time.sleep(0.100)
-                restore_pci_conf_space((pci_list, cfg_list))
             else:
                 if self.bus == 0:
                     self.do_FLR_for_integrated_device()
diff -r 94ffd85005c5 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Wed Apr 22 14:27:15 2009 +0900
@@ -764,6 +764,23 @@
         return self.getDeviceController(dev_type).sxpr(devid)
 
 
+    def pci_device_attach(self, dev_sxp):
+        """PCI device attachment.
+        
+        @param dev_sxp: device configuration
+        @type  dev_sxp: SXP object (parsed config)
+        """
+        pci_dev = sxp.children(dev_sxp, 'dev')[0]
+        dev_config = self.info.pci_convert_sxp_to_dict(dev_sxp)
+
+        vslot = self.hvm_pci_device_create(dev_config)
+        # Update vslot
+        dev['vslot'] = vslot
+        for n in sxp.children(pci_dev):
+            if(n[0] == 'vslot'):
+                n[1] = vslot
+
+
     def pci_device_configure(self, dev_sxp, devid = 0):
         """Configure an existing pci device.
         
@@ -794,15 +811,7 @@
                 
         # Do HVM specific processing
         if self.info.is_hvm():
-            if pci_state == 'Initialising':
-                # HVM PCI device attachment
-                vslot = self.hvm_pci_device_create(dev_config)
-                # Update vslot
-                dev['vslot'] = vslot
-                for n in sxp.children(pci_dev):
-                    if(n[0] == 'vslot'):
-                        n[1] = vslot
-            else:
+            if pci_state == 'Closing':
                 # HVM PCI device detachment
                 existing_dev_uuid = sxp.child_value(existing_dev_info, 'uuid')
                 existing_pci_conf = self.info['devices'][existing_dev_uuid][1]
@@ -829,15 +838,21 @@
         # If pci platform does not exist, create and exit.
         if existing_dev_info is None:
             self.device_create(dev_sxp)
+            if self.info.is_hvm():
+                # HVM PCI device attachment
+                self.pci_device_attach(dev_sxp)
             return True
 
         if self.domid is not None:
             # use DevController.reconfigureDevice to change device config
             dev_control = self.getDeviceController(dev_class)
             dev_uuid = dev_control.reconfigureDevice(devid, dev_config)
-            if not self.info.is_hvm():
-                # in PV case, wait until backend state becomes connected.
-                dev_control.waitForDevice_reconfigure(devid)
+            dev_control.waitForDevice_reconfigure(devid)
+
+            if self.info.is_hvm():
+                if pci_state == 'Initialising':
+                    # HVM PCI device attachment
+                    self.pci_device_attach(dev_sxp)
             num_devs = dev_control.cleanupDevice(devid)
 
             # update XendConfig with new device info
diff -r 94ffd85005c5 tools/python/xen/xend/server/DevConstants.py
--- a/tools/python/xen/xend/server/DevConstants.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/server/DevConstants.py	Wed Apr 22 14:27:15 2009 +0900
@@ -29,6 +29,8 @@
 Timeout      = 4
 Busy         = 5
 Disconnected = 6
+Initialised  = 7
+Reconfigured = 8
 
 xenbusState = {
     'Unknown'       : 0,
diff -r 94ffd85005c5 tools/python/xen/xend/server/pciif.py
--- a/tools/python/xen/xend/server/pciif.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/server/pciif.py	Wed Apr 22 14:27:15 2009 +0900
@@ -17,6 +17,7 @@
 #============================================================================
 
 
+from threading import Event
 import types
 import time
 
@@ -27,7 +28,7 @@
 from xen.xend.XendConstants import *
 
 from xen.xend.server.DevController import DevController
-from xen.xend.server.DevConstants import xenbusState
+from xen.xend.server.DevConstants import *
 
 import xen.lowlevel.xc
 
@@ -489,13 +490,16 @@
                     "bind your slot/device to the PCI backend using sysfs" \
                     )%(dev.name))
 
-        if not self.vm.info.is_hvm():
-            pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
-            bdf = xc.deassign_device(fe_domid, pci_str)
-            if bdf > 0:
-                raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
-                              % (bus, slot, func))
-            log.debug("pci: deassign device %x:%x.%x" % (bus, slot, func))
+        # Need to do FLR here before deassign device in order to terminate
+        # DMA transaction, etc
+        dev.do_FLR()
+
+        pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
+        bdf = xc.deassign_device(fe_domid, pci_str)
+        if bdf > 0:
+            raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
+                          % (bus, slot, func))
+        log.debug("pci: Deassign device %x:%x.%x" % (bus, slot, func))
 
         for (start, size) in dev.ioports:
             log.debug('pci: disabling ioport 0x%x/0x%x'%(start,size))
@@ -528,7 +532,6 @@
             if rc<0:
                 raise VmError(('pci: failed to configure irq on device '+
                             '%s - errno=%d')%(dev.name,rc))
-        dev.do_FLR()
 
     def cleanupDevice(self, devid):
         """ Detach I/O resources for device and cleanup xenstore nodes
@@ -603,8 +606,53 @@
         except:
             log.exception("Unwatching aerState failed.")
   
-    def waitForBackend(self,devid):
-        return (0, "ok - no hotplug")
+    def waitForBackend(self, devid):
+        return self.waitForBackend_reconfigure(devid)
 
     def migrate(self, config, network, dst, step, domName):
         raise XendError('Migration not permitted with assigned PCI device.')
+
+    def createDevice(self, config):
+        """@see DevController.createDevice"""
+        devid = DevController.createDevice(self, config)
+        if devid:
+            self.waitForDevice(devid)
+
+        return devid
+
+    def waitForBackend_reconfigure(self, devid):
+        frontpath = self.frontendPath(devid)
+        backpath = xstransact.Read(frontpath, "backend")
+        if backpath:
+            statusPath = backpath + '/' + "state"
+            ev = Event()
+            result = { 'status': Timeout }
+
+            xswatch(statusPath, xenbusStatusCallback, ev, result)
+
+            ev.wait(DEVICE_CREATE_TIMEOUT)
+
+            return (result['status'], None)
+        else:
+            return (Missing, None)
+
+
+def xenbusStatusCallback(statusPath, ev, result):
+    log.debug("xenbusStatusCallback %s.", statusPath)
+
+    status = xstransact.Read(statusPath)
+
+    if status == str(xenbusState['Connected']):
+        result['status'] = Connected
+    elif status == str(xenbusState['Initialised']):
+        result['status'] = Initialised
+    elif status == str(xenbusState['Reconfigured']):
+        result['status'] = Reconfigured
+    else:
+        return 1
+
+    log.debug("xenbusStatusCallback %d.", result['status'])
+
+    ev.set()
+    return 0
+

RE: [PATCH 2/3] remove saving/restoring method in Xend.

[Ian Pratt]

> This patch removes Xend method which saves/restores PCI configuration
> space.
> And this patch modifies the timing of saving/restoring configuration
> space like below.
> 
>   When pciback is bound to devices.
>    - Pciback saves configuration space.
> 
>   When pciback is unbound to devices.
>    - Pciback restores configuration space.
> 
>   When guest OS boots or a device is hotadded.
>    - Pciback restores configuration space.
>    - Pciback changes state of backend device to
> Initialised/Reconfigured.
>    - Xend waits for the transition to Initialised/Reconfigured.
> 
>   When guest OS shutdowns or a device is hotremoved.
>    - Pciback restores configuration space.
>    - Xend resets devices.
>      * If D-state of the device is not D0, the state is changed to D0
>        before resetting the device.
>    - Xend deassigns devices.

Does it not make sense to have pciback do the reset too? I think I've seen draft patches from Ross Philipson to move the reset into pciback -- they may even be checked in to the xenbits.xen.org/xenclient tree.

Thanks,
Ian

Re: [PATCH 2/3] remove saving/restoring method in Xend.

[Yuji Shimada]

On Wed, 22 Apr 2009 11:38:13 +0100
Ian Pratt <Ian.Pratt@eu.citrix.com> wrote:
> Does it not make sense to have pciback do the reset too? I think I've seen draft patches from Ross Philipson to move the reset into pciback -- they may even be checked in to the xenbits.xen.org/xenclient tree.

Hi, Ian

Of course, it is better to reset in pciback. So Ross's patches seems
to be nice. But as the future freeze is announced for Xen 3.4, I think
these patches are too big to merge with it. I think it is better to
merge these patches after Xen 3.4 is released.

My patches are sufficient enough to make Xen 3.4 robust in resetting/
restoring devices.

Thanks,
--
Yuji Shimada

Re: [PATCH 2/3] remove saving/restoring method in Xend.

[Keir Fraser]

On 23/04/2009 03:57, "Yuji Shimada" <shimada-yxb@necst.nec.co.jp> wrote:

> Of course, it is better to reset in pciback. So Ross's patches seems
> to be nice. But as the future freeze is announced for Xen 3.4, I think
> these patches are too big to merge with it. I think it is better to
> merge these patches after Xen 3.4 is released.
> 
> My patches are sufficient enough to make Xen 3.4 robust in resetting/
> restoring devices.

Your patch is rather big for 3.4 at this stage. I wasn't going to check it
in before the branch. Do people want them enough to shove them in? I'm
certainly not keen.

 -- Keir

Re: [PATCH 2/3] remove saving/restoring method in Xend.

[Yuji Shimada]

On Thu, 23 Apr 2009 08:22:49 +0100
Keir Fraser <keir.fraser@eu.citrix.com> wrote:

> On 23/04/2009 03:57, "Yuji Shimada" <shimada-yxb@necst.nec.co.jp> wrote:
> 
> > Of course, it is better to reset in pciback. So Ross's patches seems
> > to be nice. But as the future freeze is announced for Xen 3.4, I think
> > these patches are too big to merge with it. I think it is better to
> > merge these patches after Xen 3.4 is released.
> > 
> > My patches are sufficient enough to make Xen 3.4 robust in resetting/
> > restoring devices.
> 
> Your patch is rather big for 3.4 at this stage. I wasn't going to check it
> in before the branch. Do people want them enough to shove them in? I'm
> certainly not keen.
> 
>  -- Keir
> 

Hi, Keir & Ian

OK, I understand that my patches are also big. But I'm worried about
one problem for Xen 3.4. The problem is that devices are deassigned
before the reset on current Xen. Dom0 memory may be broken by DMA if
the problem is not fixed.
My patches fix the problem. I mean that devices are deassigned after
the reset with my patches.

I can work on making the patch just changing order of resetting/
deassigning devices if you need the fix for Xen 3.4.

Thanks,
--
Yuji Shimada

Re: [PATCH 2/3] remove saving/restoring method in Xend.

[Keir Fraser]

On 23/04/2009 09:27, "Yuji Shimada" <shimada-yxb@necst.nec.co.jp> wrote:

> OK, I understand that my patches are also big. But I'm worried about
> one problem for Xen 3.4. The problem is that devices are deassigned
> before the reset on current Xen. Dom0 memory may be broken by DMA if
> the problem is not fixed.
> My patches fix the problem. I mean that devices are deassigned after
> the reset with my patches.
> 
> I can work on making the patch just changing order of resetting/
> deassigning devices if you need the fix for Xen 3.4.

If we stick with the existing pciback mechanisms and interfaces for 3.4
(which is preferable at this point) will we simply break 3.4 branch as soon
as the new pciback patches go into linux-2.6.18-xen when 3.5 opens? Or is
there a story on backward compatibility between different pciback/xend
versions?

 -- Keir

Re: [PATCH 2/3] remove saving/restoring method in Xend.

[Yuji Shimada]

On Thu, 23 Apr 2009 10:01:02 +0100
Keir Fraser <keir.fraser@eu.citrix.com> wrote:

> On 23/04/2009 09:27, "Yuji Shimada" <shimada-yxb@necst.nec.co.jp> wrote:
> 
> > OK, I understand that my patches are also big. But I'm worried about
> > one problem for Xen 3.4. The problem is that devices are deassigned
> > before the reset on current Xen. Dom0 memory may be broken by DMA if
> > the problem is not fixed.
> > My patches fix the problem. I mean that devices are deassigned after
> > the reset with my patches.
> > 
> > I can work on making the patch just changing order of resetting/
> > deassigning devices if you need the fix for Xen 3.4.
> 
> If we stick with the existing pciback mechanisms and interfaces for 3.4
> (which is preferable at this point) will we simply break 3.4 branch as soon
> as the new pciback patches go into linux-2.6.18-xen when 3.5 opens? Or is
> there a story on backward compatibility between different pciback/xend
> versions?

Hi, Keir

I don't have any idea for backward compatibility between different
pciback/xend versions.
I give up being applied to linux-2.6.18-xen.

I will sent the patch to change the order of resetting/deassigning
device for Xen 3.4.

Thanks,
--
Yuji Shimada

[PATCH 0/2] modify the order of resetting/deassigning device.

[Yuji Shimada]

This series of patches modify the order of resetting/deassigning
device.

I modify the order of resetting/deassigning device like below.

  When guest OS shutdowns or a device is hotremoved.
   1. Xend resets devices.
   2. Xend deassigns devices.

Because if devices are deassigned before the reset, dom0 memory may be
broken by DMA

Thanks,
--
Yuji Shimada

[PATCH 1/2] modify the order of resetting/deassigning device.

[Yuji Shimada]

This patch modifies the order of resetting/deassigning device like
below.

  When guest OS shutdowns or a device is hotremoved.
   1. Xend resets devices.
   2. Xend deassigns devices.

Because if devices are deassigned before the reset, dom0 memory may be
broken by DMA

Thanks,
--
Yuji Shimada


Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

diff -r 94ffd85005c5 tools/python/xen/xend/server/pciif.py
--- a/tools/python/xen/xend/server/pciif.py	Tue Apr 14 15:23:53 2009 +0100
+++ b/tools/python/xen/xend/server/pciif.py	Fri Apr 24 14:17:00 2009 +0900
@@ -489,13 +489,16 @@
                     "bind your slot/device to the PCI backend using sysfs" \
                     )%(dev.name))
 
-        if not self.vm.info.is_hvm():
-            pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
-            bdf = xc.deassign_device(fe_domid, pci_str)
-            if bdf > 0:
-                raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
-                              % (bus, slot, func))
-            log.debug("pci: deassign device %x:%x.%x" % (bus, slot, func))
+        # Need to do FLR here before deassign device in order to terminate
+        # DMA transaction, etc
+        dev.do_FLR()
+
+        pci_str = "0x%x, 0x%x, 0x%x, 0x%x" % (domain, bus, slot, func)
+        bdf = xc.deassign_device(fe_domid, pci_str)
+        if bdf > 0:
+            raise VmError("Failed to deassign device from IOMMU (%x:%x.%x)"
+                          % (bus, slot, func))
+        log.debug("pci: Deassign device %x:%x.%x" % (bus, slot, func))
 
         for (start, size) in dev.ioports:
             log.debug('pci: disabling ioport 0x%x/0x%x'%(start,size))
@@ -528,7 +531,6 @@
             if rc<0:
                 raise VmError(('pci: failed to configure irq on device '+
                             '%s - errno=%d')%(dev.name,rc))
-        dev.do_FLR()
 
     def cleanupDevice(self, devid):
         """ Detach I/O resources for device and cleanup xenstore nodes

[PATCH 2/2] ioemu: don't call xc_deassign_device().

[Yuji Shimada]

This patch modifies ioemu not to call xc_deassign_device()

Thanks,
--
Yuji Shimada


Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 7bd2feb..6a53137 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -3972,13 +3972,6 @@ static int unregister_real_device(int slot)
     /* unregister real device's MMIO/PIO BARs */
     pt_unregister_regions(assigned_device);
 
-    /* deassign the dev to dom0 */
-    bdf |= (pci_dev->bus  & 0xff) << 16;
-    bdf |= (pci_dev->dev  & 0x1f) << 11;
-    bdf |= (pci_dev->func & 0x1f) << 8;
-    if ( (rc = xc_deassign_device(xc_handle, domid, bdf)) != 0)
-        PT_LOG("Error: Revoking the device failed! rc=%d\n", rc);
-
     /* mark this slot as free */
     php_dev->valid = 0;
     php_dev->pt_dev = NULL;

Re: [PATCH 2/2] ioemu: don't call xc_deassign_device().

[Yuji Shimada]

Hi Ian,

I sent two patches about the reset of devices.

The patch [1/2] has already applied to Xen.
Could you apply the patch [2/2] to ioemu?

Thanks,
--
Yuji Shimada

On Fri, 24 Apr 2009 17:43:39 +0900
Yuji Shimada <shimada-yxb@necst.nec.co.jp> wrote:

> This patch modifies ioemu not to call xc_deassign_device()
> 
> Thanks,
> --
> Yuji Shimada
> 
> 
> Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>
> 
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 7bd2feb..6a53137 100644
> --- a/hw/pass-through.c
> +++ b/hw/pass-through.c
> @@ -3972,13 +3972,6 @@ static int unregister_real_device(int slot)
>      /* unregister real device's MMIO/PIO BARs */
>      pt_unregister_regions(assigned_device);
>  
> -    /* deassign the dev to dom0 */
> -    bdf |= (pci_dev->bus  & 0xff) << 16;
> -    bdf |= (pci_dev->dev  & 0x1f) << 11;
> -    bdf |= (pci_dev->func & 0x1f) << 8;
> -    if ( (rc = xc_deassign_device(xc_handle, domid, bdf)) != 0)
> -        PT_LOG("Error: Revoking the device failed! rc=%d\n", rc);
> -
>      /* mark this slot as free */
>      php_dev->valid = 0;
>      php_dev->pt_dev = NULL;

[PATCH 3/3] ioemu: remove power state transition and xc_deassign_device().

[Yuji Shimada]

This patch adds two changes to ioemu.
  - Modify ioemu not to call xc_deassign_device()
  - Remove power state transition of real device when ioemu initialise
    PMCSR.

Thanks,
--
Yuji Shimada


Signed-off-by: Yuji Shimada <shimada-yxb@necst.nec.co.jp>

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 710acbf..65e7634 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -2465,29 +2465,6 @@ static uint32_t pt_pmcsr_reg_init(struct pt_dev *ptdev,
         ptdev->pm_state->no_soft_reset = (*(uint8_t *)(d->config + real_offset)
             & (uint8_t)PCI_PM_CTRL_NO_SOFT_RESET);
 
-    /* wake up real physical device */
-    switch ( pci_read_word(ptdev->pci_dev, real_offset) 
-             & PCI_PM_CTRL_STATE_MASK )
-    {
-    case 0:
-        break;
-    case 1:
-        PT_LOG("Power state transition D1 -> D0active\n");
-        pci_write_word(ptdev->pci_dev, real_offset, 0);
-        break;
-    case 2:
-        PT_LOG("Power state transition D2 -> D0active\n");
-        pci_write_word(ptdev->pci_dev, real_offset, 0);
-        usleep(200);
-        break;
-    case 3:
-        PT_LOG("Power state transition D3hot -> D0active\n");
-        pci_write_word(ptdev->pci_dev, real_offset, 0);
-        usleep(10 * 1000);
-        pt_init_pci_config(ptdev);
-        break;
-    }
-
     return reg->init_val;
 }
 
@@ -3963,13 +3940,6 @@ static int unregister_real_device(int slot)
     /* unregister real device's MMIO/PIO BARs */
     pt_unregister_regions(assigned_device);
 
-    /* deassign the dev to dom0 */
-    bdf |= (pci_dev->bus  & 0xff) << 16;
-    bdf |= (pci_dev->dev  & 0x1f) << 11;
-    bdf |= (pci_dev->func & 0x1f) << 8;
-    if ( (rc = xc_deassign_device(xc_handle, domid, bdf)) != 0)
-        PT_LOG("Error: Revoking the device failed! rc=%d\n", rc);
-
     /* mark this slot as free */
     php_dev->valid = 0;
     php_dev->pt_dev = NULL;