Re: [PATCH v2] PCI: Reset PCIe devices to stop ongoing DMA

[Takao Indoh]

(2013/06/12 22:19), Don Dutile wrote:
> On 06/11/2013 07:19 PM, Sumner, William wrote:
>>
>>> (2013/06/11 11:20), Bjorn Helgaas wrote:
>>>> On Fri, Jun 7, 2013 at 2:46 AM, Takao Indoh<indou.takao@xxxxxxxxxxxxxx>  wrote:
>>>>> (2013/06/07 13:14), Bjorn Helgaas wrote:
>>>>
>>>>>> One thing I'm not sure about is that you are only resetting PCIe
>>>>>> devices, but I don't think the problem is actually specific to PCIe,
>>>>>> is it?  I think the same issue could occur on any system with an
>>>>>> IOMMU.  In the x86 PC world, most IOMMUs are connected with PCIe, but
>>>>>> there are systems with IOMMUs for plain old PCI devices, e.g.,
>>>>>> PA-RISC.
>>>>>
>>>>> Right, this is not specific to PCIe. The reasons why the target is only
>>>>> PCIe is just to make algorithm to reset simple. It is possible to reset
>>>>> legacy PCI devices in my patch, but code becomes somewhat complicated. I
>>>>> thought recently most systems used PCIe and there was little demand for
>>>>> resetting legacy PCI. Therefore I decided not to reset legacy PCI
>>>>> devices, but I'll do if there are requests :-)
>>>>
>>>> I'm not sure you need to reset legacy devices (or non-PCI devices)
>>>> yet, but the current hook isn't anchored anywhere -- it's just an
>>>> fs_initcall() that doesn't give the reader any clue about the
>>>> connection between the reset and the problem it's solving.
>>>>
>>>> If we do something like this patch, I think it needs to be done at the
>>>> point where we enable or disable the IOMMU.  That way, it's connected
>>>> to the important event, and there's a clue about how to make
>>>> corresponding fixes for other IOMMUs.
>>>
>>> Ok. pci_iommu_init() is appropriate place to add this hook?
>>>
>>>> We already have a "reset_devices" boot option.  This is for the same
>>>> purpose, as far as I can tell, and I'm not sure there's value in
>>>> having both "reset_devices" and "pci=pcie_reset_endpoint_devices".  In
>>>> fact, there's nothing specific even to PCI here.  The Intel VT-d docs
>>>> seem carefully written so they could apply to either PCIe or non-PCI
>>>> devices.
>>>
>>> Yeah, I can integrate this option into reset_devices. The reason why
>>> I separate them is to avoid regression.
>>>
>>> I have tested my patch on many machines and basically it worked, but I
>>> found two machines where this reset patch caused problem. The first one
>>> was caused by bug of raid card firmware. After updating firmware, this
>>> patch worked. The second one was due to bug of PCIe switch chip. I
>>> reported this bug to the vendor but it is not fixed yet.
>>>
>>> Anyway, this patch may cause problems on such a buggy machine, so I
>>> introduced new boot parameter so that user can enable or disable this
>>> function aside from reset_devices.
>>>
>>> Actually Vivek Goyal, kdump maintainer said same thing. He proposed using
>>> reset_devices instead of adding new one, and using quirk or something to
>>> avoid such a buggy devices.
>>
>> With respect to "and using quirk or something to avoid such buggy devices",
>> I believe that it will be necessary to provide a mechanism for devices that
>> need special handling to do the reset -- perhaps something like a list
>> of tuples: (device_type, function_to_call) with a default function_to_call
>> when the device_type is not found in the list.  These functions would
>> need to be physically separate from the device driver because if the device
>> is present it needs to be reset even if the crash kernel chooses not to load
>> the driver for that device.
>>
>>>
>>> So, basically I agree with using reset_devices, but I want to prepare
>>> workaround in case this reset causes something wrong.
>>>
>> I like the ability to specify the original "reset_devices" separately from
>> invoking this new mechanism.  With so many different uses for Linux in
>> so many different environments and with so many different device drivers
>> it seems reasonable to keep the ability to tell the device drivers to
>> reset their devices -- instead of pulling the reset line on all devices.
>>
>> I also like the ability to invoke the new reset feature separately from
>> telling the device drivers to do it.
>>
>>>
>>>>
>>>>>> I tried to make a list of the interesting scenarios and the events
>>>>>> that are relevant to this problem:
>>>>>>
>>>>>>        Case 1: IOMMU off in system, off in kdump kernel
>>>>>>          system kernel leaves IOMMU off
>>>>>>            DMA targets system-kernel memory
>>>>>>          kexec to kdump kernel (IOMMU off, devices untouched)
>>>>>>            DMA targets system-kernel memory (harmless)
>>>>>>          kdump kernel re-inits device
>>>>>>            DMA targets kdump-kernel memory
>>>>>>
>>>>>>        Case 2: IOMMU off in system kernel, on in kdump kernel
>>>>>>          system kernel leaves IOMMU off
>>>>>>            DMA targets system-kernel memory
>>>>>>          kexec to kdump kernel (IOMMU off, devices untouched)
>>>>>>            DMA targets system-kernel memory (harmless)
>>>>>>          kdump kernel enables IOMMU with no valid mappings
>>>>>>            DMA causes IOMMU errors (annoying but harmless)
>>>>>>          kdump kernel re-inits device
>>>>>>            DMA targets IOMMU-mapped kdump-kernel memory
>>>>>>
>>>>>>        Case 3a: IOMMU on in system kernel, kdump kernel doesn't touch IOMMU
>>>>>>          system kernel enables IOMMU
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kexec to kdump kernel (IOMMU on, devices untouched)
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kdump kernel doesn't know about IOMMU or doesn't touch it
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kdump kernel re-inits device
>>>>>>            kernel assumes no IOMMU, so all new DMA mappings are invalid
>>>>>> because DMAs actually do go through the IOMMU
>>>>>>            (** corruption or other non-recoverable error likely **)
>>>>>>
>>>>>>
>>>>>>        Case 3b: IOMMU on in system kernel, kdump kernel disables IOMMU
>>>>>>          system kernel enables IOMMU
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kexec to kdump kernel (IOMMU on, devices untouched)
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kdump kernel disables IOMMU
>>>>>>            DMA targets IOMMU-mapped system-kernel memory, but IOMMU is disabled
>>>>>>            (** corruption or other non-recoverable error likely **)
>>>>>>          kdump kernel re-inits device
>>>>>>            DMA targets kdump-kernel memory
>>>>>>
>>>>>>        Case 4: IOMMU on in system kernel, on in kdump kernel
>>>>>>          system kernel enables IOMMU
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kexec to kdump kernel (IOMMU on, devices untouched)
>>>>>>            DMA targets IOMMU-mapped system-kernel memory
>>>>>>          kdump kernel enables IOMMU with no valid mappings
>>>>>>            DMA causes IOMMU errors (annoying but harmless)
>>>>>>          kdump kernel re-inits device
>>>>>>            DMA targets IOMMU-mapped kdump-kernel memory
>>>>>
>>>>> This is not harmless. Errors like PCI SERR are detected here, and it
>>>>> makes driver or system unstable, and kdump fails. I also got report that
>>>>> system hangs up due to this.
>>>>
>>>> OK, let's take this slowly.  Does an IOMMU error in the system kernel
>>>> also cause SERR or make the system unstable?  Is that the expected
>>>> behavior on IOMMU errors, or is there something special about the
>>>> kdump scenario that causes SERRs?  I see lots of DMAR errors, e.g.,
>>>> those in https://bugzilla.redhat.com/show_bug.cgi?id=743790, that are
>>>> reported with printk and don't seem to cause an SERR.  Maybe the SERR
>>>> is system-specific behavior?
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=568153 is another (public)
>>>> report of IOMMU errors related to a driver bug where we just get
>>>> printks, not SERR.
>>>
>>> Yes, it depends on platform or devices. At least PCI SERR is detected on
>>> Fujitsu PRIMERGY BX920S2 and TX300S6.
>>>
>>> Intel VT-d documents[1] says:
>>>
>>>     3.5.1 Hardware Handling of Faulting DMA Requests
>>>     DMA requests that result in remapping faults must be blocked by
>>>     hardware. The exact method of DMA blocking is
>>>     implementation-specific.  For example:
>>>
>>>     - Faulting DMA write requests may be handled in much the same way as
>>>       hardware handles write requests to non-existent memory. For
>>>       example, the DMA request is discarded in a manner convenient for
>>>       implementations (such as by dropping the cycle, completing the
>>>       write request to memory with all byte enables masked off,
>>>       re-directed to a dummy memory location, etc.).
>>>
>>>     - Faulting DMA read requests may be handled in much the same way as
>>>       hardware handles read requests to non-existent memory. For
>>>       example, the request may be redirected to a dummy memory location,
>>>       returned as all 0<92>s or 1<92>s in a manner convenient to the
>>>       implementation, or the request may be completed with an explicit
>>>       error indication (preferred). For faulting DMA read requests from
>>>       PCI Express devices, hardware must indicate "Unsupported Request"
>>>       (UR) in the completion status field of the PCI Express read
>>>       completion.
>>>
>>> So, after IOMMU error, its behavior is implementation-specific.
>>>
>>> [1]
>>> Intel Virtualization Technology for Directed I/O Architecture
>>> Specification Rev1.3
>>>
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=743495 looks like a hang
>>>> when the kdump kernel reboots (after successfully saving a crashdump).
>>>>    But it is using "iommu=pt", which I don't believe makes sense.  The
>>>> scenario is basically case 4 above, but instead of the kdump kernel
>>>> starting with no valid IOMMU mappings, it identity-maps bus addresses
>>>> to physical memory addresses.  That's completely bogus because it's
>>>> certainly not what the system kernel did, so it's entirely likely to
>>>> make the system unstable or hang.  This is not an argument for doing a
>>>> reset; it's an argument for doing something smarter than "iommu=pt" in
>>>> the kdump kernel.
>>
>>>> We might still want to reset PCIe devices, but I want to make sure
>>>> that we're not papering over other issues when we do.  Therefore, I'd
>>>> like to understand why IOMMU errors seem harmless in some cases but
>>>> not in others.
>>>
>>> As I wrote above, IOMMU behavior on error is up to platform/devcies. I
>>> think it also depends on the value of Uncorrectable Error Mask Register
>>> in AER registers of each device.
>>>
>>>>> Case 1:  Harmless
>>>>> Case 2:  Not tested
>>>>> Case 3a: Not tested
>>>>> Case 3b: Cause problem, fixed by my patch
>>>>> Case 4:  Cause problem, fixed by my patch
>>>>>
>>>>> I have never tested case 2 and 3a, but I think it also causes problem.
>>>>
>>>> I do not believe we need to support case 3b (IOMMU on in system kernel
>>>> but disabled in kdump kernel).  There is no way to make that reliable
>>>> unless every single device that may perform DMA is reset, and since
>>>> you don't reset legacy PCI or VGA devices, you're not even proposing
>>>> to do that.
>>>>
>>>> I think we need to support case 1 (for systems with no IOMMU at all)
>>>> and case 4 (IOMMU enabled in both system kernel and kdump kernel).  If
>>>> the kdump kernel can detect whether the IOMMU is enabled, that should
>>>> be enough -- it could figure out automatically whether we're in case 1
>>>> or 4.
>>>
>>> Ok, I completely agree.
>>>
>>>
>>>>>> Do you have any bugzilla references or problem report URLs you could
>>>>>> include here?
>>>>>
>>>>> I know three Red Hat bugzilla, but I think these are private article and
>>>>> you cannot see. I'll add you Cc list in bz so that you can see.
>>>>>
>>>>> BZ#743790: Kdump fails with intel_iommu=on
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=743790
>>>>>
>>>>> BZ#743495: Hardware error is detected with intel_iommu=on
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=743495
>>>>>
>>>>> BZ#833299: megaraid_sas doesn't work well with intel_iommu=on and iommu=pt
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=833299
>>>>
>>>> Thanks for adding me to the CC lists.  I looked all three and I'm not
>>>> sure there's anything sensitive in them.  It'd be nice if they could
>>>> be made public if there's not.
>>>
>>> I really appreciate your comments! I'll confirm I can make it public.
>>
>> I would greatly appreciate being able to see the bugzillas relating to
>> this patch.
>>>
>>> Thanks,
>>> Takao Indoh
>>
>> Thinking out of the box:
>> Much of the discussion about dealing with the ongoing DMA leftover
>> from the system kernel has assumed that the crash kernel will reset
>> the IOMMU -- which causes various problems if done while there is any DMA
>> still active -- which leads to the idea of stopping all of the DMA.
>>
>> Suppose the crash kernel does not reset the hardware IOMMU, but simply
>> detects that it is active, resets only the devices that are necessary
>> for the crash kernel to operate, and re-programs only the translations
> 
> This suggestion brings up this one:
> Only reset the devices that are generating IOMMU faults, i.e., add a kdump kernel
> hook to the IOMMU fault handler.  While kdump kernel re-initing, IOMMU faults are grabbed
> by the kexec'd kernel and the device is reset. IOW, directed, kdump device reset.
> If no faults, then device is idle, or re-init'd and using new maps, and
> all is well.  Once kdump kernel fully init'd, then just return from
> kdump kernel callout, and let system do its fault handling.
> It can be made mostly common (reset code in kexec mod under drivers/iommu),
> with simple calls out from each IOMMU fault handler.

