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

UefiCpuPkg: RISC-V: Initialize FPU #6210

Merged
merged 3 commits into from
Oct 4, 2024
Merged

Conversation

xypron
Copy link
Contributor

@xypron xypron commented Sep 17, 2024

Description

EDK II build with TLS running on top of QEMU/KVM crashes when reaching the first floating point instruction in the OpenSSL library (https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/2036604). This is due to the floating point unit being in an disabled state (FS bit-field in CSR mstatus).

With the series a library module BaseRiscVFpuLib is added. Here the relevant CSRs are initialized.
In future we can add here the initialization of the vector unit once any library uses it.

How This Was Tested

Running with QEMU/KVM on SiFive HiFive Premier.

Integration Instructions

N/A

Copy link
Contributor

@vlsunil vlsunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Heinrich,
Thank you very much for the PR. The changes look good to me. However, please fix CI issues like adding to spellcheck ignore list.

@xypron
Copy link
Contributor Author

xypron commented Sep 23, 2024

@vlsunil:
Thank you for reviewing.
There are some issues that I should fix. E.g PatchCheck.py does not like changing two packages with the same patch.
But spellcheck (in https://dev.azure.com/tianocore/11ea4a10-ac9f-4e5f-8b13-7def1f19d478/_apis/build/builds/150553/logs/312) complains about words that are not in my patches, e.g. 'abvoe' in

UefiCpuPkg/Library/MpInitLib/MpLib.c:952:
@param[out] SizeAbove1Mb Return the size of abvoe 1MB memory for AP reset area.

@xypron
Copy link
Contributor Author

xypron commented Sep 24, 2024

For #6210 I get this test result:
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=151342&view=ms.vss-test-web.build-test-results-tab&runId=1394573&resultId=100012&paneView=debug
How can I find out what file CI is complaining about?
When I run uncrustify locally I do not see any error except parsing errors for EmbeddedPkg and BaseTools, which I did not touch in my PR.

@leiflindholm
Copy link
Member

Hi Heinrich, Thank you very much for the PR. The changes look good to me. However, please fix CI issues like adding to spellcheck ignore list.

@vlsunil The spellchecker is in audit mode, so that's not the failure, just noise.
The failure is in uncrustify, but I can't figure out what it's upset about...

@makubacki can you help figure out what's going on?

@makubacki
Copy link
Member

@makubacki can you help figure out what's going on?

  1. If you go to the link provided in xypron's earlier comment - https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=151342&view=ms.vss-test-web.build-test-results-tab&runId=1394573&resultId=100012&paneView=debug.

  2. Click "Attachments".

    image

  3. Click "Standard_Error_Output.log".

    image

  4. There you will see a diff showing what needs to be done:

Uncrustify found 1 files with formatting errors:
Formatting errors in Include\Register\RiscV64\RiscVEncoding.h
--- D:\a\1\s\MdePkg\Include\Register\RiscV64\RiscVEncoding.h
+++ D:\a\1\s\MdePkg\Include\Register\RiscV64\RiscVEncoding.h.uncrustify_plugin
@@ -78,7 +78,7 @@
#define CSR_TIME   0xc01

/* Floating-Point */
-#define CSR_FCSR     0x003
+#define CSR_FCSR  0x003

/* Supervisor Trap Setup */
#define CSR_SSTATUS  0x100

That said, I will take this an opportunity to add the information directly to the build output. That was avoided early on due to concern about the larger number of errors while the codebase was in transition. Now, I think it's obvious placing this information front and center would be more efficient to resolve the problem.

@xypron
Copy link
Contributor Author

xypron commented Sep 25, 2024

@makubacki: Thanks for pointing me to the right attachment.

Looking at https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification/draft/5_source_files/52_spacing.html#52-spacing I would not know which rule I have broken.

Does the CI assume that all files are formatted via

uncrustify -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg -f

even if they don't break any rule in https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification?

@vlsunil
Copy link
Contributor

vlsunil commented Sep 25, 2024

Thanks! @makubacki for the help!. Thanks @xypron for fixing the CI issue. I always run uncrustify locally using the edk2's config but you have a good point on documentation vs uncrustify expectations. May be @makubacki or @mdkinney can help clarify.

Since major changes are in MdePkg and UefiPkg, need help from @mdkinney and @niruiyu to review and merge this PR.

@makubacki
Copy link
Member

Does the CI assume that all files are formatted via

uncrustify -c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg -f

even if they don't break any rule in https://tianocore-docs.github.io/edk2-CCodingStandardsSpecification?

Yes. There was much deliberation, that ultimately led to the decision to apply consistency to elements like alignment and spacing that were too tedious to include in the specification.

Many IDEs have a way to run an external tool with additional context. I personally use VS Code with the uncrustify plugin as described here: EDK II Code Formatting - Recommended Usage: Visual Studio (VS) Code Plugin. I stage or commit the file with actual modifications and then use a keyboard shortcut to run uncrustify on the current file. After a comparison to see what changed, I add the changes to the staging area or amend my commit.

@vlsunil
Copy link
Contributor

vlsunil commented Sep 30, 2024

@mdkinney @niruiyu - Let me know if you have any feedback on this PR. I plan to merge this in next couple of days if no objection.

@vlsunil vlsunil force-pushed the fpu_init branch 2 times, most recently from 8026849 to 4817ab0 Compare October 3, 2024 15:22
* Define CSR fcsr
* Define bitmasks for vs and fs bit fields in the mstatus register

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
The OpenSSL library uses floating point registers.
The is no guarantee that a prior firmware stage has enabled the FPU.

Provide a library BaseRiscVFpuLib to

* Enable the FPU and set it to state 'dirty'.
* Clear the fcsr CSR.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Enable building the BaseRiscVFpuLib library for OvmfPkg to

* Enable the FPU and set it to state 'dirty'.
* Clear the fcsr CSR.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
@vlsunil vlsunil added the push Auto push patch series in PR if all checks pass label Oct 4, 2024
@mergify mergify bot merged commit 91d8069 into tianocore:master Oct 4, 2024
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants