Re: [edk2] Debugging why Build Rebuilds Something

[Cohen, Eugene]

Liming - thank you - this technique of diff'ing the Makefiles immediately 
identified the issue.

We were defining an environment variable that contained date/time values that 
was used in tools_def resulting in all this unnecessary rebuild behavior.

Thanks a ton!

Eugene

From: Cohen, Eugene
Sent: Thursday, March 21, 2019 4:15 AM
To: 'Gao, Liming' ; edk2-devel@lists.01.org
Subject: RE: Debugging why Build Rebuilds Something

Great - I will try this now, thanks Liming!

From: Gao, Liming mailto:liming@intel.com>>
Sent: Thursday, March 21, 2019 4:02 AM
To: Cohen, Eugene mailto:eug...@hp.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: RE: Debugging why Build Rebuilds Something

Could you help check the timestamp of AutoGen files in OpenSslLib output 
directory?

After the first build, copy OpenSslLib output directory to another directory
After the second build, compare the output directory between two builds, please 
check whether there is the difference for AutoGen.h and Makefile. If no 
difference, please directly trig Makefile to see whether rebuild happen. If 
rebuild happen, it may be the issue in Makefile. Then, further check Makefile.

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Cohen, Eugene
>Sent: Thursday, March 21, 2019 5:50 PM
>To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>Subject: [edk2] Debugging why Build Rebuilds Something
>
>I'm experiencing an annoying problem where OpenSslLib is constantly being
>rebuilt. I don't think I've done anything unusual or different to it but
>nevertheless it gets built almost every time I rebuild the same platform.
>
>I don't believe any source file timestamps are changing so I think this may be
>the build.py tool deciding to re-generate stuff. (As a side note: does the
>Autogen process only run when changes are detected on dependencies? I
>assume this must be the case otherwise everything would get rebuilt, right?)
>
>Is there a debug flag that can be turned on to see why build might choose to
>rebuild something? The normal build report and debug flags don't see to
>provide the information for "why" something is being rebuilt.
>
>Thanks,
>
>Eugene
>
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>https://lists.01.org/mailman/listinfo/edk2-devel<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] Debugging why Build Rebuilds Something

[Cohen, Eugene]

Great - I will try this now, thanks Liming!

From: Gao, Liming 
Sent: Thursday, March 21, 2019 4:02 AM
To: Cohen, Eugene ; edk2-devel@lists.01.org
Subject: RE: Debugging why Build Rebuilds Something

Could you help check the timestamp of AutoGen files in OpenSslLib output 
directory?

After the first build, copy OpenSslLib output directory to another directory
After the second build, compare the output directory between two builds, please 
check whether there is the difference for AutoGen.h and Makefile. If no 
difference, please directly trig Makefile to see whether rebuild happen. If 
rebuild happen, it may be the issue in Makefile. Then, further check Makefile.

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Cohen, Eugene
>Sent: Thursday, March 21, 2019 5:50 PM
>To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>Subject: [edk2] Debugging why Build Rebuilds Something
>
>I'm experiencing an annoying problem where OpenSslLib is constantly being
>rebuilt. I don't think I've done anything unusual or different to it but
>nevertheless it gets built almost every time I rebuild the same platform.
>
>I don't believe any source file timestamps are changing so I think this may be
>the build.py tool deciding to re-generate stuff. (As a side note: does the
>Autogen process only run when changes are detected on dependencies? I
>assume this must be the case otherwise everything would get rebuilt, right?)
>
>Is there a debug flag that can be turned on to see why build might choose to
>rebuild something? The normal build report and debug flags don't see to
>provide the information for "why" something is being rebuilt.
>
>Thanks,
>
>Eugene
>
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>https://lists.01.org/mailman/listinfo/edk2-devel<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

[edk2] Debugging why Build Rebuilds Something

[Cohen, Eugene]

I'm experiencing an annoying problem where OpenSslLib is constantly being 
rebuilt.  I don't think I've done anything unusual or different to it but 
nevertheless it gets built almost every time I rebuild the same platform.

I don't believe any source file timestamps are changing so I think this may be 
the build.py tool deciding to re-generate stuff.  (As a side note: does the 
Autogen process only run when changes are detected on dependencies?  I assume 
this must be the case otherwise everything would get rebuilt, right?)

Is there a debug flag that can be turned on to see why build might choose to 
rebuild something?  The normal build report and debug flags don't see to 
provide the information for "why" something is being rebuilt.

Thanks,

Eugene

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

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

[Cohen, Eugene]

Tested-by: Eugene Cohen 

Thanks again.

From: Ashish Singhal 
Sent: Wednesday, March 6, 2019 4:07 PM
To: Cohen, Eugene ; Wu, Hao A ; 
edk2-devel@lists.01.org
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA Support

Hi Eugene,

Thanks for confirming. Can you please validate the v2 patch I sent as well for 
completeness?

Thanks
Ashish

From: Cohen, Eugene mailto:eug...@hp.com>>
Sent: Wednesday, March 6, 2019 4:05 PM
To: Wu, Hao A mailto:hao.a...@intel.com>>; Ashish Singhal 
mailto:ashishsin...@nvidia.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA Support


Ø  I verified the patch on SDHC version 3.00 with 64-bit System Address
Ø  Support. Hope more configurations are available for testing on Eugene's
Ø  side.

This patch works for us.  Tested that the V3 64-bit DMA works and verified that 
addresses above 4GB DMA correctly.

Thanks for putting this together.

Feel free to add my Tested-By.

Eugene

From: Wu, Hao A mailto:hao.a...@intel.com>>
Sent: Tuesday, March 5, 2019 8:01 PM
To: Ashish Singhal mailto:ashishsin...@nvidia.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Cohen, Eugene mailto:eug...@hp.com>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA Support

Hi Ashish,

One thing to confirm, for the updated checks within
SdMmcPciHcDriverBindingStart():

> 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;
> }

When the SDHC with version greater than 4.10, the check is only performed
against the 'SysBus64V4' bit. My understanding of the purpose is that:

1. For SDHC with version 4.00, the support of V3 mode and V4 mode of
64-bit System Address are reflect by bit 'SysBus64V3'. Thus, I can infer
that the possible support case is both or neither.

2. The spec states that SDHC with version greater than 4.10 divides the V3
mode and V4 mode support into 2 bits (SysBus64V3, SysBus64V4) so that the
V3 mode support can be optional.

So based on 1 & 2, we do not even bother to check the 'SysBus64V3' bit
when HC version >= 4.10. Is that right?


I verified the patch on SDHC version 3.00 with 64-bit System Address
Support. Hope more configurations are available for testing on Eugene's
side.


Besides, some minor comments below:

> -Original Message-
> From: Ashish Singhal [mailto:ashishsin...@nvidia.com]
> Sent: Saturday, March 02, 2019 2:30 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Wu, Hao A; eug...@hp.com<mailto:eug...@hp.com>; Ashish Singhal
> Subject: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA
> Support

Please help to add the below Bugzilla tracker for reference:
https://bugzilla.tianocore.org/show_bug.cgi?id=1583<https://bugzilla.tianocore.org/show_bug.cgi?id=1583>

I have updated the above tracker to match the purpose of the proposed
patch.


>
> 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 
> mailto:ashishsin...@nvidia.com>>
> ---
> 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 th

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

[Cohen, Eugene]

Ø  I verified the patch on SDHC version 3.00 with 64-bit System Address

Ø  Support. Hope more configurations are available for testing on Eugene's

Ø  side.

This patch works for us.  Tested that the V3 64-bit DMA works and verified that 
addresses above 4GB DMA correctly.

Thanks for putting this together.

Feel free to add my Tested-By.

Eugene

From: Wu, Hao A 
Sent: Tuesday, March 5, 2019 8:01 PM
To: Ashish Singhal ; edk2-devel@lists.01.org
Cc: Cohen, Eugene 
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA Support

Hi Ashish,

One thing to confirm, for the updated checks within
SdMmcPciHcDriverBindingStart():

> 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;
> }

When the SDHC with version greater than 4.10, the check is only performed
against the 'SysBus64V4' bit. My understanding of the purpose is that:

1. For SDHC with version 4.00, the support of V3 mode and V4 mode of
64-bit System Address are reflect by bit 'SysBus64V3'. Thus, I can infer
that the possible support case is both or neither.

2. The spec states that SDHC with version greater than 4.10 divides the V3
mode and V4 mode support into 2 bits (SysBus64V3, SysBus64V4) so that the
V3 mode support can be optional.

So based on 1 & 2, we do not even bother to check the 'SysBus64V3' bit
when HC version >= 4.10. Is that right?


I verified the patch on SDHC version 3.00 with 64-bit System Address
Support. Hope more configurations are available for testing on Eugene's
side.


Besides, some minor comments below:

> -Original Message-
> From: Ashish Singhal [mailto:ashishsin...@nvidia.com]
> Sent: Saturday, March 02, 2019 2:30 AM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Wu, Hao A; eug...@hp.com<mailto:eug...@hp.com>; Ashish Singhal
> Subject: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Add V3 64b DMA
> Support

Please help to add the below Bugzilla tracker for reference:
https://bugzilla.tianocore.org/show_bug.cgi?id=1583<https://bugzilla.tianocore.org/show_bug.cgi?id=1583>

I have updated the above tracker to match the purpose of the proposed
patch.


>
> 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 
> mailto:ashishsin...@nvidia.com>>
> ---
> 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/SdMmcPciH

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

[Cohen, Eugene]

Ashish,

Thanks - I haven't forgotten.  We are still doing tests with the 32-bit ADMA 
driver and running into issues on our newer platform - once we have those 
resolved we will test the patch.  (We are seeing a strange issue where Read 
Multiple Block works but Write Multiple Block does not.)

Eugene

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

Hi Hao,

I agree that there has been a bug all along which got exposed just now. We 
should submit the patch as proposed by Eugene.

Also, I have submitted the patch for enabling 64b DMA for V3. Please take that 
into consideration once the freeze is over so that we can fix the issue in real 
sense.

Eugene,

Please let me know once you have tried my patch on your board.

Thanks
Ashish

-Original Message-
From: Wu, Hao A mailto:hao.a...@intel.com>>
Sent: Sunday, March 3, 2019 7:39 PM
To: Cohen, Eugene mailto:eug...@hp.com>>; Ashish Singhal 
mailto:ashishsin...@nvidia.com>>; Ard Biesheuvel 
mailto:ard.biesheu...@linaro.org>>
Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, Sangwoo (??? 
SW1Lab.) mailto:sangwoo@hp.com>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Hi Eugene, Ashish and Ard

Sorry for the delayed response, I was out of office in the previous several 
days.

According to the discussion, my understanding is that (quote the comments from
Ard):

> Driver should not set the EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE
> attribute 1. If the device does not support it; 2. If the driver does
> not implement the 64-bit DMA mode that the device does
> support.

Thus, for the current implementation of the SdMmcPciHcDxe driver (including the
V4 ADMA descriptor support from Ashish):

* The driver should set the DUAL_ADDRESS_CYCLE attribute only when 'SysBus64V4'
bit set, because of the statement 2 above.

And for the previous implementation (before the change from Ashish):

* The driver should not set the DUAL_ADDRESS_CYCLE attribute at all, since the
implementation was written to support only the 32b ADMA descriptor.

If this is true, I am fine with your proposed fix.


Eugene,

Could you help to state the reason for the fix a bit more clear in the commit 
log?

Also, I have filed a Bugzilla tracker for this one:
https://bugzilla.tianocore.org/show_bug.cgi?id=1583<https://bugzilla.tianocore.org/show_bug.cgi?id=1583>

Could you help to add this information into the commit log as well? Thanks.

Best Regards,
Hao Wu

> -Original Message-
> From: Ashish Singhal [mailto:ashishsin...@nvidia.com]
> Sent: Friday, March 01, 2019 11:25 PM
> To: Ard Biesheuvel; Cohen, Eugene
> Cc: Wu, Hao A; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, 
> Sangwoo (김상우 SW1Lab.)
> Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3
> 64-bit systems
>
> Acked-by: Ashish Singhal 
> mailto:ashishsin...@nvidia.com>>
>
> -Original Message-
> From: Ard Biesheuvel 
> mailto:ard.biesheu...@linaro.org>>
> Sent: Friday, March 1, 2019 4:39 AM
> To: Cohen, Eugene mailto:eug...@hp.com>>
> Cc: Ashish Singhal mailto:ashishsin...@nvidia.com>>; 
> Wu, Hao A
> mailto:hao.a...@intel.com>>; 
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kim, Sangwoo (김상우
> SW1Lab.) mailto:sangwoo@hp.com>>
> 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

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

[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<https://aka.ms/o0ukef>


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<mailto: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<mailto: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<mailto: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<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>.

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<mailto: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.c

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

[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] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit systems

[Cohen, Eugene]

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 
Sent: Thursday, February 28, 2019 4:58 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,

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<mailto: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<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>.

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<mailto: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<mailto: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,

Ø  Right now, we disable 64b DMA Support in PCI if the controller cannot 
support 64b DMA in V3 as well as V4.  If either of these support 64b DMA, we do 
not disable it. In the code, we set Support64BitDma to TRUE by default and 
change it to FALSE only if any of the controller does not support it in V3 as 
well as V4. If all controllers support it in V3 or V4 we keep 64b DMA support 
enabled.

That is precisely the problem.  An SDHC v3 controller might support 64b DMA in 
V3 but not in V4 mode.  The current code will leave 64b DMA support enabled 
resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738>
 ) which then causes buffers to be allocated that cannot be DMAed.

For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622<https://github.com/tianocore/edk2

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

[Cohen, Eugene]

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<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>.

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

Eugene

From: Ashish Singhal 
Sent: Thursday, February 28, 2019 3:21 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,

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<mailto: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,

Ø  Right now, we disable 64b DMA Support in PCI if the controller cannot 
support 64b DMA in V3 as well as V4.  If either of these support 64b DMA, we do 
not disable it. In the code, we set Support64BitDma to TRUE by default and 
change it to FALSE only if any of the controller does not support it in V3 as 
well as V4. If all controllers support it in V3 or V4 we keep 64b DMA support 
enabled.

That is precisely the problem.  An SDHC v3 controller might support 64b DMA in 
V3 but not in V4 mode.  The current code will leave 64b DMA support enabled 
resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738>
 ) which then causes buffers to be allocated that cannot be DMAed.

For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622>
 and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is 
clear.

So since we do not have V3 64b DMA (96-bit descriptor) support in place we must 
not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this 
check: 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426<https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426>


I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support.

Eugene


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

Eugene,

We do not have support for V4 64b DMA right now but it can be added later if 
needed. I am trying to understand the reason behind changing the check from AND 
to OR. Right now, we disable 64b DMA Support in PCI if the controller cannot 
support 64b DMA in V3 as well as V4. If either of these support 64b DMA, we do 
not disable it. In

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

[Cohen, Eugene]

Ashish,


Ø  Right now, we disable 64b DMA Support in PCI if the controller cannot 
support 64b DMA in V3 as well as V4.  If either of these support 64b DMA, we do 
not disable it. In the code, we set Support64BitDma to TRUE by default and 
change it to FALSE only if any of the controller does not support it in V3 as 
well as V4. If all controllers support it in V3 or V4 we keep 64b DMA support 
enabled.

That is precisely the problem.  An SDHC v3 controller might support 64b DMA in 
V3 but not in V4 mode.  The current code will leave 64b DMA support enabled 
resulting in the issuing of the PCI DUAL_ADDRESS_CYCLE attribute ( see 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c#L738
 ) which then causes buffers to be allocated that cannot be DMAed.

For reference look at this snippet of the NonDiscoverablePciDeviceIo driver: 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c#L622
 and you can see that bounce buffering will only occur if DUAL_ADDRESS_CYCLE is 
clear.

So since we do not have V3 64b DMA (96-bit descriptor) support in place we must 
not allow the DUAL_ADDRESS_CYCLE attribute to be set or we will fail with this 
check: 
https://github.com/tianocore/edk2/blob/ece4c1de3e7b2340d351c2054c79ea689a954ed6/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c#L1426


I've added Ard who updated the driver with DUAL_ADDRESS_CYCLE support.

Eugene


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

Eugene,

We do not have support for V4 64b DMA right now but it can be added later if 
needed. I am trying to understand the reason behind changing the check from AND 
to OR. Right now, we disable 64b DMA Support in PCI if the controller cannot 
support 64b DMA in V3 as well as V4. If either of these support 64b DMA, we do 
not disable it. In the code, we set Support64BitDma to TRUE by default and 
change it to FALSE only if any of the controller does not support it in V3 as 
well as V4. If all controllers support it in V3 or V4 we keep 64b DMA support 
enabled.

  //
  // Enable 64-bit DMA support in the PCI layer if this controller
  // supports it.
  //
  if (Support64BitDma) {
Status = PciIo->Attributes (
  PciIo,
  EfiPciIoAttributeOperationEnable,
  EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE,
  NULL
  );
if (EFI_ERROR (Status)) {
  DEBUG ((DEBUG_WARN, "SdMmcPciHcDriverBindingStart: failed to enable 
64-bit DMA (%r)\n", Status));
}
  }

Thanks
Ashish

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

Ashish,

Ø  With my change, if any of the controller did not support 64b DMA in V3 as 
well as V4 capability, we are not enabling it in PCI layer.

The logic is:

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

which means that for a SDHC v3 controller you have SysBus64V3=1 and 
SysBus64V4=0 the FALSE assignment is never done - this is not correct.  Perhaps 
you intended an OR instead of an AND?  Either way changing this to an || or 
using my patch is the same logical result because a V3 controller will use 
32-bit DMA and V4 controller will use 64-bit DMA (a V4 controller should have 
the V3 bit set).  I really saw no reason to be checking the V3 bit since the 
driver was unprepared to do V3 64-bit DMA operations anyways.

Eugene


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

Hello Eugene,

My patch enabled support for SDHC 4.0 and above in general including support 
for 64b ADMA descriptor. The check for V3 capability for 64b DMA was already 
there and similar check was implemented for V4 capability for 64b DMA. Earlier, 
if any of the V3 controller did not support 64b DMA, we were not enabling it in 
PCI layer. With my change, if any of the controller did not support 64b DMA in 
V3 as well as V4 capability, we are not enabling it in PCI layer.

This check in my opinion is better because we only disable 64b DMA PCI support 
when both V3 and V4 have 

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

[Cohen, Eugene]

Ashish,


Ø  With my change, if any of the controller did not support 64b DMA in V3 as 
well as V4 capability, we are not enabling it in PCI layer.

The logic is:

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

which means that for a SDHC v3 controller you have SysBus64V3=1 and 
SysBus64V4=0 the FALSE assignment is never done - this is not correct.  Perhaps 
you intended an OR instead of an AND?  Either way changing this to an || or 
using my patch is the same logical result because a V3 controller will use 
32-bit DMA and V4 controller will use 64-bit DMA (a V4 controller should have 
the V3 bit set).  I really saw no reason to be checking the V3 bit since the 
driver was unprepared to do V3 64-bit DMA operations anyways.

Eugene


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

Hello Eugene,

My patch enabled support for SDHC 4.0 and above in general including support 
for 64b ADMA descriptor. The check for V3 capability for 64b DMA was already 
there and similar check was implemented for V4 capability for 64b DMA. Earlier, 
if any of the V3 controller did not support 64b DMA, we were not enabling it in 
PCI layer. With my change, if any of the controller did not support 64b DMA in 
V3 as well as V4 capability, we are not enabling it in PCI layer.

This check in my opinion is better because we only disable 64b DMA PCI support 
when both V3 and V4 have it disabled.

Thanks
Ashish

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

Hao,

> I remember the commit b5547b9ce97e80c3127682a2a5d4b9bd14af353e from
> Ashish only handles the controllers with version greater or equal to 4.00.

Right - that commit added support for SDHC 4.0 and above. The original driver 
supported SDHC 3.0 albeit only with SDMA and 32-bit ADMA support.

With that commit two descriptor types are supported the 32-bit ADMA descriptor 
(SD_MMC_HC_ADMA_32_DESC_LINE which is 64-bits in size) and the V4 64-bit ADMA 
descriptor (SD_MMC_HC_ADMA_64_DESC_LINE which is 128-bits in size).

However the commit mistakenly added a check for the V3 capability for 64-bit 
DMA and used it to set the PCI DUAL_ADDRESS_CYCLE attributre which then does 
not the 32-bit compatible bounce buffer mechanism. Later, when we attempt an 
ADMA data transfer we hit an ASSERT because the PCI DMA subsystem is not using 
bounce buffers to provide 32-bit DMA compatible memory. So the patch I 
submitted simply removes the unnecessary check of the V3 64-bit DMA capability 
check so the PCI DUAL_ADDRESS_CYCLE attribute is not set allowing 32-bit DMA to 
succeed on these platforms.

> And the ADMA2 (96-bit Descriptor) mode for V3 controllers is selected
> by setting the 'DMA Select' filed in the Host Control 1 Register to
> 11b. But the currently behavior of the driver is setting the field to
> 10b, which I think will not switch to the ADMA2 (96-bit Descriptor) mode for 
> V3.

Correct, right now for a V3 controller only 32-bit DMA is supported. An 
enhancement for V3 64-bit ADMA would improve performance on controllers that 
support that mode by eliminating the bounce buffer and associated memory 
copies. I think we should file a BZ for SD HCI V3 64-bit ADMA support - if you 
agree I would be happy to do that.

I should point out that we have done extensive testing of this change on our 
host controller.

Thanks,

Eugene

---

From: Wu, Hao A mailto:hao.a...@intel.com>>
Sent: Wednesday, February 27, 2019 8:25 PM
To: Cohen, Eugene mailto:eug...@hp.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ashish Singhal mailto:ashishsin...@nvidia.com>>
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Loop Ashish in.
Some comments below.

> -Original Message-----
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Cohen, Eugene
> Sent: Wednesday, February 27, 2019 6:59 PM
> To: mailto:edk2-devel@lists.01.org; Wu, Hao A
> Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC
> v3 64-bit systems
>
> The SdMmcPciHcDriverBindingStart function was checking two different
> capability bits in determining whether 64-bit DMA modes were
> supported, one mode is defined in the SDHC version
> 3 specification (using 96-bit descriptors) and another is defined in
> the SDHC version 4 specification (using 128-bit descriptors). Since
> the

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

[Cohen, Eugene]

Hao,

> I remember the commit b5547b9ce97e80c3127682a2a5d4b9bd14af353e from Ashish
> only handles the controllers with version greater or equal to 4.00.

Right - that commit added support for SDHC 4.0 and above.  The original driver 
supported SDHC 3.0 albeit only with SDMA and 32-bit ADMA support.

With that commit two descriptor types are supported the 32-bit ADMA descriptor 
(SD_MMC_HC_ADMA_32_DESC_LINE which is 64-bits in size) and the V4 64-bit ADMA 
descriptor (SD_MMC_HC_ADMA_64_DESC_LINE which is 128-bits in size).

However the commit mistakenly added a check for the V3 capability for 64-bit 
DMA and used it to set the PCI DUAL_ADDRESS_CYCLE attributre which then does 
not the 32-bit compatible bounce buffer mechanism.  Later, when we attempt an 
ADMA data transfer we hit an ASSERT because the PCI DMA subsystem is not using 
bounce buffers to provide 32-bit DMA compatible memory.  So the patch I 
submitted simply removes the unnecessary check of the V3 64-bit DMA capability 
check so the PCI DUAL_ADDRESS_CYCLE attribute is not set allowing 32-bit DMA to 
succeed on these platforms.

> And the ADMA2 (96-bit Descriptor) mode for V3 controllers is selected by
> setting the 'DMA Select' filed in the Host Control 1 Register to 11b. But
> the currently behavior of the driver is setting the field to 10b, which I
> think will not switch to the ADMA2 (96-bit Descriptor) mode for V3.

Correct, right now for a V3 controller only 32-bit DMA is supported.  An 
enhancement for V3 64-bit ADMA would improve performance on controllers that 
support that mode by eliminating the bounce buffer and associated memory 
copies.  I think we should file a BZ for SD HCI V3 64-bit ADMA support - if you 
agree I would be happy to do that.

I should point out that we have done extensive testing of this change on our 
host controller.

Thanks,

Eugene

---

From: Wu, Hao A  
Sent: Wednesday, February 27, 2019 8:25 PM
To: Cohen, Eugene ; edk2-devel@lists.01.org
Cc: Ashish Singhal 
Subject: RE: [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC v3 64-bit 
systems

Loop Ashish in.
Some comments below.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Cohen, Eugene
> Sent: Wednesday, February 27, 2019 6:59 PM
> To: mailto:edk2-devel@lists.01.org; Wu, Hao A
> Subject: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: Fix DMA on SDHC
> v3 64-bit systems
> 
> The SdMmcPciHcDriverBindingStart function was checking two
> different capability bits in determining whether 64-bit DMA
> modes were supported, one mode is defined in the SDHC version
> 3 specification (using 96-bit descriptors) and another is
> defined in the SDHC version 4 specification (using 128-bit
> descriptors). Since the currently implementation of 64-bit
> ADMA2 only supports the SDHC version 4 implementation it is
> incorrect to check the V3 64-bit capability bit since this
> will activate V4 ADMA2 on V3 controllers.

I remember the commit b5547b9ce97e80c3127682a2a5d4b9bd14af353e from Ashish
only handles the controllers with version greater or equal to 4.00.

And the ADMA2 (96-bit Descriptor) mode for V3 controllers is selected by
setting the 'DMA Select' filed in the Host Control 1 Register to 11b. But
the currently behavior of the driver is setting the field to 10b, which I
think will not switch to the ADMA2 (96-bit Descriptor) mode for V3.

Maybe there is something I miss here. Could you help to provide some more
detail on the issue you met? Thanks.

Best Regards,
Hao Wu

> 
> Cc: Hao Wu <mailto:hao.a...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eugene Cohen <mailto:eug...@hp.com>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index b474f8d..5bc91c5 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -666,8 +666,7 @@ 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->Capability[Slot].SysBus64V4 == 0) {
> Support64BitDma = FALSE;
> }
> 
> --
> 2.7.4
> ___
> edk2-devel mailing list
> mailto: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

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

[Cohen, Eugene]

The SdMmcPciHcDriverBindingStart function was checking two
different capability bits in determining whether 64-bit DMA
modes were supported, one mode is defined in the SDHC version
3 specification (using 96-bit descriptors) and another is
defined in the SDHC version 4 specification (using 128-bit
descriptors).  Since the currently implementation of 64-bit
ADMA2 only supports the SDHC version 4 implementation it is
incorrect to check the V3 64-bit capability bit since this
will activate V4 ADMA2 on V3 controllers.

Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eugene Cohen 
---
MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index b474f8d..5bc91c5 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -666,8 +666,7 @@ 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->Capability[Slot].SysBus64V4 == 0) {
   Support64BitDma = FALSE;
 }

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

Re: [edk2] [PATCH v2 04/11] MdePkg/Include: Add StandaloneMmServicesTableLib library

[Cohen, Eugene]

I saw this thread earlier this week and wanted to chime in.

> > Also, there are some other pieces missing (which I mentioned in one of
> > the other threads but I suppose you may not have caught up yet):
> > EndOfDxe (as well as some other PI defined events) needs to be
> > signalled to the standalone MM context by some non-MM agent, and I
> > think there are other parts of the traditional SMM IPL that have not
> > been ported to standalone MM yet.

I haven't been following closely the state of StandaloneMmPkg on edk2  - as we 
were ready to sync up some of our earlier MM stuff to edk2 I learned that the 
support in place is only partial as patches have been coming in slowly so we 
chose to implement a version based on the early joint prototype work we did 
("uefiproto" repo).  In this there is a DXE component that produces the SMM 
Communication protocol and also ensures that when key GUIDed events occur in 
DXE that they are forwarded to MM including EndOfDxe.

I don't see a strong argument for not forwarding the event signaling 
information to MM - MM can either use the information or ignore it as it sees 
fit.  I can see scenarios around variable services where knowing what phase of 
boot the normal world is in is necessary.


Eugene

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

Re: [edk2] [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

[Cohen, Eugene]

Mike, Chao, Jiewen


  *   [Chao] Infineon chip mentioned by Mike is an example but its register 
space doesn’t comply to PTP spec
  *   [Mike] My experience is with DTPM and some I2C TPMs at 1.2 level.

We have experience with the TPM 1.2 Infineon I2C device and used a completely 
custom solution.  But I think that may be a 1.2 versus 2.0 difference.  I get 
the impression that TCG cleaned up their act a bit for the 2.0 spec - in fact 
we can see text to this effect in the PTP 1.03 
spec<https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf>
 Seciton 2.3:

The CRB Interface is intended to be physical-bus agnostic, so that it could
be implemented on an LPC or SPI interface, as specified in this specification 
or on
another physical interface not specified.

Reading a bit deeper in the PTP spec it looks like there are two register 
layouts but not driven by the physical bus (LPC, SPI, I2C) but rather the 
access method (FIFO or CRB access mode) - see section 5.3.2 called "Register 
Space Addresses" to see the FIFO and CRB register layouts juxtaposed.

Looking at I2C in the PTP spec I can now see the situation is totally different 
- I2C uses a variation of FIFO mode and has a significantly different layout of 
registers, comparing Table 10 to Table 48 in the PTP spec.  So now I see where 
you're coming from (and why we didn't initially understand the concern).


Given that HP's use case is SPI and SPI is aligned to LPC, we believe going 
forward with the TpmIoLib abstraction is still quite useful.  Whenever somebody 
needs to support I2C TPM2 devices then they will need to author another 
DeviceLib instance since the register layout is different.  (Will there ever be 
a Quark with an I2C TPM2?)

TPM experts, I'd appreciate if you could confirm that my analysis of 
LPC/I2C/SPI and CRB/FIFO is correct.



  *   [Mike] I would recommend that a full implementation of TpmIoLib for a few 
non MMIO TPM devices be completed and pass validation before we consider adding 
new lib class to edk2.  Perhaps using an edk2-staging branch would be a better 
place to start and you can document in the Readme.md the criteria that must be 
met before the new lib class meets the requirements for edk2/master.

This plan is fine by me - our intent in the original Request for Comment emails 
was to understand if a TpmIoLib style abstraction would be acceptable to save 
us the work of going down a path that can't be upstreamed and having to fix it 
later.  I think the answer you are giving is "yes, as long as it really works". 
 We are developing and validating this support right now so as long as the TPM 
stack doesn't change out from under us on edk2/master while we are developing 
it should be straightforward to upstream the portions we can share and the 
results, so long as it is understood that we cannot upstream the full stack due 
to the proprietary HW elements - as such I'm not sure how useful the staging 
branch would be although I don't mind doing it.  (In fact I think branch-based 
pull requests are more reviewable than email patchsets but I think I may be in 
the minority on this mailing list).

Thanks,

Eugene


From: Zhang, Chao B 
Sent: Tuesday, November 13, 2018 6:53 PM
To: Kinney, Michael D ; Cohen, Eugene 
; edk2-devel@lists.01.org; Yao, Jiewen 
Cc: Bin, Sung-Uk (빈성욱) 
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Hi All:
   PTP 1.3 spec already include I2C support. It sees I2C TPM communication into 
3 layers
  Application  Layer  -> Already implemented TCG PEI/TCG DXE
  TCG-I2C   ->  Not implemented by UEFI TCG (Infineon chip 
mentioned by Mike is an example but its register space doesn’t comply to PTP 
spec)
I2C  ->  What TpmIoLib also need to address
   It will be good to have more use cases to see if TpmIoLib is sufficiently 
designed to meet generic TPM devices covered by TCG spec.


From: Kinney, Michael D
Sent: Wednesday, November 14, 2018 8:44 AM
To: Cohen, Eugene mailto:eug...@hp.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen 
mailto:jiewen@intel.com>>; Zhang, Chao B 
mailto:chao.b.zh...@intel.com>>; Kinney, Michael D 
mailto:michael.d.kin...@intel.com>>
Cc: Bin, Sung-Uk (???) mailto:sunguk-...@hp.com>>
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Hi Eugene,

My experience is with DTPM and some I2C TPMs at 1.2 level.

One of the I2C TPMs was significantly different, so the TpmIoLib concept does 
not apply.

QuarkPlatformPkg\Library\Tpm12DeviceLibAtmelI2c

The other could potentially use something like TpmIoLib, but may require some 
different delays and timeout values than DTPM.

QuarkPlatformPkg\Library\Tpm12DeviceLibInfineonI2c

So maybe we offer 2 methods to port a TPM.  Once is TpmIoLib if the register 
layout and required delays are the sa

Re: [edk2] [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

[Cohen, Eugene]

Mike,

There is a prevalence of base address nomenclature in the DTPM library like:

return TisPcRequestUseTpm ((TIS_PC_REGISTERS_PTR) (UINTN) 
PcdGet64 (PcdTpmBaseAddress));

like from Tpm2Tis.c.


Ø  shouldn’t the Address be the relative address from a base?

I think it is, to the extent that the PcdTpmBaseAddress already exists.


Ø  Or are you using the full DTPM MMIO address on purpose and you expect non 
DTPM MMIO implementations to translate from the DTPM MMIO address to the 
equivalent register access in on an alternate bus type?

My thought is that the PcdTpmBaseAddress could be set to zero (or whatever 
non-MMIO offset makes sense).


Ø  Wouldn't it be better to have an abstraction for different TPM registers and 
the DTPM implementation would convert to a full MMIO address in the lib 
implementation?

I've been led to understand that the TPM registers are the same in both cases.  
I haven't verified this myself - but if you have data to the contrary please 
let us know so we stop going down this path.

My main reason for resisting creating a new library at the Tpm2DeviceLib layer 
because the DTPM library contains a lot of logic around how to talk to the TPM 
that extends well beyond the access mechanism that we would not want to 
duplicate to another library instance.  I'm looking inside Tpm2Tis.c and 
Tpm2Ptp.c to get this perspective.

Thanks,

Eugene

From: Kinney, Michael D 
Sent: Tuesday, November 13, 2018 3:58 PM
To: Cohen, Eugene ; edk2-devel@lists.01.org; Yao, Jiewen 
; Zhang, Chao B ; Kinney, Michael 
D 
Cc: Bin, Sung-Uk (빈성욱) 
Subject: RE: [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

Eugene,

It appears you are expecting the Address parameter
to be the full MMIO address for DTPM. If you are
wanting this lib class to abstract the access for
different bus types, shouldn’t the Address be the
relative address from a base?

Or are you using the full DTPM MMIO address on
purpose and you expect non DTPM MMIO implementations
to translate from the DTPM MMIO address to the
equivalent register access in on an alternate bus
type?

Wouldn't it be better to have an abstraction for
different TPM registers and the DTPM implementation
would convert to a full MMIO address in the lib
implementation?

Thanks,

Mike


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org<mailto:boun...@lists.01.org>] On Behalf Of Cohen, Eugene
> Sent: Tuesday, November 13, 2018 2:13 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Yao, Jiewen
> mailto:jiewen@intel.com>>; Zhang, Chao B
> mailto:chao.b.zh...@intel.com>>
> Cc: Bin, Sung-Uk (빈성욱) mailto:sunguk-...@hp.com>>
> Subject: [edk2] [PATCH 3/4] SecurityPkg: add
> TpmIoLibMmio instance
>
> SecurityPkg: add TpmIoLibMmio instance
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Chao Zhang mailto:chao.b.zh...@intel.com>>
> Cc: Jiewen Yao mailto:jiewen@intel.com>>
> Signed-off-by: Eugene Cohen mailto:eug...@hp.com>>
> ---
> SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c |
> 221 
> SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf |
> 41 
> 2 files changed, 262 insertions(+)
>
> diff --git
> a/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
> b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
> new file mode 100644
> index 000..56f3187
> --- /dev/null
> +++ b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
> @@ -0,0 +1,221 @@
> +/** @file
> + This library is to abstract TPM2 register accesses
> so that a common
> + interface can be used for multiple underlying busses
> such as TPM,
> + SPI, or I2C access.
> +
> +Copyright (c) 2018 HP Development Company, L.P.
> +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<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 
> +#include 
> +
> +
> +
> +/**
> + Reads an 8-bit TPM register.
> +
> + Reads the 8-bit TPM register specified by Address.
> The 8-bit read value is
> + returned. This function must guarantee that all TPM
> read and write
> + operations are serialized.
> +
> + If 8-bit TPM register operations are not supported,
> then ASSERT().
> +
> + @param Address The TPM register to read.
> +
> + @return The value read.
> +
> +**/
> +UINT8
> +EFIAPI
> +TpmRead8 (
> + IN UIN

Re: [edk2] [PATCH 1/4] SecurityPkg: enable TPM components to build for ARM and AARCH64

[Cohen, Eugene]

Jiewen,

1 and 2 - okay, no problem, working on it.

3 - we are developing the SPI TPM support as we speak.  Our SPI controller is 
proprietary so there's no value in trying to share that driver.  Our internal 
SPI driver currently does not support the PI spec SPI_IO_PROTOCOL but are 
looking at adopting it - if so then we could contribute the TpmIoLibSpi 
instance which would connect the TPM stack to the SPI_IO_PROTOCOL.  Is this 
what you wanted or is there something more specific you were looking for?

Thanks,

Eugene

> -Original Message-
> From: Yao, Jiewen 
> Sent: Tuesday, November 13, 2018 3:22 PM
> To: Cohen, Eugene ; edk2-devel@lists.01.org; Zhang,
> Chao B 
> Cc: Bin, Sung-Uk (빈성욱) 
> Subject: RE: [PATCH 1/4] SecurityPkg: enable TPM components to build for
> ARM and AARCH64
> 
> HI Eugene
> Thanks to enable SPI TPM chip.
> In general, I am OK on this patch series.
> 
> There are some additional work here.
> 1) Please split this patch to 2. The TpmIoLib is not present in at this point 
> of
> time. We should add it after TpmIoLib instance is added.
> 
> 2) Since this patch series adds the dependency of TpmIoLib, please update
> *all* impacted platform in EDKII repo and EDKII platform repo.
> We need make sure this patch series does not break any existing platform
> build.
> 
> 3) I hope, (if possible) you can provide one *real example* on how to add
> SPI instance, to demonstrate the usage and value of this one more layer
> abstraction.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: Cohen, Eugene [mailto:eug...@hp.com]
> > Sent: Wednesday, November 14, 2018 6:13 AM
> > To: edk2-devel@lists.01.org; Yao, Jiewen ;
> > Zhang, Chao B 
> > Cc: Bin, Sung-Uk (빈성욱) 
> > Subject: [PATCH 1/4] SecurityPkg: enable TPM components to build for
> > ARM and AARCH64
> >
> >  SecurityPkg: enable TPM components to build for ARM and AARCH64
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Cc: Chao Zhang 
> > Cc: Jiewen Yao 
> > Signed-off-by: Eugene Cohen 
> > ---
> >  SecurityPkg/SecurityPkg.dsc | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/SecurityPkg/SecurityPkg.dsc b/SecurityPkg/SecurityPkg.dsc
> > index 68a2953..6fb9ad2 100644
> > --- a/SecurityPkg/SecurityPkg.dsc
> > +++ b/SecurityPkg/SecurityPkg.dsc
> > @@ -53,6 +53,7 @@
> >IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> >OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> >IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
> > +  TpmIoLib|SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf
> >TpmCommLib|SecurityPkg/Library/TpmCommLib/TpmCommLib.inf
> >
> >
> PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSe
> > PlatformSecureLib|cu
> > reLibNull.inf
> >
> >
> TcgPhysicalPresenceLib|SecurityPkg/Library/DxeTcgPhysicalPresenceLib/D
> > TcgPhysicalPresenceLib|xe
> > TcgPhysicalPresenceLib.inf
> > @@ -199,7 +200,7 @@
> >  [Components.IA32, Components.X64, Components.ARM,
> Components.AARCH64]
> >SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> >
> > -[Components.IA32, Components.X64]
> > +[Components.IA32, Components.X64 Components.ARM,
> > Components.AARCH64]
> >  #
> >
> SecurityPkg/UserIdentification/PwdCredentialProviderDxe/PwdCredential
> P
> > r
> > oviderDxe.inf
> >  #
> >
> SecurityPkg/UserIdentification/UsbCredentialProviderDxe/UsbCredentialP
> > ro
> > viderDxe.inf
> >
> >
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> fi
> > gDxe.inf
> > --
> > 2.7.4

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

[edk2] [PATCH 4/4] SecurityPkg: convert Tpm2DeviceLibDTpm to use TpmIoLib

[Cohen, Eugene]

SecurityPkg: convert Tpm2DeviceLibDTpm to use TpmIoLib

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Chao Zhang 
Cc: Jiewen Yao 
Signed-off-by: Eugene Cohen 
---
 SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf   |  2 +-
 SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf |  2 +-
 SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c   | 60 
++--
 SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Tis.c   | 28 -
 4 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf 
b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
index c6d23c9..2da8949 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
@@ -47,7 +47,7 @@
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
-  IoLib
+  TpmIoLib
   TimerLib
   DebugLib
   PcdLib
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf 
b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
index 14e5e2e..4de76a3 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
@@ -43,7 +43,7 @@
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
-  IoLib
+  TpmIoLib
   TimerLib
   DebugLib
   PcdLib
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c 
b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
index ad2f188..6525416 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
@@ -16,7 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -51,7 +51,7 @@ Tpm2IsPtpPresence (
 {
   UINT8 RegRead;
 
-  RegRead = MmioRead8 ((UINTN)Reg);
+  RegRead = TpmRead8 ((UINTN)Reg);
   if (RegRead == 0xFF) {
 //
 // No TPM chip
@@ -84,7 +84,7 @@ PtpCrbWaitRegisterBits (
   UINT32WaitTime;
 
   for (WaitTime = 0; WaitTime < TimeOut; WaitTime += 30){
-RegRead = MmioRead32 ((UINTN)Register);
+RegRead = TpmRead32 ((UINTN)Register);
 if ((RegRead & BitSet) == BitSet && (RegRead & BitClear) == 0) {
   return EFI_SUCCESS;
 }
@@ -114,7 +114,7 @@ PtpCrbRequestUseTpm (
 return EFI_NOT_FOUND;
   }
 
-  MmioWrite32((UINTN)>LocalityControl, 
PTP_CRB_LOCALITY_CONTROL_REQUEST_ACCESS);
+  TpmWrite32((UINTN)>LocalityControl, 
PTP_CRB_LOCALITY_CONTROL_REQUEST_ACCESS);
   Status = PtpCrbWaitRegisterBits (
  >LocalityStatus,
  PTP_CRB_LOCALITY_STATUS_GRANTED,
@@ -180,7 +180,7 @@ PtpCrbTpmCommand (
   // STEP 0:
   // if CapCRbIdelByPass == 0, enforce Idle state before sending command
   //
-  if (PcdGet8(PcdCRBIdleByPass) == 0 && 
(MmioRead32((UINTN)>CrbControlStatus) & 
PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE) == 0){
+  if (PcdGet8(PcdCRBIdleByPass) == 0 && 
(TpmRead32((UINTN)>CrbControlStatus) & 
PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE) == 0){
 Status = PtpCrbWaitRegisterBits (
   >CrbControlStatus,
   PTP_CRB_CONTROL_AREA_STATUS_TPM_IDLE,
@@ -202,7 +202,7 @@ PtpCrbTpmCommand (
   // of 1 by software to Request.cmdReady, as indicated by the Status field
   // being cleared to 0.
   //
-  MmioWrite32((UINTN)>CrbControlRequest, 
PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
+  TpmWrite32((UINTN)>CrbControlRequest, 
PTP_CRB_CONTROL_AREA_REQUEST_COMMAND_READY);
   Status = PtpCrbWaitRegisterBits (
  >CrbControlRequest,
  0,
@@ -231,21 +231,21 @@ PtpCrbTpmCommand (
   // of 1 to Start.
   //
   for (Index = 0; Index < SizeIn; Index++) {
-MmioWrite8 ((UINTN)>CrbDataBuffer[Index], BufferIn[Index]);
+TpmWrite8 ((UINTN)>CrbDataBuffer[Index], BufferIn[Index]);
   }
-  MmioWrite32 ((UINTN)>CrbControlCommandAddressHigh, (UINT32)RShiftU64 
((UINTN)CrbReg->CrbDataBuffer, 32));
-  MmioWrite32 ((UINTN)>CrbControlCommandAddressLow, 
(UINT32)(UINTN)CrbReg->CrbDataBuffer);
-  MmioWrite32 ((UINTN)>CrbControlCommandSize, 
sizeof(CrbReg->CrbDataBuffer));
+  TpmWrite32 ((UINTN)>CrbControlCommandAddressHigh, (UINT32)RShiftU64 
((UINTN)CrbReg->CrbDataBuffer, 32));
+  TpmWrite32 ((UINTN)>CrbControlCommandAddressLow, 
(UINT32)(UINTN)CrbReg->CrbDataBuffer);
+  TpmWrite32 ((UINTN)>CrbControlCommandSize, 
sizeof(CrbReg->CrbDataBuffer));
 
-  MmioWrite64 ((UINTN)>CrbControlResponseAddrss, 
(UINT32)(UINTN)CrbReg->CrbDataBuffer);
-  MmioWrite32 ((UINTN)>CrbControlResponseSize, 
sizeof(CrbReg->CrbDataBuffer));
+  TpmWrite64 ((UINTN)>CrbControlResponseAddrss, 
(UINT32)(UINTN)CrbReg->CrbDataBuffer);
+  TpmWrite32 ((UINTN)>CrbControlResponseSize, 
sizeof(CrbReg->CrbDataBuffer));
 
   //
   // STEP 3:
   // Command Execution occurs after receipt of a 1 to Start and the TPM
   // clearing Start to 0.
   //
-  MmioWrite32((UINTN)>CrbControlStart, PTP_CRB_CONTROL_START);
+  

[edk2] [PATCH 3/4] SecurityPkg: add TpmIoLibMmio instance

[Cohen, Eugene]

SecurityPkg: add TpmIoLibMmio instance

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Chao Zhang 
Cc: Jiewen Yao 
Signed-off-by: Eugene Cohen 
---
 SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c   | 221 
 SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf |  41 
 2 files changed, 262 insertions(+)

diff --git a/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c 
b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
new file mode 100644
index 000..56f3187
--- /dev/null
+++ b/SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.c
@@ -0,0 +1,221 @@
+/** @file
+  This library is to abstract TPM2 register accesses so that a common
+  interface can be used for multiple underlying busses such as TPM,
+  SPI, or I2C access.
+
+Copyright (c) 2018 HP Development Company, L.P.
+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 
+#include 
+
+
+
+/**
+  Reads an 8-bit TPM register.
+
+  Reads the 8-bit TPM register specified by Address. The 8-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 8-bit TPM register operations are not supported, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/
+UINT8
+EFIAPI
+TpmRead8 (
+  IN  UINTN Address
+  )
+{
+  return MmioRead8 (Address);
+}
+
+/**
+  Writes an 8-bit TPM register.
+
+  Writes the 8-bit TPM register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all TPM read
+  and write operations are serialized.
+
+  If 8-bit TPM register operations are not supported, then ASSERT().
+
+  @param  Address The TPM register to write.
+  @param  Value   The value to write to the TPM register.
+
+  @return Value.
+
+**/
+UINT8
+EFIAPI
+TpmWrite8 (
+  IN  UINTN Address,
+  IN  UINT8 Value
+  )
+{
+  return MmioWrite8 (Address, Value);
+}
+
+
+/**
+  Reads a 16-bit TPM register.
+
+  Reads the 16-bit TPM register specified by Address. The 16-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 16-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 16-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/
+UINT16
+EFIAPI
+TpmRead16 (
+  IN  UINTN Address
+  )
+{
+  return MmioRead16 (Address);
+}
+
+
+/**
+  Writes a 16-bit TPM register.
+
+  Writes the 16-bit TPM register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all TPM read
+  and write operations are serialized.
+
+  If 16-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 16-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to write.
+  @param  Value   The value to write to the TPM register.
+
+  @return Value.
+
+**/
+UINT16
+EFIAPI
+TpmWrite16 (
+  IN  UINTN Address,
+  IN  UINT16Value
+  )
+{
+  return MmioWrite16 (Address, Value);
+}
+
+/**
+  Reads a 32-bit TPM register.
+
+  Reads the 32-bit TPM register specified by Address. The 32-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 32-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 32-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/
+UINT32
+EFIAPI
+TpmRead32 (
+  IN  UINTN Address
+  )
+{
+  return MmioRead32 (Address);
+}
+
+/**
+  Writes a 32-bit TPM register.
+
+  Writes the 32-bit TPM register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all TPM read
+  and write operations are serialized.
+
+  If 32-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 32-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to write.
+  @param  Value   The value to write to the TPM register.
+
+  @return Value.
+
+**/
+UINT32
+EFIAPI
+TpmWrite32 (
+  IN  UINTN Address,
+  IN  UINT32Value
+  )
+{
+  return MmioWrite32 (Address, Value);
+}
+
+
+/**
+  Reads a 64-bit TPM register.
+
+  Reads the 64-bit TPM register specified by Address. The 64-bit read value is
+  returned. This 

[edk2] [PATCH 2/4] SecurityPkg: introduce TpmIoLib to abstract TPM IO access

[Cohen, Eugene]

SecurityPkg: introduce TpmIoLib to abstract TPM IO access
 
Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Chao Zhang 
Cc: Jiewen Yao 
Signed-off-by: Eugene Cohen 
---
 SecurityPkg/Include/Library/TpmIoLib.h | 196 
 1 file changed, 196 insertions(+)

diff --git a/SecurityPkg/Include/Library/TpmIoLib.h 
b/SecurityPkg/Include/Library/TpmIoLib.h
new file mode 100644
index 000..aaf0f0c
--- /dev/null
+++ b/SecurityPkg/Include/Library/TpmIoLib.h
@@ -0,0 +1,196 @@
+/** @file
+  This library is to abstract TPM2 register accesses so that a common
+  interface can be used for multiple underlying busses such as TPM,
+  SPI, or I2C access.
+
+Copyright (c) 2018 HP Development Company, L.P.
+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.
+
+**/
+
+#ifndef _TPM_IO_LIB_H_
+#define _TPM_IO_LIB_H_
+
+#include 
+
+
+/**
+  Reads an 8-bit TPM register.
+
+  Reads the 8-bit TPM register specified by Address. The 8-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 8-bit TPM register operations are not supported, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/
+UINT8
+EFIAPI
+TpmRead8 (
+  IN  UINTN Address
+  );
+
+/**
+  Writes an 8-bit TPM register.
+
+  Writes the 8-bit TPM register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all TPM read
+  and write operations are serialized.
+
+  If 8-bit TPM register operations are not supported, then ASSERT().
+
+  @param  Address The TPM register to write.
+  @param  Value   The value to write to the TPM register.
+
+  @return Value.
+
+**/
+UINT8
+EFIAPI
+TpmWrite8 (
+  IN  UINTN Address,
+  IN  UINT8 Value
+  );
+
+/**
+  Reads a 16-bit TPM register.
+
+  Reads the 16-bit TPM register specified by Address. The 16-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 16-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 16-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/
+UINT16
+EFIAPI
+TpmRead16 (
+  IN  UINTN Address
+  );
+
+/**
+  Writes a 16-bit TPM register.
+
+  Writes the 16-bit TPM register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all TPM read
+  and write operations are serialized.
+
+  If 16-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 16-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to write.
+  @param  Value   The value to write to the TPM register.
+
+  @return Value.
+
+**/
+UINT16
+EFIAPI
+TpmWrite16 (
+  IN  UINTN Address,
+  IN  UINT16Value
+  );
+
+/**
+  Reads a 32-bit TPM register.
+
+  Reads the 32-bit TPM register specified by Address. The 32-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 32-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 32-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/
+UINT32
+EFIAPI
+TpmRead32 (
+  IN  UINTN Address
+  );
+
+/**
+  Writes a 32-bit TPM register.
+
+  Writes the 32-bit TPM register specified by Address with the value specified
+  by Value and returns Value. This function must guarantee that all TPM read
+  and write operations are serialized.
+
+  If 32-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 32-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to write.
+  @param  Value   The value to write to the TPM register.
+
+  @return Value.
+
+**/
+UINT32
+EFIAPI
+TpmWrite32 (
+  IN  UINTN Address,
+  IN  UINT32Value
+  );
+
+
+/**
+  Reads a 64-bit TPM register.
+
+  Reads the 64-bit TPM register specified by Address. The 64-bit read value is
+  returned. This function must guarantee that all TPM read and write
+  operations are serialized.
+
+  If 64-bit TPM register operations are not supported, then ASSERT().
+  If Address is not aligned on a 64-bit boundary, then ASSERT().
+
+  @param  Address The TPM register to read.
+
+  @return The value read.
+
+**/

[edk2] [PATCH 1/4] SecurityPkg: enable TPM components to build for ARM and AARCH64

[Cohen, Eugene]

 SecurityPkg: enable TPM components to build for ARM and AARCH64

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Chao Zhang 
Cc: Jiewen Yao 
Signed-off-by: Eugene Cohen 
---
 SecurityPkg/SecurityPkg.dsc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/SecurityPkg/SecurityPkg.dsc b/SecurityPkg/SecurityPkg.dsc
index 68a2953..6fb9ad2 100644
--- a/SecurityPkg/SecurityPkg.dsc
+++ b/SecurityPkg/SecurityPkg.dsc
@@ -53,6 +53,7 @@
   IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
   OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
   IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+  TpmIoLib|SecurityPkg/Library/TpmIoLibMmio/TpmIoLibMmio.inf
   TpmCommLib|SecurityPkg/Library/TpmCommLib/TpmCommLib.inf
   
PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf
   
TcgPhysicalPresenceLib|SecurityPkg/Library/DxeTcgPhysicalPresenceLib/DxeTcgPhysicalPresenceLib.inf
@@ -199,7 +200,7 @@
 [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
   SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
 
-[Components.IA32, Components.X64]
+[Components.IA32, Components.X64 Components.ARM, Components.AARCH64]
 #  
SecurityPkg/UserIdentification/PwdCredentialProviderDxe/PwdCredentialProviderDxe.inf
 #  
SecurityPkg/UserIdentification/UsbCredentialProviderDxe/UsbCredentialProviderDxe.inf
   SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf
-- 
2.7.4

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

Re: [edk2] [RFC] TPM non-MMIO Access

[Cohen, Eugene]

Jiewen,

We don't have a patch yet – we wanted to check with you first before going too 
far.


Ø  Or just a simple replace-all for MmioRead/Write->TpmMmioRead/Write.

Yes, basically – with a library class to support it.

We should not call it "TpmMmioRead8" since on some paths it is not MMIO at all. 
 We should call it TpmRead8 (or something like that) and then the MMIO instance 
of the library class would call Mmio functions.

Thanks,

Eugene

From: Yao, Jiewen 
Sent: Sunday, November 11, 2018 9:33 PM
To: Bin, Sung-Uk (빈성욱) ; Cohen, Eugene ; 
edk2-devel@lists.01.org; Zhang, Chao B 
Cc: Chae, Matthew 
Subject: RE: [RFC] TPM non-MMIO Access

OK. Thanks for the clarification.

If possible, would you please post your patch to somewhere?

Do you request to change any other logic in existing Tpm2DeviceLib ?
Or just a simple replace-all for MmioRead/Write->TpmMmioRead/Write.

Thank you
Yao Jiewen

From: Bin, Sung-Uk (빈성욱) [mailto:sunguk-...@hp.com]
Sent: Monday, November 12, 2018 12:17 PM
To: Yao, Jiewen mailto:jiewen....@intel.com>>; Cohen, 
Eugene mailto:eug...@hp.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zhang, Chao B 
mailto:chao.b.zh...@intel.com>>
Cc: Chae, Matthew mailto:matthew.c...@hp.com>>
Subject: RE: [RFC] TPM non-MMIO Access

Hi Jiewen

What Eugene proposes is not to make those another TPM device library instances.
Instead, we propose new “TpmIoLib” which can handle both MMIO and non-MMIO 
device.

è Currently Tpm12Tis.c, Tpm2Tis.c and Tpm2Ptp.c uses IoLib (CPU IO), and what 
we propose is to replace it with TpmIoLib.

For example, TpmIoLib can provide TpmMmioRead() and Tpm12Tis.c, Tpm2Tis.c and 
Tpm2Ptp.c can use TpmMmioRead() instead of MmioRead().

-  TpmIoLib for PC (default in SecurityPkg)
UINT8 TpmMmioRead8 (UINTN  Address )
{
return MmioRead8(Address);
}


-  TpmIoLib for SPI (vendor creates new instance)
UINT8 TpmMmioRead8 (UINTN  Address )
{
UINT8 Data, SpiCs;
SpiCs = (Address & 0xFF) >> 16;
TpmAddress = Address & 0x;

   /* Vendor specific SPI control for TPM */
…
SpiWrite(SpiCs, TpmAddress);
…
SpiRead(SpiCs, TpmAddress, );
return Data;
}

This proposal came from code maintanance,
when we need to update SecurityPkg, then in this case it’s more easy to update 
than making another Tpm2DeviceLibDTpm instance.

Thanks,
Bin

From: Yao, Jiewen mailto:jiewen....@intel.com>>
Sent: Saturday, November 10, 2018 8:18 AM
To: Cohen, Eugene mailto:eug...@hp.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zhang, Chao B 
mailto:chao.b.zh...@intel.com>>
Cc: Bin, Sung-Uk (빈성욱) mailto:sunguk-...@hp.com>>; Chae, 
Matthew mailto:matthew.c...@hp.com>>
Subject: RE: [RFC] TPM non-MMIO Access

Hi Eugene
The TpmIoLib proposal is similar to the existing TpmDeviceLib.
We have I2C TPM instance as example. You may create Tpm12DeviceLibXXXSpi.

Please let us know if there is any gap to support your non-MMIO device.

Thank you
Yao Jiewen


From: Yao, Jiewen
Sent: Saturday, November 10, 2018 7:12 AM
To: 'Cohen, Eugene' mailto:eug...@hp.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zhang, Chao B 
mailto:chao.b.zh...@intel.com>>
Cc: Bin, Sung-Uk (빈성욱) mailto:sunguk-...@hp.com>>; Chae, 
Matthew mailto:matthew.c...@hp.com>>
Subject: RE: [RFC] TPM non-MMIO Access

Hi Eugene
Complete agree.


1)   please ignore TpmCommLib. It is deprecated. ☺


2)   we did notice there is non-MMIO TPM device before and abstract the 
device access via Tpm2DeviceLib and Tpm12DeviceLib library class. The 
Tpm2DeviceLibDTpm and Tpm12DeviceLibDTpm are the library instance for MMIO TIS 
access.

We do have other instance such as 
QuarkPlatformPkg\Library\Tpm12DeviceLibAtmelI2c or 
QuarkPlatformPkg\Library\Tpm12DeviceLibInfineonI2c.


3)   Tcg2Config is also a TPM device oriented driver. It is optional.
You may want to take a look at QuarkPlatformPkg\Quark.dsc to see different TPM 
device is chosen.

  #
  # TPM 1.2 Hardware.  Options are [NONE, LPC, ATMEL_I2C, INFINEON_I2C]
  #
  DEFINE TPM_12_HARDWARE  = NONE

Thank you
Yao Jiewen

From: Cohen, Eugene [mailto:eug...@hp.com]
Sent: Saturday, November 10, 2018 6:38 AM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Zhang, Chao B 
mailto:chao.b.zh...@intel.com>>; Yao, Jiewen 
mailto:jiewen@intel.com>>
Cc: Bin, Sung-Uk (빈성욱) mailto:sunguk-...@hp.com>>; Chae, 
Matthew mailto:matthew.c...@hp.com>>
Subject: [RFC] TPM non-MMIO Access

Dear SecurityPkg maintainers,

We are trying to use the SecurityPkg TPM support (Tcg2Config, 
Tpm2DeviceLibDTpm, etc) and see that access to the TPM are using the MMIO 
routines such as MmioRead8().

Not all platforms support memory-mapped access to the TPM

[edk2] [RFC] TPM non-MMIO Access

[Cohen, Eugene]

Dear SecurityPkg maintainers,

We are trying to use the SecurityPkg TPM support (Tcg2Config, 
Tpm2DeviceLibDTpm, etc) and see that access to the TPM are using the MMIO 
routines such as MmioRead8().

Not all platforms support memory-mapped access to the TPM so we would like to 
propose that we create a library to abstract the TPM access, called the 
TpmIoLib.  One instance of the library would provide the Mmio (TpmIoLibMmio) 
method but others can use protocols like SPI (TpmIoLibSpi).

Before starting this work I wanted to check if you agree with the approach of 
replacing the Mmio calls with new TpmIoLib ones.  We can upstream the library 
header and mmio library instance (or you could do the work if you think it 
would be easier for you).

I counted up the number of instances of unique MMIO calls in the TPM libraries 
and got

Library/TpmCommLib/TisPc.c:7
Library/Tpm2DeviceLibDTpm/Tpm2Tis.c:13
Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c:29
Library/Tpm12DeviceLibDTpm/Tpm12Tis.c:16

Please let me know if this works for you and how you'd like to structure the 
change.

Thanks,

Eugene




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

Re: [edk2] FDF Specification Updates for PI 1.6 Standalone MM Components

[Cohen, Eugene]

Zhu Yonghong,

I was about to file one and then saw that one already exists:

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

This was created in May 2017.  So I guess my original question still applies: 
is there a plan (namely "when") for when this will get updated? :)

Thanks,

Eugene


From: edk2-devel  On Behalf Of Zhu, Yonghong
Sent: Wednesday, October 17, 2018 7:48 PM
To: Cohen, Eugene ; edk2-devel@lists.01.org; Shaw, Kevin W 

Subject: Re: [edk2] FDF Specification Updates for PI 1.6 Standalone MM 
Components

Hi Eugene,

Could you help to file a bugzilla ? thanks.
Refer to 
https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues<https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues>

Best Regards,
Zhu Yonghong


-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Cohen, 
Eugene
Sent: Thursday, October 18, 2018 1:11 AM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Shaw, Kevin W 
mailto:kevin.w.s...@intel.com>>
Subject: [edk2] FDF Specification Updates for PI 1.6 Standalone MM Components

The FDF spec appears to be missing definitions for the new PI 1.6 component 
types like MM_CORE_STANDALONE and MM_STANDALONE.

Is there a plan to update the spec to reflect these new types?

See 
https://edk2-docs.gitbooks.io/edk-ii-fdf-specification/content/v/release/1.28/3_edk_ii_fdf_file_format/39_[rule]_sections.html#39-rule-sections<https://edk2-docs.gitbooks.io/edk-ii-fdf-specification/content/v/release/1.28/3_edk_ii_fdf_file_format/39_[rule]_sections.html#39-rule-sections>


Thanks,

Eugene

___
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel<https://lists.01.org/mailman/listinfo/edk2-devel>
___
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel<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

[edk2] FDF Specification Updates for PI 1.6 Standalone MM Components

[Cohen, Eugene]

The FDF spec appears to be missing definitions for the new PI 1.6 component 
types like MM_CORE_STANDALONE and MM_STANDALONE.

Is there a plan to update the spec to reflect these new types? 

See 
https://edk2-docs.gitbooks.io/edk-ii-fdf-specification/content/v/release/1.28/3_edk_ii_fdf_file_format/39_[rule]_sections.html#39-rule-sections


Thanks,

Eugene

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

Re: [edk2] Missing Library in StandaloneMmPkg

[Cohen, Eugene]

Supreeth, thanks for the fast response.

I'm struggling with what to do next - it sounds like we have a partial 
StandaloneMmPkg implementation on master.  I'm willing to complete the 
implementation for our needs but would first like to understand what the plan 
is for completing and maintaining the package (and not just on the ARM side).

Thanks,

Eugene

From: Supreeth Venkatesh 
Sent: Monday, October 15, 2018 1:49 PM
To: Cohen, Eugene ; edk2-devel@lists.01.org; Achin Gupta 
; Jiewen Yao ; Sughosh Ganu 

Cc: Dong Wei 
Subject: RE: Missing Library in StandaloneMmPkg

Eugene,

The working StandaloneMm available here:
https://github.com/supven01/edk2<https://github.com/supven01/edk2>
https://github.com/supven01/edk2-platforms<https://github.com/supven01/edk2-platforms>
(Caveat: Working Version as of July 2018, May not be latest)

As you mentioned, the patches were sent in June/July for Review.
I have not received any comments/feedback on those.
As you say, it has either not been reviewed by the maintainers or merged yet.

I am no longer working on StandaloneMm at this point.
However, Achin or Sughosh will be able to point you to the latest code base.

Thanks,
Supreeth

-Original Message-
From: Cohen, Eugene mailto:eug...@hp.com>>
Sent: Monday, October 15, 2018 2:29 PM
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Achin Gupta 
mailto:achin.gu...@arm.com>>; Jiewen Yao 
mailto:jiewen@intel.com>>; Supreeth Venkatesh 
mailto:supreeth.venkat...@arm.com>>; Sughosh Ganu 
mailto:sughosh.g...@arm.com>>
Subject: Missing Library in StandaloneMmPkg

Greetings,

It appears that StandaloneMmPkg/StandaloneMmPkg.dsc contains a reference to 
this library:

ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf

but it does not actually appear in the tree.

The AArch64StandaloneMm branch on edk2-staging is "stale" (not my words but 
what GitHub calls it) and it does not contain this library so I'm led to 
believe that there is some other branch where this is being developed. I also 
see patch submissions from July that are not yet integrated 
(StandaloneMmServicesTableLib in particular).


Can I get a summary of the state of the project, in general and on ARM 
platforms? Is there a repo or branch we should be going to where we can see a 
usable system?

Thanks,

Eugene



IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] Missing Library in StandaloneMmPkg

[Cohen, Eugene]

Greetings,

It appears that StandaloneMmPkg/StandaloneMmPkg.dsc contains a reference to 
this library:

ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuStandaloneMmCoreLib.inf

but it does not actually appear in the tree.

The AArch64StandaloneMm branch on edk2-staging is "stale" (not my words but 
what GitHub calls it) and it does not contain this library so I'm led to 
believe that there is some other branch where this is being developed.  I also 
see patch submissions from July that are not yet integrated 
(StandaloneMmServicesTableLib in particular).


Can I get a summary of the state of the project, in general and on ARM 
platforms?  Is there a repo or branch we should be going to where we can see a 
usable system?

Thanks,

Eugene



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

Re: [edk2] Inquiry regarding early DxeIplPeim loading.

[Cohen, Eugene]

Marvin,

> Though to be honest, I am not sure why these PPIs are exposed by
> DxeIplPeim instead of a dedicated module or maybe even PeiCore itself, if
> it's the de facto standard.

When we were originally facing the issue one of the ideas was in fact to move 
this to PeiCore.  I don't recall why that path wasn't pursued.

> Also, if the change was done to avoid disabling CAR prematurely, why
> wasn't SEC code adapted?

This was an ARM SoC not X86 so the modules involved were different than what 
you're seeing.  I'm trying to remember the specifics but it was long enough ago 
that it escapes me and my time machine is broken.

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

Re: [edk2] Inquiry regarding early DxeIplPeim loading.

[Cohen, Eugene]

The depex change did not pertain to S3 resume without memory initialization 
(obviously that wouldn't work).  The change was whether "permanent memory" was 
installed from a PI architecture perspective.  We had a situation that required 
that the S3 Resume path remain in PEI Cache-as-RAM (actual code in CAR, not 
just data) and this change was necessary to prevent CAR from being turned off 
prematurely on S3 resume.

By the way it was never our goal for DXE IPL to host the S3 resume path - my 
understanding is that this was a matter of convenience back in ancient history 
of S3 development (according to an old thread I dug up from a few years ago).  
If S3 resume were parked somewhere else such that DXE IPL only did the IPL of 
DXE then this issue might go away.  So you're seeing a secondary effect of 
hosting S3 resume in DXE IPL and DXE IPL depex changes to support an unusual 
use case.


> -Original Message-
> From: edk2-devel  On Behalf Of
> Marvin H?user
> Sent: Friday, July 13, 2018 7:26 AM
> To: edk2-devel@lists.01.org
> Cc: eric.d...@intel.com; Zeng, Star 
> Subject: Re: [edk2] Inquiry regarding early DxeIplPeim loading.
> 
> Hey Star,
> 
> Thank you very much for your reply.
> Interesting, that is basically the case I described as "insane" because I did
> not consider any platform to allow S3 resume without memory
> initialization. So, this code definitely makes sense.
> You are right, according to the specification, moving it to the PostMem FV
> should be fine. However that will cost a shadow call and a re-entry for non-
> S3 and an event registration for the S3 boot path.
> As the information whether S3 resume without meminit is intended is
> known at compile-time, what's your opinion on a FeatureFlag PCD which
> chooses between direct calls and the shadow/event system?
> I would prepare a patch as soon as I can properly test its working, if you are
> interested. The changes would be most minimal, I imagine.
> 
> Thanks,
> Marvin.
> 
> > -Original Message-
> > From: Zeng, Star 
> > Sent: Friday, July 13, 2018 11:24 AM
> > To: Marvin H?user ; edk2-
> > de...@lists.01.org
> > Cc: Dong, Eric ; Cohen, Eugene
> ;
> > Gao, Liming ; Zeng, Star 
> > Subject: RE: Inquiry regarding early DxeIplPeim loading.
> >
> > Marvin,
> >
> > You can check SHA-1: ebaafbe62c70309d0ceb44a0c4199093d0a823c4.
> > It is for the case "Allow S3 Resume without having installed permanent
> > memory (via InstallPeiMemory)" (PI Mantis 1532, you can search the
> > sentence in PI spec) requested by HP.
> > Yes before ebaafbe62c70309d0ceb44a0c4199093d0a823c4, DxeIpl.inf had
> > gEfiPeiMemoryDiscoveredPpiGuid DEPEX.
> > For the case you mentioned about MinPlatformPkg, I think you can put
> > the DxeIpl.inf into a Post Memory FV if the platform will publish
> > gEfiPeiMemoryDiscoveredPpiGuid indeed.
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of
> > Marvin H?user
> > Sent: Friday, July 13, 2018 7:19 AM
> > To: edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star 
> > Subject: [edk2] Inquiry regarding early DxeIplPeim loading.
> >
> > Good day developers,
> >
> > While checking out which edk2 modules request being shadowed, I came
> > across DxeIplPeim being one of them, however I am not sure why it was
> > designed this way.
> >
> > If the Boot Mode is != S3, the module will register for shadowing and
> > immediately return during the pre-memory phase
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/D
> xe
> > IplPeim/DxeLoad.c#L92
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/D
> xe
> > IplPeim/DxeLoad.c#L111
> >
> > If the Boot Mode is S3, the module will register a Memory Discovered
> > event to install crucial PPIs...
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/D
> xe
> > IplPeim/DxeLoad.c#L125
> > ... and install the DxeIpl PPI before returning
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/D
> xe
> > IplPeim/DxeLoad.c#L132
> >
> > However, by design, the DxeIpl PPI is not located until the very end
> > of PeiCore, meaning the dispatcher ran out of modules to dispatch
> >
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/P
> ei/
> > PeiMain/PeiMain.c#L467
> > Hence installing the DxeIpl PPI early in the S3 boot path does not
> > seem to have any effect to me, as both paths are left awaiting memory
> > availability

Re: [edk2] [PATCH v4 4/6] ArmPkg/Library/CompilerIntrinsicsLib: Enable VS2017/ARM builds

[Cohen, Eugene]

Pete,

> How about I modify the patch to use "AREA s_"
> instead of "AREA Math" as per the current proposal?

That's how it used to work before the macro was introduced, per commit 
dcb2e4bb61931e2dee1739bb76aba315002f0a82 two years ago.

I personally have no problem going back to the individual AREA s_  
approach now that we have a good reason to do so.

> Also, you'll notice that the current div.asm [1], which is used by RVCT does
> *not* rely on the macro, so, unless this is intentional, there already seems
> to exist inconsistencies with regards to using the RVCT_ASM_EXPORT
> macro to ensure the removal of dead code...

I think this was likely an oversight.

> So, to summarise, I would much prefer if we could keep most of the
> current patch, and simply use the following where needed:
> 
> AREA  s___aeabi_ldivmod, CODE, READONLY, ARM AREA  s___aeabi_llsr,
> CODE, READONLY, ARM AREA  s___aeabi_uldivmod, CODE, READONLY,
> ARM
>

Agreed, this is fine so long as we agree on the definition of "where needed".  
In general I would expect each independent assembly function to have its own 
AREA directive (e.g. math functions).  In some cases there will clearly be a 
collection of dependent functions that would be better served by a single area 
directive (e.g. MMU initialization functions).

Much Thanks!

Eugene

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

Re: [edk2] [PATCH v4 4/6] ArmPkg/Library/CompilerIntrinsicsLib: Enable VS2017/ARM builds

[Cohen, Eugene]

> > Introduce CRT assembly replacements for __rt_sdiv, __rt_udiv,
> > __rt_udiv64, __rt_sdiv64, __rt_srsh (by reusing the RVCT code) as well
> > as memcpy and memset.
> > For MSFT compatibility, some of the code needs to be explicitly forced
> > to ARM, and the /oldit assembly flag needs to be added.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Pete Batard 
> 
> This looks fine to me but I haven't been able to test it.
> 
> Reviewed-by: Ard Biesheuvel 
> 
> Eugene: any comments regarding the changes to RVCT code?

I've been away in Linux land for a while, it's nice that the day I catch up on 
my edk2-devel backlog I see my name! :)

One concern for RVCT is that every function is in its own section to enable 
proper dead code removal.  This is addressed with this macro:

  MACRO
  RVCT_ASM_EXPORT $func
EXPORT  $func
AREA s_$func, CODE, READONLY
$func
  MEND

note the AREA directive - this makes a section per function.

Pete's patchset removes these macros which breaks dead code removal.

Please restore these macros for at least RVCT and please verify that the VS2017 
path does appropriate dead code removal for these assembly functions.

Thanks!

Eugene

> 
> 
> > ---
> >  ArmPkg/Library/CompilerIntrinsicsLib/Arm/div.asm   | 43
> +---
> >  ArmPkg/Library/CompilerIntrinsicsLib/Arm/ldivmod.asm   | 40
> +-
> >  ArmPkg/Library/CompilerIntrinsicsLib/Arm/llsr.asm  | 22 
> > +
> -
> >  ArmPkg/Library/CompilerIntrinsicsLib/Arm/uldiv.asm | 29
> +++--
> >  ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf | 16
> +++-
> >  ArmPkg/Library/CompilerIntrinsicsLib/memcpy_ms.c   | 34
> 
> >  ArmPkg/Library/CompilerIntrinsicsLib/memset_ms.c   | 33
> +++
> >  7 files changed, 185 insertions(+), 32 deletions(-)
> >
> > diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/div.asm
> > b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/div.asm
> > index b539e516892d..f9e0107395f2 100644
> > --- a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/div.asm
> > +++ b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/div.asm
> > @@ -1,6 +1,7 @@
> >
> > //
> > --
> >  //
> >  // Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> > +// Copyright (c) 2018, Pete Batard. All rights reserved.
> >  //
> >  // This program and the accompanying materials  // are licensed and
> > made available under the terms and conditions of the BSD License @@
> > -17,18 +18,19 @@
> >  EXPORT  __aeabi_uidivmod
> >  EXPORT  __aeabi_idiv
> >  EXPORT  __aeabi_idivmod
> > +EXPORT  __rt_udiv
> > +EXPORT  __rt_sdiv
> >
> >  AREA  Math, CODE, READONLY
> >
> >  ;
> >  ;UINT32
> >  ;EFIAPI
> > -;__aeabi_uidivmode (
> > -;  IN UINT32  Dividen
> > +;__aeabi_uidivmod (
> > +;  IN UINT32  Dividend
> >  ;  IN UINT32  Divisor
> >  ;  );
> >  ;
> > -
> >  __aeabi_uidiv
> >  __aeabi_uidivmod
> >  RSBSr12, r1, r0, LSR #4
> > @@ -40,10 +42,40 @@ __aeabi_uidivmod
> >  B   __arm_div_large
> >
> >  ;
> > +;UINT64
> > +;EFIAPI
> > +;__rt_udiv (
> > +;  IN UINT32  Divisor,
> > +;  IN UINT32  Dividend
> > +;  );
> > +;
> > +__rt_udiv
> > +; Swap R0 and R1
> > +MOV r12, r0
> > +MOV r0, r1
> > +MOV r1, r12
> > +B   __aeabi_uidivmod
> > +
> > +;
> > +;UINT64
> > +;EFIAPI
> > +;__rt_sdiv (
> > +;  IN INT32  Divisor,
> > +;  IN INT32  Dividend
> > +;  );
> > +;
> > +__rt_sdiv
> > +; Swap R0 and R1
> > +MOV r12, r0
> > +MOV r0, r1
> > +MOV r1, r12
> > +B   __aeabi_idivmod
> > +
> > +;
> >  ;INT32
> >  ;EFIAPI
> > -;__aeabi_idivmode (
> > -;  IN INT32  Dividen
> > +;__aeabi_idivmod (
> > +;  IN INT32  Dividend
> >  ;  IN INT32  Divisor
> >  ;  );
> >  ;
> > @@ -152,4 +184,3 @@ __aeabi_idiv0
> >  BX  r14
> >
> >  END
> > -
> > diff --git a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/ldivmod.asm
> > b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/ldivmod.asm
> > index c71bd59e4520..3794cac35eed 100644
> > --- a/ArmPkg/Library/CompilerIntrinsicsLib/Arm/ldivmod.asm
> > +++ b/ArmPkg/Library/CompilerIntrinsicsLib/Arm/ldivmod.asm
> > @@ -1,6 +1,7 @@
> >
> > //
> > --
> >  //
> >  // Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> > +// Copyright (c) 2018, Pete Batard. All rights reserved.
> >  //
> >  // This program and the accompanying materials  // are licensed and
> > made available under the terms and conditions of the BSD License @@
> > -13,20 +14,41 @@
> >
> > //
> > --
> >
> >
> > -EXTERN  __aeabi_uldivmod
> > +IMPORT  __aeabi_uldivmod
> > +EXPORT  __aeabi_ldivmod
> > +EXPORT  __rt_sdiv64
> >

[edk2] Terminal Binding to Serial IO Protocol

[Cohen, Eugene]

Hi edk2 community, it's been a while.

We have a situation where we need to use instances of the SERIAL_IO_PROTOCOL to 
talk to hardware devices that are not part of a console.  As written today 
TerminalDxe's driver binding will consume any handle that implements the 
SERIAL_IO_PROTOCOL (like in a BDS connect-all case).  Clearly this is 
problematic when the device on the other side is not a terminal.  

The UEFI Specification does not limit the Serial IO protocol to 
console/terminal only applications yet the edk2 implementation of TerminalDxe 
effectively does this.

I'm looking for some advice on how to resolve this.

I think it's important for the BDS connect-all case to keep working so I don't 
think customizing the driver connect process at a BDS level is a good solution.

The UEFI Driver Binding architecture has a capable precedence system for 
determining which drivers get to control which devices (binding versioning, bus 
overrides, platform overrides) but I don't think this a proper use of the 
system since I understand that this system was intended for choosing the better 
of two possible drivers for the hardware (like a motherboard versus option rom 
for the same PCI network adapter).  In this case having TerminalDxe trying to 
connect to a non-terminal device isn't slightly worse, it's broken.  

We need a way to differentiate serial port types in the connect process.  Since 
the Serial IO protocol can't be used to detect what's on the other side we need 
to get the information from the platform with it having a list like:

Serial 0: console
Serial 1: fax
Serial 2: keyboard
Serial 3: mouse
Serial 4: console

so we would only want TerminalDxe to connect to instance 0 and 4 and leave the 
others alone.

Any ideas on how to best do this?

Thanks,

Eugene


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

[edk2] .clang-format for edk2

[Cohen, Eugene]

It looks like the ".clang-format" syntax it being adopted by the majority of 
editors (including Visual Studio and Visual Studio Code).

Do you know of any effort to map the edk2 coding convention into a 
.clang-format file that can be committed to edk2 eventually?

Thanks,

Eugene

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

Re: [edk2] ArmCpuLib users

[Cohen, Eugene]

> Eugene, what is your take on this?

I just checked and we don't use it anywhere.

Thanks!

Eugene


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

Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Mike,

> I agree that accessing DXE protocols in an SMI handler is not allowed.
>
> It is legal for an SMM Driver to access DXE content in the SMM Driver Entry 
> Point.

To digress from the original thread a bit..

There's legal from a PI perspective but for the situations that warrant 
stricter security where this would not be (execution of non-SMM code inside 
SMM).  I think it would be useful to come up with terminology so we know what 
model we're talking about.  I can envision four different SMM models:

1. Framework 0.9 SMM - dual DXE/SMM drivers
2. PI SMM (pre-1.5) - IPL from DXE, SMM drivers use Boot Services at init
3. PI Standalone SMM (1.5) - IPL from SEC or PEI, SMM drivers may use Boot 
Services when they become available
4. PI Strict Standalone SMM (1.5) - IPL from SEC or PEI, SMM drivers never use 
Boot Services

So for the statement I made I'm referring to the "Strict Standalone" - as you 
can probably tell that is what I'm targeting right now.

> If you are providing an abstraction for policy data, would a PCD be a better 
> way 
> to store/access that information that already works for all phases?

The policy data isn't static on some platform so the protocol provides a good 
way to evaluate the conditions at runtime.  I'm sure a dynamic PCD could be 
used to accomplish this (although with the strict standalone model this would 
require more infrastructure to be developed) but my goal at this point is not 
to review the use of protocols for policies but to provide an example of a use 
case for the ProtocolLib proposal.  This was the first example I came up with 
but I expect there to be more functional cases as well.

Earlier in the thread you mentioned that protocol GUIDs should not be shared 
across DXE and SMM - I didn't want to lose track of that since what I'm 
proposing would directly contradict the proposal, so could you elaborate on 
what you were referring to with that statement?

Thanks,

Eugene

-Original Message-
From: Kinney, Michael D [mailto:michael.d.kin...@intel.com] 
Sent: Monday, October 10, 2016 2:40 PM
To: Cohen, Eugene <eug...@hp.com>; Gao, Liming <liming@intel.com>; Laszlo 
Ersek <ler...@redhat.com>; edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; 
Yao, Jiewen <jiewen@intel.com>; Andrew Fish (af...@apple.com) 
<af...@apple.com>; Kinney, Michael D <michael.d.kin...@intel.com>
Subject: RE: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle 
Services

Eugene,

I agree that accessing DXE protocols in an SMI handler is not allowed.

It is legal for an SMM Driver to access DXE content in the SMM Driver Entry 
Point.

For example, if an SMM Driver depends on PCDs, the PCD values can be read from 
the 
PCD database through the PCD Protocol in the driver entry point.

If you are providing an abstraction for policy data, would a PCD be a better 
way 
to store/access that information that already works for all phases?

Thanks,

Mike

> -Original Message-----
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Cohen, 
> Eugene
> Sent: Monday, October 10, 2016 1:11 PM
> To: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> <liming@intel.com>; Laszlo Ersek <ler...@redhat.com>; 
> edk2-devel@lists.01.org
> <edk2-de...@ml01.01.org>; Yao, Jiewen <jiewen@intel.com>; Andrew Fish
> (af...@apple.com) <af...@apple.com>
> Subject: Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle
> Services
> 
> Mike,
> 
> > Can you provide examples in EDK II today where the same GUID Value
> > and Structure definition
> > are used in both the UEFI Handle Database and the SMM Handle
> > Database.
> 
> The example exists in our internal code right now.  We have two platform 
> families:
> one with SMM and one without.  We have a library, originally developed as a 
> DXE
> library, that use a protocol to determine a secure boot policy setting.  This 
> library
> is linked against our variable driver.  In our non-SMM system the variable 
> driver
> runs as a Runtime DXE component and the policy protocol referenced is 
> published in
> the Boot Services protocol DB.  In our SMM system the variable driver runs in 
> SMM and
> the policy protocol is published in the SMM protocol DB.  The protocol is 
> identical
> and uses the same GUID.  So in this scenario we don't install the protocol
> simultaneously in both environments, rather we have different platforms where 
> the
> protocol resides on one side or the other.  Since this protocol is really 
> simple
> (it's not using events, TPL or depending on UEFI boot services stuff) it 
> works well
> for this model.
> 
> > I am aware of cases where an SMM Driver looks for protocols in the
> > DXE Handle database,

Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Mike,
 
> Can you provide examples in EDK II today where the same GUID Value
> and Structure definition
> are used in both the UEFI Handle Database and the SMM Handle
> Database.

The example exists in our internal code right now.  We have two platform 
families: one with SMM and one without.  We have a library, originally 
developed as a DXE library, that use a protocol to determine a secure boot 
policy setting.  This library is linked against our variable driver.  In our 
non-SMM system the variable driver runs as a Runtime DXE component and the 
policy protocol referenced is published in the Boot Services protocol DB.  In 
our SMM system the variable driver runs in SMM and the policy protocol is 
published in the SMM protocol DB.  The protocol is identical and uses the same 
GUID.  So in this scenario we don't install the protocol simultaneously in both 
environments, rather we have different platforms where the protocol resides on 
one side or the other.  Since this protocol is really simple (it's not using 
events, TPL or depending on UEFI boot services stuff) it works well for this 
model.
 
> I am aware of cases where an SMM Driver looks for protocols in the
> DXE Handle database,
> but I don't think your proposed lib would cover that case.

Correct - in our usage we are trying to discourage the cross-pollination of SMM 
and DXE in this way since security minded people get nervous when SMM executes 
outside of the secure sandbox.

Thanks,

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

Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Mike,

> The GUID values for PPIs, DXE Protocols, UEFI Protocols,
> and SMM Protocols are unique.  Which means if code is written
> to work in one phase, you can not share code to another
> phase because the GUID values must be changed.

My original use case was a protocol definition where both the protocol 
structure and GUID value are shared across DXE and SMM.  I was not aware of the 
"GUIDs must be unique" requirement - can you elaborate on this?

> The other libs you mentioned have the attribute that the
> parameters to the library APIs do not have to be updated as
> source code is moved or shared between phase types.

This API usage would have to be consistent across phases as well for this 
proposal to be of value.  I agree - if the users of the library have to change 
the way they call then the library is of little (or maybe even negative) value.

> Given that the source can not be shared, what is gained by
> adding a library?

The use case is definitely to share the source.

In our envisioned use case we would have these two stacks:

DXE Driver
Library X that uses a "protocol"
ProtocolLib (DXE instance)

and

SMM Driver
Library X that uses a "protocol"
ProtocolLib (SMM instance)

so the value is being able to reuse Library X since all it depends on is a 
common protocol.  The protocol would need to have absolutely identical usage 
(and in our use case this is true).

> Would you recommend using this lib in all module types?

I was originally envisioning only DXE and SMM Drivers (and Cores) only.  Jiewen 
suggested PEI which I had not considered which could theoretically be supported 
so long as a common "protocol" definition was usable across PEI and DXE/SMM 
which is a situation I have not yet had a need to explore.  (I think the 
semantics of the PEI no-writeable-globals due to Flash XIP drives differences 
in design that may make this impractical but I'm not sure.)
 
> Maybe you can share both the proposed library class APIs and
> typical usage from different module types.

Yes, I think I need to make it a little more real at this point.  Action Item 
Taken.

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

Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Liming,
>  Could this library cover PEI PPI?

Yes - excellent suggestion for PEI - I hadn't considered it because the use 
case hadn't come up for me yet.

> And, I think this library class will include Install, Notify and Locate APIs. 
> Right?

Yes, this library class proposal incorporates all of the protocol and handle 
database related functionality.  Notify may be tricky since in DXE we have TPL 
and both PEI and SMM we have direct function callbacks but perhaps there's a 
way to abstract if we assume a default TPL or an alternate library API to 
initialize the callback TPL in advance.  (On the other hand this could 
introduce subtle bugs if driver developers lose sight of the TPL used for 
callbacks on this interface.)

Given the positive feedback the next step is to publish a proposed library 
header file for review.

Thanks,

Eugene

From: Gao, Liming [mailto:liming@intel.com]
Sent: Saturday, October 08, 2016 7:50 PM
To: Cohen, Eugene <eug...@hp.com>; Laszlo Ersek <ler...@redhat.com>; 
edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Kinney, Michael D 
<michael.d.kin...@intel.com>; Yao, Jiewen <jiewen@intel.com>; Andrew Fish 
(af...@apple.com) <af...@apple.com>
Subject: RE: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle 
Services

Eugene:
 Could this library cover PEI PPI?  LocatePpi and LocateProtocol both return 
VOID**. And, I think this library class will include Install, Notify and Locate 
APIs. Right?

typedef
EFI_STATUS
(EFIAPI *EFI_PEI_LOCATE_PPI)(
  IN CONST EFI_PEI_SERVICES**PeiServices,
  IN CONST EFI_GUID*Guid,
  IN UINTN Instance,
  IN OUT   EFI_PEI_PPI_DESCRIPTOR  **PpiDescriptor OPTIONAL,
  IN OUT   VOID**Ppi
  );

typedef
EFI_STATUS
(EFIAPI *EFI_LOCATE_PROTOCOL)(
  IN  EFI_GUID  *Protocol,
  IN  VOID  *Registration, OPTIONAL
  OUT VOID  **Interface
  );

Thanks
Liming
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Cohen, 
Eugene
Sent: Saturday, October 1, 2016 6:05 AM
To: Laszlo Ersek <ler...@redhat.com<mailto:ler...@redhat.com>>; 
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> 
<edk2-de...@ml01.01.org<mailto:edk2-de...@ml01.01.org>>; Kinney, Michael D 
<michael.d.kin...@intel.com<mailto:michael.d.kin...@intel.com>>; Yao, Jiewen 
<jiewen@intel.com<mailto:jiewen@intel.com>>; Andrew Fish 
(af...@apple.com<mailto:af...@apple.com>) 
<af...@apple.com<mailto:af...@apple.com>>
Subject: Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle 
Services

> I believe I understood this. However, in the entry point function of an
> SMM driver, it is permitted to look for, and invoke member functions
> of,
> both SMM and DXE protocols [1]. If the library instance that is
> supposed
> to be linked into SMM drivers is tied to the SMM protocol database
> solely, then it won't be able to serve the use case when an SMM driver
> looks for a DXE protocol in its entry point function.

Agreed - non-standalone SMM drivers (notice the new terminology I'm injecting 
to prepare us for PI 1.5) can peek over at UEFI/DXE. This is not the use case 
I'm trying to enable here (and I would argue as an industry is a practice we 
will try to discourage over time).

> ... Actually, I believe this applies even to MemoryAllocationLib. In an
> SMM driver, the SMM-tailored MemoryAllocationLib instance only
> allocates
> SMRAM, which is mostly fine. However, it is unsuitable for allocating
> (for example) EfiBootServicesData type memory, even though that too
> is
> permitted in the SMM driver's entry point function, I think. For that,
> gBS->AllocatePool() or gBS->AllocatePages() have to be called
> explicitly.

Right - so the common library abstraction allocates from the "native" memory 
pool for the driver type and if you want something else you have to go above 
and beyond. So in the spirit of that precedent I'd propose the same approach 
for a ProtocolLib implementation.

Thanks, great feedback.

Eugene
___
edk2-devel mailing list
edk2-devel@lists.01.org<mailto: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: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

> I believe I understood this. However, in the entry point function of an
> SMM driver, it is permitted to look for, and invoke member functions
> of,
> both SMM and DXE protocols [1]. If the library instance that is
> supposed
> to be linked into SMM drivers is tied to the SMM protocol database
> solely, then it won't be able to serve the use case when an SMM driver
> looks for a DXE protocol in its entry point function.

Agreed - non-standalone SMM drivers (notice the new terminology I'm injecting 
to prepare us for PI 1.5) can peek over at UEFI/DXE.  This is not the use case 
I'm trying to enable here (and I would argue as an industry is a practice we 
will try to discourage over time).

> ... Actually, I believe this applies even to MemoryAllocationLib. In an
> SMM driver, the SMM-tailored MemoryAllocationLib instance only
> allocates
> SMRAM, which is mostly fine. However, it is unsuitable for allocating
> (for example) EfiBootServicesData type memory, even though that too
> is
> permitted in the SMM driver's entry point function, I think. For that,
> gBS->AllocatePool() or gBS->AllocatePages() have to be called
> explicitly.

Right - so the common library abstraction allocates from the "native" memory 
pool for the driver type and if you want something else you have to go above 
and beyond.  So in the spirit of that precedent I'd propose the same approach 
for a ProtocolLib implementation.

Thanks, great feedback.

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

Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Tim,

Agreed - When BaseTools gets the standalone support I expect us to be able to 
differentiate library instances.

I wanted to gather feedback now while we prototype on a branch.

Eugene

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of Tim Lewis
> Sent: Friday, September 30, 2016 10:56 AM
> To: Cohen, Eugene <eug...@hp.com>; Laszlo Ersek
> <ler...@redhat.com>; edk2-devel@lists.01.org  de...@ml01.01.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Yao, Jiewen <jiewen@intel.com>;
> Andrew Fish (af...@apple.com) <af...@apple.com>
> Subject: Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol
> and Handle Services
> 
> Eugene --
> 
> Since the standalone file type isn't yet in the EDK2 code, the build
> system will not be able to make this distinction in the library's INF file.
> 
> Tim
> 
> 
> -Original Message-
> From: Cohen, Eugene [mailto:eug...@hp.com]
> Sent: Friday, September 30, 2016 9:51 AM
> To: Tim Lewis <tim.le...@insyde.com>; Laszlo Ersek
> <ler...@redhat.com>; edk2-devel@lists.01.org  de...@ml01.01.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Yao, Jiewen <jiewen@intel.com>;
> Andrew Fish (af...@apple.com) <af...@apple.com>
> Subject: RE: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol
> and Handle Services
> 
> Tim,
> 
> My focus at the moment is on standalone SMM drivers, but in order to
> support the dual-mode DXE_SMM_DRIVER modules we could have
> another instance that does the InSmm check at runtime.
> 
> Eugene
> 
> > -Original Message-
> > From: Tim Lewis [mailto:tim.le...@insyde.com]
> > Sent: Friday, September 30, 2016 10:41 AM
> > To: Cohen, Eugene <eug...@hp.com>; Laszlo Ersek
> <ler...@redhat.com>;
> > edk2-devel@lists.01.org ; Kinney,
> Michael D
> > <michael.d.kin...@intel.com>; Yao, Jiewen
> <jiewen@intel.com>;
> > Andrew Fish (af...@apple.com) <af...@apple.com>
> > Subject: RE: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol
> > and Handle Services
> >
> > Eugene --
> >
> > Since SMM drivers today are actually DXE drivers during the
> > initialization phase, are you going to (a) have your library check
> > InSmm? or (b) only work with pure SMM stand-alone drivers?
> >
> > Thanks,
> >
> > Tim
> >
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On
> Behalf Of
> > Cohen, Eugene
> > Sent: Friday, September 30, 2016 9:37 AM
> > To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org
> > <edk2-de...@ml01.01.org>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; Yao, Jiewen
> <jiewen@intel.com>;
> > Andrew Fish (af...@apple.com) <af...@apple.com>
> > Subject: Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol
> > and Handle Services
> >
> > Laszlo,
> >
> > > As far as I know:
> > > - the DXE and SMM protocol databases are distinct,
> > > - the same protocol GUID may or may not be installed (on one or
> > more)
> > > handle(s) in either,
> > > - even if a protocol GUID exists uniquely in exactly one of those
> > > databases, the locator function would have to return which
> database
> > > the GUID was found.
> > >
> > > My point is that every wrapper function that returns a protocol
> > > interface (or several protocol interfaces), or handles, each such
> > > return value will likely have to be qualified with the database
> > > where
> > it was found.
> >
> > The intent here is to only search the UEFI DB from a DXE/UEFI driver
> > and the SMM DB from an SMM driver and not to cross between.  So
> which
> > protocol DB is searched is purely a function of the module type (i.e.
> > what instance of the ProtocolLib it was linked against).  This is
> > analogous to what is done with MemoryAllocationLib which either
> > allocates from the UEFI memory pools for UEFI/DXE modules
> > (UefiMemoryAllocationLib instance) or from the SMM memory pools
> for
> > SMM modules (SmmMemoryAllocationLib).
> >
> > Sorry I wasn't more clear initially.
> >
> > Eugene
> > ___
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Tim,

My focus at the moment is on standalone SMM drivers, but in order to support 
the dual-mode DXE_SMM_DRIVER modules we could have another instance that does 
the InSmm check at runtime.

Eugene

> -Original Message-
> From: Tim Lewis [mailto:tim.le...@insyde.com]
> Sent: Friday, September 30, 2016 10:41 AM
> To: Cohen, Eugene <eug...@hp.com>; Laszlo Ersek
> <ler...@redhat.com>; edk2-devel@lists.01.org  de...@ml01.01.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Yao, Jiewen <jiewen@intel.com>;
> Andrew Fish (af...@apple.com) <af...@apple.com>
> Subject: RE: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol
> and Handle Services
> 
> Eugene --
> 
> Since SMM drivers today are actually DXE drivers during the
> initialization phase, are you going to (a) have your library check
> InSmm? or (b) only work with pure SMM stand-alone drivers?
> 
> Thanks,
> 
> Tim
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of Cohen, Eugene
> Sent: Friday, September 30, 2016 9:37 AM
> To: Laszlo Ersek <ler...@redhat.com>; edk2-devel@lists.01.org
> <edk2-de...@ml01.01.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Yao, Jiewen <jiewen@intel.com>;
> Andrew Fish (af...@apple.com) <af...@apple.com>
> Subject: Re: [edk2] RFC: ProtocolLib for cross DXE and SMM Protocol
> and Handle Services
> 
> Laszlo,
> 
> > As far as I know:
> > - the DXE and SMM protocol databases are distinct,
> > - the same protocol GUID may or may not be installed (on one or
> more)
> > handle(s) in either,
> > - even if a protocol GUID exists uniquely in exactly one of those
> > databases, the locator function would have to return which database
> > the GUID was found.
> >
> > My point is that every wrapper function that returns a protocol
> > interface (or several protocol interfaces), or handles, each such
> > return value will likely have to be qualified with the database where
> it was found.
> 
> The intent here is to only search the UEFI DB from a DXE/UEFI driver
> and the SMM DB from an SMM driver and not to cross between.  So
> which protocol DB is searched is purely a function of the module type
> (i.e. what instance of the ProtocolLib it was linked against).  This is
> analogous to what is done with MemoryAllocationLib which either
> allocates from the UEFI memory pools for UEFI/DXE modules
> (UefiMemoryAllocationLib instance) or from the SMM memory pools
> for SMM modules (SmmMemoryAllocationLib).
> 
> Sorry I wasn't more clear initially.
> 
> Eugene
> ___
> 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: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Laszlo,

> As far as I know:
> - the DXE and SMM protocol databases are distinct,
> - the same protocol GUID may or may not be installed (on one or more)
> handle(s) in either,
> - even if a protocol GUID exists uniquely in exactly one of those databases,
> the locator function would have to return which database the GUID was
> found.
> 
> My point is that every wrapper function that returns a protocol interface (or
> several protocol interfaces), or handles, each such return value will likely
> have to be qualified with the database where it was found.

The intent here is to only search the UEFI DB from a DXE/UEFI driver and the 
SMM DB from an SMM driver and not to cross between.  So which protocol DB is 
searched is purely a function of the module type (i.e. what instance of the 
ProtocolLib it was linked against).  This is analogous to what is done with 
MemoryAllocationLib which either allocates from the UEFI memory pools for 
UEFI/DXE modules (UefiMemoryAllocationLib instance) or from the SMM memory 
pools for SMM modules (SmmMemoryAllocationLib).

Sorry I wasn't more clear initially.

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

[edk2] RFC: ProtocolLib for cross DXE and SMM Protocol and Handle Services

[Cohen, Eugene]

Request for Comments...

Both UEFI/DXE and SMM support the protocol / handle database concept.  Some 
protocol definitions are able used in both environments with different 
implementations behind them.

We'd like to create a library that could be used in either DXE or SMM making 
use of protocol and handle services.  For example, we'd like to be able to do a 
LocateProtocol for a certain protocol and make use of the protocol regardless 
of the environment we're executing in.

In order to create a neutral API for protocol and handle services from either 
environment, I'm proposing that we create a "ProtocolLib" that abstracts 
protocol install, uninstall, HandleProtocol, install notification, LocateHandle 
and LocateProtocol implementations - basically all the protocol and handle 
services common across UEFI Boot Services and SMST.   A DXE instance of 
ProtocolLib would direct functions through the Boot Services table and an SMM 
instance of ProtocolLib would go through the SMST.   (We also would like to 
maintain separation between DXE and SMM in support of the PI 1.5 Standalone SMM 
model which is easy to achieve with separate library instances.)

We have a similar model already with the MemoryAllocationLib so this would 
follow in its footsteps.


Please share your thoughts...

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

Re: [edk2] What is the right way to print a UINTN?

[Cohen, Eugene]

MIke,

> Portable sources that use type UINTN must never use values larger
> than
> 32-bits.  Same for type INTN.  Only use values in signed 32-bit range.

If the value is something like an enumeration or bitmask then I agree, but not 
if it's something numeric that is supposed to scale to larger numbers on 64-bit 
platforms like the Length field of CopyMem.

I like the proposals to use a custom format specifier (although the 
concatenated string with macro will take some getting used to).

Thanks,

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

Re: [edk2] What is the right way to print a UINTN?

[Cohen, Eugene]

> Printing UINTN with %x *or* with %d are equally bugs.
> 
> For X64 / AARCH64 / IA64 builds, they are actual bugs (that happen to
> work most of the time).

Feel free to file a Bugzilla on the extensive usage of this in edk2 [ducking 
and running].  :)
 
> > I'm envisioning having to create a slide in the future for UEFI
> > training about the proper use of UINTNs and describing "If you think
> > it may exceed 2^32-1 then upcast to UINT64, otherwise don't worry
> > about it" and it makes me squirm.
> 
> It makes me squirm too. I think the slide should recommend the
> casting
> that I proposed. ;) "There is no conversion specifier dedicated to
> UINTN; the portable way to print it is to cast it to UINT64, then print
> it with %Lx."

This is reasonable although I expect to get asked why a lot of the other code 
doesn't adhere to this recommendation.

Thanks,

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

Re: [edk2] What is the right way to print a UINTN?

[Cohen, Eugene]

Laszlo,
 
> I print INTN / UINTN values with:
> - casting them unconditionally to INT64 / UINT64,
> - printing the converted values with the matching conversion
> specifiers,
> such as %Ld (for INT64) and %Lu or %Lx (for UINT64).
> 
> This solution requires a bit more typing, and it is a bit pessimistic
> for 32-bit builds. On the positive side, it is robust / portable, and
> completely valid C.
> 
> It is inspired by the standard C types intmax_t / uintmax_t. If you
> write portable C code and want to print a value of some integer type,
> where the spec only states "signed" or "unsigned integer type", but
> the
> actual type is either implementation defined or unspecified,
> converting
> the value to intmax_t / uintmax_t, and then printing it with %jd vs. %ju
> / %jx, is safe.

Thanks - this makes sense.  If this methodology is consistent with standard C 
then perhaps it's the best compromise even if it's messy to read.

>From a consistency perspective I see a lot of variation in usage - often 
>UINTNs are printed with %x / %d (technically it should be %u but this is a 
>common error - just compare the number of occurrences of %u in MdeModulePkg 
>versus %d).  This means that the caller is expecting that the value will never 
>exceed 2^32-1 on 64-bit systems since we are doing 64-bit to 32-bit truncation 
>through the cast in the VA_ARG macro.  I'm concerned that this requires the 
>developer to know the constraints on the value in all circumstances which 
>seems dubious - after all that's why we have types in the first place, so the 
>tools can help us do the right thing.

I'm envisioning having to create a slide in the future for UEFI training about 
the proper use of UINTNs and describing "If you think it may exceed 2^32-1 then 
upcast to UINT64, otherwise don't worry about it" and it makes me squirm.

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

[edk2] What is the right way to print a UINTN?

[Cohen, Eugene]

Get ready for a potentially stupid question (or at least a question I probably 
should know the answer to by now)...

The implementation of BasePrintLib treats types like %d and %x as the size of 
the compiler's "int" type.  On many 64-bit architectures the size of int is 
32-bits.  On these same architectures we declare a "UINTN" type as 64-bits.

The handling for integral types (%d, %x, %u, etc) in BasePrintLib is as 
follows, note the use of 'int':

  if (BaseListMarker == NULL) {
Value = VA_ARG (VaListMarker, int);
  } else {
Value = BASE_ARG (BaseListMarker, int);
  }

So it would seem to be improper to try to Print/DEBUG a UINTN value with 
%d/%u/%x since the size will be mismatched on some architecture (INTN 64 bits, 
int 32 bits).  But it also would be improper to try to print this as %ld/%lx 
because the size will be mismatched on 32-bit architectures (INTN 32 bits and 
print will use a INT64 with the 'l' prefix).  It's not obvious then how to 
create a portable format specifier that works for UINTN.

I did some research in how this is handled in edk2 for modules we know to be 
portable and I see multiple conflicting techniques being used.  The predominant 
pattern for this is to try to print a UINTN parameter with the %d/%x format 
specifier anyways like this:

  DEBUG ((EFI_D_ERROR, "FATAL ERROR - RaiseTpl with OldTpl(0x%x) > 
NewTpl(0x%x)\n", OldTpl, NewTpl));

in this case the parameter if EFI_TPL which was typedef'ed as a UINTN.  In this 
case the compiler does the variadic thing and on a 64-bit architecture puts a 
64-bit UINTN on the variadic stack (using the term stack in the abstract sense 
here).

When the print code tries to pop it off the stack using VA_ARG (VaListMarker, 
int) the 'int' gets upgraded in size to UINTN due to this construct in Base.h 
(I'm using GCC):

  #define VA_ARG(Marker, TYPE) ((sizeof (TYPE) < sizeof (UINTN)) ? 
(TYPE)(__builtin_va_arg (Marker, UINTN)) : (TYPE)(__builtin_va_arg (Marker, 
TYPE)))

since sizeof(int) is 4 and this is less than sizeof(UINTN) at 8 the result will 
be  (TYPE)(__builtin_va_arg (Marker, UINTN)) where TYPE is 'int'.  So we end up 
popping 64-bits off the varidic stack but immediately typecast it to an int (4 
byte) resulting in loss of the upper-64 bits.  If I'm reading this right it 
means we can only print UINTNs whose value is below 2^32 (confirmed with a 
debugger on AArch64 using GCC).  Maybe this seems benign for the simple EFI_TPL 
enumeration but if for some reason you add a new value that exceeds 2^32-1 then 
the print code is broken.  Is every user of Print/DEBUG expected understand 
this range limitation?


What is the preferred pattern for printing a INTN/UINTN then?  Two yucky hacks 
I can think of: upcasting to UINT64/%ld when you "know" the value may exceed 
2^32-1 or using %p for non-pointer types since this happens to get the right 
size treatment although we lose formatting options.

Eugene



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

Re: [edk2] [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Cohen, Eugene]

> I think this is the right thing to do; Arguably, on the modern ARM
> architectures, UNCACHEABLE and WRITE_COMBINEABLE are mutually
> exclusive. I'll discuss with Charles whether we should codify this in
> the UEFI specification.

Given the corresponding X86 semantics it makes sense for UNCACHEABLE to map to 
Strongly Ordered and WRITE_COMBINEABLE to map to "Normal" Uncacheable.   It's 
useful to expose this separately in case a DMA common buffer has semantics that 
require the strongly ordered behavior.

Since this is providing a list of capabilities I'm not sure what the statement 
about mutual exclusivity refers to.

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

Re: [edk2] [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Cohen, Eugene]

Ard,

> So remove the EFI_MEMORY_UC attribute that we set by default on
> system RAM.
> If any region requires this attribute, it is up to the driver to set this
> attribute, and to ensure that no offending operations are performed
> on it.
>

For DMA common-buffer operations on systems without snooping DMA capabilities, 
UC or WC mapping of system memory regions is required.

>EFI_RESOURCE_ATTRIBUTE_PRESENT |
>EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> -  EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |

The EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE bit your removing is removing the 
capability for uncacheable memory such that even if a driver wanted to make a 
DMA buffer uncacheable GCD will no longer allow this because the resource does 
not support this capability.

Is it your intent to indicate that system memory is no longer capable of being 
uncacheable?  If so how would you plan to accomodate the DMA use case for GCD 
SetMemorySpaceAttributes?

Thanks,

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

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

Ting, sounds good to me - the discussion here seems to be growing increasingly 
unproductive.

I'll tee this up with UNST.

Eugene

> -Original Message-
> From: Ye, Ting [mailto:ting...@intel.com]
> Sent: Wednesday, August 17, 2016 9:11 PM
> To: Wu, Jiaxin <jiaxin...@intel.com>; Cohen, Eugene
> <eug...@hp.com>; edk2-devel@lists.01.org; Kinney, Michael D
> <michael.d.kin...@intel.com>; Fu, Siyuan <siyuan...@intel.com>
> Subject: RE: DHCP Automatic Configure at Driver Connect
> 
> Hi Eugene,
> 
> If you propose separating DHCP process from DHCP policy setting, I
> think it is more like to update/clarify existing behavior defined in
> IP4Config2/IP6Config of UEFI specification. How about we move the
> discussion to UNST forum, in order to draw more attention for the
> topic?
> 
> Thanks,
> Ting
> -Original Message-
> From: Wu, Jiaxin
> Sent: Thursday, August 18, 2016 10:49 AM
> To: Cohen, Eugene <eug...@hp.com>; Ye, Ting <ting...@intel.com>;
> edk2-devel@lists.01.org; Kinney, Michael D
> <michael.d.kin...@intel.com>; Fu, Siyuan <siyuan...@intel.com>
> Subject: RE: DHCP Automatic Configure at Driver Connect
> 
> Eugene,
> 
> > > Ip4Config2OnDhcp4SbInstalled() maybe confused you, but it's not
> > > accurate as you said "when the DHCP SB is installed you do a
> > > configure
> > automatically".
> > > It's more proper to say "IPv4 driver is requiring DHCP SB due to it
> > > detects the DHCP policy." In such a case, it's reasonable with the
> > > function Ip4Config2OnDhcp4SbInstalled().
> >
> > I see - my observation was based on the fact that removing this
> > prevented the undesirable automatic configuration, but what you
> say makes sense.
> >
> > > I agree with you this code did not exist in *Ip4Dxe* before. We
> > > implemented this function because of we need start the
> AutoConfig
> > > due to the DHCP policy is detected by Ip4Dxe driver (DHCP policy
> (note:
> > > NOT DHCP SB) together with D.O.R.A process). It does may appear
> > > AutoConfig straight away with DHCP SB if Ip4Dxe ahead of
> Dhcp4Dxe.
> > > But the precondition is that the DHCP policy has been detected.  If
> > > the policy is not DHCP during the system boot, I think your concern
> > > will not
> > appear.
> >
> > Are you saying the IP4 interface will not come "up" even for a static
> IP policy?
> 
> If the policy is static and the default interface has not been
> referenced/used, we can say it's not come up.  Once you get involved
> and do any IP assignment, the default interface is only ready to use,
> we can say it has been configured and it's available now.
> 
> For *Ip4Dxe*, I think no matter what kind of policy behavior will only
> affect the default interface (configured or un-configured), currently :
> 1) If the policy is DHCP, Ip4Dxe will try his best to be in available state.
> 2)  If the policy is static, it will depend on the third part configuration.
> After the default interface is ready and available, the only way I can
> think to make it in un-configured state is leveraging the DXE_DRIVER (I
> mentioned it before) to do some cleanup according the current
> implementation.
> 
> For #1, the DHCP process will be triggered automatically.
> For #2, I agree such a DXE_DRIVER driver will destroy the previous
> configuration data (Actually, the old Ip4Config behavior also did it), but
> if not, how can we reach such a un-configured state since the data
> already saved?
> 
> 
> > I was assuming it would in this case.  I'm not sure how big a deal
> > this is because if there are no listeners or connections initiated
> > it's kind of a don't-care - I think the only difference would be
> whether responses to ARP are provided.
> >
> > > Now, compared to old Ip4Config behavior, we take 'ifconfig' tool
> > > command as example - "ifconfig -s eth0 dhcp":
> > > The Ip4Config->Start will be invoked to start the auto configuration.
> > > It was implemented in the deprecated driver -- Ip4ConfigDxe.
> When
> > > the system boot next time, the previous IP configuration will
> > > cleaned and the interface will be in UN-CONFIG status again.
> > > Current implementation don't have this clean-up operation no
> matter
> > > Static/DHCP policy has been set. Is this the difference you
> mentioned?
> >
> > Okay - this must be it - so DHCP magically turned back into
> "unconfigured"
> > effectively creating the on-demand Configure() effect, at least for
> > DHCP case

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

Jiaxin,

> Ip4Config2OnDhcp4SbInstalled() maybe confused you, but it's not accurate
> as you said "when the DHCP SB is installed you do a configure automatically".
> It's more proper to say "IPv4 driver is requiring DHCP SB due to it detects 
> the
> DHCP policy." In such a case, it's reasonable with the function
> Ip4Config2OnDhcp4SbInstalled().

I see - my observation was based on the fact that removing this prevented the 
undesirable automatic configuration, but what you say makes sense.

> I agree with you this code did not exist in *Ip4Dxe* before. We implemented
> this function because of we need start the AutoConfig due to the DHCP
> policy is detected by Ip4Dxe driver (DHCP policy (note: NOT DHCP SB)
> together with D.O.R.A process). It does may appear AutoConfig straight away
> with DHCP SB if Ip4Dxe ahead of Dhcp4Dxe. But the precondition is that the
> DHCP policy has been detected.  If the policy is not DHCP during the system
> boot, I think your concern will not appear.

Are you saying the IP4 interface will not come "up" even for a static IP 
policy?  I was assuming it would in this case.  I'm not sure how big a deal 
this is because if there are no listeners or connections initiated it's kind of 
a don't-care - I think the only difference would be whether responses to ARP 
are provided.

> Now, compared to old Ip4Config behavior, we take 'ifconfig' tool command
> as example - "ifconfig -s eth0 dhcp":
> The Ip4Config->Start will be invoked to start the auto configuration. It was
> implemented in the deprecated driver -- Ip4ConfigDxe. When the system
> boot next time, the previous IP configuration will cleaned and the interface
> will be in UN-CONFIG status again.  Current implementation don't have this
> clean-up operation no matter Static/DHCP policy has been set. Is this the
> difference you mentioned?

Okay - this must be it - so DHCP magically turned back into "unconfigured" 
effectively creating the on-demand Configure() effect, at least for DHCP cases, 
that we now desire.  Interesting.

> Now, let us  consider your requirement:
> 1) The IP config information stored in NVRAM
> 2) A separate policy to delay the IP interface coming up until a component
> calls Configure ().
>
> #1 is also the current implementation. For 2), I remember you have one
> patch to do this implementation, can you share it to us for better
> understanding?

Yes, my first thought was to make the handling of Ip4Config2OnDhcp4SbInstalled 
conditional based on a PCD value (the policy in this case, would be in the PCD 
value).  Based on the limited experiment I did this seems to have the desired 
effect but based on your expertise perhaps there is a better location for such 
a check?

Thank you for patiently helping me with this - it's been enlightening to learn 
the history and thinking behind the config process.

Eugene




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

Re: [edk2] IP4 Config Troubles with DHCP

[Cohen, Eugene]

Jiaxin,

> Yeah. When I was drafting the UDP APP to test the new fix, I found the
> same case you mentioned. We must issue another UDP Configure () to
> clean the previous state once the  Ip4Mode.IsConfigured is TRUE. So,
> the above example is not accurate with my the current
> implementation:(. But I'm still not recommend to loop the UDP
> configuration every time if Ip4Mode.IsConfigured is false. The right
> behavior for UDP/TCP is 1) timer check the Ip4Mode.IsConfigured, 2)
> Once Ip4Mode.IsConfigured is TRUE, reconfigure the instance again.
> Sorry for the above example was troubling you. Also use UDP as
> example, correct as below:

Can't we just call this a defect and make it so the first Configure() that 
returns IsConfigured=TRUE works?  It seems much safer to handle this in the 
stack than to expect hundreds or thousands of different network applications 
and services to try to implement this sequence correctly.

I don't see where in the UEFI spec it states that you must call Configure(cfg) 
Configure(NULL) Configure(cfg) just to make it work...

Thanks,

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

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

Jiaxin,
 
> I mean I didn't know whether the *un-configured* status you want
> contain *no policy setting * or not. But it doesn't matter now, from
> your statement here, I know you don't care the policy setting.

Correct the static vs dhcp is irrelevant - it's the fact that the IP layer 
comes up that is the issue.

> As I said in previous email, " The current behavior of Ip4Config2: DHCP
> policy together with D.O.R.A process, which is the same case as the old
> Ip4Config behavior ". 

I'm confused by this statement because I can see a distinct difference in 
behavior from the past.  I can see the new code which enables the new behavior 
as well Ip4Config2OnDhcp4SbInstalled- when the DHCP SB is installed you do a 
configure automatically - this code simply did not exist before -- see 
Ip4Config2OnDhcp4SbInstalled.

> From the case you described here, are you want
> to separate the DHCP policy setting and D.O.R.A process? We don't
> know.

Yes, You could say I want to separate the DHCP vs static policy from D.O.R.A. 
but I don't think that's a good way to state it - I would state that we don't 
want the IP protocol (whether it be DHCP or statically configured) to be "up" 
until a piece of UEFI calls Configure().
 
> The provided solution for you (such a DXE Driver) is only based on you
> want an *un-configured* status at each boot time until the third part
> configuration. I think it does an approach for this. But I think Ting is
> right, "we need fully understand your usage case before analyzing the
> problem". Perhaps you have more detailed requirements we don't
> know clearly.

We want the IP config information stored in NVRAM (dhcp or static) to be 
preserved but want a separate policy choice to delay the IP interface coming 
"up" until a component calls Configure().

Eugene




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

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

 
> Hi Eugene,
> 
> I think we need fully understand your usage before analyzing the
> problem. Could you please explain more details? You mentioned you
> only wanted to enable the network interface when directed by an
> application. If so, is it possible that you don't connect your network
> device until you really need that?
> 
> Thanks,
> Ting

Ting,

I agree that if we could suppress driver connection for the network devices 
then that would result in the network being disabled.  Our use case is that we 
still want to connect all drivers but not have the network start until directed 
through a Configure() call.

Thanks,

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

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

Jiaxin,

> > > In my memory, we only talked about the performance issue before.
> > > Here, I understand your requirement: 1) The network interface should
> > > be in
> > > *un-
> > > configured* status at each boot time until the third part
> > > configuration;

Yes, we would like a policy setting (either runtime/protocol or build-time/PCD 
is fine) for this.

 2) the policy setting should be ignored, right? I
> > > don't

I don't know what you mean by this.  The static/dhcp policy shouldn't 
necessarily be ignored but if the interface is not yet configured (i.e. the IP 
layer won't respond to packets) then the static/dhcp policy doesn't really 
matter until some component calls Configure() for the first time.

> This is what I want to confirm with you that my understanding is right? You
> complained the network interface comes "up" at each boot, what does
> "comes up" exactly mean? So, I gave the above two guesses 1) and 2).

And I tried to clarify what I meant.  Another way of saying it is by being "up" 
it means performing DHCP (if that is the setting) and transmitting/receiving 
packets using the IP address.  What we are asking for is a policy where we can 
suppress this behavior until the first Configure() call like before.

> Of course not. I mean if my guessing 2) is correct, it's not reasonable 
> because
> UEFI 2.6 spec only defined Static/DHCP policy. The policy should be in one of
> them. I just recalled the old Ip4Config behavior: no policy assigned default 
> (if
> I remember correctly).  So, don't misunderstand my truly means. It's
> unrelated to the *behavior consistency*.

Perhaps not spec consistency but we need this for implementation consistency 
since we've built up years of expectations from the previous implementation.

> First, *based on the current UEFI Spec (only static / dhcp policy)*, I want to
> highlight my understanding of *un-configured* status you mentioned: policy
> should be always set, *un-configured* means *no IP address configuration*.

Correct, in that the device will neither transmit or respond at an IP layer 
initially.

> If we want to implement such a *un-configured* state, one *DXE_DRIVER*
> can be used with Ip4Config2 protocol. Detailed see below pseudocode:
> 
> First,  register a notify for Ip4Config2 protocol in your Dxe Driver 
> EntryPoint:
> EfiCreateProtocolNotifyEvent (
> ,
> TPL_CALLBACK,
> Ip4Config2InstalledCallback,
> NULL,
> 
> );
> 
> Then, In your Ip4Config2InstalledCallback() function, you can change the
> default policy for the current boot:
>  Ip4Config2InstalledCallback () {
>  gBS->LocateProtocol (
>   ,
>   Registration,
>   (VOID **) 
>   );
> 
>  //
>  // Change the policy to Ip4Config2PolicyDhcp to clean the static 
> setting.
> //
> Policy = Ip4Config2PolicyDhcp;
> Ip4Config2Instance->SetData(
> Ip4Config2Instance,
> 
> Ip4Config2DataTypePolicy,
> sizeof 
> (EFI_IP4_CONFIG2_POLICY),
> 
> );
> 
>  //
>  // Change the policy to Ip4Config2PolicyStatic to clean the DHCP 
> policy
> setting.
> //
> Policy = Ip4Config2PolicyStatic;
> Ip4Config2Instance->SetData(
> Ip4Config2Instance,
> 
> Ip4Config2DataTypePolicy,
> sizeof 
> (EFI_IP4_CONFIG2_POLICY),
> 
> );
> }
> 
> After that, your previous setting will be cleaned, and reach a  *un-
> configured* state. I know the addition Dxe Driver is an indirect way to meet
> your requirement, but based on current UEFI Spec, it's one-time event if you
> added in  your platform ahead.

There's two issues I see with this approach:

1. It destroys any previous configuration data that the user may have made 
(static IP entry or explicit selection of DHCP)
2. It's a hack where we're using one policy interface to try to accomplish 
something unrelated (auto vs on-demand config) based on attributes of the 
current implementation and not the spec

So I'd like to propose again that we define another, separate, policy value for 
automatic versus on-demand IP configuration.  It can be either through a PCD or 
a protocol interface.  My preference in the short term is to do the PCD thing 
since we can do it now where the protocol approach will require spec 
modification.

Let me know if that makes sense - I'm prepared to provide a patch for the PCD 
option if/when you're 

Re: [edk2] IP4 Config Troubles with DHCP

[Cohen, Eugene]

Jiaxin,
 
> Actually, you don't need to retry the UDP configuration loop according
> the Ip4Mode.IsConfigured flag. You are only recommended to set a
> timer to check the mapping status after the configuration:
> 
> For example:
>   Status = Nlc->Udp4->Configure(Nlc->Udp4, >UdpConfig);
>   if (EFI_ERROR (Status) && (Status != EFI_NO_MAPPING)) {
>   return  Status;
>   }
>   if (Status == EFI_NO_MAPPING && !UdpGetMapping (Nlc->Udp4)) {
>   return  Status;
>   }
> 
> In UdpGetMapping () function, create one timer to check
> Ip4Mode.IsConfigured:
> 
> For example:
> UdpGetMapping () {
>   IsMapDone = FALSE;
>   gBS->CreateEvent (EVT_TIMER, TPL_CALLBACK, NULL, NULL,
> );
>   gBS->SetTimer (TimeoutEvent, TimerRelative, AnyValue);
>   while (EFI_ERROR (gBS->CheckEvent (TimeoutEvent))) {
> Udp4->Poll (Udp4);
> Udp4->GetModeData (Udp4, , & Ip4Mode, NULL,
> NULL);
> if (Ip4Mode.IsConfigured) {
>   IsMapDone = TRUE;
>   break;
> }
>   }
>   return IsMapDone;
> }
> 
> If DHCP process succeed, Ip4Mode.IsConfigured should be updated. If
> not, any bug may be existed.

In testing the new patch (removing RECONFIG=TRUE) I see that the statement you 
made above is not accurate when the protocol is TCP.  When Configure is called 
the first time it returns EFI_NO_MAPPING.  This seems to be remembered in the 
socket state: ((Sock)->ConfigureState == SO_NO_MAPPING) so that any attempt to 
use the instance after Ip4Mode.IsConfigured goes TRUE fails (like for a TCP4 
Listen).

So for TCP we must issue another Configure request to clean up this state so 
it's not as simple as just polling the GetModeData result, at least for TCP.

Do you believe this is expected behavior?

Thanks,

Eugene


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

Re: [edk2] IP4 Config Troubles with DHCP

[Cohen, Eugene]

> Eugene,
> 
> I can reproduce the issue now. And root cause as below:
> 
> #1. Set policy to DHCP.
> #2. If DHCP process is not complete yet, then run one App to invoke the
> UDP4 Configure with "UseDefaultAddress = TRUE" (loop to keep calling
> Udp4->Configure until Ip4Mode.IsConfigured changes to TRUE)
> #3. Even DHCP succeed but Ip4Mode.IsConfigured flag never set to TRUE --
> -- failure here!!!
> 
> In step1, the policy will be set to DHCP, and then
> Ip4Config2OnPolicyChanged() will be called. In this function, "IpSb-
> >Reconfig" flag will be set to TRUE before Ip4StartAutoConfig() called. That
> means the original "IpSb->DefaultInterface" will be abandoned/freed once
> this DHCP process finished. Detailed see Ip4Config2SetDefaultAddr()
> function.
> 
> In step2, UDP4 Configure with "UseDefaultAddress = TRUE" is called, that
> means the default interface (IpSb->DefaultInterface) will be selected as
> current instance's interface. Detailed see Ip4ConfigProtocol() function.
> 
> In step3, When DHCP process finished, as I said in step1, the original "IpSb-
> >DefaultInterface" will be abandoned/freed because "IpSb->Reconfig" flag is
> true. Meanwhile, one new interface is assigned to "IpSb->DefaultInterface".
> This "IpSb->DefaultInterface" is different to the original one assigned to the
> UDP4 Configured instance. So, even DHCP process succeed, the up caller will
> never have the chance to get it's truly status.
> 
> I will send one patch to fix this issue later.
> 
> Thanks your reporting.
> 
> Best Regards!
> Jiaxin

Jiaxin, excellent to hear.  I'm glad to hear you've isolated the issue.  So the 
service binding instance we have in this case is orphaned which explains why 
destroying it and creating a new one resolves the issue.  Of course we'd be 
happy to test the patch as soon as it's available.

Thank you!

Eugene

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

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

Jiaxin,

> > The issue is not just a performance issue around DHCP timing.  Even
> > with a static IP address set the fact that the network interface comes
> > "up" at each boot is problematic because our requirement is only to
> > enable the network interface when directed to by an application.
> 
> In my memory, we only talked about the performance issue before. Here, I
> understand your requirement: 1) The network interface should be in *un-
> configured* status at each boot time until the third part configuration; 2) 
> the
> policy setting should be ignored, right? I don't think it's reasonable. I know
> the old Ip4Config did as you said here, but currently, the IPv4 default policy
> concept is new enrolled by Ip4Config2 protocol, the device policy should be
> in one state no matter static or DHCP.