What I tried before is:
1) Prepare work queue handler
2) In IOMMU fault handler, wake up the work queue handler
3) In the work queue handler, reset devices against the error
   source.

As a result, this method did not work. The device are reset during its
initialization. Please take a look at the message below.  This is a
message when megaraid_sas driver is loaded in second kernel.

megasas: 00.00.05.40-rh2 Thu. Aug. 4 17:00:00 PDT 2011
megaraid_sas 0000:01:00.0: resetting MSI-X
megasas: 0x1000:0x0073:0x1734:0x1177: bus 1:slot 0:func 0
megaraid_sas 0000:01:00.0: PCI INT A -> GSI 28 (level, low) -> IRQ 28
megaraid_sas 0000:01:00.0: setting latency timer to 64
megasas: Waiting for FW to come to ready state
DRHD: handling fault status reg 702
DMAR:[DMA Write] Request device [01:00.0] fault addr ffff9000
DMAR:[fault reason 05] PTE Write access is not set
megasas: FW now in Ready state
  alloc irq_desc for 51 on node -1
  alloc kstat_irqs on node -1
megaraid_sas 0000:01:00.0: irq 51 for MSI/MSI-X
megasas_init_mfi: fw_support_ieee=67108864


Note that DMAR error is detected during driver initialization. So when I
added reset method which I mentioned above, the device is reset here,
and after that, megasas could not find its disks and kdump failed.

