(2013/04/26 3:01), Don Dutile wrote: > On 04/25/2013 01:11 AM, Takao Indoh wrote: >> (2013/04/25 4:59), Don Dutile wrote: >>> On 04/24/2013 12:58 AM, Takao Indoh wrote: >>>> This patch resets PCIe devices on boot to stop ongoing DMA. When >>>> "pci=pcie_reset_devices" is specified, a hot reset is triggered on each >>>> PCIe root port and downstream port to reset its downstream endpoint. >>>> >>>> Problem: >>>> This patch solves the problem that kdump can fail when intel_iommu=on is >>>> specified. When intel_iommu=on is specified, many dma-remapping errors >>>> occur in second kernel and it causes problems like driver error or PCI >>>> SERR, at last kdump fails. This problem is caused as follows. >>>> 1) Devices are working on first kernel. >>>> 2) Switch to second kernel(kdump kernel). The devices are still working >>>> and its DMA continues during this switch. >>>> 3) iommu is initialized during second kernel boot and ongoing DMA causes >>>> dma-remapping errors. >>>> >>>> Solution: >>>> All DMA transactions have to be stopped before iommu is initialized. By >>>> this patch devices are reset and in-flight DMA is stopped before >>>> pci_iommu_init. >>>> >>>> To invoke hot reset on an endpoint, its upstream link need to be reset. >>>> reset_pcie_devices() is called from fs_initcall_sync, and it finds root >>>> port/downstream port whose child is PCIe endpoint, and then reset link >>>> between them. If the endpoint is VGA device, it is skipped because the >>>> monitor blacks out if VGA controller is reset. >>>> >>> Couple questions wrt VGA device: >>> (1) Many graphics devices are multi-function, one function being VGA; >>> is the VGA always function 0, so this scan sees it first& doesn't >>> do a reset on that PCIe link? if the VGA is not function 0, won't >>> this logic break (will reset b/c function 0 is non-VGA graphics) ? >> >> VGA is not reset irrespective of its function number. The logic of this >> patch is: >> >> for_each_pci_dev(dev) { >> if (dev is not PCIe) >> continue; >> if (dev is not root port/downstream port) ---(1) >> continue; >> list_for_each_entry(child,&dev->subordinate->devices, bus_list) { >> if (child is upstream port or bridge or VGA) ---(2) >> continue; >> } >> do_reset_its_child(dev); >> } >> >> Therefore VGA itself is skipped by (1), and upstream device(root port or >> downstream port) of VGA is also skipped by (2). >> >> >>> (2) I'm hearing VGA will soon not be the a required console; this logic >>> assumes it is, and why it isn't blanked. >>> Q: Should the filter be based on a device having a device-class of display ? >> >> I want to avoid the situation that user's monitor blacks out and user >> cannot know what's going on. That's reason why I introduced the logic to >> skip VGA. As far as I tested the logic based on device-class works well, > sorry, I read your description, which said VGA, but your are filtering on display class, > which includes non-VGA as well. So, all set ... but large, (x16) non-VGA display devices > are probably one of the most aggressive DMA engines on a system.... and will grow as > asymmetric processing using GPUs gets architected into a device-agnostic manner. > So, this may work well for servers, which is the primary consumer/user of this feature, > and they typically have built-in graphics that are generally used in simple VGA mode, > so this may be sufficient for now. > > >> but I would appreciate it if there are better ways. >> > You probably don't want to hear it but.... > a) only turn off cmd-reg master enable bit > b) only do reset based on a list of devices known not to > obey their cmd-reg master enable bit, and only do reset to those devices. > But, given the testing you've done so far, this optional (need cmdline) feature, > let's start here. > >>> >>>> Actually this is v8 patch but quite different from v7 and it's been so >>>> long since previous post, so I start over again. >>> Thanks for this re-start. I need to continue reviewing the rest. >> >> Thank you for your review! >> >>> >>> Q: Why not force IOMMU off when re-booting a kexec kernel to perform a crash >>> dump? After the crash dump, the system is rebooting to previous (iommu=on) setting. >>> That logic, along w/your previous patch to disable the IOMMU if iommu=off >>> is set, would remove this (relatively slow) PCI init sequencing ? >> >> To force iommu off, all ongoing DMA have to be stopped before that since >> they are accessing the device address, not physical address. If we disable >> iommu without stopping in-flihgt DMA, devices access invalid memory area >> and it causes memory corruption or PCI-SERR due to DMA error. > Right, that's a 'duh' on my part. > I thought 'disable iommu' == 'block all dma' and it just turns it off & > let's the ongoing DMA run... > Please ignore this question... sigh. > >> >> So, whether we use iommu or not in second kernel, we have to stop DMA in >> second kernel if iommu is used in first kernel. >> >> Thanks, >> Takao Indoh >> >> >>> >>>> Previous post: >>>> [PATCH v7 0/5] Reset PCIe devices to address DMA problem on kdump >>>> https://lkml.org/lkml/2012/11/26/814 >>>> >>>> Signed-off-by: Takao Indoh<indou.takao@xxxxxxxxxxxxxx> >>>> --- >>>> Documentation/kernel-parameters.txt | 2 + >>>> drivers/pci/pci.c | 103 +++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 105 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt >>>> index 4609e81..2a31ade 100644 >>>> --- a/Documentation/kernel-parameters.txt >>>> +++ b/Documentation/kernel-parameters.txt >>>> @@ -2250,6 +2250,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted. >>>> any pair of devices, possibly at the cost of >>>> reduced performance. This also guarantees >>>> that hot-added devices will work. >>>> + pcie_reset_devices Reset PCIe endpoint on boot by hot >>>> + reset >>>> cbiosize=nn[KMG] The fixed amount of bus space which is >>>> reserved for the CardBus bridge's IO window. >>>> The default value is 256 bytes. >>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >>>> index b099e00..42385c9 100644 >>>> --- a/drivers/pci/pci.c >>>> +++ b/drivers/pci/pci.c >>>> @@ -3878,6 +3878,107 @@ void __weak pci_fixup_cardbus(struct pci_bus *bus) >>>> } >>>> EXPORT_SYMBOL(pci_fixup_cardbus); >>>> >>>> +/* >>>> + * Return true if dev is PCIe root port or downstream port whose child is PCIe >>>> + * endpoint except VGA device. >>>> + */ >>>> +static int __init need_reset(struct pci_dev *dev) >>>> +{ >>>> + struct pci_bus *subordinate; >>>> + struct pci_dev *child; >>>> + >>>> + if (!pci_is_pcie(dev) || !dev->subordinate || >>>> + list_empty(&dev->subordinate->devices) || >>>> + ((pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)&& >>>> + (pci_pcie_type(dev) != PCI_EXP_TYPE_DOWNSTREAM))) >>>> + return 0; >>>> + >>>> + subordinate = dev->subordinate; >>>> + list_for_each_entry(child,&subordinate->devices, bus_list) { >>>> + if ((pci_pcie_type(child) == PCI_EXP_TYPE_UPSTREAM) || >>>> + (pci_pcie_type(child) == PCI_EXP_TYPE_PCI_BRIDGE) || >>>> + ((child->class>> 16) == PCI_BASE_CLASS_DISPLAY)) >>>> + /* Don't reset switch, bridge, VGA device */ >>>> + return 0; >>>> + } >>>> + >>>> + return 1; >>>> +} >>>> + >>>> +static void __init save_config(struct pci_dev *dev) > This should be renamed. it implies it is saving the config of the pdev passed in, > when in reality, it is saving the config of all the devices attached to this pdev. > i.e., save_downstream_configs() > >>>> +{ >>>> + struct pci_bus *subordinate; >>>> + struct pci_dev *child; >>>> + >>>> + if (!need_reset(dev)) >>>> + return; >>>> + >>>> + subordinate = dev->subordinate; >>>> + list_for_each_entry(child,&subordinate->devices, bus_list) { >>>> + dev_info(&child->dev, "save state\n"); >>>> + pci_save_state(child); >>>> + } >>>> +} >>>> + >>>> +static void __init restore_config(struct pci_dev *dev) > inverse of above: restore_downstream_configs() >>>> +{ >>>> + struct pci_bus *subordinate; >>>> + struct pci_dev *child; >>>> + >>>> + if (!need_reset(dev)) >>>> + return; >>>> + >>>> + subordinate = dev->subordinate; >>>> + list_for_each_entry(child,&subordinate->devices, bus_list) { >>>> + dev_info(&child->dev, "restore state\n"); >>>> + pci_restore_state(child); >>>> + } >>>> +} >>>> + >>>> +static void __init do_device_reset(struct pci_dev *dev) > do_downstream_device_reset() -- it's not resetting this pdev, > but the pdev's of the devices attached to it. > >>>> +{ >>>> + u16 ctrl; >>>> + >>>> + if (!need_reset(dev)) >>>> + return; >>>> + >>>> + dev_info(&dev->dev, "Reset Secondary bus\n"); >>>> + >>>> + /* 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); >>>> + > This works well for x86, which uses ioport registers to access > these <256-offset registers, b/c the write() function can't return > until the write is actually completed, but for a non-x86 system, > with fully mmconf'd PCI space, a write() may still be a write & run > (sitting in CPU write-merge) buffer, so if you need a full 2ms, > you ought to do another read_config() to the device, to flush the write, > before starting the msleep(2) clock. > >>>> + /* De-assert Secondary Bus Reset */ >>>> + ctrl&= ~PCI_BRIDGE_CTL_BUS_RESET; >>>> + pci_write_config_word(dev, PCI_BRIDGE_CONTROL, ctrl); >>>> +} >>>> + >>>> +static int __initdata pcie_reset_devices; >>>> +static int __init reset_pcie_devices(void) > this should be reset_pcie_endpoints() ... it's not resetting all pcie devices >>>> +{ >>>> + struct pci_dev *dev = NULL; >>>> + >>>> + if (!pcie_reset_devices) >>>> + return 0; >>>> + >>>> + for_each_pci_dev(dev) >>>> + save_config(dev); >>>> + >>>> + for_each_pci_dev(dev) >>>> + do_device_reset(dev); >>>> + >>>> + msleep(1000); >>>> + >>>> + for_each_pci_dev(dev) >>>> + restore_config(dev); >>>> + > My apologies if past thread covered this sequence... > Why three loops through all PCIe devices on the system? > why not have the first for-each-pci-dev() loop filter devices > to be reset, and save_config those pdev's, > return a list of saved_pdev's; feed that list into the do_device_reset(); > then mpsleep(), then restore the list. > in fact, once you create a link list of pdev's to reset, > just loop that list doing save, then reset; rtn the list, do msleep(), > then restore the config of pdevs in the list. > Otherwise, doing much more traversing than what's needed. > Doing a great deal more config-saving then needed right now as well > (saving all non-endpt devices that aren't reset). One question, do you mean we should have two lists? For example, LIST_HEAD(pci_reset_list); /* pdev list to reset */ LIST_HEAD(pci_saved_list); /* pdev list to save/restore config */ Or simply making one list and just loop it like this: LIST_HEAD(pdev_list); for_each_pci_dev(pdev) if (need_reset(pdev)) /* Add pdev to pdev_list */ list_for_each_entry(pdev, &pdev_list) save_downstream_configs(pdev); list_for_each_entry(pdev, &pdev_list) do_downstream_device_reset(pdev); list_for_each_entry(pdev, &pdev_list) restore_downstream_configs(pdev); Thanks, Takao Indoh > >>>> + return 0; >>>> +} >>>> +fs_initcall_sync(reset_pcie_devices); >>>> + >>>> static int __init pci_setup(char *str) >>>> { >>>> while (str) { >>>> @@ -3920,6 +4021,8 @@ static int __init pci_setup(char *str) >>>> pcie_bus_config = PCIE_BUS_PEER2PEER; >>>> } else if (!strncmp(str, "pcie_scan_all", 13)) { >>>> pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS); >>>> + } else if (!strncmp(str, "pcie_reset_devices", 18)) { > pcie_reset_endpoint_devices >>>> + pcie_reset_devices = 1; > pcie_reset_endpoint_devices >>>> } else { >>>> printk(KERN_ERR "PCI: Unknown option `%s'\n", >>>> str); >>> >>> >>> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html