Re: [edk2] [PATCH v2 0/8] RFC: ovmf: preliminary TPM2 support

2018-03-11 Thread Shi, Steven 
It works in my side after specify the full path to swtpm tpmemu.sock in qemu 
command options. Thanks!


Steven Shi
Intel\SSG\STO\UEFI Firmware

Tel: +86 021-61166522
iNet: 821-6522

> -Original Message-
> From: Stefan Berger [mailto:stef...@linux.vnet.ibm.com]
> Sent: Friday, March 9, 2018 9:54 PM
> To: Shi, Steven ; Marc-André Lureau
> 
> Cc: edk2-devel@lists.01.org; ler...@redhat.com; pjo...@redhat.com; Yao,
> Jiewen ; qemu-de...@nongnu.org;
> javi...@redhat.com
> Subject: Re: [edk2] [PATCH v2 0/8] RFC: ovmf: preliminary TPM2 support
> 
> On 03/08/2018 10:03 PM, Shi, Steven wrote:
> > Hi Marcandre,
> > Thanks for your command steps and I tried them, but my qemu failed to
> connect the socket tpmemu.sock. When I added the control channel to the
> TPM, the swtpm socket command stuck there and never exit. Not sure
> whether it was successful.
> > Below are the command steps running output in my side
> >
> >> Then you can run:
> >> mkdir tpmstatedir
> >> swtpm_setup.sh --tpm2 --tpm-state tpmstatedir
> > $ swtpm_setup.sh --tpm2 --tpm-state tpmstatedir
> > Starting vTPM manufacturing as jshi19:jshi19 @ 2018年03月09日 星期
> 五 10时28分39秒
> > TPM is listening on TCP port 47364.
> > Successfully authored TPM state.
> > Ending vTPM manufacturing @ 2018年03月09日 星期五 10时28分39
> 秒
> >
> >> Run the emulator:
> >> swtpm socket --tpmstate dir=tpmstatedir --ctrl
> type=unixio,path=tpmemu.sock  --tpm2
> > $ swtpm socket --tpmstate dir=tpmstatedir --ctrl
> type=unixio,path=tpmemu.sock --tpm2
> > (the swtpm socket command stuck there and never exit)
> >
> >> Run qemu (from git) with ovmf (with this series):
> >> qemu ... -chardev socket,id=chrtpm,path=tpmemu.sock -tpmdev
> >> emulator,id=tpm0,chardev=chrtpm  -device tpm-crb,tpmdev=tpm0
> >> -drive if=pflash,format=raw,file=OVMF_CODE.fd,readonly -drive
> >> if=pflash,format=raw,file=OVMF_VARS.fd ..
> > $ qemu-system-x86_64  -serial file:serial.log -m 5120 -hda fat:. -monitor
> stdio --enable-kvm -smp 4 -bios ../Ovmf3264/NOOPT_GCC5/FV/OVMF.fd -
> chardev socket,id=chrtpm,path=tpmemu.sock -tpmdev
> emulator,id=tpm0,chardev=chrtpm  -device tpm-crb,tpmdev=tpm0
> > qemu-system-x86_64: -chardev socket,id=chrtpm,path=tpmemu.sock:
> Failed to connect socket tpmemu.sock: No such file or directory
> 
> Try giving it both, swtpm and qemu, the full path to the socket.
> 
> 
> >
> > I use the latest version qemu as below:
> > $ qemu-system-x86_64 --version
> > QEMU emulator version 2.11.50 (v2.10.0-4184-g930b01138b-dirty)
> > Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
> >
> > Thanks
> > Steven Shi
> >

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

Re: [edk2] [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after supporting IOMMU

2018-03-11 Thread Zeng, Star 
I have handled the mistake at 
https://github.com/lzeng14/edk2/tree/DebugCommUsb3AfterIOMMUV2_WIP.

If you need, I can resend V2 patch. :)

Thanks,
Star
-Original Message-
From: Zeng, Star 
Sent: Monday, March 12, 2018 10:28 AM
To: Wu, Hao A ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Ni, Ruiyu ; Zeng, 
Star 
Subject: RE: [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after 
supporting IOMMU

Hao,

Good catch. :)
It is a mistake when splitting patch.


