[RFC][PATCH] Reset PCIe devices to address DMA problem on kdump with iommu

[Takao Indoh]

Hi Vivek,

(2012/08/03 20:46), Vivek Goyal wrote:
> On Fri, Aug 03, 2012 at 08:24:31PM +0900, Takao Indoh wrote:
>> Hi all,
>>
>> This patch adds kernel parameter "reset_pcie_devices" which resets PCIe
>> devices at boot time to address DMA problem on kdump with iommu. When
>> this parameter is specified, a hot reset is triggered on each PCIe root
>> port and downstream port to reset its downstream endpoint.
>
> Hi Takao,
>
> Why not use existing "reset_devices" parameter instead of introducing
> a new one?

"reset_devices" is used for each driver to reset their own device, and
this patch resets all devices forcibly, so I thought they were different
things.

Thanks,
Takao Indoh

>
> Thanks
> Vivek
>
>>
>> Background:
>> A kdump problem about DMA has been discussed for a long time. That is,
>> when a kernel is switched to the kdump kernel DMA derived from first
>> kernel affects second kernel. Recently this problem surfaces when iommu
>> is used for PCI passthrough on KVM guest. In the case of the machine I
>> use, when intel_iommu=on is specified, DMAR error is detected in kdump
>> kernel and PCI SERR is also detected. Finally kdump fails because some
>> devices does not work correctly.
>>
>> The root cause is that ongoing DMA from first kernel causes DMAR fault
>> because page table of DMAR is initialized while kdump kernel is booting
>> up. Therefore to address this problem DMA needs to be stopped before DMAR
>> is initialized at kdump kernel boot time. By this patch, PCIe devices
>> are reset by hot reset and its DMA is stopped when reset_pcie_devices is
>> specified. One problem of this solution is that VGA is reset and the
>> monitor blacks out when the link between the port and VGA controller was
>> reset. So this patch does not reset the port whose child endpoint is VGA
>> device.
>>
>> Any comments would be appreciated.
>>
>> Signed-off-by: Takao Indoh <indou.takao at jp.fujitsu.com>
>> ---
>>   Documentation/kernel-parameters.txt |    4 +
>>   drivers/pci/quirks.c                |   59 ++++++++++++++++++++++++++
>>   2 files changed, 63 insertions(+)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index e714a02..e694e9f 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -2489,6 +2489,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>   	reset_devices	[KNL] Force drivers to reset the underlying device
>>   			during initialization.
>>
>> +	reset_pcie_devices
>> +			[PCIE] Reset PCIe endpoint at boot time by sending a
>> +			hot reset to root port and downstream port
>> +
>>   	resume=		[SWSUSP]
>>   			Specify the partition device for software suspend
>>   			Format:
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 5155317..7f7fc02 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -32,6 +32,65 @@
>>   #include "pci.h"
>>
>>   /*
>> + * Reset PCIe endpoint by sending a hot reset to root port and downstream port
>> + */
>> +unsigned int reset_pcie_devices;
>> +EXPORT_SYMBOL(reset_pcie_devices);
>> +static int __init set_reset_pcie_devices(char *str)
>> +{
>> +	reset_pcie_devices = 1;
>> +	return 1;
>> +}
>> +__setup("reset_pcie_devices", set_reset_pcie_devices);
>> +
>> +static void __devinit quirk_pcie_device_reset(struct pci_dev *dev)
>> +{
>> +	struct pci_bus *subordinate;
>> +	struct pci_dev *child;
>> +	u16 ctrl;
>> +
>> +	if (!reset_pcie_devices || !pci_is_pcie(dev) || !dev->subordinate ||
>> +	    ((dev->pcie_type != PCI_EXP_TYPE_ROOT_PORT) &&
>> +	     (dev->pcie_type != PCI_EXP_TYPE_DOWNSTREAM)))
>> +		return;
>> +
>> +	subordinate = dev->subordinate;
>> +	list_for_each_entry(child, &subordinate->devices, bus_list) {
>> +		if ((child->pcie_type == PCI_EXP_TYPE_UPSTREAM) ||
>> +		    (child->pcie_type == PCI_EXP_TYPE_PCI_BRIDGE) ||
>> +		    ((child->class >> 16) == PCI_BASE_CLASS_DISPLAY))
>> +		/* Don't reset switch, bridge, VGA device */
>> +		return;
>> +	}
>> +
>> +	dev_info(&dev->dev, "Reset Secondary bus\n");
>> +
>> +	list_for_each_entry(child, &subordinate->devices, bus_list) {
>> +		dev_info(&child->dev, "save state\n");
>> +		pci_save_state(child);
>> +	}
>> +
>> +	/* Assert Secondary Bus Reset */
>> +	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &ctrl);
>> +	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
>> +	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
>> +
>> +	msleep(2);
>> +
>> +	/* De-assert Secondary Bus Reset */
>> +	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
>> +	pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl);
>> +
>> +	msleep(200);
>> +
>> +	list_for_each_entry(child, &subordinate->devices, bus_list) {
>> +		dev_info(&child->dev, "restore state\n");
>> +		pci_restore_state(child);
>> +	}
>> +}
>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_pcie_device_reset);
>> +
>> +/*
>>    * Decoding should be disabled for a PCI device during BAR sizing to avoid
>>    * conflict. But doing so may cause problems on host bridge and perhaps other
>>    * key system devices. For devices that need to have mmio decoding always-on,
>
>