RFC: EDK II Build System Config<T> Interop

[kouchekiniad]

Description

State: Draft

RFC for EDK II Build System and Config<T> inter-operation.

[makubacki]

This feels intrusive and arbitrary to me. The data owner is not necessarily aware of which values a platform may need to configure and platform owners might not even agree with each other.

Please consider an approach that allows component config authors to not be burdened by this and platform owners to not have to depend on config authors to get this right.

For example (just an example, not advocating for this), a tool could instead convert all config to a data representation format (like JSON), platforms could map their config values to the exported JSON fields, and that mapping could result in autogenerated code to connect the chosen platform input (e.g. PCD) to a given config value.

The point being here that the Rust code does not need any awareness that this is happening.

[kouchekiniad]

Removing the macro would certainly be doable, but would come with some limitations:

• There would be no Patina-side indication that a struct isn't compatible with external configuration (has some non-standard, non-deserializable data type), but I suppose the EDK2 plugin could throw an error if it detects someone is trying to modify a non-compatible struct
• Configuration structs would need to be #repr(C) and #pack(1) for cross-compiler-version (and nightly) compatibility.
  • One possibility I was exploring with the derive macro was to derive a custom deserialization routine per-struct that would allow non-#repr(C) and non-packed data structures to be used. I don't think this would be possible without explicit derives.

[kouchekiniad]

Adding onto my first point, I think not calling this out explicitly could become a liability if one user of Patina updates a configuration struct in a way that makes it incompatible with other users of Patina using external configuration. Perhaps one way around this would be to require a set of rules any structure used in a Config<> must follow.

[makubacki]

I was envisioning this "translation component" to break the binary compat requirements between a Config<T> and anything else. I look at Config<T>'s belonging to Rust producers and HOB<T> belonging to C producers. This component is Rust producer.