Sorry I'm having trouble understanding what you meant - can you rephrase or 
expand this?  Are you saying that it is not reasonable to want behavior 
consistent with the original Ip4Config behavior?  This seems like something 
that should be doable - in my experiment suppressing the configuration in 
Ip4Config2OnDhcp4SbInstalled does exactly this so it seems like some additional 
policy (automatic versus on-demand) could be defined to handle this.

> >
> > Modifying the IP configuration dynamically to suppress the interface
> > coming up at every boot is also problematic because it means we would
> > need to store the IP address configuration in another NVRAM location.
> > In other words, the combining of the IP address settings storage *and*
> > the policy of whether to configure at boot or wait until explicitly
> > requested is problematic - we really would like to control these
> > settings independently.  Is that possible within the scope of the
> > spec?  Could we just have a PCD that suppresses the automatic configure at
> boot under any circumstance?
> 
> Actually, if you want to recover the Ipv4 setting to the *un-configured*
> status (Note here: policy should be always set, *un-configured* means no IP
> address configuration), Ip4Config2 provide the ability to change the default
> policy. As git commit version SHA-1:
> 7648748e99eeeadec38fda7568adb260c4acc861 described, "This update let
> the other platform drivers have chance to change the default config data by
> consume Ip4Config2Protocol. "  So, you don't need to set another NVRAM
> location to do that, that means additional DXE_DRIVER can add in your
> platform ahead to change the default config data. The only operation in this
> DXE_DRIVER is to register a notify for Ip4Config2 protocol to change the
> default policy. That's also the reason why we don't want to enroll PCD to
> change it, you know two interface to do the same thing is not a good idea.

Let's say I have configured a static IP of 192.168.0.1.  But since I don't want 
the network interface to automatically configure at boot I think you are saying 
that I should implement a platform policy driver that uses Ip4Config2 to set an 
*unconfigured* state.  But there is no definition for an "unconfigured" state 
in UEFI 2.6 so how would one do this?  Furthermore, wouldn't this effectively 
delete the previous static IP data?  Maybe I just don't understand what you're 
describing - perhaps some pseudocode for your proposal that accomplishes the 
use case I'm describing would help.

Again, I think this is just two different pieces of information that needs to 
be stored separately: 1. static vs dhcp vs unconfigured IP address settings, 2. 
automatic versus on-demand startup .  Trying to to modify the IP address 
settings as an indirect way of communicating whether the stack should be 
configured seems far more troublesome than just storing another policy variable.

If you feel this should be brought to UNST instead let me know and I'll switch 
forums.

Eugene


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

Re: [edk2] [PATCH 1/3] ArmPkg/CompilerIntrinsicsLib: replace memcpy and memset with C code

[Cohen, Eugene]

> > Why does memcpy performance matter?  In addition to the overall
> memcpy stuff scattered around C code we have an instance that is
> particularly sensitive to memcpy performance.  For DMA operations
> when invoking double-buffering or access to portions of a buffer that
> is common mapped (i.e. uncached on non-coherent DMA systems) the
> impact of a non-optimized memcpy is enormous compared to the
> optimized ones because the penalty is amplified by orders of
> magnitude due to uncached memory access latency.
> >
> 
> That code would be using CopyMem(), no? This only serves the
> compiler
> generated calls, which are few since Tianocore does not allow
> initialized locals.

I see and agree that should minimize the impact.   I guess I'll ask the naive 
question.  Could the BaseMemoryLib and CompilerIntrinsicsLib share the same 
stuff?

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

Re: [edk2] [PATCH 1/3] ArmPkg/CompilerIntrinsicsLib: replace memcpy and memset with C code

[Cohen, Eugene]

> This replaces the various implementations of memset and memcpy,
> including the ARM RTABI ones (__aeabi_mem[set|clr]_[|4|8]) with
> a single C implementation for each. The ones we have are either not
> very sophisticated (ARM), or they are too sophisticated (memcpy() on
> AARCH64, which may perform unaligned accesses) or already coded in
> C
> (memset on AArch64).

Ard,

I'm concerned about the performance impact of this change... there's a reason 
for all that complexity and it's to optimize performance.

Why does memcpy performance matter?  In addition to the overall memcpy stuff 
scattered around C code we have an instance that is particularly sensitive to 
memcpy performance.  For DMA operations when invoking double-buffering or 
access to portions of a buffer that is common mapped (i.e. uncached on 
non-coherent DMA systems) the impact of a non-optimized memcpy is enormous 
compared to the optimized ones because the penalty is amplified by orders of 
magnitude due to uncached memory access latency.

So I would ask that before a change like this is brought in that we 
characterize the cached-cached and cached-uncached (and perhaps unaligned 
cached-cached) performance across the implementations.  Based on my experience 
I'm expecting both cases will take a massive performance hit.

>From your commit message I'm inferring that the problem you're solving is to 
>play nice in environments that can't tolerate unaligned access like when the 
>MMU is off.  I get that - and I think a variant of the library that plays nice 
>in these limited cases makes sense.  However, I don't think we should drag 
>down the performance down of the rest of the environment where we spend the 
>vast majority of our time executing.

Eugene



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

Re: [edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

Ting and Jiaxin, thank you both for clarifying.

In our situation DHCP is being set on a previous boot and we are now observing 
DHCP being attempted on every boot (since the controllers are connected).  So 
this is consistent with the behavior you describe - even though the default was 
originally static/manual the fact that we configured DHCP once causes this to 
be stored to NVRAM and perform dhcp process at every boot.

The issue is not just a performance issue around DHCP timing.  Even with a 
static IP address set the fact that the network interface comes "up" at each 
boot is problematic because our requirement is only to enable the network 
interface when directed to by an application.

Modifying the IP configuration dynamically to suppress the interface coming up 
at every boot is also problematic because it means we would need to store the 
IP address configuration in another NVRAM location.  In other words, the 
combining of the IP address settings storage *and* the policy of whether to 
configure at boot or wait until explicitly requested is problematic - we really 
would like to control these settings independently.  Is that possible within 
the scope of the spec?  Could we just have a PCD that suppresses the automatic 
configure at boot under any circumstance?

Thanks,

Eugene

> -Original Message-
> From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
> Sent: Thursday, August 11, 2016 12:00 AM
> To: Ye, Ting <ting...@intel.com>; Cohen, Eugene <eug...@hp.com>;
> edk2-devel@lists.01.org
> Subject: RE: DHCP Automatic Configure at Driver Connect
> 
> Thanks Ting's more background clarification.
> 
> I assume the difference you mentioned is between "SHA-1:
> 3d0a49ad47619c30c84bbee8a33f54b64dddbcec" and "SHA-1:
> 7648748e99eeeadec38fda7568adb260c4acc861". The two commits
> does cause the different behavior as Ting said below. Git version
> 3d0a49ad will only set the policy to dhcp but don't trigger D.O.R.A while
> 7648748e always trigger D.O.R.A if policy is DHCP.
> 
> Version 7648748e commit is also the current behavior of Ip4Config2:
> DHCP policy together with D.O.R.A process, which is similar to the old
> Ip4Config behavior. The version 3d0a49ad did was trying to resolve the
> Ip4Dxe performance but it's not workable for IPv6, so we reverted it.
> 
> Thanks,
> Jiaxin
> 
> > -Original Message-
> > From: Ye, Ting
> > Sent: Thursday, August 11, 2016 11:03 AM
> > To: Cohen, Eugene <eug...@hp.com>; edk2-devel@lists.01.org;
> Wu, Jiaxin
> > <jiaxin...@intel.com>
> > Subject: RE: DHCP Automatic Configure at Driver Connect
> >
> > Hi Eugene,
> >
> > Actually this is exactly the problem Samer raised to the mailing list in
> Aug 2015.
> > We ever fixed it with following patch:
> >
> > SHA-1: 3d0a49ad47619c30c84bbee8a33f54b64dddbcec
> >
> > * MdeModulePkg: Fix issue about current Ip4Dxe implementation
> for DHCP
> > DORA process
> >
> > DHCP policy is applied as default at boot time on all NICs in the
> system, which
> > results in all NIC ports attempting DHCP and trying to acquire IP
> addresses
> > during boot.
> > Ip4 driver should only set dhcp as default policy, and not trigger
> DORA at
> > driver binding start(). We should start DORA until one IP child is
> configured to
> > use default address.
> >
> > Later HP raised the same performance impact in IPv6 stack. We
> realized we
> > couldn't use the same logic to defer DHCP6 SARR process.
> > Instead, we discussed the issue in spec group and we removed the
> > restriction from UEFI specification that the default policy should be
> > Ip4Config2PolicyDhcp or Ip6ConfigPolicyAutomatic. It's up to
> > implementation's choice.
> > The EDKII implementation was later updated that the default policy
> was
> > changed to Ip4Config2PolicyStatic and IP6ConfigPolicyManual. Also
> the
> > previous change was reverted, in order to keep IP4/IP6 solution
> consistent.
> > See patch (also reviewed by Samer):
> >
> > SHA-1: 7648748e99eeeadec38fda7568adb260c4acc861
> >
> > * MdeModulePkg: Change the default IPv4 config policy
> >
> > Git version '3d0a49ad' commit provided a scenario to resolve the
> > performance issue for IPv4, but it's not workable for IPv6. To avoid
> IPv4 and
> > IPv6 inconsistency, we decided to revert that version fix.
> >
> > If so, the default policy for Ip4Config2 is Ip4Config2PolicyDhcp, which
> results
> > in all NIC ports attempting DHCP. So, this patch is used to changes the
> the
> > default IPv4 config policy to Ip4Config2PolicyStatic and also defer the
> SetData
> &g

Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt

[Cohen, Eugene]

> My description was not very clear, and the point is academic if you are
> happy with the solution.

I think it's important that we're aligned on how the GIC works so thanks for 
humoring me.

> However:
> The GIC spec has a state machine diagram (Figure 4.3), where:
> Transition D, pending to active and pending
> This transition occurs on acknowledgement of the interrupt by the PE
> for level-sensitive SPIs, SGIs,
> and PPIs.
> Transition B1 or B2, remove pending state
> This transition occurs when the interrupt has been deasserted by the
> peripheral, if the interrupt is a
> level-sensitive interrupt, or when software has changed the pending
> state.
> Transition E1 or E2, remove active state
> This transition occurs when software deactivates an interrupt for SPIs,
> SGIs, and PPIs.
> 
> I suspect that because we EOI ("deactivate" for E1/E2) without
> "deasserting" the peripheral interrupt then the GIC may restore the
> pending state (transition E2 instead of B2 then E1), which will look
> remarkably like a latch.

But no latching will occur because, for a level sensitive interrupt, the 
EOI-before deassert should manifest as transition E2 (caused by EOI) followed 
by transition B1 (caused by clearing the source).  This is per the text that 
transition B1 occurs if "the level-sensitive interrupt is pending only because 
of the assertion of an input signal, and that signal is deasserted".  So the 
transition is Active+Pending -> Pending -> Inactive for this odd ordering 
versus the more sensible Active+Pending -> Active -> Inactive.

> That looks like a pretty sensible way forward.  I'll vote for EFI_SUCCESS,
> white lies are sometimes needed.

Okay, what's the worst that can happen? :)

Perhaps the real test would be that a driver that uses HwInterrupt is shown to 
work equally well on a fake-EOI interface system as well as a real-EOI 
interface system.  I doubt if we have any common peripherals (timer block or 
serial port or whatever) to really test this.

Eugene

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

Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt

[Cohen, Eugene]

Evan,
 
> I'd like to re-think our GIC EIOR changes in the light of comments from
> Heyi Guo. (inserted before Eugene's e-mail below)
> Despite Eugene's cogent advocacy of the change, and the fact that
> Alexei's fix is now accepted, I have come to the conclusion that it is not
> the best thing to do.  As Heyi points out, it opens a race condition
> where the Timer interrupt line is still asserted after the GIC EIOR has
> been written.
> The timer IRQ needs to be de-asserted before the EIOR write, or the
> GIC sees the IRQ and latches it ("active and pending").
> This is clearly an error, and a minor mystery is that we do not see that
> on our Juno boards.

According to the GIC architecture spec for level-sensitive interrupts are 
pending only based on the immediate state of the signal, there is no latching, 
see "Control of the pending status of level-sensitive interrupts" in the GICv2 
Architecture Specification:

"
For an edge-triggered interrupt, the includes pending status is latched on 
either a write to the GICD_ISPENDRn or
the assertion of the interrupt signal to the GIC. However, for a 
level-sensitive interrupt, the includes pending status
either:
  • is latched on a write to the GICD_ISPENDRn
  • follows the state of the interrupt signal to the GIC, without any latching.
"

So there's no "memory" for level sensitive interrupts and an edge triggered 
interrupt will only latch on an edge, not an EOI so I'm not sure there's an 
issue.

>From what I can tell the primary purpose of EOI is for interrupt priority 
>management - when you issue the EOI a priority drop occurs allowing the 
>next-highest priority interrupt to be serviced, if any.  But since we are not 
>making use of nested interrupts (i.e. IRQ is masked the whole time during 
>interrupt processing) I don't think the EOI sequencing matters.  This is based 
>on about 10 minutes of me reading this GIC spec so please correct any 
>confusion I may have.  I agree the double-EOI has to go away no matter what.

> This is very much at odds with Eugene's position (which, BTW,  is not a
> stance I find comfortable).
> Further, it implies removing any EIOR writes from extant interrupt
> handlers - however, since they already have the "double write"
> problem, that is not really a concern.  (BTW, the GIC spec is very clear
> that "A write to this register must correspond to the most recent valid
> read from an Interrupt Acknowledge Register... otherwise the system
> behavior is UNPREDICTABLE", so the double write is not good.)

I did some detective work and the double-write problem goes way back to the 
BeagleBoard/Omap35xx code from 2010 where the NEWIRQAGR register was written 
multiple times as well.  This code actually writes the EOI-like NEWIRQAGR 
register two times in addition to the additional EOI interface write:

  // Needed to prevent infinite nesting when Time Driver lowers TPL
  MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR);
  ArmDataSynchronizationBarrier ();

  InterruptHandler = gRegisteredInterruptHandlers[Vector];
  if (InterruptHandler != NULL) {
// Call the registered interrupt handler.
InterruptHandler (Vector, SystemContext);
  }

  // Needed to clear after running the handler
  MmioWrite32 (INTCPS_CONTROL, INTCPS_CONTROL_NEWIRQAGR);
  ArmDataSynchronizationBarrier ();

Note the comment about infinite nesting - I don't know what this is referring 
to since I would expect TPL to remain at HIGH_LEVEL for the duration of timer 
interrupt processing.  I'd really like to get Mr. Fish to chime in on this one.

> A more elegant solution is for the GIC register access to be done as
> part of the GIC handling (i.e. revert the GIC code, and remove the EIOR
> from the Timer handling.)  This also caters for any surreptitious use of
> other interrupts "under the covers".
> As an additional benefit, it clearly partitions the peripheral specific
> handling from the GIC interface.

I have no specific objection to this - let's just find a way to do this that 
maintains compatibility with the existing HardwareInterrupt protocol definition 
- maybe return EFI_UNSUPPORTED (or perhaps just EFI_SUCCESS) for the EOI 
protocol interface on systems that don't want to expose this functionality.  
Then an old driver could try to use the EOI interface and on old systems it 
will issue the EOI and on new systems it will do nothing, deferring the real 
EOI to when the ISR returns.

Eugene

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

Re: [edk2] [PATCH 05/26] ArmPkg: introduce ASM_FUNC, MOV32/MOV64 and ADRL/LDRL macros

[Cohen, Eugene]

> > Why does this work?  In my experimentation the C preprocessor
> would collapse the stuff onto a single line (the backslash being a
> continuation on the preprocessor input, but preprocessor output
> revealed the newlines being removed), thereby violating the assembly
> requirement that labels appear in column 1.
> >
> 
> I have tested this with both GNU as and Clang, and neither complains.
> Is this requirement documented anywhere?

Not sure - just the dusty recesses of my mind.  That and the fact that I 
thought I tried this already.  :)