Thanks,
Star
-Original Message-
From: Wu, Hao A
Sent: Monday, March 12, 2018 10:10 AM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Ni, Ruiyu 
Subject: RE: [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after 
supporting IOMMU

One minor comment:
For patch 3, within changes in function Usb3PciIoNotify():

Usb3NamedEventListen (
  ,
  TPL_NOTIFY,
  Usb3DxeSmmReadyToLockNotify,
<---  Does this change related with patch 2?
  );

Otherwise, for me, patch 2 is storing the event for DxeSmmReadyToLockProtocol 
in an event which will be closed right after.


Best Regards,
Hao Wu


> -Original Message-
> From: Zeng, Star
> Sent: Sunday, March 11, 2018 11:16 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star; Yao, Jiewen; Ni, Ruiyu; Wu, Hao A
> Subject: [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements 
> after supporting IOMMU
> 
> Please get detailed information in the separated patches.
> 
> Cc: Jiewen Yao 
> Cc: Ruiyu Ni 
> Cc: Hao Wu 
> 
> Star Zeng (3):
>   SourceLevelDebugPkg DebugCommUsb3: Refine some formats
>   SourceLevelDebugPkg DebugCommUsb3: Realloc granted DXE DMA buffer
>   SourceLevelDebugPkg DebugCommUsb3: Use the Handle from DebugAgentLib
> 
>  .../DebugCommunicationLibUsb3Common.c  |  96 +-
>  .../DebugCommunicationLibUsb3Dxe.c | 365 
> -
>  .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
>  .../DebugCommunicationLibUsb3Internal.h|  60 +---
>  .../DebugCommunicationLibUsb3Pei.c |  50 ++-
>  .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
>  6 files changed, 332 insertions(+), 254 deletions(-)
> 
> --
> 2.13.3.windows.1

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

Re: [edk2] [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after supporting IOMMU

2018-03-11 Thread Zeng, Star 
Hao,

Good catch. :)
It is a mistake when splitting patch.


Thanks,
Star
-Original Message-
From: Wu, Hao A 
Sent: Monday, March 12, 2018 10:10 AM
To: Zeng, Star ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Ni, Ruiyu 
Subject: RE: [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after 
supporting IOMMU

One minor comment:
For patch 3, within changes in function Usb3PciIoNotify():

Usb3NamedEventListen (
  ,
  TPL_NOTIFY,
  Usb3DxeSmmReadyToLockNotify,
<---  Does this change related with patch 2?
  );

Otherwise, for me, patch 2 is storing the event for DxeSmmReadyToLockProtocol 
in an event which will be closed right after.


Best Regards,
Hao Wu


> -Original Message-
> From: Zeng, Star
> Sent: Sunday, March 11, 2018 11:16 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star; Yao, Jiewen; Ni, Ruiyu; Wu, Hao A
> Subject: [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements 
> after supporting IOMMU
> 
> Please get detailed information in the separated patches.
> 
> Cc: Jiewen Yao 
> Cc: Ruiyu Ni 
> Cc: Hao Wu 
> 
> Star Zeng (3):
>   SourceLevelDebugPkg DebugCommUsb3: Refine some formats
>   SourceLevelDebugPkg DebugCommUsb3: Realloc granted DXE DMA buffer
>   SourceLevelDebugPkg DebugCommUsb3: Use the Handle from DebugAgentLib
> 
>  .../DebugCommunicationLibUsb3Common.c  |  96 +-
>  .../DebugCommunicationLibUsb3Dxe.c | 365 
> -
>  .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
>  .../DebugCommunicationLibUsb3Internal.h|  60 +---
>  .../DebugCommunicationLibUsb3Pei.c |  50 ++-
>  .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
>  6 files changed, 332 insertions(+), 254 deletions(-)
> 
> --
> 2.13.3.windows.1

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

[edk2] [Patch] MdeModulePkg/Mtftp4Dxe: Fix the incorrect return status.

2018-03-11 Thread Jiaxin Wu 
The incorrect return status was caused by the commit of 39b0867d, which
was to resolve the token status error that does not compliance with spec
definition, but it results the protocol status not compliance with spec
definition.

This patch is to resolve above issue.

Cc: Wang Fan 
Cc: Fu Siyuan 
Cc: Ye Ting 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 .../Universal/Network/Mtftp4Dxe/Mtftp4Impl.c   | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c 
b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
index d8c48ec8b2..065528c937 100644
--- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
+++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.c
@@ -364,10 +364,11 @@ Mtftp4Start (
   MTFTP4_PROTOCOL   *Instance;
   EFI_MTFTP4_OVERRIDE_DATA  *Override;
   EFI_MTFTP4_CONFIG_DATA*Config;
   EFI_TPL   OldTpl;
   EFI_STATUSStatus;
+  EFI_STATUSTokenStatus;
 
   //
   // Validate the parameters
   //
   if ((This == NULL) || (Token == NULL) || (Token->Filename == NULL) ||
@@ -391,28 +392,28 @@ Mtftp4Start (
 return EFI_INVALID_PARAMETER;
   }
 
   Instance = MTFTP4_PROTOCOL_FROM_THIS (This);
 
-  Status = EFI_SUCCESS;
+  Status  = EFI_SUCCESS;
+  TokenStatus = EFI_SUCCESS;
+  
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (Instance->State != MTFTP4_STATE_CONFIGED) {
 Status = EFI_NOT_STARTED;
   }
 
   if (Instance->Operation != 0) {
 Status = EFI_ACCESS_DENIED;
   }
 
-  if (EFI_ERROR (Status)) {
-gBS->RestoreTPL (OldTpl);
-return Status;
-  }
-
   if ((Token->OverrideData != NULL) && !Mtftp4OverrideValid (Instance, 
Token->OverrideData)) {
 Status = EFI_INVALID_PARAMETER;
+  }
+
+  if (EFI_ERROR (Status)) {
 gBS->RestoreTPL (OldTpl);
 return Status;
   }
 
   //
@@ -429,11 +430,11 @@ Mtftp4Start (
TRUE,
>RequestOption
);
 
 if (EFI_ERROR (Status)) {
-  Status = EFI_DEVICE_ERROR;
+  TokenStatus = EFI_DEVICE_ERROR;
   goto ON_ERROR;
 }
   }
 
   //
@@ -482,13 +483,12 @@ Mtftp4Start (
 
   //
   // Config the unicast UDP child to send initial request
   //
   Status = Mtftp4ConfigUnicastPort (Instance->UnicastPort, Instance);
-
   if (EFI_ERROR (Status)) {
-Status = EFI_DEVICE_ERROR;
+TokenStatus = EFI_DEVICE_ERROR;
 goto ON_ERROR;
   }
 
   //
   // Set initial status.
@@ -503,11 +503,11 @@ Mtftp4Start (
   } else {
 Status = Mtftp4RrqStart (Instance, Operation);
   }
 
   if (EFI_ERROR (Status)) {
-Status = EFI_DEVICE_ERROR;
+TokenStatus = EFI_DEVICE_ERROR;
 goto ON_ERROR;
   }
 
   if (Token->Event != NULL) {
 gBS->RestoreTPL (OldTpl);
@@ -524,11 +524,11 @@ Mtftp4Start (
 
   gBS->RestoreTPL (OldTpl);
   return Token->Status;
 
 ON_ERROR:
-  Mtftp4CleanOperation (Instance, Status);
+  Mtftp4CleanOperation (Instance, TokenStatus);
   gBS->RestoreTPL (OldTpl);
 
   return Status;
 }
 
-- 
2.16.2.windows.1

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

Re: [edk2] [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after supporting IOMMU

2018-03-11 Thread Wu, Hao A 
One minor comment:
For patch 3, within changes in function Usb3PciIoNotify():

Usb3NamedEventListen (
  ,
  TPL_NOTIFY,
  Usb3DxeSmmReadyToLockNotify,
<---  Does this change related with patch 2?
  );

Otherwise, for me, patch 2 is storing the event for
DxeSmmReadyToLockProtocol in an event which will be closed right after.


Best Regards,
Hao Wu


> -Original Message-
> From: Zeng, Star
> Sent: Sunday, March 11, 2018 11:16 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star; Yao, Jiewen; Ni, Ruiyu; Wu, Hao A
> Subject: [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements
> after supporting IOMMU
> 
> Please get detailed information in the separated patches.
> 
> Cc: Jiewen Yao 
> Cc: Ruiyu Ni 
> Cc: Hao Wu 
> 
> Star Zeng (3):
>   SourceLevelDebugPkg DebugCommUsb3: Refine some formats
>   SourceLevelDebugPkg DebugCommUsb3: Realloc granted DXE DMA buffer
>   SourceLevelDebugPkg DebugCommUsb3: Use the Handle from DebugAgentLib
> 
>  .../DebugCommunicationLibUsb3Common.c  |  96 +-
>  .../DebugCommunicationLibUsb3Dxe.c | 365 
> -
>  .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
>  .../DebugCommunicationLibUsb3Internal.h|  60 +---
>  .../DebugCommunicationLibUsb3Pei.c |  50 ++-
>  .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
>  6 files changed, 332 insertions(+), 254 deletions(-)
> 
> --
> 2.13.3.windows.1

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

Re: [edk2] [PATCH 3/3] Hisilicon/D05: Support SBSA watchdog

2018-03-11 Thread Guo Heyi 
On Wed, Mar 07, 2018 at 04:10:23PM +, Ard Biesheuvel wrote:
> On 7 March 2018 at 06:55, Heyi Guo  wrote:
> > From: Chenhui Sun 
> >
> > Add description of SBSA watchdogs to ACPI GTDT on D05.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chenhui Sun 
> > Signed-off-by: Heyi Guo 
> > Cc: Ard Biesheuvel 
> > Cc: Leif Lindholm 
> > Cc: Graeme Gregory 
> > ---
> >  Platform/Hisilicon/D05/D05.dsc  |  4 
> >  Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf |  2 ++
> >  Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc| 19 
> > +++
> >  3 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc
> > index 0792b0814ea1..22eaf356224d 100644
> > --- a/Platform/Hisilicon/D05/D05.dsc
> > +++ b/Platform/Hisilicon/D05/D05.dsc
> > @@ -418,6 +418,10 @@ [PcdsFixedAtBuild.common]
> >
> >gHisiTokenSpaceGuid.Pcdsoctype|0x1610
> >
> > +  # SBSA watchdog on Hi1616
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0x4050
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0x4060
> > +
> >  
> > 
> >  #
> >  # Components Section - list of all EDK II Modules needed by this Platform
> > diff --git a/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf 
> > b/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf
> > index bb279c8e428e..6955e6145c30 100644
> > --- a/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf
> > +++ b/Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf
> > @@ -55,5 +55,7 @@ [FixedPcd]
> >gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
> >gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum
> >gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase
> > +  gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase
> >gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
> >
> > diff --git a/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc 
> > b/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc
> > index 2a9d209c00f0..6bc1bde2a490 100644
> > --- a/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc
> > +++ b/Silicon/Hisilicon/Hi1616/D05AcpiTables/Gtdt.aslc
> > @@ -29,6 +29,7 @@
> >  #define GTDT_TIMER_ALWAYS_ON_CAPABILITY 
> > EFI_ACPI_6_1_GTDT_TIMER_FLAG_ALWAYS_ON_CAPABILITY
> >
> >  #define GTDT_GTIMER_FLAGS   (GTDT_TIMER_ALWAYS_ON_CAPABILITY | 
> > GTDT_TIMER_ACTIVE_LOW | GTDT_TIMER_LEVEL_TRIGGERED)
> > +#define WATCHDOG_SPAN  0x2000
> >
> 
> Please don't use
> 
> gArmTokenSpaceGuid.PcdGenericWatchdogXXXBase
> 
> to describe two different instances of the IP that are %!@ MB apart.
> 
> Instead, you could introduce your own PCDs in the HiSilicon token
> space, but I am also fine with creating local #defines in this file if
> the watchdog is not used anywhere else.

Yes they are not used anywhere else, and the number of SBSA watchdogs is SoC
related, so I will use local #defines. And I think we can simply change the
content of the macros when things change in the future.

Thanks,
Heyi

> 
> 
> >  #pragma pack (1)
> >
> > @@ -57,22 +58,16 @@ EFI_ACPI_6_1_GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
> >  FixedPcdGet32 (PcdArmArchTimerHypIntrNum),// UINT32  
> > NonSecurePL2TimerGSIV
> >  GTDT_GTIMER_FLAGS,// UINT32  
> > NonSecurePL2TimerFlags
> >  0x,   // UINT64  
> > CntReadBasePhysicalAddress
> > -#ifdef notyet
> > -PV660_WATCHDOG_COUNT,  // UINT32  
> > PlatformTimerCount
> > +HI1616_WATCHDOG_COUNT,// UINT32  
> > PlatformTimerCount
> >  sizeof (EFI_ACPI_6_1_GENERIC_TIMER_DESCRIPTION_TABLE) // UINT32 
> > PlatfromTimerOffset
> >},
> >{
> > -EFI_ACPI_6_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > -//FixedPcdGet32 (PcdGenericWatchdogRefreshBase), FixedPcdGet32 
> > (PcdGenericWatchdogControlBase), 93, 0),
> > -0, 0, 0, 0),
> > -EFI_ACPI_6_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > -//FixedPcdGet32 (PcdGenericWatchdogRefreshBase), FixedPcdGet32 
> > (PcdGenericWatchdogControlBase), 94, 
> > EFI_ACPI_6_1_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_SECURE_TIMER)
> > -0, 0, 0, 0)
> > +EFI_ACPI_5_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > +  FixedPcdGet32 (PcdGenericWatchdogRefreshBase), FixedPcdGet32 
> > (PcdGenericWatchdogControlBase), 400, 0),
> > +EFI_ACPI_5_1_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
> > +  FixedPcdGet32 (PcdGenericWatchdogRefreshBase) + WATCHDOG_SPAN, 
> > FixedPcdGet32 (PcdGenericWatchdogControlBase) + WATCHDOG_SPAN, 496, 0)
> > +
> >}
> > -#else /* !notyet */
> > -  0, 0
> >

[edk2] HELP: Qemu not able to load Duet image

2018-03-11 Thread david moheban 
Just wanted to followup on my experimentation with Qemu and EDK2's DuetPkg.
So far I have successfully been able to load the Duet image in Qemu after
refining my image making skills. Though having an issue with UDK2010 NOT
being able to load the 'Bootx64' or 'BootIa32' efi boot loader. I also
compiled 2010-SR1 and this problem is not present and works with 2010-SR1.
Using DDK3790 and VS2010 to compile both packages in DEBUG mode. Should
also mention was able to load the EDK-2010 standard Duet Image inside
Virtualbox as well HOWEVER need to turn on 'EFI' mode inside Virtualbox
settings for the bootx64 or bootia32 efi loader to load!

You might ask why using such an old version of EDK? Well I have noticed
that UDK-2010 compiles with the smallest file size and I am planing to
insert the final image into my firmware with limited space so every KB
counts!

So if anyone can offer some advice I would very much appreciate it..




Thank you.

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

[edk2] [PATCH 0/3] SouceLevelDebugPkg DebugCommUsb3: Enhancements after supporting IOMMU

2018-03-11 Thread Star Zeng 
Please get detailed information in the separated patches.

Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Cc: Hao Wu 

Star Zeng (3):
  SourceLevelDebugPkg DebugCommUsb3: Refine some formats
  SourceLevelDebugPkg DebugCommUsb3: Realloc granted DXE DMA buffer
  SourceLevelDebugPkg DebugCommUsb3: Use the Handle from DebugAgentLib

 .../DebugCommunicationLibUsb3Common.c  |  96 +-
 .../DebugCommunicationLibUsb3Dxe.c | 365 -
 .../DebugCommunicationLibUsb3Dxe.inf   |  11 +-
 .../DebugCommunicationLibUsb3Internal.h|  60 +---
 .../DebugCommunicationLibUsb3Pei.c |  50 ++-
 .../DebugCommunicationLibUsb3Pei.inf   |   4 +-
 6 files changed, 332 insertions(+), 254 deletions(-)

--
2.13.3.windows.1

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

[edk2] [PATCH 1/3] SourceLevelDebugPkg DebugCommUsb3: Refine some formats

2018-03-11 Thread Star Zeng 
Refine some formats and remove some unused prototypes.

Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3Common.c  | 10 ++---
 .../DebugCommunicationLibUsb3Dxe.inf   |  6 ++-
 .../DebugCommunicationLibUsb3Internal.h| 43 ++
 .../DebugCommunicationLibUsb3Pei.inf   |  2 +-
 4 files changed, 13 insertions(+), 48 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index c577df7dea97..e67ff1fe5ca7 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -87,8 +87,8 @@ XhcClearR32Bit(
 VOID
 XhcWriteDebugReg (
   IN USB3_DEBUG_PORT_HANDLE  *Handle,
-  IN UINT32   Offset,
-  IN UINT32   Data
+  IN UINT32  Offset,
+  IN UINT32  Data
   )
 {
   EFI_PHYSICAL_ADDRESS  DebugCapabilityBase;
@@ -111,7 +111,7 @@ XhcWriteDebugReg (
 UINT32
 XhcReadDebugReg (
   IN  USB3_DEBUG_PORT_HANDLE *Handle,
-  IN  UINT32   Offset
+  IN  UINT32 Offset
   )
 {
   UINT32  Data;
@@ -1049,8 +1049,8 @@ DebugPortInitialize (
   }
 
   if (Function != NULL) {
-Function (Context, UsbDebugPortHandle);
+Function (Context, (DEBUG_PORT_HANDLE) UsbDebugPortHandle);
   }
 
-  return (DEBUG_PORT_HANDLE)(UINTN)UsbDebugPortHandle;
+  return (DEBUG_PORT_HANDLE) UsbDebugPortHandle;
 }
diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.inf
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.inf
index c4e4282c98b4..3af7e7180d37 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.inf
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.inf
@@ -59,8 +59,10 @@ [Pcd]
   gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugPortHandleBufferSize|250   ## 
SOMETIMES_CONSUMES
 
 [Protocols]
-  gEfiPciIoProtocolGuid ## CONSUMES
-  gEdkiiIoMmuProtocolGuid   ## CONSUMES
+   ## NOTIFY
+   ## SOMETIMES_CONSUMES
+  gEfiPciIoProtocolGuid
+  gEdkiiIoMmuProtocolGuid   ## SOMETIMES_CONSUMES
 
 [LibraryClasses]
   BaseLib
diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
index 66757dafaebe..961786e2a41f 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Internal.h
@@ -561,7 +561,7 @@ typedef struct _USB3_DEBUG_PORT_INSTANCE {
 UINT32
 XhcReadDebugReg (
   IN  USB3_DEBUG_PORT_HANDLE*Handle,
-  IN  UINT32  Offset
+  IN  UINT32Offset
   );
 
 /**
@@ -575,8 +575,8 @@ XhcReadDebugReg (
 VOID
 XhcSetDebugRegBit (
   IN USB3_DEBUG_PORT_HANDLE  *Handle,
-  IN UINT32   Offset,
-  IN UINT32   Bit
+  IN UINT32  Offset,
+  IN UINT32  Bit
   );
   
 /**
@@ -595,43 +595,6 @@ XhcWriteDebugReg (
   );
 
 /**
-  Discover the USB3 debug device.
-  
-  @param  HandleDebug port handle.
-  
-  @retval RETURN_SUCCESSThe serial device was initialized.
-  @retval RETURN_DEVICE_ERROR   The serial device could not be initialized.
-
-**/
-RETURN_STATUS
-DiscoverUsb3DebugPort(
-  USB3_DEBUG_PORT_HANDLE  *Handle
-  );
-  
-/**
-  Initialize the Serial Device hardware.
-  
-  @param  HandleDebug port handle.
-
-  @retval RETURN_SUCCESSThe serial device was initialized successfully.
-  @retval !RETURN_SUCCESS   Error.
-
-**/
-RETURN_STATUS
-InitializeUsb3DebugPort (
-  USB3_DEBUG_PORT_HANDLE  *Handle
-  );
-
-/**
-  Return XHCI MMIO base address.
-
-**/
-EFI_PHYSICAL_ADDRESS
-GetXhciBaseAddress (
-  VOID
-  );
-
-/**
   Verifies if the bit positions specified by a mask are set in a register.
 
   @param[in, out] RegisterUNITN register
diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Pei.inf
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Pei.inf
index 33074db49a78..4f367622d826 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Pei.inf
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Pei.inf
@@ -41,7 +41,7 @@ [Packages]
 
 [Ppis]

[edk2] [PATCH 2/3] SourceLevelDebugPkg DebugCommUsb3: Realloc granted DXE DMA buffer

2018-03-11 Thread Star Zeng 
For the case that the USB3 debug port instance and DMA buffers are
from PEI HOB with IOMMU enabled, de8373fa07f8 is to reallocate
the DMA buffers by AllocateAddress with the memory type accessible
by SMM environment.

But reallocating the DMA buffers by AllocateAddress may fail.

Instead, this patch is to reinitialize USB3 debug port with granted
DXE DMA buffer accessible by SMM environment.

Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3Dxe.c | 246 +
 .../DebugCommunicationLibUsb3Dxe.inf   |   3 +
 .../DebugCommunicationLibUsb3Pei.c |   2 +-
 3 files changed, 155 insertions(+), 96 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
index 29cec56f39dc..588c9715bd00 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Dxe.c
@@ -18,11 +18,13 @@
 #include 
 #include 
 #include 
+#include 
 #include "DebugCommunicationLibUsb3Internal.h"
 
 GUIDgUsb3DbgGuid =  USB3_DBG_GUID;
 
 USB3_DEBUG_PORT_HANDLE  *mUsb3Instance = NULL;
+EFI_PCI_IO_PROTOCOL *mUsb3PciIo = NULL;
 
 /**
   Creates a named event that can be signaled.
@@ -85,20 +87,16 @@ Usb3NamedEventListen (
 /**
   USB3 map one DMA buffer.
 
-  @param Instance   Pointer to USB3 debug port instance.
   @param PciIo  Pointer to PciIo for USB3 debug port.
   @param AddressDMA buffer address to be mapped.
   @param NumberOfBytes  Number of bytes to be mapped.
-  @param BackupBuffer   Backup buffer address.
 
 **/
 VOID
 Usb3MapOneDmaBuffer (
-  IN USB3_DEBUG_PORT_HANDLE *Instance,
   IN EFI_PCI_IO_PROTOCOL*PciIo,
   IN EFI_PHYSICAL_ADDRESS   Address,
-  IN UINTN  NumberOfBytes,
-  IN EFI_PHYSICAL_ADDRESS   BackupBuffer
+  IN UINTN  NumberOfBytes
   )
 {
   EFI_STATUSStatus;
@@ -117,23 +115,6 @@ Usb3MapOneDmaBuffer (
 );
   ASSERT_EFI_ERROR (Status);
   ASSERT (DeviceAddress == ((EFI_PHYSICAL_ADDRESS) (UINTN) HostAddress));
-  if (Instance->FromHob) {
-//
-// Reallocate the DMA buffer by AllocateAddress with
-// the memory type accessible by SMM.
-//
-CopyMem ((VOID *) (UINTN) BackupBuffer, (VOID *) (UINTN) Address, 
NumberOfBytes);
-Status = gBS->FreePages (Address, EFI_SIZE_TO_PAGES (NumberOfBytes));
-ASSERT_EFI_ERROR (Status);
-Status = gBS->AllocatePages (
-AllocateAddress,
-EfiACPIMemoryNVS,
-EFI_SIZE_TO_PAGES (NumberOfBytes),
-
-);
-ASSERT_EFI_ERROR (Status);
-CopyMem ((VOID *) (UINTN) Address, (VOID *) (UINTN) BackupBuffer, 
NumberOfBytes);
-  }
 }
 
 /**
@@ -149,99 +130,111 @@ Usb3MapDmaBuffers (
   IN EFI_PCI_IO_PROTOCOL*PciIo
   )
 {
-  EFI_STATUSStatus;
-  EDKII_IOMMU_PROTOCOL  *IoMmu;
-  EFI_PHYSICAL_ADDRESS  BackupBuffer;
-  UINTN BackupBufferSize;
-
-  IoMmu = NULL;
-  Status = gBS->LocateProtocol (, NULL, (VOID **) 
);
-  if (EFI_ERROR (Status) || (IoMmu == NULL)) {
-//
-// No need to map the DMA buffers.
-//
-return;
-  }
-
-  //
-  // Allocate backup buffer for the case that the USB3
-  // debug port instance and DMA buffers are from PEI HOB.
-  // For this case, the DMA buffers need to be reallocated
-  // by AllocateAddress with the memory type accessible by
-  // SMM.
-  //
-  BackupBufferSize = MAX (XHCI_DEBUG_DEVICE_MAX_PACKET_SIZE * 2 + 
USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE,
-  MAX (sizeof (TRB_TEMPLATE) * TR_RING_TRB_NUMBER,
-   MAX (sizeof (TRB_TEMPLATE) * 
EVENT_RING_TRB_NUMBER,
-MAX (sizeof (EVENT_RING_SEG_TABLE_ENTRY) * 
ERST_NUMBER,
- MAX (sizeof (XHC_DC_CONTEXT),
-  STRING0_DESC_LEN + MANU_DESC_LEN 
+ PRODUCT_DESC_LEN + SERIAL_DESC_LEN);
-
-  Status = gBS->AllocatePages (
-  AllocateAnyPages,
-  EfiBootServicesData,
-  EFI_SIZE_TO_PAGES (BackupBufferSize),
-  
-  );
-  ASSERT_EFI_ERROR (Status);
-
   Usb3MapOneDmaBuffer (
-Instance,
 PciIo,
 Instance->UrbIn.Data,
-XHCI_DEBUG_DEVICE_MAX_PACKET_SIZE * 2 + 
USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE,
-BackupBuffer
+XHCI_DEBUG_DEVICE_MAX_PACKET_SIZE * 2 + 
USB3_DEBUG_PORT_WRITE_MAX_PACKET_SIZE
 );

[edk2] [PATCH 3/3] SourceLevelDebugPkg DebugCommUsb3: Use the Handle from DebugAgentLib

2018-03-11 Thread Star Zeng 
For notifications to get the Instance, de8373fa07f8 was to build
HOB/SystemTable in DebugCommunicationLibUsb3 itself, that works well
at normal boot, but will fail at S3 resume.
At S3 resume, after the code is transferred to PiSmmCpuDxeSmm from
S3Resume2Pei, HOB is still needed to be used for DMA operation, but
PiSmmCpuDxeSmm has no way to get the HOB at S3 resume.

In fact, DebugAgentLib has been managing the instance as Handle in
HOB/SystemTable.
This patch is to use the Handle from DebugAgentLib for the instance
needed in DebugCommunicationLibUsb3.

Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Cc: Hao Wu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../DebugCommunicationLibUsb3Common.c  |  86 +--
 .../DebugCommunicationLibUsb3Dxe.c | 167 ++---
 .../DebugCommunicationLibUsb3Dxe.inf   |   2 +-
 .../DebugCommunicationLibUsb3Internal.h|  17 +--
 .../DebugCommunicationLibUsb3Pei.c |  48 +++---
 .../DebugCommunicationLibUsb3Pei.inf   |   2 +-
 6 files changed, 188 insertions(+), 134 deletions(-)

diff --git 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
index e67ff1fe5ca7..740d9f98b41d 100644
--- 
a/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
+++ 
b/SourceLevelDebugPkg/Library/DebugCommunicationLibUsb3/DebugCommunicationLibUsb3Common.c
@@ -811,6 +811,42 @@ DiscoverInitializeUsbDebugPort (
 }
 
 /**
+  Set USB3 debug instance address.
+
+**/  
+VOID
+SetUsb3DebugPortInstance (
+  IN USB3_DEBUG_PORT_HANDLE *Instance
+  )
+{
+  EFI_PHYSICAL_ADDRESS  *AddrPtr;
+
+  AddrPtr = GetUsb3DebugPortInstanceAddrPtr ();
+  ASSERT (AddrPtr != NULL);
+  *AddrPtr = (EFI_PHYSICAL_ADDRESS) (UINTN) Instance;
+}
+
+/**
+  Return USB3 debug instance address.
+
+**/  
+USB3_DEBUG_PORT_HANDLE *
+GetUsb3DebugPortInstance (
+  VOID
+  )
+{
+  EFI_PHYSICAL_ADDRESS  *AddrPtr;
+  USB3_DEBUG_PORT_HANDLE*Instance;
+
+  AddrPtr = GetUsb3DebugPortInstanceAddrPtr ();
+  ASSERT (AddrPtr != NULL);
+
+  Instance = (USB3_DEBUG_PORT_HANDLE *) (UINTN) *AddrPtr;
+
+  return Instance;
+}
+
+/**
   Read data from debug device and save the data in buffer.
 
   Reads NumberOfBytes data bytes from a debug device into the buffer
@@ -844,16 +880,23 @@ DebugPortReadBuffer (
 return 0;
   }
 
-  UsbDebugPortHandle = GetUsb3DebugPortInstance ();
+  if (Handle != NULL) {
+UsbDebugPortHandle = (USB3_DEBUG_PORT_HANDLE *) Handle;
+SetUsb3DebugPortInstance (UsbDebugPortHandle);
+  } else {
+UsbDebugPortHandle = GetUsb3DebugPortInstance ();
+  }
   if (UsbDebugPortHandle == NULL) {
 return 0;
   }
 
-  if (UsbDebugPortHandle->Initialized != USB3DBG_ENABLED) {
+  if (UsbDebugPortHandle->InNotify) {
 return 0;
   }
 
-  if (UsbDebugPortHandle->InNotify) {
+  DiscoverInitializeUsbDebugPort (UsbDebugPortHandle);
+
+  if (UsbDebugPortHandle->Initialized != USB3DBG_ENABLED) {
 return 0;
   }
 
@@ -914,16 +957,23 @@ DebugPortWriteBuffer (
   Sent  = 0;
   Total = 0;
 
-  UsbDebugPortHandle = GetUsb3DebugPortInstance ();
+  if (Handle != NULL) {
+UsbDebugPortHandle = (USB3_DEBUG_PORT_HANDLE *) Handle;
+SetUsb3DebugPortInstance (UsbDebugPortHandle);
+  } else {
+UsbDebugPortHandle = GetUsb3DebugPortInstance ();
+  }
   if (UsbDebugPortHandle == NULL) {
 return 0;
   }
 
-  if (UsbDebugPortHandle->Initialized != USB3DBG_ENABLED) {
+  if (UsbDebugPortHandle->InNotify) {
 return 0;
   }
 
-  if (UsbDebugPortHandle->InNotify) {
+  DiscoverInitializeUsbDebugPort (UsbDebugPortHandle);
+
+  if (UsbDebugPortHandle->Initialized != USB3DBG_ENABLED) {
 return 0;
   }
 
@@ -968,16 +1018,23 @@ DebugPortPollBuffer (
   USB3_DEBUG_PORT_HANDLE *UsbDebugPortHandle;
   UINTN Length;
 
-  UsbDebugPortHandle = GetUsb3DebugPortInstance ();
+  if (Handle != NULL) {
+UsbDebugPortHandle = (USB3_DEBUG_PORT_HANDLE *) Handle;
+SetUsb3DebugPortInstance (UsbDebugPortHandle);
+  } else {
+UsbDebugPortHandle = GetUsb3DebugPortInstance ();
+  }
   if (UsbDebugPortHandle == NULL) {
 return FALSE;
   }
 
-  if (UsbDebugPortHandle->Initialized != USB3DBG_ENABLED) {
+  if (UsbDebugPortHandle->InNotify) {
 return FALSE;
   }
 
-  if (UsbDebugPortHandle->InNotify) {
+  DiscoverInitializeUsbDebugPort (UsbDebugPortHandle);
+
+  if (UsbDebugPortHandle->Initialized != USB3DBG_ENABLED) {
 return FALSE;
   }
 
@@ -1043,11 +1100,22 @@ DebugPortInitialize (
 {
   USB3_DEBUG_PORT_HANDLE*UsbDebugPortHandle;
 
+  //
+  // Validate the PCD PcdDebugPortHandleBufferSize value 
+  //
+  ASSERT (PcdGet16 (PcdDebugPortHandleBufferSize) == sizeof 
(USB3_DEBUG_PORT_HANDLE));
+
+  if (Function == NULL && Context

Re: [edk2] [PATCH 15/45] OvmfPkg/IoMmuDxe: list "AmdSevIoMmu.h" in the INF file

2018-03-11 Thread Brijesh Singh 


On 3/10/18 7:48 PM, Laszlo Ersek wrote:
> The header file declares the AmdSevInstallIoMmuProtocol() function, which
> is implemented in "AmdSevIoMmu.c" and called from "IoMmuDxe.c".
>
> Cc: Ard Biesheuvel 
> Cc: Brijesh Singh 
> Cc: Jordan Justen 
> Suggested-by: Michael Kinney 
> Ref: 
> http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F56327F7D3@ORSMSX113.amr.corp.intel.com
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 

Reviewed-by: Brijesh Singh 

thanks

> ---
>  OvmfPkg/IoMmuDxe/IoMmuDxe.inf | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> index 307849706800..547b51352618 100644
> --- a/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> +++ b/OvmfPkg/IoMmuDxe/IoMmuDxe.inf
> @@ -26,6 +26,7 @@ [Defines]
>  
>  [Sources]
>AmdSevIoMmu.c
> +  AmdSevIoMmu.h
>IoMmuDxe.c
>  
>  [Packages]

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

Re: [edk2] [PATCH 00/45] ArmVirtPkg, OvmfPkg: list module-internal headers in INF files

2018-03-11 Thread Ard Biesheuvel 
On 11 March 2018 at 11:48, Laszlo Ersek  wrote:
> On 03/11/18 09:15, Ard Biesheuvel wrote:
>> Hi Laszlo,
>>
>> On 11 March 2018 at 01:48, Laszlo Ersek  wrote:
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: hdr_inf_cleanup
>>>
>>> In
>>> ,
>>> Mike explained why it's a good idea to list module-internal *.h files in
>>> the [Sources*] sections of the INF files:
>>>
>>> On 11/23/15 21:28, Kinney, Michael D wrote:
 There are 2 reasons to list all source files used in a module build in
 the [Sources] section.

 1) Support incremental builds.  If a change to the .h file is made,
then the module may  not be rebuilt if the .h file is not listed in
[Sources]
 2) Support of UEFI Distribution Package distribution format.  The UPT
tools that creates UDP packages uses the [Sources] section for the
inventory of files.  If a file is missing, then it will not be
included in the UDP file.
>>>
>>> In only two years and three-four months, I've finally come around
>>> addressing (1) under ArmVirtPkg and OvmfPkg.
>>
>> Thanks for doing this.
>>
>> However, while I highly appreciate your thoroughness and verbosity in
>> most cases, I do think you've crossed a line this time :-)
>>
>> Do we *really* need 4 different patches for CsmSupportLib, each adding
>> a single .h file to [Sources], with an elaborate description how it is
>> being used? If it is used, it needs to be listed, and if it is not, it
>> needs to be removed, that's all there is to it IMO.
>
> The structuring of the patch series reflects my thinking process and the
> work I did precisely. I didn't (couldn't) investigate multiple header
> files at once / in parallel; I investigated them one by one. It's easy
> to squash patches, and it's hard to split them, so I maintain that
> writing up and posting these patches one by one, in v1, was the right
> thing to do. Personally I find it much easier to read many trivial
> patches than half as many complex / divergent ones. If you prefer that I
> squash patches into one per module, I can do that (I'd wait for more
> feedback first though).
>
> Second, I disagree that it's as simple as "list it if it's used". I
> didn't just want to dump the .h filenames into the INF files; I wanted
> to see each time whether the use of the header file was justified in the
> first place -- this is not a given if there are multiple INF files in
> the same directory, or an INF file has architecture-specific Sources
> sections.
>
> For example, in patch 06/45, I removed "QemuLoader.h" from "Qemu.c", and
> "Qemu.c" is only built into one of the INF files under
> "OvmfPkg/AcpiPlatformDxe". (Ultimately I had to list "QemuLoader.h" in
> both INF files, in patch 07/45, due to "QemuFwCfgAcpi.c", which is built
> into both INFs.)
>
> For another example, in patch 37/45, I added "VbeShim.h" to
> [Sources.Ia32, Sources.X64], and not to another of the [Sources*]
> sections. The same applies to patch 17/45, where "X64/VirtualMemory.h"
> belongs under [Sources.X64] only.
>
> I find this is not as easy as it looks, and I meant to be thorough. If
> you don't have time to wade through the patches, I'll thank you if you
> ACK just the first three (ArmVirtPkg) patches.
>

Please understand that this is not criticism on your thinking process,
and I highly value the quality of your work in general, and for this
series in particular.

I am merely saying that it is not always necessary to share your
personal journey resulting in the patches at this level of detail,
simply because it doesn't scale.

In any case, I am happy with this to go in as is, if you prefer.

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

Re: [edk2] [PATCH 00/45] ArmVirtPkg, OvmfPkg: list module-internal headers in INF files

2018-03-11 Thread Laszlo Ersek 
On 03/11/18 09:15, Ard Biesheuvel wrote:
> Hi Laszlo,
> 
> On 11 March 2018 at 01:48, Laszlo Ersek  wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: hdr_inf_cleanup
>>
>> In
>> ,
>> Mike explained why it's a good idea to list module-internal *.h files in
>> the [Sources*] sections of the INF files:
>>
>> On 11/23/15 21:28, Kinney, Michael D wrote:
>>> There are 2 reasons to list all source files used in a module build in
>>> the [Sources] section.
>>>
>>> 1) Support incremental builds.  If a change to the .h file is made,
>>>then the module may  not be rebuilt if the .h file is not listed in
>>>[Sources]
>>> 2) Support of UEFI Distribution Package distribution format.  The UPT
>>>tools that creates UDP packages uses the [Sources] section for the
>>>inventory of files.  If a file is missing, then it will not be
>>>included in the UDP file.
>>
>> In only two years and three-four months, I've finally come around
>> addressing (1) under ArmVirtPkg and OvmfPkg.
> 
> Thanks for doing this.
> 
> However, while I highly appreciate your thoroughness and verbosity in
> most cases, I do think you've crossed a line this time :-)
> 
> Do we *really* need 4 different patches for CsmSupportLib, each adding
> a single .h file to [Sources], with an elaborate description how it is
> being used? If it is used, it needs to be listed, and if it is not, it
> needs to be removed, that's all there is to it IMO.

The structuring of the patch series reflects my thinking process and the
work I did precisely. I didn't (couldn't) investigate multiple header
files at once / in parallel; I investigated them one by one. It's easy
to squash patches, and it's hard to split them, so I maintain that
writing up and posting these patches one by one, in v1, was the right
thing to do. Personally I find it much easier to read many trivial
patches than half as many complex / divergent ones. If you prefer that I
squash patches into one per module, I can do that (I'd wait for more
feedback first though).

Second, I disagree that it's as simple as "list it if it's used". I
didn't just want to dump the .h filenames into the INF files; I wanted
to see each time whether the use of the header file was justified in the
first place -- this is not a given if there are multiple INF files in
the same directory, or an INF file has architecture-specific Sources
sections.

For example, in patch 06/45, I removed "QemuLoader.h" from "Qemu.c", and
"Qemu.c" is only built into one of the INF files under
"OvmfPkg/AcpiPlatformDxe". (Ultimately I had to list "QemuLoader.h" in
both INF files, in patch 07/45, due to "QemuFwCfgAcpi.c", which is built
into both INFs.)

For another example, in patch 37/45, I added "VbeShim.h" to
[Sources.Ia32, Sources.X64], and not to another of the [Sources*]
sections. The same applies to patch 17/45, where "X64/VirtualMemory.h"
belongs under [Sources.X64] only.

I find this is not as easy as it looks, and I meant to be thorough. If
you don't have time to wade through the patches, I'll thank you if you
ACK just the first three (ArmVirtPkg) patches.

> Apart from that, the series looks correct to me.

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

Re: [edk2] [PATCH 00/45] ArmVirtPkg, OvmfPkg: list module-internal headers in INF files

2018-03-11 Thread Ard Biesheuvel 
Hi Laszlo,

On 11 March 2018 at 01:48, Laszlo Ersek  wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: hdr_inf_cleanup
>
> In
> ,
> Mike explained why it's a good idea to list module-internal *.h files in
> the [Sources*] sections of the INF files:
>
> On 11/23/15 21:28, Kinney, Michael D wrote:
>> There are 2 reasons to list all source files used in a module build in
>> the [Sources] section.
>>
>> 1) Support incremental builds.  If a change to the .h file is made,
>>then the module may  not be rebuilt if the .h file is not listed in
>>[Sources]
>> 2) Support of UEFI Distribution Package distribution format.  The UPT
>>tools that creates UDP packages uses the [Sources] section for the
>>inventory of files.  If a file is missing, then it will not be
>>included in the UDP file.
>
> In only two years and three-four months, I've finally come around
> addressing (1) under ArmVirtPkg and OvmfPkg.

Thanks for doing this.

However, while I highly appreciate your thoroughness and verbosity in
most cases, I do think you've crossed a line this time :-)

Do we *really* need 4 different patches for CsmSupportLib, each adding
a single .h file to [Sources], with an elaborate description how it is
being used? If it is used, it needs to be listed, and if it is not, it
needs to be removed, that's all there is to it IMO.

Apart from that, the series looks correct to me.

Thanks,
Ard.


> The affected *.inf and *.h
> files were located with the following crude script:
>
>> #!/bin/bash
>>
>> export LC_ALL=C
>>
>> find ArmVirtPkg/ OvmfPkg/ -type f -name '*.inf' \
>> | sort \
>> | while read INF; do
>> INF_D=$(dirname -- "$INF")
>> INF_F=$(basename -- "$INF")
>> (
>>   cd "$INF_D"
>>   find -type f -name '*.h' \
>>   | cut -c 3- \
>>   | sort \
>>   | while read REL_H; do
>>   if ! grep -q -F -e "  $REL_H" -- "$INF_F"; then
>> printf '%s: %s\n' "$INF" "$REL_H"
>>   fi
>> done
>> )
>>   done
>
> This patch set brings the output down to nil, from the following 45
> lines (note that the patch count equaling 45 is a coincidence):
>
>> ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf: 
>> PlatformBm.h
>> ArmVirtPkg/PrePi/ArmVirtPrePiUniCoreRelocatable.inf: PrePi.h
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf: AcpiPlatform.h
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf: QemuLoader.h
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf: AcpiPlatform.h
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf: QemuLoader.h
>> OvmfPkg/BlockMmioToBlockIoDxe/BlockIo.inf: BlockIo.h
>> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: CsmSupportLib.h
>> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyInterrupt.h
>> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyPlatform.h
>> OvmfPkg/Csm/CsmSupportLib/CsmSupportLib.inf: LegacyRegion.h
>> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf: Fvb.h
>> OvmfPkg/IoMmuDxe/IoMmuDxe.inf: AmdSevIoMmu.h
>> OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf: AcpiTimerLib.h
>> OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf: AcpiTimerLib.h
>> OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf: AcpiTimerLib.h
>> OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf: 
>> X64/VirtualMemory.h
>> OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf: LoadLinuxLib.h
>> OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf: LockBoxLib.h
>> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf: LockBoxLib.h
>> OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf: NvVarsFileLib.h
>> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf: 
>> DebugLibDetect.h
>> OvmfPkg/Library/PlatformDebugLibIoPort/PlatformRomDebugLibIoPort.inf: 
>> DebugLibDetect.h
>> OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf: ExtraRootBusMap.h
>> OvmfPkg/Library/SerializeVariablesLib/SerializeVariablesLib.inf: 
>> SerializeVariablesLib.h
>> OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf: 
>> VirtioMmioDevice.h
>> OvmfPkg/PlatformDxe/Platform.inf: Platform.h
>> OvmfPkg/PlatformDxe/Platform.inf: PlatformConfig.h
>> OvmfPkg/PlatformPei/PlatformPei.inf: Cmos.h
>> OvmfPkg/PlatformPei/PlatformPei.inf: Platform.h
>> OvmfPkg/PlatformPei/PlatformPei.inf: Xen.h
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf: 
>> FwBlockService.h
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf: QemuFlash.h
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf: FwBlockService.h
>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesSmm.inf: QemuFlash.h
>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: Qemu.h
>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: UnalignedIoInternal.h
>> OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf: VbeShim.h
>> OvmfPkg/Virtio10Dxe/Virtio10.inf: Virtio10.h
>> OvmfPkg/VirtioBlkDxe/VirtioBlk.inf: VirtioBlk.h
>> OvmfPkg/VirtioNetDxe/VirtioNet.inf: VirtioNet.h
>>