Change in coreboot[master]: [RFC] device: Add method for configuring bus master based on config o...
Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboots code, but the reason for that is not quite clear. Therefore we examined not setting the bus master bit and tried booting without it, which worked fine.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/1
diff --git a/src/device/Kconfig b/src/device/Kconfig index f7f4b90..38bc4eb 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -516,6 +516,10 @@ bool default y
+config PCI_ALLOW_BUS_MASTER + bool "Enable PCI bus master" + default n + endif # PCI
if PCIEXP_PLUGIN_SUPPORT diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 032e15c..2ec78fc 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -1649,3 +1649,9 @@ pci_update_config16(dev, PCI_COMMAND, ~PCI_COMMAND_MASTER, 0x0); } #endif + +void pci_configure_bus_master(const struct device *dev) +{ + if (CONFIG(PCI_ALLOW_BUS_MASTER)) + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); +} diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 4529074..abc1523 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -132,4 +132,6 @@
void pci_early_bridge_init(void);
+void pci_configure_bus_master(const struct device *device); + #endif /* PCI_H */
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#2).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboots code, but the reason for that is not quite clear. Therefore we examined not setting the bus master bit on all PCI(e) devices and tried booting without it, which worked fine for internal PCI devices but not for PCIe devices.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option for internal PCI devices.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/2
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#3).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboots code, but the reason for that is not quite clear. Therefore we examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe devices.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option for internal PCI devices.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42459/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/1//COMMIT_MSG@9 PS1, Line 9: coreboots coreboot's
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig@519 PS1, Line 519: config PCI_ALLOW_BUS_MASTER Why this option? Why default to n?
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/pci_device.c@165... PS1, Line 1653: void pci_configure_bus_master(const struct device *dev) I would prefer having inline functions in pci_ops.h to handle the PCI command register.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig@519 PS1, Line 519: config PCI_ALLOW_BUS_MASTER
Why this option? Why default to n?
Since we are able to boot without setting this bit, we would like to disable bus mastering by default. If anyone depends on this, you can re-enable it with this option, which would correspond to the current state.
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#4).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboots code, but the reason for that is not quite clear. Therefore we examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe devices.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option for internal PCI devices.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig@519 PS1, Line 519: config PCI_ALLOW_BUS_MASTER
Since we are able to boot without setting this bit, we would like to disable bus mastering by defaul […]
Why, though? Security? To prevent PCI devices from initiating transfers?
In any case, please test this on a platform where coreboot code actually does something. With FSP, coreboot does not do much, and FSP is most likely setting bus master bits regardless of what this Kconfig says.
https://review.coreboot.org/c/coreboot/+/42459/4/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/42459/4/src/device/pci_device.c@165... PS4, Line 1653: void pci_configure_bus_master(struct device *dev) void pci_dev_enable_bus_master(const struct device *dev)
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h@13... PS4, Line 131: void pci_configure_bus_master(struct device *dev); void pci_dev_enable_bus_master(const struct device *dev);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig@519 PS1, Line 519: config PCI_ALLOW_BUS_MASTER
Why, though? Security? To prevent PCI devices from initiating transfers? […]
FSP might set it temporarily (didn't check the code), but in our tests it left it disabled when returning to coreboot.
The basic rule is that enabling bus mastering is wrong unless you want a device to do DMA (or take over your system). There is a slight chance that some integrated devices are buggy enough to require it enabled during their init sequence, but that is no excuse to leave it enabled. In some cases, coreboot enables it _after_ the init sequence which makes no sense at all.
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h@13... PS4, Line 131: void pci_configure_bus_master(struct device *dev);
void pci_dev_enable_bus_master(const struct device *dev);
Ack to the `const` but we didn't name it `enable` on purpose, because the point is to not enable it unconditionally.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig@519 PS1, Line 519: config PCI_ALLOW_BUS_MASTER
FSP might set it temporarily (didn't check the code), but in our tests it […]
Right, I see the point now. I'd still make this default to y for now and flip it on a per-platform basis once tested, just in case.
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h@13... PS4, Line 131: void pci_configure_bus_master(struct device *dev);
Ack to the `const` but we didn't name it `enable` on purpose, because the point […]
Right, maybe do s/enable/request/ then?
Another detail is the "_dev" part, for consistency with the disable operation
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#5).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboots code, but the reason for that is not quite clear. Therefore we examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe devices.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option for internal PCI devices.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/5
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h@13... PS4, Line 131: void pci_configure_bus_master(struct device *dev);
Right, maybe do s/enable/request/ then? […]
I would rather use "configure". IMO this relates more to a config option. "request" sounds like there is happening some black magic.
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#6).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboots code, but the reason for that is not quite clear. Therefore we examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe devices.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option for internal PCI devices.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG@16 PS6, Line 16: for internal PCI devices. Is a global option granular enough to set the desired policy? There are lots of internal devices that may stop decoding transactions properly. Were we wanting to enable such a thing for every access? And, presumably, we would then punt new behavior to the payloads that may have been taking advantage of all that behavior?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG@16 PS6, Line 16: for internal PCI devices.
Is a global option granular enough to set the desired policy? There are lots of internal devices tha […]
I was confused on bus master vs memory decode. However, the side effects still stand downstream for payloads.
I would like to hear people's thoughts on a global option for all devices being appropriate.
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#7).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. But depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option and enable it by default to keep the current behaviour.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/device/pci_device.c M src/include/device/pci.h 3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/7
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42459/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/1//COMMIT_MSG@9 PS1, Line 9: coreboots
coreboot's
Done
https://review.coreboot.org/c/coreboot/+/42459/4/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/42459/4/src/device/pci_device.c@165... PS4, Line 1653: void pci_configure_bus_master(struct device *dev)
void pci_dev_enable_bus_master(const struct device *dev)
Done
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/4/src/include/device/pci.h@13... PS4, Line 131: void pci_configure_bus_master(struct device *dev);
I would rather use "configure". IMO this relates more to a config option. […]
Done
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#8).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. But depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures the bus master bit based on an additional config option and enable it by default to keep the current behaviour.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/8
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 8:
(2 comments)
So, long story short, we would like to clean the BM situation in coreboot up before we start fiddling with IOMMUs in coreboot or the payloads. There are many devices in coreboot for which we enable it and we couldn't find a reason.
Beside random devices throughout the tree (which are covered by the next commit) there are two cases in the device/ code, 1. PCI bridges and 2. "system" class devices. At least for the PCI bridges we know it's needed because most payloads (in their current state) wouldn't be able to work with devices below bridges. The "system" class devices are very odd, the only ones we encountered are SD card controllers and Intel's newish Gaussian Mixture Model device... both don't seem to need special attention. Devices that just need it, like Intel's host and LPC bridges have the BM bit hardwired to 1.
For all but the bridge devices we wanted to add the Kconfig switch here in case somebody feels uncomfortable about just bluntly disabling BM everywhere. But if nobody sees a problem in that, we could also just skip the Kconfig ;)
Opinions most welcome!
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG@16 PS6, Line 16: for internal PCI devices.
I was confused on bus master vs memory decode. […]
Most promiment payloads would indeed be affected by a global switch. They don't enable bus mastering for PCI bridges. So the plan is to have a separate option for those.
The original thought was to only have a switch for devices where we couldn't find a reason (e.g. in EDS/BIOS Spec) why coreboot enables bus mastering. Turned out that we couldn't find a reason for any device but Intel's IGD (which, rather oddly, even scans out a linear framebuffer with disabled bus mastering ^^).
https://review.coreboot.org/c/coreboot/+/42459/8/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/8/src/device/Kconfig@520 PS8, Line 520: Enable This should say `allow` too. And some help text would be nice that this enables bus mastering for devices for unknown reasons.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 8:
Patch Set 8:
(2 comments)
So, long story short, we would like to clean the BM situation in coreboot up before we start fiddling with IOMMUs in coreboot or the payloads. There are many devices in coreboot for which we enable it and we couldn't find a reason.
Beside random devices throughout the tree (which are covered by the next commit) there are two cases in the device/ code,
- PCI bridges and 2. "system" class devices. At least for the
PCI bridges we know it's needed because most payloads (in their current state) wouldn't be able to work with devices below bridges. The "system" class devices are very odd, the only ones we encountered are SD card controllers and Intel's newish Gaussian Mixture Model device... both don't seem to need special attention. Devices that just need it, like Intel's host and LPC bridges have the BM bit hardwired to 1.
For all but the bridge devices we wanted to add the Kconfig switch here in case somebody feels uncomfortable about just bluntly disabling BM everywhere. But if nobody sees a problem in that, we could also just skip the Kconfig ;)
Opinions most welcome!
I'm ok either way. We just need to coordinate with the change so we can make the necessary changes in our payloads.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/8/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/8/src/device/Kconfig@520 PS8, Line 520: Enable
This should say `allow` too. And some help text would be nice that this […]
maybe: "Allow PCI bus master bit to be enabled by coreboot"?
Of course, to enforce that better, we'd could use some way to guard against it happening in the future. Someone could still go pci_or_config16(blah blah blah, PCI_COMMAND_MASTER).
What about a linter for usage of PCI_COMMAND_MASTER as well?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
Patch Set 8: Code-Review+1
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#9).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/9
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#10).
Change subject: [RFC] device: Add method for configuring bus master based on config option ......................................................................
[RFC] device: Add method for configuring bus master based on config option
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/10
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#11).
Change subject: device: Add option for configuring bus mastering ......................................................................
device: Add option for configuring bus mastering
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/11
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#12).
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/12
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/Kconfig@519 PS1, Line 519: config PCI_ALLOW_BUS_MASTER
Right, I see the point now. […]
Done
https://review.coreboot.org/c/coreboot/+/42459/8/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/8/src/device/Kconfig@520 PS8, Line 520: Enable
maybe: "Allow PCI bus master bit to be enabled by coreboot"? […]
I've taken over your suggestion and added a help text. I hope this is fine.
Using a linter sounds good to me.
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/42459/1/src/device/pci_device.c@165... PS1, Line 1653: void pci_configure_bus_master(const struct device *dev)
I would prefer having inline functions in pci_ops.h to handle the PCI command register.
We are planning to expand this function in the future, since we think about adding other options too. So I would keep this for now. We can still make this inline later.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 12: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig@526 PS12, Line 526: Because of nit: For security reasons ...
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig@527 PS12, Line 527: Since not all payloads do that let those payloads select this?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 12: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig@527 PS12, Line 527: Since not all payloads do that
let those payloads select this?
We have to keep downstream payloads in mind. All upstream payloads should be updated to enable BM in the payload anyway.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master pci_dev_configure_bus_master -> pci_dev_enable_bus_master because u don't have any other option here rather enable?
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#13).
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/13
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig@526 PS12, Line 526: Because of
nit: For security reasons ...
Done
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
pci_dev_configure_bus_master -> pci_dev_enable_bus_master […]
Oh yes, you are right. Thanks :)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 13: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/13/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/13/src/device/Kconfig@526 PS13, Line 526: nit: is this supposed to be 2 space or 1 tab?
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#14).
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/14
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/13/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/13/src/device/Kconfig@526 PS13, Line 526:
nit: is this supposed to be 2 space or 1 tab?
Replaced it with two spaces
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
Oh yes, you are right. […]
The original name explicitly didn't say `enable` because that is wrong depending on the Kconfig option. `configure` might be inaccurate but at least it's not wrong... I'm open for other suggestions.
The most accurate name I can come up with is
pci_dev_conditionally_enable_bus_master()
but that seems a bit long.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14: Code-Review+2
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
The original name explicitly didn't say `enable` because that is wrong […]
Done
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/6//COMMIT_MSG@16 PS6, Line 16: for internal PCI devices.
Most promiment payloads would indeed be affected by a global switch. They […]
Done
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/12/src/device/Kconfig@527 PS12, Line 527: Since not all payloads do that
We have to keep downstream payloads in mind. All upstream payloads should be […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
Done
I came up with `pci_dev_request_bus_master` a while ago, but I don't remember if I ever told anyone about it
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
I came up with `pci_dev_request_bus_master` a while ago, but I don't remember if I ever told anyone […]
Well, i don't mind. Any other opinions? I will leave this unresolved for a bit again.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
I came up with `pci_dev_request_bus_master`
Sounds good.
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Angel Pons, Subrata Banik, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#15).
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/15
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h File src/include/device/pci.h:
https://review.coreboot.org/c/coreboot/+/42459/12/src/include/device/pci.h@1... PS12, Line 134: pci_dev_configure_bus_master
I came up with `pci_dev_request_bus_master` […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 15: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/15/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/15/src/device/Kconfig@527 PS15, Line 527: Since not all payloads do that : currently, this option gives some sort of "backwards compatibility" : and is enabled by default to keep the behaviour for now. Please give an example of a payload doing it, and one not doing it.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 15: Code-Review+2
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#16).
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
An example for a payload, that configures bus mastering, is grub, while filo doesn't configure it.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/16
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/15/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/15/src/device/Kconfig@527 PS15, Line 527: Since not all payloads do that : currently, this option gives some sort of "backwards compatibility" : and is enabled by default to keep the behaviour for now.
Please give an example of a payload doing it, and one not doing it.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 16: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/16//COMMIT_MSG@27 PS16, Line 27: filo doesn't configure it. Thank you for adding this sentence. I’d put it into the Kconfig help text though.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42459/16/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/16/src/device/Kconfig@529 PS16, Line 529: and is enabled by default to keep the behaviour for now. It only has to be done in the payload for devices that are used by the payload. I'd phrase it as follows:
For security reasons, bus mastering should be enabled as late as possible. In coreboot, it's usually not necessary and payloads should only enable it for devices they use. Since not all payloads enable bus mastering properly yet, this option gives some sort of "backwards compatibility" and is enabled by default to keep the traditional behaviour for now. This is currently necessary, for instance, for libpayload based payloads as the drivers don't enable bus mastering for PCI bridges.
Hello build bot (Jenkins), Patrick Georgi, Jonathan Zhang, Paul Menzel, Duncan Laurie, John Zhao, Subrata Banik, Angel Pons, Arthur Heymans, Michael Niewöhner, Patrick Rudolph, Nico Huber, Martin Roth, Marshall Dawson, Tim Wawrzynczak, Ron Minnich,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42459
to look at the new patch set (#17).
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/42459/17
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 17: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/42459/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42459/16//COMMIT_MSG@27 PS16, Line 27: filo doesn't configure it.
Thank you for adding this sentence. I’d put it into the Kconfig help text though.
Done
https://review.coreboot.org/c/coreboot/+/42459/16/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/42459/16/src/device/Kconfig@529 PS16, Line 529: and is enabled by default to keep the behaviour for now.
It only has to be done in the payload for devices that are used by the […]
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
Patch Set 17: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42459 )
Change subject: device: Add method to configure bus mastering based on Kconfig ......................................................................
device: Add method to configure bus mastering based on Kconfig
The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD.
For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it.
To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now.
Tested with Siemens/Chili, a Coffee Lake based platform.
Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer felix.singer@secunet.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42459 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Subrata Banik subrata.banik@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/Kconfig M src/include/device/pci.h 2 files changed, 25 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Subrata Banik: Looks good to me, approved
diff --git a/src/device/Kconfig b/src/device/Kconfig index d0d72f9..439118f 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -519,6 +519,19 @@ bool default y
+config PCI_ALLOW_BUS_MASTER + bool "Allow PCI bus master bit to be enabled by coreboot" + default y + help + For security reasons, bus mastering should be enabled as late as + possible. In coreboot, it's usually not necessary and payloads + should only enable it for devices they use. Since not all payloads + enable bus mastering properly yet, this option gives some sort of + "backwards compatibility" and is enabled by default to keep the + traditional behaviour for now. This is currently necessary, for + instance, for libpayload based payloads as the drivers don't enable + bus mastering for PCI bridges. + endif # PCI
if PCIEXP_PLUGIN_SUPPORT diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 4529074..ec3d45e 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -128,6 +128,18 @@ }
void pci_dev_disable_bus_master(const struct device *dev); + +static __always_inline +#if ENV_PCI_SIMPLE_DEVICE +void pci_dev_request_bus_master(pci_devfn_t dev) +#else +void pci_dev_request_bus_master(const struct device *dev) +#endif /* ENV_PCI_SIMPLE_DEVICE */ +{ + if (CONFIG(PCI_ALLOW_BUS_MASTER)) + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); +} + #endif /* CONFIG_PCI */
void pci_early_bridge_init(void);
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Singer (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)