Reviewed-by: Eugene Cohen 

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

Re: [edk2] [PATCH 05/26] ArmPkg: introduce ASM_FUNC, MOV32/MOV64 and ADRL/LDRL macros

[Cohen, Eugene]

Ard,

> +#define _ASM_FUNC(Name, Section)\
> +  .global   Name  ; \
> +  .section  #Section, "ax"; \
> +  .type Name, %function   ; \
> +  Name:
> +
> +#define ASM_FUNC(Name)_ASM_FUNC(ASM_PFX(Name),
> .text. ## Name)

Why does this work?  In my experimentation the C preprocessor would collapse 
the stuff onto a single line (the backslash being a continuation on the 
preprocessor input, but preprocessor output revealed the newlines being 
removed), thereby violating the assembly requirement that labels appear in 
column 1.

Thanks for all your work on this, now I'm just trying to understand what I'm 
looking at!

Eugene

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

[edk2] DHCP Automatic Configure at Driver Connect

[Cohen, Eugene]

With this commit:

Revision: 1f6729ffe98095107ce82e67a4a0209674601a90
Author: jiaxinwu 
Date: 7/7/2015 2:19:55 AM
Message:
MdeModulePkg: Update Ip4Dxe driver to support Ip4Config2 protocol,

a new behavior seemed to come in to the network stack that was not present 
before: It appears now that as soon as the DHCP Service Binding protocol is 
installed the DHCP process will be initiated (see 
Ip4Config2OnDhcp4SbInstalled).  This differs from past behavior where DHCP 
would only occur if a driver or application specifically did Configure() on the 
network interface.

For some systems this is problematic because they need to defer DHCP until it 
is certain that the network interface will be used for something.

Can you provide the reason for this change?  Can we have a policy (PCD) to 
disable this mode of operation?

Thanks,

Eugene


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

[edk2] IP4 Config Troubles with DHCP

[Cohen, Eugene]

We have been running into an issue when trying to configure an interface as 
DHCP where if the DHCP process is not yet complete (Ip4Mode.IsConfigured is 
FALSE) the configure process will never succeed.

We have a case where we attempt to invoke the UDP4 Configure:

Status = Nlc->Udp4->Configure(Nlc->Udp4, >UdpConfig);

We had a retry loop where we keep calling Udp4->Configure until we finally see 
Ip4Mode.IsConfigured go TRUE (similar to what you see in Mtftp4GetMapping) - 
this has worked for many years but recently something broke this.   Now, even 
when DHCP succeeds the Ip4Mode.IsConfigured flag is set to FALSE.  

Only if we retry by destroying and re-creating new service binding children can 
we actually get this logic to succeed.  This logic is getting ridiculously 
complicated so I'm thinking there has to be a better way of doing this.

Do you have an example of specifically how a driver/app should handle the case 
where the DHCP process is not yet complete and wants to wait?

Thanks,

Eugene



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

Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt

[Cohen, Eugene]

Guys, sorry to join so late, something about timezones...  Let me try to 
provide some context and history.

> > it does change the contract we have with registered interrupt handlers
> 
> Looks like it does not:
> From edk2\EmbeddedPkg\Include\Protocol\HardwareInterrupt.h:
> 
> " Abstraction for hardware based interrupt routine
> 
>   ...The driver implementing
>   this protocol is responsible for clearing the pending interrupt in the
>   interrupt routing hardware. The HARDWARE_INTERRUPT_HANDLER is
> responsible
>   for clearing interrupt sources from individual devices."

I think you are reading more deeply into this verbiage than was intended.  From 
a separation-of-concerns perspective one driver is concerned with writing to 
the hardware that generates the interrupt (handler) and another is concerned 
with writing to the hardware for the interrupt controller to signal the end of 
interrupt.  So all this is saying is that "the code that touches the interrupt 
controller is implemented in the driver that publishes this protocol".  It does 
not say how this code is activated, only who is responsible for poking the 
register.

The historical expectation is that the handler driver calls the EOI interface 
in the protocol.  (If it was the opposite then this interface wouldn't even 
exist since the interrupt controller driver could just do it implicitly.)  
You're next question will probably be why it was designed this way - for that 
we'll have to ask Andrew Fish (added).

I did a little digging and see that the PC-AT chipset implemented an 8259 
interrupt protocol (IntelFrameworkPkg\Include\Protocol\Legacy8259.h) that is 
quite similar to HwInterrupt.  Notice the explicit EndOfInterrupt interface 
here and how it's used by the timer driver at 
PcAtChipsetPkg\8254TimerDxe\Timer.c(88).

Given this I asked that you keep the EndOfInterrupt in the handler driver(s) 
and remove the auto-EOI in the interrupt controller driver, at least for cases 
where a driver handled the interrupt.

Feel free to clarify the text in the protocol header to align with this - the 
current wording is not very clear.

Thanks,

Eugene

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Monday, August 08, 2016 5:51 AM
> To: Alexei Fedorov <alexei.fedo...@arm.com>
> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Heyi Guo
> <heyi@linaro.org>; Leif Lindholm <leif.lindh...@linaro.org>; Cohen,
> Eugene <eug...@hp.com>
> Subject: Re: [edk2] [PATCH] ArmPkg: Fix double GIC EIOR write per interrupt
> 
> On 8 August 2016 at 13:08, Ard Biesheuvel <ard.biesheu...@linaro.org>
> wrote:
> > On 8 August 2016 at 13:06, Alexei Fedorov <alexei.fedo...@arm.com>
> wrote:
> >> Timer's pending interrupt is cleared  HARDWARE_INTERRUPT_HANDLER
> TimerInterruptHandler()  in when timer is re-programmed for the next shot.
> >
> > Yes, that is the timer side.
> >
> >> Does it mean that TimerDxe driver is part
> EFI_HARDWARE_INTERRUPT_PROTOCOL?
> >>
> >
> > No. The peripheral and the GIC each have their own mask and enable
> > registers for the interrupt. That is exactly why the comment describes
> > in detail which part is the responsibility of the GIC, and which is
> > the responsibility of the peripheral.
> >
> 
> ... and actually, looking at TimerInterruptHandler (), I don't see the point 
> of
> re-enabling the interrupt early, given that
> 
> 308: OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> 
> disables interrupts globally, and only re-enables them on line 346, at which
> point the mTimerNotifyFunction() has already returned.
> 
> So I propose we simply do
> 
> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> index 1169d426b255..f0fcb05757ac 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -308,10 +308,7 @@ TimerInterruptHandler (
>OriginalTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL);
> 
>// Check if the timer interrupt is active
> -  if ((ArmGenericTimerGetTimerCtrlReg () ) & ARM_ARCH_TIMER_ISTATUS)
> {
> -
> -// Signal end of interrupt early to help avoid losing subsequent
> ticks from long duration handlers
> -gInterrupt->EndOfInterrupt (gInterrupt, Source);
> +  while ((ArmGenericTimerGetTimerCtrlReg () ) &
> ARM_ARCH_TIMER_ISTATUS)
> + {
> 
>  if (mTimerNotifyFunction) {
>mTimerNotifyFunction (mTimerPeriod * mElapsedPeriod);
> 
> so that if the condition exists that we know will trigger the interrupt
> immediately as soon as we unmask it, we simply enter the loop again just like
> we would when taking the [nested] interrupt.
> 
> @Heyi: any thoughts?
> 
> --
> 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] Breaking change issue with NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]

[Cohen, Eugene]

We've been hit by this same kind of issue and it's really painful, especially 
as it affects shipping systems.

Long term I think we need an extensible/revisioned data format so we can get 
forwards and backwards compatibility between NVRAM data and FW.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Larry Cleeton
> Sent: Friday, August 05, 2016 9:35 AM
> To: Ye, Ting ; edk2-devel@lists.01.org
> Subject: Re: [edk2] Breaking change issue with
> NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]
> 
> I agree with your assessment about leaving the data structure as it is.   I 
> just
> wanted to highlight it as it may impact others.
> 
> The bottom line is my development group is entirely responsible for vetting
> any changes coming from the EDK2 into our product.  This one slipped by us.
> 
> --Larry
> 
> -Original Message-
> From: Ye, Ting [mailto:ting...@intel.com]
> Sent: Thursday, August 4, 2016 8:25 PM
> To: Larry Cleeton ; edk2-devel@lists.01.org
> Subject: RE: Breaking change issue with
> NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]
> 
> Hi Larry,
> 
> We are very sorry about the impact you suffered today. We made the
> change in early 2013 to support the existing NVRAM variable when firmware
> image was updated from IA32 to X64. Unfortunately we introduced an
> incompatibility issue as you raised. Now we prefer to keep the existing
> definition, since if we change it back that would introduce another similar
> incompatibility issue. What do you think about this?
> 
> Best Regards,
> Ye Ting
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Larry Cleeton
> Sent: Wednesday, August 03, 2016 4:55 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] Breaking change issue with
> NetworkPkg/Ip6Dxe/Ip6ConfigImlp.[c, h]
> 
> This commit (fdc4b0b147b386e966e99893526181dfae9eaeef) changed a data
> structure that is stored in an NVRAM variable.
> See NetworkPkg/Ip6Dxe/Ip6ConfigImpl.[c,h]
> 
> This data structure:
> 
> typedef struct {
>   UINT16Offset;
>   UINTN DataSize;
>   EFI_IP6_CONFIG_DATA_TYPE  DataType;
> } IP6_CONFIG_DATA_RECORD;
> 
> Is now:
> 
> typedef struct {
>   UINT16Offset;
>   UINT32 DataSize;< changed size in 
> 64bit
> environments
>   EFI_IP6_CONFIG_DATA_TYPE  DataType;
> } IP6_CONFIG_DATA_RECORD;
> 
> Unfortunately with a 64bit implementation this current structure is now
> *not* compatible with an existing NVRAM variable written with the previous
> version of the structure. It's causing me considerable grief so I'm just 
> sharing
> the discovery.  It would only impact you if you update some 64bit machine's
> firmware with a new version containing this change.
> 
> --Larry
> ___
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Managing GCC Assembly Code Size (AArch64)

[Cohen, Eugene]

Ard, as usual you rock...

> FYI there is a null token \() for GAS which you can use to concatenate
> a string with a macro argument, e.g.,
> 
>.macro func, x
>.globl \x
>.type \x, %function
>.section .text.\x
> \x\():
>   .endm
>

Using the GAS .macro syntax this all collapses nicely.  I tested it with one 
assembly function and all the right stuff happens.

So the request becomes: can we modify all of the assembly (at least Aarch64 
please) to use this?  How would you like to phase this in?

> > I think this would be an improvement, so go for it. The only thing to
> > be wary of is routines that fall through into the subsequent one.
> > Those need to remain in the same section.

Yes, I've accidentally modified these with disastrous results.  I now know to 
stay away from them (ExceptionSupport.S in particular).  :)

Thanks,

Eugene

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, August 04, 2016 1:47 PM
> To: Cohen, Eugene <eug...@hp.com>
> Cc: Leif Lindholm <leif.lindh...@linaro.org>; edk2-devel@lists.01.org
> Subject: Re: Managing GCC Assembly Code Size (AArch64)
> 
> On 4 August 2016 at 21:18, Ard Biesheuvel
> <ard.biesheu...@linaro.org> wrote:
> > On 4 August 2016 at 20:08, Cohen, Eugene <eug...@hp.com>
> wrote:
> >> Ard and Leif,
> >>
> >> I've been too backlogged to provide a real patchset at this point but
> wanted to get your approval on this proposal...
> >>
> >>
> >> As you know we have some code size sensitive uncompressed XIP
> stuff going on.  For C code we get dead code stripping thanks to the "-
> ffunction-sections" switch which places each function in its own
> section so the linker can strip unreferenced sections.
> >>
> >> For assembly there is not a solution that's as easy.  For RVCT we
> handled this with an assembler macro that combined the procedure
> label definition, export of global symbols and placement of the
> procedure in its own section.  For GCC I haven't found a way to fully do
> this because we rely on the C preprocessor for assembly which means
> you cannot expand to multi-line macros.  (The label and assembler
> directives require their own lines but the preprocessor collapses stuff
> onto one line because in the C language newlines don't matter.)
> >>
> >> So the solution I've settled on is to do this:
> >>
> >> in MdePkg\Include\AArch64\ProcessorBind.h define:
> >>
> >>   /// Macro to place a function in its own section for dead code
> elimination
> >>   /// This must be placed directly before the corresponding code
> since the
> >>   /// .section directive applies to the code that follows it.
> >>   #define GCC_ASM_EXPORT_SECTION(func__)   \
> >>   .global  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
> ;\
> >>   .section  .text._CONCATENATE (__USER_LABEL_PREFIX__,
> func__);\
> >>   .type ASM_PFX(func__), %function; \
> >>
> >> This has the effect of placing the function in a section called
> .text. so the linker can do its dead code stripping stuff.  It also
> absorbs the making the symbol globally visible so the corresponding
> GCC_ASM_EXPORT statement can be removed.
> >>
> >> then for every single assembly procedure change from this:
> >>
> >> [top of file]
> >> GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryByMVA)
> >>
> >> [lower down]
> >> ASM_PFX(ArmInvalidateDataCacheEntryByMVA):
> >>   dc  ivac, x0// Invalidate single data cache line
> >>   ret
> >>
> >> to this:
> >>
> >>
> GCC_ASM_EXPORT_SECTION(ArmInvalidateDataCacheEntryByMVA)
> >> ASM_PFX(ArmInvalidateDataCacheEntryByMVA):
> >>   dc  ivac, x0// Invalidate single data cache line
> >>   ret
> >>
> >> Because the assembly label must appear in column 1 I couldn't find
> a way to use the C preprocessor to absorb it so hence the two lines.  If
> you can find a way to improve on this it would be great.
> >>
> >
> > What about GAS macros (.macro / .endm). I prefer those over cpp
> macros
> > in assembler anyway.
> >
> 
> FYI there is a null token \() for GAS which you can use to concatenate
> a string with a macro argument, e.g.,
> 
>.macro func, x
>.globl \x
>.type \x, %function
>.section .text.\x
> \x\():
>   .endm
> 
> 
> >> I'm not sure what impacts this might have to other toolchains - can
> this be translated to CLANG and ARM Compiler?
> >>
>

[edk2] Managing GCC Assembly Code Size (AArch64)

[Cohen, Eugene]

Ard and Leif,

I've been too backlogged to provide a real patchset at this point but wanted to 
get your approval on this proposal...


As you know we have some code size sensitive uncompressed XIP stuff going on.  
For C code we get dead code stripping thanks to the "-ffunction-sections" 
switch which places each function in its own section so the linker can strip 
unreferenced sections.

For assembly there is not a solution that's as easy.  For RVCT we handled this 
with an assembler macro that combined the procedure label definition, export of 
global symbols and placement of the procedure in its own section.  For GCC I 
haven't found a way to fully do this because we rely on the C preprocessor for 
assembly which means you cannot expand to multi-line macros.  (The label and 
assembler directives require their own lines but the preprocessor collapses 
stuff onto one line because in the C language newlines don't matter.)

So the solution I've settled on is to do this:

in MdePkg\Include\AArch64\ProcessorBind.h define:

  /// Macro to place a function in its own section for dead code elimination
  /// This must be placed directly before the corresponding code since the
  /// .section directive applies to the code that follows it.
  #define GCC_ASM_EXPORT_SECTION(func__)   \
  .global  _CONCATENATE (__USER_LABEL_PREFIX__, func__);\
  .section  .text._CONCATENATE (__USER_LABEL_PREFIX__, func__);\
  .type ASM_PFX(func__), %function; \

This has the effect of placing the function in a section called .text. 
so the linker can do its dead code stripping stuff.  It also absorbs the making 
the symbol globally visible so the corresponding GCC_ASM_EXPORT statement can 
be removed.

then for every single assembly procedure change from this:

[top of file]
GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryByMVA)

[lower down]
ASM_PFX(ArmInvalidateDataCacheEntryByMVA):
  dc  ivac, x0// Invalidate single data cache line
  ret

to this:

GCC_ASM_EXPORT_SECTION(ArmInvalidateDataCacheEntryByMVA)
ASM_PFX(ArmInvalidateDataCacheEntryByMVA):
  dc  ivac, x0// Invalidate single data cache line
  ret

Because the assembly label must appear in column 1 I couldn't find a way to use 
the C preprocessor to absorb it so hence the two lines.  If you can find a way 
to improve on this it would be great.

I'm not sure what impacts this might have to other toolchains - can this be 
translated to CLANG and ARM Compiler?

I'd like to get your OK on this conceptually and then I could upstream some 
patches that modify the AArch64 *.S files to use this approach.  Unfortunately 
it won't be complete because I only updated the libraries that we use.  My hope 
is that long term all assembly (or at least assembly in libraries) adopt this 
approach so we are positioned for maximum dead code stripping.

Thanks,

Eugene


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

Re: [edk2] Shell Path Issues with "../.."

[Cohen, Eugene]

Thanks - I searched for the subject on my edk2-devel archive and didn't see 
anything.  I now see the patch sent 6/21, so yeah about 3 months later. :)

Next time I ask that the maintainer respond to the original email please 
(granted I screwed up because I forgot to include them specifically on the To: 
line) and include me on the patch review (again on the To: line).

Eugene

> -Original Message-
> From: Alcantara, Paulo
> Sent: Wednesday, August 03, 2016 1:27 PM
> To: Cohen, Eugene <eug...@hp.com>; edk2-devel@lists.01.org;
> Carsey, Jaben <jaben.car...@intel.com> (jaben.car...@intel.com)
> <jaben.car...@intel.com>
> Cc: Thompson, Mark L. (Boise IPG) <mark.l.thomp...@hp.com>
> Subject: RE: Shell Path Issues with "../.."
> 
> Hi Eugene,
> 
> I think it has been fixed on master already. See the commit below:
> 
> commit 85df61243cb56c3d9c52a5005e65c4ea8bf60e52
> Author: Qiu Shumin <shumin@intel.com>
> Date:   Tue Jun 21 16:18:56 2016 +0800
> 
> MdePkg: Fix 'cd ..\..' go up only 1 level.
> 
> When we try to cd up two levels using the "../.." notation we
> only go up one level. This patch fix this bug.
> 
> Thanks,
> 
> Paulo
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of Cohen, Eugene
> Sent: quarta-feira, 3 de agosto de 2016 16:17
> To: edk2-devel@lists.01.org; Carsey, Jaben <jaben.car...@intel.com>
> (jaben.car...@intel.com) <jaben.car...@intel.com>
> Cc: Thompson, Mark L. (Boise IPG) <mark.l.thomp...@hp.com>
> Subject: Re: [edk2] Shell Path Issues with "../.."
> 
> Reminder (a few months later)
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On
> Behalf Of
> > Cohen, Eugene
> > Sent: Wednesday, March 30, 2016 4:51 PM
> > To: edk2-devel@lists.01.org
> > Cc: Thompson, Mark L. (Boise IPG) <mark.l.thomp...@hp.com>
> > Subject: [edk2] Shell Path Issues with "../.."
> >
> > Dear ShellPkg maintainer,
> >
> > We're seeing some weird stuff related to '..' in the shell.
> >
> > The issue is that when we try to cd up two levels using the "../.."
> > notation we only go up one level:
> >
> > FS0:\dir1\dir2\dir3\>
> > FS0:\dir1\dir2\dir3\> cd ..\..
> > FS0:\dir1\dir2\>
> >
> > The same issue occurs using forward and back slashes.  The old (EDK)
> > shell used to handle this correctly.
> >
> > Any ideas what's going on here?
> >
> > Thanks,
> >
> > Eugene
> >
> > ___
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Shell Path Issues with "../.."

[Cohen, Eugene]

Reminder (a few months later)

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of Cohen, Eugene
> Sent: Wednesday, March 30, 2016 4:51 PM
> To: edk2-devel@lists.01.org
> Cc: Thompson, Mark L. (Boise IPG) <mark.l.thomp...@hp.com>
> Subject: [edk2] Shell Path Issues with "../.."
> 
> Dear ShellPkg maintainer,
> 
> We're seeing some weird stuff related to '..' in the shell.
> 
> The issue is that when we try to cd up two levels using the "../.."
> notation we only go up one level:
> 
> FS0:\dir1\dir2\dir3\>
> FS0:\dir1\dir2\dir3\> cd ..\..
> FS0:\dir1\dir2\>
> 
> The same issue occurs using forward and back slashes.  The old (EDK)
> shell used to handle this correctly.
> 
> Any ideas what's going on here?
> 
> Thanks,
> 
> Eugene
> 
> ___
> 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] AArch64 XIP Recipe

[Cohen, Eugene]

Ard,


> I coded this up here. Could you take a look whether this works for you, 
> please?
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/commitdiff/3194d6c71014017df323ff22b1b0af819bd0e760

>From a build perspective it looks good, I see this:

 31c:   d021adrpx1, 6000 
 320:   b9400e64ldr w4, [x19,#12]

in ELF turning into this in PECOFF:

  031C: 1002E721  adr x1,6000
  0320: B9400E64  ldr w4,[x19,#0xC]

I booted our platform and this change works.

>From a size perspective our XIP FV grows by 5.8KB changing from the tiny model 
>to the small model - it's better than both the 18KB from large and 300-ish KB 
>from small+page aligned.

Thanks to you we have a number of options for how to address the XIP issue, 
many of which work for us.  How can we capture these learnings so others can 
benefit?  Some sort of AArch64 XIP wiki?  Which approach do you want to use 
with edk2 reference platforms?

Thank you!

Eugene

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

Re: [edk2] AArch64 XIP Recipe

[Cohen, Eugene]

Ard,

> OK, sorry for the monologue, but I just had an idea: we could build
> everything with the small model, and convert ADRP instructions to ADR
> instructions in GenFw if the section alignment < 4 KB. This will cause
> build failures if this conversion is not possible due to the fact that
> the binary exceeds 1 MB, but this means we only need to set the 4 KB
> page size for modules where this can reasonably be expected (i.e.,
> UEFI_APPLICATION modules) Everything else will be just as small as
> the
> small model.
> 
> I will cook something up, but don't expect anything before next week.

Fascinating idea - this effectively turns 'small' into 'tiny' but does so after 
linking.  You could even make the logic dynamic based on the size of the image.

It would be great to get this as an optimization into GCC's ld even.

Thanks!

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

Re: [edk2] AArch64 XIP Recipe

[Cohen, Eugene]

> Indeed.  The Shell in RELEASE mode builds fine with the tiny model
> though. Since the Shell is the only problematic module (afaik), it
> would suffice to simply build the shell components in RELEASE mode
> unconditionally, assuming that you are not interested in debugging the
> shell and your platform at the same time.

Yes, this works.  Another workaround is to do separate build invocations for 
the XIP and non-XIP modules with different memory models for each and thereby 
get different sets of libraries.

> This is not a toolchain bug, but an ISA problem, and so it is not
> going away, unfortunately.

Right, a combination of the ISA's ADRP and the fact that toolchains 
unfortunately choose to use it.  :)

For what it's worth I did an experiment forcing the 'large' memory model and 
given the 4KB page alignment effects on XIP placement it turned out to be much 
more efficient - 18KB of growth versus 327KB.

If GCC could somehow link tiny libraries into a small/large module without 
generating relocation displacement errors from the tiny components that can no 
longer reach their counterparts it could solve this nicely - then you only pay 
for what you use.

Eugene

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Monday, July 25, 2016 10:28 AM
> To: Cohen, Eugene <eug...@hp.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: AArch64 XIP Recipe
> 
> On 25 July 2016 at 18:23, Cohen, Eugene <eug...@hp.com> wrote:
> > Doing some git archaeology, I suspect that stuff started to go bad for
> us
> > around this commit:
> >
> >
> >
> > Revision: f37d891c1b870b294964adf65f619a661700fcab
> >
> > Author: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >
> > Date: 3/25/2016 5:33:28 AM
> >
> > Message:
> >
> > BaseTools AARCH64: move DEBUG GCC49 to the small code model
> >
> >
> >
> > When building AARCH64 platforms that include a Shell binary built
> from
> >
> > source, we run into trouble when using the tiny code model for
> DEBUG
> >
> > builds. The reason is that the Shell binary built in DEBUG mode
> exceeds
> >
> > the 1 MB range of the ADR instruction, so anything that gets pulled
> into
> >
> > the final link of the Shell binary either needs to be built with the small
> >
> > or large model, or needs to be sorted in some way to put the ADR
> references
> >
> > close to their targets.
> >
> >
> >
> > The rationale with this commit was that not only does the shell itself
> need
> > to use the small model (to address the 1MB displacement limitation
> of the
> > ADR instruction) but every library it pulls in must as well.  In our
> testing
> > before this commit we did not see a problem, what did you see (if
> you can
> > remember)?
> >
> 
> The Shell failed to link due to the fact that the BASE libraries it
> depends on were built with the tiny model
> 
> >
> >
> > But any time the small memory model is used the common page size
> is set to
> > 4KB per the requirement stated in this change to BaseTools
> Elf64Convert.c:
> >
> >
> >
> > Revision: 24d610e67752ac0325c7027e2fea2f8f2ff110e2
> >
> > Author: Ard Biesheuvel <ard.biesheu...@linaro.org>
> >
> > Date: 8/10/2015 1:55:18 AM
> >
> > Message:
> >
> > BaseTools/GenFw: allow AArch64 tiny and small code model
> relocations
> >
> >
> >
> > The AArch64 small C model makes extensive use of ADRP/ADD and
> >
> > ADRP/{LDR,STR} pairs to emit PC-relative symbol references with
> >
> > a +/- 4 GB range. Since the relocation pair splits the relative
> >
> > offset into a relative page offset and an absolute offset into
> >
> > a 4 KB page, we need to take extra care to ensure that the target
> >
> > of the relocation preserves its alignment relative to a 4 KB
> >
> > alignment boundary.
> >
> >
> >
> > So since the shell requires all libraries to be built in the small memory
> > model and all small memory model components must have 4KB
> alignment, this
> > means that any component shared between XIP and the shell will
> get a 4KB
> > alignment treatment.  As discussed before this just eats up
> flash/sram space
> > in XIP regions since the FVs get padded out to meet this
> requirement.  This
> > results in an untenable situation.
> >
> 
> Indeed.  The Shell in RELEASE mode builds fine with the tiny model
> though. Since the Shell is the only problematic module (afaik), it
> would suffice to simply build th

[edk2] AArch64 XIP Recipe

[Cohen, Eugene]

Ard,

I'm in the midst of syncing up with master and am running into XIP size issues 
once again.  My XIP regions are picking up 4KB alignments and blowing up.

I need a way in the same build to get the following:

A. XIP Regions (SEC, PEI): tiny memory model, common page size 0x20, 
relocatable exceptions
B. Non-XIP Regions (DXE): small memory model, common page size 0x1000, in-place 
exceptions

It looks like the current tools_def stuff for GCC on AArch64 assumes the 4KB 
common page size in all cases.

Furthermore, what is the recommend procedure for enabling the large memory 
model as required for large applications?  Do you recommend a specific inf or 
dsc override?

Thanks,

Eugene


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

Re: [edk2] PCI performance issue

[Cohen, Eugene]

Shaveta,

> Have you tried some optimizations to get better performance in UEFI code

I think the question you should be asking is how do you measure how the current 
code is performing, this is a tools and methodology thing.  With ARM there are 
all sorts of options from JTAG debuggers that can sample things to full up 
ETM/PTM that can show the flow.  So first figure out where time is being spent 
during your network transfers using your favorite debug tools and with that 
data you will then know where to focus.

Eugene

> -Original Message-
> From: Shaveta Leekha [mailto:shaveta.lee...@nxp.com]
> Sent: Thursday, July 14, 2016 10:03 PM
> To: Cohen, Eugene <eug...@hp.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List  u...@lists.linaro.org>
> Subject: RE: PCI performance issue
> 
> You are right Eugene!
> 
> Performance is getting impacted by all these factors, but that's true that I
> shall get better performance figure than this.
> That's why I am trying to figure out the possible optimizations in code that
> may help in improving it.
> 
> Have you tried some optimizations to get better performance in UEFI code?
> 
> Thanks and Regards,
> Shaveta
> 
> 
> -Original Message-
> From: Cohen, Eugene [mailto:eug...@hp.com]
> Sent: Thursday, July 14, 2016 7:29 PM
> To: Shaveta Leekha <shaveta.lee...@nxp.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>
> Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List  u...@lists.linaro.org>
> Subject: RE: PCI performance issue
> 
> I've been down this road before...
> 
> Network performance (on non-coherent DMA architectures) can be affected
> by:
> 
> 1. Excessive double buffering caused by unaligned buffers (PCI
> BusMasterRead / BusMasterWrite cases) 2. Excessive accesses to uncached
> buffers (like PCI common buffer cases) 3. Packet loss due to the lack of
> interrupts in UEFI, I mean, due to a network polling rate that is too slow 
> (look
> at the MNP poll and UEFI tick periods)
> 
> You should be able to get far better performance than 3MB/min!
> 
> Eugene
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Shaveta Leekha
> > Sent: Thursday, July 14, 2016 7:45 AM
> > To: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List  > u...@lists.linaro.org>
> > Subject: Re: [edk2] PCI performance issue
> >
> > Ok, I can try that !!
> >
> > Thanks and Regards,
> > Shaveta
> >
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Thursday, July 14, 2016 7:11 PM
> > To: Shaveta Leekha <shaveta.lee...@nxp.com>
> > Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List  > u...@lists.linaro.org>
> > Subject: Re: PCI performance issue
> >
> > On 14 July 2016 at 15:29, Shaveta Leekha <shaveta.lee...@nxp.com>
> wrote:
> > > But I have not tested the code (software) on any other hardware/board.
> > > As I have not yet ported PCI code on any other board yet.
> > >
> >
> > I would recommend to base your expectations not on U-Boot but on UEFI
> > running on a different architecture using similar network hardware.
> > ___
> > 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] PCI performance issue

[Cohen, Eugene]

I've been down this road before...

Network performance (on non-coherent DMA architectures) can be affected by:

1. Excessive double buffering caused by unaligned buffers (PCI BusMasterRead / 
BusMasterWrite cases)
2. Excessive accesses to uncached buffers (like PCI common buffer cases)
3. Packet loss due to the lack of interrupts in UEFI, I mean, due to a network 
polling rate that is too slow (look at the MNP poll and UEFI tick periods)

You should be able to get far better performance than 3MB/min!

Eugene

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Shaveta Leekha
> Sent: Thursday, July 14, 2016 7:45 AM
> To: Ard Biesheuvel 
> Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List  u...@lists.linaro.org>
> Subject: Re: [edk2] PCI performance issue
> 
> Ok, I can try that !!
> 
> Thanks and Regards,
> Shaveta
> 
> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, July 14, 2016 7:11 PM
> To: Shaveta Leekha 
> Cc: edk2-devel@lists.01.org; Linaro UEFI Mailman List  u...@lists.linaro.org>
> Subject: Re: PCI performance issue
> 
> On 14 July 2016 at 15:29, Shaveta Leekha  wrote:
> > But I have not tested the code (software) on any other hardware/board.
> > As I have not yet ported PCI code on any other board yet.
> >
> 
> I would recommend to base your expectations not on U-Boot but on UEFI
> running on a different architecture using similar network hardware.
> ___
> 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] Fix bug in TCP which not sending out ACK

[Cohen, Eugene]

Series Reviewed-By: Eugene Cohen 

Thank you for your ongoing help with our TCP issues, it is much appreciated.

Eugene

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Wu, Jiaxin
> Sent: Tuesday, July 12, 2016 8:12 PM
> To: Fu, Siyuan ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch 0/2] Fix bug in TCP which not sending out ACK
> 
> Series Reviewed-By: Wu Jiaxin 
> 
> Best Regards!
> Jiaxin
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Fu Siyuan
> > Sent: Tuesday, July 12, 2016 9:50 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [Patch 0/2] Fix bug in TCP which not sending out ACK
> >
> > Consider the situation as shown in below chart. The last ACK message
> > has acknowledged the Tcb->RcvWl2, and all the segments until
> > Tcb->RcvNxt have been received by TCP driver. The Tcb->RcvNxt is not
> > acknowledged due to the delayed ACK. In this case an incoming segment
> > (Seg->Seq, Seg->End) should not be accepted by TCP driver, and an
> immediate ACK is required.
> >
> > Current TcpSeqAcceptable() thought it  s an acceptable segment
> > incorrectly, it continues the TcpInput() process instead of sending
> > out an ACK and droping the segment immediately.
> >
> > Tcb->RcvWl2   Tcb->RcvNxtTcb->RcvWl2 + 
> > Tcb->RcvWnd
> > Seg->Seq   Seg->End |  |
> > | |   | |  |
> >  
> > ---+-+---+-+--+---
> > 
> >
> > Fu Siyuan (2):
> >   NetworkPkg: Fix bug in TCP which not sending out ACK in certain
> > circumstance.
> >   MdeModulePkg: Fix bug in TCP which not sending out ACK in certain
> > circumstance.
> >
> >  MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Input.c | 2 +-
> >  NetworkPkg/TcpDxe/TcpInput.c   | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > --
> > 2.7.4.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] CryptoPkg: update openssl to ignore RVCT 3079

[Cohen, Eugene]

> > corrects x509_vfy.c(875): error C3017: ok may be used before being
> set
> >
> > Change-Id: I0d38193569b29f96861a191908c343831fd957c2
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Eugene Cohen 
> 
> Can we "fix" the upstream code instead?

No objection here - could I provide a patch that initializes 'ok' and work with 
someone who already contributes to openssl to upstream it?

Eugene

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

[edk2] [PATCH] CryptoPkg: update openssl to ignore RVCT 3079

[Cohen, Eugene]

Getting openssl 1.0.2g building with ARM RVCT requires a change to ignore an 
unset
variable used before set was necessary.

corrects x509_vfy.c(875): error C3017: ok may be used before being set

Change-Id: I0d38193569b29f96861a191908c343831fd957c2
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eugene Cohen 
---
 edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf 
b/edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf
index 8757100..0c3b609 100644
--- a/edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf
+++ b/edk2/CryptoPkg/Library/OpensslLib/OpensslLib.inf
@@ -531,6 +531,7 @@
   #  546: transfer of control bypasses initialization - may be emitted 
inappropriately if the uninitialized
   #   variable is never referenced after the jump
   #1: ignore "#1-D: last line of file ends without a newline"
-  RVCT:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS) 
--library_interface=aeabi_clib99 
--diag_suppress=1296,1295,550,1293,111,68,177,223,144,513,188,128,546,1 
-JCryptoPkg/Include
+  # 3017:  may be used before being set
+  RVCT:*_*_ARM_CC_FLAGS = $(OPENSSL_FLAGS) 
--library_interface=aeabi_clib99 
--diag_suppress=1296,1295,550,1293,111,68,177,223,144,513,188,128,546,1,3017 
-JCryptoPkg/Include
   XCODE:*_*_IA32_CC_FLAGS   = -mmmx -msse -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w
   XCODE:*_*_X64_CC_FLAGS= -mmmx -msse -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w
-- 
1.9.5.msysgit.0

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

Re: [edk2] TCP4: Failure to Acknowledge due to DPC Dispatch Nesting (+PATCH)

[Cohen, Eugene]

> We have been testing a change where nesting of DPCs is prevented -
> in DispatchDPC we check if we are already processing a dispatch and
> exit if we are.  (We also modified the dispatch loop to keep looping
> until the DPC queues are emptied.)  With this approach we still meet
> the same-TPL async execution but avoid the whole mess of nesting.

In our testing the proposed approach of avoiding nested DPCs caused other 
problems, we suspect because there is an expectation in some modules that DPCs 
are handled immediately on DispatchDpc and deferring them until later violated 
this assumption resulting in bad behavior.  Again it would be really helpful if 
the DPC design was documented somewhere - the rules, limitations and 
assumptions are not obvious.

So at this point I'm at a loss - changing DPC semantics breaks existing code 
and TCP4 has structural issues that can corrupt connection state.  I hope we 
can come up with something short of rewriting TCP4 that can resolve this.

Thanks,

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

Re: [edk2] TCP4: Failure to Acknowledge due to DPC Dispatch Nesting

[Cohen, Eugene]

Siyuan,

Thanks for clarifying the intended design.

Given your response I understand that it's considered acceptable for DPC 
functions to preempt each other, even at the same registered TPL level and even 
sometimes with the same exact function (and in the case of TCP operating on the 
exact same connection).  This would seem to present a substantial challenge for 
DPCs to be written correctly because you have to assume that any protocol or 
library you call may result in a DPC dispatch and cause other routines at your 
TPL level (or even another instance of the same function) to be called again.  
Are there restrictions to when DispatchDPC is allowed to be called or can 
anyone do it anytime?  What if, for some evil reason, someone decided to use 
DPCs as part of a specialized DebugLib causing the stack to be preempted 
constantly - do you expect all DPC handlers to behave correctly under these 
circumstances?  Wouldn't it be safer to remove this form of preemption (or at 
least the same-TPL preemption)?

To change the TCP code as you mention, what do you recommend?  Right now we 
have a routine:

  if (TcpTransmitSegment (Tcb, Nbuf) == 0) {
TCP_CLEAR_FLG (Tcb->CtrlFlag, TCP_CTRL_ACK_NOW);
Tcb->DelayedAck = 0;
  }

That attempts the transmit and clears the DelayedAck flag as a function of the 
return status.  Are you recommending that we clear CTRL_ACK_NOW and DelayedAck 
before attempting the transmit?  In the case of an error we would need a way to 
restore the ACK_NOW and DelayedAck these flags which presents the same problem 
again.

But generally I don't think this methodology can be applied to other areas with 
the same exposure.  Consider the TcpToSendData function.  It does this:

if (TcpTransmitSegment (Tcb, Nbuf) != 0) {
NetbufTrim (Nbuf, (Nbuf->Tcp->HeadLen << 2), NET_BUF_HEAD);
Nbuf->Tcp = NULL;

if ((Flag & TCP_FLG_FIN) != 0)  {
  TCP_SET_FLG (Tcb->CtrlFlag, TCP_CTRL_FIN_SENT);
}

goto OnError;
  }

which as you can see sets a flag (FIN_SENT) after a DPC preemption point 
(TcpTransmitSegment) and even later it sets other TCB data after the DPC 
preemption point:

  //
  // update status in TCB
  //
  Tcb->DelayedAck = 0;

  if ((Flag & TCP_FLG_FIN) != 0) {
TCP_SET_FLG (Tcb->CtrlFlag, TCP_CTRL_FIN_SENT);
  }

So it seems like the TCB contents may be at risk in a number of places because 
of DPC preemption.

If we the DPC rules were changed such that DPCs at the same TPL didn't preempt 
each other it would seem to prevent this (to the extent that DPC callbacks use 
a consistent TPL level to provide protection).

Thanks,

Eugene

From: Fu, Siyuan [mailto:siyuan...@intel.com]
Sent: Monday, May 23, 2016 8:45 PM
To: Ye, Ting <ting...@intel.com>; Cohen, Eugene <eug...@hp.com>; 
edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin...@intel.com>; Zhang, Lubo 
<lubo.zh...@intel.com>
Cc: Vaughn, Gregory (IPG LJ Printer Lab) <greg.vau...@hp.com>
Subject: RE: TCP4: Failure to Acknowledge due to DPC Dispatch Nesting

Hi, Eugene

Thanks for your detail analysis to help understanding the problem. We think 
it’s a bug about the DelayedAck flag update in TCP driver, not in DPC, and here 
is the detail.

