[PATCH v2] PCI/MSI: Don't touch MSI bits when the PCI device is disconnected

[Alex_Gagniuc at Dellteam.com]

+ Borislav (ACPI guy, maybe he can answer more about HEST)

On 11/15/2018 12:24 AM, Bjorn Helgaas wrote:
> On Wed, Nov 14, 2018 at 07:22:04PM +0000, Alex_Gagniuc at Dellteam.com wrote:
>> On 11/14/2018 12:00 AM, Bjorn Helgaas wrote:
>>> On Tue, Nov 13, 2018 at 10:39:15PM +0000, Alex_Gagniuc at Dellteam.com wrote:
>> ACPI 6.2 - 6.2.11.3, Table 6-197:
>>
>> PCI Express Advanced Error Reporting control:
>>    * The firmware sets this bit to 1 to grant control over PCI Express
>> Advanced Error Reporting. If firmware allows the OS control of this
>> feature, then in the context of the _OSC method it must ensure that
>> error messages are routed to device interrupts as described in the PCI
>> Express Base Specification[...]
> 
> The PCIe Base Spec is pretty big, so I wish this reference were a
> little more explicit.  I *guess* maybe it's referring to PCIe r4.0,
> figure 6-3 in sec 6.2.6, where PCIe ERR_* Messages can be routed to
> "INTx or MSI Error Interrupts" and/or "platform-specific System Error"
> interrupts.

It's engineering slang for "I don't know what the spec says, but do what 
the spec says." What it means is that FW should set up HW in such a way, 
that PCIe-compliant OS which enables interrupts will get interrupts the 
way it expects. For example, some FW might set the root port to trigger 
an SMI instead of firing up the proper MSI vector. In this case FW must 
ensure that AER interrupts fire up the MSI vector instead of triggering SMI.

> "Device interrupts" seems like it refers to the "INTx or MSI"
> interrupts, not the platform-specific System Errors, so I would read
> that as saying "if firmware grants OS control of AER via _OSC,
> firmware must set the AER Reporting Enables in the AER Root Error
> Command register."  But that seems a little silly because the OS now
> *owns* the AER capability and it can set the AER Root Error Command
> register itself if it wants to.

When OS takes control of AER, it is responsible for setting things up, 
at least as far as standard PCIe registers are concerned. So setting up 
the the root command register would still be the OS's responsibility. 
Proprietary registers, like routing to SMI/SCI/MSI would have to be done 
by FW.


> And I still don't see the connection here with Firmware-First.  I'm
> pretty sure firmware could not be notified via INTx or MSI interrupts
> because those are totally managed by OSPM.

The connection with FFS is that FFS needs to be notified by some other 
method. FW can't commandeer interrupt vectors willie-nillie. FW gets 
notified by platform-specific mechanisms. In my case, it happens to be 
SMM, but it could be a number of other proprietary mechanisms.


>>> The closest I can find is the "Enabled" field in the HEST PCIe
>>> AER structures (ACPI v6.2, sec 18.3.2.4, .5, .6), where it says:
>>>[...] 
>>> AFAICT, Linux completely ignores the Enabled field in these
>>> structures.
>>
>> I don't think ignoring the field is a problem:
>>    * With FFS, OS should ignore it.
>>    * Without FFS, we have control, and we get to make the decisions anyway.
>> In the latter case we decide whether to use AER, independent of the crap
>> in ACPI. I'm not even sure why "Enabled" matters in native AER handling.
> 
> It seems like these HEST structures are "here's how firmware thinks
> you should set up AER on this device".  But I agree, I have no idea
> how to interpret "Enabled".  The rest of the HEST fields cover all the
> useful AER registers, including the Reporting Enables in the AER Root
> Error Command register *and* the Error Reporting Enables in the Device
> Control register.  So I don't know what the "Enabled" field adds to
> all that.  

"This table contains information platform firmware supplies to OSPM for 
configuring AER support on a given PCI Express device."

I don't fully get the point of the HEST structures for PCIe. I'm hoping 
Borislav can shed enlightment on this. I think we can ignore them in PCI 
core, with a note to revisit them if something ever goes horribly wrong. 
I've patched some changes to reduce reliance on HEST [1].

> What a mess.

It's ACPI. What else did you expect?

>>> For firmware-first to work, firmware has to get control.  How does
>>> it get control?  How does OSPM know to either set up that
>>> mechanism or keep its mitts off something firmware set up before
>>> handoff?
>>
>> My understanding is that, if FW keeps control of AER in _OSC, then
>> it will have set things up to get notified instead of the OS. OSPM
>> not touching AER bits is to make sure it doesn't mess up FW's setup.
>> I think there are some proprietary bits in the root port to route
>> interrupts to SMIs instead of the AER vectors.
> 
> It makes good sense that if OSPM doesn't have AER control, firmware
> does all AER handling, including any setup for firmware-first
> notification.  If we can assume that firmware-first notification is
> done in some way the OS doesn't know about and can't mess up, that
> would be awesome.

I think that's a reasonable assumption as long as OS respects AER ownership.

> But I think the VMD model really has nothing to do with the APEI
> firmware-first model.  With VMD, it sounds like OSPM owns the AER
> capability and doesn't know firmware exists *except* that it has to be
> careful not to step on firmware's interrupt.  So maybe we can handle it
> separately.

<sarcasm_alert>
Sounds like VMD is qualified to become part of ACPI spec.
<\sarcasm_alert>

I'll have to look into VMD in more detail, but this sounds like a design 
flaw. It's possible to use the values in HEST to set up AER, but that 
might conflict with the OS's wish on how to set up AER.

Alex


[1] https://lkml.org/lkml/2018/11/16/202