Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Add prefix for different firmware entities in memory logging #375

Closed
kuqin12 opened this issue Dec 1, 2023 · 0 comments · Fixed by #388
Closed

[Feature]: Add prefix for different firmware entities in memory logging #375

kuqin12 opened this issue Dec 1, 2023 · 0 comments · Fixed by #388
Assignees
Labels
state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal urgency:medium Important with a moderate impact

Comments

@kuqin12
Copy link
Contributor

kuqin12 commented Dec 1, 2023

Feature Overview

Currently all firmware entities that supports advanced memory logging will dump messages into the memory buffer, without any distinguishable notations. Once the parsed log is retrieved from target, all prints are mushed together, making it hard to read.

The request is to add prefix for different instances of advance logging parties, making it easier to parse and locate logs.

Solution Overview

Adding an API for each instance that produces AdvancedLoggerGetLoggerInfo, call it AdvancedLoggerGetStringPrefix. This function will return a constant string and its size. This function will be invoked from AdvancedLoggerMemoryLoggerWrite, where the return will be copied to the front of each memory log message.

We should make the prefix string minimal, as it will bloat the memoy usage in the memory logging buffer.

Alternatives Considered

  1. Create a static PCD for the prefix, so that each component type can use that PCD to prefix the messages.
    • This is a simpler solution by offloading the PCD config to platforms. However, the change will require in-depth analysis of the build configuration and could lead to integration errors.
  2. Create a message entry v2. Instead of materializing the prefix into the memory logging buffer, add a field indicating the library instance (i.e. PEI core for 0, PEIM for 1, etc.) so that the parser can handle the prefix.
    • This is a more ideal solution, but the issue is that the parser application will need to be updated accordingly as this is introducing a breaking change, of which the impact is unknown.

Urgency

Medium

Are you going to implement the feature request?

I will implement the feature

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

@kuqin12 kuqin12 added state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal labels Dec 1, 2023
@github-actions github-actions bot added state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps urgency:medium Important with a moderate impact labels Dec 1, 2023
kuqin12 added a commit that referenced this issue Jan 10, 2024
# Preface

Please ensure you have read the [contribution
docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior
to submitting the pull request. In particular,
[pull request
guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices).

## Description

This change introduces a new message entry structure, which contains
extra information of log producer and can be used to differentiate the
different boot phases, i.e. PEI, DXE, SMM, etc. Platforms that have
multiple firmware entities can leverage this extra information to
distinguish coalesced memory logging regions.

The updated applications (UEFI shell app and host OS Python script) are
also updated to support new message entry structure while maintaining
the backwards compatibility.

Resolves #375.

For each item, place an "x" in between `[` and `]` if true. Example:
`[x]`.
_(you can also check items in the GitHub UI)_

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [x] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [x] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

This change was tested on QEMU Q35 and verified that prefix are properly
injected for DXE phase prints.

## Integration Instructions

The log parsers are updated, thus platforms use the existing parser
should pick up the latest tool from mu_plus. For platforms that carry
their own implementation, they should add the proper support in their
own implementation before picking up this change from mu_plus.
kenlautner pushed a commit that referenced this issue Jan 19, 2024
# Preface

Please ensure you have read the [contribution
docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior
to submitting the pull request. In particular,
[pull request
guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices).

## Description

This change introduces a new message entry structure, which contains
extra information of log producer and can be used to differentiate the
different boot phases, i.e. PEI, DXE, SMM, etc. Platforms that have
multiple firmware entities can leverage this extra information to
distinguish coalesced memory logging regions.

The updated applications (UEFI shell app and host OS Python script) are
also updated to support new message entry structure while maintaining
the backwards compatibility.

Resolves #375.

For each item, place an "x" in between `[` and `]` if true. Example:
`[x]`.
_(you can also check items in the GitHub UI)_

- [x] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [x] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [x] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

This change was tested on QEMU Q35 and verified that prefix are properly
injected for DXE phase prints.

## Integration Instructions

The log parsers are updated, thus platforms use the existing parser
should pick up the latest tool from mu_plus. For platforms that carry
their own implementation, they should add the proper support in their
own implementation before picking up this change from mu_plus.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal urgency:medium Important with a moderate impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants