Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(5 comments)
There seems to be no need for the Kconfig. Subrata please don't push reviews with words like `required` or `need`. Or at least check the code before you talk about require- ments to avoid lies.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX
can you please help to write the same for all PCIE card vendors NVDIA, AMD, ATI whoever and say please stop selling cards with OpRom, that would help.
If they wanted to do that. I would help.
Right now my problem statement is that, i need to bring display over PCIE GFX for my board and i need to run OpRom, what is my priority.
That's just fine. But you are mixing too many things. The name HAVE_EXT_GFX implies that it is generally useful for external graphics cards. But it is actually only useful for your specific case. If you would just find a better name and place for it, you would be much closer to get it through a review.
I have protection taken to lock down all my protected range registers what any OpRom can manipulate, i have all Chipset registers lockdown as part of POSTBOOT_SAI thats the whole purpose of security that no 3rd party code can break into chipset space.
That's the problem with Intel folks. There's some guide that tells what to lock before executing oproms. It seems it makes people believe that there is nothing else to consider and that they don't have to think. But that's just wrong. If you pass execution to an OpRom, it can do what it wants. It doesn't have to return execution to coreboot. For instance, it can bypass any signature veri- fication of an OS kernel and just load whatever OS it likes. Those security guides are still useful to understand the basics, but they can't protect deve- lopers from other mistakes.
What you mean by not running OpRom code. still today there are many PC running thousands of OpRom code, all are BAD ?
Obviously, yes. You might know that most of these PCs running OpRoms don't have open-source firmware. Of course, that's all BAD. Maybe nobody told you, but be- fore FSP, we could boot Intel PCs without any proprietary code. Everything Intel does seems to be a step backwards.
and they should stop their business. Please try to be reasonable with other patches and understand what is the need of the hour.
The need of the hour is to calm and slow down. There is exactly one thing to do Kconfig wise, select VGA_ROM_RUN. That you call the other options "required" only shows that you didn't take the time to understand them. So why should we hurry to review your half-backed patches?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 213: select ALWAYS_LOAD_OPROM
i don't get you. […]
Not needed. Loading obviously is always implied by running it.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 214: select ALWAYS_RUN_OPROM Not generally needed. It ignores the vboot decision to skip gfx init. It was added to help broken OS drivers that don't work if the firmware didn't run it.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 215: select ON_DEVICE_ROM_LOAD defaults to y anyway
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
Its because with depthcharge pre os screen it needs GFX framebuffer in VESA mode to dump bmlblk imag […]
That's just your use case. How about a `default y if CHROMEOS` instead?