Bug 1714446 - edk2-aarch64 silent build is not silent enough
Summary: edk2-aarch64 silent build is not silent enough
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 8
Classification: Red Hat
Component: edk2
Version: 8.1
Hardware: aarch64
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: 8.1
Assignee: Philippe Mathieu-Daudé
QA Contact: Virtualization Bugs
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-05-28 07:11 UTC by Andrew Jones
Modified: 2020-11-14 07:00 UTC (History)
11 users (show)

Fixed In Version: edk2-20190308git89910a39dcfd-6.el8
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-11-05 20:44:44 UTC
Type: Bug
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2019:3338 0 None None None 2019-11-05 20:45:46 UTC

Description Andrew Jones 2019-05-28 07:11:37 UTC
Paraphrasing (well, mostly quoting) Laszlo:

A downstream-only regression from the latest rebase (bug 1687731) has been introduced. The regression comes from downstream commit 76b4ac28e975 ("ArmVirtPkg: silence DEBUG_VERBOSE (0x00400000) in QemuRamfbDxe (RH only)", 2019-03-20).

The problem is that the commit doesn't only clear the DEBUG_VERBOSE bit, it also unconditionally sets some other bits, which we normally keep clear in the "silent" build.

To fix up this downstream-only commit we need to replace the

  0x8000004F

constant it added, with

  ($(DEBUG_PRINT_ERROR_LEVEL) & 0xFFBFFFFF)

Because we build the "silent" binary with

  -D DEBUG_PRINT_ERROR_LEVEL=0x80000000

Comment 1 Laszlo Ersek 2019-05-28 09:42:36 UTC
To refine the suggested syntax: we should likely write:

      gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|($(DEBUG_PRINT_ERROR_LEVEL)) & 0xFFBFFFFF

Because: the edk2 DSC spec at <https://edk2-docs.gitbooks.io/edk-ii-dsc-specification/content/2_dsc_overview/22_build_description_file_format.html#table-3-using-system-environment-variable> provides the following example:

[LibraryClasses.X64, LibraryClasses.IA32]
  DEFINE PERF = PerformancePkg/Library
  TimerLib|$(PERF)/DxeTscTimerLib/DxeTscTimerLib.inf

This implies that macro expansion doesn't (mustn't) automatically wrap the replacement text in parentheses. And therefore, in preparation for "tricky" values in DEBUG_PRINT_ERROR_LEVEL, we should tightly enclose $(DEBUG_PRINT_ERROR_LEVEL) in parens.

Furthermore, an outermost set of parens doesn't look necessary (the PCD setting syntax doesn't appear to require it).

Comment 4 Laszlo Ersek 2019-07-26 16:01:57 UTC
Here's a summary of the rhvirt-patches review discussion (linked in
comment 3):

(1) The patch, as posted, uses the mask ~0x00400000 for clearing the
DEBUG_VERBOSE bit. Using the bitwise-complement operator for setting
PCDs in the DSC is not recommended. The edk2 DSC spec refers (somewhat
vaguely) to the C standard when it specifies such PCD assignments. In C,
on all platforms that we care about, (~0x00400000) leads to flipping the
sign bit in a signed integer. We should avoid that. If we could write
(~0x00400000u), that would be fine, but my understanding is that the DSC
spec does not permit that. I suggest using 0xFFBFFFFF as mask,
explicitly. (This C language constant has type "unsigned int" on all
platforms that we care about.)

(2) There are two new log messages that the patch does not suppress:

- "Error: Image at 00047505000 start failed: Not Found"
- "remove-symbol-file ..."

Suppressing each of these will require a dedicated patch; see below:

(2.1) The first message comes from the DXE Core. It can only be
suppressed with downstream-only changes, as printing the message at
DEBUG_ERROR level is the right thing for the DXE Core otherwise, and it
is also right for QemuRamfbDxe to exit with an error under the given
circumstances. Options:

(2.1.1) Downgrade the log mask to DEBUG_WARN in the DXE Core. Bad idea,
we'd lose valuable reports of driver startup failures.

(2.1.2) Change QemuRamfbDxe to return EFI_SUCCESS even when it fails
(e.g. ramfb is not configured). Wastes some memory (driver stays
resident without any good use), but it is mostly harmless, as the memory
is released by the OS after ExitBootServices().

(2.1.3) Replace "-D DEBUG_PRINT_ERROR_LEVEL=0x80000000" in the spec file
with "-D DEBUG_PRINT_ERROR_LEVEL=0x0". This will suppress *all* debug
messages (even errors). Not ideal because (a) it allows errors to
"snowball" (we could notice an issue only much later, from behavioral
problems), (b) it makes the "silent" firmware binary entirely unsuitable
for bug reporting and triaging, (c) it prevents us from cleaning up new
debug messages up-stream that are needlessly logged at DEBUG_ERROR
level.

(2.2) The second message should be downgraded to DEBUG_INFO level, with
an upstream patch, similarly to upstream commit 1fce963d89f3 ("ArmPkg:
DebugPeCoffExtraActionLib: debugger commands are not errors",
2015-03-02). This upstream patch should be backported then.


My recommendation is a series of 3 patches:
- patch #1: what was posted already (downstream-only patch), but replace
            ~0x00400000 with 0xFFBFFFFF
- patch #2: downstream-only patch implementing (2.1.2)
- patch #3: backport of new upstream patch implementing (2.2)

Thanks!

Comment 5 Andrew Jones 2019-07-27 09:53:10 UTC
(In reply to Laszlo Ersek from comment #4)
> My recommendation is a series of 3 patches:
> - patch #1: what was posted already (downstream-only patch), but replace
>             ~0x00400000 with 0xFFBFFFFF
> - patch #2: downstream-only patch implementing (2.1.2)

Yes, I prefer 2.1.2 as well.

> - patch #3: backport of new upstream patch implementing (2.2)
> 
> Thanks!

Thanks,
drew

Comment 6 Laszlo Ersek 2019-07-30 16:51:14 UTC
For (2.2), Phil posted:

[edk2-devel] [PATCH 0/2] ArmPkg: DebugPeCoffExtraActionLib: debugger
                         commands are not errors
http://mid.mail-archive.com/20190729180321.1715-1-philmd@redhat.com
https://edk2.groups.io/g/devel/message/44569

Pushed by Leif as commit range 8fed4e47d9a6..3d34b5f32692:

1 a6cd7fbac494 ArmPkg: DebugPeCoffExtraActionLib: debugger commands are
               not errors
2 3d34b5f32692 ArmPkg: DebugPeCoffExtraActionLib: fix trivial comment
               typos

Comment 22 errata-xmlrpc 2019-11-05 20:44:44 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2019:3338


Note You need to log in before you can comment on or make changes to this bug.