Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29] MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

2019-10-01 Thread Abner Chang 



> -Original Message-
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, October 1, 2019 5:07 PM
> To: Philippe Mathieu-Daudé 
> Cc: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> 
> Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29]
> MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.
> 
> On Tue, Oct 01, 2019 at 10:49:38AM +0200, Philippe Mathieu-Daudé wrote:
> > Hi Leif,
> >
> > On 9/27/19 1:39 AM, Leif Lindholm wrote:
> > > On Mon, Sep 23, 2019 at 08:31:35AM +0800, Abner Chang wrote:
> > > > RISC-V MMIO library instance.  RISC-V only supports memory map I/O.
> > >
> > > We need fewer, not more, C implementations of MMIO accessors.
> > > While this set doesn't need to wait for upstream to get sorted,
> > > please just use IoLibArm.c which should be completely equivalent to
> > > what you have implemented here.
> >
> > This shows this file name is misleading. However I can't come with a
> > clever one :/
> 
> This has been discussed before, only the current situation "works", so sorting
> it out never takes priority (I know it doesn't for me).
> 
> There should be exactly one variant of IoLib.c. Well, these days we need a
> separate one for ARM/AARCH64 under hw virtualization.
> 
> IoLibArm, IoLibEbc and IoLibRiscV have *exactly* the same requirements.
> And now x86 uses NASM regardless of build platform, I think it would make
> sense to move the contents of IoLibGcc and IoLibMsc into assembler.

That looks weird and doesn't make sense to use Arm code for RISC-V even the 
functionality is exactly the same to IoLibRiscV. I will still keep it as 
IoLibRiscV.c until there is a generic IoLib for different arch.
.
> 
> /
> Leif
> 
> > > > Signed-off-by: Abner Chang 
> > > > ---
> > > >   .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf  |   8 +-
> > > >   MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c | 601
> +
> > > >   2 files changed, 607 insertions(+), 2 deletions(-)
> > > >   create mode 100644
> > > > MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48359): https://edk2.groups.io/g/devel/message/48359
Mute This Topic: https://groups.io/mt/34258203/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms][PATCH V1 1/1] Readme.md: Add a reference to SimicsOpenBoardPkg

2019-10-01 Thread Kubacki, Michael A 
Adds a link to SimicsOpenBoardPkg in the list of supported Intel
platforms.

Cc: Wei David Y 
Cc: Agyeman Prince 
Cc: Nate DeSimone 
Signed-off-by: Michael Kubacki 
---
 Readme.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Readme.md b/Readme.md
index efdbe8523f..2b54b8e0b4 100644
--- a/Readme.md
+++ b/Readme.md
@@ -227,6 +227,7 @@ they will be documented with the platform.
 ### [Minimum Platforms](Platform/Intel/Readme.md)
 * [Kaby Lake](Platform/Intel/KabylakeOpenBoardPkg)
 * [Purley](Platform/Intel/PurleyOpenBoardPkg)
+* [Simics](Platform/Intel/SimicsOpenBoardPkg)
 * [Whiskey Lake](Platform/Intel/WhiskeylakeOpenBoardPkg)
 
 For more information, see the
-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48358): https://edk2.groups.io/g/devel/message/48358
Mute This Topic: https://groups.io/mt/34364927/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH V1 1/1] BaseTools: Fix GenMake multi-workspace failure

2019-10-01 Thread Kubacki, Michael A 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2232

Commit 0075ab2cec introduced an issue that causes an exception
when multiple workspace packages paths are specified. For example,
if edk2-platforms is used, the root directory will contain an edk
and edk2-platforms directory representing the respective
repositories.

In GenMake, the path to the package DEC file for a module is
discovered by getting the relative path of the INF to the
workspace root directory. Each directory in the relative path
is incrementally joined to the WORKSPACE directory. The file
list in the joined path is searched for a DEC file.

As an example, if the build command is used on a package outside
the edk2 repository, the INF file path is relative to the
edk2-platforms directory not edk2. This causes directory paths
to be built that do not exist. Commit 0075ab2cec replaced the
os.path.exists() call with a try except block that always fails
when os.listdir() is invoked to enumerate the list of files in
the built directory path on packages outside edk2.

This commit restores the original conditional statement which
avoids calling os.listdir() with an invalid directory path.

Cc: Bob Feng 
Cc: Liming Gao 
Cc: Michael D Kinney 
Signed-off-by: Michael Kubacki 
---
 BaseTools/Source/Python/AutoGen/GenMake.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py 
b/BaseTools/Source/Python/AutoGen/GenMake.py
index 584156dab9..97ba158ff2 100755
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -637,13 +637,11 @@ cleanlib:
 while not found and os.sep in package_rel_dir:
 index = package_rel_dir.index(os.sep)
 current_dir = mws.join(current_dir, package_rel_dir[:index])
-try:
+if os.path.exists(current_dir):
 for fl in os.listdir(current_dir):
 if fl.endswith('.dec'):
 found = True
 break
-except:
-EdkLogger.error('build', FILE_NOT_FOUND, "WORKSPACE does not 
exist.")
 package_rel_dir = package_rel_dir[index + 1:]
 
 MakefileTemplateDict = {
-- 
2.16.2.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48357): https://edk2.groups.io/g/devel/message/48357
Mute This Topic: https://groups.io/mt/34364042/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2-platforms] Remove ClevoOpenBoardPkg from Bugzilla

2019-10-01 Thread Kubacki, Michael A 
Hi Mike,

ClevoOpenBoardPkg was removed from edk2-platforms in commit 
1dc2b96e767f7d1289f90539b6be162430225043.

The single board supported in the package (N1xxWU) was moved to 
KabylakeOpenBoardPkg (as GalagoPro3). Any future bugs for this board should be 
filed against KabylakeOpenBoardPkg.

All Readme files have been updated to reflect this change and all Tianocore 
Bugzilla issues have been moved to KabylakeOpenBoardPkg. Can the 
ClevoOpenBoardPkg component be removed from Bugzilla?

Thanks,
Michael


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48356): https://edk2.groups.io/g/devel/message/48356
Mute This Topic: https://groups.io/mt/34363609/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

2019-10-01 Thread Spottswood, Jason 
Hey Mike,

Specifically, my team has run into ASSERTs on systems loaded with many 
identical model HDDs.  Each HDD provides an FMP where the type GUID is 
identical, and the HW instance is not provided (zero).  Attached serial log 
with additional debug output to print the FMP version. (We commented out the 
ASSERT to gather the log.)

Given the current text in the UEFI spec, I do not believe the device vendor is 
necessarily at fault here, even if I would like for them to implement the HW 
instance.


  1.  First sentence from the FW image HW instance description says this field 
is optional.
 ///
  /// An optional number to identify the unique hardware instance within the 
system for
  /// devices that may have multiple instances (Example: a plug in pci network 
card). This


  1.  Near the end of the FW image HW instance description says that an FMP can 
use zero to mean uniqueness cannot be determined.
 /// instance a zero can be used. A zero means the FMP provider is not able to 
determine a
  /// unique hardware instance number or a hardware instance number is not 
needed. Only
  /// present in version 3 or higher.

Another point is that the ESRT can only have one entry for each given type 
GUID.  There is also no HW instance field for entries in the ESRT.  As it 
relates to building the ESRT, checking for duplicate type GUID/IDs is the only 
requirement, not HW instance.

-Jason

From: Kinney, Michael D 
Sent: Tuesday, October 1, 2019 12:31 PM
To: devel@edk2.groups.io; Spottswood, Jason ; 
Rothman, Michael A ; Kinney, Michael D 

Subject: RE: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Hi Jason,

I believe the logic to check for uniqueness of FMP Descriptor is correct.

The UEFI Spec has 2 structs with HardwareInstance.  One is FMP Descriptor and 
the other is UEFI Capsule for FMP.

The HardwareInstance in FMP must be unique and not 0 unless there is a 
guarantee there is only one instance.

  ///
  /// An optional number to identify the unique hardware instance within the 
system for
  /// devices that may have multiple instances (Example: a plug in pci network 
card). This
  /// number must be unique within the namespace of the ImageTypeId GUID and
  /// ImageIndex. For FMP instances that have multiple descriptors for a single
  /// hardware instance, all descriptors must have the same HardwareInstance 
value.
  /// This number must be consistent between boots and should be based on some 
sort of
  /// hardware identified unique id (serial number, etc) whenever possible. If 
a hardware
  /// based number is not available the FMP provider may use some other 
characteristic
  /// such as device path, bus/dev/function, slot num, etc for generating the
  /// HardwareInstance. For implementations that will never have more than one
  /// instance a zero can be used. A zero means the FMP provider is not able to 
determine a
 /// unique hardware instance number or a hardware instance number is not 
needed. Only
  /// present in version 3 or higher.
  ///
  UINT64   HardwareInstance;
} EFI_FIRMWARE_IMAGE_DESCRIPTOR;

The UEFI Capsule one can be 0 to specify match any.

  ///
  /// The HardwareInstance to target with this update. If value is zero it 
means match all
  /// HardwareInstances. This field allows update software to target only a 
single device in
  /// cases where there are more than one device with the same ImageTypeId GUID.
  /// This header is outside the signed data of the Authentication Info 
structure and
  /// therefore can be modified without changing the Auth data.
  ///
  UINT64   UpdateHardwareInstance;
} EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;

Best regards,

Mike


From: devel@edk2.groups.io 
mailto:devel@edk2.groups.io>> On Behalf Of Spottswood, 
Jason
Sent: Monday, September 30, 2019 2:24 PM
To: Rothman, Michael A 
mailto:michael.a.roth...@intel.com>>; 
devel@edk2.groups.io
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Cut-n-paste problem. My apologies.

Copying again in plain text:

  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  for (Index = 0; Index < *NumberOfDescriptors; Index++) {
if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, 
&FmpImageInfoBuf->ImageTypeId)) {
  if (HardwareInstances[Index].HardwareInstance == FmpHardwareInstance) {
DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descriptor 
with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, 
FmpHardwareInstance));
ASSERT (
  !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, 
&FmpImageInfoBuf->ImageTypeId) ||
  HardwareInstances[Index].HardwareInstance != FmpHardwareInstance
  );
return EFI_UNSUPPORTED;
  }
}
  }

-Jason

From: Rothman, Michael A 
mailto:michael.a.roth...@intel.com>>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io

Re: [edk2-devel] [plaforms/devel-riscv-v2 PATCHv2 02/14] Silicon/SiFive: Add library module of SiFive RISC-V cores

2019-10-01 Thread Leif Lindholm 
On Thu, Sep 19, 2019 at 11:51:19AM +0800, Gilbert Chen wrote:
> Initial version of SiFive RISC-V core libraries. Library of each core
>  creates processor core SMBIOS data hob for building SMBIOS
>  records in DXE phase.

So yes, this implementation needs to change.
These should all implement the same LibraryClass.
Also, U54 appears to be a simple superset of U51.

What I would suggest is creating a
Silicon/SiFive/Library/SiFiveCoreInfoLib, which calls into a
SiFiveSoCCoreInfoLib in Silicon/SiFive//Library, providing the
acual SoC-specific bits.

/
Leif

> Signed-off-by: Gilbert Chen 
> ---
>  .../E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c| 242 +
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|  51 
>  .../U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c| 294 
> +
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|  51 
>  .../Library/PeiCoreInfoHobLib/CoreInfoHob.c| 185 +
>  .../PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf|  50 
>  6 files changed, 873 insertions(+)
>  create mode 100644 Silicon/SiFive/E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>  create mode 100644 
> Silicon/SiFive/E51/Library/PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf
>  create mode 100644 Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>  create mode 100644 
> Silicon/SiFive/U54/Library/PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf
>  create mode 100644 
> Silicon/SiFive/U54MCCoreplex/Library/PeiCoreInfoHobLib/CoreInfoHob.c
>  create mode 100644 
> Silicon/SiFive/U54MCCoreplex/Library/PeiCoreInfoHobLib/PeiCoreInfoHobLib.inf

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48354): https://edk2.groups.io/g/devel/message/48354
Mute This Topic: https://groups.io/mt/34196349/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Qemu-devel] [PATCH 1/2] q35: implement 128K SMRAM at default SMBASE address

2019-10-01 Thread Laszlo Ersek 
On 09/30/19 16:22, Yao, Jiewen wrote:
> 
>> -Original Message-
>> From: devel@edk2.groups.io  On Behalf Of Igor
>> Mammedov
>> Sent: Monday, September 30, 2019 8:37 PM
>> To: Laszlo Ersek 

>>> To me it looks like we need to figure out how QEMU can make the OS call
>>> into SMM (in the GPE cpu hotplug handler), passing in parameters and
>>> such. This would be step (03).
>>>
>>> Do you agree?
>>>
>>> If so, I'll ask Jiewen about such OS->SMM calls separately, because I
>>> seem to remember that there used to be an "SMM communcation table" of
>>> sorts, for flexible OS->SMM calls. However, it appears to be deprecated
>>> lately.
>> we can try to resurrect and put over it some kind of protocol
>> to describe which CPUs to where hotplugged.
>>
>> or we could put a parameter into SMI status register (IO port 0xb3)
>> and the trigger SMI from GPE handler to tell SMI handler that cpu
>> hotplug happened and then use QEMU's cpu hotplug interface
>> to enumerate hotplugged CPUs for SMI handler.
>>
>> The later is probably simpler as we won't need to reinvent the wheel
>> (just reuse the interface that's already in use by GPE handler).

Based on "docs/specs/acpi_cpu_hotplug.txt", this seems to boil down to a
bunch of IO port accesses at base 0x0cd8.

Is that correct?

> [Jiewen] The PI specification Volume 4 - SMM defines 
> EFI_MM_COMMUNICATION_PROTOCOL.Communicate() - It can be used to communicate 
> between OS and SMM handler. But it requires the runtime protocol call. I am 
> not sure how OS loader passes this information to OS kernel.
> 
> As such, I think using ACPI SCI/GPE -> software SMI handler is an easier way 
> to achieve this. I also recommend this way.
> For parameter passing, we can use 1) Port B2 (1 byte), 2) Port B3 (1 byte), 
> 3) chipset scratch register (4 bytes or 8 bytes, based upon scratch register 
> size), 4) ACPI NVS OPREGION, if the data structure is complicated.

I'm confused about the details. In two categories:
(1) what values to use,
(2) how those values are passed.

Assume a CPU is hotpluged, QEMU injects an SCI, and the ACPI GPE handler
in the OS -- which also originates from QEMU -- writes a particular byte
to the Data port (0xB3), and then to the Control port (0xB2),
broadcasting an SMI.

(1) What values to use.

Note that values ICH9_APM_ACPI_ENABLE (2) and ICH9_APM_ACPI_DISABLE (3)
are already reserved in QEMU for IO port 0xB2, for different purposes.
So we can't use those in the GPE handler.

Furthermote, OVMF's EFI_SMM_CONTROL2_PROTOCOL.Trigger() implementation
(in "OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c") writes 0 to both ports,
as long as the caller does not specify them.

  IoWrite8 (ICH9_APM_STS, DataPort== NULL ? 0 : *DataPort);
  IoWrite8 (ICH9_APM_CNT, CommandPort == NULL ? 0 : *CommandPort);

And, there is only one Trigger() call site in edk2: namely in
"MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c", in the
SmmCommunicationCommunicate() function.

(That function implements EFI_SMM_COMMUNICATION_PROTOCOL.Communicate().)
This call site passes NULL for both DataPort and CommandPort.

Yet further, EFI_SMM_COMMUNICATION_PROTOCOL.Communicate() is used for
example by the UEFI variable driver, for talking between the
unprivileged (runtime DXE) and privileged (SMM) half.

As a result, all of the software SMIs currently in use in OVMF, related
to actual firmware services, write 0 to both ports.

I guess we can choose new values, as long as we avoid 2 and 3 for the
control port (0xB2), because those are reserved in QEMU -- see
ich9_apm_ctrl_changed() in "hw/isa/lpc_ich9.c".


(2) How the parameters are passed.


(2a) For the new CPU, the SMI remains pending, until it gets an
INIT-SIPI-SIPI from one of the previously plugged CPUs (most likely, the
BSP). At that point, the new CPU will execute the "initial SMI handler
for hotplugged CPUs", at the default SMBASE.

That's a routine we'll have to write in assembly, from zero. In this
routine, we can read back IO ports 0xB2 and 0xB3. And QEMU will be happy
to provide the values last written (see apm_ioport_readb() in
"hw/isa/apm.c"). So we can receive the values in this routine. Alright.


(2b) On all other CPUs, the SMM foundation already accepts the SMI.

There point where it makes sense to start looking is SmmEntryPoint()
[MdeModulePkg/Core/PiSmmCore/PiSmmCore.c].

(2b1) This function first checks whether the SMI is synchronous. The
logic for determining that is based on
"gSmmCorePrivate->CommunicationBuffer" being non-NULL. This field is set
to non-NULL in SmmCommunicationCommunicate() -- see above, in (1).

In other words, the SMI is deemed synchronous if it was initiated with
EFI_SMM_COMMUNICATION_PROTOCOL.Communicate(). In that case, the
HandlerType GUID is extracted from the communication buffer, and passed
to SmiManage(). In turn, SmiManage() locates the SMI handler registered
with the same handler GUID, and delegates the SMI handling to that
specific handler.

This is how the UEFI variable driver is split in two halves

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

2019-10-01 Thread Michael D Kinney 
Hi Jason,

I believe the logic to check for uniqueness of FMP Descriptor is correct.

The UEFI Spec has 2 structs with HardwareInstance.  One is FMP Descriptor and 
the other is UEFI Capsule for FMP.

The HardwareInstance in FMP must be unique and not 0 unless there is a 
guarantee there is only one instance.

  ///
  /// An optional number to identify the unique hardware instance within the 
system for
  /// devices that may have multiple instances (Example: a plug in pci network 
card). This
  /// number must be unique within the namespace of the ImageTypeId GUID and
  /// ImageIndex. For FMP instances that have multiple descriptors for a single
  /// hardware instance, all descriptors must have the same HardwareInstance 
value.
  /// This number must be consistent between boots and should be based on some 
sort of
  /// hardware identified unique id (serial number, etc) whenever possible. If 
a hardware
  /// based number is not available the FMP provider may use some other 
characteristic
  /// such as device path, bus/dev/function, slot num, etc for generating the
  /// HardwareInstance. For implementations that will never have more than one
  /// instance a zero can be used. A zero means the FMP provider is not able to 
determine a
  /// unique hardware instance number or a hardware instance number is not 
needed. Only
  /// present in version 3 or higher.
  ///
  UINT64   HardwareInstance;
} EFI_FIRMWARE_IMAGE_DESCRIPTOR;

The UEFI Capsule one can be 0 to specify match any.

  ///
  /// The HardwareInstance to target with this update. If value is zero it 
means match all
  /// HardwareInstances. This field allows update software to target only a 
single device in
  /// cases where there are more than one device with the same ImageTypeId GUID.
  /// This header is outside the signed data of the Authentication Info 
structure and
  /// therefore can be modified without changing the Auth data.
  ///
  UINT64   UpdateHardwareInstance;
} EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER;

Best regards,

Mike


From: devel@edk2.groups.io  On Behalf Of Spottswood, Jason
Sent: Monday, September 30, 2019 2:24 PM
To: Rothman, Michael A ; devel@edk2.groups.io
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Cut-n-paste problem. My apologies.

Copying again in plain text:

  //
  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique
  //
  for (Index = 0; Index < *NumberOfDescriptors; Index++) {
if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, 
&FmpImageInfoBuf->ImageTypeId)) {
  if (HardwareInstances[Index].HardwareInstance == FmpHardwareInstance) {
DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descriptor 
with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, 
FmpHardwareInstance));
ASSERT (
  !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, 
&FmpImageInfoBuf->ImageTypeId) ||
  HardwareInstances[Index].HardwareInstance != FmpHardwareInstance
  );
return EFI_UNSUPPORTED;
  }
}
  }

-Jason

From: Rothman, Michael A 
mailto:michael.a.roth...@intel.com>>
Sent: Monday, September 30, 2019 3:53 PM
To: devel@edk2.groups.io; Spottswood, Jason 
mailto:jason.spottsw...@hpe.com>>
Subject: Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

Jason, Agreed - though that image you sent was challenging for these old eyes. 
Black on dark grey? Ack!
Thanks,
Michael A. Rothman
---
Let no excuse be a barrier to your success.

On Sep 30, 2019, at 1:48 PM, 
"jason.spottsw...@hpe.com" 
mailto:jason.spottsw...@hpe.com>> wrote:
In EsrtFmp.c, function CreateEsrtEntry, line 196, the code asserts if the FMP 
image hardware instance matches that of an existing instance.  This is fine if 
the hardware instance is supported.  The field is optional though.  In the UEFI 
spec, "a zero hardware instance means the FMP provider is not able to determine 
a unique hardware instance number or a hardware instance number is not needed." 
 The code below needs to also check if the hardware instance is supported (by 
comparing it to zero) before checking it against existing entries.

  //

  // Check to see of FmpImageInfoBuf GUID/HardwareInstance is unique

  //

  for (Index = 0; Index < *NumberOfDescriptors; Index++) {

if (CompareGuid (&HardwareInstances[Index].ImageTypeGuid, 
&FmpImageInfoBuf->ImageTypeId)) {

  if (HardwareInstances[Index].HardwareInstance == FmpHardwareInstance) {

DEBUG ((DEBUG_ERROR, "EsrtFmpDxe: Duplicate firmware image descriptor 
with GUID %g HardwareInstance:0x%x\n", &FmpImageInfoBuf->ImageTypeId, 
FmpHardwareInstance));

ASSERT (

  !CompareGuid (&HardwareInstances[Index].ImageTypeGuid, 
&FmpImageInfoBuf->ImageTypeId) ||

  HardwareInstances[Index].HardwareInstance != FmpHardwareInstance

Re: [edk2-devel] Recent changes to EsrtFmp causing ASSERTs

2019-10-01 Thread Sean via Groups.Io 
Jason/Mike,

What would you propose is done if the hw instance in 0 but an entry with same 
guid and instance 0 already exists?
Ignore it and first in wins?

It asserts because i am not aware of the right answer for the ESRT and it 
creates a situation with unpredictable results for capsule processing using 
ESRT.

Thanks
Sean

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48351): https://edk2.groups.io/g/devel/message/48351
Mute This Topic: https://groups.io/mt/34350126/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature

2019-10-01 Thread Laszlo Ersek 
On 09/27/19 13:35, Igor Mammedov wrote:
> On Tue, 24 Sep 2019 13:34:55 +0200
> "Laszlo Ersek"  wrote:

>> Going forward, if I understand correctly, the plan is to populate the
>> new SMRAM with the hotplug SMI handler. (This would likely be put in
>> place by OvmfPkg/PlatformPei, from NASM source code. The
>> SmmRelocateBases() function in UefiCpuPkg/PiSmmCpuDxeSmm backs up the
>> original contents before, and restores them after, the initial SMBASE
>> relocation of the cold-plugged VCPUs. Thus the hotplug SMI handler would
>> survive the initial relocation of the cold-plugged VCPUs.)

> that's the tricky part, can we safely detect (in SmmRelocateBases)
> race condition in case of several hotplugged CPUs are executing
> SMI relocation handler at the same time? (crashing system in case
> that happens is good enough)

Wait, there are *two* initial SMI handlers here, one for cold-plugged
CPUs, and another for hot-plugged CPUs. Let's just call them coldplug
and hotplug handlers, for simplicity.

In chronological order, during boot:

(1) OvmfPkg/PlatformPei would put the hotplug handler in place, at the
default SMBASE. This code would lie dormant for a long time.

(2) UefiCpuPkg/PiSmmCpuDxeSmm backs up the current contents of the
default SMBASE area, and installs the coldplug handler. This handler is
then actually used, for relocating SMBASE for the present (cold-plugged)
CPUs. Finally, UefiCpuPkg/PiSmmCpuDxeSmm restores the original contents
of the default SMBASE area. This would in effect re-establish the
hotplug handler from (1).

(3) At OS runtime, a CPU is hotplugged, and then the hotplug handler
runs, at the default SMBASE.

The function SmmRelocateBases() is strictly limited to step (2), and has
nothing to do with hotplug. Therefore it need not deal with any race
conditions related to multi-hotplug.

This is just to clarify that your question applies to the initial SMI
handler (the hotplug one) that is put in place in step (1), and then
used in step (3). The question does not apply to SmmRelocateBases().


With that out of the way, I do think we should be able to detect this,
with a simple -- haha, famous last words! -- compare-and-exchange (LOCK
CMPXCHG); a kind of semaphore.

The initial value of an integer variable in SMRAM should be 1. At the
start of the hotplug initial SMI handler, the CPU should try to swap
that with 0. If it succeeds, proceed. If it fails (the variable's value
is not 1), force a platform reset. Finally, right before the RSM,
restore 1 (swap it back).

(It does not matter if another hotplug CPU starts the relocation in SMM
while the earlier one is left with *only* the RSM instruction in SMM,
immediately after the swap-back.)

Alternatively, if it's friendlier or simpler, we can spin on the initial
LOCK CMPXCHG until it succeeds, executing PAUSE in the loop body. That
should be friendly with KVM too (due to pause loop exiting).


>> Further, when a VCPU is hot-plugged, we seem to intend to raise an SMI
>> for it (perhaps internally to QEMU?),

> I'd rather rise SMI from guest side (using IO port write from
> ACPI cpu hotplug handler). Which in typical case (QEMU negotiated
> SMI broadcast) will trigger pending SMI on hotplugged CPU as well.
> (if it's acceptable I can prepare corresponding QEMU patches)

What if the OS does not send the SMI (from the GPE handler) at once, and
boots the new CPU instead?

Previously, you wrote that the new CPU "won't get access to privileged
SMRAM so OS can't subvert firmware. The next time SMI broadcast is sent
the CPU will use SMI handler at default 3 SMBASE. It's up to us to
define behavior here (for example relocation handler can put such CPU in
shutdown state)."

How can we discover in the hotplug initial SMI handler at 0x3_ that
the CPU executing the code should have been relocated *earlier*, and the
OS just didn't obey the ACPI GPE code? I think a platform reset would be
a proper response, but I don't know how to tell, "this CPU should not be
here".


> In case of unicast SMIs, a CPU handling guest ACPI triggered
> SMI, can send SMI to hotpluged CPU as an additional step.

I think it's OK if we restrict this feature to "broadcast SMI" only.

> 
>> which will remain pending until
>> the VCPU is launched with INIT-SIPI-SIPI. Once the INIT-SIPI-SIPI is
>> sent, the VCPU will immediately enter SMM, and run the SMI handler in
>> the locked SMRAM at the default SMBASE. At RSM, it may continue at the
>> vector requested in the INIT-SIPI-SIPI.
> Firmware can arbitrate/send INIT-SIPI-SIPI to hotplugged CPUs.
> Enumerating hotplugged CPUs, can be done by reusing CPU hotplug
> interface described at QEMU/docs/specs/acpi_cpu_hotplug.txt.
> (preferably in read-only mode)

... I'll have to look at that later :)

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48350): https://edk2.groups.io/g/devel/message/48350
Mute This Topic: https://groups.io/mt/34274934/21656
Group Owner:

[edk2-devel] Upcoming Event: TianoCore Design / Bug Triage - EMEA - Wed, 10/02/2019 8:00am-9:00am #cal-reminder

2019-10-01 Thread devel@edk2.groups.io Calendar 
*Reminder:* TianoCore Design / Bug Triage - EMEA

*When:* Wednesday, 2 October 2019, 8:00am to 9:00am, (GMT-07:00) America/Los 
Angeles

*Where:* https://zoom.us/j/695893389

View Event ( https://edk2.groups.io/g/devel/viewevent?eventid=503244 )

*Organizer:* Stephano Cetola stephano.cet...@linux.intel.com ( 
stephano.cet...@linux.intel.com?subject=Re:%20Event:%20TianoCore%20Design%20%2F%20Bug%20Triage%20-%20EMEA
 )

*Description:*

Join Zoom Meeting

https://zoom.us/j/695893389

One tap mobile

+17207072699,,695893389# US

+16465588656,,695893389# US (New York)

Dial by your location

+1 720 707 2699 US

+1 646 558 8656 US (New York)

Meeting ID: 695 893 389

Find your local number: https://zoom.us/u/abOtdJckxL

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48349): https://edk2.groups.io/g/devel/message/48349
Mute This Topic: https://groups.io/mt/34358661/21656
Mute #cal-reminder: https://groups.io/mk?hashtag=cal-reminder&subid=3846945
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at default SMBASE" feature

2019-10-01 Thread Laszlo Ersek 
(+David)

On 09/27/19 03:14, Yao, Jiewen wrote:
> Sounds good.
> 
> Maybe you can write that info in the commit message. A simple sentence such 
> as :
> No CSM support is needed because legacy BIOS boot don't use SMM.
> 
> So other people won't have same question in the future.

I've given this more thought. I can't decide what's the best approach,
without David's input.

David, here's the problem statement, stripped down: with a new QEMU
feature that's dynamically detected and enabled by the firmware, the
128KB memory area at 0x3_ becomes inaccessible to the OS. (This
subfeature is necessary for enabling the larger "VCPU hotplug with SMM"
feature). Right now, this patch series communcates that fact to the UEFI
OS through the UEFI memory map, but it's not told to a legacy OS through
the E820 memmap.

Which option do you prefer?

(1) The feature depends on building OVMF with -D SMM_REQUIRE. We can say
that -D SMM_REQUIRE and -D CSM_ENABLE are mutually exclusive, and add an
!error directive to the DSC files to catch a build that is passed both
-D flags.

This option is the most convenient for me, but it's likely not flexible
enough for users that have no access to hypervisor configuration (such
as QEMU cmdline options and/or firmware binary choice). They might want
to pick UEFI boot vs. CSM boot "unpredictably".


(2) Jiewen suggested a patch for LegacyBiosBuildE820()
[OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c], punching a hole into
E820Table[0].

While this would make the CSM honest about the memory map, to legacy
OSes, I strongly doubt legacy OSes would actually *work* with such a
memory map (i.e. with 128 KB missing in the middle of the <=640KB RAM).
It would require a bunch of testing anyway, and it could break with any
newly tried legacy OS.


(3) Modify OvmfPkg/SmmAccess/SmmAccess2Dxe to set up an event
notification function for the "gEfiEventLegacyBootGuid" event group, in
case the feature has been detected in PlatformPei. In the notification
function, log an error message, call ASSERT (FALSE), and call
CpuDeadLoop ().

Advantages:
- when OVMF is built without -D SMM_REQUIRE, SmmAccess2Dxe is not included

- when OVMF is built without -D CSM_REQUIRE, then the event group is
never signaled -- the only EfiSignalEventLegacyBoot() call is in
"OvmfPkg/Csm/LegacyBiosDxe/LegacyBootSupport.c".

- when OVMF is built with both options, but no legacy boot is performed,
the callback does nothing

- the callback catches the invalid constellation and prevents the boot
cleanly. The error message in the OVMF debug log would instruct users to
disable the dynamically detectable feature on the QEMU command line, and
to boot again.

Disadvantages:

- users have to look at the OVMF log

- users may not have the power to change the QEMU configuration -- if
they had that power, they'd simply use SeaBIOS for booting the legacy
OS, not OVMF+CSM.


(4) Identify a CSM build in time for the feature detection (in
PlatformPei), and force-disable the feature (don't even attempt
detecting it from QEMU) if this is a CSM build.

In the DXE phase (and especially in the BDS phase), we can detect a CSM
build, from the presence of gEfiLegacyBiosProtocolGuid in the UEFI
protocol database. However, the protocol is not available in the PEI
phase, which is when this particular QEMU feature has to be enabled.

Therefore we could introduce a dedicated FeaturePCD, which would reflect
-D CSM_ENABLE. When set, the PCD would short-circuit the dynamic
detection in PlatformPei, and pretend that the feature doesn't exist.

This option would allow -D SMM_REQUIRE and -D CSM_ENABLE to co-exist
(like they do now); the latter would only disable the new feature
(regardless of QEMU offering it), not all of SMM. With -D CSM_ENABLE,
users would not lose the protection they get from SMM_REQUIRE when
booting a UEFI OS; they'd only lose VCPU hotplug capability.


My preference is (1), but I never use -D CSM_ENABLE.

David, do you see (or do you foresee) OVMF binaries that are built with
*both* of -D CSM_ENABLE and -D SMM_REQUIRE?

> With the commit message change, the series reviewed-by: jiewen@intel.com

Thank you Jiewen -- it's possible that I'll post a v2, based on David's
answer, and then I'll pick up your R-b for unchanged patches from v1.

Thanks
Laszlo

> 
>> -Original Message-
>> From: Laszlo Ersek 
>> Sent: Thursday, September 26, 2019 10:52 PM
>> To: Yao, Jiewen ; devel@edk2.groups.io
>> Cc: Ard Biesheuvel ; Boris Ostrovsky
>> ; Brijesh Singh ; Igor
>> Mammedov ; Joao M Martins
>> ; Justen, Jordan L ;
>> Nakajima, Jun ; Kinney, Michael D
>> ; Paolo Bonzini ; Phillip
>> Goerl ; Chen, Yingwen 
>> Subject: Re: [edk2-devel] [PATCH wave 1 00/10] support QEMU's "SMRAM at
>> default SMBASE" feature
>>
>> On 09/26/19 10:46, Yao, Jiewen wrote:
>>> Hi Laszlo
>>> May I know if you want to support legacy BIOS?
>>
>> No, thanks.
>>
>> The end-goal is to implement VCPU hotplug at OS runtime, when the OS is
>> booted with OVMF, Secure Boot is enab

Re: [edk2-devel] [edk2-platforms PATCH 1/1] Platforms/RPi3: DisplayDxe virtual resolution improvements

2019-10-01 Thread Philippe Mathieu-Daudé 

On 10/1/19 3:10 PM, Leif Lindholm wrote:

On Mon, Sep 30, 2019 at 10:32:03AM +0100, Pete Batard wrote:

Hi Leif,

On 2019.09.29 00:05, Leif Lindholm wrote:

On Fri, Sep 27, 2019 at 10:20:15AM +0100, Pete Batard wrote:

From: Andrei Warkentin 

The Pi GPU decouples requested resolution from actual physical resolution
and can perform scaling of virtual resolutions. This enables platform users
to do something like ask for 1024x768 and get a framebuffer of that size,
regardless of the actual output (which could be a very blurry SDTV).

Specifically, this patch allows selecting which specific virtual
resolutions to enable, thus replacing the old all-or-nothing behaviour
with either all virtual resolutions supported, or just the native one.

This patch also adds enables the common 7" Pi (800x480) screen to be used
at 800x600 resolution, instead of forcing 640x480 as the only usable
resolution.


Slightly too late nit: " ... also ..." suggest the 7" display forced to 
800x600 change could have go in a separate patch.




I am basically OK with this patch, but I note that the change in
variable name/content means existing users will end up with stale
variables.

So I wonder if it would be worth explicitly adding a stanza deleting
the old variable if found?


That would be a valid point *if* the Pi 3 was using actual NVRAM storage to
write those variables.

However, we have no such thing on the hardware, so we currently store those
variables on the SD card, within the firmware file itself.

Which means, the minute you "install" a new firmware (by replacing the
existing RPI_EFI.fd on your SD card with a new one), you're losing all your
existing variables anyway, including stale ones.


Ah. That is sort of unique, but I see how it could still be useful.
It was the EFI_VARIABLE_NON_VOLATILE that threw me.


Maybe with the Pi 4 and its 512 KB EEPROM, we'll be able to look into
preserving the variables. Or we may also look into writing variables to a
separate virtual NVRAM file on the SD card, instead of just reusing the .fd
(which we are doing for convenience). But for our current model, what you
highlight is a non issue, as the only "upgrade" path forces users to always
start with a virtual NVRAM that has been reset and that is therefore free
from any stale variable.


Sure. this works.
Reviewed-by: Leif Lindholm 
Pushed as 777573818e0c.


Thanks Leif.

Regards,

Phil.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48347): https://edk2.groups.io/g/devel/message/48347
Mute This Topic: https://groups.io/mt/34309623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms PATCH 1/1] Platforms/RPi3: DisplayDxe virtual resolution improvements

2019-10-01 Thread Leif Lindholm 
On Tue, Oct 01, 2019 at 12:20:03PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Pete,
> 
> On 9/27/19 10:41 PM, Pete Batard wrote:
> > Try this after making sure that you have edk2/, edk2-platforms/ and
> > edk-non-osi/ in /home/phil/source:
> > 
> > cd /home/phil/source
> > export GCC5_AARCH64_PREFIX=aarch64-linux-gnu-
> > export WORKSPACE=$PWD
> > export 
> > PACKAGES_PATH=$WORKSPACE/edk2:$WORKSPACE/edk2-platforms:$WORKSPACE/edk2-non-osi
> > 
> > source edk2/edksetup.sh --reconfig
> > build -a AARCH64 -t GCC5 -p
> > edk2-platforms/Platform/RaspberryPi/RPi3/RPi3.dsc -b DEBUG
> 
> [note for other readers: I'm answering to Pete top-posted comment,
>  start of the discussion can be followed below my reply]
> 
> I'm confused because this works like charm.
> 
> I always used:
> 
> WORKSPACE= /home/phil/source/edk2
> PACKAGES_PATH=
> /home/phil/source/edk2:/home/phil/source/edk2-platforms
> 
> and built into my edk2/ folder (my $WORKSPACE).
> 
> Leif noticed I was missing edk2-non-osi, so I added it (this reply below):
> 
> PACKAGES_PATH=
> /home/phil/source/edk2:/home/phil/source/edk2-platforms:/home/phil/source/edk2-non-osi
> 
> 
> I don't understand why modifying $WORKSPACE out of edk2/ this changes the
> behavior into failure, I thought it was would mostly change the Build/
> folder location, and the packages were only searched into $PACKAGES_PATH
> (which did not change).

Yeah, I recall this from the early days of moving from OpenPlatformPkg
(symlink or submodule) to edk2-platforms (and PACKAGES_PATH).
Something in the "let's avoid getting namespace clashes" logic goes
bonkers[1] when you get any part of path overlap (i.e. Platform/ in
more than one PACKAGES_PATH directory) ... but only when using edk2/
as WORKSPACE.

[1] does what Leif wouldn't have expected

> Anyhow I'll adapt my setup and not use edk2/ as $WORKSPACE anymore.

Yeah, that's what I did.

/
Leif

> Thanks for the pointers,
> 
> Phil.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48346): https://edk2.groups.io/g/devel/message/48346
Mute This Topic: https://groups.io/mt/34309623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-platforms PATCH 1/1] Platforms/RPi3: DisplayDxe virtual resolution improvements

2019-10-01 Thread Leif Lindholm 
On Mon, Sep 30, 2019 at 10:32:03AM +0100, Pete Batard wrote:
> Hi Leif,
> 
> On 2019.09.29 00:05, Leif Lindholm wrote:
> > On Fri, Sep 27, 2019 at 10:20:15AM +0100, Pete Batard wrote:
> > > From: Andrei Warkentin 
> > > 
> > > The Pi GPU decouples requested resolution from actual physical resolution
> > > and can perform scaling of virtual resolutions. This enables platform 
> > > users
> > > to do something like ask for 1024x768 and get a framebuffer of that size,
> > > regardless of the actual output (which could be a very blurry SDTV).
> > > 
> > > Specifically, this patch allows selecting which specific virtual
> > > resolutions to enable, thus replacing the old all-or-nothing behaviour
> > > with either all virtual resolutions supported, or just the native one.
> > > 
> > > This patch also adds enables the common 7" Pi (800x480) screen to be used
> > > at 800x600 resolution, instead of forcing 640x480 as the only usable
> > > resolution.
> > 
> > I am basically OK with this patch, but I note that the change in
> > variable name/content means existing users will end up with stale
> > variables.
> > 
> > So I wonder if it would be worth explicitly adding a stanza deleting
> > the old variable if found?
> 
> That would be a valid point *if* the Pi 3 was using actual NVRAM storage to
> write those variables.
> 
> However, we have no such thing on the hardware, so we currently store those
> variables on the SD card, within the firmware file itself.
> 
> Which means, the minute you "install" a new firmware (by replacing the
> existing RPI_EFI.fd on your SD card with a new one), you're losing all your
> existing variables anyway, including stale ones.

Ah. That is sort of unique, but I see how it could still be useful.
It was the EFI_VARIABLE_NON_VOLATILE that threw me.

> Maybe with the Pi 4 and its 512 KB EEPROM, we'll be able to look into
> preserving the variables. Or we may also look into writing variables to a
> separate virtual NVRAM file on the SD card, instead of just reusing the .fd
> (which we are doing for convenience). But for our current model, what you
> highlight is a non issue, as the only "upgrade" path forces users to always
> start with a virtual NVRAM that has been reset and that is therefore free
> from any stale variable.

Sure. this works.
Reviewed-by: Leif Lindholm 
Pushed as 777573818e0c.

Regards,

Leif

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48345): https://edk2.groups.io/g/devel/message/48345
Mute This Topic: https://groups.io/mt/34309623/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/7] BaseTools: strip trailing whitespace

2019-10-01 Thread Leif Lindholm 
Cc: Bob Feng 
Cc: Liming Gao 
Signed-off-by: Leif Lindholm 
---

Resubmitting a v2 with changes to external projects:
- BrotliCompress
- LzmaCompress
- Pccts
backed out for now.

BaseTools/Source/C/GNUmakefile | 2 +-
 BaseTools/Source/C/Makefiles/app.makefile  | 4 ++--
 BaseTools/Source/C/Makefiles/footer.makefile   | 4 ++--
 BaseTools/Source/C/Makefiles/header.makefile   | 8 
 BaseTools/Source/C/Makefiles/lib.makefile  | 2 +-
 BaseTools/Source/C/Makefiles/ms.common | 4 ++--
 BaseTools/Source/C/VfrCompile/GNUmakefile  | 6 +++---
 BaseTools/Source/Python/Ecc/Check.py   | 2 +-
 BaseTools/Source/Python/GenFds/GenFdsGlobalVariable.py | 2 +-
 BaseTools/Source/Python/Makefile   | 2 +-
 10 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/BaseTools/Source/C/GNUmakefile b/BaseTools/Source/C/GNUmakefile
index 37bcce519c7e..df4eb64ea95e 100644
--- a/BaseTools/Source/C/GNUmakefile
+++ b/BaseTools/Source/C/GNUmakefile
@@ -77,7 +77,7 @@ $(SUBDIRS):
 $(patsubst %,%-clean,$(sort $(SUBDIRS))):
-$(MAKE) -C $(@:-clean=) clean
 
-$(VFRAUTOGEN): VfrCompile/VfrSyntax.g 
+$(VFRAUTOGEN): VfrCompile/VfrSyntax.g
$(MAKE) -C VfrCompile VfrLexer.h
 
 clean:  $(patsubst %,%-clean,$(sort $(SUBDIRS)))
diff --git a/BaseTools/Source/C/Makefiles/app.makefile 
b/BaseTools/Source/C/Makefiles/app.makefile
index fcadb4ed2194..6a2a8f5e8a0e 100644
--- a/BaseTools/Source/C/Makefiles/app.makefile
+++ b/BaseTools/Source/C/Makefiles/app.makefile
@@ -12,9 +12,9 @@ include $(MAKEROOT)/Makefiles/header.makefile
 APPLICATION = $(MAKEROOT)/bin/$(APPNAME)
 
 .PHONY:all
-all: $(MAKEROOT)/bin $(APPLICATION) 
+all: $(MAKEROOT)/bin $(APPLICATION)
 
-$(APPLICATION): $(OBJECTS) 
+$(APPLICATION): $(OBJECTS)
$(LINKER) -o $(APPLICATION) $(BUILD_LFLAGS) $(OBJECTS) 
-L$(MAKEROOT)/libs $(LIBS)
 
 $(OBJECTS): $(MAKEROOT)/Include/Common/BuildVersion.h
diff --git a/BaseTools/Source/C/Makefiles/footer.makefile 
b/BaseTools/Source/C/Makefiles/footer.makefile
index e823246cfbb4..85c3374224f2 100644
--- a/BaseTools/Source/C/Makefiles/footer.makefile
+++ b/BaseTools/Source/C/Makefiles/footer.makefile
@@ -14,10 +14,10 @@ $(MAKEROOT)/libs-$(HOST_ARCH):
 install: $(MAKEROOT)/libs-$(HOST_ARCH) $(LIBRARY)
cp $(LIBRARY) $(MAKEROOT)/libs-$(HOST_ARCH)
 
-$(LIBRARY): $(OBJECTS) 
+$(LIBRARY): $(OBJECTS)
$(BUILD_AR) crs $@ $^
 
-%.o : %.c 
+%.o : %.c
$(BUILD_CC)  -c $(BUILD_CPPFLAGS) $(BUILD_CFLAGS) $< -o $@
 
 %.o : %.cpp
diff --git a/BaseTools/Source/C/Makefiles/header.makefile 
b/BaseTools/Source/C/Makefiles/header.makefile
index 52cbffcb4423..4e9b36d98bdb 100644
--- a/BaseTools/Source/C/Makefiles/header.makefile
+++ b/BaseTools/Source/C/Makefiles/header.makefile
@@ -61,7 +61,7 @@ else
 $(error Bad HOST_ARCH)
 endif
 
-INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I $(MAKEROOT)/Include/Common -I 
$(MAKEROOT)/Include/ -I $(MAKEROOT)/Include/IndustryStandard -I 
$(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE) 
+INCLUDE = $(TOOL_INCLUDE) -I $(MAKEROOT) -I $(MAKEROOT)/Include/Common -I 
$(MAKEROOT)/Include/ -I $(MAKEROOT)/Include/IndustryStandard -I 
$(MAKEROOT)/Common/ -I .. -I . $(ARCH_INCLUDE)
 BUILD_CPPFLAGS = $(INCLUDE)
 
 # keep EXTRA_OPTFLAGS last
@@ -82,7 +82,7 @@ BUILD_CXXFLAGS = -Wno-unused-result
 
 ifeq ($(HOST_ARCH), IA32)
 #
-# Snow Leopard  is a 32-bit and 64-bit environment. uname -m returns i386, but 
gcc defaults 
+# Snow Leopard  is a 32-bit and 64-bit environment. uname -m returns i386, but 
gcc defaults
 #  to x86_64. So make sure tools match uname -m. You can manual have a 64-bit 
kernal on Snow Leopard
 #  so only do this is uname -m returns i386.
 #
@@ -96,7 +96,7 @@ endif
 # keep BUILD_OPTFLAGS last
 BUILD_CFLAGS   += $(BUILD_OPTFLAGS)
 BUILD_CXXFLAGS += $(BUILD_OPTFLAGS)
-  
+
 # keep EXTRA_LDFLAGS last
 BUILD_LFLAGS += $(EXTRA_LDFLAGS)
 
@@ -107,7 +107,7 @@ BUILD_LFLAGS += $(EXTRA_LDFLAGS)
 all:
 
 $(MAKEROOT)/libs:
-   mkdir $(MAKEROOT)/libs 
+   mkdir $(MAKEROOT)/libs
 
 $(MAKEROOT)/bin:
mkdir $(MAKEROOT)/bin
diff --git a/BaseTools/Source/C/Makefiles/lib.makefile 
b/BaseTools/Source/C/Makefiles/lib.makefile
index a9965fc628d9..2577c15380a9 100644
--- a/BaseTools/Source/C/Makefiles/lib.makefile
+++ b/BaseTools/Source/C/Makefiles/lib.makefile
@@ -9,6 +9,6 @@ include $(MAKEROOT)/Makefiles/header.makefile
 
 LIBRARY = $(MAKEROOT)/libs/lib$(LIBNAME).a
 
-all: $(MAKEROOT)/libs $(LIBRARY) 
+all: $(MAKEROOT)/libs $(LIBRARY)
 
 include $(MAKEROOT)/Makefiles/footer.makefile
diff --git a/BaseTools/Source/C/Makefiles/ms.common 
b/BaseTools/Source/C/Makefiles/ms.common
index 75c9bb43a32c..f5f77fdc0bc5 100644
--- a/BaseTools/Source/C/Makefiles/ms.common
+++ b/BaseTools/Source/C/Makefiles/ms.common
@@ -57,6 +57,6 @@ LINKER = $(LD)
 
 INC = -I . -I $(SOURCE_PATH)\Include -I $(ARCH_INCLUDE) -I 
$(SOURCE_PATH)\Common $(INC)
 
-CFLAGS = $(CFLAGS) /nologo /Zi /c /O2 /MT /W4

Re: [edk2-devel] [edk2-platforms PATCH 1/1] Platforms/RPi3: DisplayDxe virtual resolution improvements

2019-10-01 Thread Philippe Mathieu-Daudé 

Hi Pete,

On 9/27/19 10:41 PM, Pete Batard wrote:
Try this after making sure that you have edk2/, edk2-platforms/ and 
edk-non-osi/ in /home/phil/source:


cd /home/phil/source
export GCC5_AARCH64_PREFIX=aarch64-linux-gnu-
export WORKSPACE=$PWD
export 
PACKAGES_PATH=$WORKSPACE/edk2:$WORKSPACE/edk2-platforms:$WORKSPACE/edk2-non-osi 


source edk2/edksetup.sh --reconfig
build -a AARCH64 -t GCC5 -p 
edk2-platforms/Platform/RaspberryPi/RPi3/RPi3.dsc -b DEBUG


[note for other readers: I'm answering to Pete top-posted comment,
 start of the discussion can be followed below my reply]

I'm confused because this works like charm.

I always used:

WORKSPACE= /home/phil/source/edk2
PACKAGES_PATH=
/home/phil/source/edk2:/home/phil/source/edk2-platforms

and built into my edk2/ folder (my $WORKSPACE).

Leif noticed I was missing edk2-non-osi, so I added it (this reply below):

PACKAGES_PATH=
/home/phil/source/edk2:/home/phil/source/edk2-platforms:/home/phil/source/edk2-non-osi 



I don't understand why modifying $WORKSPACE out of edk2/ this changes 
the behavior into failure, I thought it was would mostly change the 
Build/ folder location, and the packages were only searched into 
$PACKAGES_PATH (which did not change).


Anyhow I'll adapt my setup and not use edk2/ as $WORKSPACE anymore.

Thanks for the pointers,

Phil.


On 2019.09.27 21:23, Philippe Mathieu-Daudé wrote:

On 9/27/19 7:49 PM, Leif Lindholm wrote:

On Fri, Sep 27, 2019 at 06:38:07PM +0200, Philippe Mathieu-Daudé wrote:

Hi Pete,

On 9/27/19 11:20 AM, Pete Batard wrote:

From: Andrei Warkentin 

The Pi GPU decouples requested resolution from actual physical 
resolution
and can perform scaling of virtual resolutions. This enables 
platform users
to do something like ask for 1024x768 and get a framebuffer of that 
size,

regardless of the actual output (which could be a very blurry SDTV).

Specifically, this patch allows selecting which specific virtual
resolutions to enable, thus replacing the old all-or-nothing behaviour
with either all virtual resolutions supported, or just the native one.

This patch also adds enables the common 7" Pi (800x480) screen to 
be used

at 800x600 resolution, instead of forcing 640x480 as the only usable
resolution.


I tried to build the RPi3 platform but I get errors because it seems to
use an older edk2 repository. What tag should I use?


edk2-platforms master should always build against edk2 - if it does
not, that's a bug. But I can't see any issues when building rpi3 with
the current master branches.
Any platform that is not willing to commit to this state of things can
live on stable- or devel- branches in the edk-platforms repository.
This process is described in
https://github.com/tianocore/edk2-platforms/blob/about/Readme.md

But please provide some more information than "I get errors". It is a
much better use of maintainer time than sending me off verifying that
something I expected to work still works (for me).


Yes, sorry :/

So the first error was:

: error 000E: File/directory not found in workspace
   Platform/RaspberryPi/RPi3/Drivers/LogoDxe/LogoDxe.inf is not found in
packages path:

And as you noted on IRC, I was missing edk2-non-osi, silly me :S

Using it I now get:

/home/phil/source/edk2$ GCC5_AARCH64_PREFIX=aarch64-linux-gnu- build -a
AARCH64 -t GCC5 -b DEBUG -p Platform/RaspberryPi/RPi3/RPi3.dsc -D
ATF_BUILD_DIR=$ATF_BUILD_DIR -n 1
Build environment:
Linux-5.2.11-100.fc29.x86_64-x86_64-with-Ubuntu-16.04-xenial
Build start time: 20:10:25, Sep.27 2019

WORKSPACE    = /home/phil/source/edk2
PACKAGES_PATH    =
/home/phil/source/edk2:/home/phil/source/edk2-platforms:/home/phil/source/edk2-non-osi 


EDK_TOOLS_PATH   = /home/phil/source/edk2/BaseTools
CONF_PATH    = /home/phil/source/edk2/Conf


Architecture(s)  = AARCH64
Build target = DEBUG

Processing meta-data .Toolchain    = GCC5

Active Platform  =
/home/phil/source/edk2-platforms/Platform/RaspberryPi/RPi3/RPi3.dsc
. done!
Building ...
/home/phil/source/edk2/ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf 


[AARCH64]
make: Nothing to be done for 'tbuild'.
Building ...
/home/phil/source/edk2/MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf 


[AARCH64]
make: Nothing to be done for 'tbuild'.
Building ...
/home/phil/source/edk2/MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf 


[AARCH64]
make: Nothing to be done for 'tbuild'.
Building ... /home/phil/source/edk2/MdePkg/Library/UefiLib/UefiLib.inf
[AARCH64]
make: Nothing to be done for 'tbuild'.
Building ...
/home/phil/source/edk2/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf 


[AARCH64]
make: Nothing to be done for 'tbuild'.
Building ...
/home/phil/source/edk2/MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf 


[AARCH64]
make: Nothing to be done for 'tbuild'.
Building ...
/home/phil/source/edk2/MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf 


[AARCH64]
make: Nothing

Re: [edk2-devel] [PATCH 1/1] BaseTools: use stdint.h for GCC ProcessorBind.h typedefs

2019-10-01 Thread Leif Lindholm 
Thanks all - pushed as 5be5439a5a4e.

(And special thanks to Laszlo for the below.)

On Tue, Oct 01, 2019 at 12:01:31AM +0200, Laszlo Ersek wrote:
> On 09/27/19 12:06, Philippe Mathieu-Daudé wrote:
> > On 9/26/19 9:28 PM, Leif Lindholm wrote:
> >> The AArch64 definitions of UINT64/INT64 differ from the X64 ones.
> >> Since this is on the tool side, doing like X64 and picking the
> >> definitions from stdint.h feels like a better idea than hardcoding
> >> them. So copy the pattern from X64/ProcesorBind.h.
> > 
> > Typo: X64/ProcessorBind.h ('s' missing).
> > 
> >>
> >> Cc: Ard Biesheuvel 
> >> Cc: Bob Feng 
> >> Cc: Liming Gao 
> >> Cc: Laszlo Ersek 
> >> Signed-off-by: Leif Lindholm 
> >> ---
> >>
> >> This was triggered by one of the Risc-V patches which may need to end up
> >> being modified to the point where this issue goes away, but the current
> >> situation seems suboptimal. (Do you use %llx or %lx to print an Elf64_Addr
> >> on a 64-bit LP architecture?)
> > 
> > What is the answer? :)
> 
> For a hosted C99 program, you cast it to uint64_t, and print it with
> 
>   "%"PRIx64
> 
> (Note: "uint64_t" is an optional type, per C99.
> 
> """
> However, if an implementation provides integer types with widths of 8,
> 16, 32, or 64 bits, no padding bits, and (for the signed types) that
> have a two’s complement representation, it shall define the
> corresponding typedef names.
> """
> 
> The existence of Elf64_Addr suggests the implementation is like that,
> hence we can assume "uint64_t". Otherwise, we'd have to use
> "uint_least64_t" and "%"PRIxLEAST64.)
> 
> 
> When restricted to C89, you can only use "%lx" and "unsigned long", and
> you also have to rely on -- for example -- SUSv2 (from 1997 -- the last
> Single Unix Spec defined in terms of C89), for ensuring / selecting the
> XBS5_LP64_OFF64, or XBS5_LPBIG_OFFBIG compilation environments.
> 
> (SUSv2 defines uint64_t, so you could use that in itself, but SUSv2
> defines no matching format string macros, for printing uint64_t.)
> 
> How you print a 64-bit unsigned integer in Visual Studio, I can't say.
> It's not fully C99 conformant, it's likely also not SUSv2 conformant (in
> case we wanted to rely on C89 + SUSv2); so I have no idea. It's likely
> documented in MSDN or some place similar.
> 
> Thanks
> Laszlo
> 
> > 
> >>
> >>  BaseTools/Source/C/Include/AArch64/ProcessorBind.h | 26 
> >> ++--
> >>  1 file changed, 13 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/BaseTools/Source/C/Include/AArch64/ProcessorBind.h 
> >> b/BaseTools/Source/C/Include/AArch64/ProcessorBind.h
> >> index bfaf1e28e446..dfa725b2e363 100644
> >> --- a/BaseTools/Source/C/Include/AArch64/ProcessorBind.h
> >> +++ b/BaseTools/Source/C/Include/AArch64/ProcessorBind.h
> >> @@ -41,21 +41,21 @@
> >>typedef signed char INT8;
> >>  #else
> >>//
> >> -  // Assume standard AARCH64 alignment.
> >> +  // Use ANSI C 2000 stdint.h integer width declarations
> >>//
> >> -  typedef unsigned long long  UINT64;
> >> -  typedef long long   INT64;
> >> -  typedef unsigned intUINT32;
> >> -  typedef int INT32;
> >> -  typedef unsigned short  UINT16;
> >> -  typedef unsigned short  CHAR16;
> >> -  typedef short   INT16;
> >> -  typedef unsigned char   BOOLEAN;
> >> -  typedef unsigned char   UINT8;
> >> -  typedef charCHAR8;
> >> -  typedef signed char INT8;
> >> +  #include 
> >> +  typedef uint8_t   BOOLEAN;
> >> +  typedef int8_tINT8;
> >> +  typedef uint8_t   UINT8;
> >> +  typedef int16_t   INT16;
> >> +  typedef uint16_t  UINT16;
> >> +  typedef int32_t   INT32;
> >> +  typedef uint32_t  UINT32;
> >> +  typedef int64_t   INT64;
> >> +  typedef uint64_t  UINT64;
> >> +  typedef char  CHAR8;
> >> +  typedef uint16_t  CHAR16;
> >>  
> >> -  #define UINT8_MAX 0xff
> >>  #endif
> >>  
> >>  ///
> >>
> > 
> > Reviewed-by: Philippe Mathieu-Daude 
> > 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48342): https://edk2.groups.io/g/devel/message/48342
Mute This Topic: https://groups.io/mt/34302597/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29] MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

2019-10-01 Thread Leif Lindholm 
On Tue, Oct 01, 2019 at 10:49:38AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Leif,
> 
> On 9/27/19 1:39 AM, Leif Lindholm wrote:
> > On Mon, Sep 23, 2019 at 08:31:35AM +0800, Abner Chang wrote:
> > > RISC-V MMIO library instance.  RISC-V only supports memory map I/O.
> > 
> > We need fewer, not more, C implementations of MMIO accessors.
> > While this set doesn't need to wait for upstream to get sorted, please
> > just use IoLibArm.c which should be completely equivalent to what you
> > have implemented here.
> 
> This shows this file name is misleading. However I can't come with a clever
> one :/

This has been discussed before, only the current situation "works", so
sorting it out never takes priority (I know it doesn't for me).

There should be exactly one variant of IoLib.c. Well, these days we
need a separate one for ARM/AARCH64 under hw virtualization.

IoLibArm, IoLibEbc and IoLibRiscV have *exactly* the same
requirements. And now x86 uses NASM regardless of build platform, I
think it would make sense to move the contents of IoLibGcc and
IoLibMsc into assembler.

/
Leif

> > > Signed-off-by: Abner Chang 
> > > ---
> > >   .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf  |   8 +-
> > >   MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c | 601 
> > > +
> > >   2 files changed, 607 insertions(+), 2 deletions(-)
> > >   create mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48341): https://edk2.groups.io/g/devel/message/48341
Mute This Topic: https://groups.io/mt/34258203/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

2019-10-01 Thread David Woodhouse 
On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote:
> On 09/29/19 08:09, Wang, Jian J wrote:
> > For this patch series,
> > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed 
> > any more.
> >   Remove it at push time and no need to send a v2.
> > 2. Since it's security patch which had been reviewed separately, I see no 
> > reason for new r-b
> >   required. Please raise it asap if any objections.
> > 3. Acked-by: Jian J Wang 
> 
> 
> * Can you please confirm that these patches match those that we
> discussed here:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19
> 
> 
> * In the BZ, David and Bret raised some questions:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36
> 
> and
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40
> 
> The latest comment in the bug is c#41. I'm not under the impression that
> all concerns raised by David and Bret have been addressed (or
> abandoned). I'd like David and Bret to ACK the patches.

I do not believe my comment #35 has been addressed, nor the requested
testing performed.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48340): https://edk2.groups.io/g/devel/message/48340
Mute This Topic: https://groups.io/mt/34307578/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-



smime.p7s
Description: S/MIME cryptographic signature

Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 09/29] MdePkg/BaseIoLibIntrinsic: RISC-V I/O intrinsic functions.

2019-10-01 Thread Philippe Mathieu-Daudé 

Hi Leif,

On 9/27/19 1:39 AM, Leif Lindholm wrote:

On Mon, Sep 23, 2019 at 08:31:35AM +0800, Abner Chang wrote:

RISC-V MMIO library instance.  RISC-V only supports memory map I/O.


We need fewer, not more, C implementations of MMIO accessors.
While this set doesn't need to wait for upstream to get sorted, please
just use IoLibArm.c which should be completely equivalent to what you
have implemented here.


This shows this file name is misleading. However I can't come with a 
clever one :/



Signed-off-by: Abner Chang 
---
  .../BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf  |   8 +-
  MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c | 601 +
  2 files changed, 607 insertions(+), 2 deletions(-)
  create mode 100644 MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c

diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf 
b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
index 457cce9..fbb568e 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
@@ -2,13 +2,14 @@
  #  Instance of I/O Library using compiler intrinsics.
  #
  #  I/O Library that uses compiler intrinsics to perform IN and OUT 
instructions
-#  for IA-32 and x64.  On IPF, I/O port requests are translated into MMIO 
requests.
+#  for IA-32, x64 and RISC-V.  On IPF, I/O port requests are translated into 
MMIO requests.
  #  MMIO requests are forwarded directly to memory.  For EBC, I/O port requests
  #  ASSERT().
  #
  #  Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
  #  Copyright (c) 2017, AMD Incorporated. All rights reserved.
+#  Portinos Copyright (c) 2016, Hewlett Packard Enterprise Development LP. All 
rights reserved.
  #
  #  SPDX-License-Identifier: BSD-2-Clause-Patent
  #
@@ -25,7 +26,7 @@
  
  
  #

-#  VALID_ARCHITECTURES   = IA32 X64 EBC ARM AARCH64
+#  VALID_ARCHITECTURES   = IA32 X64 EBC ARM AARCH64 RISCV64
  #
  
  [Sources]

@@ -55,6 +56,9 @@
  [Sources.AARCH64]
IoLibArm.c
  
+[Sources.RISCV64]

+  IoLibRiscV.c
+
  [Packages]
MdePkg/MdePkg.dec
  
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c b/MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c

