Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
[WIP] Documentation: PCI resource allocations
Change-Id: I5479c4a60c3d8c7545759c0f48aa9f5d628d65f3 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- A Documentation/device/pci_resource.md 1 file changed, 26 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/36493/1
diff --git a/Documentation/device/pci_resource.md b/Documentation/device/pci_resource.md new file mode 100644 index 0000000..0fa2a51 --- /dev/null +++ b/Documentation/device/pci_resource.md @@ -0,0 +1,26 @@ +PCI resource allocation rules +============================= + +Definition: To opt-out from PCI resource allocation for a specific PCI device is to enter payload with the *PCI COMMAND* register having a defined state where *IO* and *MEMORY* spaces and *BUS_MASTER* bits are cleared, while all the *PCI standard BARs* are in an undefined state. + +1. #### Rule A +PCI devices that do not bind to PCI drivers can always implicitly opt-out, because PCI subsystem code itself will not address register banks or buffers behind standard BARs. + +1. Most external/add-on PCIe devices located behind PCIe bridges will hit `Rule A`. This includes the troublesome PCIe graphics with large *PREFMEM* resources. + +1. With the recursive nature of how *.read_resources* already works, upstream PCIe bridges will implicitly opt-out in the case all the secondary side devices have opted-out. + +1. Within PCI drivers, *.init* is the only callback where resource assignment may be required. + +1. #### Rule B +For cases where *.init* is not implemented at all, we can implicitly opt-out. + +1. #### Rule C +If .init does not call any *find_resource()*, we can explicitly opt-out by using alternative *.read_resources()* implementation with a conditional *MINIMAL_PCI_RESOURCES* check in it. + +1. #### Rule D +If *.init* does call *find_resource()*, it's likely for the reason that it does configurations that are dependent of either some *KConfig* option or *devicetree.cb*. Or it could be a static chipset configuration that is not easy to delay to payload. These devices can rarely opt-out, but it could be done explicitly using custom *.read_resources* like *Rule C*. + +1. Platforms with blobbed chipset initialisation would currently likely hit *Rule C*, but it might be possible to have them hit *Rule B* instead. + +1. If choice of payload is capable of completing the resource allocation, possibly extending to 64-bit MMIO space, it may be desirable to leave as many resources as possible unallocated and potentially even unallocate the ones that had been assigned.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... File Documentation/device/pci_resource.md:
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 13: 1. Within PCI drivers, *.init* is the only callback where resource assignment may be required. And `.final`?
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 25: Maybe
1. Platforms with blobbed chipset initialisation that hook into boot states and expect resources assigned are borked.
I guess the best thing we can do with chipset devices on FSP platforms is to allocate resources and unallocate like described below. Alternatively, we could go trough intensive vendor-code reviews or testing to figure out what happens when we violate FSP.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Patch Set 1:
(2 comments)
This seems like it's capturing potential future behavior and not documenting current, correct?
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... File Documentation/device/pci_resource.md:
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: *.init* This is also implying not having a __pci_driver that matches on didvid. Correct?
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: implicitly opt-out Why the change of behavior? This is implying that command register will not be enabled, correct?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Patch Set 1:
(4 comments)
Patch Set 1:
(2 comments)
This seems like it's capturing potential future behavior and not documenting current, correct?
Yep. This is more of future behaviour, should we implement MINIMAL_PCI_RESOURCE.
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... File Documentation/device/pci_resource.md:
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 13: 1. Within PCI drivers, *.init* is the only callback where resource assignment may be required.
And `. […]
Right.
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: *.init*
This is also implying not having a __pci_driver that matches on didvid. […]
I think we would continue to allow __pci_driver with empty .init and .final. Maybe I did not completely understand the question.
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: implicitly opt-out
Why the change of behavior? This is implying that command register will not be enabled, correct?
That's the approach Ron wants to drive, to opt-out as much as possible. I would not necessarily agree with him here. Not having BARs programmed implies indeed command register must keep decodes disabled.
To opt-out would always be conditional on CONFIG(MINIMAL_PCI_RESOURCES). I am trying to point out here that little devices would need to be explicitly opted-out, if we make decisions on .init/.final existence.
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 25:
Maybe […]
Subrata placed some comment about SoC PCI tree being static. It's almost as if he is saying that Intel wants this MINIMAL_PCI_RESOURCE thing because they want to assign the required/temporary resources inside the blob instead.
If someone is saying payload/OS can re-enumerate and reallocate everything, I think we should explicitly make it so that everything is unassigned on entry to payload.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36493
to look at the new patch set (#2).
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
[WIP] Documentation: PCI resource allocations
Write down some rules for partial PCI enumeration and/or resources allocations.
Change-Id: I5479c4a60c3d8c7545759c0f48aa9f5d628d65f3 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- A Documentation/device/pci_resource.md 1 file changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/36493/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... File Documentation/device/pci_resource.md:
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: *.init*
I think we would continue to allow __pci_driver with empty .init and .final. […]
I was just trying to understand the change in policy. The condition below about not using find_resource() needs we need state per device to determine if it was used. It also means we have to change the code to not assume static bars when static bars were provided through read_resources. I think the policy becomes becomes complicated with the state handling proposed in this doc.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... File Documentation/device/pci_resource.md:
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: *.init*
I was just trying to understand the change in policy. […]
Not sure if we understood Rule C below the same way. I tried to indicate that .init or .final without .find_resource() calls implies it does not need the resources allocated.
Yes, the cases where statically assigned resources are accessed without find_resource() call, but directly via some #define address, would have to be banned. Other than that, I don's see static bars as an added complication.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... File Documentation/device/pci_resource.md:
https://review.coreboot.org/c/coreboot/+/36493/1/Documentation/device/pci_re... PS1, Line 16: *.init*
Not sure if we understood Rule C below the same way. I tried to indicate that .init or . […]
Fair. I'm still concerned about either adding lots of code or trying to be slick in maintaining state on what one should do w/ the device resources if they were accessed or not.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36493 )
Change subject: [WIP] Documentation: PCI resource allocations ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)