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?
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,