At the time the TcpTicking() is running, the under layer (MnpSystemPoll) also 
has a timer to receive network packets and queue them to the DPC. In you 
described case, when TcpTickingDpc() calls TcpSendAck() to send a delayed ACK 
to the remote, it first calls TcpTransmitSegment to transmit the ACK (point 1), 
then clear the TCP_CTRL_ACK_NOW and set DelayedAck to zero (point 2) as below.

   TcpTicking
  TcpSendAck
 TcpTransmitSegment (#1)
 Clear Tcb’s flag (#2)

But there are some DispatchDpc between #1 and #2, it not only transmited the 
ACK message to the under layer driver, but also invoked these MNP queued DPC 
and delivered these packets to the upper layer driver (TCP), then set the Tcb’s 
flag. So things come to:

   TcpTicking
  TcpSendAck
 TcpTransmitSegment (#1)
DispatchDpc
   New 
TCP packet arrived, and set the Tcb’s flag (#3)
 Clear Tcb’s flag (#2)

In another word, the TcpSendAck() function transmit the packet first, then 
clear the flag later, while the flag may be set by the incoming data during the 
transmit, so the clear operation is incorrect. The correct to operate the flag 
is to set the flag first, then make transmit. In this way, if the transmit 
operation invoke the DPC and deliver some new data to upper layer, the new set 
flag won’t be override.

Best Regards
Siyuan

From: Ye, Ting
Sent: Tuesday, May 24

Re: [edk2] TCP4: Failure to Acknowledge due to DPC Dispatch Nesting

[Cohen, Eugene]

Adding some maintainers...

We are looking forward to a response.

Thanks,

Eugene

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Cohen, Eugene
> Sent: Friday, May 20, 2016 2:11 PM
> To: edk2-devel@lists.01.org
> Cc: Vaughn, Gregory (IPG LJ Printer Lab) <greg.vau...@hp.com>
> Subject: [edk2] TCP4: Failure to Acknowledge due to DPC Dispatch Nesting
> 
> 
> We have isolated a problem where the TCP4 driver fails to acknowledge
> received data under certain circumstances.
> 
> Background on DPCs: The DPC mechanism allows functions to be called at a
> later point in time at a requested TPL level.  Functions are queued through
> the DPC Protocol's QueueDpc interface.  DPCs are dispatched anytime a
> module calls the DispatchDpc function in the DPC protocol.  The dispatch
> process will execute all queued DPCs that were registered to execute at or
> above the caller's TPL level (e.g. if caller is at TPL_CALLBACK the DPC 
> dispatch
> will execute anything between CALLBACK and HIGH_LEVEL).
> 
> The stack depends on DispatchDpc being called at appropriate preemption
> points to advance packet processing.  The dispatch function is called in
> multiple layers as you can see by searching for DispatchDpc with calls
> originating from ArpDxe, Ip4Dxe, MnpDxe, Udp4Dxe, UefiPxeBcDxe,
> DnsDxe, HttpDxe, Ip6Dxe and Udp6Dxe.
> 
> Currently it's possible for DPC dispatching to occur in a nested manner.
> Imagine a case where an upper network stack layer queues a DPC (for
> example the TCP layer's TcpTickingDpc) and in the DPC handler it makes use
> of lower layers (like sending a packet through IP+MNP).  As part of packet
> processing these lower layers will call DispatchDpc resulting in nested DPCs
> execution.
> 
> 
> Here's an example of the DPC nesting, the indent level indicates the level of
> nesting for the DPC
> 
> TcpTicking
> DpcDispatchDpc TPL=CALLBACK
> TcpTickingDpc
> TcpTickingDpc Tcb: 0x49a42dd0, DelayedAck=1, CtrlFlag: 0x1019
> TcpTickingDpc call TcpSendAck delayed is: 1
> TcpSendAck  Seq=158464588 Ack=4152002304
> TcpTransmitSegment DelayedAck = 0
> Nbuf SendIpPacket: DestPort=62462 Seq=158464588 Ack=4152002304
> Window=19840
> MnpSyncSendPacket call DispatchDpc
> DpcDispatchDpc TPL=CALLBACK
> Ip4AccpetFrame call DispatchDpc
> DpcDispatchDpc TPL=CALLBACK
> Ip4FreeTxToken call DispatchDpc
> DpcDispatchDpc TPL=CALLBACK
> TcpInput: DestPort=8816 Seq=4152002304 Ack=158464588 Len=1460
> TcpClearTimer Tcb: 0x49a42dd0
> 
> Notice how the process of sending the TCP ACK via IP then MNP causes
> another DispatchDpc to occur before the TCP segment transmit call returns.
> This nesting continues on and a whole bunch of code has executed (all at
> CALLBACK TPL).  You can see near the end that we even begin processing
> another TCP packet.
> 
> 
> If we look in detail what the TcpSendAck does there are two key steps:
> 
>   if (TcpTransmitSegment (Tcb, Nbuf) == 0) {
> TCP_CLEAR_FLG (Tcb->CtrlFlag, TCP_CTRL_ACK_NOW);
> Tcb->DelayedAck = 0;
>   }
> 
> It transmits the segment and after the transmit returns it clears the
> DelayedAck counter since the presumption is that we sent an ACK for the
> most recent segment and we are caught up.  But because DPCs are
> dispatched within TcpTransmitSegment the assumption that this transmit
> was the last one is incorrect.
> 
> 
> Here is a portion of a trace illustrating the problem:
> 
> TcpTicking
> DpcDispatchDpc TPL=CALLBACK
> TcpTickingDpc
> TcpTickingDpc Tcb: 0x49a42dd0, DelayedAck=1, CtrlFlag: 0x1019
> TcpTickingDpc call TcpSendAck delayed is: 1
> TcpSndAck  Seq=158464588 Ack=4152002304
> TcpTransmitSegment DelayedAck = 0
> Nbuf SendIpPacket: DestPort=62462 Seq=158464588 Ack=4152002304
> Window=19840
> MnpSyncSendPacket call DispatchDpc
> DpcDispatchDpc TPL=CALLBACK
> 
> [snip - a bunch of nested DPC processing removed]
> 
>  DpcDispatchDpc Tpl= TPL=CALLBACK
>   TcpInput: DestPort=8816 Seq=4152019824 Ack=158464588 Len=1460
>   TcpClearTimer Tcb: 0x49a42dd0
>   TcpInput Seq=4152019824 Tcb: 0x49a42dd0, Tcb->DupAck = 0
>   TcpToSendAck add to delayedack Seq=158464588 Ack=4152021284
>   TcpToSendAck add to delayedack Tcb: 0x49a42dd0, Seq=158464588
> Ack=4152021284, DelayedAck=1
>   ^^ NOTE: the DelayedAck flag has been set to one indicating that we
> haven't acknowledged yet and need to soon
> 
>   [we return from 14 nested DPC calls !!]
> 
> TcpSndAck  Tcb->DelayedAck = 0
>  

[edk2] TCP4: Failure to Acknowledge due to DPC Dispatch Nesting

[Cohen, Eugene]

We have isolated a problem where the TCP4 driver fails to acknowledge received 
data under certain circumstances.

Background on DPCs: The DPC mechanism allows functions to be called at a later 
point in time at a requested TPL level.  Functions are queued through the DPC 
Protocol's QueueDpc interface.  DPCs are dispatched anytime a module calls the 
DispatchDpc function in the DPC protocol.  The dispatch process will execute 
all queued DPCs that were registered to execute at or above the caller's TPL 
level (e.g. if caller is at TPL_CALLBACK the DPC dispatch will execute anything 
between CALLBACK and HIGH_LEVEL).

The stack depends on DispatchDpc being called at appropriate preemption points 
to advance packet processing.  The dispatch function is called in multiple 
layers as you can see by searching for DispatchDpc with calls originating from 
ArpDxe, Ip4Dxe, MnpDxe, Udp4Dxe, UefiPxeBcDxe, DnsDxe, HttpDxe, Ip6Dxe and 
Udp6Dxe.

Currently it's possible for DPC dispatching to occur in a nested manner.  
Imagine a case where an upper network stack layer queues a DPC (for example the 
TCP layer's TcpTickingDpc) and in the DPC handler it makes use of lower layers 
(like sending a packet through IP+MNP).  As part of packet processing these 
lower layers will call DispatchDpc resulting in nested DPCs execution.


Here's an example of the DPC nesting, the indent level indicates the level of 
nesting for the DPC

TcpTicking
DpcDispatchDpc TPL=CALLBACK
TcpTickingDpc
TcpTickingDpc Tcb: 0x49a42dd0, DelayedAck=1, CtrlFlag: 0x1019
TcpTickingDpc call TcpSendAck delayed is: 1
TcpSendAck  Seq=158464588 Ack=4152002304
TcpTransmitSegment DelayedAck = 0
Nbuf SendIpPacket: DestPort=62462 Seq=158464588 Ack=4152002304 Window=19840
MnpSyncSendPacket call DispatchDpc
DpcDispatchDpc TPL=CALLBACK
Ip4AccpetFrame call DispatchDpc
DpcDispatchDpc TPL=CALLBACK
Ip4FreeTxToken call DispatchDpc
DpcDispatchDpc TPL=CALLBACK
TcpInput: DestPort=8816 Seq=4152002304 Ack=158464588 Len=1460
TcpClearTimer Tcb: 0x49a42dd0

Notice how the process of sending the TCP ACK via IP then MNP causes another 
DispatchDpc to occur before the TCP segment transmit call returns.  This 
nesting continues on and a whole bunch of code has executed (all at CALLBACK 
TPL).  You can see near the end that we even begin processing another TCP 
packet.


If we look in detail what the TcpSendAck does there are two key steps:

  if (TcpTransmitSegment (Tcb, Nbuf) == 0) {
TCP_CLEAR_FLG (Tcb->CtrlFlag, TCP_CTRL_ACK_NOW);
Tcb->DelayedAck = 0;
  }

It transmits the segment and after the transmit returns it clears the 
DelayedAck counter since the presumption is that we sent an ACK for the most 
recent segment and we are caught up.  But because DPCs are dispatched within 
TcpTransmitSegment the assumption that this transmit was the last one is 
incorrect.


Here is a portion of a trace illustrating the problem:

TcpTicking
DpcDispatchDpc TPL=CALLBACK
TcpTickingDpc
TcpTickingDpc Tcb: 0x49a42dd0, DelayedAck=1, CtrlFlag: 0x1019
TcpTickingDpc call TcpSendAck delayed is: 1
TcpSndAck  Seq=158464588 Ack=4152002304
TcpTransmitSegment DelayedAck = 0
Nbuf SendIpPacket: DestPort=62462 Seq=158464588 Ack=4152002304 Window=19840
MnpSyncSendPacket call DispatchDpc
DpcDispatchDpc TPL=CALLBACK

[snip - a bunch of nested DPC processing removed]

 DpcDispatchDpc Tpl= TPL=CALLBACK
  TcpInput: DestPort=8816 Seq=4152019824 Ack=158464588 Len=1460
  TcpClearTimer Tcb: 0x49a42dd0
  TcpInput Seq=4152019824 Tcb: 0x49a42dd0, Tcb->DupAck = 0
  TcpToSendAck add to delayedack Seq=158464588 Ack=4152021284
  TcpToSendAck add to delayedack Tcb: 0x49a42dd0, Seq=158464588 
Ack=4152021284, DelayedAck=1
  ^^ NOTE: the DelayedAck flag has been set to one indicating that we 
haven't acknowledged yet and need to soon

  [we return from 14 nested DPC calls !!]

TcpSndAck  Tcb->DelayedAck = 0
^^ But when the Dispatch returns from the TcpTransmitSegment we clear 
DelayedAck back to zero such that we never acknowledge the last packet we 
received.

TcpTickingDpc No timer active or expired
TcpTickingDpc Tcb: 0x49918bd0, DelayedAck=0, CtrlFlag: 0x1019
TcpTickingDpc No timer active or expired

In cases where TCP is waiting for more data from the remote endpoint and the 
endpoint is waiting for acknowledgement (like a situation where the sliding 
window is small or closed) the remote endpoint will retransmit the last packet 
but for reasons I don't yet understand duplicate acks aren't sent (I think 
there's another bug around retransmitting acks but I haven't isolated it).   
The result is that the connection is reset by the remote endpoint after a 
timeout period elapses.


This seems kind of like a critical section / locking problem around the 
DelayedAck member.  I'm not sure if the issue 

[edk2] Shell Path Issues with "../.."

[Cohen, Eugene]

Dear ShellPkg maintainer,

We're seeing some weird stuff related to '..' in the shell.  

The issue is that when we try to cd up two levels using the "../.." notation we 
only go up one level:

FS0:\dir1\dir2\dir3\>
FS0:\dir1\dir2\dir3\> cd ..\..
FS0:\dir1\dir2\>

The same issue occurs using forward and back slashes.  The old (EDK) shell used 
to handle this correctly.

Any ideas what's going on here?

Thanks,

Eugene

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

Re: [edk2] Error while loading a symbol file (No Debug Directory)

[Cohen, Eugene]

> This is very odd. The commit in question introduces two new
> implementations of CpuExceptionHandlerLib, but does not yet use them
> anywhere. The next patch updates CpuDxe to start using the default
> (non-copying) one. What are your resolutions for CpuExceptionHandlerLib in
> your .DSC, and do any of them refer to the libraries added in this patch?

Agreed - from a CpuDxe point of view the change should be transparent - it's 
the same exception handling implemented in a library.  It should have the same 
alignment characteristics as before.  If there was a bug that existing in debug 
scripts for scanning modules or PE-coff extra actions - I would not expect this 
patch to worsen (or improve) the situation.

> > If I do "add-symbol-file.." in the DS-5 Debug console as the exception
> > handler reports, I have an ability to debug (so the address is shifted
> > +0x1 comparing to the proper address) Do you know if  anybody else
> > had this issue on an armv8 64 bit platform after this patch was
> > applied?
> > Please advise what I could be missing...
> >
> 
> Hmm, that looks like a separate issue. Due to the padding required for
> runtime drivers, which have a 64k section alignment, the start of the file and
> the start of the text section are 64k apart, so that output is not unexpected.
> In GDB, add-symbol-file takes the start of the .text section, not the start of
> the image.

Are you saying this also started happening with the commit in question or is 
that something independent?  As Ard says if the debugger needs the start of 
.text then this address may not be correct since the output is generated as 
follows:

DEBUG ((EFI_D_LOAD | EFI_D_INFO, "add-symbol-file %a 0x%p\n", 
DeCygwinPathIfNeeded (ImageContext->PdbPointer, Temp, sizeof (Temp)), 
(UINTN)(ImageContext->ImageAddress + ImageContext->SizeOfHeaders)));

With our debugger (TRACE32) we only emit ImageAddress and don't add 
SizeOfHeaders:

   DEBUG((EFI_D_ERROR, "data.load.elf %a a:0x%08x /NOCODE /NOCLEAR\n", 
ImageContext->PdbPointer, ImageAddress));

The adding of SizeOfHeaders used to be required when the PE-COFF conversion 
would shift the .text section to make room for the PE-COFF header but recent 
changes made the ELF and PE-COFF layout align so that this offset was no longer 
necessary.

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

Re: [edk2] [PATCH 1/7] ArmPkg/AsmMacroIoLibV8: remove undocumented assumption from ELx macros

[Cohen, Eugene]

Alexei and Ard,

> The suggested code for EL1_OR_EL2 macro:
> 
> mrsSAFE_XREG, CurrentEL ;\
> cmpSAFE_XREG, #0x8  ;\
> b.eq   2f   ;\
> tbnz   SAFE_XREG, #2, 1f;\
> b  .;// We should never get here
> 
> will successfully branch to 1f label in case of EL3 with SAFE_XREG = 0xC
> because bit #2 will be set.

Agreed.  If you assume that EL1_OR_EL2 can never be invoked at EL3 then 
obviously this wouldn't be a concern.  But since there's already a case for 
handling neither EL1 nor EL2 (b .) then it would make sense to correct this so 
if it was accidentally used in EL3 it would hit the default case which is 
better for debugability.  The last tbnz needs to be reverted to the previous 
cmp/b.ne construct or equivalent.



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

Re: [edk2] [PATCH 2/7] ArmPkg/ArmExceptionLib: fold exception handler prologue into vector table

[Cohen, Eugene]

> So refactor this code into a single macro, and expand it into each vector 
> table
> slot.

 
> +  .macro  ExceptionEntry, val
> +  // Move the stackpointer so we can reach our structure with the str
> instruction.
> +  sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
> +
> +  // Save all the General regs before touching x0 and x1.
> +  // This does not save r31(SP) as it is special. We do that later.
> +  ALL_GP_REGS
> +
> +  // Record the type of exception that occurred.
> +  mov   x0, #\val
> +
> +  // Jump to our general handler to deal with all the common parts and
> process the exception.
> +  ldr   x1, =ASM_PFX(CommonExceptionEntry)
> +  brx1
> +  .ltorg
> +  .endm

The simplification to push the context save into the vector slot looks good to 
me.

Although I see why you defined a macro for saving exception context (avoiding 
duplication) I'm not sure if this is a good idea.  I'm envisioning stepping 
through the exception handling with a debugger and the resulting confusion 
because of the code hiding behind the macro.  My preference would be to 
tolerate the duplication (it's just 5 lines of assembly) in favor of 
readability / debuggability.

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

Re: [edk2] [PATCH 5/7] ArmPkg/ArmExceptionLib: make build time define visible to the compiler

[Cohen, Eugene]

I have to admit that on my previous patchset I was saddened that I had to 
resort to a #define to solve this since it's counter to the codebase convention 
but this was the only way to get conditional assembly of the .align directive.  
Since inline #ifdefs were also counter to the codebase convention I figured a 
global initialized conditionally would be more familiar (like a boolean PCD) 
but I hadn't considered the optimization impact.

Reviewed-by: Eugene Cohen <eug...@hp.com>

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Thursday, March 17, 2016 7:20 AM
> To: edk2-devel@lists.01.org; leif.lindh...@linaro.org; Cohen, Eugene
> <eug...@hp.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Subject: [edk2] [PATCH 5/7] ArmPkg/ArmExceptionLib: make build time
> define visible to the compiler
> 
> The global gArmRelocateVectorTable is a build time constant, but due to its
> external linkage and lack of constness, the compiler does not see that.
> So turn it into a static boolean, and at the same time, make the function
> CopyExceptionHandlers() (which is only called if gArmRelocateVectorTable is
> set) static as well, so that the compiler can eliminate it completely if we 
> are
> using the vector table in place.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>  ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c
> b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c
> index 5977a3e8fae1..0cf0766b9cbf 100644
> --- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c
> +++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
> 
> +STATIC
>  RETURN_STATUS
>  CopyExceptionHandlers(
>IN  PHYSICAL_ADDRESSBaseAddress
> @@ -66,9 +67,9 @@ extern UINTNgDebuggerNoHandlerValue;
>  // library we cannot represent this in a PCD since PCDs are evaluated on  // 
> a
> per-module basis.
>  #if defined(ARM_RELOCATE_VECTORS)
> -BOOLEAN gArmRelocateVectorTable = TRUE;
> +STATIC CONST BOOLEAN gArmRelocateVectorTable = TRUE;
>  #else
> -BOOLEAN gArmRelocateVectorTable = FALSE;
> +STATIC CONST BOOLEAN gArmRelocateVectorTable = FALSE;
>  #endif
> 
> 
> @@ -151,6 +152,7 @@ with default exception handlers.
>  @retval EFI_UNSUPPORTED   This function is not supported.
> 
>  **/
> +STATIC
>  RETURN_STATUS
>  CopyExceptionHandlers(
>IN  PHYSICAL_ADDRESSBaseAddress
> --
> 2.5.0
> 
> ___
> 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/7] ArmPkg/AsmMacroIoLibV8: remove undocumented assumption from ELx macros

[Cohen, Eugene]

Thanks - I agree, the fallthrough was totally non-obvious and inconsistent with 
other cases.  The whole '1f' syntax is pretty odd as well - I had to dig deep 
to find out what the locally defined numbered forward-only labels were.

Reviewed-by: Eugene Cohen <eug...@hp.com>


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, March 17, 2016 7:20 AM
> To: edk2-devel@lists.01.org; leif.lindh...@linaro.org; Cohen, Eugene
> <eug...@hp.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Subject: [PATCH 1/7] ArmPkg/AsmMacroIoLibV8: remove undocumented
> assumption from ELx macros
> 
> The macros EL1_OR_EL2() and EL1_OR_EL2_OR_EL3() allow conditional
> execution of assembly sequences based on the current exception level, by
> jumping to caller supplied labels 1f, 2f or 3f. However, the jump to 1f is
> actually a fallthrough, which means the EL1 code needs to follow right after
> the macro invocation, and the 1f label is ignored.
> 
> So let's fix this by making all jumps explicit.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>  ArmPkg/Include/AsmMacroIoLibV8.h | 15 +++
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h
> b/ArmPkg/Include/AsmMacroIoLibV8.h
> index a9f8491bc922..efc47d3bbbc7 100644
> --- a/ArmPkg/Include/AsmMacroIoLibV8.h
> +++ b/ArmPkg/Include/AsmMacroIoLibV8.h
> @@ -25,9 +25,9 @@
>  mrsSAFE_XREG, CurrentEL ;\
>  cmpSAFE_XREG, #0x8  ;\
>  b.eq   2f   ;\
> -cmpSAFE_XREG, #0x4  ;\
> -b.ne   .;// We should never get here
> -// EL1 code starts here
> +tbnz   SAFE_XREG, #2, 1f;\
> +b  .;// We should never get here
> +
> 
>  // CurrentEL : 0xC = EL3; 8 = EL2; 4 = EL1  // This only selects between EL1 
> and
> EL2 and EL3, else we die.
> @@ -36,11 +36,10 @@
>  mrsSAFE_XREG, CurrentEL ;\
>  cmpSAFE_XREG, #0xC  ;\
>  b.eq   3f   ;\
> -cmpSAFE_XREG, #0x8  ;\
> -b.eq   2f   ;\
> -cmpSAFE_XREG, #0x4  ;\
> -b.ne   .;// We should never get here
> -// EL1 code starts here
> +tbnz   SAFE_XREG, #3, 2f;\
> +tbnz   SAFE_XREG, #2, 1f;\
> +b  .;// We should never get here
> +
>  #if defined(__clang__)
> 
>  // load x0 with _Data
> --
> 2.5.0

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

Re: [edk2] [PATCH v2 3/3] ArmPkg: Update CpuDxe to use CpuExceptionHandlerLib

[Cohen, Eugene]

Ard,

> The patches look fine to me. I will proceed and apply the first two.
> However, this third one needs to wait until all users of CpuDxe have
> the correct resolution for CpuExceptionHandlerLib. I think this is
> only ArmVirtPkg/ArmVirt.dsc.inc, but I'd like a nod from Leif before
> pushing this final change through.

By my analysis across edk2 and OpenPlatformPkg the following platforms need to 
get updated to pick up the library to prevent build breakage:

For edk2 it's ArmVirtQemu, ArmVirtQemuKernel, ArmVirtXen, and BeagleBoardPkg
For OpenPlatformPkg it's ArmJuno, ArmVExpress-CTA15-A7, 
ArmVExpress-FVP-AArch64.dsc, and BeagleBoardPkg

This was obtained just by searching for platforms that use the ARM CpuDxe 
driver.

Thanks,

Eugene

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

Re: [edk2] [PATCH 1/2] BeagleBoardPkg: move to ARM version of CpuExceptionHandlerLib

[Cohen, Eugene]

Series Reviewed-by: Eugene Cohen 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> Of Ard Biesheuvel
> Sent: Wednesday, March 16, 2016 8:08 AM
> To: edk2-devel@lists.01.org; leif.lindh...@linaro.org
> Cc: Ard Biesheuvel 
> Subject: [edk2] [PATCH 1/2] BeagleBoardPkg: move to ARM version of
> CpuExceptionHandlerLib
> 
> Change our resolution for the previously unused
> CpuExceptionHandlerLib
> from the null implementation to the newly added implementation
> specific
> to AARCH64 and ARM. This is needed since our CpuDxe will start using
> it
> in a subsequent patch.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  BeagleBoardPkg/BeagleBoardPkg.dsc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BeagleBoardPkg/BeagleBoardPkg.dsc
> b/BeagleBoardPkg/BeagleBoardPkg.dsc
> index 37e285af1b25..17f9f8e7d5ad 100644
> --- a/BeagleBoardPkg/BeagleBoardPkg.dsc
> +++ b/BeagleBoardPkg/BeagleBoardPkg.dsc
> @@ -84,7 +84,7 @@ [LibraryClasses.common]
> 
> 
> CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/Ar
> mCacheMaintenanceLib.inf
> 
> DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandler
> Lib/DefaultExceptionHandlerLib.inf
> -
> CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandl
> erLibNull/CpuExceptionHandlerLibNull.inf
> +
> CpuExceptionHandlerLib|ArmPkg/Library/ArmExceptionLib/ArmExcep
> tionLib.inf
>PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
> 
>SerialPortLib|Omap35xxPkg/Library/SerialPortLib/SerialPortLib.inf
> --
> 2.5.0
> 
> ___
> 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

[edk2] [PATCH v2 2/3] ArmPkg: ARM/AArch64 implementation of CpuExceptionHandlerLib

[Cohen, Eugene]

Introduce ARM and AArch64 instances of the  CpuExceptionHandlerLib
which provides exception handling and registration of handlers
regardless of execution phase.

Two variants of the ArmExceptionLib are provided: one where
exception handlers reside within the module (meeting appropriate
architectural alignment requirements for the vector table) and
another one that will relocate a copy of exception handlers to
an address specified by PcdCpuVectorBaseAddress.  The
ArmRelocateExceptionLib is intended for use in cases where
ArmExceptionLib is too large for the application (uncompressed
XIP images) as driven by the vector table alignment padding.

The AArch64 build of this library supports execution at
EL1, EL2, and EL3 exception levels.

Tested on ARM, and AArch64 with SEC, DXE Core, and CpuDxe
modules.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eugene Cohen 
---
 ArmPkg/ArmPkg.dsc  |   3 +
 .../ArmExceptionLib/AArch64/AArch64Exception.c |  44 +++
 .../ArmExceptionLib/AArch64/ExceptionSupport.S | 425 +
 ArmPkg/Library/ArmExceptionLib/Arm/ArmException.c  |  50 +++
 .../Library/ArmExceptionLib/Arm/ExceptionSupport.S | 305 +++
 .../ArmExceptionLib/Arm/ExceptionSupport.asm   | 302 +++
 ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c   | 320 
 ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf |  63 +++
 .../ArmExceptionLib/ArmRelocateExceptionLib.inf|  65 
 9 files changed, 1577 insertions(+)
 create mode 100644 ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
 create mode 100644 ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
 create mode 100644 ArmPkg/Library/ArmExceptionLib/Arm/ArmException.c
 create mode 100644 ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.S
 create mode 100644 ArmPkg/Library/ArmExceptionLib/Arm/ExceptionSupport.asm
 create mode 100644 ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c
 create mode 100644 ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
 create mode 100644 ArmPkg/Library/ArmExceptionLib/ArmRelocateExceptionLib.inf

diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index 688244b..df5be6e 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -63,6 +63,7 @@
   
UncachedMemoryAllocationLib|ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
   
DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
   
DefaultExceptionHandlerLib|ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
+  CpuExceptionHandlerLib|ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
 
   CpuLib|MdePkg/Library/BaseCpuLib/BaseCpuLib.inf
   ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
@@ -122,6 +123,8 @@
   ArmPkg/Library/SemihostLib/SemihostLib.inf
   ArmPkg/Library/UncachedMemoryAllocationLib/UncachedMemoryAllocationLib.inf
   ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.inf
+  ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
+  ArmPkg/Library/ArmExceptionLib/ArmRelocateExceptionLib.inf
 
   ArmPkg/Drivers/CpuDxe/CpuDxe.inf
   ArmPkg/Drivers/CpuPei/CpuPei.inf
diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c 
b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
new file mode 100644
index 000..b005a78
--- /dev/null
+++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
@@ -0,0 +1,44 @@
+/** @file
+*  Exception Handling support specific for AArch64
+*
+*  Copyright (c) 2016 HP Development Company, L.P.
+*
+*  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 
+
+#include  // for MAX_AARCH64_EXCEPTION
+
+UINTN   gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
+EFI_EXCEPTION_CALLBACK  gExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] = { 0 };
+EFI_EXCEPTION_CALLBACK  gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] 
= { 0 };
+PHYSICAL_ADDRESSgExceptionVectorAlignmentMask = 
ARM_VECTOR_TABLE_ALIGNMENT;
+UINTN   gDebuggerNoHandlerValue = 0; // todo: define for 
AArch64
+
+RETURN_STATUS ArchVectorConfig(
+  IN  UINTN   VectorBaseAddress
+  )
+{
+  UINTN HcrReg;
+  
+  if (ArmReadCurrentEL() == AARCH64_EL2) {
+HcrReg = ArmReadHcr();
+
+// Trap General Exceptions. All exceptions that would be routed to EL1 are 
routed to EL2
+HcrReg |= ARM_HCR_TGE;
+
+ArmWriteHcr(HcrReg);
+  }
+
+  return RETURN_SUCCESS;
+}
diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S 
b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S

[edk2] [PATCH v2 3/3] ArmPkg: Update CpuDxe to use CpuExceptionHandlerLib

[Cohen, Eugene]

Use the new ARM/AArch64 implementation of the base
CpuExceptionHandlerLib library from CpuDxe to centralize
exception handling.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eugene Cohen 
---
 ArmPkg/Drivers/CpuDxe/AArch64/Exception.c| 154 -
 ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S | 402 ---
 ArmPkg/Drivers/CpuDxe/Arm/Exception.c| 234 -
 ArmPkg/Drivers/CpuDxe/Arm/ExceptionSupport.S | 304 -
 ArmPkg/Drivers/CpuDxe/Arm/ExceptionSupport.asm   | 301 -
 ArmPkg/Drivers/CpuDxe/CpuDxe.inf |  10 +-
 ArmPkg/Drivers/CpuDxe/Exception.c|  95 ++
 7 files changed, 98 insertions(+), 1402 deletions(-)
 delete mode 100644 ArmPkg/Drivers/CpuDxe/AArch64/Exception.c
 delete mode 100644 ArmPkg/Drivers/CpuDxe/AArch64/ExceptionSupport.S
 delete mode 100644 ArmPkg/Drivers/CpuDxe/Arm/Exception.c
 delete mode 100644 ArmPkg/Drivers/CpuDxe/Arm/ExceptionSupport.S
 delete mode 100644 ArmPkg/Drivers/CpuDxe/Arm/ExceptionSupport.asm
 create mode 100644 ArmPkg/Drivers/CpuDxe/Exception.c

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Exception.c 
b/ArmPkg/Drivers/CpuDxe/AArch64/Exception.c
deleted file mode 100644
index ce1c6ce..000
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Exception.c
+++ /dev/null
@@ -1,154 +0,0 @@
-/** @file
-
-  Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
-  Portions Copyright (c) 2011 - 2014, 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 "CpuDxe.h"
-
-#include 
-
-VOID
-ExceptionHandlersStart (
-  VOID
-  );
-
-VOID
-ExceptionHandlersEnd (
-  VOID
-  );
-
-VOID
-CommonExceptionEntry (
-  VOID
-  );
-
-VOID
-AsmCommonExceptionEntry (
-  VOID
-  );
-
-
-EFI_EXCEPTION_CALLBACK  gExceptionHandlers[MAX_AARCH64_EXCEPTION + 1];
-EFI_EXCEPTION_CALLBACK  gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1];
-
-
-
-/**
-  This function registers and enables the handler specified by 
InterruptHandler for a processor
-  interrupt or exception type specified by InterruptType. If InterruptHandler 
is NULL, then the
-  handler for the processor interrupt or exception type specified by 
InterruptType is uninstalled.
-  The installed handler is called once for each processor interrupt or 
exception.
-
-  @param  InterruptTypeA pointer to the processor's current interrupt 
state. Set to TRUE if interrupts
-   are enabled and FALSE if interrupts are disabled.
-  @param  InterruptHandler A pointer to a function of type 
EFI_CPU_INTERRUPT_HANDLER that is called
-   when a processor interrupt occurs. If this 
parameter is NULL, then the handler
-   will be uninstalled.
-
-  @retval EFI_SUCCESS   The handler for the processor interrupt was 
successfully installed or uninstalled.
-  @retval EFI_ALREADY_STARTED   InterruptHandler is not NULL, and a handler 
for InterruptType was
-previously installed.
-  @retval EFI_INVALID_PARAMETER InterruptHandler is NULL, and a handler for 
InterruptType was not
-previously installed.
-  @retval EFI_UNSUPPORTED   The interrupt specified by InterruptType is 
not supported.
-
-**/
-EFI_STATUS
-RegisterInterruptHandler (
-  IN EFI_EXCEPTION_TYPE InterruptType,
-  IN EFI_CPU_INTERRUPT_HANDLER  InterruptHandler
-  )
-{
-  if (InterruptType > MAX_AARCH64_EXCEPTION) {
-return EFI_UNSUPPORTED;
-  }
-
-  if ((InterruptHandler != NULL) && (gExceptionHandlers[InterruptType] != 
NULL)) {
-return EFI_ALREADY_STARTED;
-  }
-
-  gExceptionHandlers[InterruptType] = InterruptHandler;
-
-  return EFI_SUCCESS;
-}
-
-
-
-VOID
-EFIAPI
-CommonCExceptionHandler (
-  IN EFI_EXCEPTION_TYPE   ExceptionType,
-  IN OUT EFI_SYSTEM_CONTEXT   SystemContext
-  )
-{
-  if (ExceptionType <= MAX_AARCH64_EXCEPTION) {
-if (gExceptionHandlers[ExceptionType]) {
-  gExceptionHandlers[ExceptionType] (ExceptionType, SystemContext);
-  return;
-}
-  } else {
-DEBUG ((EFI_D_ERROR, "Unknown exception type %d from %016lx\n", 
ExceptionType, SystemContext.SystemContextAArch64->ELR));
-ASSERT (FALSE);
-  }
-
-  DefaultExceptionHandler (ExceptionType, SystemContext);
-}
-
-
-
-EFI_STATUS
-InitializeExceptions (
-  IN EFI_CPU_ARCH_PROTOCOL*Cpu
-  )
-{
-  EFI_STATUS   Status;
-  BOOLEAN  IrqEnabled;
-  BOOLEAN  FiqEnabled;
-
-  Status = EFI_SUCCESS;
-  ZeroMem 

Re: [edk2] [PATCH 0/3] Implement ARM/AArch64 instance of CpuExceptionHandlerLib

[Cohen, Eugene]

Here's the second revision of the patch series with these changes:

1. Remove the unnecessary patching of the CommonExceptionEntry since the linker 
already does it
2. Remove a duplicate write to VBAR in the case where exception handlers are 
relocated

Both of these only affect Patch 2/3, 1 and 3 remain unchanged.

Eugene

> -Original Message-
> From: Cohen, Eugene
> Sent: Thursday, March 03, 2016 4:05 PM
> To: edk2-devel@lists.01.org; 'Ard Biesheuvel'
> <ard.biesheu...@linaro.org>; Leif Lindholm <leif.lindh...@linaro.org>
> Subject: [PATCH 0/3] Implement ARM/AArch64 instance of
> CpuExceptionHandlerLib
> 
> This patch series refactors the exception handling in ArmPkg CpuDxe
> to a base library implementing the existing CpuExceptionHandlerLib
> interface.  This interface allows for exception handling in any phase of
> execution so we can get exception handlers in place early for effective
> debugging.
> 
> The issue raised earlier around EL2 taking over exception/interrupt
> handling (old subject was "ArmPkg: AArch64 exception handling init
> configures HCR in EL2") is now handled in this patchset across patches 1
> and 2.
> 
> These changes have been tested on AArch64 and ARM platforms.
> Testing consisted of inducing exceptions at SEC and DxeMain (after the
> CpuExceptionHandlerLib is initialized and before CpuDxe is loaded), as
> well as ensuring correct timer tick / IRQ interrupts delivery.
> 
> Read the patchset description of patch 2 about why there are two
> instances of the ArmExceptionLib.  The short answer is that it offers a
> choice between self-contained exception handlers (ArmExceptionLib)
> and optimizing for code size (ArmRelocateExceptionLib).
> 
> 
> NOTE:  Platforms that use the ArmPkg CpuDxe will need their DSC files
> updated to include a non-null instance of the CpuExceptionHandlerLib.
> For DXE phase components which are not as size sensitive the self-
> contained variant should suffice:
> 
> 
> CpuExceptionHandlerLib|ArmPkg/Library/ArmExceptionLib/ArmExcep
> tionLib.inf
> 
> The following platforms will need their DSC files updated with this
> instance for the build to succeed since they use ArmPkg CpuDxe:
> 
> edk2: ArmVirtQemu, ArmVirtQemuKernel, ArmVirtXen, and
> BeagleBoardPkg
> OpenPlatformPkg: ArmJuno, ArmVExpress-CTA15-A7, ArmVExpress-
> FVP-AArch64.dsc, and BeagleBoardPkg
> 
> [please excuse me not generating patches for each of these platforms,
> I hope you understand]
> 
> 
> Thanks,
> 
> Eugene

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

[edk2] [PATCH v2 1/3] ArmPkg: Add ArmReadHcr to enable read-modify-write of HCR

[Cohen, Eugene]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eugene Cohen 
---
 ArmPkg/Include/Chipset/AArch64.h   | 5 +
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 6 ++
 2 files changed, 11 insertions(+)

diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
index e53605f..aa6a7e0 100644
--- a/ArmPkg/Include/Chipset/AArch64.h
+++ b/ArmPkg/Include/Chipset/AArch64.h
@@ -204,6 +204,11 @@ ArmWriteHcr (
   );
 
 UINTN
+ArmReadHcr (
+  VOID
+  );
+
+UINTN
 ArmReadCurrentEL (
   VOID
   );
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S 
b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index db21f73..1a3023b 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -54,6 +54,7 @@ GCC_ASM_EXPORT (ArmIsArchTimerImplemented)
 GCC_ASM_EXPORT (ArmReadIdPfr0)
 GCC_ASM_EXPORT (ArmReadIdPfr1)
 GCC_ASM_EXPORT (ArmWriteHcr)
+GCC_ASM_EXPORT (ArmReadHcr)
 GCC_ASM_EXPORT (ArmReadCurrentEL)
 
 .set CTRL_M_BIT,  (1 << 0)
@@ -470,6 +471,11 @@ ASM_PFX(ArmWriteHcr):
   msr   hcr_el2, x0// Write the passed HCR value
   ret
 
+// UINTN ArmReadHcr(VOID)
+ASM_PFX(ArmReadHcr):
+  mrs   x0, hcr_el2
+  ret
+
 // UINTN ArmReadCurrentEL(VOID)
 ASM_PFX(ArmReadCurrentEL):
   mrs   x0, CurrentEL
-- 
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 2/3] ArmPkg: ARM/AArch64 implementation of CpuExceptionHandlerLib

[Cohen, Eugene]

Ard,

> > +  //
> > +  // Patch in the common Assembly exception handler
> > +  //
> > +  Offset = (UINTN)CommonExceptionEntry -
> (UINTN)ExceptionHandlersStart;
> > +  *(UINTN *)((UINT8 *)(UINTN)(BaseAddress + Offset)) =
> (UINTN)AsmCommonExceptionEntry;
> > +
> 
> Why is this needed? CommonExceptionEntry should already contain the
> absolute address of AsmCommonExceptionEntry, this is taken care of by
> the linker and/or the PE/COFF relocation (exactly the same as how your
> reference here got its actual value)

The silly answer is that this is what was already done in 
ArmPkg/Drivers/CpuDxe/Arm/Exception.c that I leveraged this from.  :)  This 
goes back to the dawn of time (BeagleBoard and first ARM support in edk2 in 
2009 contributed by Andrew Fish).  Back then CommonExceptionEntry needed to be 
patched at runtime because it just contained a placeholder at build time:

ASM_PFX(CommonExceptionEntry):
  .byte   0x12
  .byte   0x34
  .byte   0x56
  .byte   0x78

in 2011 a patch came in for the ARMv6 exception handling that changed this to 
populate the pointer with the address at build time:

ASM_PFX(CommonExceptionEntry):
  .word   ASM_PFX(AsmCommonExceptionEntry)

This came in for ARMv6 but the ARMv4 exception handling file was not updated.  
The ARMv4 exception handling was removed last December.

So historically there was a reason for it and now it no longer applies.  So 
I'll generate an updated patchset that removes this.

Nice catch.

Eugene


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