Date   

Re: [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

Liming Gao
 

Also include NetworkPkg reviewer to collect the feedback for this change.

Thanks
Liming

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, February 28, 2020 1:40 AM
To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
<michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org>
Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com>
Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

On 02/27/20 14:14, Laszlo Ersek wrote:
(+Liming and stewards; CC Nick)

On 02/27/20 12:02, Maciej Rabeda wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already
recycled packet with EFI_SUCCESS token status and finally
dereference invalid pointers from RxData structure.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

ON_EXIT:

+ //
+ // Recycle the packet before reusing RxToken
+ //
+ gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)-
RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal);
+
if (Private->RxCount < Private->SendNum) {
//
// Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
//
Private->Status = EFI_SUCCESS;
}
- //
- // Singal to recycle the each rxdata here, not at the end of process.
- //
- gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)-
RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal);
}

/**
(1) This patch proposes to fix one of the BZs (2032) that fall under
CVE-2019-14559 (joint tracker: 2550).

Consequently:

(1a) Do we want to include this in the upcoming stable tag?

If so, we might want to extend the hard feature freeze by a few days.

(1b) Please append the string " (CVE-2019-14559)" -- note the separating
space! -- to the subject line.

(2) However: I remember from an earlier Bugzilla entry (can't tell
off-hand, which one, sorry) that ShellPkg issues are *never* considered
CVE-worthy, because the shell is not considered a "production element"
of the UEFI boot path.
I misremembered -- there is indeed a comment like that, in the TianoCore
bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which
has since been split off to the edk2-libc project):

https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1

StdLib is supposed to be used only by applications in shell, all of
which are meant for debug, diagnosis and/or test purpose, not for
product UEFI BIOS. Any issue in it will not be taken as security
issue but just normal bug.

Sorry about causing confusion. So, the ShellPkg maintainers should
decide what to do about this bug (keep it under the CVE scope vs.
exclude it from the CVE scope; and then, propose it for the stable tag
or merge it afterwards).

One data point: the bug appears to go back to the inception of the Ping
command, in historical commit 68fb05272b45 ("Add Network1 profile.",
2011-03-25). It's not a new bug, it seems.

Thanks
Laszlo


TianoCore#2032 was originally filed for NetworkPkg, and indeed that
seemed to justify the CVE assignment. However, now that Nick's and
Maciej's analysis shows that NetworkPkg is unaffected (and we know, per
above, that ShellPkg is not CVE-worthy), should we rather *remove* this
BZ from the CVE-2019-14559 umbrella?

Because, in that case, modifying the subject line on the patch is not
necessary; and more importantly, we might not even want to put this into
edk2-stable202002. (It's still a bugfix, but may not be important enough.)

Thanks!
Laszlo


Re: [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build

Bob Feng
 

Laszlo, I agree BZ status should be update in time. You do a very good practices about that. I need to learn from you. And thank you that you have done a lot on BZ maintenance for me and other assignees.

I think we need have a document to record these good practices on BZ maintenance so that people can clear to know what information should be published on BZ in each development stage.

Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Friday, February 28, 2020 1:18 AM
To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Leif Lindholm <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>
Subject: Re: [edk2-devel] [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build

On 02/27/20 16:53, Feng, Bob C wrote:

[Bob] I agree the BZ status should be update in time. I don't think BZ status update is the reviewer's/maintainer's responsibility, the BZ owner should be responsible for it.
Agreed.


NOTE: GitHub.com Pull Requests would not help *at all* in the face of such sloppiness; even on GitHub.com, people have to at least *name* issue numbers in commit messages.

- TianoCore#2563 (which tracks the regression) identifies *neither* the BZ for which the regression was introduced (2481), *nor* the faulty commit (818283de3f6d). You realize it's *completely useless* to file BZs with such negligence, right? It has no more information than "stuff broke, we need to fix it" -- but ain't that the general state of things, at all times? Are you only trying to fill a BZ quota?
[Bob] I don't agree this comments.
I added the bug reproduce steps in BZ description. I think it's enough when I submit a new BZ. I'll append the root cause and solution ( would be just patch review link) in its comments when I update the BZ status later.
Yes, the patch explains the issue well. If the link had been in the BZ, I wouldn't have complained (as much).

We found this critical bug in this afternoon (PRC time) and root cause and created patch very quickly. I don't think that I did not update the BZ in time is process violation.
It was not clear that you ever intended to add the link to the BZ.

I think the necessary information was provided when the patch send out. The bug description and reproduce steps are in BZ, root cause is in the patch commit message, the solution is the patch itself, test result is in the commit message.
Yes. There was no link from the BZ to the patch however. And it wasn't possible to determine whether you were going to add the link later. I tend to add the link immediately after posting, so I don't forget. My experience tell me that most patch submitters that don't add the link at once forget for good.

Yes, it was a generalization, sorry about that.

One thing I do have to admit (because I brought up GitHub.com before) is that, on GitHub.com, if you submit a pull request, and at least one of the commit messages references an Issue (like your commit message here references TianoCore#2563), then the issue automatically gets an update.
So in that regard GitHub.com does save some manual work.

Laszlo


Re: [PATCH 0/3] Arm*Pkg: convert LFs to CRLF, expand hard TABs

Ard Biesheuvel
 

On Thu, 27 Feb 2020 at 22:39, Laszlo Ersek <lersek@redhat.com> wrote:

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1659
Repo: https://github.com/lersek/edk2.git
Branch: crlf_bz_1659

Future patches for some Arm*Pkg source files would either introduce CRLF
lines into LF files, or add more LF lines. The first would increase
inconsistency, the second would make "PatchCheck.py" yell. Convert these
files to CRLF uniformly (and while at it, expand hard TABS, because
"PatchCheck.py" catches those too).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks,
Laszlo

Laszlo Ersek (3):
ArmPkg: convert LFs to CRLF, expand hard TABs
ArmVirtPkg: convert LFs to CRLF
ArmPlatformPkg: convert LFs to CRLF, expand hard TABs
For the series,

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c | 32 +-
ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c | 566 ++++++++---------
ArmPkg/Library/CompilerIntrinsicsLib/memset.c | 110 ++--
ArmPkg/Library/GccLto/liblto-aarch64.s | 42 +-
ArmPkg/Library/GccLto/liblto-arm.s | 110 ++--
ArmPlatformPkg/Scripts/Ds5/profile.py | 656 ++++++++++----------
ArmVirtPkg/Include/Platform/Hidden.h | 44 +-
7 files changed, 780 insertions(+), 780 deletions(-)

--
2.19.1.3.g30247aa5d201


[PATCH 3/3] ArmPlatformPkg: convert LFs to CRLF, expand hard TABs

Laszlo Ersek
 

We're going to switch the internal line terminators globally to LF at some
point, but until then, let's use CRLF consistently. Convert source files
with LFs in them to CRLF, using "unix2dos".

"git show -b" prints no code changes for this patch.

(I collected all the file name suffixes in this package, with:

$ git ls-files -- $PACKAGE | rev | cut -f 1 -d . | sort -u | rev

I eliminated those suffixes that didn't stand for text files, then
blanket-converted the rest with unix2dos. Finally, picked up the actual
changes with git-add.)

At the same time, the following file had to undergo TAB expansion:

ArmPlatformPkg/Scripts/Ds5/profile.py

I used "expand -t 4", conforming to the Indentation section of PEP-8
<https://www.python.org/dev/peps/pep-0008/#indentation>.

Both the CRLF conversion and the TAB expansion are motivated by
"PatchCheck.py". "PatchCheck.py" is also the reason why CRLF conversion
and TAB expansion have to happen in the same patch.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1659
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPlatformPkg/Scripts/Ds5/profile.py | 656 ++++++++++----------
1 file changed, 328 insertions(+), 328 deletions(-)

diff --git a/ArmPlatformPkg/Scripts/Ds5/profile.py b/ArmPlatformPkg/Scripts/Ds5/profile.py
index f87dee24695c..979c6ea2bdc2 100644
--- a/ArmPlatformPkg/Scripts/Ds5/profile.py
+++ b/ArmPlatformPkg/Scripts/Ds5/profile.py
@@ -1,328 +1,328 @@
-#!/usr/bin/python
-
-#
-# Copyright (c) 2014, ARM Limited. All rights reserved.
-#
-# SPDX-License-Identifier: BSD-2-Clause-Patent
-#
-
-import getopt
-import operator
-import os
-import pickle
-import sys
-from sys import argv
-from cStringIO import StringIO
-
-modules = {}
-functions = {}
-functions_addr = {}
-
-def usage():
- print "-t,--trace: Location of the Trace file"
- print "-s,--symbols: Location of the symbols and modules"
-
-def get_address_from_string(address):
- return int(address.strip("S:").strip("N:").strip("EL2:").strip("EL1:"), 16)
-
-def get_module_from_addr(modules, addr):
- for key,value in modules.items():
- if (value['start'] <= addr) and (addr <= value['end']):
- return key
- return None
-
-def add_cycles_to_function(functions, func_name, addr, cycles):
- if func_name != "<Unknown>":
- # Check if we are still in the previous function
- if add_cycles_to_function.prev_func_name == func_name:
- add_cycles_to_function.prev_entry['cycles'] += cycles
- return (add_cycles_to_function.prev_func_name, add_cycles_to_function.prev_module_name)
-
- if func_name in functions.keys():
- for module_name, module_value in functions[func_name].iteritems():
- if (module_value['start'] <= addr) and (addr < module_value['end']):
- module_value['cycles'] += cycles
-
- add_cycles_to_function.prev_func_name = func_name
- add_cycles_to_function.prev_module_name = module_name
- add_cycles_to_function.prev_entry = module_value
- return (func_name, module_name)
- elif (module_value['end'] == 0):
- module_value['cycles'] += cycles
-
- add_cycles_to_function.prev_func_name = func_name
- add_cycles_to_function.prev_module_name = module_name
- add_cycles_to_function.prev_entry = module_value
- return (func_name, module_name)
-
- # Workaround to fix the 'info func' limitation that does not expose the 'static' function
- module_name = get_module_from_addr(modules, addr)
- functions[func_name] = {}
- functions[func_name][module_name] = {}
- functions[func_name][module_name]['start'] = 0
- functions[func_name][module_name]['end'] = 0
- functions[func_name][module_name]['cycles'] = cycles
- functions[func_name][module_name]['count'] = 0
-
- add_cycles_to_function.prev_func_name = func_name
- add_cycles_to_function.prev_module_name = module_name
- add_cycles_to_function.prev_entry = functions[func_name][module_name]
- return (func_name, module_name)
- else:
- # Check if we are still in the previous function
- if (add_cycles_to_function.prev_entry is not None) and (add_cycles_to_function.prev_entry['start'] <= addr) and (addr < add_cycles_to_function.prev_entry['end']):
- add_cycles_to_function.prev_entry['cycles'] += cycles
- return (add_cycles_to_function.prev_func_name, add_cycles_to_function.prev_module_name)
-
- # Generate the key for the given address
- key = addr & ~0x0FFF
-
- if key not in functions_addr.keys():
- if 'Unknown' not in functions.keys():
- functions['Unknown'] = {}
- if 'Unknown' not in functions['Unknown'].keys():
- functions['Unknown']['Unknown'] = {}
- functions['Unknown']['Unknown']['cycles'] = 0
- functions['Unknown']['Unknown']['count'] = 0
- functions['Unknown']['Unknown']['cycles'] += cycles
-
- add_cycles_to_function.prev_func_name = None
- return None
-
- for func_key, module in functions_addr[key].iteritems():
- for module_key, module_value in module.iteritems():
- if (module_value['start'] <= addr) and (addr < module_value['end']):
- module_value['cycles'] += cycles
-
- # In case o <Unknown> we prefer to fallback on the direct search
- add_cycles_to_function.prev_func_name = func_key
- add_cycles_to_function.prev_module_name = module_key
- add_cycles_to_function.prev_entry = module_value
- return (func_key, module_key)
-
- print "Warning: Function %s @ 0x%x not found" % (func_name, addr)
-
- add_cycles_to_function.prev_func_name = None
- return None
-
-# Static variables for the previous function
-add_cycles_to_function.prev_func_name = None
-add_cycles_to_function.prev_entry = None
-
-def trace_read():
- global trace_process
- line = trace.readline()
- trace_process += len(line)
- return line
-
-#
-# Parse arguments
-#
-trace_name = None
-symbols_file = None
-
-opts,args = getopt.getopt(sys.argv[1:], "ht:vs:v", ["help","trace=","symbols="])
-if (opts is None) or (not opts):
- usage()
- sys.exit()
-
-for o,a in opts:
- if o in ("-h","--help"):
- usage()
- sys.exit()
- elif o in ("-t","--trace"):
- trace_name = a
- elif o in ("-s","--symbols"):
- symbols_file = a
- else:
- assert False, "Unhandled option (%s)" % o
-
-#
-# We try first to see if we run the script from DS-5
-#
-try:
- from arm_ds.debugger_v1 import Debugger
- from arm_ds.debugger_v1 import DebugException
-
- # Debugger object for accessing the debugger
- debugger = Debugger()
-
- # Initialisation commands
- ec = debugger.getExecutionContext(0)
- ec.getExecutionService().stop()
- ec.getExecutionService().waitForStop()
- # in case the execution context reference is out of date
- ec = debugger.getExecutionContext(0)
-
- #
- # Get the module name and their memory range
- #
- info_file = ec.executeDSCommand("info file")
- info_file_str = StringIO(info_file)
-
- line = info_file_str.readline().strip('\n')
- while line != '':
- if ("Symbols from" in line):
- # Get the module name from the line 'Symbols from "/home/...."'
- module_name = line.split("\"")[1].split("/")[-1]
- modules[module_name] = {}
-
- # Look for the text section
- line = info_file_str.readline().strip('\n')
- while (line != '') and ("Symbols from" not in line):
- if ("ER_RO" in line):
- modules[module_name]['start'] = get_address_from_string(line.split()[0])
- modules[module_name]['end'] = get_address_from_string(line.split()[2])
- line = info_file_str.readline().strip('\n')
- break;
- if (".text" in line):
- modules[module_name]['start'] = get_address_from_string(line.split()[0])
- modules[module_name]['end'] = get_address_from_string(line.split()[2])
- line = info_file_str.readline().strip('\n')
- break;
- line = info_file_str.readline().strip('\n')
- line = info_file_str.readline().strip('\n')
-
- #
- # Get the function name and their memory range
- #
- info_func = ec.executeDSCommand("info func")
- info_func_str = StringIO(info_func)
-
- # Skip the first line 'Low-level symbols ...'
- line = info_func_str.readline().strip('\n')
- func_prev = None
- while line != '':
- # We ignore all the functions after 'Functions in'
- if ("Functions in " in line):
- line = info_func_str.readline().strip('\n')
- while line != '':
- line = info_func_str.readline().strip('\n')
- line = info_func_str.readline().strip('\n')
- continue
-
- if ("Low-level symbols" in line):
- # We need to fixup the last function of the module
- if func_prev is not None:
- func_prev['end'] = modules[module_name]['end']
- func_prev = None
-
- line = info_func_str.readline().strip('\n')
- continue
-
- func_name = line.split()[1]
- func_start = get_address_from_string(line.split()[0])
- module_name = get_module_from_addr(modules, func_start)
-
- if func_name not in functions.keys():
- functions[func_name] = {}
- functions[func_name][module_name] = {}
- functions[func_name][module_name]['start'] = func_start
- functions[func_name][module_name]['cycles'] = 0
- functions[func_name][module_name]['count'] = 0
-
- # Set the end address of the previous function
- if func_prev is not None:
- func_prev['end'] = func_start
- func_prev = functions[func_name][module_name]
-
- line = info_func_str.readline().strip('\n')
-
- # Fixup the last function
- func_prev['end'] = modules[module_name]['end']
-
- if symbols_file is not None:
- pickle.dump((modules, functions), open(symbols_file, "w"))
-except:
- if symbols_file is None:
- print "Error: Symbols file is required when run out of ARM DS-5"
- sys.exit()
-
- (modules, functions) = pickle.load(open(symbols_file, "r"))
-
-#
-# Build optimized table for the <Unknown> functions
-#
-functions_addr = {}
-for func_key, module in functions.iteritems():
- for module_key, module_value in module.iteritems():
- key = module_value['start'] & ~0x0FFF
- if key not in functions_addr.keys():
- functions_addr[key] = {}
- if func_key not in functions_addr[key].keys():
- functions_addr[key][func_key] = {}
- functions_addr[key][func_key][module_key] = module_value
-
-#
-# Process the trace file
-#
-if trace_name is None:
- sys.exit()
-
-trace = open(trace_name, "r")
-trace_size = os.path.getsize(trace_name)
-trace_process = 0
-
-# Get the column names from the first line
-columns = trace_read().split()
-column_addr = columns.index('Address')
-column_cycles = columns.index('Cycles')
-column_function = columns.index('Function')
-
-line = trace_read()
-i = 0
-prev_callee = None
-while line:
- try:
- func_name = line.split('\t')[column_function].strip()
- address = get_address_from_string(line.split('\t')[column_addr])
- cycles = int(line.split('\t')[column_cycles])
- callee = add_cycles_to_function(functions, func_name, address, cycles)
- if (prev_callee != None) and (prev_callee != callee):
- functions[prev_callee[0]][prev_callee[1]]['count'] += 1
- prev_callee = callee
- except ValueError:
- pass
- line = trace_read()
- if ((i % 1000000) == 0) and (i != 0):
- percent = (trace_process * 100.00) / trace_size
- print "Processing file ... (%.2f %%)" % (percent)
- i = i + 1
-
-# Fixup the last callee
-functions[prev_callee[0]][prev_callee[1]]['count'] += 1
-
-#
-# Process results
-#
-functions_cycles = {}
-all_functions_cycles = {}
-total_cycles = 0
-
-for func_key, module in functions.iteritems():
- for module_key, module_value in module.iteritems():
- key = "%s/%s" % (module_key, func_key)
- functions_cycles[key] = (module_value['cycles'], module_value['count'])
- total_cycles += module_value['cycles']
-
- if func_key not in all_functions_cycles.keys():
- all_functions_cycles[func_key] = (module_value['cycles'], module_value['count'])
- else:
- all_functions_cycles[func_key] = tuple(map(sum, zip(all_functions_cycles[func_key], (module_value['cycles'], module_value['count']))))
-
-sorted_functions_cycles = sorted(functions_cycles.iteritems(), key=operator.itemgetter(1), reverse = True)
-sorted_all_functions_cycles = sorted(all_functions_cycles.items(), key=operator.itemgetter(1), reverse = True)
-
-print
-print "----"
-for (key,value) in sorted_functions_cycles[:20]:
- if value[0] != 0:
- print "%s (cycles: %d - %d%%, count: %d)" % (key, value[0], (value[0] * 100) / total_cycles, value[1])
- else:
- break;
-print "----"
-for (key,value) in sorted_all_functions_cycles[:20]:
- if value[0] != 0:
- print "%s (cycles: %d - %d%%, count: %d)" % (key, value[0], (value[0] * 100) / total_cycles, value[1])
- else:
- break;
+#!/usr/bin/python
+
+#
+# Copyright (c) 2014, ARM Limited. All rights reserved.
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+
+import getopt
+import operator
+import os
+import pickle
+import sys
+from sys import argv
+from cStringIO import StringIO
+
+modules = {}
+functions = {}
+functions_addr = {}
+
+def usage():
+ print "-t,--trace: Location of the Trace file"
+ print "-s,--symbols: Location of the symbols and modules"
+
+def get_address_from_string(address):
+ return int(address.strip("S:").strip("N:").strip("EL2:").strip("EL1:"), 16)
+
+def get_module_from_addr(modules, addr):
+ for key,value in modules.items():
+ if (value['start'] <= addr) and (addr <= value['end']):
+ return key
+ return None
+
+def add_cycles_to_function(functions, func_name, addr, cycles):
+ if func_name != "<Unknown>":
+ # Check if we are still in the previous function
+ if add_cycles_to_function.prev_func_name == func_name:
+ add_cycles_to_function.prev_entry['cycles'] += cycles
+ return (add_cycles_to_function.prev_func_name, add_cycles_to_function.prev_module_name)
+
+ if func_name in functions.keys():
+ for module_name, module_value in functions[func_name].iteritems():
+ if (module_value['start'] <= addr) and (addr < module_value['end']):
+ module_value['cycles'] += cycles
+
+ add_cycles_to_function.prev_func_name = func_name
+ add_cycles_to_function.prev_module_name = module_name
+ add_cycles_to_function.prev_entry = module_value
+ return (func_name, module_name)
+ elif (module_value['end'] == 0):
+ module_value['cycles'] += cycles
+
+ add_cycles_to_function.prev_func_name = func_name
+ add_cycles_to_function.prev_module_name = module_name
+ add_cycles_to_function.prev_entry = module_value
+ return (func_name, module_name)
+
+ # Workaround to fix the 'info func' limitation that does not expose the 'static' function
+ module_name = get_module_from_addr(modules, addr)
+ functions[func_name] = {}
+ functions[func_name][module_name] = {}
+ functions[func_name][module_name]['start'] = 0
+ functions[func_name][module_name]['end'] = 0
+ functions[func_name][module_name]['cycles'] = cycles
+ functions[func_name][module_name]['count'] = 0
+
+ add_cycles_to_function.prev_func_name = func_name
+ add_cycles_to_function.prev_module_name = module_name
+ add_cycles_to_function.prev_entry = functions[func_name][module_name]
+ return (func_name, module_name)
+ else:
+ # Check if we are still in the previous function
+ if (add_cycles_to_function.prev_entry is not None) and (add_cycles_to_function.prev_entry['start'] <= addr) and (addr < add_cycles_to_function.prev_entry['end']):
+ add_cycles_to_function.prev_entry['cycles'] += cycles
+ return (add_cycles_to_function.prev_func_name, add_cycles_to_function.prev_module_name)
+
+ # Generate the key for the given address
+ key = addr & ~0x0FFF
+
+ if key not in functions_addr.keys():
+ if 'Unknown' not in functions.keys():
+ functions['Unknown'] = {}
+ if 'Unknown' not in functions['Unknown'].keys():
+ functions['Unknown']['Unknown'] = {}
+ functions['Unknown']['Unknown']['cycles'] = 0
+ functions['Unknown']['Unknown']['count'] = 0
+ functions['Unknown']['Unknown']['cycles'] += cycles
+
+ add_cycles_to_function.prev_func_name = None
+ return None
+
+ for func_key, module in functions_addr[key].iteritems():
+ for module_key, module_value in module.iteritems():
+ if (module_value['start'] <= addr) and (addr < module_value['end']):
+ module_value['cycles'] += cycles
+
+ # In case o <Unknown> we prefer to fallback on the direct search
+ add_cycles_to_function.prev_func_name = func_key
+ add_cycles_to_function.prev_module_name = module_key
+ add_cycles_to_function.prev_entry = module_value
+ return (func_key, module_key)
+
+ print "Warning: Function %s @ 0x%x not found" % (func_name, addr)
+
+ add_cycles_to_function.prev_func_name = None
+ return None
+
+# Static variables for the previous function
+add_cycles_to_function.prev_func_name = None
+add_cycles_to_function.prev_entry = None
+
+def trace_read():
+ global trace_process
+ line = trace.readline()
+ trace_process += len(line)
+ return line
+
+#
+# Parse arguments
+#
+trace_name = None
+symbols_file = None
+
+opts,args = getopt.getopt(sys.argv[1:], "ht:vs:v", ["help","trace=","symbols="])
+if (opts is None) or (not opts):
+ usage()
+ sys.exit()
+
+for o,a in opts:
+ if o in ("-h","--help"):
+ usage()
+ sys.exit()
+ elif o in ("-t","--trace"):
+ trace_name = a
+ elif o in ("-s","--symbols"):
+ symbols_file = a
+ else:
+ assert False, "Unhandled option (%s)" % o
+
+#
+# We try first to see if we run the script from DS-5
+#
+try:
+ from arm_ds.debugger_v1 import Debugger
+ from arm_ds.debugger_v1 import DebugException
+
+ # Debugger object for accessing the debugger
+ debugger = Debugger()
+
+ # Initialisation commands
+ ec = debugger.getExecutionContext(0)
+ ec.getExecutionService().stop()
+ ec.getExecutionService().waitForStop()
+ # in case the execution context reference is out of date
+ ec = debugger.getExecutionContext(0)
+
+ #
+ # Get the module name and their memory range
+ #
+ info_file = ec.executeDSCommand("info file")
+ info_file_str = StringIO(info_file)
+
+ line = info_file_str.readline().strip('\n')
+ while line != '':
+ if ("Symbols from" in line):
+ # Get the module name from the line 'Symbols from "/home/...."'
+ module_name = line.split("\"")[1].split("/")[-1]
+ modules[module_name] = {}
+
+ # Look for the text section
+ line = info_file_str.readline().strip('\n')
+ while (line != '') and ("Symbols from" not in line):
+ if ("ER_RO" in line):
+ modules[module_name]['start'] = get_address_from_string(line.split()[0])
+ modules[module_name]['end'] = get_address_from_string(line.split()[2])
+ line = info_file_str.readline().strip('\n')
+ break;
+ if (".text" in line):
+ modules[module_name]['start'] = get_address_from_string(line.split()[0])
+ modules[module_name]['end'] = get_address_from_string(line.split()[2])
+ line = info_file_str.readline().strip('\n')
+ break;
+ line = info_file_str.readline().strip('\n')
+ line = info_file_str.readline().strip('\n')
+
+ #
+ # Get the function name and their memory range
+ #
+ info_func = ec.executeDSCommand("info func")
+ info_func_str = StringIO(info_func)
+
+ # Skip the first line 'Low-level symbols ...'
+ line = info_func_str.readline().strip('\n')
+ func_prev = None
+ while line != '':
+ # We ignore all the functions after 'Functions in'
+ if ("Functions in " in line):
+ line = info_func_str.readline().strip('\n')
+ while line != '':
+ line = info_func_str.readline().strip('\n')
+ line = info_func_str.readline().strip('\n')
+ continue
+
+ if ("Low-level symbols" in line):
+ # We need to fixup the last function of the module
+ if func_prev is not None:
+ func_prev['end'] = modules[module_name]['end']
+ func_prev = None
+
+ line = info_func_str.readline().strip('\n')
+ continue
+
+ func_name = line.split()[1]
+ func_start = get_address_from_string(line.split()[0])
+ module_name = get_module_from_addr(modules, func_start)
+
+ if func_name not in functions.keys():
+ functions[func_name] = {}
+ functions[func_name][module_name] = {}
+ functions[func_name][module_name]['start'] = func_start
+ functions[func_name][module_name]['cycles'] = 0
+ functions[func_name][module_name]['count'] = 0
+
+ # Set the end address of the previous function
+ if func_prev is not None:
+ func_prev['end'] = func_start
+ func_prev = functions[func_name][module_name]
+
+ line = info_func_str.readline().strip('\n')
+
+ # Fixup the last function
+ func_prev['end'] = modules[module_name]['end']
+
+ if symbols_file is not None:
+ pickle.dump((modules, functions), open(symbols_file, "w"))
+except:
+ if symbols_file is None:
+ print "Error: Symbols file is required when run out of ARM DS-5"
+ sys.exit()
+
+ (modules, functions) = pickle.load(open(symbols_file, "r"))
+
+#
+# Build optimized table for the <Unknown> functions
+#
+functions_addr = {}
+for func_key, module in functions.iteritems():
+ for module_key, module_value in module.iteritems():
+ key = module_value['start'] & ~0x0FFF
+ if key not in functions_addr.keys():
+ functions_addr[key] = {}
+ if func_key not in functions_addr[key].keys():
+ functions_addr[key][func_key] = {}
+ functions_addr[key][func_key][module_key] = module_value
+
+#
+# Process the trace file
+#
+if trace_name is None:
+ sys.exit()
+
+trace = open(trace_name, "r")
+trace_size = os.path.getsize(trace_name)
+trace_process = 0
+
+# Get the column names from the first line
+columns = trace_read().split()
+column_addr = columns.index('Address')
+column_cycles = columns.index('Cycles')
+column_function = columns.index('Function')
+
+line = trace_read()
+i = 0
+prev_callee = None
+while line:
+ try:
+ func_name = line.split('\t')[column_function].strip()
+ address = get_address_from_string(line.split('\t')[column_addr])
+ cycles = int(line.split('\t')[column_cycles])
+ callee = add_cycles_to_function(functions, func_name, address, cycles)
+ if (prev_callee != None) and (prev_callee != callee):
+ functions[prev_callee[0]][prev_callee[1]]['count'] += 1
+ prev_callee = callee
+ except ValueError:
+ pass
+ line = trace_read()
+ if ((i % 1000000) == 0) and (i != 0):
+ percent = (trace_process * 100.00) / trace_size
+ print "Processing file ... (%.2f %%)" % (percent)
+ i = i + 1
+
+# Fixup the last callee
+functions[prev_callee[0]][prev_callee[1]]['count'] += 1
+
+#
+# Process results
+#
+functions_cycles = {}
+all_functions_cycles = {}
+total_cycles = 0
+
+for func_key, module in functions.iteritems():
+ for module_key, module_value in module.iteritems():
+ key = "%s/%s" % (module_key, func_key)
+ functions_cycles[key] = (module_value['cycles'], module_value['count'])
+ total_cycles += module_value['cycles']
+
+ if func_key not in all_functions_cycles.keys():
+ all_functions_cycles[func_key] = (module_value['cycles'], module_value['count'])
+ else:
+ all_functions_cycles[func_key] = tuple(map(sum, zip(all_functions_cycles[func_key], (module_value['cycles'], module_value['count']))))
+
+sorted_functions_cycles = sorted(functions_cycles.iteritems(), key=operator.itemgetter(1), reverse = True)
+sorted_all_functions_cycles = sorted(all_functions_cycles.items(), key=operator.itemgetter(1), reverse = True)
+
+print
+print "----"
+for (key,value) in sorted_functions_cycles[:20]:
+ if value[0] != 0:
+ print "%s (cycles: %d - %d%%, count: %d)" % (key, value[0], (value[0] * 100) / total_cycles, value[1])
+ else:
+ break;
+print "----"
+for (key,value) in sorted_all_functions_cycles[:20]:
+ if value[0] != 0:
+ print "%s (cycles: %d - %d%%, count: %d)" % (key, value[0], (value[0] * 100) / total_cycles, value[1])
+ else:
+ break;
--
2.19.1.3.g30247aa5d201


[PATCH 2/3] ArmVirtPkg: convert LFs to CRLF

Laszlo Ersek
 

We're going to switch the internal line terminators globally to LF at some
point, but until then, let's use CRLF consistently. Convert source files
with LFs in them to CRLF, using "unix2dos".

"git show -b" prints no code changes for this patch.

(I collected all the file name suffixes in this package, with:

$ git ls-files -- $PACKAGE | rev | cut -f 1 -d . | sort -u | rev

I eliminated those suffixes that didn't stand for text files, then
blanket-converted the rest with unix2dos. Finally, picked up the actual
changes with git-add.)

The CRLF conversion is motivated by "PatchCheck.py".

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1659
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Include/Platform/Hidden.h | 44 ++++++++++----------
1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/ArmVirtPkg/Include/Platform/Hidden.h b/ArmVirtPkg/Include/Platform/Hidden.h
index c69cc662fa62..7a7bdb42b8bd 100644
--- a/ArmVirtPkg/Include/Platform/Hidden.h
+++ b/ArmVirtPkg/Include/Platform/Hidden.h
@@ -1,22 +1,22 @@
-/** @file
-
- Copyright (c) 2018, Linaro Limited. All rights reserved.
-
- SPDX-License-Identifier: BSD-2-Clause-Patent
-
-**/
-
-#ifndef __PLATFORM_HIDDEN_H
-#define __PLATFORM_HIDDEN_H
-
-//
-// Setting the GCC -fvisibility=hidden command line option is not quite the same
-// as setting the pragma below: the former only affects definitions, whereas the
-// pragma affects extern declarations as well. So if we want to ensure that no
-// GOT indirected symbol references are emitted, we need to use the pragma, or
-// GOT based cross object references could be emitted, e.g., in libraries, and
-// these cannot be relaxed to ordinary symbol references at link time.
-//
-#pragma GCC visibility push (hidden)
-
-#endif
+/** @file
+
+ Copyright (c) 2018, Linaro Limited. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PLATFORM_HIDDEN_H
+#define __PLATFORM_HIDDEN_H
+
+//
+// Setting the GCC -fvisibility=hidden command line option is not quite the same
+// as setting the pragma below: the former only affects definitions, whereas the
+// pragma affects extern declarations as well. So if we want to ensure that no
+// GOT indirected symbol references are emitted, we need to use the pragma, or
+// GOT based cross object references could be emitted, e.g., in libraries, and
+// these cannot be relaxed to ordinary symbol references at link time.
+//
+#pragma GCC visibility push (hidden)
+
+#endif
--
2.19.1.3.g30247aa5d201


[PATCH 0/3] Arm*Pkg: convert LFs to CRLF, expand hard TABs

Laszlo Ersek
 

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1659
Repo: https://github.com/lersek/edk2.git
Branch: crlf_bz_1659

Future patches for some Arm*Pkg source files would either introduce CRLF
lines into LF files, or add more LF lines. The first would increase
inconsistency, the second would make "PatchCheck.py" yell. Convert these
files to CRLF uniformly (and while at it, expand hard TABS, because
"PatchCheck.py" catches those too).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks,
Laszlo

Laszlo Ersek (3):
ArmPkg: convert LFs to CRLF, expand hard TABs
ArmVirtPkg: convert LFs to CRLF
ArmPlatformPkg: convert LFs to CRLF, expand hard TABs

ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c | 32 +-
ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c | 566 ++++++++---------
ArmPkg/Library/CompilerIntrinsicsLib/memset.c | 110 ++--
ArmPkg/Library/GccLto/liblto-aarch64.s | 42 +-
ArmPkg/Library/GccLto/liblto-arm.s | 110 ++--
ArmPlatformPkg/Scripts/Ds5/profile.py | 656 ++++++++++----------
ArmVirtPkg/Include/Platform/Hidden.h | 44 +-
7 files changed, 780 insertions(+), 780 deletions(-)

--
2.19.1.3.g30247aa5d201


[PATCH 1/3] ArmPkg: convert LFs to CRLF, expand hard TABs

Laszlo Ersek
 

We're going to switch the internal line terminators globally to LF at some
point, but until then, let's use CRLF consistently. Convert source files
with LFs in them to CRLF, using "unix2dos".

"git show -b" prints no code changes for this patch.

(I collected all the file name suffixes in this package, with:

$ git ls-files -- $PACKAGE | rev | cut -f 1 -d . | sort -u | rev

I eliminated those suffixes that didn't stand for text files, then
blanket-converted the rest with unix2dos. Finally, picked up the actual
changes with git-add.)

At the same time, the following three files had to undergo TAB expansion:

ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c
ArmPkg/Library/GccLto/liblto-aarch64.s
ArmPkg/Library/GccLto/liblto-arm.s

I used "expand -t 2", in order to stay close to the edk2 coding style
(which uses two spaces for indentation.)

Both the CRLF conversion and the TAB expansion are motivated by
"PatchCheck.py". "PatchCheck.py" is also the reason why CRLF conversion
and TAB expansion have to happen in the same patch.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1659
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c | 32 +-
ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c | 566 ++++++++++----------
ArmPkg/Library/CompilerIntrinsicsLib/memset.c | 110 ++--
ArmPkg/Library/GccLto/liblto-aarch64.s | 42 +-
ArmPkg/Library/GccLto/liblto-arm.s | 110 ++--
5 files changed, 430 insertions(+), 430 deletions(-)

diff --git a/ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c b/ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c
index 2cd292aabf88..2d79aadaf1fa 100644
--- a/ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c
+++ b/ArmPkg/Library/ArmSmcLibNull/ArmSmcLibNull.c
@@ -1,16 +1,16 @@
-//
-// Copyright (c) 2016, Linaro Limited. All rights reserved.
-//
-// SPDX-License-Identifier: BSD-2-Clause-Patent
-//
-//
-
-#include <Base.h>
-#include <Library/ArmSmcLib.h>
-
-VOID
-ArmCallSmc (
- IN OUT ARM_SMC_ARGS *Args
- )
-{
-}
+//
+// Copyright (c) 2016, Linaro Limited. All rights reserved.
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//
+
+#include <Base.h>
+#include <Library/ArmSmcLib.h>
+
+VOID
+ArmCallSmc (
+ IN OUT ARM_SMC_ARGS *Args
+ )
+{
+}
diff --git a/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c b/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c
index 42bed7700c99..77e2473678a2 100644
--- a/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c
+++ b/ArmPkg/Library/ArmSoftFloatLib/ArmSoftFloatLib.c
@@ -1,283 +1,283 @@
-/*
- * Copyright (c) 2015 - 2019, Linaro Limited
- *
- * SPDX-License-Identifier: BSD-2-Clause-Patent
- */
-
-#include "platform.h"
-#include <softfloat.h>
-
-/*
- * On ARM32 EABI defines both a soft-float ABI and a hard-float ABI,
- * hard-float is basically a super set of soft-float. Hard-float requires
- * all the support routines provided for soft-float, but the compiler may
- * choose to optimize to not use some of them.
- *
- * The AEABI functions uses soft-float calling convention even if the
- * functions are compiled for hard-float. So where float and double would
- * have been expected we use aeabi_float_t and aeabi_double_t respectively
- * instead.
- */
-typedef uint32_t aeabi_float_t;
-typedef uint64_t aeabi_double_t;
-
-/*
- * Helpers to convert between float32 and aeabi_float_t, and float64 and
- * aeabi_double_t used by the AEABI functions below.
- */
-static aeabi_float_t f32_to_f(float32_t val)
-{
- return val.v;
-}
-
-static float32_t f32_from_f(aeabi_float_t val)
-{
- float32_t res;
-
- res.v = val;
-
- return res;
-}
-
-static aeabi_double_t f64_to_d(float64_t val)
-{
- return val.v;
-}
-
-static float64_t f64_from_d(aeabi_double_t val)
-{
- float64_t res;
-
- res.v = val;
-
- return res;
-}
-
-/*
- * From ARM Run-time ABI for ARM Architecture
- * ARM IHI 0043D, current through ABI release 2.09
- *
- * 4.1.2 The floating-point helper functions
- */
-
-/*
- * Table 2, Standard aeabi_double_t precision floating-point arithmetic helper
- * functions
- */
-
-aeabi_double_t __aeabi_dadd(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_to_d(f64_add(f64_from_d(a), f64_from_d(b)));
-}
-
-aeabi_double_t __aeabi_ddiv(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_to_d(f64_div(f64_from_d(a), f64_from_d(b)));
-}
-
-aeabi_double_t __aeabi_dmul(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_to_d(f64_mul(f64_from_d(a), f64_from_d(b)));
-}
-
-
-aeabi_double_t __aeabi_drsub(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_to_d(f64_sub(f64_from_d(b), f64_from_d(a)));
-}
-
-aeabi_double_t __aeabi_dsub(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_to_d(f64_sub(f64_from_d(a), f64_from_d(b)));
-}
-
-/*
- * Table 3, double precision floating-point comparison helper functions
- */
-
-int __aeabi_dcmpeq(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_eq(f64_from_d(a), f64_from_d(b));
-}
-
-int __aeabi_dcmplt(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_lt(f64_from_d(a), f64_from_d(b));
-}
-
-int __aeabi_dcmple(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_le(f64_from_d(a), f64_from_d(b));
-}
-
-int __aeabi_dcmpge(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_le(f64_from_d(b), f64_from_d(a));
-}
-
-int __aeabi_dcmpgt(aeabi_double_t a, aeabi_double_t b)
-{
- return f64_lt(f64_from_d(b), f64_from_d(a));
-}
-
-/*
- * Table 4, Standard single precision floating-point arithmetic helper
- * functions
- */
-
-aeabi_float_t __aeabi_fadd(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_to_f(f32_add(f32_from_f(a), f32_from_f(b)));
-}
-
-aeabi_float_t __aeabi_fdiv(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_to_f(f32_div(f32_from_f(a), f32_from_f(b)));
-}
-
-aeabi_float_t __aeabi_fmul(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_to_f(f32_mul(f32_from_f(a), f32_from_f(b)));
-}
-
-aeabi_float_t __aeabi_frsub(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_to_f(f32_sub(f32_from_f(b), f32_from_f(a)));
-}
-
-aeabi_float_t __aeabi_fsub(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_to_f(f32_sub(f32_from_f(a), f32_from_f(b)));
-}
-
-/*
- * Table 5, Standard single precision floating-point comparison helper
- * functions
- */
-
-int __aeabi_fcmpeq(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_eq(f32_from_f(a), f32_from_f(b));
-}
-
-int __aeabi_fcmplt(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_lt(f32_from_f(a), f32_from_f(b));
-}
-
-int __aeabi_fcmple(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_le(f32_from_f(a), f32_from_f(b));
-}
-
-int __aeabi_fcmpge(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_le(f32_from_f(b), f32_from_f(a));
-}
-
-int __aeabi_fcmpgt(aeabi_float_t a, aeabi_float_t b)
-{
- return f32_lt(f32_from_f(b), f32_from_f(a));
-}
-
-/*
- * Table 6, Standard floating-point to integer conversions
- */
-
-int __aeabi_d2iz(aeabi_double_t a)
-{
- return f64_to_i32_r_minMag(f64_from_d(a), false);
-}
-
-unsigned __aeabi_d2uiz(aeabi_double_t a)
-{
- return f64_to_ui32_r_minMag(f64_from_d(a), false);
-}
-
-long long __aeabi_d2lz(aeabi_double_t a)
-{
- return f64_to_i64_r_minMag(f64_from_d(a), false);
-}
-
-unsigned long long __aeabi_d2ulz(aeabi_double_t a)
-{
- return f64_to_ui64_r_minMag(f64_from_d(a), false);
-}
-
-int __aeabi_f2iz(aeabi_float_t a)
-{
- return f32_to_i32_r_minMag(f32_from_f(a), false);
-}
-
-unsigned __aeabi_f2uiz(aeabi_float_t a)
-{
- return f32_to_ui32_r_minMag(f32_from_f(a), false);
-}
-
-long long __aeabi_f2lz(aeabi_float_t a)
-{
- return f32_to_i64_r_minMag(f32_from_f(a), false);
-}
-
-unsigned long long __aeabi_f2ulz(aeabi_float_t a)
-{
- return f32_to_ui64_r_minMag(f32_from_f(a), false);
-}
-
-/*
- * Table 7, Standard conversions between floating types
- */
-
-aeabi_float_t __aeabi_d2f(aeabi_double_t a)
-{
- return f32_to_f(f64_to_f32(f64_from_d(a)));
-}
-
-aeabi_double_t __aeabi_f2d(aeabi_float_t a)
-{
- return f64_to_d(f32_to_f64(f32_from_f(a)));
-}
-
-/*
- * Table 8, Standard integer to floating-point conversions
- */
-
-aeabi_double_t __aeabi_i2d(int a)
-{
- return f64_to_d(i32_to_f64(a));
-}
-
-aeabi_double_t __aeabi_ui2d(unsigned a)
-{
- return f64_to_d(ui32_to_f64(a));
-}
-
-aeabi_double_t __aeabi_l2d(long long a)
-{
- return f64_to_d(i64_to_f64(a));
-}
-
-aeabi_double_t __aeabi_ul2d(unsigned long long a)
-{
- return f64_to_d(ui64_to_f64(a));
-}
-
-aeabi_float_t __aeabi_i2f(int a)
-{
- return f32_to_f(i32_to_f32(a));
-}
-
-aeabi_float_t __aeabi_ui2f(unsigned a)
-{
- return f32_to_f(ui32_to_f32(a));
-}
-
-aeabi_float_t __aeabi_l2f(long long a)
-{
- return f32_to_f(i64_to_f32(a));
-}
-
-aeabi_float_t __aeabi_ul2f(unsigned long long a)
-{
- return f32_to_f(ui64_to_f32(a));
-}
+/*
+ * Copyright (c) 2015 - 2019, Linaro Limited
+ *
+ * SPDX-License-Identifier: BSD-2-Clause-Patent
+ */
+
+#include "platform.h"
+#include <softfloat.h>
+
+/*
+ * On ARM32 EABI defines both a soft-float ABI and a hard-float ABI,
+ * hard-float is basically a super set of soft-float. Hard-float requires
+ * all the support routines provided for soft-float, but the compiler may
+ * choose to optimize to not use some of them.
+ *
+ * The AEABI functions uses soft-float calling convention even if the
+ * functions are compiled for hard-float. So where float and double would
+ * have been expected we use aeabi_float_t and aeabi_double_t respectively
+ * instead.
+ */
+typedef uint32_t aeabi_float_t;
+typedef uint64_t aeabi_double_t;
+
+/*
+ * Helpers to convert between float32 and aeabi_float_t, and float64 and
+ * aeabi_double_t used by the AEABI functions below.
+ */
+static aeabi_float_t f32_to_f(float32_t val)
+{
+ return val.v;
+}
+
+static float32_t f32_from_f(aeabi_float_t val)
+{
+ float32_t res;
+
+ res.v = val;
+
+ return res;
+}
+
+static aeabi_double_t f64_to_d(float64_t val)
+{
+ return val.v;
+}
+
+static float64_t f64_from_d(aeabi_double_t val)
+{
+ float64_t res;
+
+ res.v = val;
+
+ return res;
+}
+
+/*
+ * From ARM Run-time ABI for ARM Architecture
+ * ARM IHI 0043D, current through ABI release 2.09
+ *
+ * 4.1.2 The floating-point helper functions
+ */
+
+/*
+ * Table 2, Standard aeabi_double_t precision floating-point arithmetic helper
+ * functions
+ */
+
+aeabi_double_t __aeabi_dadd(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_to_d(f64_add(f64_from_d(a), f64_from_d(b)));
+}
+
+aeabi_double_t __aeabi_ddiv(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_to_d(f64_div(f64_from_d(a), f64_from_d(b)));
+}
+
+aeabi_double_t __aeabi_dmul(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_to_d(f64_mul(f64_from_d(a), f64_from_d(b)));
+}
+
+
+aeabi_double_t __aeabi_drsub(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_to_d(f64_sub(f64_from_d(b), f64_from_d(a)));
+}
+
+aeabi_double_t __aeabi_dsub(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_to_d(f64_sub(f64_from_d(a), f64_from_d(b)));
+}
+
+/*
+ * Table 3, double precision floating-point comparison helper functions
+ */
+
+int __aeabi_dcmpeq(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_eq(f64_from_d(a), f64_from_d(b));
+}
+
+int __aeabi_dcmplt(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_lt(f64_from_d(a), f64_from_d(b));
+}
+
+int __aeabi_dcmple(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_le(f64_from_d(a), f64_from_d(b));
+}
+
+int __aeabi_dcmpge(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_le(f64_from_d(b), f64_from_d(a));
+}
+
+int __aeabi_dcmpgt(aeabi_double_t a, aeabi_double_t b)
+{
+ return f64_lt(f64_from_d(b), f64_from_d(a));
+}
+
+/*
+ * Table 4, Standard single precision floating-point arithmetic helper
+ * functions
+ */
+
+aeabi_float_t __aeabi_fadd(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_to_f(f32_add(f32_from_f(a), f32_from_f(b)));
+}
+
+aeabi_float_t __aeabi_fdiv(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_to_f(f32_div(f32_from_f(a), f32_from_f(b)));
+}
+
+aeabi_float_t __aeabi_fmul(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_to_f(f32_mul(f32_from_f(a), f32_from_f(b)));
+}
+
+aeabi_float_t __aeabi_frsub(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_to_f(f32_sub(f32_from_f(b), f32_from_f(a)));
+}
+
+aeabi_float_t __aeabi_fsub(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_to_f(f32_sub(f32_from_f(a), f32_from_f(b)));
+}
+
+/*
+ * Table 5, Standard single precision floating-point comparison helper
+ * functions
+ */
+
+int __aeabi_fcmpeq(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_eq(f32_from_f(a), f32_from_f(b));
+}
+
+int __aeabi_fcmplt(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_lt(f32_from_f(a), f32_from_f(b));
+}
+
+int __aeabi_fcmple(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_le(f32_from_f(a), f32_from_f(b));
+}
+
+int __aeabi_fcmpge(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_le(f32_from_f(b), f32_from_f(a));
+}
+
+int __aeabi_fcmpgt(aeabi_float_t a, aeabi_float_t b)
+{
+ return f32_lt(f32_from_f(b), f32_from_f(a));
+}
+
+/*
+ * Table 6, Standard floating-point to integer conversions
+ */
+
+int __aeabi_d2iz(aeabi_double_t a)
+{
+ return f64_to_i32_r_minMag(f64_from_d(a), false);
+}
+
+unsigned __aeabi_d2uiz(aeabi_double_t a)
+{
+ return f64_to_ui32_r_minMag(f64_from_d(a), false);
+}
+
+long long __aeabi_d2lz(aeabi_double_t a)
+{
+ return f64_to_i64_r_minMag(f64_from_d(a), false);
+}
+
+unsigned long long __aeabi_d2ulz(aeabi_double_t a)
+{
+ return f64_to_ui64_r_minMag(f64_from_d(a), false);
+}
+
+int __aeabi_f2iz(aeabi_float_t a)
+{
+ return f32_to_i32_r_minMag(f32_from_f(a), false);
+}
+
+unsigned __aeabi_f2uiz(aeabi_float_t a)
+{
+ return f32_to_ui32_r_minMag(f32_from_f(a), false);
+}
+
+long long __aeabi_f2lz(aeabi_float_t a)
+{
+ return f32_to_i64_r_minMag(f32_from_f(a), false);
+}
+
+unsigned long long __aeabi_f2ulz(aeabi_float_t a)
+{
+ return f32_to_ui64_r_minMag(f32_from_f(a), false);
+}
+
+/*
+ * Table 7, Standard conversions between floating types
+ */
+
+aeabi_float_t __aeabi_d2f(aeabi_double_t a)
+{
+ return f32_to_f(f64_to_f32(f64_from_d(a)));
+}
+
+aeabi_double_t __aeabi_f2d(aeabi_float_t a)
+{
+ return f64_to_d(f32_to_f64(f32_from_f(a)));
+}
+
+/*
+ * Table 8, Standard integer to floating-point conversions
+ */
+
+aeabi_double_t __aeabi_i2d(int a)
+{
+ return f64_to_d(i32_to_f64(a));
+}
+
+aeabi_double_t __aeabi_ui2d(unsigned a)
+{
+ return f64_to_d(ui32_to_f64(a));
+}
+
+aeabi_double_t __aeabi_l2d(long long a)
+{
+ return f64_to_d(i64_to_f64(a));
+}
+
+aeabi_double_t __aeabi_ul2d(unsigned long long a)
+{
+ return f64_to_d(ui64_to_f64(a));
+}
+
+aeabi_float_t __aeabi_i2f(int a)
+{
+ return f32_to_f(i32_to_f32(a));
+}
+
+aeabi_float_t __aeabi_ui2f(unsigned a)
+{
+ return f32_to_f(ui32_to_f32(a));
+}
+
+aeabi_float_t __aeabi_l2f(long long a)
+{
+ return f32_to_f(i64_to_f32(a));
+}
+
+aeabi_float_t __aeabi_ul2f(unsigned long long a)
+{
+ return f32_to_f(ui64_to_f32(a));
+}
diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/memset.c b/ArmPkg/Library/CompilerIntrinsicsLib/memset.c
index c5ae32e5ee71..24398d591f79 100644
--- a/ArmPkg/Library/CompilerIntrinsicsLib/memset.c
+++ b/ArmPkg/Library/CompilerIntrinsicsLib/memset.c
@@ -1,55 +1,55 @@
-//------------------------------------------------------------------------------
-//
-// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
-//
-// SPDX-License-Identifier: BSD-2-Clause-Patent
-//
-//------------------------------------------------------------------------------
-
-typedef __SIZE_TYPE__ size_t;
-
-static __attribute__((__used__))
-void *__memset(void *s, int c, size_t n)
-{
- unsigned char *d = s;
-
- while (n--)
- *d++ = c;
-
- return s;
-}
-
-//
-// Other modules (such as CryptoPkg/IntrinsicLib) may provide another
-// implementation of memset(), which may conflict with this one if this
-// object was pulled into the link due to the definitions below. So make
-// our memset() 'weak' to let the other implementation take precedence.
-//
-__attribute__((__weak__, __alias__("__memset")))
-void *memset(void *dest, int c, size_t n);
-
-#ifdef __arm__
-
-void __aeabi_memset(void *dest, size_t n, int c)
-{
- __memset(dest, c, n);
-}
-
-__attribute__((__alias__("__aeabi_memset")))
-void __aeabi_memset4(void *dest, size_t n, int c);
-
-__attribute__((__alias__("__aeabi_memset")))
-void __aeabi_memset8(void *dest, size_t n, int c);
-
-void __aeabi_memclr(void *dest, size_t n)
-{
- __memset(dest, 0, n);
-}
-
-__attribute__((__alias__("__aeabi_memclr")))
-void __aeabi_memclr4(void *dest, size_t n);
-
-__attribute__((__alias__("__aeabi_memclr")))
-void __aeabi_memclr8(void *dest, size_t n);
-
-#endif
+//------------------------------------------------------------------------------
+//
+// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+//------------------------------------------------------------------------------
+
+typedef __SIZE_TYPE__ size_t;
+
+static __attribute__((__used__))
+void *__memset(void *s, int c, size_t n)
+{
+ unsigned char *d = s;
+
+ while (n--)
+ *d++ = c;
+
+ return s;
+}
+
+//
+// Other modules (such as CryptoPkg/IntrinsicLib) may provide another
+// implementation of memset(), which may conflict with this one if this
+// object was pulled into the link due to the definitions below. So make
+// our memset() 'weak' to let the other implementation take precedence.
+//
+__attribute__((__weak__, __alias__("__memset")))
+void *memset(void *dest, int c, size_t n);
+
+#ifdef __arm__
+
+void __aeabi_memset(void *dest, size_t n, int c)
+{
+ __memset(dest, c, n);
+}
+
+__attribute__((__alias__("__aeabi_memset")))
+void __aeabi_memset4(void *dest, size_t n, int c);
+
+__attribute__((__alias__("__aeabi_memset")))
+void __aeabi_memset8(void *dest, size_t n, int c);
+
+void __aeabi_memclr(void *dest, size_t n)
+{
+ __memset(dest, 0, n);
+}
+
+__attribute__((__alias__("__aeabi_memclr")))
+void __aeabi_memclr4(void *dest, size_t n);
+
+__attribute__((__alias__("__aeabi_memclr")))
+void __aeabi_memclr8(void *dest, size_t n);
+
+#endif
diff --git a/ArmPkg/Library/GccLto/liblto-aarch64.s b/ArmPkg/Library/GccLto/liblto-aarch64.s
index 66819534a045..02a55ef4456c 100644
--- a/ArmPkg/Library/GccLto/liblto-aarch64.s
+++ b/ArmPkg/Library/GccLto/liblto-aarch64.s
@@ -1,21 +1,21 @@
-//
-// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
-//
-// SPDX-License-Identifier: BSD-2-Clause-Patent
-//
-
-//
-// GCC in LTO mode interoperates poorly with non-standard libraries that
-// provide implementations of compiler intrinsics such as memcpy/memset
-// or the stack protector entry points.
-//
-// By referencing these functions from a non-LTO object that can be passed
-// to the linker via the -plugin-opt=-pass-through=-lxxx options, the
-// intrinsics are included in the link in a way that allows them to be
-// pruned again if no other references to them exist.
-//
-
- .long memcpy - .
- .long memset - .
- .long __stack_chk_fail - .
- .long __stack_chk_guard - .
+//
+// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+
+//
+// GCC in LTO mode interoperates poorly with non-standard libraries that
+// provide implementations of compiler intrinsics such as memcpy/memset
+// or the stack protector entry points.
+//
+// By referencing these functions from a non-LTO object that can be passed
+// to the linker via the -plugin-opt=-pass-through=-lxxx options, the
+// intrinsics are included in the link in a way that allows them to be
+// pruned again if no other references to them exist.
+//
+
+ .long memcpy - .
+ .long memset - .
+ .long __stack_chk_fail - .
+ .long __stack_chk_guard - .
diff --git a/ArmPkg/Library/GccLto/liblto-arm.s b/ArmPkg/Library/GccLto/liblto-arm.s
index 4b26d4320d52..f19fb455519d 100644
--- a/ArmPkg/Library/GccLto/liblto-arm.s
+++ b/ArmPkg/Library/GccLto/liblto-arm.s
@@ -1,55 +1,55 @@
-//
-// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
-//
-// SPDX-License-Identifier: BSD-2-Clause-Patent
-//
-
-//
-// GCC in LTO mode interoperates poorly with non-standard libraries that
-// provide implementations of compiler intrinsics such as memcpy/memset
-// or the stack protector entry points.
-//
-// By referencing these functions from a non-LTO object that can be passed
-// to the linker via the -plugin-opt=-pass-through=-lxxx options, the
-// intrinsics are included in the link in a way that allows them to be
-// pruned again if no other references to them exist.
-//
-
- .long memcpy - .
- .long memset - .
- .long __stack_chk_fail - .
- .long __stack_chk_guard - .
- .long __ashrdi3 - .
- .long __ashldi3 - .
- .long __aeabi_idiv - .
- .long __aeabi_idivmod - .
- .long __aeabi_uidiv - .
- .long __aeabi_uidivmod - .
- .long __divdi3 - .
- .long __divsi3 - .
- .long __lshrdi3 - .
- .long __aeabi_memcpy - .
- .long __aeabi_memset - .
- .long memmove - .
- .long __modsi3 - .
- .long __moddi3 - .
- .long __muldi3 - .
- .long __aeabi_lmul - .
- .long __ARM_ll_mullu - .
- .long __udivsi3 - .
- .long __umodsi3 - .
- .long __udivdi3 - .
- .long __umoddi3 - .
- .long __udivmoddi4 - .
- .long __clzsi2 - .
- .long __ctzsi2 - .
- .long __ucmpdi2 - .
- .long __switch8 - .
- .long __switchu8 - .
- .long __switch16 - .
- .long __switch32 - .
- .long __aeabi_ulcmp - .
- .long __aeabi_uldivmod - .
- .long __aeabi_ldivmod - .
- .long __aeabi_llsr - .
- .long __aeabi_llsl - .
+//
+// Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+
+//
+// GCC in LTO mode interoperates poorly with non-standard libraries that
+// provide implementations of compiler intrinsics such as memcpy/memset
+// or the stack protector entry points.
+//
+// By referencing these functions from a non-LTO object that can be passed
+// to the linker via the -plugin-opt=-pass-through=-lxxx options, the
+// intrinsics are included in the link in a way that allows them to be
+// pruned again if no other references to them exist.
+//
+
+ .long memcpy - .
+ .long memset - .
+ .long __stack_chk_fail - .
+ .long __stack_chk_guard - .
+ .long __ashrdi3 - .
+ .long __ashldi3 - .
+ .long __aeabi_idiv - .
+ .long __aeabi_idivmod - .
+ .long __aeabi_uidiv - .
+ .long __aeabi_uidivmod - .
+ .long __divdi3 - .
+ .long __divsi3 - .
+ .long __lshrdi3 - .
+ .long __aeabi_memcpy - .
+ .long __aeabi_memset - .
+ .long memmove - .
+ .long __modsi3 - .
+ .long __moddi3 - .
+ .long __muldi3 - .
+ .long __aeabi_lmul - .
+ .long __ARM_ll_mullu - .
+ .long __udivsi3 - .
+ .long __umodsi3 - .
+ .long __udivdi3 - .
+ .long __umoddi3 - .
+ .long __udivmoddi4 - .
+ .long __clzsi2 - .
+ .long __ctzsi2 - .
+ .long __ucmpdi2 - .
+ .long __switch8 - .
+ .long __switchu8 - .
+ .long __switch16 - .
+ .long __switch32 - .
+ .long __aeabi_ulcmp - .
+ .long __aeabi_uldivmod - .
+ .long __aeabi_ldivmod - .
+ .long __aeabi_llsr - .
+ .long __aeabi_llsl - .
--
2.19.1.3.g30247aa5d201