new file mode 100644
index 000..789928b
--- /dev/null
+++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLibRiscV.c
@@ -0,0 +1,601 @@
+/** @file
+  Common I/O Library routines for RISC-V
+
+  Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights 
reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution. The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+**/
+
+#include "BaseIoLibIntrinsicInternal.h"
+
+/**
+  Reads an 8-bit MMIO register.
+
+  Reads the 8-bit MMIO register specified by Address. The 8-bit read value is
+  returned. This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  If 8-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT8
+EFIAPI
+MmioRead8 (
+  IN  UINTN Address
+  )
+{
+  return *(volatile UINT8*)Address;
+}
+
+/**
+  Writes an 8-bit MMIO register.
+
+  Writes the 8-bit MMIO register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all MMIO read
+  and write operations are serialized.
+
+  If 8-bit MMIO register operations are not supported, then ASSERT().
+
+  @param  Address The MMIO register to write.
+  @param  Value   The value to write to the MMIO register.
+
+  @return Value.
+
+**/
+UINT8
+EFIAPI
+MmioWrite8 (
+  IN  UINTN Address,
+  IN  UINT8 Value
+  )
+{
+  *(volatile UINT8 *)Address = Value;
+  return Value;
+}
+
+/**
+  Reads a 16-bit MMIO register.
+
+  Reads the 16-bit MMIO register specified by Address. The 16-bit read value is
+  returned. This function must guarantee that all MMIO read and write
+  operations are serialized.
+
+  If 16-bit MMIO register operations are not supported, then ASSERT().
+  If Address is not aligned on a 16-bit boundary, then ASSERT().
+
+  @param  Address The MMIO register to read.
+
+  @return The value read.
+
+**/
+UINT16
+EFIAPI
+MmioRead16 (
+  IN  UINTN Address
+  )
+{
+  return *(volatile UINT16 *)Address;
+}
+
+/**
+  Writes a 16-bit MMIO register.
+
+  Writes the 16-bit MMIO register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all MMIO read
+  and write operations

Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 08/29] MdePkg/BaseCacheMaintenanceLib: RISC-V cache maintenance implementation.

2019-10-01 Thread Philippe Mathieu-Daudé 

On 9/23/19 2:31 AM, Abner Chang wrote:

Implement RISC-V cache maintenance functions in
BaseCacheMaintenanceLib.

Signed-off-by: Abner Chang 
---
  .../BaseCacheMaintenanceLib.inf|   4 +
  .../Library/BaseCacheMaintenanceLib/RiscVCache.c   | 250 +
  2 files changed, 254 insertions(+)
  create mode 100644 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c

diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf 
b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
index ec7feec..d9bfa04 100644
--- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf
@@ -6,6 +6,7 @@
  #
  #  Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
  #  Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
+#  Copyright (c) 2016, Hewlett Packard Enterprise Development LP. All rights 
reserved.
  #
  #  SPDX-License-Identifier: BSD-2-Clause-Patent
  #
@@ -41,6 +42,9 @@
  [Sources.AARCH64]
ArmCache.c
  
+[Sources.RISCV64]

+  RiscVCache.c
+
  [Packages]
MdePkg/MdePkg.dec
  
diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c

new file mode 100644
index 000..d8e4665
--- /dev/null
+++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
@@ -0,0 +1,250 @@
+/** @file
+  RISC-V specific functionality for cache.
+
+  Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights 
reserved.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include 
+#include 
+#include 
+
+/**
+  RISC-V invalidate instruction cache.
+
+**/
+VOID
+EFIAPI
+RiscVInvalidateInstCacheAsm (
+  VOID
+  );
+
+/**
+  RISC-V invalidate data cache.
+
+**/
+VOID
+EFIAPI
+RiscVInvalidateDataCacheAsm (
+  VOID
+  );
+
+/**
+  Invalidates the entire instruction cache in cache coherency domain of the
+  calling CPU.
+
+**/
+VOID
+EFIAPI
+InvalidateInstructionCache (
+  VOID
+  )
+{
+  RiscVInvalidateInstCacheAsm ();
+}
+
+/**
+  Invalidates a range of instruction cache lines in the cache coherency domain
+  of the calling CPU.
+
+  Invalidates the instruction cache lines specified by Address and Length. If
+  Address is not aligned on a cache line boundary, then entire instruction
+  cache line containing Address is invalidated. If Address + Length is not
+  aligned on a cache line boundary, then the entire instruction cache line
+  containing Address + Length -1 is invalidated. This function may choose to
+  invalidate the entire instruction cache if that is more efficient than
+  invalidating the specified range. If Length is 0, then no instruction cache
+  lines are invalidated. Address is returned.
+
+  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
+
+  @param  Address The base address of the instruction cache lines to
+  invalidate. If the CPU is in a physical addressing mode, then
+  Address is a physical address. If the CPU is in a virtual
+  addressing mode, then Address is a virtual address.
+
+  @param  Length  The number of bytes to invalidate from the instruction cache.
+
+  @return Address.
+
+**/
+VOID *
+EFIAPI
+InvalidateInstructionCacheRange (
+  IN VOID *Address,
+  IN UINTN Length
+  )
+{
+  DEBUG((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __FUNCTION__));
+  return Address;
+}
+
+/**
+  Writes back and invalidates the entire data cache in cache coherency domain
+  of the calling CPU.
+
+  Writes back and invalidates the entire data cache in cache coherency domain
+  of the calling CPU. This function guarantees that all dirty cache lines are
+  written back to system memory, and also invalidates all the data cache lines
+  in the cache coherency domain of the calling CPU.
+
+**/
+VOID
+EFIAPI
+WriteBackInvalidateDataCache (
+  VOID
+  )
+{
+  DEBUG((DEBUG_ERROR, "%a:RISC-V unsupported function.\n", __FUNCTION__));
+}
+
+/**
+  Writes back and invalidates a range of data cache lines in the cache
+  coherency domain of the calling CPU.
+
+  Writes back and invalidates the data cache lines specified by Address and
+  Length. If Address is not aligned on a cache line boundary, then entire data
+  cache line containing Address is written back and invalidated. If Address +
+  Length is not aligned on a cache line boundary, then the entire data cache
+  line containing Address + Length -1 is written back and invalidated. This
+  function may choose to write back and invalidate the entire data cache if
+  that is more efficient than writing back and invalidating the specified
+  range. If Length is 0, then no data cache lines are written back and
+  invalidated. Address is returned.
+
+  If Length is greater than (MAX_ADDRESS - Address + 1), then ASSERT().
+
+  @param  Address The base address of the data cache lines to write back and
+  invalidate. If the CPU is in a

Re: [edk2-devel] [PATCH 19/35] NetworkPkg: fix CloseProtocol & UninstallMultipleProtocolInterfaces calls

2019-10-01 Thread Philippe Mathieu-Daudé 

On 9/30/19 10:16 PM, Laszlo Ersek wrote:

Hi Phil,

On 09/26/19 14:42, Philippe Mathieu-Daudé wrote:

Hi Laszlo,

On 9/17/19 9:49 PM, Laszlo Ersek wrote:

Both the "ControllerHandle" parameter of CloseProtocol()


Maybe worth adding "of type EFI_CLOSE_PROTOCOL"


and the "Handle"
parameter of UninstallMultipleProtocolInterfaces()


"of type EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES"


I don't think these changes would add much information.

The above names are -- strictly speaking -- field names in the
EFI_BOOT_SERVICES structure. However, in UEFI code, they are so
frequently used that people simply refer to them
- either by just the member name,
- or by the pointer type *in itself*.

So using EFI_CLOSE_PROTOCOL in itself would certainly be correct and
directly understandable, same as CloseProtocol(). Using both at the same
type is redundant.

You can observe this simplification in the spec as well, BTW. For
example, in UEFI-2.8 "2.5.4 Device Drivers", we find

 OpenProtocol() and CloseProtocol() update the handle database
 maintained by the system firmware to track which drivers are
 consuming protocol interfaces.


Fine.


Another comment below:



have type EFI_HANDLE,

not (EFI_HANDLE*).

This patch fixes actual bugs. The issues have been dormant likely because
they are on error paths. (Or, in case of TlsAuthConfigDxe, because the
driver is unloaded likely very infrequently.)

Cc: Jiaxin Wu 
Cc: Siyuan Fu 
Signed-off-by: Laszlo Ersek 
---

Notes:
 build-tested only

  NetworkPkg/DnsDxe/DnsDriver.c  | 4 ++--
  NetworkPkg/IScsiDxe/IScsiConfig.c  | 2 +-
  NetworkPkg/Ip4Dxe/Ip4Driver.c  | 2 +-
  NetworkPkg/Ip6Dxe/Ip6Driver.c  | 2 +-
  NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c| 2 +-
  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.c | 2 +-
  6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsDriver.c b/NetworkPkg/DnsDxe/DnsDriver.c
index 94d072159a4d..ad007da8b7d6 100644
--- a/NetworkPkg/DnsDxe/DnsDriver.c
+++ b/NetworkPkg/DnsDxe/DnsDriver.c
@@ -1145,7 +1145,7 @@ Dns4ServiceBindingCreateChild (
 DnsSb->ConnectUdp->UdpHandle,
 &gEfiUdp4ProtocolGuid,
 gDns4DriverBinding.DriverBindingHandle,
-   ChildHandle
+   *ChildHandle


EFI_CLOSE_PROTOCOL, OK.


 );
  
   gBS->UninstallMultipleProtocolInterfaces (

@@ -1388,7 +1388,7 @@ Dns6ServiceBindingCreateChild (
 DnsSb->ConnectUdp->UdpHandle,
 &gEfiUdp6ProtocolGuid,
 gDns6DriverBinding.DriverBindingHandle,
-   ChildHandle
+   *ChildHandle


EFI_CLOSE_PROTOCOL, OK.


 );
  
   gBS->UninstallMultipleProtocolInterfaces (

diff --git a/NetworkPkg/IScsiDxe/IScsiConfig.c 
b/NetworkPkg/IScsiDxe/IScsiConfig.c
index b876da7f5ccd..d773849fd3b0 100644
--- a/NetworkPkg/IScsiDxe/IScsiConfig.c
+++ b/NetworkPkg/IScsiDxe/IScsiConfig.c
@@ -3852,7 +3852,7 @@ IScsiConfigFormInit (
   );
if (CallbackInfo->RegisteredHandle == NULL) {
  gBS->UninstallMultipleProtocolInterfaces (
-   &CallbackInfo->DriverHandle,
+   CallbackInfo->DriverHandle,


EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.


 &gEfiDevicePathProtocolGuid,
 &mIScsiHiiVendorDevicePath,
 &gEfiHiiConfigAccessProtocolGuid,
diff --git a/NetworkPkg/Ip4Dxe/Ip4Driver.c b/NetworkPkg/Ip4Dxe/Ip4Driver.c
index ebd4dec1dfe4..62be8b681a18 100644
--- a/NetworkPkg/Ip4Dxe/Ip4Driver.c
+++ b/NetworkPkg/Ip4Dxe/Ip4Driver.c
@@ -891,7 +891,7 @@ Ip4ServiceBindingCreateChild (
);
if (EFI_ERROR (Status)) {
  gBS->UninstallMultipleProtocolInterfaces (
-   ChildHandle,
+   *ChildHandle,


EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.


 &gEfiIp4ProtocolGuid,
 &IpInstance->Ip4Proto,
 NULL
diff --git a/NetworkPkg/Ip6Dxe/Ip6Driver.c b/NetworkPkg/Ip6Dxe/Ip6Driver.c
index 7dc9e45af7b6..63d8428dbced 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Driver.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Driver.c
@@ -888,7 +888,7 @@ Ip6ServiceBindingCreateChild (
);
if (EFI_ERROR (Status)) {
  gBS->UninstallMultipleProtocolInterfaces (
-   ChildHandle,
+   *ChildHandle,


EFI_UNINSTALL_MULTIPLE_PROTOCOL_INTERFACES, OK.


 &gEfiIp6ProtocolGuid,
 &IpInstance->Ip6Proto,
 NULL
diff --git a/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c 
b/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
index ae9e65544a86..06c4e202d3ef 100644
--- a/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
+++ b/NetworkPkg/Mtftp4Dxe/Mtftp4Driver.c
@@ -592,7 +592,7 @@ Mtftp4ServiceBindingCreateChild (
 MtftpSb->ConnectUdp->UdpHandle,
 &gEfiUdp4ProtocolGuid,
 gMtftp4DriverBinding.DriverBindingHandle,
-   ChildHandle
+   *ChildHandle


EFI_CLOSE_PROTOCOL, OK.


 );
  goto ON_ERROR;
}
diff