[PATCH 0/4] Support registering specific reset handler

[PATCH 0/4] Support registering specific reset handler

[Gavin Shan]

VFIO PCI infrastructure depends on pci_reset_function() to do reset on
PCI devices so that they would be in clean state when host or guest grabs
them. Unfortunately, the function doesn't work (or not well) on some PCI
devices that require EEH PE reset.

The patchset extends the quirk for PCI device speicific reset methods to
allow dynamically registration. With it, we can translate reset requests
for those special PCI devcies to EEH PE reset, which is only avaialble on
64-bits PowerPC platforms.

Gavin Shan (4):
  PCI: Rename struct pci_dev_reset_methods
  PCI: Introduce list for device reset methods
  PCI: Allow registering reset method
  powerpc/powernv: Register PCI dev specific reset handlers

 arch/powerpc/platforms/powernv/pci.c |  61 +++++++++++++++
 drivers/pci/pci.h                    |   3 +-
 drivers/pci/quirks.c                 | 139 ++++++++++++++++++++++++++++++-----
 include/linux/pci.h                  |   9 +++
 4 files changed, 192 insertions(+), 20 deletions(-)

-- 
1.8.3.2

[PATCH 1/4] PCI: Rename struct pci_dev_reset_methods

[Gavin Shan]

It'd better to have "struct pci_dev_reset_method" as its name.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/pci/pci.h    | 2 +-
 drivers/pci/quirks.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4091f82..e67b22f 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -306,7 +306,7 @@ static inline resource_size_t pci_resource_alignment(struct pci_dev *dev,
 
 void pci_enable_acs(struct pci_dev *dev);
 
-struct pci_dev_reset_methods {
+struct pci_dev_reset_method {
 	u16 vendor;
 	u16 device;
 	int (*reset)(struct pci_dev *dev, int probe);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 85f247e..512bbbb 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3508,7 +3508,7 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
 #define PCI_DEVICE_ID_INTEL_IVB_M_VGA      0x0156
 #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA     0x0166
 
-static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
+static const struct pci_dev_reset_method pci_dev_reset_methods[] = {
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
 		 reset_intel_82599_sfp_virtfn },
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M_VGA,
@@ -3529,7 +3529,7 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
  */
 int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 {
-	const struct pci_dev_reset_methods *i;
+	const struct pci_dev_reset_method *i;
 
 	for (i = pci_dev_reset_methods; i->reset; i++) {
 		if ((i->vendor == dev->vendor ||
-- 
1.8.3.2

[PATCH 2/4] PCI: Introduce list for device reset methods

[Gavin Shan]

The patch introduces list to hold the PCI device specific reset
methods. With help of the list, we can register PCI device specific
reset method dynamically as we do in next one patch.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/pci/pci.h    |  1 +
 drivers/pci/quirks.c | 75 +++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 57 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e67b22f..82e648a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -310,6 +310,7 @@ struct pci_dev_reset_method {
 	u16 vendor;
 	u16 device;
 	int (*reset)(struct pci_dev *dev, int probe);
+	struct list_head node;
 };
 
 #ifdef CONFIG_PCI_QUIRKS
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 512bbbb..37f4c5d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3508,20 +3508,44 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, int probe)
 #define PCI_DEVICE_ID_INTEL_IVB_M_VGA      0x0156
 #define PCI_DEVICE_ID_INTEL_IVB_M2_VGA     0x0166
 
-static const struct pci_dev_reset_method pci_dev_reset_methods[] = {
-	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
-		 reset_intel_82599_sfp_virtfn },
-	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M_VGA,
-		reset_ivb_igd },
-	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
-		reset_ivb_igd },
-	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
-		reset_intel_generic_dev },
-	{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
-		reset_chelsio_generic_dev },
-	{ 0 }
+static LIST_HEAD(pci_dev_reset_list);
+static DEFINE_MUTEX(pci_dev_reset_mutex);
+static bool pci_dev_reset_populated = false;
+static struct pci_dev_reset_method pci_dev_reset_methods[] = {
+	{ .vendor = PCI_VENDOR_ID_INTEL,
+	  .device = PCI_DEVICE_ID_INTEL_82599_SFP_VF,
+	  .reset  = reset_intel_82599_sfp_virtfn
+	},
+	{ .vendor = PCI_VENDOR_ID_INTEL,
+	  .device = PCI_DEVICE_ID_INTEL_IVB_M_VGA,
+	  .reset  = reset_ivb_igd
+	},
+	{ .vendor = PCI_VENDOR_ID_INTEL,
+	  .device = PCI_DEVICE_ID_INTEL_IVB_M2_VGA,
+	  .reset  =reset_ivb_igd
+	},
+	{ .vendor = PCI_VENDOR_ID_INTEL,
+	  .device = PCI_ANY_ID,
+	  .reset  = reset_intel_generic_dev
+	},
+	{ .vendor = PCI_VENDOR_ID_CHELSIO,
+	  .device = PCI_ANY_ID,
+	  .reset  = reset_chelsio_generic_dev
+	}
 };
 
+static void pci_dev_populate_reset(void)
+{
+	struct pci_dev_reset_method *m;
+	int i;
+
+	for (i = ARRAY_SIZE(pci_dev_reset_methods) - 1; i >= 0; i--) {
+		m = &pci_dev_reset_methods[i];
+		INIT_LIST_HEAD(&m->node);
+		list_add(&m->node, &pci_dev_reset_list);
+	}
+}
+
 /*
  * These device-specific reset methods are here rather than in a driver
  * because when a host assigns a device to a guest VM, the host may need
@@ -3529,16 +3553,29 @@ static const struct pci_dev_reset_method pci_dev_reset_methods[] = {
  */
 int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 {
-	const struct pci_dev_reset_method *i;
+	struct pci_dev_reset_method *m;
+	int ret;
 
-	for (i = pci_dev_reset_methods; i->reset; i++) {
-		if ((i->vendor == dev->vendor ||
-		     i->vendor == (u16)PCI_ANY_ID) &&
-		    (i->device == dev->device ||
-		     i->device == (u16)PCI_ANY_ID))
-			return i->reset(dev, probe);
+	mutex_lock(&pci_dev_reset_mutex);
+
+	if (!pci_dev_reset_populated) {
+		pci_dev_populate_reset();
+		pci_dev_reset_populated = true;
 	}
 
+	list_for_each_entry(m, &pci_dev_reset_list, node) {
+		if ((m->vendor == dev->vendor ||
+		     m->vendor == (u16)PCI_ANY_ID) &&
+		    (m->device == dev->device ||
+		     m->device == (u16)PCI_ANY_ID)) {
+			ret = m->reset(dev, probe);
+			mutex_unlock(&pci_dev_reset_mutex);
+			return ret;
+		}
+	}
+
+	mutex_unlock(&pci_dev_reset_mutex);
+
 	return -ENOTTY;
 }
 
-- 
1.8.3.2

[PATCH 3/4] PCI: Allow registering reset method

[Gavin Shan]

The patch exposes function pci_dev_add_specific_reset() to allow
registering PCI device specific reset methods.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 drivers/pci/quirks.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h  |  9 ++++++++
 2 files changed, 73 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 37f4c5d..ef811ec 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3534,6 +3534,70 @@ static struct pci_dev_reset_method pci_dev_reset_methods[] = {
 	}
 };
 
+static bool pci_dev_reset_match(struct pci_dev_reset_method *m,
+				u16 vendor, u16 device,
+				int (*reset)(struct pci_dev *, int))
+{
+	if (m->vendor != (u16)PCI_ANY_ID &&
+	    vendor != (u16)PCI_ANY_ID &&
+	    m->vendor != vendor)
+		return false;
+
+	if (m->device != (u16)PCI_ANY_ID &&
+	    device != (u16)PCI_ANY_ID &&
+	    m->device != device)
+		return false;
+
+	if (m->reset != reset)
+		return false;
+
+	return true;
+}
+
+int pci_dev_add_specific_reset(u16 vendor, u16 device,
+			       int (*reset)(struct pci_dev *, int))
+{
+	struct pci_dev_reset_method *m, *method;
+	int i;
+
+	/* Always expect valid handler */
+	if (!reset)
+		return -EINVAL;
+
+	method = kzalloc(sizeof(*method), GFP_KERNEL);
+	if (!method)
+		return -ENOMEM;
+
+	/* Check if we have conflicting method */
+	mutex_lock(&pci_dev_reset_mutex);
+	if (!pci_dev_reset_populated) {
+		for (i = 0; i < ARRAY_SIZE(pci_dev_reset_methods); i++) {
+			m = &pci_dev_reset_methods[i];
+			if (pci_dev_reset_match(m, vendor, device, reset))
+				goto found;
+		}
+	} else {
+		list_for_each_entry(m, &pci_dev_reset_list, node) {
+			if (pci_dev_reset_match(m, vendor, device, reset))
+				goto found;
+		}
+	}
+
+	/* Populate it */
+	method->vendor = vendor;
+	method->device = device;
+	method->reset  = reset;
+	INIT_LIST_HEAD(&method->node);
+	list_add_tail(&method->node, &pci_dev_reset_list);
+	mutex_unlock(&pci_dev_reset_mutex);
+	return 0;
+found:
+	mutex_unlock(&pci_dev_reset_mutex);
+	kfree(method);
+	return -EEXIST;
+}
+EXPORT_SYMBOL(pci_dev_add_specific_reset);
+
 static void pci_dev_populate_reset(void)
 {
 	struct pci_dev_reset_method *m;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 211e9da..b98be1a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1605,6 +1605,8 @@ enum pci_fixup_pass {
 
 #ifdef CONFIG_PCI_QUIRKS
 void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev);
+int pci_dev_add_specific_reset(u16 vendor, u16 device,
+			       int (*reset)(struct pci_dev *, int));
 int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
 void pci_dev_specific_enable_acs(struct pci_dev *dev);
 #else
@@ -1615,6 +1617,13 @@ static inline int pci_dev_specific_acs_enabled(struct pci_dev *dev,
 {
 	return -ENOTTY;
 }
+
+int pci_dev_add_specific_reset(u16 vendor, u16 device,
+			       int (*reset)(struct pci_dev *, int))
+{
+	return -ENOTTY;
+}
+
 static inline void pci_dev_specific_enable_acs(struct pci_dev *dev) { }
 #endif
 
-- 
1.8.3.2

[PATCH 4/4] powerpc/powernv: Register PCI dev specific reset handlers

[Gavin Shan]

Currently, PCI VFIO infrastructure depends on pci_reset_function()
to ensure the PCI device in clean state when passing to guest, or
being returned back to host. However, the function doesn't work
(or well) on some PCI devices, which potentially brings pending
traffic over the boundary between host/guest and usually causes
memory corruption.

The patch registers PCI device specific reset handlers for those
PCI devices, pci_reset_function() doesn't work or not well, to
translate the request to EEH PE reset if possible.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/pci.c | 61 ++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index e69142f..c68d508 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -727,6 +727,64 @@ static void pnv_pci_dma_fallback_setup(struct pci_controller *hose,
 	set_iommu_table_base_and_group(&pdev->dev, pdn->iommu_table);
 }
 
+/*
+ * VFIO infrastructure depends on pci_reset_function() to do
+ * reset on the PCI devices when their owership is changed to
+ * ensure consistent clean state on those devices when someone
+ * grabs them. However, pci_reset_function() doesn't work for
+ * some deivces that require EEH PE reset.
+ */
+static int pnv_pci_dev_specific_reset(struct pci_dev *pdev, int probe)
+{
+	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
+	struct eeh_pe *pe = edev ? edev->pe : NULL;
+
+	if (!pe)
+		return -ENOTTY;
+
+	if (probe)
+		return 0;
+
+	pci_set_pcie_reset_state(pdev, pcie_hot_reset);
+	pci_set_pcie_reset_state(pdev, pcie_deassert_reset);
+	return 0;
+}
+
+static int pnv_pci_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	static bool pnv_pci_reset_registered = false;
+	int ret = 0;
+
+	if (pnv_pci_reset_registered)
+		return 0;
+
+	pnv_pci_reset_registered = true;
+	ret += pci_dev_add_specific_reset(PCI_VENDOR_ID_IBM,
+					  PCI_ANY_ID,
+					  pnv_pci_dev_specific_reset);
+	ret += pci_dev_add_specific_reset(PCI_VENDOR_ID_BROADCOM,
+					  PCI_DEVICE_ID_NX2_57800,
+					  pnv_pci_dev_specific_reset);
+	ret += pci_dev_add_specific_reset(PCI_VENDOR_ID_BROADCOM,
+					  PCI_DEVICE_ID_NX2_57840,
+					  pnv_pci_dev_specific_reset);
+	ret += pci_dev_add_specific_reset(PCI_VENDOR_ID_BROADCOM,
+					  PCI_DEVICE_ID_NX2_57810,
+					  pnv_pci_dev_specific_reset);
+	ret += pci_dev_add_specific_reset(PCI_VENDOR_ID_MELLANOX,
+					  PCI_ANY_ID,
+					  pnv_pci_dev_specific_reset);
+	ret += pci_dev_add_specific_reset(PCI_VENDOR_ID_TI,
+					  PCI_ANY_ID,
+					  pnv_pci_dev_specific_reset);
+	if (ret)
+		pr_warn("%s: Failure adding PCI specific reset handlers\n",
+			__func__);
+
+	/* Don't return error to keep PCI core going */
+	return 0;
+}
+
 static void pnv_pci_dma_dev_setup(struct pci_dev *pdev)
 {
 	struct pci_controller *hose = pci_bus_to_host(pdev->bus);
@@ -820,6 +878,9 @@ void __init pnv_pci_init(void)
 	/* Setup the linkage between OF nodes and PHBs */
 	pci_devs_phb_init();
 
+	/* Setup root bridge */
+	ppc_md.pcibios_root_bridge_prepare = pnv_pci_root_bridge_prepare;
+
 	/* Configure IOMMU DMA hooks */
 	ppc_md.pci_dma_dev_setup = pnv_pci_dma_dev_setup;
 	ppc_md.tce_build = pnv_tce_build_vm;
-- 
1.8.3.2

Re: [PATCH 0/4] Support registering specific reset handler

[cascardo]

On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
> PCI devices so that they would be in clean state when host or guest grabs
> them. Unfortunately, the function doesn't work (or not well) on some PCI
> devices that require EEH PE reset.
> 
> The patchset extends the quirk for PCI device speicific reset methods to
> allow dynamically registration. With it, we can translate reset requests
> for those special PCI devcies to EEH PE reset, which is only avaialble on
> 64-bits PowerPC platforms.
> 

Hi, Gavin.

I like your approach overall. That allows us to confine these quirks to
the platforms where they are relevant. I would make the quirks more
specific, though, instead of doing them for all IBM and Mellanox
devices.

I wonder if we should not have some form of domain reset, where we would
reset all the devices on the same group, and use that on vfio. Grouping
the devices would then be made platform-dependent, as well as the reset
method. On powernv, we would group by IOMMU group and issue a
fundamental reset.

Cascardo.

> Gavin Shan (4):
>   PCI: Rename struct pci_dev_reset_methods
>   PCI: Introduce list for device reset methods
>   PCI: Allow registering reset method
>   powerpc/powernv: Register PCI dev specific reset handlers
> 
>  arch/powerpc/platforms/powernv/pci.c |  61 +++++++++++++++
>  drivers/pci/pci.h                    |   3 +-
>  drivers/pci/quirks.c                 | 139 ++++++++++++++++++++++++++++++-----
>  include/linux/pci.h                  |   9 +++
>  4 files changed, 192 insertions(+), 20 deletions(-)
> 
> -- 
> 1.8.3.2
> 

Re: [PATCH 0/4] Support registering specific reset handler

[Gavin Shan]

On Mon, Feb 16, 2015 at 11:14:27AM -0200, cascardo@linux.vnet.ibm.com wrote:
>On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
>> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
>> PCI devices so that they would be in clean state when host or guest grabs
>> them. Unfortunately, the function doesn't work (or not well) on some PCI
>> devices that require EEH PE reset.
>> 
>> The patchset extends the quirk for PCI device speicific reset methods to
>> allow dynamically registration. With it, we can translate reset requests
>> for those special PCI devcies to EEH PE reset, which is only avaialble on
>> 64-bits PowerPC platforms.
>> 
>
>Hi, Gavin.
>
>I like your approach overall. That allows us to confine these quirks to
>the platforms where they are relevant. I would make the quirks more
>specific, though, instead of doing them for all IBM and Mellanox
>devices.
>

Yeah, we need have more specific vendor/device IDs for PATCH[4/4]. Could
you please take a look on PATCH[4/4] and then suggest the specific devices
that requries the platform-dependent reset quirk? Especially the device IDs
for IBM/Mellanox we need put add quirks for.

>I wonder if we should not have some form of domain reset, where we would
>reset all the devices on the same group, and use that on vfio. Grouping
>the devices would then be made platform-dependent, as well as the reset
>method. On powernv, we would group by IOMMU group and issue a
>fundamental reset.
>

I'm assuming "domain reset" is PE reset, which is the specific reset handler
tries to do on PowerNV platform. The reason why we need platform specific
reset handler is some adapters can't support function level reset methods
(except pci_dev_specific_reset()) in __pci_dev_reset().

Thanks,
Gavin

>Cascardo.
>
>> Gavin Shan (4):
>>   PCI: Rename struct pci_dev_reset_methods
>>   PCI: Introduce list for device reset methods
>>   PCI: Allow registering reset method
>>   powerpc/powernv: Register PCI dev specific reset handlers
>> 
>>  arch/powerpc/platforms/powernv/pci.c |  61 +++++++++++++++
>>  drivers/pci/pci.h                    |   3 +-
>>  drivers/pci/quirks.c                 | 139 ++++++++++++++++++++++++++++++-----
>>  include/linux/pci.h                  |   9 +++
>>  4 files changed, 192 insertions(+), 20 deletions(-)
>> 
>> -- 
>> 1.8.3.2
>> 

Re: [PATCH 0/4] Support registering specific reset handler

[cascardo]

On Tue, Feb 17, 2015 at 09:36:47AM +1100, Gavin Shan wrote:
> On Mon, Feb 16, 2015 at 11:14:27AM -0200, cascardo@linux.vnet.ibm.com wrote:
> >On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
> >> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
> >> PCI devices so that they would be in clean state when host or guest grabs
> >> them. Unfortunately, the function doesn't work (or not well) on some PCI
> >> devices that require EEH PE reset.
> >> 
> >> The patchset extends the quirk for PCI device speicific reset methods to
> >> allow dynamically registration. With it, we can translate reset requests
> >> for those special PCI devcies to EEH PE reset, which is only avaialble on
> >> 64-bits PowerPC platforms.
> >> 
> >
> >Hi, Gavin.
> >
> >I like your approach overall. That allows us to confine these quirks to
> >the platforms where they are relevant. I would make the quirks more
> >specific, though, instead of doing them for all IBM and Mellanox
> >devices.
> >
> 
> Yeah, we need have more specific vendor/device IDs for PATCH[4/4]. Could
> you please take a look on PATCH[4/4] and then suggest the specific devices
> that requries the platform-dependent reset quirk? Especially the device IDs
> for IBM/Mellanox we need put add quirks for.
> 
> >I wonder if we should not have some form of domain reset, where we would
> >reset all the devices on the same group, and use that on vfio. Grouping
> >the devices would then be made platform-dependent, as well as the reset
> >method. On powernv, we would group by IOMMU group and issue a
> >fundamental reset.
> >
> 
> I'm assuming "domain reset" is PE reset, which is the specific reset handler
> tries to do on PowerNV platform. The reason why we need platform specific
> reset handler is some adapters can't support function level reset methods
> (except pci_dev_specific_reset()) in __pci_dev_reset().
> 

Well, in the case of Power servers, this would be the PE reset, I am not
sure what this would be on other platforms. No other platform implements
pci_set_pcie_reset_state, which I think would be one possible to
implement such a reset.

What I am saying is that we should consider doing this on all platforms
and for all adapters, because this is not specific to Power and this is
not specific to this particular set of adapters. Otherwise, one can
simply program one adapter in the guest to write to host memory,
shutdown, and since no reset will take place, the card will simply write
to host memory when the guest is finished with and IOMMU is off.

Regards.
Cascardo.

> Thanks,
> Gavin
> 
> >Cascardo.
> >
> >> Gavin Shan (4):
> >>   PCI: Rename struct pci_dev_reset_methods
> >>   PCI: Introduce list for device reset methods
> >>   PCI: Allow registering reset method
> >>   powerpc/powernv: Register PCI dev specific reset handlers
> >> 
> >>  arch/powerpc/platforms/powernv/pci.c |  61 +++++++++++++++
> >>  drivers/pci/pci.h                    |   3 +-
> >>  drivers/pci/quirks.c                 | 139 ++++++++++++++++++++++++++++++-----
> >>  include/linux/pci.h                  |   9 +++
> >>  4 files changed, 192 insertions(+), 20 deletions(-)
> >> 
> >> -- 
> >> 1.8.3.2
> >> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 0/4] Support registering specific reset handler

[Gavin Shan]

On Thu, Feb 19, 2015 at 04:57:47PM -0200, cascardo@linux.vnet.ibm.com wrote:
>On Tue, Feb 17, 2015 at 09:36:47AM +1100, Gavin Shan wrote:
>> On Mon, Feb 16, 2015 at 11:14:27AM -0200, cascardo@linux.vnet.ibm.com wrote:
>> >On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
>> >> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
>> >> PCI devices so that they would be in clean state when host or guest grabs
>> >> them. Unfortunately, the function doesn't work (or not well) on some PCI
>> >> devices that require EEH PE reset.
>> >> 
>> >> The patchset extends the quirk for PCI device speicific reset methods to
>> >> allow dynamically registration. With it, we can translate reset requests
>> >> for those special PCI devcies to EEH PE reset, which is only avaialble on
>> >> 64-bits PowerPC platforms.
>> >> 
>> >
>> >Hi, Gavin.
>> >
>> >I like your approach overall. That allows us to confine these quirks to
>> >the platforms where they are relevant. I would make the quirks more
>> >specific, though, instead of doing them for all IBM and Mellanox
>> >devices.
>> >
>> 
>> Yeah, we need have more specific vendor/device IDs for PATCH[4/4]. Could
>> you please take a look on PATCH[4/4] and then suggest the specific devices
>> that requries the platform-dependent reset quirk? Especially the device IDs
>> for IBM/Mellanox we need put add quirks for.
>> 
>> >I wonder if we should not have some form of domain reset, where we would
>> >reset all the devices on the same group, and use that on vfio. Grouping
>> >the devices would then be made platform-dependent, as well as the reset
>> >method. On powernv, we would group by IOMMU group and issue a
>> >fundamental reset.
>> >
>> 
>> I'm assuming "domain reset" is PE reset, which is the specific reset handler
>> tries to do on PowerNV platform. The reason why we need platform specific
>> reset handler is some adapters can't support function level reset methods
>> (except pci_dev_specific_reset()) in __pci_dev_reset().
>> 
>
>Well, in the case of Power servers, this would be the PE reset, I am not
>sure what this would be on other platforms. No other platform implements
>pci_set_pcie_reset_state, which I think would be one possible to
>implement such a reset.
>
>What I am saying is that we should consider doing this on all platforms
>and for all adapters, because this is not specific to Power and this is
>not specific to this particular set of adapters. Otherwise, one can
>simply program one adapter in the guest to write to host memory,
>shutdown, and since no reset will take place, the card will simply write
>to host memory when the guest is finished with and IOMMU is off.
>

Yes, I believe your way will make things simpler and we don't need the
code to support registering reset handler dynamically at atll. Please
confirm if following idea is what you're suggesting: 

- Introduce function drivers/pci/quirks.c::pci_dev_specific_reset(), which
  will be added to pci_dev_reset_methods[] for various vendor/device IDs.
- pci_dev_specific_reset() routes the reset (or probe) request to
  pci_set_pcie_reset_state(), which needs one more syntax for reset probing.
  In turn, pci_set_pcie_reset_state() calls pcibios_set_pcie_reset_state(),
  which returns -ETTY by default.

For PowerPC, pcibios_set_pcie_reset_state() will do PE reset, which can't
be ported to other platforms as it depends on PowerPC unique EEH feature.
So other platforms have to override this function and do things similar to
PowerPC PE reset there.

Thanks,
Gavin

>> >> Gavin Shan (4):
>> >>   PCI: Rename struct pci_dev_reset_methods
>> >>   PCI: Introduce list for device reset methods
>> >>   PCI: Allow registering reset method
>> >>   powerpc/powernv: Register PCI dev specific reset handlers
>> >> 
>> >>  arch/powerpc/platforms/powernv/pci.c |  61 +++++++++++++++
>> >>  drivers/pci/pci.h                    |   3 +-
>> >>  drivers/pci/quirks.c                 | 139 ++++++++++++++++++++++++++++++-----
>> >>  include/linux/pci.h                  |   9 +++
>> >>  4 files changed, 192 insertions(+), 20 deletions(-)
>> >> 
>> >> -- 
>> >> 1.8.3.2
>> >> 
>> 
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 0/4] Support registering specific reset handler

[Bjorn Helgaas]

On Fri, Feb 20, 2015 at 04:53:08PM +1100, Gavin Shan wrote:
> On Thu, Feb 19, 2015 at 04:57:47PM -0200, cascardo@linux.vnet.ibm.com wrote:
> >On Tue, Feb 17, 2015 at 09:36:47AM +1100, Gavin Shan wrote:
> >> On Mon, Feb 16, 2015 at 11:14:27AM -0200, cascardo@linux.vnet.ibm.com wrote:
> >> >On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
> >> >> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
> >> >> PCI devices so that they would be in clean state when host or guest grabs
> >> >> them. Unfortunately, the function doesn't work (or not well) on some PCI
> >> >> devices that require EEH PE reset.
> >> >> 
> >> >> The patchset extends the quirk for PCI device speicific reset methods to
> >> >> allow dynamically registration. With it, we can translate reset requests
> >> >> for those special PCI devcies to EEH PE reset, which is only avaialble on
> >> >> 64-bits PowerPC platforms.
> >> >> 
> >> >
> >> >Hi, Gavin.
> >> >
> >> >I like your approach overall. That allows us to confine these quirks to
> >> >the platforms where they are relevant. I would make the quirks more
> >> >specific, though, instead of doing them for all IBM and Mellanox
> >> >devices.
> >> >
> >> 
> >> Yeah, we need have more specific vendor/device IDs for PATCH[4/4]. Could
> >> you please take a look on PATCH[4/4] and then suggest the specific devices
> >> that requries the platform-dependent reset quirk? Especially the device IDs
> >> for IBM/Mellanox we need put add quirks for.
> >> 
> >> >I wonder if we should not have some form of domain reset, where we would
> >> >reset all the devices on the same group, and use that on vfio. Grouping
> >> >the devices would then be made platform-dependent, as well as the reset
> >> >method. On powernv, we would group by IOMMU group and issue a
> >> >fundamental reset.
> >> >
> >> 
> >> I'm assuming "domain reset" is PE reset, which is the specific reset handler
> >> tries to do on PowerNV platform. The reason why we need platform specific
> >> reset handler is some adapters can't support function level reset methods
> >> (except pci_dev_specific_reset()) in __pci_dev_reset().
> >> 
> >
> >Well, in the case of Power servers, this would be the PE reset, I am not
> >sure what this would be on other platforms. No other platform implements
> >pci_set_pcie_reset_state, which I think would be one possible to
> >implement such a reset.
> >
> >What I am saying is that we should consider doing this on all platforms
> >and for all adapters, because this is not specific to Power and this is
> >not specific to this particular set of adapters. Otherwise, one can
> >simply program one adapter in the guest to write to host memory,
> >shutdown, and since no reset will take place, the card will simply write
> >to host memory when the guest is finished with and IOMMU is off.
> >
> 
> Yes, I believe your way will make things simpler and we don't need the
> code to support registering reset handler dynamically at atll. Please
> confirm if following idea is what you're suggesting: 
> 
> - Introduce function drivers/pci/quirks.c::pci_dev_specific_reset(), which
>   will be added to pci_dev_reset_methods[] for various vendor/device IDs.
> - pci_dev_specific_reset() routes the reset (or probe) request to
>   pci_set_pcie_reset_state(), which needs one more syntax for reset probing.
>   In turn, pci_set_pcie_reset_state() calls pcibios_set_pcie_reset_state(),
>   which returns -ETTY by default.
> 
> For PowerPC, pcibios_set_pcie_reset_state() will do PE reset, which can't
> be ported to other platforms as it depends on PowerPC unique EEH feature.
> So other platforms have to override this function and do things similar to
> PowerPC PE reset there.

Dropping for now because it sounds like you are considering reworking based
on Cascardo's ideas.  If not, please re-post these so they show up in
patchwork again.

Bjorn

Re: [PATCH 0/4] Support registering specific reset handler

[Gavin Shan]

On Fri, Mar 06, 2015 at 12:38:59PM -0600, Bjorn Helgaas wrote:
>On Fri, Feb 20, 2015 at 04:53:08PM +1100, Gavin Shan wrote:
>> On Thu, Feb 19, 2015 at 04:57:47PM -0200, cascardo@linux.vnet.ibm.com wrote:
>> >On Tue, Feb 17, 2015 at 09:36:47AM +1100, Gavin Shan wrote:
>> >> On Mon, Feb 16, 2015 at 11:14:27AM -0200, cascardo@linux.vnet.ibm.com wrote:
>> >> >On Fri, Feb 13, 2015 at 03:54:55PM +1100, Gavin Shan wrote:
>> >> >> VFIO PCI infrastructure depends on pci_reset_function() to do reset on
>> >> >> PCI devices so that they would be in clean state when host or guest grabs
>> >> >> them. Unfortunately, the function doesn't work (or not well) on some PCI
>> >> >> devices that require EEH PE reset.
>> >> >> 
>> >> >> The patchset extends the quirk for PCI device speicific reset methods to
>> >> >> allow dynamically registration. With it, we can translate reset requests
>> >> >> for those special PCI devcies to EEH PE reset, which is only avaialble on
>> >> >> 64-bits PowerPC platforms.
>> >> >> 
>> >> >
>> >> >Hi, Gavin.
>> >> >
>> >> >I like your approach overall. That allows us to confine these quirks to
>> >> >the platforms where they are relevant. I would make the quirks more
>> >> >specific, though, instead of doing them for all IBM and Mellanox
>> >> >devices.
>> >> >
>> >> 
>> >> Yeah, we need have more specific vendor/device IDs for PATCH[4/4]. Could
>> >> you please take a look on PATCH[4/4] and then suggest the specific devices
>> >> that requries the platform-dependent reset quirk? Especially the device IDs
>> >> for IBM/Mellanox we need put add quirks for.
>> >> 
>> >> >I wonder if we should not have some form of domain reset, where we would
>> >> >reset all the devices on the same group, and use that on vfio. Grouping
>> >> >the devices would then be made platform-dependent, as well as the reset
>> >> >method. On powernv, we would group by IOMMU group and issue a
>> >> >fundamental reset.
>> >> >
>> >> 
>> >> I'm assuming "domain reset" is PE reset, which is the specific reset handler
>> >> tries to do on PowerNV platform. The reason why we need platform specific
>> >> reset handler is some adapters can't support function level reset methods
>> >> (except pci_dev_specific_reset()) in __pci_dev_reset().
>> >> 
>> >
>> >Well, in the case of Power servers, this would be the PE reset, I am not
>> >sure what this would be on other platforms. No other platform implements
>> >pci_set_pcie_reset_state, which I think would be one possible to
>> >implement such a reset.
>> >
>> >What I am saying is that we should consider doing this on all platforms
>> >and for all adapters, because this is not specific to Power and this is
>> >not specific to this particular set of adapters. Otherwise, one can
>> >simply program one adapter in the guest to write to host memory,
>> >shutdown, and since no reset will take place, the card will simply write
>> >to host memory when the guest is finished with and IOMMU is off.
>> >
>> 
>> Yes, I believe your way will make things simpler and we don't need the
>> code to support registering reset handler dynamically at atll. Please
>> confirm if following idea is what you're suggesting: 
>> 
>> - Introduce function drivers/pci/quirks.c::pci_dev_specific_reset(), which
>>   will be added to pci_dev_reset_methods[] for various vendor/device IDs.
>> - pci_dev_specific_reset() routes the reset (or probe) request to
>>   pci_set_pcie_reset_state(), which needs one more syntax for reset probing.
>>   In turn, pci_set_pcie_reset_state() calls pcibios_set_pcie_reset_state(),
>>   which returns -ETTY by default.
>> 
>> For PowerPC, pcibios_set_pcie_reset_state() will do PE reset, which can't
>> be ported to other platforms as it depends on PowerPC unique EEH feature.
>> So other platforms have to override this function and do things similar to
>> PowerPC PE reset there.
>
>Dropping for now because it sounds like you are considering reworking based
>on Cascardo's ideas.  If not, please re-post these so they show up in
>patchwork again.
>

Yes, I'll rework on this according to Cascardo's input and repost.

Thanks,
Gavin

>Bjorn
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev