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]: Enhance the FdSizeReportGenerator.py to support multiple FD #644

Closed
MarcChen46 opened this issue Dec 7, 2023 · 1 comment · Fixed by #647
Closed

[Feature]: Enhance the FdSizeReportGenerator.py to support multiple FD #644

MarcChen46 opened this issue Dec 7, 2023 · 1 comment · Fixed by #647
Assignees
Labels
state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps type:bug Something isn't working type:feature-request A new feature proposal urgency:medium Important with a moderate impact

Comments

@MarcChen46
Copy link
Contributor

Feature Overview

It is possible that UEFI will define multiple FDs in FDF file, but currently below implementation from FdSizeReportGenerator.py only use the first FD it find from the Build Report.

It would be better to enhance it to capture all FDs info first, then each FV can show correct system address with the FV offset + FD base address it belongs to.

def ParseFdInfo(self):
    CurrentLine = 0
    FoundAll = False
    FoundIt = False
    while(CurrentLine < self.TotalLines) and not FoundAll:
        line = self.ReportFile[CurrentLine]

        if FoundIt:
            tokens = line.strip().split(':')
            if len(tokens) == 2:
                if(tokens[0].strip().lower() == "fd name"):
                    self.FdName = tokens[1]
                elif(tokens[0].strip().lower() == 'base address'):
                    self.FdBase=tokens[1]
                    self.FdBaseNum = int(self.FdBase, 0)
                elif(tokens[0].strip().lower() == 'size'):
                    self.FdSize = tokens[1].split()[0].strip()
                    self.FdSizeNum = int(self.FdSize, 0)
                    FoundAll = True
        elif line.strip().lower() == 'firmware device (fd)':
            FoundIt = True

        CurrentLine = CurrentLine + 1

Solution Overview

Catch all FD base address in ParseFdInfo method, then calculate the FV's system address by FV offset + FD base address it belongs to.

Alternatives Considered

No response

Urgency

Medium

Are you going to implement the feature request?

Someone else needs to implement the feature

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

@MarcChen46 MarcChen46 added state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal labels Dec 7, 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 state:needs-owner Needs an issue owner to be assigned labels Dec 7, 2023
@github-actions github-actions bot removed the state:needs-owner Needs an issue owner to be assigned label Dec 7, 2023
@Javagedes
Copy link
Contributor

#647 has been created, which re-writes the report to support multiple FDs as expected.

@Javagedes Javagedes added type:bug Something isn't working and removed state:needs-triage Needs to triaged to determine next steps labels Dec 13, 2023
Javagedes added a commit that referenced this issue Dec 18, 2023
## Description

Re-writes most of the FDSizeReportGenerator Plugin to enable support
accurately showing multiple FDs in a single FDF file. Previously, only
the first FD detected was used for generating the report. While all
regions would be shown, the offsets were based off the first FD and
would be innacurate. This resolves #644

### Before Report
<img width="378" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/f96633ee-a4af-45c3-9dab-6704a0802c3e">
<img width="1244" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/2768b48c-8796-4b24-b4a8-98ca88235a8b">

### After Report
<img width="722" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/8fc67096-d9d6-4e06-ad5e-7092b3e39aab">
<img width="1244" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/dff50ba3-7e2a-4e25-a1fb-e8b106552f50">

- [ ] 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

Verified report is correctly generated both via the plugin and command
line. Verified report generates for multiple different platforms.
## Integration Instructions
kenlautner pushed a commit that referenced this issue Jan 19, 2024
## Description

Re-writes most of the FDSizeReportGenerator Plugin to enable support
accurately showing multiple FDs in a single FDF file. Previously, only
the first FD detected was used for generating the report. While all
regions would be shown, the offsets were based off the first FD and
would be innacurate. This resolves #644

### Before Report
<img width="378" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/f96633ee-a4af-45c3-9dab-6704a0802c3e">
<img width="1244" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/2768b48c-8796-4b24-b4a8-98ca88235a8b">

### After Report
<img width="722" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/8fc67096-d9d6-4e06-ad5e-7092b3e39aab">
<img width="1244" alt="image"
src="https://github.com/microsoft/mu_basecore/assets/24388509/dff50ba3-7e2a-4e25-a1fb-e8b106552f50">

- [ ] 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

Verified report is correctly generated both via the plugin and command
line. Verified report generates for multiple different platforms.
## Integration Instructions
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 type:bug Something isn't working 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