| Message ID | 20180427192207.GG8199@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Bjorn, Thanks for adding me. On 4/27/2018 3:22 PM, Bjorn Helgaas wrote: > Sinan mooted the idea of using a "no-wait" path of sending the "don't > generate hotplug interrupts" command. I think we should work on this > idea a little more. If we're shutting down the whole system, I can't > believe there's much value in *anything* we do in the pciehp_remove() > path. > > Maybe we should just get rid of pciehp_remove() (and probably > pcie_port_remove_service() and the other service driver remove methods) > completely. That dates from when the service drivers could be modules that > could be potentially unloaded, but unloading them hasn't been possible for > years. Shutdown path is also used for kexec. Leaving hotplug interrupts pending is dangerous for the newly loaded kernel as it leaves spurious interrupts during the new kernel boot. I think we should always disable the hotplug interrupt on shutdown. We might think of not waiting for command-completion as a middle-ground or go to polling path instead of interrupts all the time. Sinan
[+cc Eric, Vivek, kexec list] On Fri, Apr 27, 2018 at 03:34:30PM -0400, Sinan Kaya wrote: > On 4/27/2018 3:22 PM, Bjorn Helgaas wrote: > > Sinan mooted the idea of using a "no-wait" path of sending the "don't > > generate hotplug interrupts" command. I think we should work on this > > idea a little more. If we're shutting down the whole system, I can't > > believe there's much value in *anything* we do in the pciehp_remove() > > path. > > > > Maybe we should just get rid of pciehp_remove() (and probably > > pcie_port_remove_service() and the other service driver remove methods) > > completely. That dates from when the service drivers could be modules that > > could be potentially unloaded, but unloading them hasn't been possible for > > years. > > Shutdown path is also used for kexec. Leaving hotplug interrupts > pending is dangerous for the newly loaded kernel as it leaves > spurious interrupts during the new kernel boot. > > I think we should always disable the hotplug interrupt on shutdown. > We might think of not waiting for command-completion as a > middle-ground or go to polling path instead of interrupts all the > time. Ah, I forgot about the kexec path. The kexec path is used for crashdump, too, so ideally the newly-loaded kernel would defend itself when possible so it doesn't depend on the original kernel doing things correctly. Seems like this question of whether to do things in the original kernel or the kexec-ed kernel comes up periodically, but I can never remember a definitive answer. My initial reaction is that it'd be nice if we didn't have to do *any* shutdown in the original kernel, but I'm sure there are reasons that's not practical. I copied Eric (kexec maintainer) and Vivek (contact listed in Documentation/kdump/kdump.txt) in case they have suggestions or would consider some sort of Documentation/ update. Bjorn
On 04/27/18 at 04:12pm, Bjorn Helgaas wrote: > [+cc Eric, Vivek, kexec list] > > On Fri, Apr 27, 2018 at 03:34:30PM -0400, Sinan Kaya wrote: > > On 4/27/2018 3:22 PM, Bjorn Helgaas wrote: > > > Sinan mooted the idea of using a "no-wait" path of sending the "don't > > > generate hotplug interrupts" command. I think we should work on this > > > idea a little more. If we're shutting down the whole system, I can't > > > believe there's much value in *anything* we do in the pciehp_remove() > > > path. > > > > > > Maybe we should just get rid of pciehp_remove() (and probably > > > pcie_port_remove_service() and the other service driver remove methods) > > > completely. That dates from when the service drivers could be modules that > > > could be potentially unloaded, but unloading them hasn't been possible for > > > years. > > > > Shutdown path is also used for kexec. Leaving hotplug interrupts > > pending is dangerous for the newly loaded kernel as it leaves > > spurious interrupts during the new kernel boot. > > > > I think we should always disable the hotplug interrupt on shutdown. > > We might think of not waiting for command-completion as a > > middle-ground or go to polling path instead of interrupts all the > > time. > > Ah, I forgot about the kexec path. The kexec path is used for > crashdump, too, so ideally the newly-loaded kernel would defend itself > when possible so it doesn't depend on the original kernel doing things > correctly. It is true for kdump. But kexec needs device shutdown. > > Seems like this question of whether to do things in the original > kernel or the kexec-ed kernel comes up periodically, but I can never > remember a definitive answer. My initial reaction is that it'd be > nice if we didn't have to do *any* shutdown in the original kernel, > but I'm sure there are reasons that's not practical. Devices sometimes assume it is in a good state initialized in firmware boot phase, so we need a shutdown in 1st kernel so that kexec kernel can boot correctly for those devices. For kdump since kernel already panicked and it is not reliable so we do as less as we can in the 1st kernel crash path, but there are some special handling for kdump in various drivers to reset the devices in 2nd kernel, eg. when it see "reset_devices" kernel parameter. > > I copied Eric (kexec maintainer) and Vivek (contact listed in > Documentation/kdump/kdump.txt) in case they have suggestions or would > consider some sort of Documentation/ update. > > Bjorn > > _______________________________________________ > kexec mailing list > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec Thanks Dave
On 04/28/18 at 08:56am, Dave Young wrote: > On 04/27/18 at 04:12pm, Bjorn Helgaas wrote: > > [+cc Eric, Vivek, kexec list] > > > > On Fri, Apr 27, 2018 at 03:34:30PM -0400, Sinan Kaya wrote: > > > On 4/27/2018 3:22 PM, Bjorn Helgaas wrote: > > > > Sinan mooted the idea of using a "no-wait" path of sending the "don't > > > > generate hotplug interrupts" command. I think we should work on this > > > > idea a little more. If we're shutting down the whole system, I can't > > > > believe there's much value in *anything* we do in the pciehp_remove() > > > > path. > > > > > > > > Maybe we should just get rid of pciehp_remove() (and probably > > > > pcie_port_remove_service() and the other service driver remove methods) > > > > completely. That dates from when the service drivers could be modules that Hmm, if it is the remove() method then kexec does not use it. kexec use the shutdown() method instead. I missed this details when I replied. > > > > could be potentially unloaded, but unloading them hasn't been possible for > > > > years. > > > > > > Shutdown path is also used for kexec. Leaving hotplug interrupts > > > pending is dangerous for the newly loaded kernel as it leaves > > > spurious interrupts during the new kernel boot. > > > > > > I think we should always disable the hotplug interrupt on shutdown. > > > We might think of not waiting for command-completion as a > > > middle-ground or go to polling path instead of interrupts all the > > > time. > > > > Ah, I forgot about the kexec path. The kexec path is used for > > crashdump, too, so ideally the newly-loaded kernel would defend itself > > when possible so it doesn't depend on the original kernel doing things > > correctly. > > It is true for kdump. But kexec needs device shutdown. > > > > > Seems like this question of whether to do things in the original > > kernel or the kexec-ed kernel comes up periodically, but I can never > > remember a definitive answer. My initial reaction is that it'd be > > nice if we didn't have to do *any* shutdown in the original kernel, > > but I'm sure there are reasons that's not practical. > > Devices sometimes assume it is in a good state initialized in firmware boot > phase, so we need a shutdown in 1st kernel so that kexec kernel can boot > correctly for those devices. For kdump since kernel already panicked > and it is not reliable so we do as less as we can in the 1st kernel > crash path, but there are some special handling for kdump in various drivers > to reset the devices in 2nd kernel, eg. when it see "reset_devices" kernel parameter. > > > > > I copied Eric (kexec maintainer) and Vivek (contact listed in > > Documentation/kdump/kdump.txt) in case they have suggestions or would > > consider some sort of Documentation/ update. > > > > Bjorn > > > > _______________________________________________ > > kexec mailing list > > kexec@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/kexec > > Thanks > Dave > > _______________________________________________ > kexec mailing list > kexec@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/kexec
On 2018-04-27 21:18, Dave Young wrote: > On 04/28/18 at 08:56am, Dave Young wrote: >> On 04/27/18 at 04:12pm, Bjorn Helgaas wrote: >> > [+cc Eric, Vivek, kexec list] >> > >> > On Fri, Apr 27, 2018 at 03:34:30PM -0400, Sinan Kaya wrote: >> > > On 4/27/2018 3:22 PM, Bjorn Helgaas wrote: >> > > > Sinan mooted the idea of using a "no-wait" path of sending the "don't >> > > > generate hotplug interrupts" command. I think we should work on this >> > > > idea a little more. If we're shutting down the whole system, I can't >> > > > believe there's much value in *anything* we do in the pciehp_remove() >> > > > path. >> > > > >> > > > Maybe we should just get rid of pciehp_remove() (and probably >> > > > pcie_port_remove_service() and the other service driver remove methods) >> > > > completely. That dates from when the service drivers could be modules that > > Hmm, if it is the remove() method then kexec does not use it. kexec > use > the shutdown() method instead. I missed this details when I replied. Portdrv hooks up remove handler to shutdown. That's why remove is getting called. > >> > > > could be potentially unloaded, but unloading them hasn't been possible for >> > > > years. >> > > >> > > Shutdown path is also used for kexec. Leaving hotplug interrupts >> > > pending is dangerous for the newly loaded kernel as it leaves >> > > spurious interrupts during the new kernel boot. >> > > >> > > I think we should always disable the hotplug interrupt on shutdown. >> > > We might think of not waiting for command-completion as a >> > > middle-ground or go to polling path instead of interrupts all the >> > > time. >> > >> > Ah, I forgot about the kexec path. The kexec path is used for >> > crashdump, too, so ideally the newly-loaded kernel would defend itself >> > when possible so it doesn't depend on the original kernel doing things >> > correctly. >> >> It is true for kdump. But kexec needs device shutdown. >> >> > >> > Seems like this question of whether to do things in the original >> > kernel or the kexec-ed kernel comes up periodically, but I can never >> > remember a definitive answer. My initial reaction is that it'd be >> > nice if we didn't have to do *any* shutdown in the original kernel, >> > but I'm sure there are reasons that's not practical. >> >> Devices sometimes assume it is in a good state initialized in firmware >> boot >> phase, so we need a shutdown in 1st kernel so that kexec kernel can >> boot >> correctly for those devices. For kdump since kernel already panicked >> and it is not reliable so we do as less as we can in the 1st kernel >> crash path, but there are some special handling for kdump in various >> drivers >> to reset the devices in 2nd kernel, eg. when it see "reset_devices" >> kernel parameter. >> >> > >> > I copied Eric (kexec maintainer) and Vivek (contact listed in >> > Documentation/kdump/kdump.txt) in case they have suggestions or would >> > consider some sort of Documentation/ update. >> > >> > Bjorn >> > >> > _______________________________________________ >> > kexec mailing list >> > kexec@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/kexec >> >> Thanks >> Dave >> >> _______________________________________________ >> kexec mailing list >> kexec@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/kexec
Bjorn, On 4/28/2018 9:03 AM, okaya@codeaurora.org wrote: >> Hmm, if it is the remove() method then kexec does not use it. kexec use >> the shutdown() method instead. I missed this details when I replied. > > Portdrv hooks up remove handler to shutdown. That's why remove is getting called. What should we do about this? Since there is an actual HW errata involved, should we quirk this root port and not wait as if remove/shutdown doesn't exist? Paul, You might want to file a bugzilla so that we can keep our debug efforts out of this list. Sinan
On Mon, Apr 30, 2018 at 04:48:15PM -0400, Sinan Kaya wrote: > Bjorn, > > On 4/28/2018 9:03 AM, okaya@codeaurora.org wrote: > >> Hmm, if it is the remove() method then kexec does not use it. kexec use > >> the shutdown() method instead. I missed this details when I replied. > > > > Portdrv hooks up remove handler to shutdown. That's why remove is getting called. > > What should we do about this? > > Since there is an actual HW errata involved, should we quirk this > root port and not wait as if remove/shutdown doesn't exist? I was hoping to avoid a quirk because AFAIK all Intel parts have this issue so it will be an ongoing maintenance issue. I tried to avoid the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute timeout from hotplug command start time"). But we still see the alarming messages, so we should probably add a quirk to get rid of those. But I haven't given up on the idea of getting rid of the pciehp_remove() path. I'm not convinced yet that we actually need to do anything to shut this device down. I don't like the assumption that kexec requires this. The kexec is fundamentally just a branch, and anything we do before the branch (i.e., in the old kernel), we should also be able to do after the branch (i.e., in the kexec-ed kernel). > Paul, > You might want to file a bugzilla so that we can keep our debug > efforts out of this list.
On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: >> What should we do about this? >> >> Since there is an actual HW errata involved, should we quirk this >> root port and not wait as if remove/shutdown doesn't exist? > I was hoping to avoid a quirk because AFAIK all Intel parts have this > issue so it will be an ongoing maintenance issue. I tried to avoid > the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute > timeout from hotplug command start time"). > > But we still see the alarming messages, so we should probably add a > quirk to get rid of those. > > But I haven't given up on the idea of getting rid of the > pciehp_remove() path. I'm not convinced yet that we actually need to > do anything to shut this device down. I don't like the assumption > that kexec requires this. The kexec is fundamentally just a branch, > and anything we do before the branch (i.e., in the old kernel), we > should also be able to do after the branch (i.e., in the kexec-ed > kernel). > In my experience with kexec, MSI type edge interrupts are harmless. You might just see a few unhandled interrupt messages during boot if something is pending from the first kernel. It is the level interrupts that are more concerning. It remains pending until the interrupt source is cleared. CPU never returns from the interrupt handler to actually continue booting the second kernel. Execution doesn't reach to PCIe hp driver initialization for acknowledging the interrupt. How about remove() only if MSI is disabled? Most root port interrupts are MSI based anyhow.
On Mon, Apr 30, 2018 at 05:27:32PM -0400, Sinan Kaya wrote: > On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: > How about remove() only if MSI is disabled? Most root port interrupts > are MSI based anyhow. I think the code is perfectly fine as it is and doesn't need to nor should be modified. We should just add a quirk for this particular chipset that it doesn't support Command Completed interrupts (if that's what's broken). In the case of Thunderbolt controllers, the first few had the NoCompl bit set incorrectly in config space and Intel hardware engineers confirmed the error. These or similar oddities may occur with more chipsets, so I propose moving existing quirks to a separate file, thereby avoiding that they clutter generic code. The blinkenlights feature added with commit 576243b3f9ea is a similar quirk that could be moved to a separate pciehp_quirks.c file. Thanks, Lukas
+Marc, On 4/30/2018 5:27 PM, Sinan Kaya wrote: > On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: >>> What should we do about this? >>> >>> Since there is an actual HW errata involved, should we quirk this >>> root port and not wait as if remove/shutdown doesn't exist? >> I was hoping to avoid a quirk because AFAIK all Intel parts have this >> issue so it will be an ongoing maintenance issue. I tried to avoid >> the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute >> timeout from hotplug command start time"). >> >> But we still see the alarming messages, so we should probably add a >> quirk to get rid of those. >> >> But I haven't given up on the idea of getting rid of the >> pciehp_remove() path. I'm not convinced yet that we actually need to >> do anything to shut this device down. I don't like the assumption >> that kexec requires this. The kexec is fundamentally just a branch, >> and anything we do before the branch (i.e., in the old kernel), we >> should also be able to do after the branch (i.e., in the kexec-ed >> kernel). >> > > In my experience with kexec, MSI type edge interrupts are harmless. > You might just see a few unhandled interrupt messages during boot > if something is pending from the first kernel. > > It is the level interrupts that are more concerning. It remains pending > until the interrupt source is cleared. CPU never returns from the > interrupt handler to actually continue booting the second kernel. This makes me wonder why kexec doesn't disable all interrupt sources by itself instead of relying on the drivers shutdown routine. Some drivers don't even have a shutdown callback. Kexec could have done both as another example. Something like. 1. Call shutdown for all drivers if available. 2. Disable all interrupt sources in the interrupt controller 3. Start the new kernel. > > Execution doesn't reach to PCIe hp driver initialization for > acknowledging the interrupt. > > How about remove() only if MSI is disabled? Most root port interrupts > are MSI based anyhow. >
On 01/05/18 13:38, Sinan Kaya wrote: > +Marc, > > On 4/30/2018 5:27 PM, Sinan Kaya wrote: >> On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: >>>> What should we do about this? >>>> >>>> Since there is an actual HW errata involved, should we quirk this >>>> root port and not wait as if remove/shutdown doesn't exist? >>> I was hoping to avoid a quirk because AFAIK all Intel parts have this >>> issue so it will be an ongoing maintenance issue. I tried to avoid >>> the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute >>> timeout from hotplug command start time"). >>> >>> But we still see the alarming messages, so we should probably add a >>> quirk to get rid of those. >>> >>> But I haven't given up on the idea of getting rid of the >>> pciehp_remove() path. I'm not convinced yet that we actually need to >>> do anything to shut this device down. I don't like the assumption >>> that kexec requires this. The kexec is fundamentally just a branch, >>> and anything we do before the branch (i.e., in the old kernel), we >>> should also be able to do after the branch (i.e., in the kexec-ed >>> kernel). >>> >> >> In my experience with kexec, MSI type edge interrupts are harmless. >> You might just see a few unhandled interrupt messages during boot >> if something is pending from the first kernel. Unfortunately, that's not always the case. A number of GICv3/v4 implementations (a very common interrupt controller on ARM servers) cannot be disabled, which means they will keep writing to their pending tables long after kexec will have started the new kernel. And since we don't track memory allocation across kexec, you end-up with significant chances of observing single bit corruption as interrupts carry on being delivered. Oh, and you won't actually be able to take MSIs because you can't even reprogram the damn thing. Yes, this can be considered a HW bug. >> It is the level interrupts that are more concerning. It remains pending >> until the interrupt source is cleared. CPU never returns from the >> interrupt handler to actually continue booting the second kernel. > > This makes me wonder why kexec doesn't disable all interrupt sources by > itself instead of relying on the drivers shutdown routine. Some drivers > don't even have a shutdown callback. Kexec could have done both as another > example. Something like. > > 1. Call shutdown for all drivers if available. > 2. Disable all interrupt sources in the interrupt controller > 3. Start the new kernel. See above. Although you can shut off the end-point and to some extent mask interrupts before jumping into the payload, it is not always possible to go back to a reasonable state where you can take actually MSIs. Thanks, M.
On Tue, May 01, 2018 at 01:59:20PM +0100, Marc Zyngier wrote: > On 01/05/18 13:38, Sinan Kaya wrote: > > +Marc, > > > > On 4/30/2018 5:27 PM, Sinan Kaya wrote: > >> On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: > >>>> What should we do about this? > >>>> > >>>> Since there is an actual HW errata involved, should we quirk this > >>>> root port and not wait as if remove/shutdown doesn't exist? > >>> I was hoping to avoid a quirk because AFAIK all Intel parts have this > >>> issue so it will be an ongoing maintenance issue. I tried to avoid > >>> the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute > >>> timeout from hotplug command start time"). > >>> > >>> But we still see the alarming messages, so we should probably add a > >>> quirk to get rid of those. > >>> > >>> But I haven't given up on the idea of getting rid of the > >>> pciehp_remove() path. I'm not convinced yet that we actually need to > >>> do anything to shut this device down. I don't like the assumption > >>> that kexec requires this. The kexec is fundamentally just a branch, > >>> and anything we do before the branch (i.e., in the old kernel), we > >>> should also be able to do after the branch (i.e., in the kexec-ed > >>> kernel). > >>> > >> > >> In my experience with kexec, MSI type edge interrupts are harmless. > >> You might just see a few unhandled interrupt messages during boot > >> if something is pending from the first kernel. > > Unfortunately, that's not always the case. > > A number of GICv3/v4 implementations (a very common interrupt controller > on ARM servers) cannot be disabled, which means they will keep writing > to their pending tables long after kexec will have started the new > kernel. And since we don't track memory allocation across kexec, you > end-up with significant chances of observing single bit corruption as > interrupts carry on being delivered. Oh, and you won't actually be able > to take MSIs because you can't even reprogram the damn thing. > > Yes, this can be considered a HW bug. > > >> It is the level interrupts that are more concerning. It remains pending > >> until the interrupt source is cleared. CPU never returns from the > >> interrupt handler to actually continue booting the second kernel. > > > > This makes me wonder why kexec doesn't disable all interrupt sources by > > itself instead of relying on the drivers shutdown routine. Some drivers > > don't even have a shutdown callback. Kexec could have done both as another > > example. Something like. > > > > 1. Call shutdown for all drivers if available. > > 2. Disable all interrupt sources in the interrupt controller > > 3. Start the new kernel. > > See above. Although you can shut off the end-point and to some extent > mask interrupts before jumping into the payload, it is not always > possible to go back to a reasonable state where you can take actually MSIs. This is exactly the sort of thing it would be nice to collect and document as part of the background of "why kexec works the way it does." It certainly helps explain things that are far from obvious if you don't have the background.
On Tue, 01 May 2018 14:25:54 +0100, Bjorn Helgaas wrote: Hi Bjorn, > On Tue, May 01, 2018 at 01:59:20PM +0100, Marc Zyngier wrote: > > On 01/05/18 13:38, Sinan Kaya wrote: > > > +Marc, > > > > > > On 4/30/2018 5:27 PM, Sinan Kaya wrote: > > >> On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: > > >>>> What should we do about this? > > >>>> > > >>>> Since there is an actual HW errata involved, should we quirk this > > >>>> root port and not wait as if remove/shutdown doesn't exist? > > >>> I was hoping to avoid a quirk because AFAIK all Intel parts have this > > >>> issue so it will be an ongoing maintenance issue. I tried to avoid > > >>> the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute > > >>> timeout from hotplug command start time"). > > >>> > > >>> But we still see the alarming messages, so we should probably add a > > >>> quirk to get rid of those. > > >>> > > >>> But I haven't given up on the idea of getting rid of the > > >>> pciehp_remove() path. I'm not convinced yet that we actually need to > > >>> do anything to shut this device down. I don't like the assumption > > >>> that kexec requires this. The kexec is fundamentally just a branch, > > >>> and anything we do before the branch (i.e., in the old kernel), we > > >>> should also be able to do after the branch (i.e., in the kexec-ed > > >>> kernel). > > >>> > > >> > > >> In my experience with kexec, MSI type edge interrupts are harmless. > > >> You might just see a few unhandled interrupt messages during boot > > >> if something is pending from the first kernel. > > > > Unfortunately, that's not always the case. > > > > A number of GICv3/v4 implementations (a very common interrupt controller > > on ARM servers) cannot be disabled, which means they will keep writing > > to their pending tables long after kexec will have started the new > > kernel. And since we don't track memory allocation across kexec, you > > end-up with significant chances of observing single bit corruption as > > interrupts carry on being delivered. Oh, and you won't actually be able > > to take MSIs because you can't even reprogram the damn thing. > > > > Yes, this can be considered a HW bug. > > > > >> It is the level interrupts that are more concerning. It remains pending > > >> until the interrupt source is cleared. CPU never returns from the > > >> interrupt handler to actually continue booting the second kernel. > > > > > > This makes me wonder why kexec doesn't disable all interrupt sources by > > > itself instead of relying on the drivers shutdown routine. Some drivers > > > don't even have a shutdown callback. Kexec could have done both as another > > > example. Something like. > > > > > > 1. Call shutdown for all drivers if available. > > > 2. Disable all interrupt sources in the interrupt controller > > > 3. Start the new kernel. > > > > See above. Although you can shut off the end-point and to some extent > > mask interrupts before jumping into the payload, it is not always > > possible to go back to a reasonable state where you can take actually MSIs. > > This is exactly the sort of thing it would be nice to collect and > document as part of the background of "why kexec works the way it > does." It certainly helps explain things that are far from obvious if > you don't have the background. I'd certainly be happy to help with it if someone was willing to kickstart such a document. kexec/kdump is a huge bag of "interesting" tricks, and it has driven me mad over the past couple of months (I'm typing this from a laptop that uses kexec as its bootloader, and it is *not fun*). M.
Marc Zyngier <marc.zyngier@arm.com> writes: > On Tue, 01 May 2018 14:25:54 +0100, > Bjorn Helgaas wrote: > > Hi Bjorn, > >> On Tue, May 01, 2018 at 01:59:20PM +0100, Marc Zyngier wrote: >> > On 01/05/18 13:38, Sinan Kaya wrote: >> > > +Marc, >> > > >> > > On 4/30/2018 5:27 PM, Sinan Kaya wrote: >> > >> On 4/30/2018 5:17 PM, Bjorn Helgaas wrote: >> > >>>> What should we do about this? >> > >>>> >> > >>>> Since there is an actual HW errata involved, should we quirk this >> > >>>> root port and not wait as if remove/shutdown doesn't exist? >> > >>> I was hoping to avoid a quirk because AFAIK all Intel parts have this >> > >>> issue so it will be an ongoing maintenance issue. I tried to avoid >> > >>> the timeout delays, e.g., with 40b960831cfa ("PCI: pciehp: Compute >> > >>> timeout from hotplug command start time"). >> > >>> >> > >>> But we still see the alarming messages, so we should probably add a >> > >>> quirk to get rid of those. >> > >>> >> > >>> But I haven't given up on the idea of getting rid of the >> > >>> pciehp_remove() path. I'm not convinced yet that we actually need to >> > >>> do anything to shut this device down. I don't like the assumption >> > >>> that kexec requires this. The kexec is fundamentally just a branch, >> > >>> and anything we do before the branch (i.e., in the old kernel), we >> > >>> should also be able to do after the branch (i.e., in the kexec-ed >> > >>> kernel). >> > >>> >> > >> >> > >> In my experience with kexec, MSI type edge interrupts are harmless. >> > >> You might just see a few unhandled interrupt messages during boot >> > >> if something is pending from the first kernel. >> > >> > Unfortunately, that's not always the case. >> > >> > A number of GICv3/v4 implementations (a very common interrupt controller >> > on ARM servers) cannot be disabled, which means they will keep writing >> > to their pending tables long after kexec will have started the new >> > kernel. And since we don't track memory allocation across kexec, you >> > end-up with significant chances of observing single bit corruption as >> > interrupts carry on being delivered. Oh, and you won't actually be able >> > to take MSIs because you can't even reprogram the damn thing. >> > >> > Yes, this can be considered a HW bug. >> > >> > >> It is the level interrupts that are more concerning. It remains pending >> > >> until the interrupt source is cleared. CPU never returns from the >> > >> interrupt handler to actually continue booting the second kernel. >> > > >> > > This makes me wonder why kexec doesn't disable all interrupt sources by >> > > itself instead of relying on the drivers shutdown routine. Some drivers >> > > don't even have a shutdown callback. Kexec could have done both as another >> > > example. Something like. >> > > >> > > 1. Call shutdown for all drivers if available. >> > > 2. Disable all interrupt sources in the interrupt controller >> > > 3. Start the new kernel. >> > >> > See above. Although you can shut off the end-point and to some extent >> > mask interrupts before jumping into the payload, it is not always >> > possible to go back to a reasonable state where you can take actually MSIs. >> >> This is exactly the sort of thing it would be nice to collect and >> document as part of the background of "why kexec works the way it >> does." It certainly helps explain things that are far from obvious if >> you don't have the background. > > I'd certainly be happy to help with it if someone was willing to > kickstart such a document. kexec/kdump is a huge bag of "interesting" > tricks, and it has driven me mad over the past couple of months (I'm > typing this from a laptop that uses kexec as its bootloader, and it is > *not fun*). I don't know if it helps documentation wise but here is my memory of why things are the way they are. Case 1) kexec-on-panic. In this case we run the new kernel in memory reserved since boot of the previous kernel in memory has never been used by any device driver. This means on-going DMA transactions that we don't manage to shut off are harmless. In actual execution a bare minimum of hardware is shutdown on the kexec-on-panic path. Ideally it would be nothing. The crashing kernel simply can not be trusted to shut things down itself. The kernel that is executing in the after the crash loads a bare minimum of drivers and does it's best to initialize the hardware. Ideally if something goes wrong the kernel will hang before we write to hardware and mess anything up. With this we get something like a 50% or a 60% success rate of capture crashdump in practice in the field. Everything else that has been tried relies more on the crashing kernel and looks great in testing and then turns out to not have a measurable success rate in practice. Using lkdtm you can setup tests of various kinds of kernel corruption and failure and see some approximation of the success rate of kexec will see in practice. I forget where we are with iommus, but the principles remain and iommus tend to tricky just because they get in the middle of everything. If someone stares hard enough we are probably at the point on x86 where we can remove the irq shutdown code. The kexec on panic case tends to be tested more on enterprise kernels than on normal ones. Case 2) Ordinary kexec. The goal is to have a fully functionaly uncompromised system (unlike kexec on panic). Hardware bugs mean that in the general case the only place we can shutdown hardware reliably is the drivers themselves. All devices doing DMA must be shutdown in the kexec'ing kernel. In part because there is no guarantee that we will even load a driver for that hardware. The presence of DMA drove most of the decisions. But from this thread I see that irq handling follows the same pattern. The best place to shut anything down is in the driver where there is full knowledge of how things work. One of the more annoying things that have been discovered is the generic pci dma disable bit doesn't work uniformly acrosss hardware. Which means there is no known generic way to shut down dma across the board. In the prototypes there was only the "remove" method of drivers and that worked well. When it came time to merge the original kexec implementation the maintainer of the power mananagement subsystem insisted we add a new "shutdown" method instead, because while it is necessary to shutdown the hardware you should not need to clean up the data structures. In practice that idea flopped. The most reliable way I know to run kexec is to remmod all of the drivers before runing sys_reboot(..., LINUX_REBOT_CMD_KEXEC, ...) so that the shutdown methods get run. It has been asked and I have given my approval to anyone who wants to do the work to switch form the "shutdown" methods to "remove" on the kexec path. But so far it is a big enough project that no one has done that yet. It has been suggested that hardware does not need to be shutdown at the end of the kernel before returning to a a firmware method. Which is incorrect. Most firmware when it regains control triggers a system reset to get the hardware back into a usable state, and be able to reboot the system. There is a magic register for this on x86. On older x86 systems and others that transfer control to firmware without doing a soft hardware reset of the system and all of the devices. Without shutting down the devices they will work about as well as kexec does when you don't remove the devices. That is why I merged the reboot and the kexec code paths. Well that and so that there is a little more testing. In practice it still seems that rmmod is the only testing that reliably happens to drivers. So not sharing that code path makes kexec more fragile than necessary. Hopefully this helps put things into perspective and can help with your docuement. Eric
On Fri, Apr 27, 2018 at 02:22:07PM -0500, Bjorn Helgaas wrote: > Sinan mooted the idea of using a "no-wait" path of sending the "don't > generate hotplug interrupts" command. I think we should work on this > idea a little more. If we're shutting down the whole system, I can't > believe there's much value in *anything* we do in the pciehp_remove() > path. > > Maybe we should just get rid of pciehp_remove() (and probably > pcie_port_remove_service() and the other service driver remove methods) > completely. That dates from when the service drivers could be modules that > could be potentially unloaded, but unloading them hasn't been possible for > years. Every Thunderbolt device contains a PCIe switch with at least one (downstream) hotplug port, so pciehp_remove() is executed on unplug of a Thunderbolt device and the assumption that it's unnecessary simply because it's builtin isn't correct. Thanks, Lukas
On Wed, May 09, 2018 at 01:41:24PM +0200, Lukas Wunner wrote: > On Fri, Apr 27, 2018 at 02:22:07PM -0500, Bjorn Helgaas wrote: > > Sinan mooted the idea of using a "no-wait" path of sending the "don't > > generate hotplug interrupts" command. I think we should work on this > > idea a little more. If we're shutting down the whole system, I can't > > believe there's much value in *anything* we do in the pciehp_remove() > > path. > > > > Maybe we should just get rid of pciehp_remove() (and probably > > pcie_port_remove_service() and the other service driver remove methods) > > completely. That dates from when the service drivers could be modules that > > could be potentially unloaded, but unloading them hasn't been possible for > > years. > > Every Thunderbolt device contains a PCIe switch with at least one > (downstream) hotplug port, so pciehp_remove() is executed on unplug > of a Thunderbolt device and the assumption that it's unnecessary > simply because it's builtin isn't correct. I agree that simply being builtin isn't a sufficient argument for getting rid of pciehp_remove(). But if we do need pciehp_remove(), we should be able to make a rational case for why that is. If we're about to turn off the power, it's not obvious why we would need to deallocate memory, remove sysfs stuff, etc. If we need to configure the hardware to make it easier for a kexec'd kernel, that's a possible argument but we should make it explicit. Bjorn
On Wed, May 09, 2018 at 07:57:52AM -0500, Bjorn Helgaas wrote: > On Wed, May 09, 2018 at 01:41:24PM +0200, Lukas Wunner wrote: > > On Fri, Apr 27, 2018 at 02:22:07PM -0500, Bjorn Helgaas wrote: > > > Sinan mooted the idea of using a "no-wait" path of sending the "don't > > > generate hotplug interrupts" command. I think we should work on this > > > idea a little more. If we're shutting down the whole system, I can't > > > believe there's much value in *anything* we do in the pciehp_remove() > > > path. > > > > > > Maybe we should just get rid of pciehp_remove() (and probably > > > pcie_port_remove_service() and the other service driver remove methods) > > > completely. That dates from when the service drivers could be modules that > > > could be potentially unloaded, but unloading them hasn't been possible for > > > years. > > > > Every Thunderbolt device contains a PCIe switch with at least one > > (downstream) hotplug port, so pciehp_remove() is executed on unplug > > of a Thunderbolt device and the assumption that it's unnecessary > > simply because it's builtin isn't correct. > > I agree that simply being builtin isn't a sufficient argument for getting > rid of pciehp_remove(). > > But if we do need pciehp_remove(), we should be able to make a rational > case for why that is. If we're about to turn off the power, it's not > obvious why we would need to deallocate memory, remove sysfs stuff, etc. > If we need to configure the hardware to make it easier for a kexec'd > kernel, that's a possible argument but we should make it explicit. With Thunderbolt, up to 6 devices may be daisy-chained. This means that a hotplug port may have further hotplug ports as (grand-)children. If power is turned off manually via sysfs for a hotplug port, all children (including hotplug ports) are removed by pciehp even though they physically remain attached to the machine. If such removed-in-software-but-physically- still-present devices send an interrupt, and interrupts were not orderly disabled on ->remove, they will be considered spurious interrupts by genirq code. In particular, level-triggered INTx interrupts will immediately lead to an unpleasant user-visible splat and the interrupt will be switched to polling. So there's no way around orderly disabling interrupts in the ->remove path. I agree that ->shutdown is a different story in principle and that disabling devices seems superfluous and counter-intuitive. I imagine kexec might not be the only reason, but also devices passed through to VMs. (What happens if a VM hands a device back to the host in an unclean state on shutdown?) Thanks, Lukas
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index 332b723ff9e6..99751cc52968 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -260,14 +260,6 @@ static int pciehp_probe(struct pcie_device *dev) return -ENODEV; } -static void pciehp_remove(struct pcie_device *dev) -{ - struct controller *ctrl = get_service_data(dev); - - cleanup_slot(ctrl); - pciehp_release_ctrl(ctrl); -} - #ifdef CONFIG_PM static int pciehp_suspend(struct pcie_device *dev) { @@ -305,7 +297,6 @@ static struct pcie_port_service_driver hpdriver_portdrv = { .service = PCIE_PORT_SERVICE_HP, .probe = pciehp_probe, - .remove = pciehp_remove, #ifdef CONFIG_PM .suspend = pciehp_suspend, diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 18a42f8f5dc5..c3a9c47ed061 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -113,7 +113,7 @@ static int pcie_poll_cmd(struct controller *ctrl, int timeout) return 0; /* timeout */ } -static void pcie_wait_cmd(struct controller *ctrl) +static void pcie_wait_cmd(struct controller *ctrl, u16 cmd, u16 mask) { unsigned int msecs = pciehp_poll_mode ? 2500 : 1000; unsigned long duration = msecs_to_jiffies(msecs); @@ -155,10 +155,13 @@ static void pcie_wait_cmd(struct controller *ctrl) * don't change those bits, e.g., commands that merely enable * interrupts. */ - if (!rc) - ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago)\n", + if (!rc) { + ctrl_info(ctrl, "Timeout on hotplug command %#06x (issued %u msec ago), new command %#06x/mask %#06x\n", ctrl->slot_ctrl, - jiffies_to_msecs(jiffies - ctrl->cmd_started)); + jiffies_to_msecs(jiffies - ctrl->cmd_started), + cmd, mask); + dump_stack(); + } } static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd, @@ -172,7 +175,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd, /* * Always wait for any previous command that might still be in progress */ - pcie_wait_cmd(ctrl); + pcie_wait_cmd(ctrl, cmd, mask); pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl); if (slot_ctrl == (u16) ~0) { @@ -193,7 +196,7 @@ static void pcie_do_write_cmd(struct controller *ctrl, u16 cmd, * indicating completion of the above issued command. */ if (wait) - pcie_wait_cmd(ctrl); + pcie_wait_cmd(ctrl, cmd, mask); out: mutex_unlock(&ctrl->ctrl_lock);