Re: [PATCH v3 0/6] OvmfPkg: implement initrd shell command and mixed mode loader

Ard Biesheuvel
 

On Wed, 26 Feb 2020 at 20:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

This is tagged as a v2 since it is a followup to a couple of patches [0][1]
that have already been sent to the list.

This series is part of my effort to define a generic EFI boot protocol for
Linux, i.e,. one that is the same across all different architectures that
are able to boot Linux from EFI, and naturally reused the firmware's
infrastructure for authenticated boot and measured boot.
Logged as https://bugzilla.tianocore.org/show_bug.cgi?id=2564


Re: [PATCH v4 00/11] ArmVirtPkg: implement measured boot for ArmVirtQemu

Ard Biesheuvel
 

On Thu, 27 Feb 2020 at 19:14, Laszlo Ersek <lersek@redhat.com> wrote:

On 02/27/20 15:40, Ard Biesheuvel wrote:
Wire up the various existing pieces so that we can implement measured
boot on ArmVirtQemu based on the TPM support in QEMU, just like it has
been implemented for x86 in OvmfPkg.

The main difference is that on ARM, we first need to discover the TPM base
address from the device tree provided by QEMU, as well as the PSCI method
used to perform a cold reset.

Changes since v3:
- add Laszlo's ack to patches #3, #5, #6, #10 and #11
- incorporate Laszlo's review feedback, including splitting off #7 and #8
from patch #9
Before you merge this set after edk2-stable202002 is tagged, please add
the following line to each commit message in the series:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2560
Will do, thanks for the reminder.


Re: [PATCH v4 00/11] ArmVirtPkg: implement measured boot for ArmVirtQemu

Laszlo Ersek
 

On 02/27/20 15:40, Ard Biesheuvel wrote:
Wire up the various existing pieces so that we can implement measured
boot on ArmVirtQemu based on the TPM support in QEMU, just like it has
been implemented for x86 in OvmfPkg.

The main difference is that on ARM, we first need to discover the TPM base
address from the device tree provided by QEMU, as well as the PSCI method
used to perform a cold reset.

Changes since v3:
- add Laszlo's ack to patches #3, #5, #6, #10 and #11
- incorporate Laszlo's review feedback, including splitting off #7 and #8
from patch #9
Before you merge this set after edk2-stable202002 is tagged, please add
the following line to each commit message in the series:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2560

Thanks!
Laszlo


Re: [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

Laszlo Ersek
 

On 02/27/20 14:14, Laszlo Ersek wrote:
(+Liming and stewards; CC Nick)

On 02/27/20 12:02, Maciej Rabeda wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032

'ping' command's receive flow utilizes a single Rx token which it
attempts to reuse before recycling the previously received packet.
This causes a situation where under ICMP traffic,
Ping6OnEchoReplyReceived() function will receive an already
recycled packet with EFI_SUCCESS token status and finally
dereference invalid pointers from RxData structure.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index 23567fa2c1bb..a3fa32515192 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (

ON_EXIT:

+ //
+ // Recycle the packet before reusing RxToken
+ //
+ gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal);
+
if (Private->RxCount < Private->SendNum) {
//
// Continue to receive icmp echo reply packets.
@@ -632,10 +637,6 @@ ON_EXIT:
//
Private->Status = EFI_SUCCESS;
}
- //
- // Singal to recycle the each rxdata here, not at the end of process.
- //
- gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal);
}

/**
(1) This patch proposes to fix one of the BZs (2032) that fall under
CVE-2019-14559 (joint tracker: 2550).

Consequently:

(1a) Do we want to include this in the upcoming stable tag?

If so, we might want to extend the hard feature freeze by a few days.

(1b) Please append the string " (CVE-2019-14559)" -- note the separating
space! -- to the subject line.

(2) However: I remember from an earlier Bugzilla entry (can't tell
off-hand, which one, sorry) that ShellPkg issues are *never* considered
CVE-worthy, because the shell is not considered a "production element"
of the UEFI boot path.
I misremembered -- there is indeed a comment like that, in the TianoCore
bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which
has since been split off to the edk2-libc project):

https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1

StdLib is supposed to be used only by applications in shell, all of
which are meant for debug, diagnosis and/or test purpose, not for
product UEFI BIOS. Any issue in it will not be taken as security
issue but just normal bug.

Sorry about causing confusion. So, the ShellPkg maintainers should
decide what to do about this bug (keep it under the CVE scope vs.
exclude it from the CVE scope; and then, propose it for the stable tag
or merge it afterwards).

One data point: the bug appears to go back to the inception of the Ping
command, in historical commit 68fb05272b45 ("Add Network1 profile.",
2011-03-25). It's not a new bug, it seems.

Thanks
Laszlo


TianoCore#2032 was originally filed for NetworkPkg, and indeed that
seemed to justify the CVE assignment. However, now that Nick's and
Maciej's analysis shows that NetworkPkg is unaffected (and we know, per
above, that ShellPkg is not CVE-worthy), should we rather *remove* this
BZ from the CVE-2019-14559 umbrella?

Because, in that case, modifying the subject line on the patch is not
necessary; and more importantly, we might not even want to put this into
edk2-stable202002. (It's still a bugfix, but may not be important enough.)

Thanks!
Laszlo


[PATCHv3 5/5] MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode

Albecki, Mateusz
 

Current driver does not support PIO transfer mode for
commands other then tuning. This change adds the code
to transfer PIO data.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marcin Wojtas <mw@semihalf.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>

Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 3 +
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 132 +++++++++++++++++----
2 files changed, 114 insertions(+), 21 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 15b7d12596..fd89306fab 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -157,6 +157,9 @@ typedef struct {
UINT64 Timeout;
UINT32 Retries;

+ BOOLEAN PioModeTransferCompleted;
+ UINT32 PioBlockIndex;
+
SD_MMC_HC_ADMA_32_DESC_LINE *Adma32Desc;
SD_MMC_HC_ADMA_64_V3_DESC_LINE *Adma64V3Desc;
SD_MMC_HC_ADMA_64_V4_DESC_LINE *Adma64V4Desc;
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 422862577e..497ac08355 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1713,6 +1713,8 @@ SdMmcPrintTrb (
DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb->CommandComplete));
DEBUG ((DebugLevel, "Timeout: %ld\n", Trb->Timeout));
DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
+ DEBUG ((DebugLevel, "PioModeTransferCompleted: %d\n", Trb->PioModeTransferCompleted));
+ DEBUG ((DebugLevel, "PioBlockIndex: %d\n", Trb->PioBlockIndex));
DEBUG ((DebugLevel, "Adma32Desc: %p\n", Trb->Adma32Desc));
DEBUG ((DebugLevel, "Adma64V3Desc: %p\n", Trb->Adma64V3Desc));
DEBUG ((DebugLevel, "Adma64V4Desc: %p\n", Trb->Adma64V4Desc));
@@ -1817,6 +1819,8 @@ SdMmcCreateTrb (
Trb->CommandComplete = FALSE;
Trb->Timeout = Packet->Timeout;
Trb->Retries = SD_MMC_TRB_RETRIES;
+ Trb->PioModeTransferCompleted = FALSE;
+ Trb->PioBlockIndex = 0;
Trb->Private = Private;

if ((Packet->InTransferLength != 0) && (Packet->InDataBuffer != NULL)) {
@@ -2479,6 +2483,104 @@ SdMmcCheckCommandComplete (
return EFI_NOT_READY;
}

+/**
+ Transfers data from card using PIO method.
+
+ @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+ @param[in] Trb The pointer to the SD_MMC_HC_TRB instance.
+ @param[in] IntStatus Snapshot of the normal interrupt status register.
+
+ @retval EFI_SUCCESS PIO transfer completed successfully.
+ @retval EFI_NOT_READY PIO transfer completion still pending.
+ @retval Others PIO transfer failed to complete.
+**/
+EFI_STATUS
+SdMmcTransferDataWithPio (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN SD_MMC_HC_TRB *Trb,
+ IN UINT16 IntStatus
+ )
+{
+ EFI_STATUS Status;
+ UINT16 Data16;
+ UINT32 BlockCount;
+ EFI_PCI_IO_PROTOCOL_WIDTH Width;
+ UINTN Count;
+
+ BlockCount = (Trb->DataLen / Trb->BlockSize);
+ if (Trb->DataLen % Trb->BlockSize != 0) {
+ BlockCount += 1;
+ }
+
+ if (Trb->PioBlockIndex >= BlockCount) {
+ return EFI_SUCCESS;
+ }
+
+ switch (Trb->BlockSize % sizeof (UINT32)) {
+ case 0:
+ Width = EfiPciIoWidthFifoUint32;
+ Count = Trb->BlockSize / sizeof (UINT32);
+ break;
+ case 2:
+ Width = EfiPciIoWidthFifoUint16;
+ Count = Trb->BlockSize / sizeof (UINT16);
+ break;
+ case 1:
+ case 3:
+ default:
+ Width = EfiPciIoWidthFifoUint8;
+ Count = Trb->BlockSize;
+ break;
+ }
+
+ if (Trb->Read) {
+ if ((IntStatus & BIT5) == 0) {
+ return EFI_NOT_READY;
+ }
+ Data16 = BIT5;
+ SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (Data16), &Data16);
+
+ Status = Private->PciIo->Mem.Read (
+ Private->PciIo,
+ Width,
+ Trb->Slot,
+ SD_MMC_HC_BUF_DAT_PORT,
+ Count,
+ (VOID*)((UINT8*)Trb->Data + (Trb->BlockSize * Trb->PioBlockIndex))
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Trb->PioBlockIndex++;
+ } else {
+ if ((IntStatus & BIT4) == 0) {
+ return EFI_NOT_READY;
+ }
+ Data16 = BIT4;
+ SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (Data16), &Data16);
+
+ Status = Private->PciIo->Mem.Write (
+ Private->PciIo,
+ Width,
+ Trb->Slot,
+ SD_MMC_HC_BUF_DAT_PORT,
+ Count,
+ (VOID*)((UINT8*)Trb->Data + (Trb->BlockSize * Trb->PioBlockIndex))
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Trb->PioBlockIndex++;
+ }
+
+ if (Trb->PioBlockIndex >= BlockCount) {
+ Trb->PioModeTransferCompleted = TRUE;
+ return EFI_SUCCESS;
+ } else {
+ return EFI_NOT_READY;
+ }
+}
+
/**
Update the SDMA address on the SDMA buffer boundary interrupt.

@@ -2563,6 +2665,13 @@ SdMmcCheckDataTransfer (
return Status;
}

+ if (Trb->Mode == SdMmcPioMode && !Trb->PioModeTransferCompleted) {
+ Status = SdMmcTransferDataWithPio (Private, Trb, IntStatus);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
if ((Trb->Mode == SdMmcSdmaMode) && ((IntStatus & BIT3) != 0)) {
Data16 = BIT3;
Status = SdMmcHcRwMmio (
@@ -2605,7 +2714,6 @@ SdMmcCheckTrbResult (
EFI_STATUS Status;
EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
UINT16 IntStatus;
- UINT32 PioLength;

Packet = Trb->Packet;
//
@@ -2641,26 +2749,8 @@ SdMmcCheckTrbResult (
(Packet->SdMmcCmdBlk->CommandIndex == EMMC_SEND_TUNING_BLOCK)) ||
((Private->Slot[Trb->Slot].CardType == SdCardType) &&
(Packet->SdMmcCmdBlk->CommandIndex == SD_SEND_TUNING_BLOCK))) {
- //
- // When performing tuning procedure (Execute Tuning is set to 1) through PIO mode,
- // wait Buffer Read Ready bit of Normal Interrupt Status Register to be 1.
- // Refer to SD Host Controller Simplified Specification 3.0 figure 2-29 for details.
- //
- if ((IntStatus & BIT5) == BIT5) {
- //
- // Clear Buffer Read Ready interrupt at first.
- //
- IntStatus = BIT5;
- SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (IntStatus), &IntStatus);
- //
- // Read data out from Buffer Port register
- //
- for (PioLength = 0; PioLength < Trb->DataLen; PioLength += 4) {
- SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_BUF_DAT_PORT, TRUE, 4, (UINT8*)Trb->Data + PioLength);
- }
- Status = EFI_SUCCESS;
- goto Done;
- }
+ Status = SdMmcTransferDataWithPio (Private, Trb, IntStatus);
+ goto Done;
}

if (!Trb->CommandComplete) {
--
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


[PATCHv3 4/5] MdeModulePkg/SdMmcPciHcDxe: Do not map memory for non DMA transfer

Albecki, Mateusz
 

Driver code used to map memory for DMA transfer even if host doesn't
support DMA. This is causing memory corruption when driver transfers
data using PIO. This change refactors the code to skip call to
PciIo->Map for non DMA transfers.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marcin Wojtas <mw@semihalf.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>

Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 88 ++++++++++++++++--------
1 file changed, 61 insertions(+), 27 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index bb699027e3..422862577e 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1722,6 +1722,62 @@ SdMmcPrintTrb (
SdMmcPrintPacket (DebugLevel, Trb->Packet);
}

+/**
+ Sets up host memory to allow DMA transfer.
+
+ @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+ @param[in] Slot The slot number of the SD card to send the command to.
+ @param[in] Packet A pointer to the SD command data structure.
+
+ @retval EFI_SUCCESS Memory has been mapped for DMA transfer.
+ @retval Others Memory has not been mapped.
+**/
+EFI_STATUS
+SdMmcSetupMemoryForDmaTransfer (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN UINT8 Slot,
+ IN SD_MMC_HC_TRB *Trb
+ )
+{
+ EFI_PCI_IO_PROTOCOL_OPERATION Flag;
+ EFI_PCI_IO_PROTOCOL *PciIo;
+ UINTN MapLength;
+ EFI_STATUS Status;
+
+ if (Trb->Read) {
+ Flag = EfiPciIoOperationBusMasterWrite;
+ } else {
+ Flag = EfiPciIoOperationBusMasterRead;
+ }
+
+ PciIo = Private->PciIo;
+ if (Trb->Data != NULL && Trb->DataLen != 0) {
+ MapLength = Trb->DataLen;
+ Status = PciIo->Map (
+ PciIo,
+ Flag,
+ Trb->Data,
+ &MapLength,
+ &Trb->DataPhy,
+ &Trb->DataMap
+ );
+ if (EFI_ERROR (Status) || (Trb->DataLen != MapLength)) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+ }
+
+ if (Trb->Mode == SdMmcAdma32bMode ||
+ Trb->Mode == SdMmcAdma64bV3Mode ||
+ Trb->Mode == SdMmcAdma64bV4Mode) {
+ Status = BuildAdmaDescTable (Trb, Private->ControllerVersion[Slot]);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Create a new TRB for the SD/MMC cmd request.

@@ -1746,9 +1802,6 @@ SdMmcCreateTrb (
SD_MMC_HC_TRB *Trb;
EFI_STATUS Status;
EFI_TPL OldTpl;
- EFI_PCI_IO_PROTOCOL_OPERATION Flag;
- EFI_PCI_IO_PROTOCOL *PciIo;
- UINTN MapLength;

Trb = AllocateZeroPool (sizeof (SD_MMC_HC_TRB));
if (Trb == NULL) {
@@ -1791,29 +1844,6 @@ SdMmcCreateTrb (
(Packet->SdMmcCmdBlk->CommandIndex == SD_SEND_TUNING_BLOCK))) {
Trb->Mode = SdMmcPioMode;
} else {
- if (Trb->Read) {
- Flag = EfiPciIoOperationBusMasterWrite;
- } else {
- Flag = EfiPciIoOperationBusMasterRead;
- }
-
- PciIo = Private->PciIo;
- if (Trb->DataLen != 0) {
- MapLength = Trb->DataLen;
- Status = PciIo->Map (
- PciIo,
- Flag,
- Trb->Data,
- &MapLength,
- &Trb->DataPhy,
- &Trb->DataMap
- );
- if (EFI_ERROR (Status) || (Trb->DataLen != MapLength)) {
- Status = EFI_BAD_BUFFER_SIZE;
- goto Error;
- }
- }
-
if (Trb->DataLen == 0) {
Trb->Mode = SdMmcNoData;
} else if (Private->Capability[Slot].Adma2 != 0) {
@@ -1831,12 +1861,16 @@ SdMmcCreateTrb (
if (Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_410) {
Trb->AdmaLengthMode = SdMmcAdmaLen26b;
}
- Status = BuildAdmaDescTable (Trb, Private->ControllerVersion[Slot]);
+ Status = SdMmcSetupMemoryForDmaTransfer (Private, Slot, Trb);
if (EFI_ERROR (Status)) {
goto Error;
}
} else if (Private->Capability[Slot].Sdma != 0) {
Trb->Mode = SdMmcSdmaMode;
+ Status = SdMmcSetupMemoryForDmaTransfer (Private, Slot, Trb);
+ if (EFI_ERROR (Status)) {
+ goto Error;
+ }
} else {
Trb->Mode = SdMmcPioMode;
}
--
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


[PATCHv3 3/5] MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion

Albecki, Mateusz
 

This patch refactors the way in which the driver will check
the data transfer completion. Data transfer related
functionalities have been moved to separate function.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marcin Wojtas <mw@semihalf.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>

Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 181 ++++++++++++++---------
1 file changed, 112 insertions(+), 69 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 205ec86032..bb699027e3 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -2445,6 +2445,112 @@ SdMmcCheckCommandComplete (
return EFI_NOT_READY;
}

+/**
+ Update the SDMA address on the SDMA buffer boundary interrupt.
+
+ @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+ @param[in] Trb The pointer to the SD_MMC_HC_TRB instance.
+
+ @retval EFI_SUCCESS Updated SDMA buffer address.
+ @retval Others Failed to update SDMA buffer address.
+**/
+EFI_STATUS
+SdMmcUpdateSdmaAddress (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN SD_MMC_HC_TRB *Trb
+ )
+{
+ UINT64 SdmaAddr;
+ EFI_STATUS Status;
+
+ SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy, SD_MMC_SDMA_BOUNDARY);
+
+ if (Private->ControllerVersion[Trb->Slot] >= SD_MMC_HC_CTRL_VER_400) {
+ Status = SdMmcHcRwMmio (
+ Private->PciIo,
+ Trb->Slot,
+ SD_MMC_HC_ADMA_SYS_ADDR,
+ FALSE,
+ sizeof (UINT64),
+ &SdmaAddr
+ );
+ } else {
+ Status = SdMmcHcRwMmio (
+ Private->PciIo,
+ Trb->Slot,
+ SD_MMC_HC_SDMA_ADDR,
+ FALSE,
+ sizeof (UINT32),
+ &SdmaAddr
+ );
+ }
+
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
+ return EFI_SUCCESS;
+}
+
+/**
+ Checks if the data transfer completed and performs any actions
+ neccessary to continue the data transfer such as SDMA system
+ address fixup or PIO data transfer.
+
+ @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+ @param[in] Trb The pointer to the SD_MMC_HC_TRB instance.
+ @param[in] IntStatus Snapshot of the normal interrupt status register.
+
+ @retval EFI_SUCCESS Data transfer completed successfully.
+ @retval EFI_NOT_READY Data transfer completion still pending.
+ @retval Others Data transfer failed to complete.
+**/
+EFI_STATUS
+SdMmcCheckDataTransfer (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN SD_MMC_HC_TRB *Trb,
+ IN UINT16 IntStatus
+ )
+{
+ UINT16 Data16;
+ EFI_STATUS Status;
+
+ if ((IntStatus & BIT1) != 0) {
+ Data16 = BIT1;
+ Status = SdMmcHcRwMmio (
+ Private->PciIo,
+ Trb->Slot,
+ SD_MMC_HC_NOR_INT_STS,
+ FALSE,
+ sizeof (Data16),
+ &Data16
+ );
+ return Status;
+ }
+
+ if ((Trb->Mode == SdMmcSdmaMode) && ((IntStatus & BIT3) != 0)) {
+ Data16 = BIT3;
+ Status = SdMmcHcRwMmio (
+ Private->PciIo,
+ Trb->Slot,
+ SD_MMC_HC_NOR_INT_STS,
+ FALSE,
+ sizeof (Data16),
+ &Data16
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = SdMmcUpdateSdmaAddress (Private, Trb);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ return EFI_NOT_READY;
+}
+
/**
Check the TRB execution result.

@@ -2465,7 +2571,6 @@ SdMmcCheckTrbResult (
EFI_STATUS Status;
EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
UINT16 IntStatus;
- UINT64 SdmaAddr;
UINT32 PioLength;

Packet = Trb->Packet;
@@ -2528,80 +2633,18 @@ SdMmcCheckTrbResult (
Status = SdMmcCheckCommandComplete (Private, Trb, IntStatus);
if (EFI_ERROR (Status)) {
goto Done;
- } else {
- //
- // If the command doesn't require data transfer skip the transfer
- // complete checking.
- //
- if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) &&
- (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR1b) &&
- (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR5b)) {
- goto Done;
- }
}
}

- //
- // Check Transfer Complete bit is set or not.
- //
- if ((IntStatus & BIT1) == BIT1) {
- goto Done;
- }
-
- //
- // Check if DMA interrupt is signalled for the SDMA transfer.
- //
- if ((Trb->Mode == SdMmcSdmaMode) && ((IntStatus & BIT3) == BIT3)) {
- //
- // Clear DMA interrupt bit.
- //
- IntStatus = BIT3;
- Status = SdMmcHcRwMmio (
- Private->PciIo,
- Trb->Slot,
- SD_MMC_HC_NOR_INT_STS,
- FALSE,
- sizeof (IntStatus),
- &IntStatus
- );
- if (EFI_ERROR (Status)) {
- goto Done;
- }
- //
- // Update SDMA Address register.
- //
- SdmaAddr = SD_MMC_SDMA_ROUND_UP ((UINTN)Trb->DataPhy, SD_MMC_SDMA_BOUNDARY);
-
- if (Private->ControllerVersion[Trb->Slot] >= SD_MMC_HC_CTRL_VER_400) {
- Status = SdMmcHcRwMmio (
- Private->PciIo,
- Trb->Slot,
- SD_MMC_HC_ADMA_SYS_ADDR,
- FALSE,
- sizeof (UINT64),
- &SdmaAddr
- );
- } else {
- Status = SdMmcHcRwMmio (
- Private->PciIo,
- Trb->Slot,
- SD_MMC_HC_SDMA_ADDR,
- FALSE,
- sizeof (UINT32),
- &SdmaAddr
- );
- }
-
- if (EFI_ERROR (Status)) {
- goto Done;
- }
- Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
+ if (Packet->SdMmcCmdBlk->CommandType == SdMmcCommandTypeAdtc ||
+ Packet->SdMmcCmdBlk->ResponseType == SdMmcResponseTypeR1b ||
+ Packet->SdMmcCmdBlk->ResponseType == SdMmcResponseTypeR5b) {
+ Status = SdMmcCheckDataTransfer (Private, Trb, IntStatus);
+ } else {
+ Status = EFI_SUCCESS;
}

-
- Status = EFI_NOT_READY;
Done:
-
if (Status != EFI_NOT_READY) {
SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
if (EFI_ERROR (Status)) {
--
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


[PATCHv3 2/5] MdeModulePkg/SdMmcPciHcDxe: Read response on command completion

Albecki, Mateusz
 

SdMmcPciHcDxe driver used to read response only after
command and data transfer completed. According to SDHCI
specification response data is ready after the command
complete status is set by the host controller. Getting
the response data early will help debugging the cases
when command completed but data transfer timed out.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marcin Wojtas <mw@semihalf.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>

Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 1 +
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 201 +++++++++++++++------
2 files changed, 144 insertions(+), 58 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 5bc3577ba2..15b7d12596 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -153,6 +153,7 @@ typedef struct {

EFI_EVENT Event;
BOOLEAN Started;
+ BOOLEAN CommandComplete;
UINT64 Timeout;
UINT32 Retries;

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 71cf5a78f9..205ec86032 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1710,6 +1710,7 @@ SdMmcPrintTrb (
DEBUG ((DebugLevel, "AdmaLengthMode: %d\n", Trb->AdmaLengthMode));
DEBUG ((DebugLevel, "Event: %p\n", Trb->Event));
DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
+ DEBUG ((DebugLevel, "CommandComplete: %d\n", Trb->CommandComplete));
DEBUG ((DebugLevel, "Timeout: %ld\n", Trb->Timeout));
DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
DEBUG ((DebugLevel, "Adma32Desc: %p\n", Trb->Adma32Desc));
@@ -1760,6 +1761,7 @@ SdMmcCreateTrb (
Trb->Packet = Packet;
Trb->Event = Event;
Trb->Started = FALSE;
+ Trb->CommandComplete = FALSE;
Trb->Timeout = Packet->Timeout;
Trb->Retries = SD_MMC_TRB_RETRIES;
Trb->Private = Private;
@@ -2350,6 +2352,99 @@ SdMmcCheckAndRecoverErrors (
return ErrorStatus;
}

+/**
+ Reads the response data into the TRB buffer.
+ This function assumes that caller made sure that
+ command has completed.
+
+ @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+ @param[in] Trb The pointer to the SD_MMC_HC_TRB instance.
+
+ @retval EFI_SUCCESS Response read successfully.
+ @retval Others Failed to get response.
+**/
+EFI_STATUS
+SdMmcGetResponse (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN SD_MMC_HC_TRB *Trb
+ )
+{
+ EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
+ UINT8 Index;
+ UINT32 Response[4];
+ EFI_STATUS Status;
+
+ Packet = Trb->Packet;
+
+ if (Packet->SdMmcCmdBlk->CommandType == SdMmcCommandTypeBc) {
+ return EFI_SUCCESS;
+ }
+
+ for (Index = 0; Index < 4; Index++) {
+ Status = SdMmcHcRwMmio (
+ Private->PciIo,
+ Trb->Slot,
+ SD_MMC_HC_RESPONSE + Index * 4,
+ TRUE,
+ sizeof (UINT32),
+ &Response[Index]
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+ CopyMem (Packet->SdMmcStatusBlk, Response, sizeof (Response));
+
+ return EFI_SUCCESS;
+}
+
+/**
+ Checks if the command completed. If the command
+ completed it gets the response and records the
+ command completion in the TRB.
+
+ @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+ @param[in] Trb The pointer to the SD_MMC_HC_TRB instance.
+ @param[in] IntStatus Snapshot of the normal interrupt status register.
+
+ @retval EFI_SUCCESS Command completed successfully.
+ @retval EFI_NOT_READY Command completion still pending.
+ @retval Others Command failed to complete.
+**/
+EFI_STATUS
+SdMmcCheckCommandComplete (
+ IN SD_MMC_HC_PRIVATE_DATA *Private,
+ IN SD_MMC_HC_TRB *Trb,
+ IN UINT16 IntStatus
+ )
+{
+ UINT16 Data16;
+ EFI_STATUS Status;
+
+ if ((IntStatus & BIT0) != 0) {
+ Data16 = BIT0;
+ Status = SdMmcHcRwMmio (
+ Private->PciIo,
+ Trb->Slot,
+ SD_MMC_HC_NOR_INT_STS,
+ FALSE,
+ sizeof (Data16),
+ &Data16
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Status = SdMmcGetResponse (Private, Trb);
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ Trb->CommandComplete = TRUE;
+ return EFI_SUCCESS;
+ }
+
+ return EFI_NOT_READY;
+}
+
/**
Check the TRB execution result.

@@ -2370,9 +2465,7 @@ SdMmcCheckTrbResult (
EFI_STATUS Status;
EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet;
UINT16 IntStatus;
- UINT32 Response[4];
UINT64 SdmaAddr;
- UINT8 Index;
UINT32 PioLength;

Packet = Trb->Packet;
@@ -2400,6 +2493,54 @@ SdMmcCheckTrbResult (
goto Done;
}

+ //
+ // Tuning commands are the only ones that do not generate command
+ // complete interrupt. Process them here before entering the code
+ // that waits for command completion.
+ //
+ if (((Private->Slot[Trb->Slot].CardType == EmmcCardType) &&
+ (Packet->SdMmcCmdBlk->CommandIndex == EMMC_SEND_TUNING_BLOCK)) ||
+ ((Private->Slot[Trb->Slot].CardType == SdCardType) &&
+ (Packet->SdMmcCmdBlk->CommandIndex == SD_SEND_TUNING_BLOCK))) {
+ //
+ // When performing tuning procedure (Execute Tuning is set to 1) through PIO mode,
+ // wait Buffer Read Ready bit of Normal Interrupt Status Register to be 1.
+ // Refer to SD Host Controller Simplified Specification 3.0 figure 2-29 for details.
+ //
+ if ((IntStatus & BIT5) == BIT5) {
+ //
+ // Clear Buffer Read Ready interrupt at first.
+ //
+ IntStatus = BIT5;
+ SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (IntStatus), &IntStatus);
+ //
+ // Read data out from Buffer Port register
+ //
+ for (PioLength = 0; PioLength < Trb->DataLen; PioLength += 4) {
+ SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_BUF_DAT_PORT, TRUE, 4, (UINT8*)Trb->Data + PioLength);
+ }
+ Status = EFI_SUCCESS;
+ goto Done;
+ }
+ }
+
+ if (!Trb->CommandComplete) {
+ Status = SdMmcCheckCommandComplete (Private, Trb, IntStatus);
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ } else {
+ //
+ // If the command doesn't require data transfer skip the transfer
+ // complete checking.
+ //
+ if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) &&
+ (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR1b) &&
+ (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR5b)) {
+ goto Done;
+ }
+ }
+ }
+
//
// Check Transfer Complete bit is set or not.
//
@@ -2457,65 +2598,9 @@ SdMmcCheckTrbResult (
Trb->DataPhy = (UINT64)(UINTN)SdmaAddr;
}

- if ((Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeAdtc) &&
- (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR1b) &&
- (Packet->SdMmcCmdBlk->ResponseType != SdMmcResponseTypeR5b)) {
- if ((IntStatus & BIT0) == BIT0) {
- Status = EFI_SUCCESS;
- goto Done;
- }
- }
-
- if (((Private->Slot[Trb->Slot].CardType == EmmcCardType) &&
- (Packet->SdMmcCmdBlk->CommandIndex == EMMC_SEND_TUNING_BLOCK)) ||
- ((Private->Slot[Trb->Slot].CardType == SdCardType) &&
- (Packet->SdMmcCmdBlk->CommandIndex == SD_SEND_TUNING_BLOCK))) {
- //
- // When performing tuning procedure (Execute Tuning is set to 1) through PIO mode,
- // wait Buffer Read Ready bit of Normal Interrupt Status Register to be 1.
- // Refer to SD Host Controller Simplified Specification 3.0 figure 2-29 for details.
- //
- if ((IntStatus & BIT5) == BIT5) {
- //
- // Clear Buffer Read Ready interrupt at first.
- //
- IntStatus = BIT5;
- SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_NOR_INT_STS, FALSE, sizeof (IntStatus), &IntStatus);
- //
- // Read data out from Buffer Port register
- //
- for (PioLength = 0; PioLength < Trb->DataLen; PioLength += 4) {
- SdMmcHcRwMmio (Private->PciIo, Trb->Slot, SD_MMC_HC_BUF_DAT_PORT, TRUE, 4, (UINT8*)Trb->Data + PioLength);
- }
- Status = EFI_SUCCESS;
- goto Done;
- }
- }

Status = EFI_NOT_READY;
Done:
- //
- // Get response data when the cmd is executed successfully.
- //
- if (!EFI_ERROR (Status)) {
- if (Packet->SdMmcCmdBlk->CommandType != SdMmcCommandTypeBc) {
- for (Index = 0; Index < 4; Index++) {
- Status = SdMmcHcRwMmio (
- Private->PciIo,
- Trb->Slot,
- SD_MMC_HC_RESPONSE + Index * 4,
- TRUE,
- sizeof (UINT32),
- &Response[Index]
- );
- if (EFI_ERROR (Status)) {
- SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
- return Status;
- }
- }
- CopyMem (Packet->SdMmcStatusBlk, Response, sizeof (Response));
- }
- }

if (Status != EFI_NOT_READY) {
SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
--
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


[PATCHv3 1/5] MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces

Albecki, Mateusz
 

To allow for easier debug of failing commands we
have added a capability to print TRB and command
packet when we start execution of the TRB(on
DEBUG_VERBOSE level) and when the TRB failed to
execute correctly(on DEBUG_ERROR level). Additionally
we will also print error interrupt status and interrupt
status register on failed SD command.

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marcin Wojtas <mw@semihalf.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>

Signed-off-by: Mateusz Albecki <mateusz.albecki@intel.com>
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 87 ++++++++++++++++++++++++
1 file changed, 87 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 43626fff48..71cf5a78f9 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1645,6 +1645,82 @@ BuildAdmaDescTable (
return EFI_SUCCESS;
}

+/**
+ Prints the contents of the command packet to the debug port.
+
+ @param[in] DebugLevel Debug level at which the packet should be printed.
+ @param[in] Packet Pointer to packet to print.
+**/
+VOID
+SdMmcPrintPacket (
+ IN UINT32 DebugLevel,
+ IN EFI_SD_MMC_PASS_THRU_COMMAND_PACKET *Packet
+ )
+{
+ if (Packet == NULL) {
+ return;
+ }
+
+ DEBUG ((DebugLevel, "Printing EFI_SD_MMC_PASS_THRU_COMMAND_PACKET\n"));
+ if (Packet->SdMmcCmdBlk != NULL) {
+ DEBUG ((DebugLevel, "Command index: %d, argument: %X\n", Packet->SdMmcCmdBlk->CommandIndex, Packet->SdMmcCmdBlk->CommandArgument));
+ DEBUG ((DebugLevel, "Command type: %d, response type: %d\n", Packet->SdMmcCmdBlk->CommandType, Packet->SdMmcCmdBlk->ResponseType));
+ }
+ if (Packet->SdMmcStatusBlk != NULL) {
+ DEBUG ((DebugLevel, "Response 0: %X, 1: %X, 2: %X, 3: %X\n",
+ Packet->SdMmcStatusBlk->Resp0,
+ Packet->SdMmcStatusBlk->Resp1,
+ Packet->SdMmcStatusBlk->Resp2,
+ Packet->SdMmcStatusBlk->Resp3
+ ));
+ }
+ DEBUG ((DebugLevel, "Timeout: %ld\n", Packet->Timeout));
+ DEBUG ((DebugLevel, "InDataBuffer: %p\n", Packet->InDataBuffer));
+ DEBUG ((DebugLevel, "OutDataBuffer: %p\n", Packet->OutDataBuffer));
+ DEBUG ((DebugLevel, "InTransferLength: %d\n", Packet->InTransferLength));
+ DEBUG ((DebugLevel, "OutTransferLength: %d\n", Packet->OutTransferLength));
+ DEBUG ((DebugLevel, "TransactionStatus: %r\n", Packet->TransactionStatus));
+}
+
+/**
+ Prints the contents of the TRB to the debug port.
+
+ @param[in] DebugLevel Debug level at which the TRB should be printed.
+ @param[in] Trb Pointer to the TRB structure.
+**/
+VOID
+SdMmcPrintTrb (
+ IN UINT32 DebugLevel,
+ IN SD_MMC_HC_TRB *Trb
+ )
+{
+ if (Trb == NULL) {
+ return;
+ }
+
+ DEBUG ((DebugLevel, "Printing SD_MMC_HC_TRB\n"));
+ DEBUG ((DebugLevel, "Slot: %d\n", Trb->Slot));
+ DEBUG ((DebugLevel, "BlockSize: %d\n", Trb->BlockSize));
+ DEBUG ((DebugLevel, "Data: %p\n", Trb->Data));
+ DEBUG ((DebugLevel, "DataLen: %d\n", Trb->DataLen));
+ DEBUG ((DebugLevel, "Read: %d\n", Trb->Read));
+ DEBUG ((DebugLevel, "DataPhy: %lX\n", Trb->DataPhy));
+ DEBUG ((DebugLevel, "DataMap: %p\n", Trb->DataMap));
+ DEBUG ((DebugLevel, "Mode: %d\n", Trb->Mode));
+ DEBUG ((DebugLevel, "AdmaLengthMode: %d\n", Trb->AdmaLengthMode));
+ DEBUG ((DebugLevel, "Event: %p\n", Trb->Event));
+ DEBUG ((DebugLevel, "Started: %d\n", Trb->Started));
+ DEBUG ((DebugLevel, "Timeout: %ld\n", Trb->Timeout));
+ DEBUG ((DebugLevel, "Retries: %d\n", Trb->Retries));
+ DEBUG ((DebugLevel, "Adma32Desc: %p\n", Trb->Adma32Desc));
+ DEBUG ((DebugLevel, "Adma64V3Desc: %p\n", Trb->Adma64V3Desc));
+ DEBUG ((DebugLevel, "Adma64V4Desc: %p\n", Trb->Adma64V4Desc));
+ DEBUG ((DebugLevel, "AdmaMap: %p\n", Trb->AdmaMap));
+ DEBUG ((DebugLevel, "AdmaPages: %X\n", Trb->AdmaPages));
+
+ SdMmcPrintPacket (DebugLevel, Trb->Packet);
+}
+
/**
Create a new TRB for the SD/MMC cmd request.

@@ -2236,6 +2312,10 @@ SdMmcCheckAndRecoverErrors (
return Status;
}

+ DEBUG ((DEBUG_ERROR, "Error reported by SDHCI\n"));
+ DEBUG ((DEBUG_ERROR, "Interrupt status = %X\n", IntStatus));
+ DEBUG ((DEBUG_ERROR, "Error interrupt status = %X\n", ErrIntStatus));
+
//
// If the data timeout error is reported
// but data transfer is signaled as completed we
@@ -2439,6 +2519,13 @@ Done:

if (Status != EFI_NOT_READY) {
SdMmcHcLedOnOff (Private->PciIo, Trb->Slot, FALSE);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "TRB failed with %r\n", Status));
+ SdMmcPrintTrb (DEBUG_ERROR, Trb);
+ } else {
+ DEBUG ((DEBUG_VERBOSE, "TRB success\n"));
+ SdMmcPrintTrb (DEBUG_VERBOSE, Trb);
+ }
}

return Status;
--
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


[PATCHv3 0/5] MdeModulePkg/SdMmcPciHcDxe: Refactor command processing

Albecki, Mateusz
 

This patch series aims to refactor command processing to achieve following

- Trace the failing TRB packets to see what commands are failing and for what reasons
- Get the response data even if data transfer timed out to allow easier debugging
- Fix the PIO mode which is currently completely broken.

Changes in v2:
- Moved verbose packet prints after the command is finished to capture the successfull command response
- Fixed the debug prints
- PIO data will be moved with width matching the alignment of the block size. For majority of transfers that means UINT32 width.

Changes in v3
- Fixed the memory map in non DMA case(PATCHv3 4/5)

Tests performed:
- Each patch in the series has passed boot from eMMC with ADMAv3 data transfer mode
- SDMA based boot has been tested with the full patch series
- PIO based boot has been tested with the full patch series
- PIO based data transfer has been additionally tested by creating and modyfing a file in EFI shell
- Tested async PIO transfer - results below

Tests performed v3:
- Booted OS in ADMA mode(V3 64bit)
- Booted OS in PIO mode

Async test results:
After fixing memory map issue PIO works reliably in both async and sync cases on all paltforms.

All tests were performed with eMMC in HS400 @200MHz clock frequency.

For easier review & integration patch has been pushed here:
Whole series: https://github.com/malbecki/edk2/tree/emmc_transfer_refactor

Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Marcin Wojtas <mw@semihalf.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>


Mateusz Albecki (5):
MdeModulePkg/SdMmcPciHcDxe: Enhance driver traces
MdeModulePkg/SdMmcPciHcDxe: Read response on command completion
MdeModulePkg/SdMmcPciHcDxe: Refactor data transfer completion
MdeModulePkg/SdMmcPciHcDxe: Do not map memory for non DMA transfer
MdeModulePkg/SdMmcPciHcDxe: Fix PIO transfer mode

MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 4 +
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 609 ++++++++++++++++-----
2 files changed, 478 insertions(+), 135 deletions(-)

--
2.14.1.windows.1

--------------------------------------------------------------------

Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.


Re: Patch List for 202002 stable tag

Laszlo Ersek
 

On 02/27/20 17:23, Leif Lindholm wrote:
Hi Liming,

On Thu, Feb 27, 2020 at 16:06:22 +0000, Liming Gao wrote:
Stewards:
I update the patch lists and status. Based on current information,
only one patch (item 5) needs catch this stable tag. Its fix is
clear, and risk is low. So, I think we can still keep current
planning to create stable tag edk2-stable202002 on 2020 Feb 28th
(UTC – 8). If you think the stable tag needs to be delay for few
days, please reply the mail before Feb 28th (00:00:00 UTC-8).

1. https://edk2.groups.io/g/devel/message/54665 [edk2-devel] [PATCH v2 1/1] EmbeddedPkg: Fixed Asserts in SCT Runtime Services test.
[Liming] This patch is still under review. So, it will not catch this stable tag.

1. https://edk2.groups.io/g/devel/message/54693 [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.
Unrelated to the release process, only the formatting:
It looks like you are doing ordered lists using markdown syntax
(1.). This renders in plain text email simply as all items being 1.

[Liming] The patch has passed review. Package maintainer thinks this is an enhancement. It will be added after stable tag.

1. https://edk2.groups.io/g/devel/message/54122 [PATCH 1/1] ShellPkg: Add support for input with separately reported modifiers
[Liming] The discussion shows this change needs UEFI spec clarification. So, it may not be resolved in short team. It will not catch this stable tag. The discussion is in BZ 2510.

1. https://edk2.groups.io/g/devel/message/54797 [PATCH 0/2] UefiCpuPkg/Library: Fix bug in MpInitLib
[Liming] The solution is under discussion (BZ 2556). The submitter requests this issue to be fixed happen reasonably soon. So, it may not catch this stable tag.

1. https://edk2.groups.io/g/devel/message/54992 [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build
[Liming] This patch has passed review. This regression causes the basic incremental build not work with VS nmake tool. The fix is clear. So, it need to catch this stable tag.
I agree it needs to catch the stable tag. If it affects only VS builds
then I am not going to insist on extending the hard freeze, but I
(technically on holiday today/tomorrow) don't have time to dig much
deeper into it.

However, I think the process is pretty clear that this *should* extend
the hard freeze.

I will note that from the trail (commitdate of 818283de3f6d until
BZ2563 was raised) it appears that detecting this bug itself, which
went in two days before the soft freeze, took 15 days.
I agree with Liming's analysis on the patches (i.e., what goes in and
what gets postponed), and I agree with Leif that we should extend the
hard freeze by at least a couple of days.

This is not unusual. Originally I thought that edk2 freeze and release
dates were set in stone, but then Mike explained to me that that had
never been the intent. And other open source projects do several
pre-releases (rc0, rc1, .... pre-releases with "release critical" (rc)
bug fixes), before a final release. For example, QEMU regularly plans
rc0..rc2 or even rc3, and then *optionally* adds an rc4 if even rc3
receives significant bugfixes. The idea is that the final release / tag
should be preceded by a silent / calm period, where we've waited a few
days and become reasonably convinced that "OK, there's nothing else we
should obviously fix right now".

I wouldn't immediately suggest a full week extension, but maybe until
March 4th (middle of next week)?

Thanks
Laszlo


Re: [Patch 1/1][edk2-stable202002]BaseTools: Fixed a regression issue in Makefile for incremental build

Laszlo Ersek
 

On 02/27/20 16:53, Feng, Bob C wrote:

[Bob] I agree the BZ status should be update in time. I don't think BZ status update is the reviewer's/maintainer's responsibility, the BZ owner should be responsible for it.
Agreed.


NOTE: GitHub.com Pull Requests would not help *at all* in the face of such sloppiness; even on GitHub.com, people have to at least *name* issue numbers in commit messages.

- TianoCore#2563 (which tracks the regression) identifies *neither* the BZ for which the regression was introduced (2481), *nor* the faulty commit (818283de3f6d). You realize it's *completely useless* to file BZs with such negligence, right? It has no more information than "stuff broke, we need to fix it" -- but ain't that the general state of things, at all times? Are you only trying to fill a BZ quota?
[Bob] I don't agree this comments.
I added the bug reproduce steps in BZ description. I think it's enough when I submit a new BZ. I'll append the root cause and solution ( would be just patch review link) in its comments when I update the BZ status later.
Yes, the patch explains the issue well. If the link had been in the BZ,
I wouldn't have complained (as much).

We found this critical bug in this afternoon (PRC time) and root cause and created patch very quickly. I don't think that I did not update the BZ in time is process violation.
It was not clear that you ever intended to add the link to the BZ.

I think the necessary information was provided when the patch send out. The bug description and reproduce steps are in BZ, root cause is in the patch commit message, the solution is the patch itself, test result is in the commit message.
Yes. There was no link from the BZ to the patch however. And it wasn't
possible to determine whether you were going to add the link later. I
tend to add the link immediately after posting, so I don't forget. My
experience tell me that most patch submitters that don't add the link at
once forget for good.

Yes, it was a generalization, sorry about that.

One thing I do have to admit (because I brought up GitHub.com before) is
that, on GitHub.com, if you submit a pull request, and at least one of
the commit messages references an Issue (like your commit message here
references TianoCore#2563), then the issue automatically gets an update.
So in that regard GitHub.com does save some manual work.

Laszlo


Re: [PATCH v3 5/6] MdeModulePkg/DxeCore: defer PE/COFF emulator registration to StartImage

Ard Biesheuvel
 

On Wed, 26 Feb 2020 at 20:43, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

EDK2's implementation of the LoadImage() boot service permits non-native
binaries to be loaded (i.e., X64 images on IA32 firmware), but any
attempts to start such an image using StartImage() will return
EFI_UNSUPPORTED.

The integration of the PE/COFF emulator protocol into the DXE core
deviates slightly from this paradigm, given that its IsImageSupported
hook as well as its RegisterImage hook are invoked from LoadImage,
and by the time StartImage is called, no opportunity is given to the
provider of the PE/COFF emulator protocol to prevent an image from
being started if it only supports loading it.

To address this disparity, let's move the invocation of RegisterImage()
to the implementation of the StartImage() boot service, allowing the
emulator to permit LoadImage() but reject StartImage() on images that
turn out not to meet the requirements of the emulator as it is being
started.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
@MdeModulePkg maintainers: do you have any comments on this patch?


---
MdeModulePkg/Core/Dxe/Image/Image.c | 24 +++++++++++---------
1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c b/MdeModulePkg/Core/Dxe/Image/Image.c
index 22a87ecf6d7c..d86da89ee704 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -756,17 +756,6 @@ CoreLoadPeImage (
// Get the image entry point.
//
Image->EntryPoint = (EFI_IMAGE_ENTRY_POINT)(UINTN)Image->ImageContext.EntryPoint;
- if (Image->PeCoffEmu != NULL) {
- Status = Image->PeCoffEmu->RegisterImage (Image->PeCoffEmu,
- Image->ImageBasePage,
- EFI_PAGES_TO_SIZE (Image->NumberOfPages),
- &Image->EntryPoint);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
- "CoreLoadPeImage: Failed to register foreign image with emulator.\n"));
- goto Done;
- }
- }

//
// Fill in the image information for the Loaded Image Protocol
@@ -1603,6 +1592,19 @@ CoreStartImage (
return EFI_UNSUPPORTED;
}

+ if (Image->PeCoffEmu != NULL) {
+ Status = Image->PeCoffEmu->RegisterImage (Image->PeCoffEmu,
+ Image->ImageBasePage,
+ EFI_PAGES_TO_SIZE (Image->NumberOfPages),
+ &Image->EntryPoint);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_LOAD | DEBUG_ERROR,
+ "CoreLoadPeImage: Failed to register foreign image with emulator - %r\n",
+ Status));
+ return Status;
+ }
+ }
+
PERF_START_IMAGE_BEGIN (Handle);


--
2.17.1