[coreboot] Re: KGPE-D16 maintainership
On 9/23/19 4:42 AM, Arthur Heymans wrote:
> Hi
>
> Thanks for wanting to maintain this platform!
>
> There are a issues with the amdfam10 codebase that could be improved
> upon. I'll try to list a few of them, to give an idea of what
> maintaining this code in 2019 could mean.
>
> So first and foremost:
> 1) The amdfam10 codebase (terminology-wise this always means non-agesa
> in this context) suffers from a romcc romstage past. This code is
> derived from code that used to be compiled with romcc instead of running
> in CAR. The result of that is a big amount of romstage boilerplate code
> and lots of #include *.c in the mainboard code, that is often not
> mainboard specific. To give an example romstage spinlocks and memory
> tests are implemented in the kgpe-d16 romstage.c file while not being
> mainboard specific at all. These practices result in an unusually large
> amount of unmaintained code in mainboard dirs with a fragile inclusion
> order of headers (and *.c files!).
>
> Other issues pertain to coreboot wanting to move forward by mandating a
> few features (relocatable ramstage, postcar
> stage/no_car_global_migration, c environment bootblock):
> 2) relocatable ramstage: This is just a Kconfig switch to build the
> ramstage as a relocatable stage instead of statically linking it to
> execute at CONFIG_RAM_BASE. Typically you want to set up some caching of
> where cbmem is going to be to speed up ramstage, but on amd hardware
> it's bit more complicated. Part of ramstage is to initialize AP's and AP's
> will eventually jump to ramstage code. On AMD hardware however there is
> a TOP_MEM MTRR that creates a boundary between ram and mmio below 4G. If
> this is configured to be below cbmem AP's won't execute code. I see a
> few proper solutions:
> - AP's are also active during the romstage -> try to sync MTRR's at the
> end of romstage.
> - Use the parallel mp init code and modify the relocatable SIPI vector
> stub to have the MTRR's as arguments instead of a pointer to the BSP
> MTRR settings, in order to copy them.
>
> https://review.coreboot.org/c/coreboot/+/30063 is an attempt to make it
> work but by setting TOP_MEM to something sufficiently large...
>
> 3) Postcar-stage: This development goes hand in hand with relocatable
> ramstage, as it easily allows to program MTRR's to set up caching cbmem.
> If you tearing down CAR during romstage you need to take special care
> for where you're globals are (before and after CAR). Tearing down CAR in
> a separate stage, hence having a romstage with a clean program boundary,
> as a solution allows to get rid of a lot of code to deal with globals.
>
> https://review.coreboot.org/c/coreboot/+/30064/ is an attempt to
> implement it on amdfam10
>
> 4) C_ENVIRONMENT_BOOTBLOCK: romcc is an unmaintained 20k+ lines of C
> code in one file program, that imposes some restrictions and workarounds
> in C code. On older platforms it is used to compile a bootblock that
> often does nothing more than loading {normal,fallback}/romstage. The CAR
> setup happens at the start of romstage. The alternative is to move the CAR
> init to the bootblock and compile the bootblock with gcc. One big
> functional advantage is the ability to have a separate verstage running
> in CAR before romstage. This broadens the scope of what can be placed in
> RW_A/B slot in a VBOOT setup.
>
> 5) a few minor things: coreboot often has multiple methods of achieving
> more or less the same things, with newer methods being better in some
> aspects. It would be great if:
> - for saving the raminit timings the drivers/mrc_cache code was used
> instead of the custom implementation in
> northbridge/amd/amdmct/mct_ddr3/s3utils.c
> - for AP init CONFIG_PARALLEL_MP were used instead of the old
> cpu/x86/lapic/lapic_cpu_init.c code.
> - A stage cache were set up in TSEG to ensure the OS can't trash the
> ramstage in cbmem on S3 resume.
>
>
> I hope this list provides some useful pointers to where to code can be
> improved. Feel free to add me on reviews.
Thanks Arthur, awesome inputs.
>
>
> Piotr Król writes:
>
>> Hi all,
>> we see a lot of attention around KGPE-D16 maintainership problems.
>> After discussion with Thierry Laurion (Insurgo) at OSFC2019 3mdeb
>> decided to help in maintaining that platform by organizing crowd
>> founding campaign or getting founds in other ways (direct sponsors).
>>
>> Since we are based in Poland there is chance that even with small
>> contribution from community we would be able to cover the costs.
>>
>> Ideal plan would be to have structure similar to what we maintain for
>> PC Engines:
>> https://pcengines.github.io/
>> Where we providing signed and reproducible binaries every month and
>> keep as close to mainline as possible. Of course if development will
>> be active, then there always would be delta of patches held in review.
On my side, I'm committing to spin the need of support into Libreboot
communities, open source privacy forums and in the real world in
sec
[coreboot] Aspirant for contributing to coreboot project during google summer of code 2020
Hi, I am Ajay, pursuing my Master's in Computer Science at Stonybrook university, NewYork. I am interested in OS, Firmware, Compiler development and also being part of open source community. I would like to understand coreboot project and contribute to the project during GSOC 2020. As a part of preparation, I would like to know how can I go about understanding and contribute to coreboot project? Thank you, Ajay Paddayuru Shreepathi ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On Fri, Sep 27, 2019 at 3:58 PM Aaron Durbin via coreboot wrote: >> > >> > 5. PCI coalesce can alter PCI dev.fn assignments? >> >> That's a serious problem. I noticed that CFL FSP can reassign them >> without being asked to, unpredictably (e.g. if a device fails to show >> up in whatever timeframe FSP assumes). Solution might be simple? A >> chip->enable_dev() that updates b/d/f based on DID? > > > Ya. I think that's likely what is required. https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/soc/intel/skylake/chip_fsp20.c#88 This function should really be in common code (and I guess called from SOC specific to supply the list) as it only seems to exist in skylake/apollolake dirs... -duncan ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On Fri, Sep 27, 2019 at 8:11 PM Nico Huber wrote: > > On 26.09.19 18:45, Aaron Durbin via coreboot wrote: > > Here's some of the requirements/issues we should resolve that come to mind: > > > > 1. Easy way to directly retrieve a device's chip config object w/o > > traversing the device hierarchy. Which leads to... > > 2. Symbol alias for accessing struct device directly (no bdf lookup) > > What we already have: > > Static pointers to `struct device` for devices with a globally valid > address (PCI devices on bus 0 and PNP devices), e.g.: > > DEVTREE_CONST struct device *DEVTREE_CONST __pci_0_02_0 = &_dev6; > DEVTREE_CONST struct device *DEVTREE_CONST __pnp_002e_00 = &_dev56; > > What I suggested somewhere on Gerrit: > > At the chip driver level, add a header file that maps these to more > distinct names, e.g. > > #define igd_dev __pci_0_02_0; > > But that was last week. Since then I've written yet another override > tree and realized something. We write a lot lines like > > device pci 02.0 on end # Integrated Graphics Device > > What's wrong with that? (if you know me it's obvious) there is a > comment! IIRC, Kyösti suggested it already, we could let sconfig > read a chip specific mapping for device names. That would also > allow us to write the above as One way or another, I would like most of gone and same information derived from devicetree.cb files. For the record, I am rebasing my local trees, this work removes most if not all #if __SIMPLE_DEVICE__ under soc/intel. Except for [1] and [2] I do not have any work pending related to providing usable alias names, or much interest to work on devicetree.cb parser. [1] https://review.coreboot.org/c/coreboot/+/31936 [2] https://review.coreboot.org/c/coreboot/+/31934 Kyösti ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On Fri, Sep 27, 2019 at 10:42 AM Nico Huber wrote:
> On 27.09.19 15:42, Kyösti Mälkki wrote:
> > On Thu, Sep 26, 2019 at 7:45 PM Aaron Durbin wrote:
> >>
> >> On Thu, Sep 26, 2019 at 10:06 AM Kyösti Mälkki
> wrote:
> >>> Should be easy enough to implement
> >>> platform hook telling to not remove PCI device node from topology
> >>> links (based on BDF), even when it does not respond to ID queries.
>
> For those who missed it: we already have a `hidden` keyword in sconfig
> which complements `on`/`off`. Currently it's only used to write static
> ACPI _STA methods. Maybe we could just use that? If a device is known
> to be hidden on purpose, don't detach it from the topology but report
> attempts to access PCI config?
>
> >>
> >>
> >> Yes, we can certainly do that. However, I also think this issue and
> yours and Nico's devicetree work are somewhat related as well as
> https://review.coreboot.org/c/coreboot/+/35621.
> >
> > I'll try to rebase all my work during the weekend>
> >> Here's some of the requirements/issues we should resolve that come to
> mind:
> >>
> >> 1. Easy way to directly retrieve a device's chip config object w/o
> traversing the device hierarchy. Which leads to...
> >> 2. Symbol alias for accessing struct device directly (no bdf lookup)
>
> More on this in a separate email.
>
> >> 3. Symbol alias for pci b/d/f lookup (equivalent of simple device but
> cleaner) so we can perform pci operations.
>
> IIRC, I asked this elsewhere already. Do we want to keep simple device?
> If we reduce `struct device` to b/d/f and a pointer to the chip info
> in early stages, couldn't we just use `struct device` for PCI config
> access everywhere?
>
We could give it a go. One issue we have is the use of dev->bus->dev in
encoding buses.
static __always_inline pci_devfn_t pcidev_bdf(const struct device *dev)
{
>---return (dev->path.pci.devfn << 12) | (dev->bus->secondary << 20);
}
If we wanted to get to more code consistency we need a better way to track
bus number (even though we don't use it in practice currently). It could be
as simple as us just conditionally supporting buses > 0 or not depending
on the environment. I don't think this is a huge blocker, but I wanted to
bring it up.
To be explicit you are thinking the following?
struct device {
struct device_path path;
void *config;
};
> >> 4. possibly hidden pci devices
> >>
> >> Anything else I'm missing? I think a lot of the issues we're running
> into could be fixed w/ the above. Let me know what you think.
> >>
> >
> > 5. PCI coalesce can alter PCI dev.fn assignments?
>
> That's a serious problem. I noticed that CFL FSP can reassign them
> without being asked to, unpredictably (e.g. if a device fails to show
> up in whatever timeframe FSP assumes). Solution might be simple? A
> chip->enable_dev() that updates b/d/f based on DID?
>
Ya. I think that's likely what is required.
>
> > 6. Devicetree in ENV SMM is problematic when chip config is mutable in
> > ENV_RAMSTAGE.
>
> Do we need chip config in SMM?
>
I'm not sure. I don't recall us needing it at the moment.
>
> Nico
>
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On Fri, Sep 27, 2019 at 11:11 AM Nico Huber wrote: > On 26.09.19 18:45, Aaron Durbin via coreboot wrote: > > Here's some of the requirements/issues we should resolve that come to > mind: > > > > 1. Easy way to directly retrieve a device's chip config object w/o > > traversing the device hierarchy. Which leads to... > > 2. Symbol alias for accessing struct device directly (no bdf lookup) > > What we already have: > > Static pointers to `struct device` for devices with a globally valid > address (PCI devices on bus 0 and PNP devices), e.g.: > > DEVTREE_CONST struct device *DEVTREE_CONST __pci_0_02_0 = &_dev6; > DEVTREE_CONST struct device *DEVTREE_CONST __pnp_002e_00 = &_dev56; > > What I suggested somewhere on Gerrit: > > At the chip driver level, add a header file that maps these to more > distinct names, e.g. > > #define igd_dev __pci_0_02_0; > > But that was last week. Since then I've written yet another override > tree and realized something. We write a lot lines like > Have you pushed this patch? > > device pci 02.0 on end # Integrated Graphics Device > > What's wrong with that? (if you know me it's obvious) there is a > comment! IIRC, Kyösti suggested it already, we could let sconfig > read a chip specific mapping for device names. That would also > allow us to write the above as > > device igd on end > > Mapping could look like this: > > device pci 00.0 alias mch > device pci 02.0 alias igd > We can do this in 2 stages, I think, right? Or were you wanting to push a mapping along with the alias wording? I could run with what you had prior in patch from the get go and follow up with further clean up. > > and we could directly generate pointers with these names: > > DEVTREE_CONST struct device *DEVTREE_CONST igd_dev = &_dev6; > > Limitations: Actually, chip drivers can't provide global names. What > should sconfig generate if a second instance of a chip pops up? So > the generated pointers must somehow be limited to chips that can only > occur once, e.g. PCH, single socket CPU. > > What do you think? > > Nico > ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On Fri, Sep 27, 2019 at 7:42 PM Nico Huber wrote: > > IIRC, I asked this elsewhere already. Do we want to keep simple device? > If we reduce `struct device` to b/d/f and a pointer to the chip info > in early stages, couldn't we just use `struct device` for PCI config > access everywhere? I got side-tracked from the PCI development, also I noticed the one big [RFC] commit on my local tree I had never pushed. 1. I want to remove all preprocessor #ifdef __SIMPLE_DEVICE__ mess. We currently have that because of name collisions, some of that looked prettier prior to removal of device_t but we definitely do not want that back. 2. I have experimented with romstage, switching from passing immediate BDF value PCI_DEV(b,d,f) to struct pointer; I have not seen performance or code size penalty. Having inspected some of the generated objects, code patterns that transfer multiple register values from devtree.cb to a single PCI device are actually tighter assembly code (on x86). My vision here is we need to optimise the developer's use of time. 3. I want to keep pci_devfn_t available for PCI subsystem primitives, like and . Also I would like to keep those files simple and see at most an anonymous struct device. I am not convinced if we are ready to really require C_ENVIRONMENT_BOOTBLOCK with next release? The forms with pci_devfn_t do work with romcc and we need that little PCI configuration in bootblocks. 4. I do not want to make devicetree compulsory for (gcc-built) bootblocks. But if we can drop the topology links there, the size impact would not be that bad. If we follow this approach, and get all romstages converted to use struct device, we could reconsider #3. 5. I don't plan to fix/clean amdfam10 from the preprocessor mess discussed here (or elsewhere). Kyösti Mälkki ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On 26.09.19 18:45, Aaron Durbin via coreboot wrote: > Here's some of the requirements/issues we should resolve that come to mind: > > 1. Easy way to directly retrieve a device's chip config object w/o > traversing the device hierarchy. Which leads to... > 2. Symbol alias for accessing struct device directly (no bdf lookup) What we already have: Static pointers to `struct device` for devices with a globally valid address (PCI devices on bus 0 and PNP devices), e.g.: DEVTREE_CONST struct device *DEVTREE_CONST __pci_0_02_0 = &_dev6; DEVTREE_CONST struct device *DEVTREE_CONST __pnp_002e_00 = &_dev56; What I suggested somewhere on Gerrit: At the chip driver level, add a header file that maps these to more distinct names, e.g. #define igd_dev __pci_0_02_0; But that was last week. Since then I've written yet another override tree and realized something. We write a lot lines like device pci 02.0 on end # Integrated Graphics Device What's wrong with that? (if you know me it's obvious) there is a comment! IIRC, Kyösti suggested it already, we could let sconfig read a chip specific mapping for device names. That would also allow us to write the above as device igd on end Mapping could look like this: device pci 00.0 alias mch device pci 02.0 alias igd and we could directly generate pointers with these names: DEVTREE_CONST struct device *DEVTREE_CONST igd_dev = &_dev6; Limitations: Actually, chip drivers can't provide global names. What should sconfig generate if a second instance of a chip pops up? So the generated pointers must somehow be limited to chips that can only occur once, e.g. PCH, single socket CPU. What do you think? Nico ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On 27.09.19 15:42, Kyösti Mälkki wrote: > On Thu, Sep 26, 2019 at 7:45 PM Aaron Durbin wrote: >> >> On Thu, Sep 26, 2019 at 10:06 AM Kyösti Mälkki >> wrote: >>> Should be easy enough to implement >>> platform hook telling to not remove PCI device node from topology >>> links (based on BDF), even when it does not respond to ID queries. For those who missed it: we already have a `hidden` keyword in sconfig which complements `on`/`off`. Currently it's only used to write static ACPI _STA methods. Maybe we could just use that? If a device is known to be hidden on purpose, don't detach it from the topology but report attempts to access PCI config? >> >> >> Yes, we can certainly do that. However, I also think this issue and yours >> and Nico's devicetree work are somewhat related as well as >> https://review.coreboot.org/c/coreboot/+/35621. > > I'll try to rebase all my work during the weekend> >> Here's some of the requirements/issues we should resolve that come to mind: >> >> 1. Easy way to directly retrieve a device's chip config object w/o >> traversing the device hierarchy. Which leads to... >> 2. Symbol alias for accessing struct device directly (no bdf lookup) More on this in a separate email. >> 3. Symbol alias for pci b/d/f lookup (equivalent of simple device but >> cleaner) so we can perform pci operations. IIRC, I asked this elsewhere already. Do we want to keep simple device? If we reduce `struct device` to b/d/f and a pointer to the chip info in early stages, couldn't we just use `struct device` for PCI config access everywhere? >> 4. possibly hidden pci devices >> >> Anything else I'm missing? I think a lot of the issues we're running into >> could be fixed w/ the above. Let me know what you think. >> > > 5. PCI coalesce can alter PCI dev.fn assignments? That's a serious problem. I noticed that CFL FSP can reassign them without being asked to, unpredictably (e.g. if a device fails to show up in whatever timeframe FSP assumes). Solution might be simple? A chip->enable_dev() that updates b/d/f based on DID? > 6. Devicetree in ENV SMM is problematic when chip config is mutable in > ENV_RAMSTAGE. Do we need chip config in SMM? Nico ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Fixing Hidden PCI devices on Intel common SoCs
On Thu, Sep 26, 2019 at 7:45 PM Aaron Durbin wrote: > > On Thu, Sep 26, 2019 at 10:06 AM Kyösti Mälkki > wrote: >> Should be easy enough to implement >> platform hook telling to not remove PCI device node from topology >> links (based on BDF), even when it does not respond to ID queries. > > > Yes, we can certainly do that. However, I also think this issue and yours and > Nico's devicetree work are somewhat related as well as > https://review.coreboot.org/c/coreboot/+/35621. I'll try to rebase all my work during the weekend. > Here's some of the requirements/issues we should resolve that come to mind: > > 1. Easy way to directly retrieve a device's chip config object w/o traversing > the device hierarchy. Which leads to... > 2. Symbol alias for accessing struct device directly (no bdf lookup) > 3. Symbol alias for pci b/d/f lookup (equivalent of simple device but > cleaner) so we can perform pci operations. > 4. possibly hidden pci devices > > Anything else I'm missing? I think a lot of the issues we're running into > could be fixed w/ the above. Let me know what you think. > 5. PCI coalesce can alter PCI dev.fn assignments? 6. Devicetree in ENV SMM is problematic when chip config is mutable in ENV_RAMSTAGE. 7. In general, trend seems to be the need to do more and earlier, like assign some resources of SATA or eMMC in romstage already. Kyösti ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
