Re: [edk2] [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64

2016-01-18 Thread Laszlo Ersek
On 01/18/16 15:29, Ryan Harkin wrote:
> ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].
> 
> Currently, FVP exists both in EDK2's ArmPlatformPkg and in
> OpenPlatformPkg [2].  And they are starting to diverge, with
> OpenPlatformPkg being the most up-to-date with current developments.
> To prevent this divergence, remove the .dsc and .fdf files from
> ArmPlatformPkg and leave OpenPlatformPkg as the master.
> 
> [1] https://git.linaro.org/uefi/OpenPlatformPkg.git
> [2] 
> https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/master:/Platforms/ARM/VExpress
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ryan Harkin 
> ---
>  .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
>  .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
> -
>  2 files changed, 718 deletions(-)

Shouldn't OpenPlatformPkg be merged into edk2 first?

Although, I admit that it's been many months (... hm, apparently a bit
more than a year) that I built ArmVExpress-FVP-AArch64, so perhaps it's
not even functional in edk2. Is that the case?

If it is, then there's nothing to regress with this patch I guess.

Otherwise, if ArmVExpress-FVP-AArch64 does work in edk2 (just lacks
features / fixes), then this looks like a pretty big functionality
regression to me. I vaguely recall that Leif mentioned wanting to bring
OpenPlatformPkg into edk2 -- can that happen first?

Anyway, I've got no horse in this race; it's just that we've been
stressing "no regressions pls".

Hm... I think I found the relevant message from Leif:

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17544/focus=552

After skimming the presentation linked therein: I guess nothing has
changed since last July, and at this point you'd like to avoid
fragmentation. I do find that convincing.

Thanks
Laszlo

> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc 
> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc
> deleted file mode 100644
> index af46331..000
> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc
> +++ /dev/null
> @@ -1,317 +0,0 @@
> -#
> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> -#  Copyright (c) 2015, Intel Corporation. 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.
> -#
> -#
> -
> -
> -#
> -# Defines Section - statements that will be processed to create a Makefile.
> -#
> -
> -[Defines]
> -  PLATFORM_NAME  = ArmVExpress-FVP-AArch64
> -  PLATFORM_GUID  = 0de70077-9b3b-43bf-ba38-0ea37d77141b
> -  PLATFORM_VERSION   = 0.1
> -  DSC_SPECIFICATION  = 0x00010005
> -  OUTPUT_DIRECTORY   = Build/ArmVExpress-FVP-AArch64
> -  SUPPORTED_ARCHITECTURES= AARCH64
> -  BUILD_TARGETS  = DEBUG|RELEASE
> -  SKUID_IDENTIFIER   = DEFAULT
> -  FLASH_DEFINITION   = 
> ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf
> -
> -!ifndef ARM_FVP_RUN_NORFLASH
> -  DEFINE EDK2_SKIP_PEICORE=1
> -!endif
> -
> -
> -!include ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
> -
> -[LibraryClasses.common]
> -  ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
> -  ArmCpuLib|ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.inf
> -  
> ArmPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
> -
> -  
> ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
> -  
> NorFlashPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf
> -  
> LcdPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
> -
> -  TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
> -
> -  # Virtio Support
> -  VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> -  
> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> -
> -[LibraryClasses.common.SEC]
> -  ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf
> -  
> ArmPlatformSecLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/ArmVExpressSecLib.inf
> -  
> ArmPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLibSec.inf
> -
> -[LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION, 
> 

Re: [edk2] [PATCH 0/2] ArmPlatformPkg: Remove FVP and Juno

2016-01-18 Thread Ryan Harkin
On 18 January 2016 at 14:39, Ard Biesheuvel  wrote:
> On 18 January 2016 at 15:29, Ryan Harkin  wrote:
>> ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].
>>
>> Currently, Juno and FVP exist both in EDK2's ArmPlatformPkg and in
>> OpenPlatformPkg.  And they are starting to diverge, with
>> OpenPlatformPkg being the most up-to-date with current developments.
>> To prevent this divergence, remove the .dsc and .fdf files from
>> ArmPlatformPkg and leave OpenPlatformPkg as the master.
>>
>> We can't remove ArmJuno.dec yet because ACPI still uses it to set the
>> include path to ArmPlatform.h.
>>
>> [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64
>> [PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf
>>
>>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 
>> ---
>>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 
>> -
>>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
>> -
>>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
>> --
>>
>> [1] https://git.linaro.org/uefi/OpenPlatformPkg.git
>
> Shouldn't we sync OpenPlatformPkg with the latest EDK2 versions first?

Ooops.

I wasn't aware of any changes in the EDK2 versions that we need to
carry over.  From what I can see, there are changes in OpenPlatformPkg
that are not in EDK2, but not the other way round, except this change:

660aaec  2015-12-15  ArmVExpressPkg/ArmVExpress-FVP-AArch64: run GICv3
in v3 mode   [Ard Biesheuvel]

The ARM Landing Team (that'll be me!) ships GICv2 .dts files for the
FVP models and we don't have a plan to change to running with GICv3,
except in legacy mode.  So I'd revert that change anyway until I was
ready to support GICv3 properly.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64

2016-01-18 Thread Ryan Harkin
ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].

Currently, FVP exists both in EDK2's ArmPlatformPkg and in
OpenPlatformPkg [2].  And they are starting to diverge, with
OpenPlatformPkg being the most up-to-date with current developments.
To prevent this divergence, remove the .dsc and .fdf files from
ArmPlatformPkg and leave OpenPlatformPkg as the master.

[1] https://git.linaro.org/uefi/OpenPlatformPkg.git
[2] 
https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/master:/Platforms/ARM/VExpress

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ryan Harkin 
---
 .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
 .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 -
 2 files changed, 718 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc 
b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc
deleted file mode 100644
index af46331..000
--- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc
+++ /dev/null
@@ -1,317 +0,0 @@
-#
-#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
-#  Copyright (c) 2015, Intel Corporation. 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.
-#
-#
-
-
-#
-# Defines Section - statements that will be processed to create a Makefile.
-#
-
-[Defines]
-  PLATFORM_NAME  = ArmVExpress-FVP-AArch64
-  PLATFORM_GUID  = 0de70077-9b3b-43bf-ba38-0ea37d77141b
-  PLATFORM_VERSION   = 0.1
-  DSC_SPECIFICATION  = 0x00010005
-  OUTPUT_DIRECTORY   = Build/ArmVExpress-FVP-AArch64
-  SUPPORTED_ARCHITECTURES= AARCH64
-  BUILD_TARGETS  = DEBUG|RELEASE
-  SKUID_IDENTIFIER   = DEFAULT
-  FLASH_DEFINITION   = 
ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf
-
-!ifndef ARM_FVP_RUN_NORFLASH
-  DEFINE EDK2_SKIP_PEICORE=1
-!endif
-
-
-!include ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
-
-[LibraryClasses.common]
-  ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
-  ArmCpuLib|ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.inf
-  
ArmPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
-
-  
ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
-  
NorFlashPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf
-  
LcdPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
-
-  TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
-
-  # Virtio Support
-  VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
-  
VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
-
-[LibraryClasses.common.SEC]
-  ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64LibSec.inf
-  
ArmPlatformSecLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSecLibRTSM/ArmVExpressSecLib.inf
-  
ArmPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLibSec.inf
-
-[LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION, 
LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.DXE_DRIVER]
-  PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-
-[BuildOptions]
-  GCC:*_*_AARCH64_PLATFORM_FLAGS == 
-I$(WORKSPACE)/ArmPlatformPkg/ArmVExpressPkg/Include 
-I$(WORKSPACE)/ArmPlatformPkg/ArmVExpressPkg/Include/Platform/RTSM
-
-
-
-#
-# Pcd Section - list of all EDK II PCD Entries defined by this Platform
-#
-
-
-[PcdsFeatureFlag.common]
-
-  ## If TRUE, Graphics Output Protocol will be installed on virtual handle 
created by ConsplitterDxe.
-  #  It could be set FALSE to save size.
-  gEfiMdeModulePkgTokenSpaceGuid.PcdConOutGopSupport|TRUE
-
-[PcdsFixedAtBuild.common]
-  gArmPlatformTokenSpaceGuid.PcdFirmwareVendor|"ARM Fixed Virtual Platform"
-  gEmbeddedTokenSpaceGuid.PcdEmbeddedPrompt|"ARM-FVP"
-
-  # Up to 8 cores on Base models. This works fine if model happens to have 
less.
-  gArmPlatformTokenSpaceGuid.PcdCoreCount|8
-  gArmPlatformTokenSpaceGuid.PcdClusterCount|2
-
-  #
-  # NV Storage PCDs. Use base of 0x0C00 for NOR1
-  #
-  

[edk2] [PATCH 0/2] ArmPlatformPkg: Remove FVP and Juno

2016-01-18 Thread Ryan Harkin
ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].

Currently, Juno and FVP exist both in EDK2's ArmPlatformPkg and in
OpenPlatformPkg.  And they are starting to diverge, with
OpenPlatformPkg being the most up-to-date with current developments.
To prevent this divergence, remove the .dsc and .fdf files from
ArmPlatformPkg and leave OpenPlatformPkg as the master.

We can't remove ArmJuno.dec yet because ACPI still uses it to set the
include path to ArmPlatform.h.

[PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64
[PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf

 ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 
---
 ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 
-
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
-
 ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
--

[1] https://git.linaro.org/uefi/OpenPlatformPkg.git
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf

2016-01-18 Thread Ryan Harkin
ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].

Currently, Juno exists both in EDK2's ArmPlatformPkg and in
OpenPlatformPkg [2].  And they are starting to diverge, with
OpenPlatformPkg being the most up-to-date with current developments.
To prevent this divergence, remove the .dsc and .fdf files from
ArmPlatformPkg and leave OpenPlatformPkg as the master.

We can't remove ArmJuno.dec yet because ACPI still uses it to set the
include path to ArmPlatform.h.

[1] https://git.linaro.org/uefi/OpenPlatformPkg.git
[2] 
https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/master:/Platforms/ARM/Juno

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ryan Harkin 
---
 ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 ---
 ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 --
 2 files changed, 656 deletions(-)

diff --git a/ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc 
b/ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc
deleted file mode 100644
index 638be2e..000
--- a/ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc
+++ /dev/null
@@ -1,291 +0,0 @@
-#
-#  Copyright (c) 2013-2015, ARM Limited. All rights reserved.
-#  Copyright (c) 2015, Intel Corporation. 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.
-#
-
-
-#
-# Defines Section - statements that will be processed to create a Makefile.
-#
-
-[Defines]
-  PLATFORM_NAME  = ArmJuno
-  PLATFORM_GUID  = ca0722fd-7d3d-45ea-948c-d62b2199807d
-  PLATFORM_VERSION   = 0.1
-  DSC_SPECIFICATION  = 0x00010005
-  OUTPUT_DIRECTORY   = Build/ArmJuno
-  SUPPORTED_ARCHITECTURES= AARCH64|ARM
-  BUILD_TARGETS  = DEBUG|RELEASE
-  SKUID_IDENTIFIER   = DEFAULT
-  FLASH_DEFINITION   = ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf
-
-# On RTSM, most peripherals are VExpress Motherboard peripherals
-!include ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
-
-[LibraryClasses.common]
-  ArmPlatformLib|ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
-  ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
-
-  
ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
-  
NorFlashPlatformLib|ArmPlatformPkg/ArmJunoPkg/Library/NorFlashJunoLib/NorFlashJunoLib.inf
-  
EfiResetSystemLib|ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.inf
-
-  TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
-
-  # USB Requirements
-  UefiUsbLib|MdePkg/Library/UefiUsbLib/UefiUsbLib.inf
-
-[LibraryClasses.ARM]
-  ArmLib|ArmPkg/Library/ArmLib/ArmV7/ArmV7Lib.inf
-
-[LibraryClasses.AARCH64]
-  ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
-
-[LibraryClasses.common.SEC]
-  PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
-  
ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf
-  
LzmaDecompressLib|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
-  
MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
-  HobLib|EmbeddedPkg/Library/PrePiHobLib/PrePiHobLib.inf
-  
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
-  PerformanceLib|MdeModulePkg/Library/PeiPerformanceLib/PeiPerformanceLib.inf
-  PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
-
-[LibraryClasses.common.SEC, LibraryClasses.common.PEIM]
-  MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
-
-[LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION, 
LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.DXE_DRIVER]
-  PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
-
-[BuildOptions]
-  *_*_*_PLATFORM_FLAGS == -I$(WORKSPACE)/ArmPlatformPkg/ArmVExpressPkg/Include 
-I$(WORKSPACE)/ArmPlatformPkg/ArmJunoPkg/Include
-
-
-#
-# Pcd Section - list of all EDK II PCD Entries defined by this Platform
-#
-
-
-[PcdsFeatureFlag.common]
-  gArmPlatformTokenSpaceGuid.PcdSystemMemoryInitializeInSec|TRUE
-
-  ## If TRUE, Graphics Output Protocol will be installed on virtual handle 
created by ConsplitterDxe.
-  #  It could be set FALSE to save size.
-  

Re: [edk2] [PATCH 0/2] ArmPlatformPkg: Remove FVP and Juno

2016-01-18 Thread Ard Biesheuvel
On 18 January 2016 at 15:29, Ryan Harkin  wrote:
> ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].
>
> Currently, Juno and FVP exist both in EDK2's ArmPlatformPkg and in
> OpenPlatformPkg.  And they are starting to diverge, with
> OpenPlatformPkg being the most up-to-date with current developments.
> To prevent this divergence, remove the .dsc and .fdf files from
> ArmPlatformPkg and leave OpenPlatformPkg as the master.
>
> We can't remove ArmJuno.dec yet because ACPI still uses it to set the
> include path to ArmPlatform.h.
>
> [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64
> [PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf
>
>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 
> ---
>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 
> -
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
> -
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
> --
>
> [1] https://git.linaro.org/uefi/OpenPlatformPkg.git

Shouldn't we sync OpenPlatformPkg with the latest EDK2 versions first?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH 0/2] ArmPlatformPkg: Remove FVP and Juno

2016-01-18 Thread Ryan Harkin
On 18 January 2016 at 15:11, Ard Biesheuvel  wrote:
> On 18 January 2016 at 16:08, Ryan Harkin  wrote:
>> On 18 January 2016 at 14:39, Ard Biesheuvel  
>> wrote:
>>> On 18 January 2016 at 15:29, Ryan Harkin  wrote:
 ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].

 Currently, Juno and FVP exist both in EDK2's ArmPlatformPkg and in
 OpenPlatformPkg.  And they are starting to diverge, with
 OpenPlatformPkg being the most up-to-date with current developments.
 To prevent this divergence, remove the .dsc and .fdf files from
 ArmPlatformPkg and leave OpenPlatformPkg as the master.

 We can't remove ArmJuno.dec yet because ACPI still uses it to set the
 include path to ArmPlatform.h.

 [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64
 [PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf

  ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 
 ---
  ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 
 -
  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
 -
  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
 --

 [1] https://git.linaro.org/uefi/OpenPlatformPkg.git
>>>
>>> Shouldn't we sync OpenPlatformPkg with the latest EDK2 versions first?
>>
>> Ooops.
>>
>> I wasn't aware of any changes in the EDK2 versions that we need to
>> carry over.  From what I can see, there are changes in OpenPlatformPkg
>> that are not in EDK2, but not the other way round, except this change:
>>
>> 660aaec  2015-12-15  ArmVExpressPkg/ArmVExpress-FVP-AArch64: run GICv3
>> in v3 mode   [Ard Biesheuvel]
>>
>> The ARM Landing Team (that'll be me!) ships GICv2 .dts files for the
>> FVP models and we don't have a plan to change to running with GICv3,
>> except in legacy mode.  So I'd revert that change anyway until I was
>> ready to support GICv3 properly.
>
> In that case, could we please leave FVP in? Unlike OpenPlatformPkg,
> EDK2 is a reference implementation, i.e., someone looking to implement
> something for his own platform containing a GICv3 should have a
> reference that makes sense,

I don't think that's a good approach either.  I could make GICv2/3 a
build option in OpenPlatformPkg.  I'd even concede to making GICv3 the
default and change my own build scripts & CI jobs to use legacy mode,
despite...

> and not some bodge that happens to work
> because you guys are only interested in GICv2 mode anyway.

Well, that's not a very nice way to put it :-P  We supported FVP
before GICv3 was working, so we had to use GICv2.  We don't had the
capacity to make and test the changes needed to support GICv3.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64

2016-01-18 Thread Ryan Harkin
On 18 January 2016 at 14:55, Laszlo Ersek  wrote:
> On 01/18/16 15:29, Ryan Harkin wrote:
>> ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].
>>
>> Currently, FVP exists both in EDK2's ArmPlatformPkg and in
>> OpenPlatformPkg [2].  And they are starting to diverge, with
>> OpenPlatformPkg being the most up-to-date with current developments.
>> To prevent this divergence, remove the .dsc and .fdf files from
>> ArmPlatformPkg and leave OpenPlatformPkg as the master.
>>
>> [1] https://git.linaro.org/uefi/OpenPlatformPkg.git
>> [2] 
>> https://git.linaro.org/uefi/OpenPlatformPkg.git/tree/master:/Platforms/ARM/VExpress
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ryan Harkin 
>> ---
>>  .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
>>  .../ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
>> -
>>  2 files changed, 718 deletions(-)
>
> Shouldn't OpenPlatformPkg be merged into edk2 first?
>
> Although, I admit that it's been many months (... hm, apparently a bit
> more than a year) that I built ArmVExpress-FVP-AArch64, so perhaps it's
> not even functional in edk2. Is that the case?
>

It works fine in edk2, it's just duplicated.  I've just found from Ard
that there's a patch in EDK2 missing from OpenPlatformPkg, but that's
under discussion on the [PATCH 0/2] email from this thread.  There are
patched in OpenPlatformPkg that are missing from EDK2, however.

I'm about to refactor all of the ARM Ltd platform support and I've
agreed to do it in OpenPlatformPkg.  I'm not up for maintaining two
copies, that's for sure.

> If it is, then there's nothing to regress with this patch I guess.
>
> Otherwise, if ArmVExpress-FVP-AArch64 does work in edk2 (just lacks
> features / fixes), then this looks like a pretty big functionality
> regression to me. I vaguely recall that Leif mentioned wanting to bring
> OpenPlatformPkg into edk2 -- can that happen first?
>
> Anyway, I've got no horse in this race; it's just that we've been
> stressing "no regressions pls".
>
> Hm... I think I found the relevant message from Leif:
>
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17544/focus=552
>
> After skimming the presentation linked therein: I guess nothing has
> changed since last July, and at this point you'd like to avoid
> fragmentation. I do find that convincing.
>
> Thanks
> Laszlo
>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc 
>> b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc
>> deleted file mode 100644
>> index af46331..000
>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc
>> +++ /dev/null
>> @@ -1,317 +0,0 @@
>> -#
>> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
>> -#  Copyright (c) 2015, Intel Corporation. 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.
>> -#
>> -#
>> -
>> -
>> -#
>> -# Defines Section - statements that will be processed to create a Makefile.
>> -#
>> -
>> -[Defines]
>> -  PLATFORM_NAME  = ArmVExpress-FVP-AArch64
>> -  PLATFORM_GUID  = 0de70077-9b3b-43bf-ba38-0ea37d77141b
>> -  PLATFORM_VERSION   = 0.1
>> -  DSC_SPECIFICATION  = 0x00010005
>> -  OUTPUT_DIRECTORY   = Build/ArmVExpress-FVP-AArch64
>> -  SUPPORTED_ARCHITECTURES= AARCH64
>> -  BUILD_TARGETS  = DEBUG|RELEASE
>> -  SKUID_IDENTIFIER   = DEFAULT
>> -  FLASH_DEFINITION   = 
>> ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf
>> -
>> -!ifndef ARM_FVP_RUN_NORFLASH
>> -  DEFINE EDK2_SKIP_PEICORE=1
>> -!endif
>> -
>> -
>> -!include ArmPlatformPkg/ArmVExpressPkg/ArmVExpress.dsc.inc
>> -
>> -[LibraryClasses.common]
>> -  ArmLib|ArmPkg/Library/ArmLib/AArch64/AArch64Lib.inf
>> -  ArmCpuLib|ArmPkg/Drivers/ArmCpuLib/ArmCortexAEMv8Lib/ArmCortexAEMv8Lib.inf
>> -  
>> ArmPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/ArmVExpressLib.inf
>> -
>> -  
>> ArmPlatformSysConfigLib|ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressSysConfigLib/ArmVExpressSysConfigLib.inf
>> -  
>> NorFlashPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/NorFlashArmVExpressLib/NorFlashArmVExpressLib.inf
>> -  
>> LcdPlatformLib|ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpressLib.inf
>> 

Re: [edk2] [PATCH 0/2] ArmPlatformPkg: Remove FVP and Juno

2016-01-18 Thread Ard Biesheuvel
On 18 January 2016 at 16:28, Ryan Harkin  wrote:
> On 18 January 2016 at 15:11, Ard Biesheuvel  wrote:
>> On 18 January 2016 at 16:08, Ryan Harkin  wrote:
>>> On 18 January 2016 at 14:39, Ard Biesheuvel  
>>> wrote:
 On 18 January 2016 at 15:29, Ryan Harkin  wrote:
> ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].
>
> Currently, Juno and FVP exist both in EDK2's ArmPlatformPkg and in
> OpenPlatformPkg.  And they are starting to diverge, with
> OpenPlatformPkg being the most up-to-date with current developments.
> To prevent this divergence, remove the .dsc and .fdf files from
> ArmPlatformPkg and leave OpenPlatformPkg as the master.
>
> We can't remove ArmJuno.dec yet because ACPI still uses it to set the
> include path to ArmPlatform.h.
>
> [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64
> [PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf
>
>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 
> ---
>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 
> -
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
> -
>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
> --
>
> [1] https://git.linaro.org/uefi/OpenPlatformPkg.git

 Shouldn't we sync OpenPlatformPkg with the latest EDK2 versions first?
>>>
>>> Ooops.
>>>
>>> I wasn't aware of any changes in the EDK2 versions that we need to
>>> carry over.  From what I can see, there are changes in OpenPlatformPkg
>>> that are not in EDK2, but not the other way round, except this change:
>>>
>>> 660aaec  2015-12-15  ArmVExpressPkg/ArmVExpress-FVP-AArch64: run GICv3
>>> in v3 mode   [Ard Biesheuvel]
>>>
>>> The ARM Landing Team (that'll be me!) ships GICv2 .dts files for the
>>> FVP models and we don't have a plan to change to running with GICv3,
>>> except in legacy mode.  So I'd revert that change anyway until I was
>>> ready to support GICv3 properly.
>>
>> In that case, could we please leave FVP in? Unlike OpenPlatformPkg,
>> EDK2 is a reference implementation, i.e., someone looking to implement
>> something for his own platform containing a GICv3 should have a
>> reference that makes sense,
>
> I don't think that's a good approach either.  I could make GICv2/3 a
> build option in OpenPlatformPkg.  I'd even concede to making GICv3 the
> default and change my own build scripts & CI jobs to use legacy mode,
> despite...
>
>> and not some bodge that happens to work
>> because you guys are only interested in GICv2 mode anyway.
>
> Well, that's not a very nice way to put it :-P  We supported FVP
> before GICv3 was working, so we had to use GICv2.  We don't had the
> capacity to make and test the changes needed to support GICv3.

Apologies if I sounded a bit abrasive there. It's just that our
objectives are not entirely aligned here, I think. Removing GICv3
support from FVP does not improve EDK2's quality as a reference
implementation, even if it runs equally well for everyone that
actually uses it.

I produce FVP snapshots here:
http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/latest/

that have the DTBs for all FVP flavors built in, and run equally well
on Foundation/GICv3 and Base with either GICv2 or v3 (since the GIC
memory address is not runtime configurable).

For me personally, that is useful since using FVP Base is a pain when
your uplink is dodgy (which tends to happen if you live in an RV) so I
use Foundation and only switch to Base if i have to, prefereably with
the exact same image. But I am ny no means the standard we should all
live by, of course.

I am happy to switch to OpenPlatformPkg for building those once it is
synced up with EDK2, but I'd prefer it if we don't start backing out
changes which are arguably correct just for convenience.

Regards,
Ard.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH 0/2] ArmPlatformPkg: Remove FVP and Juno

2016-01-18 Thread Ryan Harkin
On 18 January 2016 at 15:38, Ard Biesheuvel  wrote:
> On 18 January 2016 at 16:28, Ryan Harkin  wrote:
>> On 18 January 2016 at 15:11, Ard Biesheuvel  
>> wrote:
>>> On 18 January 2016 at 16:08, Ryan Harkin  wrote:
 On 18 January 2016 at 14:39, Ard Biesheuvel  
 wrote:
> On 18 January 2016 at 15:29, Ryan Harkin  wrote:
>> ARM Ltd Platform support is migrating to use OpenPlatformPkg [1].
>>
>> Currently, Juno and FVP exist both in EDK2's ArmPlatformPkg and in
>> OpenPlatformPkg.  And they are starting to diverge, with
>> OpenPlatformPkg being the most up-to-date with current developments.
>> To prevent this divergence, remove the .dsc and .fdf files from
>> ArmPlatformPkg and leave OpenPlatformPkg as the master.
>>
>> We can't remove ArmJuno.dec yet because ACPI still uses it to set the
>> include path to ArmPlatform.h.
>>
>> [PATCH 1/2] ArmPlatformPkg: remove ArmVExpress-FVP-AArch64
>> [PATCH 2/2] ArmPlatformPkg: remove ArmJuno.dsc/fdf
>>
>>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.dsc | 291 
>> ---
>>  ArmPlatformPkg/ArmJunoPkg/ArmJuno.fdf | 365 
>> -
>>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.dsc | 317 
>> -
>>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpress-FVP-AArch64.fdf | 401 
>> --
>>
>> [1] https://git.linaro.org/uefi/OpenPlatformPkg.git
>
> Shouldn't we sync OpenPlatformPkg with the latest EDK2 versions first?

 Ooops.

 I wasn't aware of any changes in the EDK2 versions that we need to
 carry over.  From what I can see, there are changes in OpenPlatformPkg
 that are not in EDK2, but not the other way round, except this change:

 660aaec  2015-12-15  ArmVExpressPkg/ArmVExpress-FVP-AArch64: run GICv3
 in v3 mode   [Ard Biesheuvel]

 The ARM Landing Team (that'll be me!) ships GICv2 .dts files for the
 FVP models and we don't have a plan to change to running with GICv3,
 except in legacy mode.  So I'd revert that change anyway until I was
 ready to support GICv3 properly.
>>>
>>> In that case, could we please leave FVP in? Unlike OpenPlatformPkg,
>>> EDK2 is a reference implementation, i.e., someone looking to implement
>>> something for his own platform containing a GICv3 should have a
>>> reference that makes sense,
>>
>> I don't think that's a good approach either.  I could make GICv2/3 a
>> build option in OpenPlatformPkg.  I'd even concede to making GICv3 the
>> default and change my own build scripts & CI jobs to use legacy mode,
>> despite...
>>
>>> and not some bodge that happens to work
>>> because you guys are only interested in GICv2 mode anyway.
>>
>> Well, that's not a very nice way to put it :-P  We supported FVP
>> before GICv3 was working, so we had to use GICv2.  We don't had the
>> capacity to make and test the changes needed to support GICv3.
>
> Apologies if I sounded a bit abrasive there.

No offence taken :-)

> It's just that our
> objectives are not entirely aligned here, I think. Removing GICv3
> support from FVP does not improve EDK2's quality as a reference
> implementation, even if it runs equally well for everyone that
> actually uses it.
>
> I produce FVP snapshots here:
> http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/latest/
>
> that have the DTBs for all FVP flavors built in, and run equally well
> on Foundation/GICv3 and Base with either GICv2 or v3 (since the GIC
> memory address is not runtime configurable).
>
> For me personally, that is useful since using FVP Base is a pain when
> your uplink is dodgy (which tends to happen if you live in an RV) so I
> use Foundation and only switch to Base if i have to, prefereably with
> the exact same image. But I am ny no means the standard we should all
> live by, of course.
>
> I am happy to switch to OpenPlatformPkg for building those once it is
> synced up with EDK2, but I'd prefer it if we don't start backing out
> changes which are arguably correct just for convenience.
>

You don't say if you're happy with my idea of making GICv3 default but
optional in OpenPlatformPkg...  although I can't see why you would
object, unless you have a stronger than normal pet hate for ifdef
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [patch V2 3/3] MdeModulePkg: Add MorLock to variable driver.

2016-01-18 Thread Zhang, Chao B
Reviewed-by: Chao Zhang 





Thanks & Best regards
Chao Zhang


-Original Message-
From: Yao, Jiewen 
Sent: Monday, January 18, 2016 2:52 PM
To: edk2-de...@ml01.01.org
Cc: Yao, Jiewen; Zhang, Chao B; Zeng, Star
Subject: [patch V2 3/3] MdeModulePkg: Add MorLock to variable driver.

This patch adds MorLock function to Variable main function.
It also updates corresponding INF file to pass build.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: "Yao, Jiewen" 
Cc: "Zhang, Chao B" 
Cc: "Zeng, Star" 
---
 .../Universal/Variable/RuntimeDxe/Variable.c   | 60 +-
 .../Variable/RuntimeDxe/VariableRuntimeDxe.inf |  6 ++-
 .../Universal/Variable/RuntimeDxe/VariableSmm.inf  |  6 ++-
 3 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 2dc3038..5e39d44 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -16,7 +16,7 @@
   VariableServiceSetVariable() should also check authenticate data to avoid 
buffer overflow,
   integer overflow. It should also check attribute to avoid authentication 
bypass.
 
-Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
 (C) Copyright 2015 Hewlett Packard Enterprise Development LP  This program 
and the accompanying materials  are licensed and made available under the terms 
and conditions of the BSD License @@ -112,6 +112,43 @@ SecureBootHook (
   );
 
 /**
+  Initialization for MOR Lock Control.
+
+  @retval EFI_SUCEESS MorLock initialization success.
+  @return Others  Some error occurs.
+**/
+EFI_STATUS
+MorLockInit (
+  VOID
+  );
+
+/**
+  This service is an MOR/MorLock checker handler for the SetVariable().
+
+  @param  VariableName the name of the vendor's variable, as a
+   Null-Terminated Unicode String
+  @param  VendorGuid   Unify identifier for vendor.
+  @param  Attributes   Point to memory location to return the attributes of 
variable. If the point
+   is NULL, the parameter would be ignored.
+  @param  DataSize The size in bytes of Data-Buffer.
+  @param  Data Point to the content of the variable.
+
+  @retval  EFI_SUCCESSThe MOR/MorLock check pass, and Variable 
driver can store the variable data.
+  @retval  EFI_INVALID_PARAMETER  The MOR/MorLock data or data size or 
attributes is not allowed for MOR variable.
+  @retval  EFI_ACCESS_DENIED  The MOR/MorLock is locked.
+  @retval  EFI_ALREADY_STARTEDThe MorLock variable is handled inside this 
function.
+  Variable driver can just return EFI_SUCCESS.
+**/
+EFI_STATUS
+SetVariableCheckHandlerMor (
+  IN CHAR16 *VariableName,
+  IN EFI_GUID   *VendorGuid,
+  IN UINT32 Attributes,
+  IN UINTN  DataSize,
+  IN VOID   *Data
+  );
+
+/**
   Routine used to track statistical information about variable usage.
   The data is stored in the EFI system table so it can be accessed later.
   VariableInfo.efi can dump out the table. Only Boot Services variable @@ 
-3192,6 +3229,21 @@ VariableServiceSetVariable (
 }
   }
 
+  //
+  // Special Handling for MOR Lock variable.
+  //
+  Status = SetVariableCheckHandlerMor (VariableName, VendorGuid, 
+ Attributes, PayloadSize, (VOID *) ((UINTN) Data + DataSize - PayloadSize));  
if (Status == EFI_ALREADY_STARTED) {
+//
+// EFI_ALREADY_STARTED means the SetVariable() action is handled inside of 
SetVariableCheckHandlerMor().
+// Variable driver can just return SUCCESS.
+//
+return EFI_SUCCESS;
+  }
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
   Status = VarCheckLibSetVariableCheck (VariableName, VendorGuid, Attributes, 
PayloadSize, (VOID *) ((UINTN) Data + DataSize - PayloadSize), mRequestSource);
   if (EFI_ERROR (Status)) {
 return Status;
@@ -3966,6 +4018,12 @@ VariableWriteServiceInitialize (
   }
 
   ReleaseLockOnlyAtBootTime 
(>VariableGlobal.VariableServicesLock);
+
+  //
+  // Initialize MOR Lock variable.
+  //
+  MorLockInit ();
+
   return Status;
 }
 
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
index 62c1568..da9b8bb 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
@@ -9,7 +9,7 @@
 #  This external input must be validated carefully to avoid security issues 
such as  #  buffer overflow or integer overflow.
 #
-# Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+# Copyright (c) 2006 - 2016, Intel Corporation. All rights 
+reserved.
 # This program and the 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Laszlo Ersek
On 01/16/16 15:46, Zeng, Star wrote:
> On 2016/1/16 1:39, Laszlo Ersek wrote:
>> On 01/15/16 18:33, Ryan Harkin wrote:
>>> On 15 January 2016 at 17:05, Laszlo Ersek  wrote:
 Hi,

 snipping context liberally...

> Whilst simple text input seems to work ok, cursor support
> does not.
> And we need cursor support for Intel BDS.

 (1) I think this is important. See below.

> When trying to revert your patch on the latest trunk code, the build
> fails because your patch is doing too many things in one step.  It
> should have been three (or more) patches, in my opinion.

 (2)

 (Caveat: what I'm about to say / reference here may not apply fully.)

 I just want to say that I followed / partially reviewed Star's patch
 series (earlier versions of it than v4), and I had the exact same
 impression (= "this is too big"), about the patch that did more or
 less the same for ArmVirtPkg.

 Surprisingly :), I was wrong. Please see the message of commit

 commit ad7f6bc2e1163125cdb26f6b0e6695554028626a
 Author: Star Zeng 
 Date:   Thu Nov 26 08:52:12 2015 +

  ArmVirtPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

 Also, this sub-thread could be interesting:

 http://thread.gmane.org/gmane.comp.bios.edk2.devel/4582/focus=4593

>>>
>>> I don't think this is the same as ARM platform support.  I think there
>>> could have been a single patch to move the code from ExtLib to Lib.
>>> Another to remove ExtLib and another to change from to MdeModulePkg.
>>>
>>>
>
> Instead of reverting, I manually added back PL011SerialPortExtLib and
> removed the code from PL011SerialPortLib at the same time.  Then, the
> code did not compile.
>
> So I changed my patch to also make PL011SerialPortLib return 0 instead
> of calling the PL011 functions.  And everything started to work.  I
> assumed this was because I added back the ExtLib, not because I
> changed the added code.  I was wrong.
>
> I've just pushed a new branch, serialdxe-fix-006, that only changes
> PL011SerialPortLib.c to "return 0;" for all the new functions - and
> that works also.
>
> So I don't need PL011SerialPortExtLib at all.  I guess it was never
> called from there?  When you moved the code to PL011SerialPortLib, it
> started getting called - and the code is causing problems.
>
>
>> Could you check out to *SHA-1:
>> 1b96428d92c01383dc437717db679f57cf70d980*
>> that just before my SerialDxe series changes, and help to confirm
>> if there
>> is regression?
>>
>
> I already checked out the commit before yours [1] and it works fine.
>
> [1] git checkout 921e987b2b26602dc85eaee856d494b97b6e02b0^

 (3) Alright, so SVN r18974 (git 1bccab910700) is the commit that
 removes the old SerialDxe from EmbeddedPkg. At that stage, all
 client packages have been converted to the new driver in
 MdeModulePkg. Therefore, if we want to compare the two *drivers*, we
 have to check out the parent of this commit (that is, check out SVN
 r18973 == git 1bccab910700^ == git ad7f6bc2e1), and compare the
 following files:

 - EmbeddedPkg/SerialDxe/SerialIo.c
 - MdeModulePkg/Universal/SerialDxe/SerialIo.c

 I don't recommend to diff them -- instead, open them both side by
 side, and read them in parallel.

 This way I noticed the following difference:

 (a) In the EmbeddedPkg driver, we have the following *initial*
 values (not quoting everything, just what I think is relevant):

EFI_SERIAL_IO_PROTOCOL.Mode->Timeout  = 0
EFI_SERIAL_IO_PROTOCOL.Mode->ReceiveFifoDepth = 1

 (see gSerialIoTemplate and gSerialIoMode), however the SerialReset()
 function (implementing the Reset protocol member) sets the
 following, different values:

EFI_SERIAL_IO_PROTOCOL.Mode->Timeout  = 100
EFI_SERIAL_IO_PROTOCOL.Mode->ReceiveFifoDepth = 0

 Inconsistent, right? But that's not the difference I think is relevant:

 (b) The MdeModulePkg driver stores the following settings *both* at
 driver initialization and in SerialReset():

EFI_SERIAL_IO_PROTOCOL.Mode->Timeout  = 0
EFI_SERIAL_IO_PROTOCOL.Mode->ReceiveFifoDepth = 1

 (See mSerialIoTemplate and mSerialIoMode.)

 That is, the initial state for Mode is identical between the (a) old
 and (b) new drivers. However, these relevant-looking fields *differ*
 between old and new driver after a client calls
 EFI_SERIAL_IO_PROTOCOL.Reset(). Then the old driver sets a
 ReceiveFifoDepth of zero, while the new driver sets 1. (I'll ignore
 Timeout for now.)

 The UEFI spec says the following about 

Re: [edk2] Transition to GitHub Update

2016-01-18 Thread Laszlo Ersek
On 01/16/16 15:25, Ard Biesheuvel wrote:
> On 15 January 2016 at 22:14, Jarlstrom, Laurie
>  wrote:
>> To: EDK II Community
>>
>> This message is an update on the transition from SourceForge to GitHub for 
>> EDK II development.  The schedule is currently targeting the last week of 
>> January or the first week of February to perform the transition.  The 
>> transition process should only take one to two days to complete.  A 
>> notification message will be sent the week prior to the actual dates that 
>> the repositories will be impacted.  This should provide adequate 
>> notification for the scheduled down time.
>> As part of this transition some documentation and process changes have been 
>> required.  The updated process as well as other GitHub specific information 
>> can be found on the tianocore Wiki at the link provided below.  Feedback on 
>> this documentation is welcome and needed to make sure the transition is as 
>> smooth as possible.
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/SourceForge-to-Github-Quick-Start
>>
>> Please post any comments or questions related to this transition to the 
>> edk2-devel mailing list or reply to the email message.
>>
> 
> Hello,
> 
> Thanks for getting all of this set up.
> 
> Regarding the following line
> 
> 'Commits should build / boot if possible to support use of bisect'
> 
> could we make that a bit stronger please? Breaking bisect is really
> not ok, and it usually does not take that much extra work to avoid it,
> as long as you are aware of it. Given that many people are new to Git,
> presenting it as an opt-in feature like this does not send the right
> message imo.
> 
> In fact, having some guidelines about how to avoid breaking bisect
> could be helpful, especially that something like adding some temporary
> code in an early patch only to remove it again at the end of the
> series is perfectly ok, and highly preferred over losing
> bisectability. Not everybody might expect that.

I agree completely. Minimally the one package that the developer uses
for developing the series must build at each stage in the series, with
at least the developer's preferred toolchain and build flags.

Thanks
Laszlo


> 
> Thanks,
> Ard.
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [Patch] BaseTools: process the files by the priority in BUILDRULEORDER

2016-01-18 Thread Laszlo Ersek
Hello Zhu Yonghong,

On 01/18/16 08:36, Yonghong Zhu wrote:
> By the BUILDRULEORDER feature to process files listed in INF [Sources]
> sections in priority order, if a filename is listed with multiple
> extensions, the tools will use only the file that matches the first
> extension in the space separated list.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Yonghong Zhu 
> ---
>  BaseTools/Source/Python/AutoGen/AutoGen.py | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)

does this version address the issue that we found in the following threads?

http://thread.gmane.org/gmane.comp.bios.edk2.devel/5192/focus=5492
http://thread.gmane.org/gmane.comp.bios.edk2.devel/5485/focus=5501

Thanks!
Laszlo


> 
> diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
> b/BaseTools/Source/Python/AutoGen/AutoGen.py
> index abac477..e9f4888 100644
> --- a/BaseTools/Source/Python/AutoGen/AutoGen.py
> +++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
> @@ -1,9 +1,9 @@
>  ## @file
>  # Generate AutoGen.h, AutoGen.c and *.depex files
>  #
> -# Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.
> +# Copyright (c) 2007 - 2016, Intel Corporation. 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
>  #
> @@ -2739,13 +2739,40 @@ class ModuleAutoGen(AutoGen):
>  
>  # add the file path into search path list for file including
>  if F.Dir not in self.IncludePathList and self.AutoGenVersion 
> >= 0x00010005:
>  self.IncludePathList.insert(0, F.Dir)
>  self._SourceFileList.append(F)
> +
> +self._MatchBuildRuleOrder(self._SourceFileList)
> +
> +for F in self._SourceFileList:
>  self._ApplyBuildRule(F, TAB_UNKNOWN_FILE)
>  return self._SourceFileList
>  
> +def _MatchBuildRuleOrder(self, FileList):
> +Order_Dict = {}
> +self._GetModuleBuildOption()
> +for SingleFile in FileList:
> +if self.BuildRuleOrder and SingleFile.Ext in self.BuildRuleOrder 
> and SingleFile.Ext in self.BuildRules:
> +key = SingleFile.Path.split(SingleFile.Ext)[0]
> +if key in Order_Dict:
> +Order_Dict[key].append(SingleFile.Ext)
> +else:
> +Order_Dict[key] = [SingleFile.Ext]
> +
> +RemoveList = []
> +for F in Order_Dict:
> +if len(Order_Dict[F]) > 1:
> +Order_Dict[F].sort(key=lambda i: 
> self.BuildRuleOrder.index(i))
> +for Ext in Order_Dict[F][1:]:
> +RemoveList.append(F + Ext)
> +   
> +for item in RemoveList:
> +FileList.remove(item)
> +
> +return FileList
> +
>  ## Return the list of unicode files
>  def _GetUnicodeFileList(self):
>  if self._UnicodeFileList == None:
>  if TAB_UNICODE_FILE in self.FileTypes:
>  self._UnicodeFileList = self.FileTypes[TAB_UNICODE_FILE]
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Zeng, Star

[...]



The above analysis is very clear, thanks. I am a little concern about if
the code changes below follow the comments in the code.

In Terminal.c:
 //
 // Set the timeout value of serial buffer for
 // keystroke response performance issue
 //
In TerminalConIn.c:
   //
   //  if current timeout value for serial device is not identical with
   //  the value saved in TERMINAL_DEV structure, then recalculate the
   //  timeout value again and set serial attribute according to this value.
   //


Any comments about above concern?



And I also checked the
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM
platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH
value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then
SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of
the functions is "A ReceiveFifoDepth value of 0 will use the device's
default FIFO depth", so what's the device's default FIFO depth?


Well, that's exactly what SerialDxe delegates to SerialPortLib :)
SerialDxe need not be aware of the device-specific value; the platform
will provide a device-specific library instance.


I have a thought that updates SerialDxe to call SerialSetAttributes(with
0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize()
and SerialReset() to get the real default values of the device, then the
driver can also eliminate the use of PcdUartDefault.

What's your opinion?


I don't think this is a good idea -- the UEFI spec is very clear on
this. Under "11.8 Serial I/O Protocol", in the general description part,
you find:

 The default attributes for all UART-style serial device interfaces
 are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond
 timeout per character, no parity, 8 data bits, and 1 stop bit. [...]

So this is what must be in effect right after initialization, and --
with all likelihood -- after re-set as well.

If a Serial IO protocol client is not content with these settings, then
it must explicitly change them, with the SetAttributes() call.


So TerminalDxe could know it is not content with the settings?
Should platform BDS code to explicitly change them with the 
SetAttributes() call?


Thanks,
Star



Thanks
Laszlo


[...]

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Laszlo Ersek
On 01/18/16 17:17, Brian J. Johnson wrote:
> On 01/18/2016 05:55 AM, Laszlo Ersek wrote:
>> On 01/18/16 12:41, Ryan Harkin wrote:
>>> >On 18 January 2016 at 11:33, Laszlo Ersek  wrote:
 >>On 01/18/16 11:24, Zeng, Star wrote:
> >>>[...]
> >>>
>>> >
>>> >The above analysis is very clear, thanks. I am a little
>>> concern about if
>>> >the code changes below follow the comments in the code.
>>> >
>>> >In Terminal.c:
>>> >  //
>>> >  // Set the timeout value of serial buffer for
>>> >  // keystroke response performance issue
>>> >  //
>>> >In TerminalConIn.c:
>>> >//
>>> >//  if current timeout value for serial device is not
>>> identical with
>>> >//  the value saved in TERMINAL_DEV structure, then
>>> recalculate the
>>> >//  timeout value again and set serial attribute
>>> according to this
>>> >value.
>>> >//
> >>>
> >>>Any comments about above concern?
 >>
 >>Not really.
 >>
 >>I don't know what the purpose of the Timeout calculation is (what
 is the
 >>"keystroke response performance issue"?), but in any case, it is a
 good
 >>sign that TerminalDxe sets*some*  Timeout explicitly.
 >>
 >>Plus, it has worked reliably until now, so we shouldn't change the
 >>Timeout argument.
 >>
>>> >
>>> >I think "reliably" is a bit generous;-)   The most common complaint I
>>> >get about EDK2 is that it can't handle more than a FIFO's worth of
>>> >pasted characters on the serial port.
>> I tend to think this comes, at least partly, from the fact that UEFI
>> doesn't do interrupt-driven drivers. There's timer based polling (in
>> drivers), and there are busy loops (in applications, I guess).
>>
>> I think you'll see the same behavior with network packet reception.
>>
>> I said "reliably" beause in my environment I've had practically zero
>> issues with the serial terminal (beyond the one, very lacking, textual
>> resolution list -- but I patched that for myself permanently).
>>
>> Thanks
>> Laszlo
> 
> Can't you just enable hardware flow control (RTS/CTS) on the UART?

>From the spec:

Flow control is the responsibility of the software that uses the
protocol. Hardware flow control can be implemented through the use
of the GetControl() and SetControl() functions (described below) to
monitor and assert the flow control signals. The XON/XOFF flow
control algorithm can be implemented in software by inserting XON
and XOFF characters into the serial data stream as required.

(That's all I know about flow control over serial. :))

Grepping for EFI_SERIAL_HARDWARE_FLOW_CONTROL_ENABLE and
UART_FLOW_CONTROL_HARDWARE yields some results, but none in TerminalDxe.

Which makes me believe that most of the platform-specific serial IO
implementations offer hw flow control, but TerminalDxe doesn't use it.

Interestingly though, some hits are from the (now deprecated) Intel BDS:
"IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint".

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Laszlo Ersek
On 01/18/16 12:41, Ryan Harkin wrote:
> On 18 January 2016 at 11:33, Laszlo Ersek  wrote:
>> On 01/18/16 11:24, Zeng, Star wrote:
>>> [...]
>>>
>
> The above analysis is very clear, thanks. I am a little concern about if
> the code changes below follow the comments in the code.
>
> In Terminal.c:
>  //
>  // Set the timeout value of serial buffer for
>  // keystroke response performance issue
>  //
> In TerminalConIn.c:
>//
>//  if current timeout value for serial device is not identical with
>//  the value saved in TERMINAL_DEV structure, then recalculate the
>//  timeout value again and set serial attribute according to this
> value.
>//
>>>
>>> Any comments about above concern?
>>
>> Not really.
>>
>> I don't know what the purpose of the Timeout calculation is (what is the
>> "keystroke response performance issue"?), but in any case, it is a good
>> sign that TerminalDxe sets *some* Timeout explicitly.
>>
>> Plus, it has worked reliably until now, so we shouldn't change the
>> Timeout argument.
>>
> 
> I think "reliably" is a bit generous ;-)  The most common complaint I
> get about EDK2 is that it can't handle more than a FIFO's worth of
> pasted characters on the serial port.

I tend to think this comes, at least partly, from the fact that UEFI
doesn't do interrupt-driven drivers. There's timer based polling (in
drivers), and there are busy loops (in applications, I guess).

I think you'll see the same behavior with network packet reception.

I said "reliably" beause in my environment I've had practically zero
issues with the serial terminal (beyond the one, very lacking, textual
resolution list -- but I patched that for myself permanently).

Thanks
Laszlo

> 
>>>
>
> And I also checked the
> IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM
> platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH
> value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then
> SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of
> the functions is "A ReceiveFifoDepth value of 0 will use the device's
> default FIFO depth", so what's the device's default FIFO depth?

 Well, that's exactly what SerialDxe delegates to SerialPortLib :)
 SerialDxe need not be aware of the device-specific value; the platform
 will provide a device-specific library instance.

> I have a thought that updates SerialDxe to call SerialSetAttributes(with
> 0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize()
> and SerialReset() to get the real default values of the device, then the
> driver can also eliminate the use of PcdUartDefault.
>
> What's your opinion?

 I don't think this is a good idea -- the UEFI spec is very clear on
 this. Under "11.8 Serial I/O Protocol", in the general description part,
 you find:

  The default attributes for all UART-style serial device interfaces
  are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond
  timeout per character, no parity, 8 data bits, and 1 stop bit. [...]

 So this is what must be in effect right after initialization, and --
 with all likelihood -- after re-set as well.

 If a Serial IO protocol client is not content with these settings, then
 it must explicitly change them, with the SetAttributes() call.
>>>
>>> So TerminalDxe could know it is not content with the settings?
>>
>> I think so, yes. TerminalDxe knows in advance that it will handle escape
>> sequences (= bursts of scan codes). The longest such sequence (3 or 4
>> scan codes I guess?) should fit in the receive FIFO.
>>
>> So TerminalDxe could try to figure out the longest sequence it wishes to
>> handle, then configure such a receive FIFO depth with SerialIO.
>>
>> Or else, what I think would be better, just call SetAttributes() from
>> the platform's SerialPortLib instance, with ReceiveFifoDepth=0:
>> - in practice, this would restore the previous behavior,
>> - I expect that this value should enable the FIFO on most UARTs, with a
>>   sufficient depth for the escape sequence processing
>>
>>> Should platform BDS code to explicitly change them with the
>>> SetAttributes() call?
>>
>> Well, PlatformBDS is the ultimate fallback, always :), but I think
>> TerminalDxe has much more business dealing with the Serial IO
>> attributes. TerminalDxe already modifies the Timeout attribute, and in
>> the end, the FIFO depth too is important to TerminalDxe (for escape seq
>> processing). I think it's not a BDS responsibility.
>>
>> Consider for example
>> "MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf". This is a
>> UEFI driver (unlike SerialDxe, which is a DXE_DRIVER).
>>
>> If PlatformBDS wanted to control the receive FIFO depth of the SerialIo
>> protocol instance procuded by 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Laszlo Ersek
On 01/18/16 11:24, Zeng, Star wrote:
> [...]
> 
>>>
>>> The above analysis is very clear, thanks. I am a little concern about if
>>> the code changes below follow the comments in the code.
>>>
>>> In Terminal.c:
>>>  //
>>>  // Set the timeout value of serial buffer for
>>>  // keystroke response performance issue
>>>  //
>>> In TerminalConIn.c:
>>>//
>>>//  if current timeout value for serial device is not identical with
>>>//  the value saved in TERMINAL_DEV structure, then recalculate the
>>>//  timeout value again and set serial attribute according to this
>>> value.
>>>//
> 
> Any comments about above concern?

Not really.

I don't know what the purpose of the Timeout calculation is (what is the
"keystroke response performance issue"?), but in any case, it is a good
sign that TerminalDxe sets *some* Timeout explicitly.

Plus, it has worked reliably until now, so we shouldn't change the
Timeout argument.

> 
>>>
>>> And I also checked the
>>> IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM
>>> platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH
>>> value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then
>>> SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of
>>> the functions is "A ReceiveFifoDepth value of 0 will use the device's
>>> default FIFO depth", so what's the device's default FIFO depth?
>>
>> Well, that's exactly what SerialDxe delegates to SerialPortLib :)
>> SerialDxe need not be aware of the device-specific value; the platform
>> will provide a device-specific library instance.
>>
>>> I have a thought that updates SerialDxe to call SerialSetAttributes(with
>>> 0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize()
>>> and SerialReset() to get the real default values of the device, then the
>>> driver can also eliminate the use of PcdUartDefault.
>>>
>>> What's your opinion?
>>
>> I don't think this is a good idea -- the UEFI spec is very clear on
>> this. Under "11.8 Serial I/O Protocol", in the general description part,
>> you find:
>>
>>  The default attributes for all UART-style serial device interfaces
>>  are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond
>>  timeout per character, no parity, 8 data bits, and 1 stop bit. [...]
>>
>> So this is what must be in effect right after initialization, and --
>> with all likelihood -- after re-set as well.
>>
>> If a Serial IO protocol client is not content with these settings, then
>> it must explicitly change them, with the SetAttributes() call.
> 
> So TerminalDxe could know it is not content with the settings?

I think so, yes. TerminalDxe knows in advance that it will handle escape
sequences (= bursts of scan codes). The longest such sequence (3 or 4
scan codes I guess?) should fit in the receive FIFO.

So TerminalDxe could try to figure out the longest sequence it wishes to
handle, then configure such a receive FIFO depth with SerialIO.

Or else, what I think would be better, just call SetAttributes() from
the platform's SerialPortLib instance, with ReceiveFifoDepth=0:
- in practice, this would restore the previous behavior,
- I expect that this value should enable the FIFO on most UARTs, with a
  sufficient depth for the escape sequence processing

> Should platform BDS code to explicitly change them with the
> SetAttributes() call?

Well, PlatformBDS is the ultimate fallback, always :), but I think
TerminalDxe has much more business dealing with the Serial IO
attributes. TerminalDxe already modifies the Timeout attribute, and in
the end, the FIFO depth too is important to TerminalDxe (for escape seq
processing). I think it's not a BDS responsibility.

Consider for example
"MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf". This is a
UEFI driver (unlike SerialDxe, which is a DXE_DRIVER).

If PlatformBDS wanted to control the receive FIFO depth of the SerialIo
protocol instance procuded by PciSioSerialDxe, then it would have to
connect PciIo instances (so SerialIo would exist), then set the
attribute, then connect SerialIo recursively (so that TerminalDxe would
sit on top). In other words, PlatformBds would have to intrude to the
UEFI driver model -- a full bottom-up recursive ConnectAll would no
longer work.

Also, the user could disconnect/reconnect the SerialIo protocol provider
in the UEFI shell; maybe even unload/reload the driver. So I think
setting the attributes is the responsibility of the driver that sets
directly atop.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Ryan Harkin
On 18 January 2016 at 11:33, Laszlo Ersek  wrote:
> On 01/18/16 11:24, Zeng, Star wrote:
>> [...]
>>

 The above analysis is very clear, thanks. I am a little concern about if
 the code changes below follow the comments in the code.

 In Terminal.c:
  //
  // Set the timeout value of serial buffer for
  // keystroke response performance issue
  //
 In TerminalConIn.c:
//
//  if current timeout value for serial device is not identical with
//  the value saved in TERMINAL_DEV structure, then recalculate the
//  timeout value again and set serial attribute according to this
 value.
//
>>
>> Any comments about above concern?
>
> Not really.
>
> I don't know what the purpose of the Timeout calculation is (what is the
> "keystroke response performance issue"?), but in any case, it is a good
> sign that TerminalDxe sets *some* Timeout explicitly.
>
> Plus, it has worked reliably until now, so we shouldn't change the
> Timeout argument.
>

I think "reliably" is a bit generous ;-)  The most common complaint I
get about EDK2 is that it can't handle more than a FIFO's worth of
pasted characters on the serial port.

>>

 And I also checked the
 IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM
 platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH
 value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then
 SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of
 the functions is "A ReceiveFifoDepth value of 0 will use the device's
 default FIFO depth", so what's the device's default FIFO depth?
>>>
>>> Well, that's exactly what SerialDxe delegates to SerialPortLib :)
>>> SerialDxe need not be aware of the device-specific value; the platform
>>> will provide a device-specific library instance.
>>>
 I have a thought that updates SerialDxe to call SerialSetAttributes(with
 0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize()
 and SerialReset() to get the real default values of the device, then the
 driver can also eliminate the use of PcdUartDefault.

 What's your opinion?
>>>
>>> I don't think this is a good idea -- the UEFI spec is very clear on
>>> this. Under "11.8 Serial I/O Protocol", in the general description part,
>>> you find:
>>>
>>>  The default attributes for all UART-style serial device interfaces
>>>  are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond
>>>  timeout per character, no parity, 8 data bits, and 1 stop bit. [...]
>>>
>>> So this is what must be in effect right after initialization, and --
>>> with all likelihood -- after re-set as well.
>>>
>>> If a Serial IO protocol client is not content with these settings, then
>>> it must explicitly change them, with the SetAttributes() call.
>>
>> So TerminalDxe could know it is not content with the settings?
>
> I think so, yes. TerminalDxe knows in advance that it will handle escape
> sequences (= bursts of scan codes). The longest such sequence (3 or 4
> scan codes I guess?) should fit in the receive FIFO.
>
> So TerminalDxe could try to figure out the longest sequence it wishes to
> handle, then configure such a receive FIFO depth with SerialIO.
>
> Or else, what I think would be better, just call SetAttributes() from
> the platform's SerialPortLib instance, with ReceiveFifoDepth=0:
> - in practice, this would restore the previous behavior,
> - I expect that this value should enable the FIFO on most UARTs, with a
>   sufficient depth for the escape sequence processing
>
>> Should platform BDS code to explicitly change them with the
>> SetAttributes() call?
>
> Well, PlatformBDS is the ultimate fallback, always :), but I think
> TerminalDxe has much more business dealing with the Serial IO
> attributes. TerminalDxe already modifies the Timeout attribute, and in
> the end, the FIFO depth too is important to TerminalDxe (for escape seq
> processing). I think it's not a BDS responsibility.
>
> Consider for example
> "MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf". This is a
> UEFI driver (unlike SerialDxe, which is a DXE_DRIVER).
>
> If PlatformBDS wanted to control the receive FIFO depth of the SerialIo
> protocol instance procuded by PciSioSerialDxe, then it would have to
> connect PciIo instances (so SerialIo would exist), then set the
> attribute, then connect SerialIo recursively (so that TerminalDxe would
> sit on top). In other words, PlatformBds would have to intrude to the
> UEFI driver model -- a full bottom-up recursive ConnectAll would no
> longer work.
>
> Also, the user could disconnect/reconnect the SerialIo protocol provider
> in the UEFI shell; maybe even unload/reload the driver. So I think
> setting the attributes is the responsibility of the driver that sets
> directly atop.
>
> Thanks
> Laszlo
___

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Brian J. Johnson

On 01/18/2016 05:55 AM, Laszlo Ersek wrote:

On 01/18/16 12:41, Ryan Harkin wrote:

>On 18 January 2016 at 11:33, Laszlo Ersek  wrote:

>>On 01/18/16 11:24, Zeng, Star wrote:

>>>[...]
>>>

>
>The above analysis is very clear, thanks. I am a little concern about if
>the code changes below follow the comments in the code.
>
>In Terminal.c:
>  //
>  // Set the timeout value of serial buffer for
>  // keystroke response performance issue
>  //
>In TerminalConIn.c:
>//
>//  if current timeout value for serial device is not identical with
>//  the value saved in TERMINAL_DEV structure, then recalculate the
>//  timeout value again and set serial attribute according to this
>value.
>//

>>>
>>>Any comments about above concern?

>>
>>Not really.
>>
>>I don't know what the purpose of the Timeout calculation is (what is the
>>"keystroke response performance issue"?), but in any case, it is a good
>>sign that TerminalDxe sets*some*  Timeout explicitly.
>>
>>Plus, it has worked reliably until now, so we shouldn't change the
>>Timeout argument.
>>

>
>I think "reliably" is a bit generous;-)   The most common complaint I
>get about EDK2 is that it can't handle more than a FIFO's worth of
>pasted characters on the serial port.

I tend to think this comes, at least partly, from the fact that UEFI
doesn't do interrupt-driven drivers. There's timer based polling (in
drivers), and there are busy loops (in applications, I guess).

I think you'll see the same behavior with network packet reception.

I said "reliably" beause in my environment I've had practically zero
issues with the serial terminal (beyond the one, very lacking, textual
resolution list -- but I patched that for myself permanently).

Thanks
Laszlo


Can't you just enable hardware flow control (RTS/CTS) on the UART?
--

Brian J. Johnson


  Email:   bjohn...@sgi.com   U.S. Mail: SGI
  Office:  (651) 683-75212750 Blue Water Road
  VNET:6-233-7521Eagan, MN  55121-1400
  Fax: (651) 683-7696
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [patch 1/2] MdeModulePkg/Ide: return correct status when DRQ is not ready for ATAPI

2016-01-18 Thread Laszlo Ersek
On 01/18/16 19:44, John Snow wrote:
> 
> 
> On 01/18/2016 05:57 AM, Laszlo Ersek wrote:
>> Hello Feng, John,
>>
>> On 11/03/15 02:48, Tian Feng wrote:
>>> When executing ATAPI cmd at IDE mode, EFI_SUCCESS may be returned wrongly
>>> with old logic but in fact DRQ is not ready and the transaction doesn't
>>> get executed correctly at this time.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Feng Tian 
>>> Cc: Star Zeng 
>>> ---
>>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>>> index 4928ed5..f74e5ca 100644
>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>>> @@ -1949,7 +1949,7 @@ AtaPacketReadWrite (
>>>  //
>>>  Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
>>>  if (EFI_ERROR (Status)) {
>>> -  return CheckStatusRegister (PciIo, IdeRegisters);
>>> +  return Status;
>>>  }
>>>  
>>>  //
>>>
>>
>> Unfortunately, this patch breaks OVMF on QEMU's ide-cd device. The
>> symptoms were reported in ;
> 
> To confirm, does this affect both the AHCI ATAPI device and the BMDMA
> ATAPI device? (Q35 and PC default HBA, respectively) Or is it just AHCI?

I only used i440fx for testing. This is my command line:

qemu-system-x86_64 \
  -debugcon file:ovmf.log \
  -global isa-debugcon.iobase=0x402 \
  -net none \
  -enable-kvm \
  -pflash OVMF.fd \
  -drive id=cd0,if=none,readonly,format=raw,file=f22.iso \
  -device ide-cd,drive=cd0,bootindex=0

If I add "-M q35" (--> AHCI), then things seem to work fine; I can reach
grub on the F22 workstation installer CD.

>> I reproduced the issue and bisected it to this patch (git commit
>> 7cac2401 / SVN 19611). I also confirmed it by reverting the patch on top
>> of current master, and the CD-ROM started to work.
>>
>> I'm adding John Snow, who maintains QEMU's IDE emulation.
>>
>> Can you guys please investigate this patch, and discuss why it breaks on
>> QEMU's ide-cd device?
>>
>> John, if you'd like to browse the code, the following link highlights
>> the line that the above patch changes:
>>
>> https://github.com/tianocore/edk2/blob/7cac2401635317360226a235d1f95f8347081cbb/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c#L1952
>>
>> As far as I can see, this problem could be timing-sensitive. The
>> DRQReady2() function polls for the DRQ bit to become set, in a certain
>> amount of time. If that doesn't happen (or a definitive error emerges),
>> the function fails.
>>
>> Pre-patch, such failure would not be decisive; the status register would
>> be read and evaluated. Post-patch, the failure is decisive, even if it
>> is just a timeout, and the CheckStatusRegister() call would otherwise
>> return success.
>>
>> ... Hm, I added a debug log right after DRQReady2(), on the error branch
>> (i.e., when it fails). The status code is *not* EFI_TIMEOUT, it is
>> EFI_NOT_READY.
>>
>> Looking at DRQReady2(), this means:
>> - BSY is clear
>> - ERR is clear
>> - DRQ is clear too
>>
>> Apparently, the DRQReady2() function expects that as soon as BSY is
>> clear (and there is no error), DRQ must be immediately set (more or
>> less: not busy, no error, so ready to transfer data).
>>
> 
> Thank you for your research!
> 
>> Is that a valid assumption to make?
>>
> 
> Whenever BSY, DRQ and ERR are all clear that should be confirmation of a
> completed command.

That's interesting, because the edk2 code seems to treat this condition
as "failure to transition to data transfer", or some such.

>Under AHCI, this should *not* be true before the AHCI
> device has signalled completion (Either via the status shadow register,
> an interrupt, or an MSI interrupt, depending on device configuration.)
> 
> (In fact, hw/ide/core.c ide_exec_cmd uses the presence of DRQ|BSY to
> know whether or not it is in a state fit to accept a new command. If the
> guest is not blocked when BSY|DRQ are unset, this means a new command
> can be issued.)
> 
> == Thought #1: ==
> 
> However, when a command completes, it updates the shadow registers in
> the AHCI HBA right before signalling completion. It is, I think,
> possible that if you are polling the AHCI shadow registers instead of
> polling the AHCI status registers that you could find DRQ/BSY/ERR unset
> before receiving a command completed notice (by a time window of two
> passing electrons.)
> 
> Take a look at ahci_cmd_done, which calls ahci_write_fis_d2h. We
> populate a guest memory FIS buffer with the values of the registers,
> then update the status shadow register, then signal completion (via IRQ,
> MSI, or neither.)
> 
> You are probably not hitting that reliably, so can I ask what command is
> erroring out, here? 0xA0 PACKET for IDE, and 

[edk2] [PATCH 2/2] MdeModulePkg/.../IdeMode: report early finish of packet read as success

2016-01-18 Thread Laszlo Ersek
SVN r19611 (git commit 7cac240163), "MdeModulePkg/Ide: return correct
status when DRQ is not ready for ATAPI", changed the behavior of
AtaPacketReadWrite(), when DRQReady2() reported an error. The previous
logic had been to:

(a) terminate the transfer loop,
(b) check the status register with CheckStatusRegister(), and determine
AtaPacketReadWrite()'s return code directly from that.

Action (a) had been correct, but action (b) had masked genuine errors.

For example, when DRQReady2() reported EFI_TIMEOUT -- because the BSY bit
had not been cleared within the allotted time --, CheckStatusRegister()
would report EFI_SUCCESS, simply *because* BSY was still set, and the rest
of the status bits could not be evaluated.

SVN r19611 (git commit 7cac240163) intended to fix action (b) by directly
propagating the error code of DRQReady2() from AtaPacketReadWrite(),
eliminating the CheckStatusRegister() call. This was the right thing for
most of the errors reported by DRQReady2() -- timeout, command abort,
other device error --, but there was one exception: the "read" sub-case of
EFI_NOT_READY, which stands for "'read' complete, with less data available
than the requested amount".

Regarding the "write" sub-case of EFI_NOT_READY: the
AtaPacketCommandExecute() function programs the full transfer length into
the IDE device before it calls AtaPacketReadWrite(), and
AtaPacketReadWrite() only uses CylinderLsb and CylinderMsb for "chunking"
(as requested by the device). Therefore the device cannot justifiedly
clear DRQ earlier than seeing the entire data, when writing.

However, when reading from the device, a "short read" is a successful
operation. (The actual read length will be decoded by the higher level
protocols.) And "short reads" had been handled correctly by the logic
before git 7cac240163. Namely, when DRQReady2() returns EFI_NOT_READY, the
BSY bit is already clear, and we can call CheckStatusRegister() to
investigate all the other bits it cares about.

Therefore restore the logic from before git 7cac240163, but only for the
"read" sub-case of EFI_NOT_READY.

This problem was encountered with OVMF running on QEMU's i440fx IDE
emulation. Many thanks to John Snow for analyzing QEMU's behavior, and
pointing out that it adhered to the relevant specs.

Cc: Feng Tian 
Cc: Star Zeng 
Cc: John Snow 
Reference: https://github.com/tianocore/edk2/issues/43
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index b06f988..320ee90 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -1963,14 +1963,20 @@ AtaPacketReadWrite (
 
   while (ActualWordCount < RequiredWordCount) {
 //
 // before each data transfer stream, the host should poll DRQ bit ready,
 // to see whether indicates device is ready to transfer data.
 //
 Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
+if ((Status == EFI_NOT_READY) && Read) {
+  //
+  // Device provided less data than we intended to read -- exit early.
+  //
+  return CheckStatusRegister (PciIo, IdeRegisters);
+}
 if (EFI_ERROR (Status)) {
   return Status;
 }
 
 //
 // get current data transfer size from Cylinder Registers.
 //
-- 
1.8.3.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [PATCH 0/2] MdeModulePkg/.../IdeMode: restore handling of "short reads"

2016-01-18 Thread Laszlo Ersek
The series

  [edk2] [patch 0/2] Fix a DVD device compatbility issue
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/3681

fixed an IDE timeout masking (misreporting) bug in the AtaAtapiPassThru
driver. (And, separately, it increased SCSI disk timeouts to 30s.)
However, the IDE change that improved the reporting of timeouts
regressed the handling of short reads.

Users reported this in ,
running the code in question as part of OVMF, on QEMU's i440fx IDE
emulation. These patches aim to fix the short reads.

Cc: Feng Tian 
Cc: Star Zeng 
Cc: John Snow 

Thanks
Laszlo

Laszlo Ersek (2):
  MdeModulePkg/.../IdeMode: actualize DRQReady*() comment blocks
  MdeModulePkg/.../IdeMode: report early finish of packet read as
success

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 37 
 1 file changed, 31 insertions(+), 6 deletions(-)

-- 
1.8.3.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [PATCH 1/2] MdeModulePkg/.../IdeMode: actualize DRQReady*() comment blocks

2016-01-18 Thread Laszlo Ersek
The DRQReady() and DRQReady2() functions only differ in that they poll
different status registers for BSY, ERR, and DRQ: the former looks at the
Status Register (clearing interrupt status), while the latter looks at the
Alternate Status Register (not clearing interrupt status).

They both correctly return a unique status code, EFI_NOT_READY, for the

  BSY==0 && ERR==0 && DRQ==0

case; that is, when the device reports "command complete".

However, the functions' leading comments don't explain this case, so it's
easy to miss in callers. Update the comments.

Cc: Feng Tian 
Cc: Star Zeng 
Cc: John Snow 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 31 
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
index f74e5ca..b06f988 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
@@ -463,17 +463,26 @@ DRQClear2 (
   is called after the command is sent to the device and before required
   data is transferred.
 
   @param PciIoA pointer to EFI_PCI_IO_PROTOCOL data structure.
   @param IdeRegisters A pointer to EFI_IDE_REGISTERS data structure.
   @param Timeout  The time to complete the command, uses 100ns as a 
unit.
 
-  @retval EFI_SUCCESS  DRQ bit set within the time out.
-  @retval EFI_TIMEOUT  DRQ bit not set within the time out.
-  @retval EFI_ABORTED  DRQ bit not set caused by the command abort.
+  @retval EFI_SUCCESS   BSY bit cleared and DRQ bit set within the
+timeout.
+
+  @retval EFI_TIMEOUT   BSY bit not cleared within the timeout.
+
+  @retval EFI_ABORTED   Polling abandoned due to command abort.
+
+  @retval EFI_DEVICE_ERROR  Polling abandoned due to a non-abort error.
+
+  @retval EFI_NOT_READY BSY bit cleared within timeout, and device
+reported "command complete" by clearing DRQ
+bit.
 
   @note  Read Status Register will clear interrupt status.
 
 **/
 EFI_STATUS
 EFIAPI
 DRQReady (
@@ -538,17 +547,27 @@ DRQReady (
   DRQ is set when the device is ready to transfer data. So this function is 
called after
   the command is sent to the device and before required data is transferred.
 
   @param PciIoA pointer to EFI_PCI_IO_PROTOCOL data structure.
   @param IdeRegisters A pointer to EFI_IDE_REGISTERS data structure.
   @param Timeout  The time to complete the command, uses 100ns as a 
unit.
 
-  @retval EFI_SUCCESS   DRQ bit set within the time out.
-  @retval EFI_TIMEOUT   DRQ bit not set within the time out.
-  @retval EFI_ABORTED   DRQ bit not set caused by the command abort.
+  @retval EFI_SUCCESS   BSY bit cleared and DRQ bit set within the
+timeout.
+
+  @retval EFI_TIMEOUT   BSY bit not cleared within the timeout.
+
+  @retval EFI_ABORTED   Polling abandoned due to command abort.
+
+  @retval EFI_DEVICE_ERROR  Polling abandoned due to a non-abort error.
+
+  @retval EFI_NOT_READY BSY bit cleared within timeout, and device
+reported "command complete" by clearing DRQ
+bit.
+
   @note  Read Alternate Status Register will not clear interrupt status.
 
 **/
 EFI_STATUS
 EFIAPI
 DRQReady2 (
   IN  EFI_PCI_IO_PROTOCOL  *PciIo,
-- 
1.8.3.1


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [Patch] edk2: Update the maintainer list.

2016-01-18 Thread Jiaxin Wu
This patch is used to update the CryptoPkg and NetworkPkg
maintainer list.

Cc: Long Qin 
Cc: Fu Siyuan 
Cc: Tian Hot 
Cc: Li Ruth 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 Maintainers.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index dc0891e..59cd2bf 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -83,10 +83,11 @@ M: Prince Agyeman 
 S: Maintained
 
 CryptoPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
 M: Qin Long 
+M: Ting Ye 
 
 DuetPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/DuetPkg
 M: Ruiyu Ni 
 
@@ -144,10 +145,13 @@ M: Michael D Kinney 
 M: Liming Gao 
 
 NetworkPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg
 M: Siyuan Fu 
+M: Jiaxin Wu 
+M: Lubo Zhang 
+M: Fan Wang 
 
 Nt32Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/Nt32Pkg
 M: Ruiyu Ni 
 
-- 
1.9.5.msysgit.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [Patch] edk2: Update the maintainer list.

2016-01-18 Thread Fu, Siyuan
Jiaxin,

It's ok with me.

Reviewed-by: Siyuan Fu 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jiaxin Wu
Sent: Tuesday, January 19, 2016 9:40 AM
To: edk2-devel@lists.01.org
Cc: Fu, Siyuan ; Tian, Hot ; Li, Ruth 
; Long, Qin 
Subject: [edk2] [Patch] edk2: Update the maintainer list.

This patch is used to update the CryptoPkg and NetworkPkg maintainer list.

Cc: Long Qin 
Cc: Fu Siyuan 
Cc: Tian Hot 
Cc: Li Ruth 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 Maintainers.txt | 4 
 1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt index dc0891e..59cd2bf 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -83,10 +83,11 @@ M: Prince Agyeman 
 S: Maintained
 
 CryptoPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
 M: Qin Long 
+M: Ting Ye 
 
 DuetPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/DuetPkg
 M: Ruiyu Ni 
 
@@ -144,10 +145,13 @@ M: Michael D Kinney 
 M: Liming Gao 
 
 NetworkPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg
 M: Siyuan Fu 
+M: Jiaxin Wu 
+M: Lubo Zhang 
+M: Fan Wang 
 
 Nt32Pkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/Nt32Pkg
 M: Ruiyu Ni 
 
--
1.9.5.msysgit.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [patch 1/2] MdeModulePkg/Ide: return correct status when DRQ is not ready for ATAPI

2016-01-18 Thread Laszlo Ersek
On 01/19/16 00:08, John Snow wrote:
> 
> 
> On 01/18/2016 05:28 PM, Laszlo Ersek wrote:
>> On 01/18/16 22:05, John Snow wrote:
>>>
>>>
>>> On 01/18/2016 02:33 PM, Laszlo Ersek wrote:
 On 01/18/16 19:44, John Snow wrote:
>
>
> On 01/18/2016 05:57 AM, Laszlo Ersek wrote:
>> Hello Feng, John,
>>
>> On 11/03/15 02:48, Tian Feng wrote:
>>> When executing ATAPI cmd at IDE mode, EFI_SUCCESS may be returned 
>>> wrongly
>>> with old logic but in fact DRQ is not ready and the transaction doesn't
>>> get executed correctly at this time.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Feng Tian 
>>> Cc: Star Zeng 
>>> ---
>>>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
>>> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>>> index 4928ed5..f74e5ca 100644
>>> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>>> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
>>> @@ -1949,7 +1949,7 @@ AtaPacketReadWrite (
>>>  //
>>>  Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
>>>  if (EFI_ERROR (Status)) {
>>> -  return CheckStatusRegister (PciIo, IdeRegisters);
>>> +  return Status;
>>>  }
>>>  
>>>  //
>>>
>>
>> Unfortunately, this patch breaks OVMF on QEMU's ide-cd device. The
>> symptoms were reported in ;
>
> To confirm, does this affect both the AHCI ATAPI device and the BMDMA
> ATAPI device? (Q35 and PC default HBA, respectively) Or is it just AHCI?

 I only used i440fx for testing. This is my command line:

 qemu-system-x86_64 \
   -debugcon file:ovmf.log \
   -global isa-debugcon.iobase=0x402 \
   -net none \
   -enable-kvm \
   -pflash OVMF.fd \
   -drive id=cd0,if=none,readonly,format=raw,file=f22.iso \
   -device ide-cd,drive=cd0,bootindex=0

 If I add "-M q35" (--> AHCI), then things seem to work fine; I can reach
 grub on the F22 workstation installer CD.

>> I reproduced the issue and bisected it to this patch (git commit
>> 7cac2401 / SVN 19611). I also confirmed it by reverting the patch on top
>> of current master, and the CD-ROM started to work.
>>
>> I'm adding John Snow, who maintains QEMU's IDE emulation.
>>
>> Can you guys please investigate this patch, and discuss why it breaks on
>> QEMU's ide-cd device?
>>
>> John, if you'd like to browse the code, the following link highlights
>> the line that the above patch changes:
>>
>> https://github.com/tianocore/edk2/blob/7cac2401635317360226a235d1f95f8347081cbb/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c#L1952
>>
>> As far as I can see, this problem could be timing-sensitive. The
>> DRQReady2() function polls for the DRQ bit to become set, in a certain
>> amount of time. If that doesn't happen (or a definitive error emerges),
>> the function fails.
>>
>> Pre-patch, such failure would not be decisive; the status register would
>> be read and evaluated. Post-patch, the failure is decisive, even if it
>> is just a timeout, and the CheckStatusRegister() call would otherwise
>> return success.
>>
>> ... Hm, I added a debug log right after DRQReady2(), on the error branch
>> (i.e., when it fails). The status code is *not* EFI_TIMEOUT, it is
>> EFI_NOT_READY.
>>
>> Looking at DRQReady2(), this means:
>> - BSY is clear
>> - ERR is clear
>> - DRQ is clear too
>>
>> Apparently, the DRQReady2() function expects that as soon as BSY is
>> clear (and there is no error), DRQ must be immediately set (more or
>> less: not busy, no error, so ready to transfer data).
>>
>
> Thank you for your research!
>
>> Is that a valid assumption to make?
>>
>
> Whenever BSY, DRQ and ERR are all clear that should be confirmation of a
> completed command.

 That's interesting, because the edk2 code seems to treat this condition
 as "failure to transition to data transfer", or some such.

>>>
>>> If it's in the middle of a transfer loop, it's a weird condition to
>>> encounter. We expect to see BSY and/or DRQ for the duration of the
>>> command. At the end of the command, though, READY|SEEK is the normal
>>> idle status.
>>>
>Under AHCI, this should *not* be true before the AHCI
> device has signalled completion (Either via the status shadow register,
> an interrupt, or an MSI interrupt, depending on device configuration.)
>
> (In fact, hw/ide/core.c ide_exec_cmd uses the presence of DRQ|BSY to
> know whether or not it is in a state fit to accept 

Re: [edk2] [PATCH V4 10/13] ArmPlatformPkg: Use SerialDxe in MdeModulePkg instead of EmbeddedPkg

2016-01-18 Thread Zeng, Star

On 2016/1/18 19:33, Laszlo Ersek wrote:

On 01/18/16 11:24, Zeng, Star wrote:

[...]



The above analysis is very clear, thanks. I am a little concern about if
the code changes below follow the comments in the code.

In Terminal.c:
  //
  // Set the timeout value of serial buffer for
  // keystroke response performance issue
  //
In TerminalConIn.c:
//
//  if current timeout value for serial device is not identical with
//  the value saved in TERMINAL_DEV structure, then recalculate the
//  timeout value again and set serial attribute according to this
value.
//


Any comments about above concern?


Not really.

I don't know what the purpose of the Timeout calculation is (what is the
"keystroke response performance issue"?), but in any case, it is a good
sign that TerminalDxe sets *some* Timeout explicitly.

Plus, it has worked reliably until now, so we shouldn't change the
Timeout argument.





And I also checked the
IntelFrameworkModulePkg/Bus/Isa/IsaSerialDxe/Serial.c although ARM
platforms do not use it, the SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH
value is 1. if SetAttributes() is called with ReceiveFifoDepth = 0, then
SERIAL_PORT_DEFAULT_RECEIVE_FIFO_DEPTH is selected as the description of
the functions is "A ReceiveFifoDepth value of 0 will use the device's
default FIFO depth", so what's the device's default FIFO depth?


Well, that's exactly what SerialDxe delegates to SerialPortLib :)
SerialDxe need not be aware of the device-specific value; the platform
will provide a device-specific library instance.


I have a thought that updates SerialDxe to call SerialSetAttributes(with
0, 0, 0, DefaultParity, 0 and DefaultStopBits) in SerialDxeInitialize()
and SerialReset() to get the real default values of the device, then the
driver can also eliminate the use of PcdUartDefault.

What's your opinion?


I don't think this is a good idea -- the UEFI spec is very clear on
this. Under "11.8 Serial I/O Protocol", in the general description part,
you find:

  The default attributes for all UART-style serial device interfaces
  are: 115,200 baud, a 1 byte receive FIFO, a 1,000,000 microsecond
  timeout per character, no parity, 8 data bits, and 1 stop bit. [...]

So this is what must be in effect right after initialization, and --
with all likelihood -- after re-set as well.

If a Serial IO protocol client is not content with these settings, then
it must explicitly change them, with the SetAttributes() call.


So TerminalDxe could know it is not content with the settings?


I think so, yes. TerminalDxe knows in advance that it will handle escape
sequences (= bursts of scan codes). The longest such sequence (3 or 4
scan codes I guess?) should fit in the receive FIFO.

So TerminalDxe could try to figure out the longest sequence it wishes to
handle, then configure such a receive FIFO depth with SerialIO.

Or else, what I think would be better, just call SetAttributes() from
the platform's SerialPortLib instance, with ReceiveFifoDepth=0:
- in practice, this would restore the previous behavior,
- I expect that this value should enable the FIFO on most UARTs, with a
   sufficient depth for the escape sequence processing


Ok, would you please help submit the formal patches(include the 115200 
Timeout in SerialDxe)? :)


Thanks,
Star




Should platform BDS code to explicitly change them with the
SetAttributes() call?


Well, PlatformBDS is the ultimate fallback, always :), but I think
TerminalDxe has much more business dealing with the Serial IO
attributes. TerminalDxe already modifies the Timeout attribute, and in
the end, the FIFO depth too is important to TerminalDxe (for escape seq
processing). I think it's not a BDS responsibility.

Consider for example
"MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxe.inf". This is a
UEFI driver (unlike SerialDxe, which is a DXE_DRIVER).

If PlatformBDS wanted to control the receive FIFO depth of the SerialIo
protocol instance procuded by PciSioSerialDxe, then it would have to
connect PciIo instances (so SerialIo would exist), then set the
attribute, then connect SerialIo recursively (so that TerminalDxe would
sit on top). In other words, PlatformBds would have to intrude to the
UEFI driver model -- a full bottom-up recursive ConnectAll would no
longer work.

Also, the user could disconnect/reconnect the SerialIo protocol provider
in the UEFI shell; maybe even unload/reload the driver. So I think
setting the attributes is the responsibility of the driver that sets
directly atop.

Thanks
Laszlo



___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [PATCH] Maintainers.txt: Update maintainers for SourceLevelDebugPkg

2016-01-18 Thread Hao Wu
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu 
---
 Maintainers.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index dc0891e..1095fe0 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -191,6 +191,7 @@ M: Shumin Qiu 
 SourceLevelDebugPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/SourceLevelDebugPkg
 M: Jeff Fan 
+M: Hao Wu 
 
 StdLib, StdLibPrivateInternalFiles
 W: https://github.com/tianocore/tianocore.github.io/wiki/StdLib
-- 
1.9.5.msysgit.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


[edk2] [Patch] Vlv2DeviceRefcodePkg:Add setup option to control _STA of LPE Audio

2016-01-18 Thread lushifex
Signed-off-by: lushifex 
---
 Vlv2DeviceRefCodePkg/AcpiTablesPCAT/GloblNvs.asl   |   3 ++-
 Vlv2DeviceRefCodePkg/AcpiTablesPCAT/Pch.asl|  12 +---
 Vlv2DeviceRefCodePkg/AcpiTablesPCAT/PchLpss.asl|   7 +--
 Vlv2TbltDevicePkg/AcpiPlatform/AcpiPlatform.c  |   3 ++-
 Vlv2TbltDevicePkg/Include/Guid/SetupVariable.h |   3 ++-
 Vlv2TbltDevicePkg/Include/Protocol/GlobalNvsArea.h |   3 ++-
 .../PlatformSetupDxe/SouthClusterConfig.vfi|  11 ++-
 Vlv2TbltDevicePkg/PlatformSetupDxe/UqiList.uni | Bin 66540 -> 66864 bytes
 Vlv2TbltDevicePkg/PlatformSetupDxe/VfrStrings.uni  | Bin 215420 -> 216022 bytes
 9 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/GloblNvs.asl 
b/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/GloblNvs.asl
index 21e526c..32cfd9d 100644
--- a/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/GloblNvs.asl
+++ b/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/GloblNvs.asl
@@ -3,11 +3,11 @@
 ;**;
 ;*Intel Corporation - ACPI Reference Code for the Baytrail*;
 ;*Family of Customer Reference Boards.*;
 ;**;
 ;**;
-;*Copyright (c)  1999  - 2015, Intel Corporation. All rights reserved   *;
+;*Copyright (c)  1999  - 2016, Intel Corporation. All rights reserved   *;
 ;
 ; This program and the accompanying materials are licensed and made available 
under
 ; the terms and conditions of the BSD License that accompanies this 
distribution.
 ; The full text of the license may be found at
 ; http://opensource.org/licenses/bsd-license.php.
@@ -347,7 +347,8 @@ Field(GNVS,AnyAcc,Lock,Preserve)
   Offset(792),
   EDPV, 8,  //(792) Check for eDP display device
   DIDX, 32, //(793) Device ID for eDP device
   IOT,  8,  //(794) MinnowBoard Max JP1 is configured for MSFT IOT 
project.
   BATT, 8,  //(795) The Flag of RTC Battery Prensent.  
+  LPAD, 8,  //(796)   
 }
 
diff --git a/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/Pch.asl 
b/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/Pch.asl
index 38dac87..0d12719 100644
--- a/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/Pch.asl
+++ b/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/Pch.asl
@@ -3,11 +3,11 @@
 ;**;
 ;*Intel Corporation - ACPI Reference Code for the Baytrail*;
 ;*Family of Customer Reference Boards.*;
 ;**;
 ;**;
-;*Copyright (c) 2012  - 2014, Intel Corporation. All rights reserved*;
+;*Copyright (c) 2012  - 2016, Intel Corporation. All rights reserved*;
 ;
 ; This program and the accompanying materials are licensed and made available 
under
 ; the terms and conditions of the BSD License that accompanies this 
distribution.
 ; The full text of the license may be found at
 ; http://opensource.org/licenses/bsd-license.php.
@@ -137,11 +137,14 @@ scope (\_SB)
 
 Method (_STA, 0x0, NotSerialized)
 {
   If (LAnd(LAnd(LEqual(LPEE, 2), LEqual(LPED, 0)), LEqual(OSEL, 0)))
   {
-Return (0xF)
+If(LEqual(LPAD, 1))
+{
+  Return (0xF)
+}
   }
   Return (0x0)
 }
 
 Method (_DIS, 0x0, NotSerialized)
@@ -216,11 +219,14 @@ scope (\_SB)
 
 Method (_STA, 0x0, NotSerialized)
 {
   If (LAnd(LAnd(LEqual(LPEE, 2), LEqual(LPED, 0)), LEqual(OSEL, 1)))
   {
-Return (0xF)
+If(LEqual(LPAD, 1))
+{
+  Return (0xF)
+}
   }
   Return (0x0)
 }
 
 Method (_DIS, 0x0, NotSerialized)
diff --git a/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/PchLpss.asl 
b/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/PchLpss.asl
index 3e61e79..ecb20c1 100644
--- a/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/PchLpss.asl
+++ b/Vlv2DeviceRefCodePkg/AcpiTablesPCAT/PchLpss.asl
@@ -3,11 +3,11 @@
 ;**;
 ;*Intel Corporation - ACPI Reference Code for the Baytrail*;
 ;*Family of Customer Reference Boards.*;
 ;**;
 ;**;
-;*Copyright (c) 2012  - 2014, Intel Corporation. All rights reserved*;
+;*Copyright (c) 2012  - 2016, Intel Corporation. All rights reserved*;
 ;
 ; This program and the accompanying materials are licensed and made available 
under
 ; the terms and conditions of the BSD License that accompanies this 
distribution.
 ; The full text of the license may be found at
 ; 

Re: [edk2] [PATCH] Maintainers.txt: Update maintainers for SourceLevelDebugPkg

2016-01-18 Thread Gao, Liming
Reviewed-by: Liming Gao 

-Original Message-
From: Wu, Hao A 
Sent: Tuesday, January 19, 2016 11:44 AM
To: edk2-devel@lists.01.org; Fan, Jeff; Gao, Liming; Tian, Hot
Cc: Wu, Hao A
Subject: [PATCH] Maintainers.txt: Update maintainers for SourceLevelDebugPkg

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu 
---
 Maintainers.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Maintainers.txt b/Maintainers.txt index dc0891e..1095fe0 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -191,6 +191,7 @@ M: Shumin Qiu   SourceLevelDebugPkg
 W: https://github.com/tianocore/tianocore.github.io/wiki/SourceLevelDebugPkg
 M: Jeff Fan 
+M: Hao Wu 
 
 StdLib, StdLibPrivateInternalFiles
 W: https://github.com/tianocore/tianocore.github.io/wiki/StdLib
--
1.9.5.msysgit.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [Patch] BaseTools: process the files by the priority in BUILDRULEORDER

2016-01-18 Thread Gao, Liming
Reviewed-by: Liming Gao 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong 
Zhu
Sent: Monday, January 18, 2016 3:36 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch] BaseTools: process the files by the priority in 
BUILDRULEORDER

By the BUILDRULEORDER feature to process files listed in INF [Sources] sections 
in priority order, if a filename is listed with multiple extensions, the tools 
will use only the file that matches the first extension in the space separated 
list.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
---
 BaseTools/Source/Python/AutoGen/AutoGen.py | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
b/BaseTools/Source/Python/AutoGen/AutoGen.py
index abac477..e9f4888 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -1,9 +1,9 @@
 ## @file
 # Generate AutoGen.h, AutoGen.c and *.depex files  # -# Copyright (c) 2007 - 
2015, Intel Corporation. All rights reserved.
+# Copyright (c) 2007 - 2016, Intel Corporation. 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
 #
@@ -2739,13 +2739,40 @@ class ModuleAutoGen(AutoGen):
 
 # add the file path into search path list for file including
 if F.Dir not in self.IncludePathList and self.AutoGenVersion 
>= 0x00010005:
 self.IncludePathList.insert(0, F.Dir)
 self._SourceFileList.append(F)
+
+self._MatchBuildRuleOrder(self._SourceFileList)
+
+for F in self._SourceFileList:
 self._ApplyBuildRule(F, TAB_UNKNOWN_FILE)
 return self._SourceFileList
 
+def _MatchBuildRuleOrder(self, FileList):
+Order_Dict = {}
+self._GetModuleBuildOption()
+for SingleFile in FileList:
+if self.BuildRuleOrder and SingleFile.Ext in self.BuildRuleOrder 
and SingleFile.Ext in self.BuildRules:
+key = SingleFile.Path.split(SingleFile.Ext)[0]
+if key in Order_Dict:
+Order_Dict[key].append(SingleFile.Ext)
+else:
+Order_Dict[key] = [SingleFile.Ext]
+
+RemoveList = []
+for F in Order_Dict:
+if len(Order_Dict[F]) > 1:
+Order_Dict[F].sort(key=lambda i: self.BuildRuleOrder.index(i))
+for Ext in Order_Dict[F][1:]:
+RemoveList.append(F + Ext)
+   
+for item in RemoveList:
+FileList.remove(item)
+
+return FileList
+
 ## Return the list of unicode files
 def _GetUnicodeFileList(self):
 if self._UnicodeFileList == None:
 if TAB_UNICODE_FILE in self.FileTypes:
 self._UnicodeFileList = self.FileTypes[TAB_UNICODE_FILE]
--
2.6.1.windows.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [PATCH 0/2] MdeModulePkg/.../IdeMode: restore handling of "short reads"

2016-01-18 Thread Tian, Feng
Laszlo & John

Thanks for your detail analysis. It's very helpful for me.

Reviewed-by: Feng Tian 

I am thinking shall we update EFI_SCSI_INQUIRY_DATA/ATAPI_INQUIRY_DATA data 
structure as well to only contain the first 36 bytes?  According to ATAPI SFF 
8020i & SPC4 spec, they all state the inquiry data includes at least 36 bytes. 
But we defines it as 96 bytes or more.

Thanks
Feng

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Tuesday, January 19, 2016 08:35
To: edk2-de...@ml01.01.org
Cc: Tian, Feng; Zeng, Star; John Snow
Subject: [PATCH 0/2] MdeModulePkg/.../IdeMode: restore handling of "short reads"

The series

  [edk2] [patch 0/2] Fix a DVD device compatbility issue
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/3681

fixed an IDE timeout masking (misreporting) bug in the AtaAtapiPassThru driver. 
(And, separately, it increased SCSI disk timeouts to 30s.) However, the IDE 
change that improved the reporting of timeouts regressed the handling of short 
reads.

Users reported this in ,
running the code in question as part of OVMF, on QEMU's i440fx IDE emulation. 
These patches aim to fix the short reads.

Cc: Feng Tian 
Cc: Star Zeng 
Cc: John Snow 

Thanks
Laszlo

Laszlo Ersek (2):
  MdeModulePkg/.../IdeMode: actualize DRQReady*() comment blocks
  MdeModulePkg/.../IdeMode: report early finish of packet read as
success

 MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 37 
 1 file changed, 31 insertions(+), 6 deletions(-)

--
1.8.3.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] Transition to GitHub Update

2016-01-18 Thread Bjorge, Erik C
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Saturday, January 16, 2016 6:26 AM
> To: Jarlstrom, Laurie 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] Transition to GitHub Update
> 
> On 15 January 2016 at 22:14, Jarlstrom, Laurie
>  wrote:
> > To: EDK II Community
> >
> > This message is an update on the transition from SourceForge to GitHub
> for EDK II development.  The schedule is currently targeting the last
> week of January or the first week of February to perform the transition.
> The transition process should only take one to two days to complete.  A
> notification message will be sent the week prior to the actual dates
> that the repositories will be impacted.  This should provide adequate
> notification for the scheduled down time.
> > As part of this transition some documentation and process changes have
> been required.  The updated process as well as other GitHub specific
> information can be found on the tianocore Wiki at the link provided
> below.  Feedback on this documentation is welcome and needed to make
> sure the transition is as smooth as possible.
> >
> > https://github.com/tianocore/tianocore.github.io/wiki/SourceForge-to-
> Github-Quick-Start
> >
> > Please post any comments or questions related to this transition to
> the edk2-devel mailing list or reply to the email message.
> >
> 
> Hello,
> 
> Thanks for getting all of this set up.
> 
> Regarding the following line
> 
> 'Commits should build / boot if possible to support use of bisect'
> 
> could we make that a bit stronger please? Breaking bisect is really
> not ok, and it usually does not take that much extra work to avoid it,
> as long as you are aware of it. Given that many people are new to Git,
> presenting it as an opt-in feature like this does not send the right
> message imo.

I will update the wording.  You are correct that bisect just needs to work.

> 
> In fact, having some guidelines about how to avoid breaking bisect
> could be helpful, especially that something like adding some temporary
> code in an early patch only to remove it again at the end of the
> series is perfectly ok, and highly preferred over losing
> bisectability. Not everybody might expect that.
> 

I will add some guidelines/hints on how not to break bisect.  Most of the time 
it is as simple as reordering a patch series.  Adding temporary code is not 
always an obvious solution so adding that to the list of hints is good.  Do you 
have any other hints or guidelines you would like added?

Thanks,
-Erik

> Thanks,
> Ard.
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Re: [edk2] [patch 1/2] MdeModulePkg/Ide: return correct status when DRQ is not ready for ATAPI

2016-01-18 Thread Laszlo Ersek
On 01/18/16 22:05, John Snow wrote:
> 
> 
> On 01/18/2016 02:33 PM, Laszlo Ersek wrote:
>> On 01/18/16 19:44, John Snow wrote:
>>>
>>>
>>> On 01/18/2016 05:57 AM, Laszlo Ersek wrote:
 Hello Feng, John,

 On 11/03/15 02:48, Tian Feng wrote:
> When executing ATAPI cmd at IDE mode, EFI_SUCCESS may be returned wrongly
> with old logic but in fact DRQ is not ready and the transaction doesn't
> get executed correctly at this time.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Feng Tian 
> Cc: Star Zeng 
> ---
>  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
> b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> index 4928ed5..f74e5ca 100644
> --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
> @@ -1949,7 +1949,7 @@ AtaPacketReadWrite (
>  //
>  Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
>  if (EFI_ERROR (Status)) {
> -  return CheckStatusRegister (PciIo, IdeRegisters);
> +  return Status;
>  }
>  
>  //
>

 Unfortunately, this patch breaks OVMF on QEMU's ide-cd device. The
 symptoms were reported in ;
>>>
>>> To confirm, does this affect both the AHCI ATAPI device and the BMDMA
>>> ATAPI device? (Q35 and PC default HBA, respectively) Or is it just AHCI?
>>
>> I only used i440fx for testing. This is my command line:
>>
>> qemu-system-x86_64 \
>>   -debugcon file:ovmf.log \
>>   -global isa-debugcon.iobase=0x402 \
>>   -net none \
>>   -enable-kvm \
>>   -pflash OVMF.fd \
>>   -drive id=cd0,if=none,readonly,format=raw,file=f22.iso \
>>   -device ide-cd,drive=cd0,bootindex=0
>>
>> If I add "-M q35" (--> AHCI), then things seem to work fine; I can reach
>> grub on the F22 workstation installer CD.
>>
 I reproduced the issue and bisected it to this patch (git commit
 7cac2401 / SVN 19611). I also confirmed it by reverting the patch on top
 of current master, and the CD-ROM started to work.

 I'm adding John Snow, who maintains QEMU's IDE emulation.

 Can you guys please investigate this patch, and discuss why it breaks on
 QEMU's ide-cd device?

 John, if you'd like to browse the code, the following link highlights
 the line that the above patch changes:

 https://github.com/tianocore/edk2/blob/7cac2401635317360226a235d1f95f8347081cbb/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c#L1952

 As far as I can see, this problem could be timing-sensitive. The
 DRQReady2() function polls for the DRQ bit to become set, in a certain
 amount of time. If that doesn't happen (or a definitive error emerges),
 the function fails.

 Pre-patch, such failure would not be decisive; the status register would
 be read and evaluated. Post-patch, the failure is decisive, even if it
 is just a timeout, and the CheckStatusRegister() call would otherwise
 return success.

 ... Hm, I added a debug log right after DRQReady2(), on the error branch
 (i.e., when it fails). The status code is *not* EFI_TIMEOUT, it is
 EFI_NOT_READY.

 Looking at DRQReady2(), this means:
 - BSY is clear
 - ERR is clear
 - DRQ is clear too

 Apparently, the DRQReady2() function expects that as soon as BSY is
 clear (and there is no error), DRQ must be immediately set (more or
 less: not busy, no error, so ready to transfer data).

>>>
>>> Thank you for your research!
>>>
 Is that a valid assumption to make?

>>>
>>> Whenever BSY, DRQ and ERR are all clear that should be confirmation of a
>>> completed command.
>>
>> That's interesting, because the edk2 code seems to treat this condition
>> as "failure to transition to data transfer", or some such.
>>
> 
> If it's in the middle of a transfer loop, it's a weird condition to
> encounter. We expect to see BSY and/or DRQ for the duration of the
> command. At the end of the command, though, READY|SEEK is the normal
> idle status.
> 
>>>Under AHCI, this should *not* be true before the AHCI
>>> device has signalled completion (Either via the status shadow register,
>>> an interrupt, or an MSI interrupt, depending on device configuration.)
>>>
>>> (In fact, hw/ide/core.c ide_exec_cmd uses the presence of DRQ|BSY to
>>> know whether or not it is in a state fit to accept a new command. If the
>>> guest is not blocked when BSY|DRQ are unset, this means a new command
>>> can be issued.)
>>>
>>> == Thought #1: ==
>>>
>>> However, when a command completes, it updates the shadow registers in
>>> the AHCI HBA right before signalling completion. It is, I think,
>>> possible that if you are polling the 

Re: [edk2] [patch 1/2] MdeModulePkg/Ide: return correct status when DRQ is not ready for ATAPI

2016-01-18 Thread Laszlo Ersek
On 01/18/16 20:33, Laszlo Ersek wrote:
> On 01/18/16 19:44, John Snow wrote:
>>
>>
>> On 01/18/2016 05:57 AM, Laszlo Ersek wrote:
>>> Hello Feng, John,
>>>
>>> On 11/03/15 02:48, Tian Feng wrote:
 When executing ATAPI cmd at IDE mode, EFI_SUCCESS may be returned wrongly
 with old logic but in fact DRQ is not ready and the transaction doesn't
 get executed correctly at this time.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Feng Tian 
 Cc: Star Zeng 
 ---
  MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c 
 b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
 index 4928ed5..f74e5ca 100644
 --- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
 +++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c
 @@ -1949,7 +1949,7 @@ AtaPacketReadWrite (
  //
  Status = DRQReady2 (PciIo, IdeRegisters, Timeout);
  if (EFI_ERROR (Status)) {
 -  return CheckStatusRegister (PciIo, IdeRegisters);
 +  return Status;
  }
  
  //

>>>
>>> Unfortunately, this patch breaks OVMF on QEMU's ide-cd device. The
>>> symptoms were reported in ;
>>
>> To confirm, does this affect both the AHCI ATAPI device and the BMDMA
>> ATAPI device? (Q35 and PC default HBA, respectively) Or is it just AHCI?
> 
> I only used i440fx for testing. This is my command line:
> 
> qemu-system-x86_64 \
>   -debugcon file:ovmf.log \
>   -global isa-debugcon.iobase=0x402 \
>   -net none \
>   -enable-kvm \
>   -pflash OVMF.fd \
>   -drive id=cd0,if=none,readonly,format=raw,file=f22.iso \
>   -device ide-cd,drive=cd0,bootindex=0
> 
> If I add "-M q35" (--> AHCI), then things seem to work fine; I can reach
> grub on the F22 workstation installer CD.
> 
>>> I reproduced the issue and bisected it to this patch (git commit
>>> 7cac2401 / SVN 19611). I also confirmed it by reverting the patch on top
>>> of current master, and the CD-ROM started to work.
>>>
>>> I'm adding John Snow, who maintains QEMU's IDE emulation.
>>>
>>> Can you guys please investigate this patch, and discuss why it breaks on
>>> QEMU's ide-cd device?
>>>
>>> John, if you'd like to browse the code, the following link highlights
>>> the line that the above patch changes:
>>>
>>> https://github.com/tianocore/edk2/blob/7cac2401635317360226a235d1f95f8347081cbb/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/IdeMode.c#L1952
>>>
>>> As far as I can see, this problem could be timing-sensitive. The
>>> DRQReady2() function polls for the DRQ bit to become set, in a certain
>>> amount of time. If that doesn't happen (or a definitive error emerges),
>>> the function fails.
>>>
>>> Pre-patch, such failure would not be decisive; the status register would
>>> be read and evaluated. Post-patch, the failure is decisive, even if it
>>> is just a timeout, and the CheckStatusRegister() call would otherwise
>>> return success.
>>>
>>> ... Hm, I added a debug log right after DRQReady2(), on the error branch
>>> (i.e., when it fails). The status code is *not* EFI_TIMEOUT, it is
>>> EFI_NOT_READY.
>>>
>>> Looking at DRQReady2(), this means:
>>> - BSY is clear
>>> - ERR is clear
>>> - DRQ is clear too
>>>
>>> Apparently, the DRQReady2() function expects that as soon as BSY is
>>> clear (and there is no error), DRQ must be immediately set (more or
>>> less: not busy, no error, so ready to transfer data).
>>>
>>
>> Thank you for your research!
>>
>>> Is that a valid assumption to make?
>>>
>>
>> Whenever BSY, DRQ and ERR are all clear that should be confirmation of a
>> completed command.
> 
> That's interesting, because the edk2 code seems to treat this condition
> as "failure to transition to data transfer", or some such.
> 
>>Under AHCI, this should *not* be true before the AHCI
>> device has signalled completion (Either via the status shadow register,
>> an interrupt, or an MSI interrupt, depending on device configuration.)
>>
>> (In fact, hw/ide/core.c ide_exec_cmd uses the presence of DRQ|BSY to
>> know whether or not it is in a state fit to accept a new command. If the
>> guest is not blocked when BSY|DRQ are unset, this means a new command
>> can be issued.)
>>
>> == Thought #1: ==
>>
>> However, when a command completes, it updates the shadow registers in
>> the AHCI HBA right before signalling completion. It is, I think,
>> possible that if you are polling the AHCI shadow registers instead of
>> polling the AHCI status registers that you could find DRQ/BSY/ERR unset
>> before receiving a command completed notice (by a time window of two
>> passing electrons.)
>>
>> Take a look at ahci_cmd_done, which calls ahci_write_fis_d2h. We
>> populate a guest memory FIS buffer with the values of the registers,
>> then update the status shadow register, then