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

Only call HdwPortWrite if DebugLevel Met #311

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

os-d
Copy link
Contributor

@os-d os-d commented Sep 11, 2023

Description

The DebugLevel is checked twice in the hot path on serial path writes and the HdwPortWrite call is made even if the upper layer knows that the message being logged does not meet the DebugLevel criteria.

Closes #309.

In order to maintain backwards compatibility, if the LoggerInfo block is found to have a version less than the hardware logging level version, the PCD is checked to decide whether to call HdwPortWrite or not.

In SEC, because we may not have the LoggerInfo structure, we check the PCD to see if the message should be logged at this DebugLevel.

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

  • 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, ...
  • 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, ...
  • 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

Build tested.

Integration Instructions

If a bespoke AdvLoggerHdwPortLib is used, AdvLoggerHdwPortWrite should not check DebugLevel, but simply write the message to the hardware port.

@apop5
Copy link
Contributor

apop5 commented Sep 12, 2023

The intention of this second Pcd was to allow PcdFixedDebugPrintErrorLevel and PcdAdvancedLoggerHdwPortDebugPrintErrorLevel to both function independently. The debug messages written into the advanced logger memory buffer would be based on the PcdFixedDebugPrintErrorLevel and the debug messages actually written out to a hardware port could be less (since the writes to some things, like serial ports, can be much slower than copying into a memory buffer).

Are you intending to remove this feature?

@os-d
Copy link
Contributor Author

os-d commented Sep 12, 2023

The intention of this second Pcd was to allow PcdFixedDebugPrintErrorLevel and PcdAdvancedLoggerHdwPortDebugPrintErrorLevel to both function independently. The debug messages written into the advanced logger memory buffer would be based on the PcdFixedDebugPrintErrorLevel and the debug messages actually written out to a hardware port could be less (since the writes to some things, like serial ports, can be much slower than copying into a memory buffer).

Are you intending to remove this feature?

This is not intended to remove that feature. PcdAdvancedLoggerHdwPortDebugPrintErrorLevel is still checked (it is loaded into LoggerInfo->HwPrintLevel). This just avoids checking this twice (once in AdvancedLoggerLib, once in AdvancedLoggerHdwPortLib) and avoids calling into AdvancedLoggerHdwPortLib when we know we won't be logging anyway.

In the legacy case, where the new code has loaded a LoggerInfo that doesn't have the HwPrintLevel field in it, we'll check the PCD explicitly.

@kuqin12 kuqin12 merged commit 4c5d1d1 into microsoft:release/202302 Oct 10, 2023
kuqin12 added a commit that referenced this pull request Oct 11, 2023
…usage (#331)

# 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

There was a missed PCD not caught in the previous PR
(#311) when it comes to the
usage on AARCH64 platform. This change added the PCD entry in the
library inf file.

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

- [ ] 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, ...
- [ ] 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, ...
- [ ] 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 was tested on FVP based AARCH64 platform.

## Integration Instructions

N/A
ProjectMuBot referenced this pull request in microsoft/mu_tiano_platforms Oct 12, 2023
Introduces 8 new commits in [Common/MU](https://github.com/microsoft/mu_plus.git).

<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/microsoft/mu_plus/commit/c528cb5ac7eca1e0ae02f5d73dcb7c8cba4939ea">c528cb</a> Spellcheck Fixes</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/af263a940da344a33bbb715dd5db234a1e0b0196">af263a</a> Add FlatPageTableLib</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/277d45568c2b05cde80b37c95f11a255dd6a1016">277d45</a> Update DxePagingAudit to use FlatPageTableLib</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/4c5d1d1c3c78d4329a550ba6f5d049834d456dcc">4c5d1d</a> Only call HdwPortWrite if DebugLevel Met (<a href="https://github.com/microsoft/mu_plus/pull/311">#311</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/115ef0b24bdaf267d3ac08171fdd85a575704084">115ef0</a> pip: bump edk2-pytool-library from 0.18.2 to 0.19.0 (<a href="https://github.com/microsoft/mu_plus/pull/330">#330</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/bd605b16a2008a25dfc86126685a2c6928388200">bd605b</a> pip: bump edk2-pytool-extensions from 0.24.1 to 0.25.0 (<a href="https://github.com/microsoft/mu_plus/pull/329">#329</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/ec920bacb71f973240723115c3ecb3f214c827c3">ec920b</a> Repo File Sync: synced file(s) with microsoft/mu_devops (<a href="https://github.com/microsoft/mu_plus/pull/328">#328</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/142315478f245d0cb686286d79502912397530c1">142315</a> AdvLoggerPkg: BaseAdvancedLoggerLib: Fixing a missed PCD for AARCH64 usage (<a href="https://github.com/microsoft/mu_plus/pull/331">#331</a>)</li>
</ul>
</details>

Signed-off-by: Project Mu Bot <mubot@microsoft.com>
makubacki referenced this pull request in microsoft/mu_tiano_platforms Oct 13, 2023
Introduces 8 new commits in [Common/MU](https://github.com/microsoft/mu_plus.git).

<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/microsoft/mu_plus/commit/c528cb5ac7eca1e0ae02f5d73dcb7c8cba4939ea">c528cb</a> Spellcheck Fixes</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/af263a940da344a33bbb715dd5db234a1e0b0196">af263a</a> Add FlatPageTableLib</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/277d45568c2b05cde80b37c95f11a255dd6a1016">277d45</a> Update DxePagingAudit to use FlatPageTableLib</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/4c5d1d1c3c78d4329a550ba6f5d049834d456dcc">4c5d1d</a> Only call HdwPortWrite if DebugLevel Met (<a href="https://github.com/microsoft/mu_plus/pull/311">#311</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/115ef0b24bdaf267d3ac08171fdd85a575704084">115ef0</a> pip: bump edk2-pytool-library from 0.18.2 to 0.19.0 (<a href="https://github.com/microsoft/mu_plus/pull/330">#330</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/bd605b16a2008a25dfc86126685a2c6928388200">bd605b</a> pip: bump edk2-pytool-extensions from 0.24.1 to 0.25.0 (<a href="https://github.com/microsoft/mu_plus/pull/329">#329</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/ec920bacb71f973240723115c3ecb3f214c827c3">ec920b</a> Repo File Sync: synced file(s) with microsoft/mu_devops (<a href="https://github.com/microsoft/mu_plus/pull/328">#328</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/142315478f245d0cb686286d79502912397530c1">142315</a> AdvLoggerPkg: BaseAdvancedLoggerLib: Fixing a missed PCD for AARCH64 usage (<a href="https://github.com/microsoft/mu_plus/pull/331">#331</a>)</li>
</ul>
</details>

Signed-off-by: Project Mu Bot <mubot@microsoft.com>
makubacki referenced this pull request in microsoft/mu_tiano_platforms Oct 13, 2023
Introduces 8 new commits in [Common/MU](https://github.com/microsoft/mu_plus.git).

<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/microsoft/mu_plus/commit/c528cb5ac7eca1e0ae02f5d73dcb7c8cba4939ea">c528cb</a> Spellcheck Fixes</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/af263a940da344a33bbb715dd5db234a1e0b0196">af263a</a> Add FlatPageTableLib</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/277d45568c2b05cde80b37c95f11a255dd6a1016">277d45</a> Update DxePagingAudit to use FlatPageTableLib</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/4c5d1d1c3c78d4329a550ba6f5d049834d456dcc">4c5d1d</a> Only call HdwPortWrite if DebugLevel Met (<a href="https://github.com/microsoft/mu_plus/pull/311">#311</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/115ef0b24bdaf267d3ac08171fdd85a575704084">115ef0</a> pip: bump edk2-pytool-library from 0.18.2 to 0.19.0 (<a href="https://github.com/microsoft/mu_plus/pull/330">#330</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/bd605b16a2008a25dfc86126685a2c6928388200">bd605b</a> pip: bump edk2-pytool-extensions from 0.24.1 to 0.25.0 (<a href="https://github.com/microsoft/mu_plus/pull/329">#329</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/ec920bacb71f973240723115c3ecb3f214c827c3">ec920b</a> Repo File Sync: synced file(s) with microsoft/mu_devops (<a href="https://github.com/microsoft/mu_plus/pull/328">#328</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/142315478f245d0cb686286d79502912397530c1">142315</a> AdvLoggerPkg: BaseAdvancedLoggerLib: Fixing a missed PCD for AARCH64 usage (<a href="https://github.com/microsoft/mu_plus/pull/331">#331</a>)</li>
</ul>
</details>

Signed-off-by: Project Mu Bot <mubot@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: AdvancedLoggerHdwPortLib should not check PCD for header versions greater than 2
4 participants