So, that means the HOB definitions need to meet the C compat requirements (such as #repr(C)), but not necessarily the config definitions, right?

[kouchekiniad]

I was envisioning the HOB struct definition and conversion of a HOB to a Config as being derived automatically by the proc macro so that a configuration struct would only need the #[derive], which would handle the HOB-specifics. Just mapping PCDs to Patina-defined HOB<T> would be a simpler implementation, but the conversion to Config<T>s would have to be manual.

[kouchekiniad]

To be clear, I believe proc macros are required for a significant portion of the functionality this RFC aims for, namely:

• Automatically generating Config<T>s from the HOB passed in from EDK2
  • Without this proc macro, users would need to manually define HOB structures, implement or derive FromHOB, and implement their own component which would generate a Config<> from the user-defined HOB<>. Without the macros doing this automatically, the scope of this RFC would need to be reduced to only an EDK2 build plugin that searches for defined HOB structures in Patina and generates them as needed.
• Component-specific configuration
  • Without being able to embed any component metadata (such as the name of a component associated with that particular version of the configuration struct), component-specific configuration would be limited to cases where implementors of config structs manually define multiple HOBs in the code, one for each component. This would require the Patina side to know ahead of time that EDK2 wants a specific component to have a modified version, which would make a DSC PCD overrides require Patina-side changes to accommodate the new instance/identifier of the HOB.
• Conditional Config<> installation
  • Without proc macros, there would be no way of implementing a standardized way of checking whether a config struct is valid and should be installed. That would need to be done in the per-config components users would need to manually define

[makubacki]

• Automatically generating Configs from the HOB passed in from EDK2

As mentioned offline, the concern here is ownership. A config author should work in Rust and the component system. They cannot be expected to know how an arbitrary set of potential platforms might want to set configuration values.

• Component-specific configuration

This is a dependency for the currently proposed design and should technically be its own RFC. It should state that need and let that design have a focused conversation.

[kouchekiniad]

I've rewritten the RFC to call out that this macro behavior will be done without config struct owner involvement if possible using alternative means, and have removed component-specific configuration, marking it as a future RFC.

[kouchekiniad]

@makubacki I've done some preliminary research and I'm fairly confident the only options here for codegen would be:

1. Requiring a derive macro on config structs
   • To get around configuration struct authors having to take account of whether their struct might be externally configured, one option could be to require a #[derive(Config)] on all types used in Config<T>s. Some mechanism, such as a private marker trait, would need to be added to make wrapping a non-#[derive(Config)] struct in a Config a compile-time error.
2. Performing codegen through a build script (As an xtask, cargo make task, or through Build.rs) that downloads dependency source code and parses them with syn. This build script would be required on the top-level crate that is used to generate an .efi Patina binary.

[makubacki]

I may have missed it, but can you please summarize somewhere (if not done already) why this is only scoped to fixed PCDs? For example, what about Feature PCDs, build flags, etc.

I'm not necessarily disagreeing with the approach, just requesting the reason be documented.

[kouchekiniad]

I've updated the RFC to go over the different data types permitted. Right now it's at: FixedAtBuild PCDs, FeatureFlag PCDs, PatchableInModule PCDs, and EDK II Build System Variables

[makubacki]

If not already mentioned, please describe the custom changes required here and the plan to cover their support in the various edk2-style projects. For example, are you committed to take any changes necessary to tianocore?

[kouchekiniad]

I've written a short statement about this in the new draft. I think this would depend on 1) How invasive the changes will end up needing to be, and 2) Whether the build plugin will need to take a dependency on a Patina crate. If it does end up taking a dependence on the component crate that would be introduced by this RFC, then it may be best to keep it separate to ensure it can be updated to match updated versions of the crate easily.

[makubacki]

If I understand the motivation correctly, there is a desire to have platform configuration defined once and the EDK II mechanisms are the common denominator that I think should be called out more directly in this section.

[kouchekiniad]

I've updated the section to call that out directly.

[makubacki]

Let me know if I misunderstand the open, but can't this be done by changing the T if a component needs that? Like struct NewType(T);. Config<NewType>

[kouchekiniad]

That would require the Patina-side Rust code to know ahead of time that the EDK2 build system override a config structure for a particular component AFAIK.

[makubacki]

I think what you're looking for is multiple instances of the same config definition. If that's the case, can you clarify that the open is config instances targeted to specific components?

[kouchekiniad]

I've reworded that section to be more clear.

[Javagedes]

Config is considered public config and is one value per platform. Different components cannot have different config.

Private config, which is what you instantiate the component with, is a value set on a per-component basis.

From the sounds of it, the current proposed solution will not work by using Config if you are possibly wanting different values per component (like pcd overrides). In this hypothetical proposed solution, I think what will end up happening is whatever the last value of a specific pcd we parse is (from the HOB list), will be the pcd / Config value that gets passed to all components during execution.

I'm sure we could either re-write Config, or add a different parameter that is different per component - but that's how it is today.

[kouchekiniad]

@Javagedes I see the open question here as being whether Config model should be modified to allow this behavior, I'll make that more clear.

[kouchekiniad]

@Javagedes I've removed Component-specific configurations from this RFC to defer it to a potential future RFC.

[Javagedes]

I'm sure I've missed any conversation that happened around this PR but have you considered an alternative such as linking the AutoGen.h file that gets generated for each INF? You could just put the pcd in the INF and grab it from the AutoGen.h file during the cargo build. You can override the RUST_FLAGS var in the INF to link in the header.

extern "C" {
    static gMyPcdValue bool;
}

...
.with_config(MyConfig::new(gMyPcdValue)

The other option would be to create a macro that can parse the header file at compile time and grab whatever pcd you want. Something like .with_config(MyConfig::new(pcd!("gEfiMyPcd", pcd!("gEfiMyOtherPcd"))

Either of these options would lessen the intermixing of edk2 build system and patina / cargo build, and not out this burden on the config writer

[makubacki]

I just requested Daniel create the PR for feedback, so you didn't miss anything I'm aware of. It would be nice to reduce custom file formats and edk2 tools investments where possible.

[kouchekiniad]

My main reservations with this would be:

• Patina cargo modules would start to become dependent on EDK2 build artifacts
• Patina source code becomes aware of EDK2 by explicitly calling out PCD names

I'd see both of these as intermingling EDK2 and Patina build systems more than adding a Patina-aware EDK2 build plugin would be.

[Javagedes]

Yeah I think I misunderstood what you were trying to accomplish. The derive trait made me think you were wanting to intermingle edk2 build system and Patina - but you are not.

I'm wondering if it better to have a service more akin to the PCD Database where a component parses your hob(s) that publish PCDs in some well known format and stores then as their raw bytes. Then the PcdService can be requested by any component, and the component just grabs the PCD it cares about. The conversion of bytes to T would be safe so long as we use zero copy+ type validation.

PcdDb::get<T>(&self, pcd: &str)->Option<&T>

PcdDb::get_for<T, C: Component>(&self, pcd: &str)-> Option<&T>

fn my_compnent(db: Service<PcdDb>)->Result<()> {
  let generic_pcd: bool = db.get("gEfiPcd").unwrap();

  let specific: bool = db.get_for::<Self>("gEfiMyPcd").unwrap();
}

To be extra safe you could make T have some trait requirement, where the only thing that implements T are the few types supported by PCDs as you mentioned in your RFC

[makubacki]

Sorry for the longer comment, I just put everything here since most of it is related.

---

I don't think we want components to deal directly in PCDs though even through a service.

• Component: Depends on Config or Hob
  • Rules of thumb:
    • HOB definitions are built to be C compatible
    • Config definitions do not guarantee C compatibility
    • Component writer should be unaware how a Config is populated. They should simply depend on the Config. HOBs are by definition, produced in a C environment today.

Before going too far, this RFC needs to be clear about what is supported as that will shape the conversation and solution. FixedAtBuildPcds are static macros placed in the auto gen file. Mechanically then, I don't see this as solely supporting "FixedAtBuildPcds" but statically compiled values of which FixedAtBuildPcds are an instance of.

For clarity, the RFC should have a table (matrix) of common configuration types to show what is in scope and what is not - including static types such as PcdsFixedAtBuild, PcdsFeatureFlag, build variable, etc. and runtime types such as DynamicEx, UEFI variables, etc. Then, in the future, someone who uses that type can refer to the table and see whether they can use this design to configure what they need. If you only choose to support FixedAtBuildPcd as in your current direction, that's fine, but make that clear and explain why.

That will also help with naming and scope of how to think about the input data. While FixedAtBuildPcds are populated in autogen, dynamic PCDs will reside in the PCD database at runtime and introduce additional challenges such as race conditions between the PCD value being set and the HOB being produced.

A basic issue is that you're seeking to connect two disparate data structures. A data entity from the EDK II build may map to an arbitrary field in a number of various configs. By it's nature, this mapping is platform-specific. The data entity (e.g. PcdAbc for ConfigMemberX in ConfigY) chosen by one platform may not be the same for all platforms. Therefore, I think this mapping needs to be cleanly and clearly established by the platform owner. I believe that's what you're attempting to do here:

# MyComponent.inf

[Defines]
  INF_VERSION                    = 0x00010005
  BASE_NAME                      = MyComponent
  MODULE_TYPE                    = PATINA_COMPONENT

[ComponentConfigurations]
    MyConfigStruct.Field1 = PcdNameSpace.PcdName

I don't have a strong opinion about how this is implemented, mostly that it is in platform control and allows the mappings. My understanding is that this implies configuration is customized per component. That should be added to the Requirements section (I also left a comment there). This also implies that the PCD values may be customized per "Component INF". If that's the intention, call it out directly, and provide an example of how this is done. For example, showing a "Component INF" PCD override in the DSC file.

Assuming multi-instance config will be supported, can you cover the case when the same value applies to all components? For example, is the platform owner expected to create a new INF for every component?

I think you also need further explanation here of what PATINA_COMPONENT means as you've defined it - what exactly is produced from this INF file?

Finally, to help guide the design conversation with less gaps to your vision, I think you need a diagram that shows two flows:

1. The build flow - showing what is done to produce what ends up in the flash file
2. The runtime flow - showing what happens during boot from the "pre-DXE" environment to configs being consumed by components

[makubacki]

@kouchekiniad and I spoke offline. He's going to refactor the RFC a bit, so it might be useful to wait until that's done before spending too much time reviewing the current content.

[kouchekiniad]

@makubacki I've taken your feedback and have written a new draft of the RFC. The diagrams are still in-progress.

[os-d]

Can you share what an example of the configuration you are trying to share is? What patina component has some reliance on what a PCD value is? I have a few concerns with this approach, but one is the coupling of patina config with edk2 config. I think the goal should be that these are generally not coupled and most of edk2's config should be dropped from patina. edk2 is far too configurable, to its detriment.

[kouchekiniad]

FYI I've largely refactored this RFC since your comment.

One example is PerfConfig:

pub struct PerfConfig {
    /// Indicates whether the Patina Performance component is enabled.
    pub enable_component: bool,
    /// A wrapper to generate a mask of all enabled measurements.
    pub enabled_measurements: u32,
}

Whether the Performance component should be enabled and which measurements should be enabled is dependent on behaviors in C drivers which is controlled by a bits in gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask.

In the scheme I'm proposing there would be a means to provide this configuration structure through a HOB which would be automatically (through proc macros if at all possible) converted into a Config with no effort on the part of the maintainer of the PerfConfig struct. It is possible, however, that to be feasible a #[derive] may need to be placed directly on the struct.

The mapping between PCDs and these structures would be made entirely on the EDK II side, making Patina largely unaware of the existence of PCDs.

A section like the following could be placed in a EDK II .inf, which would be used to automatically generate a matching HOB.

[PatinaConfigurations]
 PerfConfig.enable_component = $(SOME_PERFORMANCE_ENABLED_BUILD_VAR)
 PerfConfig.enabled_measurements = gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask

The end result is a Component that produces Config<T>s, much like any other Component might.

[Javagedes]

Is there another example? We've already determined that the overall perf implementation is incorrect because it improperly splits functionality between the C and the Rust code.

That design is an anti-pattern that should not be done.

The intent (with an issue filed) is to move all of perf to a patina component.

[os-d]

I will go through and review the refactored RFC. Adding on to what Joey said, my main concern, as I noted before, is about bringing too much configuration to Patina. That is a major flaw of edk2 and leaves complicated code patterns that are untestable and frequently bring bugs. Patina is more opionated and tbh, I think every configuration option needs to be evaluated closely on whether it is appropriate; most often that answer will be no, it is not.

Now, there will of course be necessary configuration and I fully understand from a platform integrator's point of view it is nice to be able to write that once and have it easily transfer to all parts of the system. I think that is a reasonable goal, but that it needs to live entirely in the platform layer, not in Patina. Patina should be entirely agnostic to how a config was generated and of course should only operate on config, not anything from edk2 (which you aren't proposing).

So, like I said, let me review the refactored RFC, but my concern about this approach is that it will be too easy to bring too much config to Patina, which as Joey is saying is something we consider to be an anti-pattern. Having some tool of this nature is likely to be useful to the platform, but imo should be very scoped to the needed set of configuration (which, if your proposal requires INF changes, then it sounds like it would be anyway, but let me review).

[kouchekiniad]

@Javagedes Even if all of Perf is moved to Patina, platforms will still likely want to have control over whether performance is enabled. This may be based on a combination of build arguments that would not be easily synced manually between the module containing the core and the EDK II build system without an interop.

More clear-cut examples would be:

• Enabling/Disabling Features
• Providing platform-specific information such as
  • MMIO addresses
  • Hardware information
  • Vendor ID information
• Configuring behavior common to C and Rust such as logging verbosity

While uses that orchestrate compatibility with C and Rust drivers may be an anti-pattern, this will be unavoidable for drivers that are provided by third parties or which may have PEI components. From my perspective of working on platform code, not having this ability is a major blocker in terms of being able to port over any form of platform driver to components.