Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Cohen, Eugene 
Ard,

> So before these changes, we were in the exact same situation, but since PC
> platforms never enable DMA above 4 GB in the first place, nobody ever
> noticed until we started running this code on arm64 platforms that have no
> 32-bit addressable DRAM to begin with.

Interesting - I did not realize that there were designs that were crazy enough 
to have no addressable DRAM below 4G.
 
> The obvious conclusion is that the driver should not set the
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
> not support it, or, which seems to be our case, if the driver does not
> implement the 64-bit DMA mode that the driver does support. However,
> since there are platforms for which bounce buffering is not an option (since
> there is no 32-bit addressable memory to bounce to), this is not just a
> performance optimization, and so it would be useful to fix the code so it can
> drive all 64-bit DMA capable hardware.

Okay, that's a great reason - let's get V3 64b ADMA2 in!

Any objection to committing the original patch in the short term?

Thanks,

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

Re: [edk2] [PATCH v2] MdePkg/Library: Install dummy variable arch protocol

2019-03-01 Thread Ard Biesheuvel 
On Fri, 1 Mar 2019 at 12:14, Jagadeesh Ujja  wrote:
>
> In a system implementing the variable store in MM, there are no variable
> arch protocol and variable write arch protocol installed into the
> DXE_SMM protocol database. On such systems, it is not required to
> locate these protocols by the DXE runtime variable drivers because
> it can be assumed that these protocols are already installed in the MM
> context. But then such an implementation will deviate from the existing
> traditional MM based variable driver implementation.
>
> So in order to maintain consistency with the traditional MM variable
> driver implementation, allow platforms to install dummy versions of
> these protocols into the DXE protocol database but these protocol will
> not be consumed by non-secure variable service runtime driver.
>
> The Platform which uses StandaloneMM based secure variable storage
> have to include this library as below.
>
>   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf {
> 
>   NULL|MdePkg/Library/VariableMmDependency/VariableMmDependency.inf
>   }
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jagadeesh Ujja 
> ---
> Changes since v1:
> - This is a next version of patch
>“MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch 
> Protocol”.
>[https://lists.01.org/pipermail/edk2-devel/2019-February/036885.html]
> - Addressed the comments from Ard Biesheuvel and Zeng Star
> - Can this library be placed in MdePkg rather then the StandaloneMmPkg?
>

This does not belong in MdePkg. What is wrong with keeping it in
StandaloneMmPkg?

>  MdePkg/Library/VariableMmDependency/VariableMmDependency.c   | 85 
> 
>  MdePkg/Library/VariableMmDependency/VariableMmDependency.inf | 48 +++
>  2 files changed, 133 insertions(+)
>
> diff --git a/MdePkg/Library/VariableMmDependency/VariableMmDependency.c 
> b/MdePkg/Library/VariableMmDependency/VariableMmDependency.c
> new file mode 100644
> index 000..6e5117e
> --- /dev/null
> +++ b/MdePkg/Library/VariableMmDependency/VariableMmDependency.c
> @@ -0,0 +1,85 @@
> +/** @file
> +  Runtime DXE part corresponding to StanaloneMM variable module.
> +
> +This module installs dummy variable arch protocol and dummy variable write 
> arch protocol
> +to StandaloneMM runtime variable service.
> +

I think 'dummy' is a misnomer here. They are NULL protocols in the
sense that only their presence is signifcant, and the protocol does
not have an implementation. But this is true for traditional MM as
well.

> +Copyright (c) 2019, ARM Ltd. All rights reserved.
> +
> +This program and the accompanying materials
> +are licensed and made available under the terms and conditions of the BSD 
> License
> +which accompanies this distribution.  The full text of the license may be 
> found at
> +http://opensource.org/licenses/bsd-license.php.
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +
> +/**
> +  Notify the system that the SMM variable driver is ready.
> +**/
> +VOID
> +VariableNotifySmmReady (
> +  VOID
> +  )
> +{
> +  EFI_STATUSStatus;
> +  EFI_HANDLEHandle;
> +
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (
> +  ,
> +  ,
> +  EFI_NATIVE_INTERFACE,
> +  NULL
> +  );
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  Notify the system that the SMM variable write driver is ready.
> +**/
> +VOID
> +VariableNotifySmmWriteReady (
> +  VOID
> +  )
> +{
> +  EFI_STATUSStatus;
> +  EFI_HANDLEHandle;
> +
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (
> +  ,
> +  ,
> +  EFI_NATIVE_INTERFACE,
> +  NULL
> +  );
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  The constructor function calls and installs dummy variable arch protocol 
> and
> +  dummy variable write arch protocol to StandaloneMM runtime variable service
> +
> +  @param  ImageHandle   The firmware allocated handle for the EFI image.
> +  @param  SystemTable   A pointer to the Management mode System Table.
> +
> +  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +VariableMmDependencyLibConstructor (
> +  IN EFI_HANDLE   ImageHandle,
> +  IN EFI_SYSTEM_TABLE *SystemTable
> +  )
> +{
> +  VariableNotifySmmReady();
> +  VariableNotifySmmWriteReady();
> +  return EFI_SUCCESS;
> +}
> +

Can we replace all of this with a single call to
InstallMultipleProtocolInterfaces() please?


> diff --git a/MdePkg/Library/VariableMmDependency/VariableMmDependency.inf 
> b/MdePkg/Library/VariableMmDependency/VariableMmDependency.inf
> new file mode 100644
> index

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Ard Biesheuvel 
On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene  wrote:
>
> Ard,
>
> > So before these changes, we were in the exact same situation, but since PC
> > platforms never enable DMA above 4 GB in the first place, nobody ever
> > noticed until we started running this code on arm64 platforms that have no
> > 32-bit addressable DRAM to begin with.
>
> Interesting - I did not realize that there were designs that were crazy 
> enough to have no addressable DRAM below 4G.
>

You must be new here :-)

But seriously, it does make sense for an implementation to, say, put
all peripherals, PCIe resource windows etc in the bottom half and all
DRAM in the top half of a 40-bit address space, which is how the AMD
Seattle SoC ended with its system memory at address 0x80__.
Note that on this platform, we can still use 32-bit DMA if we want to
with the help of the SMMUs, but we haven't wired those up in UEFI (and
the generic host bridge driver did not have the IOMMU hooks at the
time)

> > The obvious conclusion is that the driver should not set the
> > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
> > not support it, or, which seems to be our case, if the driver does not
> > implement the 64-bit DMA mode that the driver does support. However,
> > since there are platforms for which bounce buffering is not an option (since
> > there is no 32-bit addressable memory to bounce to), this is not just a
> > performance optimization, and so it would be useful to fix the code so it 
> > can
> > drive all 64-bit DMA capable hardware.
>
> Okay, that's a great reason - let's get V3 64b ADMA2 in!
>
> Any objection to committing the original patch in the short term?
>

not at all

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

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Ard Biesheuvel 
On Fri, 1 Mar 2019 at 11:32, Ard Biesheuvel  wrote:
>
> On Fri, 1 Mar 2019 at 01:19, Ashish Singhal  wrote:
> >
> > Eugene,
> >
> > Small question. Did the issue appear after the V4 patch went in? Looking at 
> > the code before that patch, we were enabling 64b dma in pci based on 
> > capability register already despite of driver supporting only 32b dma.
> >
>
> I think this may have been an oversight on my part when I originally
> added the DUAL_ADDRESS_CYCLE handling.
>
> The following commit added EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE to the
> host bridge driver
>
> commit e58a71d9c50ba641b5ab19f5ce2cbf772187de4d
> Author: Ard Biesheuvel 
> Date:   Mon Sep 5 09:55:16 2016 +0100
>
> MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA to devices that
> support it
>
> Currently, the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is 
> completely
> ignored by the PCI host bridge driver, which means that, on an
> implementation
> that supports DMA above 4 GB, allocations above 4 GB may be provided to
> devices that have not expressed support for it.
>
> and the SDHCI driver was fixed accordingly in
>
> Author: Ard Biesheuvel 
> Date:   Mon Sep 5 09:51:48 2016 +0100
>
> MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA
>
> PCI controller drivers must set the 
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE
> attribute if the controller supports 64-bit DMA addressing.
>
> So before these changes, we were in the exact same situation, but
> since PC platforms never enable DMA above 4 GB in the first place,
> nobody ever noticed until we started running this code on arm64
> platforms that have no 32-bit addressable DRAM to begin with.
>
> The obvious conclusion is that the driver should not set the
> EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
> not support it, or, which seems to be our case, if the driver does not
> implement the 64-bit DMA mode that the driver does support.

Correction: that the *device* does support.

> However,
> since there are platforms for which bounce buffering is not an option
> (since there is no 32-bit addressable memory to bounce to), this is
> not just a performance optimization, and so it would be useful to fix
> the code so it can drive all 64-bit DMA capable hardware.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Cohen, Eugene 
Ashish,

Yes this issue existed before V4 support came in.

The previous code would test the (V3) SysBus64 bit: 
https://github.com/tianocore/edk2/blob/7f3b0bad4bbb3cb24014d2e6216615896ea09dbf/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L652

And then return an error building the ADMA descriptor table because the address 
is not within 32-bit DMA range: 
https://github.com/tianocore/edk2/blob/7f3b0bad4bbb3cb24014d2e6216615896ea09dbf/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1295

Eugene

From: Ashish Singhal 
Sent: Thursday, February 28, 2019 5:19 PM
To: Cohen, Eugene ; Wu, Hao A ; 
edk2-devel@lists.01.org; Ard Biesheuvel 
Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Eugene,

Small question. Did the issue appear after the V4 patch went in? Looking at the 
code before that patch, we were enabling 64b dma in pci based on capability 
register already despite of driver supporting only 32b dma.

Thanks
Ashish

Get Outlook for iOS


From: Cohen, Eugene mailto:eug...@hp.com>>
Sent: Thursday, February 28, 2019 5:11 PM
To: Ashish Singhal; Wu, Hao A; 
edk2-devel@lists.01.org; Ard Biesheuvel
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Ashish,

Agreed - #2 would be better in the long run since this will have better 
performance by eliminating the bounce buffering.  My original intent in 
submitting the patch was to fix the logic the current implementation with 
minimal impact.

Thanks,

Eugene
From: Ashish Singhal mailto:ashishsin...@nvidia.com>>
Sent: Thursday, February 28, 2019 4:58 PM
To: Cohen, Eugene mailto:eug...@hp.com>>; Wu, Hao A 
mailto:hao.a...@intel.com>>; 
edk2-devel@lists.01.org; Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Eugene,

Thanks for pointing that out. This is a use case we are not covering as of now. 
I see two options to this:


  1.  Do not enable 64b DMA support in PCI based on V3 as driver does not 
support V3 64b ADMA. This is a quick fix.
  2.  Enable V3 64b ADMA support to add the missing feature. This will take 
maybe a day or two and can be done.

Thanks
Ashish

From: Cohen, Eugene mailto:eug...@hp.com>>
Sent: Thursday, February 28, 2019 3:40 PM
To: Ashish Singhal mailto:ashishsin...@nvidia.com>>; 
Wu, Hao A mailto:hao.a...@intel.com>>; 
edk2-devel@lists.01.org; Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Ashish,

I think that code will still fail for our use case.  We are version 3 with 
64-bit support so Private->Capability[Slot].SysBus64V3 == 0 will evaluate to 
FALSE.  Since we are V3 Private->ControllerVersion[Slot] >= 
SD_MMC_HC_CTRL_VER_400 will also evaluate to FALSE.  Therefore Support64BitDma 
will still be TRUE resulting in DUAL_ADDRESS_CYCLE being set which disables 
bounce buffering.

Since no code is in place to do V3 64b DMA we will still hit the same problem, 
specifically namely that buffers that are not DMAable will be allocated and we 
will still fail the check 
here.

Until such time that V3 64b DMA support is in place I believe only the V4 bit 
should be evaluated.

Eugene

From: Ashish Singhal mailto:ashishsin...@nvidia.com>>
Sent: Thursday, February 28, 2019 3:21 PM
To: Cohen, Eugene mailto:eug...@hp.com>>; Wu, Hao A 
mailto:hao.a...@intel.com>>; 
edk2-devel@lists.01.org; Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Eugene,

Thanks for the explanation. The problem is valid and is more clear to me now. 
How about we do this:

Instead of:

if (Private->Capability[Slot].SysBus64V3 == 0 &&
Private->Capability[Slot].SysBus64V4 == 0) {
  Support64BitDma = FALSE;
}

What do you think about:

if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 &&
 Private->Capability[Slot].SysBus64V3 == 0) ||
(Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_400 &&
 Private->Capability[Slot].SysBus64V4 == 0)) {
  Support64BitDma = FALSE;
}

With this, we would be checking 64b capability based on the version we are 
using and not for something we may not be using despite of being advertised in 
the controller.

Thanks
Ashish

From: Cohen, Eugene mailto:eug...@hp.com>>
Sent: Thursday, February 28, 2019 2:59 PM
To: Ashish Singhal mailto:ashishsin...@nvidia.com>>; 
Wu, Hao A mailto:hao.a...@intel.com>>; 
edk2-devel@lists.01.org; Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>
Subject: RE: [PATCH]

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Ard Biesheuvel 
On Fri, 1 Mar 2019 at 01:19, Ashish Singhal  wrote:
>
> Eugene,
>
> Small question. Did the issue appear after the V4 patch went in? Looking at 
> the code before that patch, we were enabling 64b dma in pci based on 
> capability register already despite of driver supporting only 32b dma.
>

I think this may have been an oversight on my part when I originally
added the DUAL_ADDRESS_CYCLE handling.

The following commit added EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE to the
host bridge driver

commit e58a71d9c50ba641b5ab19f5ce2cbf772187de4d
Author: Ard Biesheuvel 
Date:   Mon Sep 5 09:55:16 2016 +0100

MdeModulePkg/PciHostBridgeDxe: restrict 64-bit DMA to devices that
support it

Currently, the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute is completely
ignored by the PCI host bridge driver, which means that, on an
implementation
that supports DMA above 4 GB, allocations above 4 GB may be provided to
devices that have not expressed support for it.

and the SDHCI driver was fixed accordingly in

Author: Ard Biesheuvel 
Date:   Mon Sep 5 09:51:48 2016 +0100

MdeModulePkg/SdMmcPciHcDxe: enable 64-bit PCI DMA

PCI controller drivers must set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE
attribute if the controller supports 64-bit DMA addressing.

So before these changes, we were in the exact same situation, but
since PC platforms never enable DMA above 4 GB in the first place,
nobody ever noticed until we started running this code on arm64
platforms that have no 32-bit addressable DRAM to begin with.

The obvious conclusion is that the driver should not set the
EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
not support it, or, which seems to be our case, if the driver does not
implement the 64-bit DMA mode that the driver does support. However,
since there are platforms for which bounce buffering is not an option
(since there is no 32-bit addressable memory to bounce to), this is
not just a performance optimization, and so it would be useful to fix
the code so it can drive all 64-bit DMA capable hardware.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2] MdePkg/Library: Install dummy variable arch protocol

2019-03-01 Thread Jagadeesh Ujja 
In a system implementing the variable store in MM, there are no variable
arch protocol and variable write arch protocol installed into the
DXE_SMM protocol database. On such systems, it is not required to
locate these protocols by the DXE runtime variable drivers because
it can be assumed that these protocols are already installed in the MM
context. But then such an implementation will deviate from the existing
traditional MM based variable driver implementation.

So in order to maintain consistency with the traditional MM variable
driver implementation, allow platforms to install dummy versions of
these protocols into the DXE protocol database but these protocol will
not be consumed by non-secure variable service runtime driver.

The Platform which uses StandaloneMM based secure variable storage
have to include this library as below.

  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf {

  NULL|MdePkg/Library/VariableMmDependency/VariableMmDependency.inf
  }

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jagadeesh Ujja 
---
Changes since v1:
- This is a next version of patch 
   “MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch 
Protocol”.
   [https://lists.01.org/pipermail/edk2-devel/2019-February/036885.html]
- Addressed the comments from Ard Biesheuvel and Zeng Star
- Can this library be placed in MdePkg rather then the StandaloneMmPkg?

 MdePkg/Library/VariableMmDependency/VariableMmDependency.c   | 85 

 MdePkg/Library/VariableMmDependency/VariableMmDependency.inf | 48 +++
 2 files changed, 133 insertions(+)

diff --git a/MdePkg/Library/VariableMmDependency/VariableMmDependency.c 
b/MdePkg/Library/VariableMmDependency/VariableMmDependency.c
new file mode 100644
index 000..6e5117e
--- /dev/null
+++ b/MdePkg/Library/VariableMmDependency/VariableMmDependency.c
@@ -0,0 +1,85 @@
+/** @file
+  Runtime DXE part corresponding to StanaloneMM variable module.
+
+This module installs dummy variable arch protocol and dummy variable write 
arch protocol
+to StandaloneMM runtime variable service.
+
+Copyright (c) 2019, ARM Ltd. All rights reserved.
+
+This program and the accompanying materials
+are licensed and made available under the terms and conditions of the BSD 
License
+which accompanies this distribution.  The full text of the license may be 
found at
+http://opensource.org/licenses/bsd-license.php.
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+#include 
+
+/**
+  Notify the system that the SMM variable driver is ready.
+**/
+VOID
+VariableNotifySmmReady (
+  VOID
+  )
+{
+  EFI_STATUSStatus;
+  EFI_HANDLEHandle;
+
+  Handle = NULL;
+  Status = gBS->InstallProtocolInterface (
+  ,
+  ,
+  EFI_NATIVE_INTERFACE,
+  NULL
+  );
+  ASSERT_EFI_ERROR (Status);
+}
+
+/**
+  Notify the system that the SMM variable write driver is ready.
+**/
+VOID
+VariableNotifySmmWriteReady (
+  VOID
+  )
+{
+  EFI_STATUSStatus;
+  EFI_HANDLEHandle;
+
+  Handle = NULL;
+  Status = gBS->InstallProtocolInterface (
+  ,
+  ,
+  EFI_NATIVE_INTERFACE,
+  NULL
+  );
+  ASSERT_EFI_ERROR (Status);
+}
+
+/**
+  The constructor function calls and installs dummy variable arch protocol and
+  dummy variable write arch protocol to StandaloneMM runtime variable service
+
+  @param  ImageHandle   The firmware allocated handle for the EFI image.
+  @param  SystemTable   A pointer to the Management mode System Table.
+
+  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
+
+**/
+EFI_STATUS
+EFIAPI
+VariableMmDependencyLibConstructor (
+  IN EFI_HANDLE   ImageHandle,
+  IN EFI_SYSTEM_TABLE *SystemTable
+  )
+{
+  VariableNotifySmmReady();
+  VariableNotifySmmWriteReady();
+  return EFI_SUCCESS;
+}
+
diff --git a/MdePkg/Library/VariableMmDependency/VariableMmDependency.inf 
b/MdePkg/Library/VariableMmDependency/VariableMmDependency.inf
new file mode 100644
index 000..09fd200
--- /dev/null
+++ b/MdePkg/Library/VariableMmDependency/VariableMmDependency.inf
@@ -0,0 +1,48 @@
+## @file
+#  Runtime DXE part corresponding to StanaloneMM variable module.
+#
+#  This module installs dummy variable arch protocol and dummy variable write 
arch protocol
+#  to StandaloneMM runtime variable service.
+#
+# Copyright (c) 2019, ARM Ltd. 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

[edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA Support

2019-03-01 Thread Ashish Singhal 
Driver was supporting only 32b DMA support for V3 controllers. Add
support for 64b DMA as well for completeness.

For V4.0 64b support, driver was looking at incorrect capability
register bit. Fix for that is present as well.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ashish Singhal 
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  10 +-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 199 ++---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  29 ++-
 4 files changed, 170 insertions(+), 74 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index b474f8d..9b7b88c 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -6,7 +6,7 @@
 
   It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
 
-  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.
   Copyright (c) 2015 - 2019, 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
@@ -666,8 +666,12 @@ SdMmcPciHcDriverBindingStart (
 // If any of the slots does not support 64b system bus
 // do not enable 64b DMA in the PCI layer.
 //
-if (Private->Capability[Slot].SysBus64V3 == 0 &&
-Private->Capability[Slot].SysBus64V4 == 0) {
+if ((Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_300 &&
+ Private->Capability[Slot].SysBus64V3 == 0) ||
+(Private->ControllerVersion[Slot] == SD_MMC_HC_CTRL_VER_400 &&
+ Private->Capability[Slot].SysBus64V3 == 0) ||
+(Private->ControllerVersion[Slot] >= SD_MMC_HC_CTRL_VER_410 &&
+ Private->Capability[Slot].SysBus64V4 == 0)) {
   Support64BitDma = FALSE;
 }
 
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index 1bb701a..68d8a5c 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -2,7 +2,7 @@
 
   Provides some data structure definitions used by the SD/MMC host controller 
driver.
 
-Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
+Copyright (c) 2018-2019, NVIDIA CORPORATION. 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
@@ -145,13 +145,15 @@ typedef struct {
   EFI_PHYSICAL_ADDRESSDataPhy;
   VOID*DataMap;
   SD_MMC_HC_TRANSFER_MODE Mode;
+  SD_MMC_HC_ADMA_LEGTHLength;
 
   EFI_EVENT   Event;
   BOOLEAN Started;
   UINT64  Timeout;
 
   SD_MMC_HC_ADMA_32_DESC_LINE *Adma32Desc;
-  SD_MMC_HC_ADMA_64_DESC_LINE *Adma64Desc;
+  SD_MMC_HC_ADMA_64_V3_DESC_LINE  *Adma64V3Desc;
+  SD_MMC_HC_ADMA_64_V4_DESC_LINE  *Adma64V4Desc;
   EFI_PHYSICAL_ADDRESSAdmaDescPhy;
   VOID*AdmaMap;
   UINT32  AdmaPages;
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index d73fa10..a6d2395 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -6,7 +6,7 @@
 
   It would expose EFI_SD_MMC_PASS_THRU_PROTOCOL for upper layer use.
 
-  Copyright (c) 2018, NVIDIA CORPORATION. All rights reserved.
+  Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.
   Copyright (c) 2015 - 2019, 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
@@ -1010,18 +1010,32 @@ SdMmcHcInitV4Enhancements (
   if (ControllerVer >= SD_MMC_HC_CTRL_VER_400) {
 HostCtrl2 = SD_MMC_HC_V4_EN;
 //
-// Check if V4 64bit support is available
+// Check if controller version V4.0
 //
-if (Capability.SysBus64V4 != 0) {
-  HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
-  DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
+if (ControllerVer == SD_MMC_HC_CTRL_VER_400) {
+  //
+  // Check if 64bit support is available
+  //
+  if (Capability.SysBus64V3 != 0) {
+HostCtrl2 |= SD_MMC_HC_64_ADDR_EN;
+DEBUG ((DEBUG_INFO, "Enabled V4 64 bit system bus support\n"));
+  }
 }
 //
 // Check if controller version V4.10 or higher
 //
-if (ControllerVer >= SD_MMC_HC_CTRL_VER_410) {
-  HostCtrl2 |= SD_MMC_HC_26_DATA_LEN_ADMA_EN;
-  DEBUG

Re: [edk2] [PATCH V2] BaseTools:Run packagedoc_cli.py to generate doc failed

2019-03-01 Thread Carsey, Jaben 
Reviewed-by: Jaben Carsey 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Fan, ZhijuX
> Sent: Thursday, February 28, 2019 6:52 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming 
> Subject: [edk2] [PATCH V2] BaseTools:Run packagedoc_cli.py to generate
> doc failed
> 
> The reason for this problem is that the file was opened incorrectly.
> 
> Cc: Bob Feng 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zhiju.Fan 
> ---
>  .../plugins/EdkPlugins/edk2/model/doxygengen.py| 7 
> ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git
> a/BaseTools/Scripts/PackageDocumentTools/plugins/EdkPlugins/edk2/mode
> l/doxygengen.py
> b/BaseTools/Scripts/PackageDocumentTools/plugins/EdkPlugins/edk2/mod
> el/doxygengen.py
> index e31df262bc..73349e2f48 100644
> ---
> a/BaseTools/Scripts/PackageDocumentTools/plugins/EdkPlugins/edk2/mode
> l/doxygengen.py
> +++
> b/BaseTools/Scripts/PackageDocumentTools/plugins/EdkPlugins/edk2/mod
> el/doxygengen.py
> @@ -376,9 +376,10 @@ class PackageDocumentAction(DoxygenAction):
>  return
> 
>  try:
> -f = open(path, 'r')
> -lines = f.readlines()
> -f.close()
> +with open(path, 'r') as f:
> +lines = f.readlines()
> +except UnicodeDecodeError:
> +return
>  except IOError:
>  ErrorMsg('Fail to open file %s' % path)
>  return
> --
> 2.14.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] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

2019-03-01 Thread Laszlo Ersek 
On 03/01/19 16:09, Peter Maydell wrote:
> On Fri, 1 Mar 2019 at 14:59, Laszlo Ersek  wrote:
>>
>> +Peter
>>
>> On 03/01/19 13:24, Heyi Guo wrote:
>>> On 2019/2/28 21:39, Laszlo Ersek wrote:
>>
 (4) What's most worrying is that this change would lead to an unexpected
 sharing of the PL011 device between the OS and the firmware. I don't
 think that's a great idea, especially if QEMU's ACPI payload explicitly
 advertises the port to the OS.
>>> That's true, so I propose to disable the feature by default. It is only
>>> used for UEFI runtime code debug. It is always painful when we don't a
>>> handy debug tool for runtime services code.
>>
>> I think this is the only problem that we have at the design level.
>>
>> What I'd like to understand is whether it is safe to drive the PL011
>> serial port from both the firmware and the kernel.
> 
> I would suggest that it probably is not safe. Certainly it
> does not seem likely to be robust against all possible future
> changes to the kernel's driver and/or the firmware's driver.
> 
>> This is why I'm not a big fan of this approach. Using separate devices
>> for kernel and firmware would be a lot better.
>>
>> I remember that Peter did some work to enable two PL011 devices on the
>> "virt" board. IIRC the issue was that the PL011 enumeration order /
>> numbering in edk2, and in the Linux (guest) kernel, was exactly the
>> opposite. And that caused both logs to go to different devices; you
>> couldn't have a single log file that started with the firmware log, and
>> continued (after a definite switch-over) with the kernel log.
> 
> Yes, that's basically it (I forget the exact details and it's the
> kind of thing that may well depend on exactly what version of the
> kernel or what version of the firmware you were using). I do still
> have a todo list item to add a 2nd UART (probably "if the user
> specifically asked for it with an extra -serial option"), but
> we can't just add it to the board by default.

Thanks for the info!

> Note that if you are using QEMU under emulation where it is emulating
> EL3 then you always get a second UART which is visible only to the
> secure world. This is specifically intended for firmware to use.

The ArmVirtQemu firmware doesn't handle the secure world at all (yet?).

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

Re: [edk2] [edk2-announce] March Community Meeting

2019-03-01 Thread Rafael Machado 
Hi Stephano

Could you please add at the https://www.tianocore.org/monthly-meeting page
the
South America TZ (São Paulo if possible ) ,   in case these calendar items
are still being used?

Thanks and Regards
Rafael

Em qui, 28 de fev de 2019 às 17:08, stephano <
stephano.cet...@linux.intel.com> escreveu:

>
> On 2/28/2019 11:48 AM, stephano wrote:
> > We will be holding our February Community Meeting next Thursday.
>
> %s/February/March  :)
> ___
> 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] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Ashish Singhal 
Eugene,

I have submitted a patch for supporting 64b DMA on V3 controllers. Can you 
please validate it at your end as well?

Thanks
Ashish

From: Ashish Singhal 
Sent: Friday, March 1, 2019 5:31 AM
To: Ard Biesheuvel ; Cohen, Eugene 
Cc: Wu, Hao A ; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 
SW1Lab.) 
Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

I've already started refactoring the driver to support V3 64b ADMA2.

Meanwhile, I'm OK with the proposed patch.

Thanks
Ashish

Get Outlook for iOS


From: Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>
Sent: Friday, March 1, 2019 4:40 AM
To: Cohen, Eugene
Cc: Ashish Singhal; Wu, Hao A; 
edk2-devel@lists.01.org; Kim, Sangwoo (김상우 
SW1Lab.)
Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene 
mailto:eug...@hp.com>> wrote:
>
> Ard,
>
> > So before these changes, we were in the exact same situation, but since PC
> > platforms never enable DMA above 4 GB in the first place, nobody ever
> > noticed until we started running this code on arm64 platforms that have no
> > 32-bit addressable DRAM to begin with.
>
> Interesting - I did not realize that there were designs that were crazy 
> enough to have no addressable DRAM below 4G.
>

You must be new here :-)

But seriously, it does make sense for an implementation to, say, put
all peripherals, PCIe resource windows etc in the bottom half and all
DRAM in the top half of a 40-bit address space, which is how the AMD
Seattle SoC ended with its system memory at address 0x80__.
Note that on this platform, we can still use 32-bit DMA if we want to
with the help of the SMMUs, but we haven't wired those up in UEFI (and
the generic host bridge driver did not have the IOMMU hooks at the
time)

> > The obvious conclusion is that the driver should not set the
> > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
> > not support it, or, which seems to be our case, if the driver does not
> > implement the 64-bit DMA mode that the driver does support. However,
> > since there are platforms for which bounce buffering is not an option (since
> > there is no 32-bit addressable memory to bounce to), this is not just a
> > performance optimization, and so it would be useful to fix the code so it 
> > can
> > drive all 64-bit DMA capable hardware.
>
> Okay, that's a great reason - let's get V3 64b ADMA2 in!
>
> Any objection to committing the original patch in the short term?
>

not at all

Acked-by: Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [edk2-announce] Hard Feature Freeze starts from 2019-03-01(00:00:00 UTC-8) for edk2-stable201903

2019-03-01 Thread Gao, Liming 
Hi, all
  Today, we enter into Hard Feature Freeze phase until edk2-stable201903 tag is 
created at 2019-03-08. In this phase, there is no feature to be pushed. The bug 
fix is still allowed. 

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gao,
>Liming
>Sent: Friday, February 22, 2019 10:26 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [edk2-announce] Soft Feature Freeze starts today for edk2-
>stable201903
>
>Hi, all
>  https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-
>Planning lists edk2-stable201903 tag planning. Now, we enter into Soft
>Feature Freeze phase. In this phase, the feature without Reviewed-by or
>Acked-by tags will be delayed after the upcoming stable tag. The patch review
>can continue without break. Below is edk2-stable201903 tag planning.
>
>2019-03-08 Beginning of development
>2019-02-22 Soft Feature Freeze
>2019-03-01 Hard Feature Freeze
>2019-03-08 Release
>
>Thanks
>Liming
>
>___
>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 v2] MdePkg/Library: Install dummy variable arch protocol

2019-03-01 Thread Zeng, Star 
Agree with Ard's feedbacks.

And it seems only needed for VariableSmmRuntimeDxe to co-work with 
VariableStandaloneMm.
So is it more suitable to be in StandaloneMmPkg?


Thanks,
Star
-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Friday, March 1, 2019 7:31 PM
To: Jagadeesh Ujja 
Cc: edk2-devel@lists.01.org; Gao, Liming ; Zhang, Chao B 
; Leif Lindholm ; Zeng, Star 
; Yao, Jiewen ; Kinney, Michael D 

Subject: Re: [PATCH v2] MdePkg/Library: Install dummy variable arch protocol

On Fri, 1 Mar 2019 at 12:14, Jagadeesh Ujja  wrote:
>
> In a system implementing the variable store in MM, there are no 
> variable arch protocol and variable write arch protocol installed into 
> the DXE_SMM protocol database. On such systems, it is not required to 
> locate these protocols by the DXE runtime variable drivers because it 
> can be assumed that these protocols are already installed in the MM 
> context. But then such an implementation will deviate from the 
> existing traditional MM based variable driver implementation.
>
> So in order to maintain consistency with the traditional MM variable 
> driver implementation, allow platforms to install dummy versions of 
> these protocols into the DXE protocol database but these protocol will 
> not be consumed by non-secure variable service runtime driver.
>
> The Platform which uses StandaloneMM based secure variable storage 
> have to include this library as below.
>
>   ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf {
> 
>   NULL|MdePkg/Library/VariableMmDependency/VariableMmDependency.inf
>   }
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jagadeesh Ujja 
> ---
> Changes since v1:
> - This is a next version of patch
>“MdeModulePkg/VariableSmmRuntimeDxe: Refactor locating Variable Arch 
> Protocol”.
>
> [https://lists.01.org/pipermail/edk2-devel/2019-February/036885.html]
> - Addressed the comments from Ard Biesheuvel and Zeng Star
> - Can this library be placed in MdePkg rather then the StandaloneMmPkg?
>

This does not belong in MdePkg. What is wrong with keeping it in 
StandaloneMmPkg?

>  MdePkg/Library/VariableMmDependency/VariableMmDependency.c   | 85 
> 
>  MdePkg/Library/VariableMmDependency/VariableMmDependency.inf | 48 
> +++
>  2 files changed, 133 insertions(+)
>
> diff --git 
> a/MdePkg/Library/VariableMmDependency/VariableMmDependency.c 
> b/MdePkg/Library/VariableMmDependency/VariableMmDependency.c
> new file mode 100644
> index 000..6e5117e
> --- /dev/null
> +++ b/MdePkg/Library/VariableMmDependency/VariableMmDependency.c
> @@ -0,0 +1,85 @@
> +/** @file
> +  Runtime DXE part corresponding to StanaloneMM variable module.
> +
> +This module installs dummy variable arch protocol and dummy variable 
> +write arch protocol to StandaloneMM runtime variable service.
> +

I think 'dummy' is a misnomer here. They are NULL protocols in the sense that 
only their presence is signifcant, and the protocol does not have an 
implementation. But this is true for traditional MM as well.

> +Copyright (c) 2019, ARM Ltd. All rights reserved.
> +
> +This program and the accompanying materials are licensed and made 
> +available under the terms and conditions of the BSD License which 
> +accompanies this distribution.  The full text of the license may be 
> +found at http://opensource.org/licenses/bsd-license.php.
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, 
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +
> +/**
> +  Notify the system that the SMM variable driver is ready.
> +**/
> +VOID
> +VariableNotifySmmReady (
> +  VOID
> +  )
> +{
> +  EFI_STATUSStatus;
> +  EFI_HANDLEHandle;
> +
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (
> +  ,
> +  ,
> +  EFI_NATIVE_INTERFACE,
> +  NULL
> +  );
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  Notify the system that the SMM variable write driver is ready.
> +**/
> +VOID
> +VariableNotifySmmWriteReady (
> +  VOID
> +  )
> +{
> +  EFI_STATUSStatus;
> +  EFI_HANDLEHandle;
> +
> +  Handle = NULL;
> +  Status = gBS->InstallProtocolInterface (
> +  ,
> +  ,
> +  EFI_NATIVE_INTERFACE,
> +  NULL
> +  );
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  The constructor function calls and installs dummy variable arch 
> +protocol and
> +  dummy variable write arch protocol to StandaloneMM runtime variable 
> +service
> +
> +  @param  ImageHandle   The firmware allocated handle for the EFI image.
> +  @param  SystemTable   A pointer to the Management mode System Table.
> +
> +  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
>

Re: [edk2] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

2019-03-01 Thread Heyi Guo 




On 2019/2/28 21:39, Laszlo Ersek wrote:

Hello Heyi,

On 02/28/19 09:05, Heyi Guo wrote:

Serial port output is useful when debugging UEFI runtime services in OS runtime.
The patches are trying to provide a handy method to enable runtime serial port
debug for ArmVirtQemu.

Cc: Jian J Wang 
Cc: Hao Wu 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Cc: Julien Grall 

Heyi Guo (3):
   MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
   ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
   ArmVirtQemu: enable runtime debug by build flag

  ArmVirtPkg/ArmVirt.dsc.inc
  |   6 ++
  ArmVirtPkg/ArmVirtQemu.dsc
  |  13 +++
  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc  
  |   5 +
  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c  
  |  29 --
  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h  
  |  27 +
  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
  |   3 +
  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
  |  27 +
  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c   
  | 104 
  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf 
  |  58 +++
  MdeModulePkg/MdeModulePkg.dec 
  |   6 ++
  MdeModulePkg/MdeModulePkg.uni 
  |   6 ++
  
MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
   |   2 +-
  
MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
 |   7 +-
  13 files changed, 279 insertions(+), 14 deletions(-)
  create mode 100644 
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
  create mode 100644 
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
  create mode 100644 
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
  create mode 100644 
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf


I'm quite swamped, so only a few general comments for now.

I'll let the MdeModulePkg maintainers comment on the first patch.

(1) The general idea to utilize

- MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode
- MdePkg/Library/DxeRuntimeDebugLibSerialPort

seems mostly OK to me.

Although, I think I would actually prefer the reverse implementation (=
don't send debug messages to the status code infrastructure; instead,
process status codes by logging them with DEBUG). I don't think there is
a status code handler already present in edk2 for that however.


(2) The use case is too generic. Until very recently, we hadn't enabled
status code routing and reporting in ArmVirtPkg at all. We only did that
recently, in commit 5c574b222ec2 ("ArmVirtPkg/ArmVirtQemu*: enable
minimal Status Code Routing in DXE", 2019-02-25), because there was no
other way to implement the boot progress reporting that I had been
after. (See commit 1797f32e0a19 ("ArmVirtPkg/PlatformBootManagerLib:
display boot option loading/starting", 2019-02-25).)

So, in the proposal, the specific use should be described. What runtime
module do you want status codes from?

And why do you prefer status codes to actual (runtime) debug messages?

... Are you perhaps using this approach only because it lets you print
DEBUGs at runtime? I.e., do you use status codes only as a "runtime
carrier" for DEBUGs?

That's true:) StatusCode is only a carrier. We can see lots of X86 platforms 
use StatusCode driver for DEBUG serial print.




(3) Patch #2 ("ArmVirtPkg: add runtime instance of
FdtPL011SerialPortLib") looks confusing. It introduces a file called
"FdtPL011SerialPortLibCommon.c", which doesn't seem common to multiple
INF files. It is added only to one file.

Yes, the file name is not good. It only contains a null hook to build a 
non-runtime instance. I didn't get a proper name for that. Maybe RuntimeDummy 
or NoRuntime?


Also, the patch changes the behavior of the current lib instance with
regard to the SerialPortInitialize() function and the library
constructor. I don't see why that is necessary just for adding a runtime
instance. The runtime instance should not affect the behavior of the
existent instance.

Sorry if I misunderstood; a more detailed commit message would be nice.

Ah, this is a mistake. In current implementation we can still enable the 
constructor.

The long story is: at first I proposed to still use BaseSerialPortDebugLib, so 
I added RuntimePrepare() function into the constructor directly, but the 
builder complained about looped constructors, for RuntimePrepare() invokes gBS 
and some RunTime libraries. Then I tried disabling the constructor and moved 
RuntimePrepare() into SerialPortInitialize()

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Ashish Singhal 
I've already started refactoring the driver to support V3 64b ADMA2.

Meanwhile, I'm OK with the proposed patch.

Thanks
Ashish

Get Outlook for iOS


From: Ard Biesheuvel 
Sent: Friday, March 1, 2019 4:40 AM
To: Cohen, Eugene
Cc: Ashish Singhal; Wu, Hao A; edk2-devel@lists.01.org; Kim, Sangwoo (김상우 
SW1Lab.)
Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene  wrote:
>
> Ard,
>
> > So before these changes, we were in the exact same situation, but since PC
> > platforms never enable DMA above 4 GB in the first place, nobody ever
> > noticed until we started running this code on arm64 platforms that have no
> > 32-bit addressable DRAM to begin with.
>
> Interesting - I did not realize that there were designs that were crazy 
> enough to have no addressable DRAM below 4G.
>

You must be new here :-)

But seriously, it does make sense for an implementation to, say, put
all peripherals, PCIe resource windows etc in the bottom half and all
DRAM in the top half of a 40-bit address space, which is how the AMD
Seattle SoC ended with its system memory at address 0x80__.
Note that on this platform, we can still use 32-bit DMA if we want to
with the help of the SMMUs, but we haven't wired those up in UEFI (and
the generic host bridge driver did not have the IOMMU hooks at the
time)

> > The obvious conclusion is that the driver should not set the
> > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does
> > not support it, or, which seems to be our case, if the driver does not
> > implement the 64-bit DMA mode that the driver does support. However,
> > since there are platforms for which bounce buffering is not an option (since
> > there is no 32-bit addressable memory to bounce to), this is not just a
> > performance optimization, and so it would be useful to fix the code so it 
> > can
> > drive all 64-bit DMA capable hardware.
>
> Okay, that's a great reason - let's get V3 64b ADMA2 in!
>
> Any objection to committing the original patch in the short term?
>

not at all

Acked-by: Ard Biesheuvel 

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

2019-03-01 Thread Ard Biesheuvel 
On Fri, 1 Mar 2019 at 15:59, Laszlo Ersek  wrote:
>
> +Peter
>
> On 03/01/19 13:24, Heyi Guo wrote:
> > On 2019/2/28 21:39, Laszlo Ersek wrote:
>
> >> (4) What's most worrying is that this change would lead to an unexpected
> >> sharing of the PL011 device between the OS and the firmware. I don't
> >> think that's a great idea, especially if QEMU's ACPI payload explicitly
> >> advertises the port to the OS.
> > That's true, so I propose to disable the feature by default. It is only
> > used for UEFI runtime code debug. It is always painful when we don't a
> > handy debug tool for runtime services code.
>
> I think this is the only problem that we have at the design level.
>
> What I'd like to understand is whether it is safe to drive the PL011
> serial port from both the firmware and the kernel. Consider a situation
> where one VCPU in the guest executes a runtime service call, and writes
> to PL011 from firmware code. Meanwhile a different VCPU in the guest
> executes some kernel code that produces a log message directly on the
> serial console. (Because, I don't think that a runtime service call on
> one CPU stops the world for *all* CPUs, in the Linux kernel.) For
> starters, the serial output could be garbled, as a consequence, but will
> the PL011 register state be messed up irrevocably as well? I don't know.
>

This is fundamentally broken. The OS will drive the PL011 in interrupt
mode, while the firmware will poll the FIFO registers directly.

> This is why I'm not a big fan of this approach. Using separate devices
> for kernel and firmware would be a lot better.
>

+1

> I remember that Peter did some work to enable two PL011 devices on the
> "virt" board. IIRC the issue was that the PL011 enumeration order /
> numbering in edk2, and in the Linux (guest) kernel, was exactly the
> opposite. And that caused both logs to go to different devices; you
> couldn't have a single log file that started with the firmware log, and
> continued (after a definite switch-over) with the kernel log.
>
> But in this case, where the firmware could produce log messages on
> serial during OS runtime, that's actually the setup I would recommend. A
> clean separation between the serial devices used by the firmware and the OS.
>
> The rest of the issues in this series should be more simple to clean up
> (rework some commit messages, remove stale code etc).
>
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

2019-03-01 Thread Ashish Singhal 
Acked-by: Ashish Singhal 

-Original Message-
From: Ard Biesheuvel  
Sent: Friday, March 1, 2019 4:39 AM
To: Cohen, Eugene 
Cc: Ashish Singhal ; Wu, Hao A ; 
edk2-devel@lists.01.org; Kim, Sangwoo (김상우 SW1Lab.) 
Subject: Re: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

On Fri, 1 Mar 2019 at 11:54, Cohen, Eugene  wrote:
>
> Ard,
>
> > So before these changes, we were in the exact same situation, but 
> > since PC platforms never enable DMA above 4 GB in the first place, 
> > nobody ever noticed until we started running this code on arm64 
> > platforms that have no 32-bit addressable DRAM to begin with.
>
> Interesting - I did not realize that there were designs that were crazy 
> enough to have no addressable DRAM below 4G.
>

You must be new here :-)

But seriously, it does make sense for an implementation to, say, put all 
peripherals, PCIe resource windows etc in the bottom half and all DRAM in the 
top half of a 40-bit address space, which is how the AMD Seattle SoC ended with 
its system memory at address 0x80__.
Note that on this platform, we can still use 32-bit DMA if we want to with the 
help of the SMMUs, but we haven't wired those up in UEFI (and the generic host 
bridge driver did not have the IOMMU hooks at the
time)

> > The obvious conclusion is that the driver should not set the 
> > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute if the device does 
> > not support it, or, which seems to be our case, if the driver does 
> > not implement the 64-bit DMA mode that the driver does support. 
> > However, since there are platforms for which bounce buffering is not 
> > an option (since there is no 32-bit addressable memory to bounce 
> > to), this is not just a performance optimization, and so it would be 
> > useful to fix the code so it can drive all 64-bit DMA capable hardware.
>
> Okay, that's a great reason - let's get V3 64b ADMA2 in!
>
> Any objection to committing the original patch in the short term?
>

not at all

Acked-by: Ard Biesheuvel 

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

2019-03-01 Thread Laszlo Ersek 
+Peter, for the last few paragraphs

On 02/28/19 13:10, Ard Biesheuvel wrote:
> On Thu, 28 Feb 2019 at 09:06, Heyi Guo  wrote:
>>
>> Serial port output is useful when debugging UEFI runtime services in OS 
>> runtime.
>> The patches are trying to provide a handy method to enable runtime serial 
>> port
>> debug for ArmVirtQemu.
>>
>> Cc: Jian J Wang 
>> Cc: Hao Wu 
>> Cc: Laszlo Ersek 
>> Cc: Ard Biesheuvel 
>> Cc: Julien Grall 
>>
>> Heyi Guo (3):
>>   MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>   ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>   ArmVirtQemu: enable runtime debug by build flag
>>
> 
> Hello Heyi,
> 
> We have had this discussion before, and IIRC, the proposed solution
> was to use status codes.
> 
> I'm not sure how that is supposed to work though - hopefully Laszlo or
> one of the MdeModulePkg maintainers can elaborate.

Here's the basic idea.

Status Code reporting and routing are defined by the PI spec for OS
runtime as well, not just boot time.

Recently we added status code *routing* to ArmVirtPkg, in commit
5c574b222ec2, via the generic runtime driver
"ReportStatusCodeRouterRuntimeDxe". We also added the library resolution

ReportStatusCodeLib -->
MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

(for some module types). As a result, when modules of those types report
status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
driver, which "routes" the status codes.

In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
(built into BdsDxe) to register a status code *handler* callback with
ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
runtime), and only for a very specific group of status codes. As a
result, when a module of suitable type reports a status code,
ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
"handles it" (in our implementation, we simply format it to the UEFI
console), assuming the status code is the kind we are interested in.


Now we come to the current series. First, the series adds the following
DebugLib class resolution:

DebugLib -->
MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf

This will cause modules to publish their DEBUG messages as status codes
via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
too lazy to check the exact status code classes etc that
PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
"routing". As another result, until we reach a late enough stage in the
boot, those messages will not be printed by anything (because there's
not going to be any "handling" for them).

(The present series also updates the ReportStatusCodeLib resolution so
it can be used at runtime too, but that's quite auxiliary to this
discussion.)

Second, this series includes the generic Status Code *handling* driver
(also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
the particular handling that we already have in BdsDxe via the earlier
series, this generic status handler driver registers a handler callback
that simply prints status codes to the serial port (dependent on a PCD
setting), via SerialPortLib.

With the modification from the first patch, this generic status code
*handler* driver is extended to runtime serial port operation. And, the
second patch in the series provides a SerialPortLib instance for it that
can work at runtime.

All in all, when a runtime driver calls DEBUG, this happens:

  runtime driver calls DEBUG
  -> PeiDxeDebugLibReportStatusCode
-> RuntimeDxeReportStatusCodeLib
   [status code reporting]

=> ReportStatusCodeRouterRuntimeDxe
   [status code routing]

=> StatusCodeHandlerRuntimeDxe
   [status code handling, such as SerialPortWrite():]
  -> FdtPL011SerialPortLibRuntime

This is actually a good idea, but it would be nice if:
- QEMU had two PL011 ports,
- the boot time firmware log, and the OS log, went to one port
- the runtme firmware log went to the other port.

Given that this series provides the SerialPortLib instance for
StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
for runtime firmware logging.

I don't insist that this series implement all of that, but it should
either prevent a conflict on the one PL011 between the firmware and the
OS, or else be very explicit about the possible conflict in the commit
messages.

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

Re: [edk2] [Patch v2 0/4] Simplify CPU Features solution.

2019-03-01 Thread Laszlo Ersek 
Hi Eric,

On 03/01/19 06:39, Eric Dong wrote:
> V2 Changes include:
> 1. Add ASSERT to make sure PcdCpuFeaturesSetting and
>PcdCpuFeaturesCapability have equal size.
> 2. Correct comment block on IsCpuFeatureSetInCpuPcd() references
>"PcdCpuFeaturesSupport". It should reference "CpuBitMask".
> 
> V1 Changes includes:
> 1. Optimize PCD PcdCpuFeaturesUserConfiguration 
> 2. Limit useage of PcdCpuFeaturesSupport 
> 3. Remove some useless APIs.
> Detail explanation please check each patch's introduction.
> 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Eric Dong (4):
>   UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions.
>   UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD 
> PcdCpuFeaturesUserConfiguration.
>   UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
>   UefiCpuPkg/RegisterCpuFeaturesLib: Correct comments.
> 
>  .../Include/Library/RegisterCpuFeaturesLib.h   |  34 --
>  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 114 
> ++---
>  .../DxeRegisterCpuFeaturesLib.inf  |   3 +-
>  .../PeiRegisterCpuFeaturesLib.inf  |   3 +-
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |   2 -
>  .../RegisterCpuFeaturesLib.c   |  64 ++--
>  UefiCpuPkg/UefiCpuPkg.dec  |   9 +-
>  7 files changed, 45 insertions(+), 184 deletions(-)
> 

sorry, I'm overloaded. Given that the platforms that I co-maintain don't
use RegisterCpuFeaturesLib, I'd like to skip the v2 review.

Please remember to push this series *after* the edk2-stable201903 tag is
made. Because this is not a bugfix.

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

Re: [edk2] [Patch v2 4/4] UefiCpuPkg/RegisterCpuFeaturesLib: Correct comments.

2019-03-01 Thread Laszlo Ersek 
On 03/01/19 06:39, Eric Dong wrote:
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git 
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c 
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 3e8e899766..a78ef44491 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -1174,8 +1174,8 @@ PreSmmCpuRegisterTableWrite (
>@param[in]  CpuBitMaskSize  The size of CPU feature bit mask buffer
>@param[in]  Feature The bit number of the CPU feature
>  
> -  @retval  TRUE   The CPU feature is set in PcdCpuFeaturesSupport.
> -  @retval  FALSE  The CPU feature is not set in PcdCpuFeaturesSupport.
> +  @retval  TRUE   The CPU feature is set in CpuBitMask.
> +  @retval  FALSE  The CPU feature is not set in CpuBitMask.
>  
>  **/
>  BOOLEAN
> 

Okay, this patch is simple enough, and I vaguely remember that I
suggested it.

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [PATCH 2] UefiCpuPkg: restore strict page attributes via #DB in nonstop mode only

2019-03-01 Thread Laszlo Ersek 
On 03/01/19 04:26, Wang, Jian J wrote:
> Thanks. To catch cold freeze, pushed earlier 
> (2a9324cfca12c66f13a41d52fb0a82fb924e)

This is definitely a bugfix, so it is eligible for pushing.

Thanks
Laszlo

>> -Original Message-
>> From: Dong, Eric
>> Sent: Friday, March 01, 2019 9:55 AM
>> To: Wang, Jian J ; edk2-devel@lists.01.org
>> Cc: Ni, Ray ; Laszlo Ersek ; Zeng, Star
>> 
>> Subject: RE: [edk2] [PATCH 2] UefiCpuPkg: restore strict page attributes via 
>> #DB
>> in nonstop mode only
>>
>> Reviewed-by: Eric Dong 
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>> Jian J Wang
>>> Sent: Friday, March 1, 2019 8:58 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Ni, Ray ; Laszlo Ersek ; Dong,
>>> Eric ; Zeng, Star 
>>> Subject: [edk2] [PATCH 2] UefiCpuPkg: restore strict page attributes via #DB
>>> in nonstop mode only
>>>
 v2: Per Laszlo's comments, repack origianl two patches into one with
 title changed and relevant commits added
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1576
>>>
>>> The root cause of this issue is that non-stop mode of Heap Guard and NULL
>>> Detection set TF bit (single-step) in EFLAG unconditionally in the common
>>> handler in CpuExceptionLib.
>>>
>>> If PcdCpuSmmStaticPageTable is FALSE, the SMM will only create page table
>>> for memory below 4G. If SMM tries to access memory beyond 4G, a page
>>> fault exception will be triggered and the memory to access will be added to
>>> page table so that SMM code can continue the access.
>>>
>>> Because of above issue, the TF bit is set after the page fault is handled 
>>> and
>>> then fall into another DEBUG exception. Since non-stop mode of Heap Guard
>>> and NULL Detection are not enabled, no special DEBUG exception handler is
>>> registered. The default handler just prints exception context and go into
>>> dead loop.
>>>
>>> Actually EFLAGS can be changed in any standard exception handler.
>>> There's no need to do single-step setup in assembly code. So the fix is to
>>> move the logic to C code part of page fault exception handler so that we can
>>> fully validate the configuration and prevent TF bit from being set
>>> unexpectedly.
>>>
>>> Fixes: dcc026217fdc363f55c217039fc43d344f69fed6
>>>16b918bbaf51211a32ae04d9d8a5ba6ccca25a6a
>>> Test:
>>>  - Pass special test of accessing memory beyond 4G in SMM mode
>>>  - Boot to OS with Qemu emulator platform (Fedora27, Ubuntu18.04,
>>>Windows7, Windows10)
>>>
>>> Cc: Eric Dong 
>>> Cc: Laszlo Ersek 
>>> Cc: Ruiyu Ni 
>>> Cc: Star Zeng 
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Jian J Wang 
>>> Acked-by: Laszlo Ersek 
>>> ---
>>>  UefiCpuPkg/CpuDxe/CpuPageTable.c  | 11 ++-
>>>  .../Ia32/ExceptionHandlerAsm.nasm |  7 ---
>>>  .../X64/ExceptionHandlerAsm.nasm  |  4 
>>>  3 files changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> index 4bee8c7772..812537417d 100644
>>> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
>>> @@ -1300,7 +1300,16 @@ PageFaultExceptionHandler (
>>>// Display ExceptionType, CPU information and Image information
>>>//
>>>DumpCpuContext (ExceptionType, SystemContext);
>>> -  if (!NonStopMode) {
>>> +  if (NonStopMode) {
>>> +//
>>> +// Set TF in EFLAGS
>>> +//
>>> +if (mPagingContext.MachineType == IMAGE_FILE_MACHINE_I386) {
>>> +  SystemContext.SystemContextIa32->Eflags |= (UINT32)BIT8;
>>> +} else {
>>> +  SystemContext.SystemContextX64->Rflags |= (UINT64)BIT8;
>>> +}
>>> +  } else {
>>>  CpuDeadLoop ();
>>>}
>>>  }
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.
>>> nasm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.
>>> nasm
>>> index 6fcf5fb23f..45d6474091 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm.
>>> nasm
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionHandlerAsm
>>> +++ .nasm
>>> @@ -383,13 +383,6 @@ ErrorCodeAndVectorOnStack:
>>>  pop dword [ebp - 4]
>>>  mov esp, ebp
>>>  pop ebp
>>> -
>>> -; Enable TF bit after page fault handler runs
>>> -cmp dword [esp], 14   ; #PF?
>>> -jne .5
>>> -bts dword [esp + 16], 8   ; EFLAGS
>>> -
>>> -.5:
>>>  add esp, 8
>>>  cmp dword [esp - 16], 0   ; check
>>> EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
>>>  jz  DoReturn
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
>>> asm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
>>> asm
>>> index f842af2336..7b97810d10 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
>>> asm
>>> +++
>>>

Re: [edk2] [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

2019-03-01 Thread Laszlo Ersek 
+Peter

On 03/01/19 13:24, Heyi Guo wrote:
> On 2019/2/28 21:39, Laszlo Ersek wrote:

>> (4) What's most worrying is that this change would lead to an unexpected
>> sharing of the PL011 device between the OS and the firmware. I don't
>> think that's a great idea, especially if QEMU's ACPI payload explicitly
>> advertises the port to the OS.
> That's true, so I propose to disable the feature by default. It is only
> used for UEFI runtime code debug. It is always painful when we don't a
> handy debug tool for runtime services code.

I think this is the only problem that we have at the design level.

What I'd like to understand is whether it is safe to drive the PL011
serial port from both the firmware and the kernel. Consider a situation
where one VCPU in the guest executes a runtime service call, and writes
to PL011 from firmware code. Meanwhile a different VCPU in the guest
executes some kernel code that produces a log message directly on the
serial console. (Because, I don't think that a runtime service call on
one CPU stops the world for *all* CPUs, in the Linux kernel.) For
starters, the serial output could be garbled, as a consequence, but will
the PL011 register state be messed up irrevocably as well? I don't know.

This is why I'm not a big fan of this approach. Using separate devices
for kernel and firmware would be a lot better.

I remember that Peter did some work to enable two PL011 devices on the
"virt" board. IIRC the issue was that the PL011 enumeration order /
numbering in edk2, and in the Linux (guest) kernel, was exactly the
opposite. And that caused both logs to go to different devices; you
couldn't have a single log file that started with the firmware log, and
continued (after a definite switch-over) with the kernel log.

But in this case, where the firmware could produce log messages on
serial during OS runtime, that's actually the setup I would recommend. A
clean separation between the serial devices used by the firmware and the OS.

The rest of the issues in this series should be more simple to clean up
(rework some commit messages, remove stale code etc).

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

Re: [edk2] [edk2-announce] Soft Feature Freeze starts today for edk2-stable201903

2019-03-01 Thread Laszlo Ersek 
On 03/01/19 02:15, Gao, Liming wrote:
> OK. To be clear, I take your suggestion. wiki page has been updated. 

Looks great, thank you!
Laszlo

> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, February 28, 2019 7:58 PM
>> To: Gao, Liming 
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [edk2-announce] Soft Feature Freeze starts today for 
>> edk2-stable201903
>>
>> On 02/28/19 03:22, Gao, Liming wrote:
>>
>>> I update wiki page 
>>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning
>>>  with UTC zone.
>>> I think the nature time of day is the begin time of one day. 00:00:00 
>>> should be common sense. We may not highlight it.
>>
>> I disagree: many organizations use "start of business" (e.g. 9AM) or
>> "end of business" (e.g. 5PM) as "common sense" deadlines, on any given date.
>>
>> I think it's a really simple update. Just replace, in the header,
>>
>>   Date(UTC-8)
>>
>> with
>>
>>   Date (00:00:00 UTC-8)
>>
>> That should suffice.
>>
>> Thanks
>> Laszlo

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