Thanks,
Takao Indoh


> 
> Of course, this assumes that systems don't turn IOMMU faults into system SERRs,
> and crash the system.  IMO, as I've stated to a number of system developers,
> IOMMU (mapping) faults should not crash the system -- they already isolate, and
> prevent corruption, so worse case, some part of the system will stop doing I/O,
> and that will either get retried, or aborted and a cleaner panic (and kdump
> kernel switch) will ensue.
> 
>> for those devices.  All other translations remain the same (and remain valid)
>> so all leftover DMA continues into its buffer in the system kernel area
>> where it is harmless.  New translations needed by the kdump kernel are
>> added to the existing tables.
>>
> A crashed system shouldn't assume that ongoing DMA is sane.... it should be stopped.
> I would expect the kdump kernel to reset devices & acquire new dma mappings
> upon reboot.   Thus, the only issue is how to 'throttle' IOMMU faults, and
> not allow them to crash systems while the system is recovering/resetting itself,
> but it's not one big (power) reset to cause it.
> 
>> I have not yet tried this, so I am not ready to propose it as anything more
>> than a discussion topic at this time.
>>
>> It might work this way: (A small modification to case 3a above)
>>
>> IOMMU on in system kernel, kdump kernel accepts active IOMMU
>>     system kernel enables IOMMU
>>        DMA targets IOMMU-mapped system-kernel memory
>>     kexec to kdump kernel (IOMMU on, devices untouched)
>>        DMA targets IOMMU-mapped system-kernel memory
> it may not, it may be bad/bogus device I/O causing the crash...
> 
>>     kdump kernel detects active IOMMU and doesn't touch it
>>        DMA targets IOMMU-mapped system-kernel memory
>>     kdump kernel does not re-initialize IOMMU hardware
>>     kdump kernel initializes IOMMU in-memory management structures
>>     kdump kernel calls device drivers' standard initialization functions
>>        Drivers initialize their own devices -- DMA from that device stops
>>        When drivers request new DMA mappings, the kdump IOMMU driver:
>>        1. Updates its in-memory mgt structures for that device&  range
>>        2. Updates IOMMU translate tables for that device&  range
>>           . Translations for all other devices&  ranges are unchanged
>>        3. Flushes IOMMU TLB to force IOMMU hardware update
>>
>>     Notes:
>>        A. This seems very similar to case 1
>>
>>        B. Only touch devices with drivers loaded into kdump kernel.
>>           . No need to touch devices that kdump is not going to use.
>>
>>        C. This assumes that all DMA device drivers used by kdump will
>>           initialize the device before requesting DMA mappings.
>>           . Seems reasonable, but need to confirm how many do (or don't)
>>           . Only device drivers that might be included in the kdump
>>             kernel need to observe this initialization ordering.
>>
>>        D. Could copy system kernel's translate tables into kdump kernel
>>           and adjust pointers if this feels more trustworthy than using
>>           the original structures where they are in the system kernel.
>>
>>        E. Handle IRQ remappings in a similar manner.
> An even more serious attack vector: flood system w/interrupts (by assigned device in a guest),
> effectively creating a DoS, b/c intrs caused raised (hw) ipl, while bogus DMA does
> not cause system ipl elevation(blockage of other system progress), except when it
> generates IOMMU faults which become intrs.
> hmmm, wondering if another solution is to flip IOMMU fault handling into a poll-mode
> at kdump init time, and then flip it to intr-driven, once I/O has been completely init'd ?
> Per-device fault throttling would be a nice hw feature! (wishful thinking)
> 
> 
>>
>> Thanks,
>> Bill Sumner
>>
>>
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/