Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Brian J. Johnson]

On 11/04/2015 02:08 PM, Laszlo Ersek wrote:

On 11/04/15 17:55, Kinney, Michael D wrote:

Laszlo,

BaseXApicX2ApicLib is intended to be used by platforms that support more >=256 
CPUs.

If the current system configuration is < 256 CPUs, then the platform will 
typically stay in APIC mode.  If >= 256 CPUs are detected, then X2 APIC mode can 
be enabled using the following API.

VOID
EFIAPI
SetApicMode (
   IN UINTN  ApicMode
   )

So just adding BaseXApicX2ApicLib to the DSC does not enable X2 APIC mode.  You 
have to add logic to enable X2 APIC mode.

I see that QEMU is limited to 255 VCPUs, so the use of BaseXApicLib makes sense.  
Are OVMF configurations supported with >= 256 VCPUs?


I don't think so, but 64-bit Linux guest kernels enable x2apic mode
anyway, apparently regardless of the VCPU count and topology.

This is normally not a problem, but with SMM support, the APIC library
gets used (within SMM) *after* Linux configures x2apic mode. This causes
the "basic" library instance to blow up. Therefore I flipped the library
class resolution to the one instance that supports x2apic mode, but only
for the SMM build of OVMF.

So, the question Paolo and I have is *not* whether we must use the
x2apic-capable library in the SMM build -- that is a fact, forced by the
X64 Linux guest kernel's behavior.

Neither is our question "how could we enable x2apic mode"? About that,
we don't care at the moment. :)

Instead, our question is if we can use BaseXApicX2ApicLib even in the
*non*-SMM build, without fear of regressions. In other words, if
BaseXApicX2ApicLib can safely replace BaseXApicLib in a build where
BaseXApicLib would be otherwise fully sufficient.

Because, if the answer is "yes, it is compatible", then we shouldn't
complicate the library class resolution in the OVMF DSC files, making it
dependent on the build type (i.e., SMM -> BaseXApicX2ApicLib, non-SMM ->
BaseXApicLib). Rather than that, we should indiscriminately use
BaseXApicX2ApicLib.

So... is BaseXApicX2ApicLib compatible with BaseXApicLib in the domain
where the latter would work sufficiently?

Thanks!
Laszlo


All I can say is that BaseXApicX2ApicLib works great on non-virtual 
hardware, in either legacy or X2Apic mode.


Brian






Thanks,

Mike


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, November 04, 2015 2:41 AM
To: Paolo Bonzini; edk2-de...@ml01.01.org; Kinney, Michael D; Fan, Jeff;
Justen, Jordan L
Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
with x2apic support if SMM_REQUIRE

On 11/04/15 09:48, Paolo Bonzini wrote:



On 03/11/2015 22:00, Laszlo Ersek wrote:

+
+!if $(SMM_REQUIRE) == TRUE
+

LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf

+!else
LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
+!endif
+


Can we enable BaseXApicX2ApicLib unconditionally?


I think I am technically capable of that :), but should we? We haven't
used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
it could regress stuff or not. If it causes problems with the
SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
such a global change for the traditional build.

I'm not against it, I just don't have experience with BaseXApicX2ApicLib.

Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always used
the former? If it has only been for simplicity's sake, and
BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
think Paolo is right, and we should just keep the OVMF DSCs simple.

Thanks
Laszlo


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




--

  My statements are my own, are not authorized by SGI, and do not
  necessarily represent SGI’s positions.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Paolo Bonzini]

On 04/11/2015 21:08, Laszlo Ersek wrote:
> On 11/04/15 17:55, Kinney, Michael D wrote:
>> Laszlo,
>>
>> BaseXApicX2ApicLib is intended to be used by platforms that support more 
>> >=256 CPUs.
>>
>> If the current system configuration is < 256 CPUs, then the platform will 
>> typically stay in APIC mode.  If >= 256 CPUs are detected, then X2 APIC mode 
>> can be enabled using the following API.
>>
>> VOID
>> EFIAPI
>> SetApicMode (
>>   IN UINTN  ApicMode
>>   )
>>
>> So just adding BaseXApicX2ApicLib to the DSC does not enable X2 APIC mode.  
>> You have to add logic to enable X2 APIC mode.
>>
>> I see that QEMU is limited to 255 VCPUs, so the use of BaseXApicLib makes 
>> sense.  Are OVMF configurations supported with >= 256 VCPUs?
> 
> I don't think so, but 64-bit Linux guest kernels enable x2apic mode
> anyway, apparently regardless of the VCPU count and topology.

Yup, x2apic-style accesses (via MSRs) are faster because you do not have
to walk the page tables.

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

[edk2] Could not add PCI device with big memory to aarch64 VMs

[liang yan]

Hello, Laszlo,

(1)I am trying to add ivshmem device(PCI device with big memory) to my 
aarch64 vm.
So far, I could find device information from vm. But it seems vm did not 
create
correct resource file for this device. Do you have any idea that this 
happens?


I used the upstream EDK2 to build my UEFI firmware.

There are three BARs for this device, and memory map is assigned too, 
but only one

resource file is created.

My qemu supports ACPI 5.1 and the command line is :

  -device ivshmem,size=256M,chardev=ivshmem,msi=on,ioeventfd=on \
  -chardev socket,path=/tmp/ivshmem_socket,id=ivshmem \

The lspci information:

00:00.0 Host bridge: Red Hat, Inc. Device 0008
Subsystem: Red Hat, Inc Device 1100
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
SERR- 

00:01.0 RAM memory: Red Hat, Inc Inter-VM shared memory
Subsystem: Red Hat, Inc QEMU Virtual Machine
Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
Stepping- SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- 
SERR- 
Interrupt: pin A routed to IRQ 255
Region 0: Memory at 20001000 (32-bit, non-prefetchable) [disabled] 
[size=256]
Region 1: Memory at 2000 (32-bit, non-prefetchable) [disabled] 
[size=4K]
Region 2: Memory at 1000 (64-bit, prefetchable) [disabled] 
[size=256M]

Capabilities: [40] MSI-X: Enable- Count=1 Masked-
Vector table: BAR=1 offset=
PBA: BAR=1 offset=0800

Boot information:

[2.380924] pci :00:01.0: BAR 2: assigned [mem 
0x1000-0x1fff 64bit pref]

[2.382836] pci :00:01.0: BAR 1: assigned [mem 0x2000-0x2fff]
[2.383557] pci :00:01.0: BAR 0: assigned [mem 0x20001000-0x200010ff]


Files under /sys/devices/pci:00/:00:01.0

broken_parity_status  devspec   local_cpus  resource
class  dma_mask_bitsmodaliassubsystem
config  driver_override  msi_bus subsystem_device
consistent_dma_mask_bits  enable   power   subsystem_vendor
d3cold_allowed  irq   remove  uevent
device  local_cpulistrescan  vendor

Information for resource:

0x20001000 0x200010ff 0x00040200
0x2000 0x2fff 0x00040200
0x1000 0x1fff 0x0014220c
0x 0x 0x
0x 0x 0x
0x 0x 0x
0x 0x 0x




(2)It also has a problem that once I use a memory bigger than 256M for 
ivshmem, it could not get through UEFI,

the error message is

PciBus: Discovered PCI @ [00|01|00]
   BAR[0]: Type =  Mem32; Alignment = 0xFFF;Length = 0x100; Offset 
= 0x10
   BAR[1]: Type =  Mem32; Alignment = 0xFFF;Length = 0x1000; Offset 
= 0x14
   BAR[2]: Type = PMem64; Alignment = 0x3FFF;Length = 
0x4000;Offset = 0x18


PciBus: HostBridge->SubmitResources() - Success
ASSERT 
/home/liang/studio/edk2/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c(449): ((BOOLEAN)(0==1))


I am wandering if there are memory limitation for pcie devices under 
Qemu environment?



Just thank you in advance and any information would be appreciated.



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

Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Kinney, Michael D]

Laszlo,

Yes.  They are compatible.  And I do recommend switching to BaseXApicX2ApicLib 
unconditionally.
 
Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Wednesday, November 04, 2015 12:09 PM
>To: Kinney, Michael D; Paolo Bonzini; edk2-de...@ml01.01.org; Fan, Jeff;
>Justen, Jordan L
>Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
>with x2apic support if SMM_REQUIRE
>
>On 11/04/15 17:55, Kinney, Michael D wrote:
>> Laszlo,
>>
>> BaseXApicX2ApicLib is intended to be used by platforms that support more
>>=256 CPUs.
>>
>> If the current system configuration is < 256 CPUs, then the platform will
>typically stay in APIC mode.  If >= 256 CPUs are detected, then X2 APIC mode
>can be enabled using the following API.
>>
>> VOID
>> EFIAPI
>> SetApicMode (
>>   IN UINTN  ApicMode
>>   )
>>
>> So just adding BaseXApicX2ApicLib to the DSC does not enable X2 APIC
>mode.  You have to add logic to enable X2 APIC mode.
>>
>> I see that QEMU is limited to 255 VCPUs, so the use of BaseXApicLib makes
>sense.  Are OVMF configurations supported with >= 256 VCPUs?
>
>I don't think so, but 64-bit Linux guest kernels enable x2apic mode
>anyway, apparently regardless of the VCPU count and topology.
>
>This is normally not a problem, but with SMM support, the APIC library
>gets used (within SMM) *after* Linux configures x2apic mode. This causes
>the "basic" library instance to blow up. Therefore I flipped the library
>class resolution to the one instance that supports x2apic mode, but only
>for the SMM build of OVMF.
>
>So, the question Paolo and I have is *not* whether we must use the
>x2apic-capable library in the SMM build -- that is a fact, forced by the
>X64 Linux guest kernel's behavior.
>
>Neither is our question "how could we enable x2apic mode"? About that,
>we don't care at the moment. :)
>
>Instead, our question is if we can use BaseXApicX2ApicLib even in the
>*non*-SMM build, without fear of regressions. In other words, if
>BaseXApicX2ApicLib can safely replace BaseXApicLib in a build where
>BaseXApicLib would be otherwise fully sufficient.
>
>Because, if the answer is "yes, it is compatible", then we shouldn't
>complicate the library class resolution in the OVMF DSC files, making it
>dependent on the build type (i.e., SMM -> BaseXApicX2ApicLib, non-SMM ->
>BaseXApicLib). Rather than that, we should indiscriminately use
>BaseXApicX2ApicLib.
>
>So... is BaseXApicX2ApicLib compatible with BaseXApicLib in the domain
>where the latter would work sufficiently?
>
>Thanks!
>Laszlo
>
>
>>
>> Thanks,
>>
>> Mike
>>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Wednesday, November 04, 2015 2:41 AM
>>> To: Paolo Bonzini; edk2-de...@ml01.01.org; Kinney, Michael D; Fan, Jeff;
>>> Justen, Jordan L
>>> Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
>>> with x2apic support if SMM_REQUIRE
>>>
>>> On 11/04/15 09:48, Paolo Bonzini wrote:


 On 03/11/2015 22:00, Laszlo Ersek wrote:
> +
> +!if $(SMM_REQUIRE) == TRUE
> +
>>>
>LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> +!else
>LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
> +!endif
> +

 Can we enable BaseXApicX2ApicLib unconditionally?
>>>
>>> I think I am technically capable of that :), but should we? We haven't
>>> used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
>>> it could regress stuff or not. If it causes problems with the
>>> SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
>>> such a global change for the traditional build.
>>>
>>> I'm not against it, I just don't have experience with BaseXApicX2ApicLib.
>>>
>>> Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
>>> and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always
>used
>>> the former? If it has only been for simplicity's sake, and
>>> BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
>>> think Paolo is right, and we should just keep the OVMF DSCs simple.
>>>
>>> Thanks
>>> Laszlo
>
>___
>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 v4 07/41] OvmfPkg: add PEIM for providing TSEG-as-SMRAM during PEI

[Kinney, Michael D]

Laszlo,

For the EFI_PEI_COMMUNICATION_PPI, is there a reason you are not using 
UefiCpuPkg\PiSmmCommunication\PiSmmCommunicationPei.inf to produce that PPI?

Thanks,

Mike

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Tuesday, November 03, 2015 1:01 PM
>To: edk2-de...@ml01.01.org
>Cc: Kinney, Michael D; Justen, Jordan L
>Subject: [PATCH v4 07/41] OvmfPkg: add PEIM for providing TSEG-as-SMRAM
>during PEI
>
>"MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf" is the library
>instance that implements the LockBoxLib library class with SMRAM access
>for the PEI phase.
>
>Introduce a PEIM that produces the EFI_PEI_SMM_COMMUNICATION_PPI and
>PEI_SMM_ACCESS_PPI interfaces, enabling SmmLockBoxPeiLib to work.
>
>Said library instance can parse and access LockBox data itself (without
>additional LockBox drivers) if the
>EFI_PEI_SMM_COMMUNICATION_PPI.Communicate() function returns
>EFI_NOT_STARTED to it. However it requires that
>EFI_PEI_SMM_COMMUNICATION_PPI exist at least. Also,
>PEI_SMM_ACCESS_PPI
>must exist and work.
>
>The load / installation order of S3Resume2Pei and SmmAccessPei is
>indifferent. SmmAccessPei produces the GUIDed HOB during its installation
>(which happens during PEI), but S3Resume2Pei accesses the HOB only when
>the DXE IPL calls its S3RestoreConfig2 PPI member, as last act of PEI.
>
>MCH_SMRAM_D_LCK and MCH_ESMRAMC_T_EN are masked out the way
>they are, in
>SmmAccessPeiEntryPoint() and SmramAccessOpen() respectively, in order to
>prevent VS20xx from warning about the (otherwise fully intentional)
>truncation in the UINT8 casts. (Warnings reported by Michael Kinney.)
>
>Cc: Michael Kinney 
>Cc: Jordan Justen 
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Laszlo Ersek 
>---
>
>Notes:
>v3:
>- update bit-neg expressions to silence VS20xx warnings [Mike]
>
> OvmfPkg/OvmfPkgIa32.dsc|   6 +
> OvmfPkg/OvmfPkgIa32X64.dsc |   6 +
> OvmfPkg/OvmfPkgX64.dsc |   6 +
> OvmfPkg/OvmfPkgIa32.fdf|   3 +
> OvmfPkg/OvmfPkgIa32X64.fdf |   3 +
> OvmfPkg/OvmfPkgX64.fdf |   3 +
> OvmfPkg/SmmAccess/SmmAccessPei.inf |  70 +++
> OvmfPkg/SmmAccess/SmramInternal.h  |  89 
> OvmfPkg/SmmAccess/SmmAccessPei.c   | 446 
> OvmfPkg/SmmAccess/SmramInternal.c  | 188 +
> 10 files changed, 820 insertions(+)
>
>diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>index c6850ff..0b729ca 100644
>--- a/OvmfPkg/OvmfPkgIa32.dsc
>+++ b/OvmfPkg/OvmfPkgIa32.dsc
>@@ -445,6 +445,12 @@ [Components]
> 
>   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>   }
>+!if $(SMM_REQUIRE) == TRUE
>+  OvmfPkg/SmmAccess/SmmAccessPei.inf {
>+
>+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>+  }
>+!endif
>
>   #
>   # DXE Phase modules
>diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>index dd65bf9..7f672d9 100644
>--- a/OvmfPkg/OvmfPkgIa32X64.dsc
>+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>@@ -451,6 +451,12 @@ [Components.IA32]
> 
>   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>   }
>+!if $(SMM_REQUIRE) == TRUE
>+  OvmfPkg/SmmAccess/SmmAccessPei.inf {
>+
>+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>+  }
>+!endif
>
> [Components.X64]
>   #
>diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>index 0de3c85..986c019 100644
>--- a/OvmfPkg/OvmfPkgX64.dsc
>+++ b/OvmfPkg/OvmfPkgX64.dsc
>@@ -450,6 +450,12 @@ [Components]
> 
>   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>   }
>+!if $(SMM_REQUIRE) == TRUE
>+  OvmfPkg/SmmAccess/SmmAccessPei.inf {
>+
>+  PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
>+  }
>+!endif
>
>   #
>   # DXE Phase modules
>diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
>index 44e4a92..650dab1 100644
>--- a/OvmfPkg/OvmfPkgIa32.fdf
>+++ b/OvmfPkg/OvmfPkgIa32.fdf
>@@ -171,6 +171,9 @@ [FV.PEIFV]
> INF  OvmfPkg/PlatformPei/PlatformPei.inf
> INF  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>+!if $(SMM_REQUIRE) == TRUE
>+INF  OvmfPkg/SmmAccess/SmmAccessPei.inf
>+!endif
>
>
>#
>###
>
>diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
>index 67bfbe7..5830401 100644
>--- a/OvmfPkg/OvmfPkgIa32X64.fdf
>+++ b/OvmfPkg/OvmfPkgIa32X64.fdf
>@@ -171,6 +171,9 @@ [FV.PEIFV]
> INF  OvmfPkg/PlatformPei/PlatformPei.inf
> INF  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> INF  UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
>+!if $(SMM_REQUIRE) == TRUE
>+INF  OvmfPkg/SmmAccess/SmmAccessPei.inf
>+!endif
>
>
>#
>###
>
>diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>index 6624789..9dd6171 100644
>--- a/OvmfPkg/OvmfPkgX64.fdf
>+++ b/OvmfPkg/OvmfPkgX64.fdf
>@@ -171,6 +171,9 @@ 

Re: [edk2] Could not add PCI device with big memory to aarch64 VMs

[Laszlo Ersek]

On 11/04/15 23:22, liang yan wrote:
> Hello, Laszlo,
> 
> (1)I am trying to add ivshmem device(PCI device with big memory) to my
> aarch64 vm.
> So far, I could find device information from vm. But it seems vm did not
> create
> correct resource file for this device. Do you have any idea that this
> happens?
> 
> I used the upstream EDK2 to build my UEFI firmware.
> 
> There are three BARs for this device, and memory map is assigned too,
> but only one
> resource file is created.
> 
> My qemu supports ACPI 5.1 and the command line is :
> 
>   -device ivshmem,size=256M,chardev=ivshmem,msi=on,ioeventfd=on \
>   -chardev socket,path=/tmp/ivshmem_socket,id=ivshmem \
> 
> The lspci information:
> 
> 00:00.0 Host bridge: Red Hat, Inc. Device 0008
> Subsystem: Red Hat, Inc Device 1100
> Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR- FastB2B- DisINTx-
> Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
> SERR-  
> 00:01.0 RAM memory: Red Hat, Inc Inter-VM shared memory
> Subsystem: Red Hat, Inc QEMU Virtual Machine
> Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr-
> Stepping- SERR- FastB2B- DisINTx-
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
> SERR-  Interrupt: pin A routed to IRQ 255
> Region 0: Memory at 20001000 (32-bit, non-prefetchable) [disabled]
> [size=256]
> Region 1: Memory at 2000 (32-bit, non-prefetchable) [disabled]
> [size=4K]
> Region 2: Memory at 1000 (64-bit, prefetchable) [disabled]
> [size=256M]
> Capabilities: [40] MSI-X: Enable- Count=1 Masked-
> Vector table: BAR=1 offset=
> PBA: BAR=1 offset=0800
> 
> Boot information:
> 
> [2.380924] pci :00:01.0: BAR 2: assigned [mem
> 0x1000-0x1fff 64bit pref]
> [2.382836] pci :00:01.0: BAR 1: assigned [mem
> 0x2000-0x2fff]
> [2.383557] pci :00:01.0: BAR 0: assigned [mem
> 0x20001000-0x200010ff]
> 
> 
> Files under /sys/devices/pci:00/:00:01.0
> 
> broken_parity_status  devspec   local_cpus  resource
> class  dma_mask_bitsmodaliassubsystem
> config  driver_override  msi_bus subsystem_device
> consistent_dma_mask_bits  enable   power   subsystem_vendor
> d3cold_allowed  irq   remove  uevent
> device  local_cpulistrescan  vendor
> 
> Information for resource:
> 
> 0x20001000 0x200010ff 0x00040200
> 0x2000 0x2fff 0x00040200
> 0x1000 0x1fff 0x0014220c
> 0x 0x 0x
> 0x 0x 0x
> 0x 0x 0x
> 0x 0x 0x
> 
> 
> 
> 
> (2)It also has a problem that once I use a memory bigger than 256M for
> ivshmem, it could not get through UEFI,
> the error message is
> 
> PciBus: Discovered PCI @ [00|01|00]
>BAR[0]: Type =  Mem32; Alignment = 0xFFF;Length = 0x100; Offset =
> 0x10
>BAR[1]: Type =  Mem32; Alignment = 0xFFF;Length = 0x1000; Offset
> = 0x14
>BAR[2]: Type = PMem64; Alignment = 0x3FFF;Length =
> 0x4000;Offset = 0x18
> 
> PciBus: HostBridge->SubmitResources() - Success
> ASSERT
> /home/liang/studio/edk2/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c(449): 
> ((BOOLEAN)(0==1))
> 
> 
> I am wandering if there are memory limitation for pcie devices under
> Qemu environment?
> 
> 
> Just thank you in advance and any information would be appreciated.

(CC'ing Ard.)

"Apparently", the firmware-side counterpart of QEMU commit 5125f9cd2532
has never been contributed to edk2.

Therefore the the ProcessPciHost() function in
"ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c" ignores the
DTB_PCI_HOST_RANGE_MMIO64 type range from the DTB. (Thus only
DTB_PCI_HOST_RANGE_MMIO32 is recognized as PCI MMIO aperture.)

However, even if said driver was extended to parse the new 64-bit
aperture into PCDs (which wouldn't be hard), the
ArmVirtPkg/PciHostBridgeDxe driver would still have to be taught to look
at that aperture (from the PCDs) and to serve MMIO BAR allocation
requests from it. That could be hard.

Please check edk2 commits e48f1f15b0e2^..e5ceb6c9d390, approximately,
for the background on the current code. See also chapter 13 "Protocols -
PCI Bus Support" in the UEFI spec.

Patches welcome. :)

(A separate note on ACPI vs. DT: the firmware forwards *both* from QEMU
to the runtime guest OS. However, the firmware parses only the DT for
its own purposes.)

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

Re: [edk2] [Patch] BaseTools: Print PACKAGES_PATH build environment if it is set.

[Kinney, Michael D]

Liming,

Why do you a different method than WORKSPACE to display the PACKAGES_PATH and 
EDK_TOOLS_BIN environment variables?

Thanks,

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Liming Gao
>Sent: Tuesday, November 03, 2015 5:15 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [Patch] BaseTools: Print PACKAGES_PATH build environment if
>it is set.
>
>Print the optional build environment PACKAGES_PATH and EDK_TOOLS_BIN.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Liming Gao 
>---
> BaseTools/Source/Python/build/build.py | 4 
> 1 file changed, 4 insertions(+)
>
>diff --git a/BaseTools/Source/Python/build/build.py
>b/BaseTools/Source/Python/build/build.py
>index 30ff5bb..67bd3b3 100644
>--- a/BaseTools/Source/Python/build/build.py
>+++ b/BaseTools/Source/Python/build/build.py
>@@ -780,10 +780,14 @@ class Build():
>
> # print current build environment and configuration
> EdkLogger.quiet("%-16s = %s" % ("WORKSPACE",
>os.environ["WORKSPACE"]))
>+if "PACKAGES_PATH" in os.environ:
>+EdkLogger.quiet("%-16s = %s" % ("PACKAGES_PATH",
>os.path.normcase(os.path.normpath(os.environ["PACKAGES_PATH"]
> EdkLogger.quiet("%-16s = %s" % ("ECP_SOURCE",
>os.environ["ECP_SOURCE"]))
> EdkLogger.quiet("%-16s = %s" % ("EDK_SOURCE",
>os.environ["EDK_SOURCE"]))
> EdkLogger.quiet("%-16s = %s" % ("EFI_SOURCE",
>os.environ["EFI_SOURCE"]))
> EdkLogger.quiet("%-16s = %s" % ("EDK_TOOLS_PATH",
>os.environ["EDK_TOOLS_PATH"]))
>+if "EDK_TOOLS_BIN" in os.environ:
>+EdkLogger.quiet("%-16s = %s" % ("EDK_TOOLS_BIN",
>os.path.normcase(os.path.normpath(os.environ["EDK_TOOLS_BIN"]
>
> EdkLogger.info("")
>
>--
>1.9.5.msysgit.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 v2] NetworkPkg:Enable Http Boot over Ipv6 stack

[Fu, Siyuan]

Hi, Lin

If you are using IPv6 address in URL you should enclose the IP address within 
square brackets ("[" and "]"), like
http://[2000::1]:3260/boot.efi
Please refer to RFC3986
http://tools.ietf.org/html/rfc3986#section-3.2.2

Siyuan

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gary 
Ching-Pang Lin
Sent: Wednesday, November 4, 2015 6:40 PM
To: Zhang, Lubo 
Cc: Ye, Ting ; edk2-devel@lists.01.org; Wu, Jiaxin 
; Fu, Siyuan 
Subject: Re: [edk2] [PATCH v2] NetworkPkg:Enable Http Boot over Ipv6 stack

On Tue, Nov 03, 2015 at 04:13:27PM +0800, Zhang Lubo wrote:
> v2:
> *Rewrite the logic of setting ipv6 gateway in httpboot driver and update some 
> comment.
> 
> Fix a potential bug which can cause assert when setting ipv6 Gateway. 
> If there is no router in network environment, it will fail to find out 
> the gateway address which can route the message to Http Server Ip . 
> When it check the IP6 route table again 1 seconds later, it will 
> assert while the router table structure had been freed last time without 
> retrieve new relevant data form ip6 driver.
> 

Hi Lubo,

I applied the patch and it worked as expected except the IP expressed url. I 
checked the code and found the function(*) to parse the url couldn't handle 
IPv6 and just treat the colon as a separator of the host name and port.
Is this a known issue?

Cheers,

Gary Lin

(*) MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c::NetHttpParseAuthority()

> Cc: Fu Siyuan 
> Cc: Ye Ting 
> CC: Wu Jiaxin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo 
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c|  234 -
>  NetworkPkg/HttpBootDxe/HttpBootComponentName.c |5 +-
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.c |   12 +-
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.h |   11 +
>  NetworkPkg/HttpBootDxe/HttpBootDhcp6.c |  984 +++
>  NetworkPkg/HttpBootDxe/HttpBootDhcp6.h |  198 
>  NetworkPkg/HttpBootDxe/HttpBootDxe.c   |  903 +++---
>  NetworkPkg/HttpBootDxe/HttpBootDxe.h   |  159 +++-
>  NetworkPkg/HttpBootDxe/HttpBootDxe.inf |9 +
>  NetworkPkg/HttpBootDxe/HttpBootImpl.c  |  109 ++-
>  NetworkPkg/HttpBootDxe/HttpBootImpl.h  |2 +-
>  NetworkPkg/HttpBootDxe/HttpBootSupport.c   |  292 +-
>  NetworkPkg/HttpBootDxe/HttpBootSupport.h   |   70 ++
>  NetworkPkg/HttpDxe/HttpDns.c   |  207 +++-
>  NetworkPkg/HttpDxe/HttpDns.h   |   20 +
>  NetworkPkg/HttpDxe/HttpDriver.c|  806 +++-
>  NetworkPkg/HttpDxe/HttpDriver.h|  143 ++-
>  NetworkPkg/HttpDxe/HttpDxe.inf |5 +
>  NetworkPkg/HttpDxe/HttpImpl.c  |  358 +++
>  NetworkPkg/HttpDxe/HttpImpl.h  |1 -
>  NetworkPkg/HttpDxe/HttpProto.c | 1210 
> 
>  NetworkPkg/HttpDxe/HttpProto.h |  182 +++-
>  22 files changed, 5017 insertions(+), 903 deletions(-)  create mode 
> 100644 NetworkPkg/HttpBootDxe/HttpBootDhcp6.c
>  create mode 100644 NetworkPkg/HttpBootDxe/HttpBootDhcp6.h
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Don't require ECP pkg in WORKSPACE when PACKAGES_PATH is set

[Kinney, Michael D]

Reviewed-by: Michael Kinney 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Liming Gao
>Sent: Tuesday, November 03, 2015 3:52 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [Patch] BaseTools: Don't require ECP pkg in WORKSPACE when
>PACKAGES_PATH is set
>
>When PACKAGES_PATH is set, ECP pkg may be in another directory, not exist
>in WORKSPACE. So, keep this check in single WORKSPACE.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Liming Gao 
>---
> BaseTools/Source/Python/build/build.py | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
>diff --git a/BaseTools/Source/Python/build/build.py
>b/BaseTools/Source/Python/build/build.py
>index b5df773..30ff5bb 100644
>--- a/BaseTools/Source/Python/build/build.py
>+++ b/BaseTools/Source/Python/build/build.py
>@@ -156,16 +156,18 @@ def CheckEnvVariable():
> EdkLogger.error("build", FORMAT_NOT_SUPPORTED, "No space is
>allowed in EFI_SOURCE path",
> ExtraData=EfiSourceDir)
>
>-# change absolute path to relative path to WORKSPACE
>-if EfiSourceDir.upper().find(WorkspaceDir.upper()) != 0:
>-EdkLogger.error("build", PARAMETER_INVALID, "EFI_SOURCE is not
>under WORKSPACE",
>-ExtraData="WORKSPACE = %s\nEFI_SOURCE = %s" %
>(WorkspaceDir, EfiSourceDir))
>-if EdkSourceDir.upper().find(WorkspaceDir.upper()) != 0:
>-EdkLogger.error("build", PARAMETER_INVALID, "EDK_SOURCE is not
>under WORKSPACE",
>-ExtraData="WORKSPACE = %s\nEDK_SOURCE = %s" %
>(WorkspaceDir, EdkSourceDir))
>-if EcpSourceDir.upper().find(WorkspaceDir.upper()) != 0:
>-EdkLogger.error("build", PARAMETER_INVALID, "ECP_SOURCE is not
>under WORKSPACE",
>-ExtraData="WORKSPACE = %s\nECP_SOURCE = %s" %
>(WorkspaceDir, EcpSourceDir))
>+# check those variables on single workspace case
>+if not PackagesPath:
>+# change absolute path to relative path to WORKSPACE
>+if EfiSourceDir.upper().find(WorkspaceDir.upper()) != 0:
>+EdkLogger.error("build", PARAMETER_INVALID, "EFI_SOURCE is not
>under WORKSPACE",
>+ExtraData="WORKSPACE = %s\nEFI_SOURCE = %s" %
>(WorkspaceDir, EfiSourceDir))
>+if EdkSourceDir.upper().find(WorkspaceDir.upper()) != 0:
>+EdkLogger.error("build", PARAMETER_INVALID, "EDK_SOURCE is not
>under WORKSPACE",
>+ExtraData="WORKSPACE = %s\nEDK_SOURCE = %s" %
>(WorkspaceDir, EdkSourceDir))
>+if EcpSourceDir.upper().find(WorkspaceDir.upper()) != 0:
>+EdkLogger.error("build", PARAMETER_INVALID, "ECP_SOURCE is not
>under WORKSPACE",
>+ExtraData="WORKSPACE = %s\nECP_SOURCE = %s" %
>(WorkspaceDir, EcpSourceDir))
>
> # check EDK_TOOLS_PATH
> if "EDK_TOOLS_PATH" not in os.environ:
>--
>1.9.5.msysgit.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 v4 06/41] OvmfPkg: PlatformPei: account for TSEG size with PcdSmmSmramRequire set

[Kinney, Michael D]

Laszlo,

Reviewed-by: Michael Kinney 

Mike

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Tuesday, November 03, 2015 1:01 PM
>To: edk2-de...@ml01.01.org
>Cc: Kinney, Michael D
>Subject: [PATCH v4 06/41] OvmfPkg: PlatformPei: account for TSEG size with
>PcdSmmSmramRequire set
>
>PlatformPei calls GetSystemMemorySizeBelow4gb() in three locations:
>
>- PublishPeiMemory(): on normal boot, the permanent PEI RAM is installed
>  so that it ends with the RAM below 4GB,
>
>- QemuInitializeRam(): on normal boot, memory resource descriptor HOBs are
>  created for the RAM below 4GB; plus MTRR attributes are set
>  (independently of S3 vs. normal boot)
>
>- MemMapInitialization(): an MMIO resource descriptor HOB is created for
>  PCI resource allocation, on normal boot, starting at max(RAM below 4GB,
>  2GB).
>
>The first two of these is adjusted for the configured TSEG size, if
>PcdSmmSmramRequire is set:
>
>- In PublishPeiMemory(), the permanent PEI RAM is kept under TSEG.
>
>- In QemuInitializeRam(), we must keep the DXE out of TSEG.
>
>  One idea would be to simply trim the [1MB .. LowerMemorySize] memory
>  resource descriptor HOB, leaving a hole for TSEG in the memory space
>  map.
>
>  The SMM IPL will however want to massage the caching attributes of the
>  SMRAM range that it loads the SMM core into, with
>  gDS->SetMemorySpaceAttributes(), and that won't work on a hole. So,
>  instead of trimming this range, split the TSEG area off, and report it
>  as a cacheable reserved memory resource.
>
>  Finally, since reserved memory can be allocated too, pre-allocate TSEG
>  in InitializeRamRegions(), after QemuInitializeRam() returns. (Note that
>  this step alone does not suffice without the resource descriptor HOB
>  trickery: if we omit that, then the DXE IPL PEIM fails to load and start
>  the DXE core.)
>
>- In MemMapInitialization(), the start of the PCI MMIO range is not
>  affected.
>
>We choose the largest option (8MB) for the default TSEG size. Michael
>Kinney pointed out that the SMBASE relocation in PiSmmCpuDxeSmm
>consumes
>SMRAM proportionally to the number of CPUs. From the three options
>available, he reported that 8MB was both necessary and sufficient for the
>SMBASE relocation to succeed with 255 CPUs:
>
>- http://thread.gmane.org/gmane.comp.bios.edk2.devel/3020/focus=3137
>- http://thread.gmane.org/gmane.comp.bios.edk2.devel/3020/focus=3177
>
>Cc: Michael Kinney 
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Laszlo Ersek 
>Reviewed-by: Jordan Justen 
>---
>
>Notes:
>v4:
>- drop the patch "OvmfPkg: double the SMRAM (TSEG) size for the 64-bit
>  DXE phase builds", and change the default TSEG size from 1MB to 8MB
>  globally [Mike]
>
> OvmfPkg/OvmfPkg.dec |  7 
> OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
> OvmfPkg/PlatformPei/MemDetect.c | 34 +++-
> 3 files changed, 41 insertions(+), 1 deletion(-)
>
>diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>index 3fab2af..04859b4 100644
>--- a/OvmfPkg/OvmfPkg.dec
>+++ b/OvmfPkg/OvmfPkg.dec
>@@ -93,6 +93,13 @@ [PcdsFixedAtBuild]
>   gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit|31|UINT16|6
>   gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxLunLimit|7|UINT32|7
>
>+  ## The following setting controls how many megabytes we configure as
>TSEG on
>+  #  Q35, for SMRAM purposes. Permitted values are: 1, 2, 8. Other values
>cause
>+  #  undefined behavior.
>+  #
>+  #  This PCD is only consulted if PcdSmmSmramRequire is TRUE (see below).
>+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8|UINT8|0x20
>+
> [PcdsFixedAtBuild]
>
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UIN
>T32|0x8
>
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT
>32|0x9
>diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
>b/OvmfPkg/PlatformPei/PlatformPei.inf
>index 62f64fe..ee044a2 100644
>--- a/OvmfPkg/PlatformPei/PlatformPei.inf
>+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>@@ -75,6 +75,7 @@ [Pcd]
>   gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
>   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd
>+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
>
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySiz
>e
>   gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
>   gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize
>diff --git a/OvmfPkg/PlatformPei/MemDetect.c
>b/OvmfPkg/PlatformPei/MemDetect.c
>index 5fe8b28..1bdc2df 100644
>--- a/OvmfPkg/PlatformPei/MemDetect.c
>+++ b/OvmfPkg/PlatformPei/MemDetect.c
>@@ -214,6 +214,12 @@ PublishPeiMemory (
> MemorySize = PcdGet32 (PcdS3AcpiReservedMemorySize);
>   } else {
> LowerMemorySize = GetSystemMemorySizeBelow4gb ();
>+if (FeaturePcdGet (PcdSmmSmramRequire)) {
>+  //

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

[Ni, Ruiyu]

The latest platform recovery patch I sent out was only two commits in local.
I split the two to eleven to avoid you complain again:)

Regards,
Ray

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, November 5, 2015 1:13 AM
To: Ni, Ruiyu 
Cc: Kinney, Michael D ; Tian, Feng 
; edk2-devel@lists.01.org; Fan, Jeff 
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi Ray,

On Tue, Nov 03, 2015 at 02:27:29PM +, Ni, Ruiyu wrote:
> Leif,
> I saw your feedbacks as your opinion if you create patches. I don't
> think your opinion is the *only* right solution.

Oh, certainly. I was only expecting some sort of response before the
patches were committed again.

> I can also split 3/4 in your patch serials to two patches: one is to
> just change DumpBridgeResource(), the other is for the remaining
> changes.
> So I think my splitting way is good enough.

And I will always argue for splitting more :)

> OK I will give response to your questions firstly.

Thanks.

Regards,

Leif
 
> Regards,
> Ray
> 
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
> Sent: Tuesday, November 3, 2015 6:45 PM
> To: Ni, Ruiyu 
> Cc: Tian, Feng ; Kinney, Michael D 
> ; edk2-devel@lists.01.org; Fan, Jeff 
> 
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Hi guys,
> 
> I never saw a response to this feedback I sent on the revised patch
> set, but I see it has now been committed.
> Could you let me know why my feedback was ignored?
> 
> Regards,
> 
> Leif
> 
> On 30 October 2015 at 18:02, Leif Lindholm  wrote:
> > Hi Ray,
> >
> > Many thanks for this.
> >
> > On Fri, Oct 30, 2015 at 04:53:54AM +, Ni, Ruiyu wrote:
> >> Ok I will revert the two patches I committed (big patch + small
> >> patch). But I want to clarify one thing that not every big patch can
> >> be easily understood by just splitting to small patches.
> >
> > Certainly, but it is always helpful to split as much as is possible.
> > The shorter a patch is to review, the more likely reviewers are to
> > spot mistakes - especially when reviewing code they are not already
> > very familiar with.
> >
> > As an example, I have split your set up a little bit further by
> > breaking 2/3 into two separate patches.
> > https://git.linaro.org/people/leif.lindholm/edk2.git/shortlog/refs/heads/ray-pci
> >
> > I have also tried to remove modifications completely unrelated to this
> > fix - resulting in a difference between the tree with your current
> > patch set and my variant as included below. The changes in your set
> > are all valid changes, but they are not related to the fix.
> >
> > The method behind all of this madness is to make the output of "git
> > blame" as relevant as possible, as well as making automated "git
> > bisect" sessions for tracking down which specific change caused an
> > issue more useful.
> >
> > Best Regards,
> >
> > Leif
> >
> >
> > Diff between Ray's v2 and my version of it:
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > index 12647be..523c698 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > @@ -281,13 +281,13 @@ DumpResourceMap (
> >IN UINTN ResourceCount
> >)
> >  {
> > -  EFI_STATUS   Status;
> > -  LIST_ENTRY   *Link;
> > -  PCI_IO_DEVICE*Device;
> > -  UINTNIndex;
> > -  CHAR16   *Str;
> > -  PCI_RESOURCE_NODE**ChildResources;
> > -  UINTNChildResourceCount;
> > +  EFI_STATUS   Status;^M
> > +  LIST_ENTRY   *Link;^M
> > +  PCI_IO_DEVICE*Device;^M
> > +  UINTNIndex;^M
> > +  CHAR16   *Str;^M
> > +  PCI_RESOURCE_NODE**ChildResources;^M
> > +  UINTNChildResourceCount;^M
> >
> >DEBUG ((EFI_D_INFO, "PciBus: Resource Map for "));
> >
> > @@ -976,7 +976,7 @@ PciScanBus (
> >UINT8 Device;
> >UINT8 Func;
> >UINT64Address;
> > -  UINT8 SecondBus;
> > +  UINTN SecondBus;^M
> >UINT8 PaddedSubBus;
> >UINT16Register;
> >UINTN HpIndex;
> > @@ -1211,7 +1211,7 @@ PciScanBus (
> >
> >Status = PciScanBus (
> >  PciDevice,
> > -SecondBus,
> > +(UINT8) (SecondBus),^M
> >  SubBusNumber,
> >

[edk2] 主旨： Re: [PATCH v2] NetworkPkg:Enable Http Boot over Ipv6 stack

[Gary Lin]

>>> "Fu, Siyuan"  2015/11/5 上午 8:44 >>>
> Hi, Lin
>
> If you are using IPv6 address in URL you should enclose the IP address within 
> square brackets ("[" and "]"), like
> http://[2000::1]:3260/boot.efi
> Please refer to RFC3986
> http://tools.ietf.org/html/rfc3986#section-3.2.2

Hi Siyuan,

Here is my testing url: http://[2001:db8:f00f:cafe::1]/shim.efi

NetHttpParseAuthorityChar() returned UrlParserPortStart when it encountered a
colon so NetHttpParseAuthority() thought the host was "[2001" and the port
was "db8:f00f:cafe::1]". Since "[2001" is not a valid IPv6 address, in the end,
HttpBootParseDhcp6Packet() set IpExpressedUri to FALSE, and the IP address was
never extracted from the the url.

Cheers,

Gary Lin

> Siyuan

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Gary 
Ching-Pang Lin
Sent: Wednesday, November 4, 2015 6:40 PM
To: Zhang, Lubo 
Cc: Ye, Ting ; edk2-devel@lists.01.org; Wu, Jiaxin 
; Fu, Siyuan 
Subject: Re: [edk2] [PATCH v2] NetworkPkg:Enable Http Boot over Ipv6 stack

On Tue, Nov 03, 2015 at 04:13:27PM +0800, Zhang Lubo wrote:
> v2:
> *Rewrite the logic of setting ipv6 gateway in httpboot driver and update some 
> comment.
> 
> Fix a potential bug which can cause assert when setting ipv6 Gateway. 
> If there is no router in network environment, it will fail to find out 
> the gateway address which can route the message to Http Server Ip . 
> When it check the IP6 route table again 1 seconds later, it will 
> assert while the router table structure had been freed last time without 
> retrieve new relevant data form ip6 driver.
> 

Hi Lubo,

I applied the patch and it worked as expected except the IP expressed url. I 
checked the code and found the function(*) to parse the url couldn't handle 
IPv6 and just treat the colon as a separator of the host name and port.
Is this a known issue?

Cheers,

Gary Lin

(*) MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c::NetHttpParseAuthority()

> Cc: Fu Siyuan 
> Cc: Ye Ting 
> CC: Wu Jiaxin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo 
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c|  234 -
>  NetworkPkg/HttpBootDxe/HttpBootComponentName.c |5 +-
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.c |   12 +-
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.h |   11 +
>  NetworkPkg/HttpBootDxe/HttpBootDhcp6.c |  984 +++
>  NetworkPkg/HttpBootDxe/HttpBootDhcp6.h |  198 
>  NetworkPkg/HttpBootDxe/HttpBootDxe.c   |  903 +++---
>  NetworkPkg/HttpBootDxe/HttpBootDxe.h   |  159 +++-
>  NetworkPkg/HttpBootDxe/HttpBootDxe.inf |9 +
>  NetworkPkg/HttpBootDxe/HttpBootImpl.c  |  109 ++-
>  NetworkPkg/HttpBootDxe/HttpBootImpl.h  |2 +-
>  NetworkPkg/HttpBootDxe/HttpBootSupport.c   |  292 +-
>  NetworkPkg/HttpBootDxe/HttpBootSupport.h   |   70 ++
>  NetworkPkg/HttpDxe/HttpDns.c   |  207 +++-
>  NetworkPkg/HttpDxe/HttpDns.h   |   20 +
>  NetworkPkg/HttpDxe/HttpDriver.c|  806 +++-
>  NetworkPkg/HttpDxe/HttpDriver.h|  143 ++-
>  NetworkPkg/HttpDxe/HttpDxe.inf |5 +
>  NetworkPkg/HttpDxe/HttpImpl.c  |  358 +++
>  NetworkPkg/HttpDxe/HttpImpl.h  |1 -
>  NetworkPkg/HttpDxe/HttpProto.c | 1210 
> 
>  NetworkPkg/HttpDxe/HttpProto.h |  182 +++-
>  22 files changed, 5017 insertions(+), 903 deletions(-)  create mode 
> 100644 NetworkPkg/HttpBootDxe/HttpBootDhcp6.c
>  create mode 100644 NetworkPkg/HttpBootDxe/HttpBootDhcp6.h
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://listsedk2-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 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Laszlo Ersek]

On 11/04/15 16:19, Ard Biesheuvel wrote:
> On 4 November 2015 at 13:17, Cohen, Eugene  wrote:
>>> The set/way operations are really only suitable for managing the caches 
>>> themselves
>>
>> This makes sense to me and I agree that the majority of developers should 
>> only be dealing with managing buffers and should only use the 
>> VA/address-range interface.
>>
> 
> OK. Does that mean you agree that blindly wiring up the generic
> WriteBackDataCache/InvalidateDataCache entry points to set/way cache
> maintenance is a bad idea?
> 
>>> there are examples of architecturally compliant systems with system level 
>>> caches where cleaned data has been observed to only make it to main memory 
>>> if you clean by VA.
>>
>> I fully expect this given the nature of the ARM architecture - the scope 
>> architectural specifications stop at the interconnect so if somebody wants 
>> to implement wacky hardware then there's nothing to stop them.  Presumably 
>> for whole-cache management some extra stuff is required on these systems to 
>> make it work - which is fine although not that helpful for interoperability 
>> purposes.
>>
>>> But seriously, I understand that there is a performance concern here, but 
>>> 'promoting' a clean by VA operation to clean by set/way really makes no 
>>> sense at all.
>>
>> Defaulting to safe/correct for all architectures makes sense to me.  But it 
>> would be nice for those of us that understand our architectures (no system 
>> level caches, SMP config understood) to choose an option to turn on 
>> thresholding through a PCD setting or library class selection.
>>
> 
> The problem remains that VA and set/way ops are completely different
> things. Each by-VA operation handles all copies of the same cacheline
> throughout the cache hierarchy at once. The set/way ops, on the other
> hand, traverse each cache level in turn, leaving a time window between
> the completion of the L1 maintenance and the completion of the L2
> maintenance where speculative accesses resulting in L1 misses are
> happily served from the L2 and allocated in L1 -- unless your MMU and
> I-cache are off. This means the logic that promotes from VA to set/way
> ops should take MMU and I-cache state into account as well, and I am
> not sure you would want to go there.

Thanks for this great summary.

So, why doesn't ARM provide instructions for flushing / invalidating the
complete cache? Just curious.

Thanks
Laszlo

> 
>>> I am not proposing to remove this functionality.
>>
>> Ok - I may have interpreted patch 4 as removing more than PoU but I can see 
>> it's just removing entire-cache flushing to PoU - this is fine by me since 
>> cache maintenance for code loading should always reference the relevant VAs.
>>
> 
> Indeed.
> 
>> Editorial: I know there's been a lot of talk about what tools to use to 
>> review code and a lot of people like email - but for me it's hard for me to 
>> get proper context with 10 patches in email when I don't work on the code 
>> every day - I'd like to think if I saw these in a tool like gerrit I would 
>> have had proper context sooner.
>>
> 
> I personally quite like Gerrit, but I don't think we are likely to
> start using it for EDK2. As Laszlo pointed out, I did not push out the
> series to a public branch this time. I will make sure to do that for
> the v2.
> 
>> Thanks for the context - glad we have you working on this.  :)
>>
> 
> Thanks.
> 

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

Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Ard Biesheuvel]

On 4 November 2015 at 13:17, Cohen, Eugene  wrote:
>> The set/way operations are really only suitable for managing the caches 
>> themselves
>
> This makes sense to me and I agree that the majority of developers should 
> only be dealing with managing buffers and should only use the 
> VA/address-range interface.
>

OK. Does that mean you agree that blindly wiring up the generic
WriteBackDataCache/InvalidateDataCache entry points to set/way cache
maintenance is a bad idea?

>> there are examples of architecturally compliant systems with system level 
>> caches where cleaned data has been observed to only make it to main memory 
>> if you clean by VA.
>
> I fully expect this given the nature of the ARM architecture - the scope 
> architectural specifications stop at the interconnect so if somebody wants to 
> implement wacky hardware then there's nothing to stop them.  Presumably for 
> whole-cache management some extra stuff is required on these systems to make 
> it work - which is fine although not that helpful for interoperability 
> purposes.
>
>> But seriously, I understand that there is a performance concern here, but 
>> 'promoting' a clean by VA operation to clean by set/way really makes no 
>> sense at all.
>
> Defaulting to safe/correct for all architectures makes sense to me.  But it 
> would be nice for those of us that understand our architectures (no system 
> level caches, SMP config understood) to choose an option to turn on 
> thresholding through a PCD setting or library class selection.
>

The problem remains that VA and set/way ops are completely different
things. Each by-VA operation handles all copies of the same cacheline
throughout the cache hierarchy at once. The set/way ops, on the other
hand, traverse each cache level in turn, leaving a time window between
the completion of the L1 maintenance and the completion of the L2
maintenance where speculative accesses resulting in L1 misses are
happily served from the L2 and allocated in L1 -- unless your MMU and
I-cache are off. This means the logic that promotes from VA to set/way
ops should take MMU and I-cache state into account as well, and I am
not sure you would want to go there.

>> I am not proposing to remove this functionality.
>
> Ok - I may have interpreted patch 4 as removing more than PoU but I can see 
> it's just removing entire-cache flushing to PoU - this is fine by me since 
> cache maintenance for code loading should always reference the relevant VAs.
>

Indeed.

> Editorial: I know there's been a lot of talk about what tools to use to 
> review code and a lot of people like email - but for me it's hard for me to 
> get proper context with 10 patches in email when I don't work on the code 
> every day - I'd like to think if I saw these in a tool like gerrit I would 
> have had proper context sooner.
>

I personally quite like Gerrit, but I don't think we are likely to
start using it for EDK2. As Laszlo pointed out, I did not push out the
series to a public branch this time. I will make sure to do that for
the v2.

> Thanks for the context - glad we have you working on this.  :)
>

Thanks.

-- 
Ard.


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Wednesday, November 04, 2015 1:03 AM
> To: Cohen, Eugene 
> Cc: edk2-devel@lists.01.org; leif.lindh...@linaro.org; mark.rutl...@arm.com; 
> ler...@redhat.com
> Subject: Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole 
> D-cache maintenance operations
>
> On 3 November 2015 at 23:19, Cohen, Eugene  wrote:
>> Ard,
>>
>> This is probably a reflection of my lack of deep understanding of ARM cache 
>> maintenance on more complex systems.
>>
>> The "entire cache" operations exist to allow transitions across major 
>> environments (initialization of non-self initializing caches, changing 
>> phases of boot, etc) to clean/clean+invalidate/invalidate cache without 
>> knowing apriori what virtual addresses have dirty data.  The set/way 
>> solution was a matter of convenience since the ARM did not have an 
>> architectural means of operating on the full cache in one instruction.  We 
>> do have cases where we need to turn off caches entirely - booting our OS as 
>> well as in core power management scenarios - I think we have to do a full 
>> cache flush in these cases if not by set/way then somehow.
>>
>
> It probably makes sense to distinguish more clearly between cache ops that 
> manage the /contents/ of the caches, and cache ops that manage the caches 
> themselves. The set/way operations are really only suitable for managing the 
> caches themselves, and core power management is one of the few cases where 
> they are actually appropriate. For managing the contents of the caches, i.e., 
> making sure that all writes have made it to main memory and are not shadowed 
> by stale cache data at any level to a CPU that is doing cacheable accesses, 
> we really only have the 

Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Mark Rutland]

On Wed, Nov 04, 2015 at 12:17:16PM +, Cohen, Eugene wrote:
> > The set/way operations are really only suitable for managing the caches 
> > themselves
> 
> This makes sense to me and I agree that the majority of developers
> should only be dealing with managing buffers and should only use the
> VA/address-range interface.
> 
> > there are examples of architecturally compliant systems with system
> > level caches where cleaned data has been observed to only make it to
> > main memory if you clean by VA.
> 
> I fully expect this given the nature of the ARM architecture - the
> scope architectural specifications stop at the interconnect so if
> somebody wants to implement wacky hardware then there's nothing to
> stop them.

It's worth noting that the (ARMv8) architecture defines some properties
of system-level caches (e.g. that they must respect maintenance by VA),
which ensures that the architected mechanism for managing caches is
portable (i.e. maintenance by VA works everywhere).

If someone builds a system cache that does not respect maintenance by VA
then they have violated the (ARMv8) architecture.

Personally, I expect that system-level caches are likely to become the
common case in server systems, assuming that there are coherent masters
other than CPUs in the system. I don't think we can consider them a
"wacky" niche case.

> Presumably for whole-cache management some extra stuff is required on
> these systems to make it work - which is fine although not that
> helpful for interoperability purposes.
>
> > But seriously, I understand that there is a performance concern
> > here, but 'promoting' a clean by VA operation to clean by set/way
> > really makes no sense at all.
> 
> Defaulting to safe/correct for all architectures makes sense to me.
> But it would be nice for those of us that understand our architectures
> (no system level caches, SMP config understood) to choose an option to
> turn on thresholding through a PCD setting or library class selection.

I am not necessarily opposed to this, however I am a little concerned
that as with other performance options, this is something people may
turn on blindly, regardless of whether such maintenance can actually
provide useful guarantees for their system.

Do we actually see a measurable performance advantage to enabling such
thresholding on systems where it is safe to do so?

What saving do we see, and in what paths?

It's also worth noting that a Set/Way sequence may not be sufficient on
all platforms even in the absence of system-level caches, and other
IMPLEMENTATION DEFINED sequences may be necessary. In the general case.
supporting thresholding would need the platform code to provide its
particular mechanism.

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

Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Cohen, Eugene]

> The set/way operations are really only suitable for managing the caches 
> themselves

This makes sense to me and I agree that the majority of developers should only 
be dealing with managing buffers and should only use the VA/address-range 
interface.

> there are examples of architecturally compliant systems with system level 
> caches where cleaned data has been observed to only make it to main memory if 
> you clean by VA.

I fully expect this given the nature of the ARM architecture - the scope 
architectural specifications stop at the interconnect so if somebody wants to 
implement wacky hardware then there's nothing to stop them.  Presumably for 
whole-cache management some extra stuff is required on these systems to make it 
work - which is fine although not that helpful for interoperability purposes.

> But seriously, I understand that there is a performance concern here, but 
> 'promoting' a clean by VA operation to clean by set/way really makes no sense 
> at all.

Defaulting to safe/correct for all architectures makes sense to me.  But it 
would be nice for those of us that understand our architectures (no system 
level caches, SMP config understood) to choose an option to turn on 
thresholding through a PCD setting or library class selection.

> I am not proposing to remove this functionality.

Ok - I may have interpreted patch 4 as removing more than PoU but I can see 
it's just removing entire-cache flushing to PoU - this is fine by me since 
cache maintenance for code loading should always reference the relevant VAs.

Editorial: I know there's been a lot of talk about what tools to use to review 
code and a lot of people like email - but for me it's hard for me to get proper 
context with 10 patches in email when I don't work on the code every day - I'd 
like to think if I saw these in a tool like gerrit I would have had proper 
context sooner.

Thanks for the context - glad we have you working on this.  :)

Eugene

-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Wednesday, November 04, 2015 1:03 AM
To: Cohen, Eugene 
Cc: edk2-devel@lists.01.org; leif.lindh...@linaro.org; mark.rutl...@arm.com; 
ler...@redhat.com
Subject: Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole 
D-cache maintenance operations

On 3 November 2015 at 23:19, Cohen, Eugene  wrote:
> Ard,
>
> This is probably a reflection of my lack of deep understanding of ARM cache 
> maintenance on more complex systems.
>
> The "entire cache" operations exist to allow transitions across major 
> environments (initialization of non-self initializing caches, changing phases 
> of boot, etc) to clean/clean+invalidate/invalidate cache without knowing 
> apriori what virtual addresses have dirty data.  The set/way solution was a 
> matter of convenience since the ARM did not have an architectural means of 
> operating on the full cache in one instruction.  We do have cases where we 
> need to turn off caches entirely - booting our OS as well as in core power 
> management scenarios - I think we have to do a full cache flush in these 
> cases if not by set/way then somehow.
>

It probably makes sense to distinguish more clearly between cache ops that 
manage the /contents/ of the caches, and cache ops that manage the caches 
themselves. The set/way operations are really only suitable for managing the 
caches themselves, and core power management is one of the few cases where they 
are actually appropriate. For managing the contents of the caches, i.e., making 
sure that all writes have made it to main memory and are not shadowed by stale 
cache data at any level to a CPU that is doing cacheable accesses, we really 
only have the by-VA operations.

> I don't think we can expect any one component (or whatever phase-thunking 
> code) to know what virtual addresses may have dirty data nor can we expect it 
> to loop across all possible cacheable virtual addresses.
>

Then that is a problem. ArmCleanDataCache() does not guarantee that data has 
made it to main memory, so we simply cannot rely on it. Mind you, this is not a 
theoretical debate, there are examples of architecturally compliant systems 
with system level caches where cleaned data has been observed to only make it 
to main memory if you clean by VA. That is the rationale behind this particular 
patch: the use of 'WriteBackDataCache()' in generic code imposes a requirement 
that we cannot fulfil, and it is better to flag it than to pretend we can 
guarantee that all data has made its way to main memory after the call returns.

> I also see that the "threshold" based modification of the cache flush from 
> MVA to entire-cache was removed earlier this year (I missed that!).  Have you 
> quantified the performance impact from removing of the threshold methodology?
>

That fixed an actual bug. The performance went from 'could not boot'
to 'could boot' :-) But seriously, I understand that there is a 

Re: [edk2] [PATCH v3 2/2] ShellPkg/UefiDpLib: Support dumping cumulative data

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Zeng, Star
> Sent: Tuesday, November 03, 2015 5:01 PM
> To: Cinnamon Shia ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v3 2/2] ShellPkg/UefiDpLib: Support dumping
> cumulative data
> Importance: High
> 
> On 2015/11/4 0:55, Cinnamon Shia wrote:
> > Add a new option -c to dump cumulative data.
> > For example:
> > shell> dp -c
> > ==[ Cumulative ]
> > (Times in microsec.) Cumulative   Average ShortestLongest
> > Name  CountDurationDurationDurationDuration
> > LoadImage: 200 1007000   0  10
> > StartImage:2002000   9   0 700
> >DB:Start:2002000  10   0 900
> > DB:Support: 20  10   0   07000
> >
> > shell> dp -c DXE
> > ==[ Cumulative ]
> > (Times in microsec.) Cumulative   Average ShortestLongest
> > Name  CountDurationDurationDurationDuration
> > LoadImage: 200 1007000   0  10
> > StartImage:2002000   9   0 700
> >DB:Start:2002000  10   0 900
> > DB:Support: 20  10   0   07000
> >  DXE  130003000   03000
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Cinnamon Shia 
> > ---
> >   ShellPkg/Library/UefiDpLib/Dp.c  |  33 +--
> >   ShellPkg/Library/UefiDpLib/DpInternal.h  |   9 +++--
> >   ShellPkg/Library/UefiDpLib/DpTrace.c |  55
> ---
> >   ShellPkg/Library/UefiDpLib/UefiDpLib.uni | Bin 17466 -> 18146 bytes
> >   4 files changed, 87 insertions(+), 10 deletions(-)
> 
> Reviewed-by: Star Zeng 
> 
> >
> > diff --git a/ShellPkg/Library/UefiDpLib/Dp.c
> b/ShellPkg/Library/UefiDpLib/Dp.c
> > index 62a4e7b..4d109d0 100644
> > --- a/ShellPkg/Library/UefiDpLib/Dp.c
> > +++ b/ShellPkg/Library/UefiDpLib/Dp.c
> > @@ -79,6 +79,7 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
> >   #endif // PROFILING_IMPLEMENTED
> > {L"-x", TypeFlag},   // -x   eXclude Cumulative Items
> > {L"-i", TypeFlag},   // -i   Display Identifier
> > +  {L"-c", TypeValue},  // -c   Display cumulative data.
> > {L"-n", TypeValue},  // -n # Number of records to display for A and R
> > {L"-t", TypeValue},  // -t # Threshold of interest
> > {NULL, TypeMax}
> > @@ -164,6 +165,9 @@ ShellCommandRunDp (
> > BOOLEAN   TraceMode;
> > BOOLEAN   ProfileMode;
> > BOOLEAN   ExcludeMode;
> > +  BOOLEAN   CumulativeMode;
> > +  CONST CHAR16  *CustomCumulativeToken;
> > +  PERF_CUM_DATA *CustomCumulativeData;
> >
> > StringPtr   = NULL;
> > SummaryMode = FALSE;
> > @@ -173,6 +177,8 @@ ShellCommandRunDp (
> > TraceMode   = FALSE;
> > ProfileMode = FALSE;
> > ExcludeMode = FALSE;
> > +  CumulativeMode = FALSE;
> > +  CustomCumulativeData = NULL;
> >
> > // Get DP's entry time as soon as possible.
> > // This is used as the Shell-Phase end time.
> > @@ -210,6 +216,7 @@ ShellCommandRunDp (
> >   #endif  // PROFILING_IMPLEMENTED
> > ExcludeMode = ShellCommandLineGetFlag (ParamPackage, L"-x");
> > mShowId = ShellCommandLineGetFlag (ParamPackage, L"-i");
> > +  CumulativeMode = ShellCommandLineGetFlag (ParamPackage, L"-c");
> >
> > // Options with Values
> > CmdLineArg  = ShellCommandLineGetValue (ParamPackage, L"-n");
> > @@ -244,6 +251,20 @@ ShellCommandRunDp (
> > InitCumulativeData ();
> >
> > //
> > +  // Init the custom cumulative data.
> > +  //
> > +  CustomCumulativeToken = ShellCommandLineGetValue (ParamPackage,
> L"-c");
> > +  if (CustomCumulativeToken != NULL) {
> > +CustomCumulativeData = AllocateZeroPool (sizeof (PERF_CUM_DATA));
> > +CustomCumulativeData->MinDur = 0;
> > +CustomCumulativeData->MaxDur = 0;
> > +CustomCumulativeData->Count  = 0;
> > +CustomCumulativeData->Duration = 0;
> > +CustomCumulativeData->Name   = AllocateZeroPool (StrLen
> (CustomCumulativeToken) + 1);
> > +UnicodeStrToAsciiStr (CustomCumulativeToken, CustomCumulativeData-
> >Name);
> > +  }
> > +
> > +  //
> > // Timer specific processing
> > //
> > // Get the Performance counter characteristics:
> > @@ -302,8 +323,10 @@ ShellCommandRunDp (
> >   !T &&  P  := (2) Only Profile records are displayed
> >    T &&  P  := (3) Same as Default, both are displayed
> >
> 
> /
> > -  GatherStatistics();
> > 

Re: [edk2] [PATCH v3 1/2] ShellPkg/UefiDpLib: Fix a DP cumulative data issue

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Zeng, Star
> Sent: Tuesday, November 03, 2015 5:01 PM
> To: Cinnamon Shia ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v3 1/2] ShellPkg/UefiDpLib: Fix a DP cumulative
> data issue
> Importance: High
> 
> On 2015/11/4 0:55, Cinnamon Shia wrote:
> > The value of PERF_CUM_DATA.Count and PERF_CUM_DATA.Duration field
> > keep cumulating on every execution of dp.
> > Initialize the CumData at dp's entry point.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Cinnamon Shia 
> > ---
> >   ShellPkg/Library/UefiDpLib/Dp.c | 27 ++-
> >   1 file changed, 26 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Star Zeng 
> 
> >
> > diff --git a/ShellPkg/Library/UefiDpLib/Dp.c
> b/ShellPkg/Library/UefiDpLib/Dp.c
> > index 8270172..62a4e7b 100644
> > --- a/ShellPkg/Library/UefiDpLib/Dp.c
> > +++ b/ShellPkg/Library/UefiDpLib/Dp.c
> > @@ -14,6 +14,7 @@
> > timer information to calculate elapsed time for each measurement.
> >
> > Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.
> > +  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of the
> BSD License
> > which accompanies this distribution.  The full text of the license may 
> > be
> found at
> > @@ -110,7 +111,26 @@ DumpStatistics( void )
> > SHELL_FREE_NON_NULL (StringPtrUnknown);
> >   }
> >
> > -/**
> > +/**
> > +  Initialize the cumulative data.
> > +
> > +**/
> > +VOID
> > +InitCumulativeData (
> > +  VOID
> > +  )
> > +{
> > +  UINTN Index;
> > +
> > +  for (Index = 0; Index < NumCum; ++Index) {
> > +CumData[Index].Count = 0;
> > +CumData[Index].MinDur = PERF_MAXDUR;
> > +CumData[Index].MaxDur = 0;
> > +CumData[Index].Duration = 0;
> > +  }
> > +}
> > +
> > +/**
> > Dump performance data.
> >
> > @param[in]  ImageHandle The image handle.
> > @@ -219,6 +239,11 @@ ShellCommandRunDp (
> > }
> >
> > //
> > +  // Initialize the pre-defined cumulative data.
> > +  //
> > +  InitCumulativeData ();
> > +
> > +  //
> > // Timer specific processing
> > //
> > // Get the Performance counter characteristics:
> >
> 
> ___
> 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 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Mark Rutland]

On Wed, Nov 04, 2015 at 04:24:20PM +0100, Laszlo Ersek wrote:
> On 11/04/15 16:19, Ard Biesheuvel wrote:

[...]

> > The problem remains that VA and set/way ops are completely different
> > things. Each by-VA operation handles all copies of the same cacheline
> > throughout the cache hierarchy at once. The set/way ops, on the other
> > hand, traverse each cache level in turn, leaving a time window between
> > the completion of the L1 maintenance and the completion of the L2
> > maintenance where speculative accesses resulting in L1 misses are
> > happily served from the L2 and allocated in L1 -- unless your MMU and
> > I-cache are off. This means the logic that promotes from VA to set/way
> > ops should take MMU and I-cache state into account as well, and I am
> > not sure you would want to go there.
> 
> Thanks for this great summary.
> 
> So, why doesn't ARM provide instructions for flushing / invalidating the
> complete cache? Just curious.

I don't have a an official answer, but there are several factors that
spring to mind.

Because of system-level coherency, dirty cacheline migration, and so on,
performing an operation on the "complete cache" would mean performing
the operation on every cache in the system (e.g. CPUs, system L3,
coherent devices, etc). That's an enormous amount of data and associated
state to keep track of, and it could take an extremely long time for
such maintenance to complete.

It's almost never necessary to clean or invalidate the entire cache
hierarchy in a system. In practically every scenario, maintenance by VA
can be used to provide the necessary guarantees, so the complexity of
implementing the above can be avoided.

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

Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Kinney, Michael D]

Laszlo,

BaseXApicX2ApicLib is intended to be used by platforms that support more >=256 
CPUs.

If the current system configuration is < 256 CPUs, then the platform will 
typically stay in APIC mode.  If >= 256 CPUs are detected, then X2 APIC mode 
can be enabled using the following API.

VOID
EFIAPI
SetApicMode (
  IN UINTN  ApicMode
  )

So just adding BaseXApicX2ApicLib to the DSC does not enable X2 APIC mode.  You 
have to add logic to enable X2 APIC mode.

I see that QEMU is limited to 255 VCPUs, so the use of BaseXApicLib makes 
sense.  Are OVMF configurations supported with >= 256 VCPUs?

Thanks,

Mike

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Wednesday, November 04, 2015 2:41 AM
>To: Paolo Bonzini; edk2-de...@ml01.01.org; Kinney, Michael D; Fan, Jeff;
>Justen, Jordan L
>Subject: Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance
>with x2apic support if SMM_REQUIRE
>
>On 11/04/15 09:48, Paolo Bonzini wrote:
>>
>>
>> On 03/11/2015 22:00, Laszlo Ersek wrote:
>>> +
>>> +!if $(SMM_REQUIRE) == TRUE
>>> +
>LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>>> +!else
>>>LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>>> +!endif
>>> +
>>
>> Can we enable BaseXApicX2ApicLib unconditionally?
>
>I think I am technically capable of that :), but should we? We haven't
>used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
>it could regress stuff or not. If it causes problems with the
>SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
>such a global change for the traditional build.
>
>I'm not against it, I just don't have experience with BaseXApicX2ApicLib.
>
>Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
>and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always used
>the former? If it has only been for simplicity's sake, and
>BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
>think Paolo is right, and we should just keep the OVMF DSCs simple.
>
>Thanks
>Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole D-cache maintenance operations

[Andrew Fish]

> On Nov 4, 2015, at 7:41 AM, Mark Rutland  wrote:
> 
> On Wed, Nov 04, 2015 at 04:24:20PM +0100, Laszlo Ersek wrote:
>> On 11/04/15 16:19, Ard Biesheuvel wrote:
> 
> [...]
> 
>>> The problem remains that VA and set/way ops are completely different
>>> things. Each by-VA operation handles all copies of the same cacheline
>>> throughout the cache hierarchy at once. The set/way ops, on the other
>>> hand, traverse each cache level in turn, leaving a time window between
>>> the completion of the L1 maintenance and the completion of the L2
>>> maintenance where speculative accesses resulting in L1 misses are
>>> happily served from the L2 and allocated in L1 -- unless your MMU and
>>> I-cache are off. This means the logic that promotes from VA to set/way
>>> ops should take MMU and I-cache state into account as well, and I am
>>> not sure you would want to go there.
>> 
>> Thanks for this great summary.
>> 
>> So, why doesn't ARM provide instructions for flushing / invalidating the
>> complete cache? Just curious.
> 
> I don't have a an official answer, but there are several factors that
> spring to mind.
> 
> Because of system-level coherency, dirty cacheline migration, and so on,
> performing an operation on the "complete cache" would mean performing
> the operation on every cache in the system (e.g. CPUs, system L3,
> coherent devices, etc). That's an enormous amount of data and associated
> state to keep track of, and it could take an extremely long time for
> such maintenance to complete.
> 
> It's almost never necessary to clean or invalidate the entire cache
> hierarchy in a system.

Well some how those almost cases always seem to show up in firmware :). 

> In practically every scenario, maintenance by VA
> can be used to provide the necessary guarantees, so the complexity of
> implementing the above can be avoided.
> 

If we step back and think about it a little
We have the generic edk2 cache maintenance, and the only one that comes to mind 
is flushing the instruction cache after PE/COFF images are loaded. Then there 
is the cache init/maintenance required by the CPU architecture so setting up 
MMUs, (MTRRs on x86), changing system mappings, and for ARM DMA. 

So it would be good to make the common cases clear. For example the Cache 
Operations in the MdePkg started out as the set of operations you need to do 
generically for an x86 or Itanium processor. 

Thanks,

Andrew Fish

> Thanks,
> Mark.
> ___
> 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] C Coding Standards Specification disappeared

[Bruce Cran]

On 11/4/15 9:44 AM, Jarlstrom, Laurie wrote:

The Previous C Coding Standards Guide is currently being updated.  A new 
revision will be posted shortly.


Could someone post that information on the website please?

By 'website' I guess I mean both the sourceforge _and_ github wikis, 
since for some reason they're both active.



--
Bruce

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

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

[Leif Lindholm]

Hi Ray,

On Tue, Nov 03, 2015 at 02:27:29PM +, Ni, Ruiyu wrote:
> Leif,
> I saw your feedbacks as your opinion if you create patches. I don't
> think your opinion is the *only* right solution.

Oh, certainly. I was only expecting some sort of response before the
patches were committed again.

> I can also split 3/4 in your patch serials to two patches: one is to
> just change DumpBridgeResource(), the other is for the remaining
> changes.
> So I think my splitting way is good enough.

And I will always argue for splitting more :)

> OK I will give response to your questions firstly.

Thanks.

Regards,

Leif
 
> Regards,
> Ray
> 
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
> Sent: Tuesday, November 3, 2015 6:45 PM
> To: Ni, Ruiyu 
> Cc: Tian, Feng ; Kinney, Michael D 
> ; edk2-devel@lists.01.org; Fan, Jeff 
> 
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Hi guys,
> 
> I never saw a response to this feedback I sent on the revised patch
> set, but I see it has now been committed.
> Could you let me know why my feedback was ignored?
> 
> Regards,
> 
> Leif
> 
> On 30 October 2015 at 18:02, Leif Lindholm  wrote:
> > Hi Ray,
> >
> > Many thanks for this.
> >
> > On Fri, Oct 30, 2015 at 04:53:54AM +, Ni, Ruiyu wrote:
> >> Ok I will revert the two patches I committed (big patch + small
> >> patch). But I want to clarify one thing that not every big patch can
> >> be easily understood by just splitting to small patches.
> >
> > Certainly, but it is always helpful to split as much as is possible.
> > The shorter a patch is to review, the more likely reviewers are to
> > spot mistakes - especially when reviewing code they are not already
> > very familiar with.
> >
> > As an example, I have split your set up a little bit further by
> > breaking 2/3 into two separate patches.
> > https://git.linaro.org/people/leif.lindholm/edk2.git/shortlog/refs/heads/ray-pci
> >
> > I have also tried to remove modifications completely unrelated to this
> > fix - resulting in a difference between the tree with your current
> > patch set and my variant as included below. The changes in your set
> > are all valid changes, but they are not related to the fix.
> >
> > The method behind all of this madness is to make the output of "git
> > blame" as relevant as possible, as well as making automated "git
> > bisect" sessions for tracking down which specific change caused an
> > issue more useful.
> >
> > Best Regards,
> >
> > Leif
> >
> >
> > Diff between Ray's v2 and my version of it:
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > index 12647be..523c698 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > @@ -281,13 +281,13 @@ DumpResourceMap (
> >IN UINTN ResourceCount
> >)
> >  {
> > -  EFI_STATUS   Status;
> > -  LIST_ENTRY   *Link;
> > -  PCI_IO_DEVICE*Device;
> > -  UINTNIndex;
> > -  CHAR16   *Str;
> > -  PCI_RESOURCE_NODE**ChildResources;
> > -  UINTNChildResourceCount;
> > +  EFI_STATUS   Status;^M
> > +  LIST_ENTRY   *Link;^M
> > +  PCI_IO_DEVICE*Device;^M
> > +  UINTNIndex;^M
> > +  CHAR16   *Str;^M
> > +  PCI_RESOURCE_NODE**ChildResources;^M
> > +  UINTNChildResourceCount;^M
> >
> >DEBUG ((EFI_D_INFO, "PciBus: Resource Map for "));
> >
> > @@ -976,7 +976,7 @@ PciScanBus (
> >UINT8 Device;
> >UINT8 Func;
> >UINT64Address;
> > -  UINT8 SecondBus;
> > +  UINTN SecondBus;^M
> >UINT8 PaddedSubBus;
> >UINT16Register;
> >UINTN HpIndex;
> > @@ -1211,7 +1211,7 @@ PciScanBus (
> >
> >Status = PciScanBus (
> >  PciDevice,
> > -SecondBus,
> > +(UINT8) (SecondBus),^M
> >  SubBusNumber,
> >  PaddedBusRange
> >  );
> > @@ -1227,7 +1227,7 @@ PciScanBus (
> >if ((Attributes == EfiPaddingPciRootBridge) &&
> >(State & EFI_HPC_STATE_ENABLED) != 0&&
> >(State & EFI_HPC_STATE_INITIALIZED) != 0) {
> > -*PaddedBusRange = (UINT8) ((UINT8) (BusRange) + 
> > *PaddedBusRange);
> > +*PaddedBusRange = (UINT8) ((UINT8) (BusRange) 
> > +*PaddedBusRange);^M
> >} else {
> >  //
> >  // Reserve the larger one between the 

Re: [edk2] C Coding Standards Specification disappeared

[Jarlstrom, Laurie]

Here is a link to a Draft to the 2.1 version :  
https://sourceforge.net/projects/edk2/files/Specifications/CCS_2_1_Draft.pdf/download
 

thanks,
Laurie
 
laurie.jarlst...@intel.com

Intel SSG/STO/EBP
(503) 712-9395



-Original Message-
From: Bruce Cran [mailto:br...@cran.org.uk] 
Sent: Wednesday, November 04, 2015 8:54 AM
To: Jarlstrom, Laurie; Sheng, Cecil (HPS SW); edk2-devel@lists.01.org
Subject: Re: [edk2] C Coding Standards Specification disappeared

On 11/4/15 9:44 AM, Jarlstrom, Laurie wrote:
> The Previous C Coding Standards Guide is currently being updated.  A new 
> revision will be posted shortly.

Could someone post that information on the website please?

By 'website' I guess I mean both the sourceforge _and_ github wikis, since for 
some reason they're both active.


-- 
Bruce

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

Re: [edk2] [Patch 3/3] UefiCpuPkg/CpuDxe: Place APs into protected mode when ExitBootService

[Paolo Bonzini]

On 26/10/2015 23:31, Laszlo Ersek wrote:
> > If QEMU could evaluate the AP state and not send an SMI to an AP in
> > Wait-forSIPI, then updating SMIs to broadcast to all AP should work
> > for SeaBios and OVMF.

Yup, this has to be fixed in both QEMU and KVM (separately).

I'm not 100% sure of how the bug happens in KVM, because KVM does
something really weird (and unintended).  The vCPU _is_ in SMM, but it
doesn't start running.  When you get the next SIPI, the vCPU runs the
startup vector in SMM rather than real mode.  Still has to be fixed, of
course.

> >  All APs in SeaBios are in Wait-for-SIPI, so
> > the BSP will be the only CPU that receives the SMI.  In OVMF, all APs
> > can be in the HLT loop that Jeff Fan provided a patch for (and I see
> > you have a good update for), so the SMI can be delivered to BSP and
> > all APs.
>
> I'd like to run this idea by Paolo (CC'd). Theoretically I might be able
> to respin my QEMU patch so that it looks at the APs' states right when
> one of the VCPUs writes to APM_CNT. But, as far as I recall, in QEMU it
> is two separate actions to generate / make an interrupt pending, and
> then to deliver it. Filtering out the SMI at generation time might not
> be the right thing to do. Doing it in the delivery / injection code
> seems possible, but that code is super quirky and I don't dare touch it.
> (Worse, I think that code might exist separately for TCG (in QEMU) and
> KVM (in the host kernel).)

I'm not sure where it's better to filter it out.  I'll have to look a
bit more at the code.

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

Re: [edk2] [Patch 3/3] UefiCpuPkg/CpuDxe: Place APs into protected mode when ExitBootService

[Paolo Bonzini]

On 27/10/2015 03:12, Fan, Jeff wrote:
> Yes. On physical hw, Aps will not response SMI if Aps received SMI in
> WFSI state. But Aps will have one pending SMI and will enter into SMM
> once Aps receive Startup IPI.

Interesting... so if the BIOS doesn't do SMBASE relocation, an
INIT-SMI-SIPI sequence will run code at 0x3 in system management
mode---thus letting the OS poke at SMRAM?

Related to this, how is SMBASE relocation handled in the case where CPUs
are hotplugged?  Is there a race between any firmware code that does
SMBASE relocation for the new code, and the OS which could overwrite the
SMBASE relocation stub at address 0x3?

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

Re: [edk2] [PATCH 10/10] ArmPlatformPkg: do not invalidate the entire data cache at startup

[Ard Biesheuvel]

On 3 November 2015 at 11:16, Ard Biesheuvel  wrote:
> Drop the call to ArmInvalidateDataCache () from the PrePi and PrePeiCore
> startup sequences. This kind of data cache maintenance should not be
> performed at the UEFI firmware level.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPlatformPkg/PrePeiCore/PrePeiCore.c | 2 --
>  ArmPlatformPkg/PrePi/PrePi.c   | 2 --
>  2 files changed, 4 deletions(-)
>

Note that I am keeping the call to ArmInvalidateDataCache () in
ArmPlatformPkg/Sec/Sec.c, which is the only one that actually makes
sense, since it is executed before ArmCpuSetup() (which is where SMP
coherency gets enabled). IOW, here we are managing the state of the
CPU and its caches straight out of reset, which is a legal use of
set/way cache maintenance ops.


> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c 
> b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> index 491d7a6f851f..42877e75aea3 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> @@ -73,8 +73,6 @@ CEntryPoint (
>  {
>// Data Cache enabled on Primary core when MMU is enabled.
>ArmDisableDataCache ();
> -  // Invalidate Data cache
> -  ArmInvalidateDataCache ();
>// Invalidate instruction cache
>ArmInvalidateInstructionCache ();
>// Enable Instruction Caches on all cores.
> diff --git a/ArmPlatformPkg/PrePi/PrePi.c b/ArmPlatformPkg/PrePi/PrePi.c
> index 99afe6fa9064..871487ff1c4f 100755
> --- a/ArmPlatformPkg/PrePi/PrePi.c
> +++ b/ArmPlatformPkg/PrePi/PrePi.c
> @@ -225,8 +225,6 @@ CEntryPoint (
>
>// Data Cache enabled on Primary core when MMU is enabled.
>ArmDisableDataCache ();
> -  // Invalidate Data cache
> -  ArmInvalidateDataCache ();
>// Invalidate instruction cache
>ArmInvalidateInstructionCache ();
>// Enable Instruction Caches on all cores.
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 13/41] OvmfPkg: LockBox: -D SMM_REQUIRE excludes our fake lockbox

[Paolo Bonzini]

On 03/11/2015 22:00, Laszlo Ersek wrote:
> When the user builds OVMF with -D SMM_REQUIRE, our LockBox implementation
> must not be used, since it doesn't actually protect data in the LockBox
> from the runtime guest OS. Add an according assert to
> LockBoxLibInitialize().
> 
> Furthermore, since our LockBox must not be selected with -D SMM_REQUIRE,
> it makes sense to set aside memory for it only if -D SMM_REQUIRE is
> absent. Modify InitializeRamRegions() accordingly.
> 
> This patch completes the -D SMM_REQUIRE-related tweaking of the special
> OVMF memory areas.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf |  3 ++
>  OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf  |  3 ++
>  OvmfPkg/Library/LockBoxLib/LockBoxLib.c   |  2 +
>  OvmfPkg/PlatformPei/MemDetect.c   | 40 ++--
>  4 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf 
> b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> index 7203d07..81c893e 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
> @@ -42,3 +42,6 @@ [LibraryClasses]
>  [Pcd]
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf 
> b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> index a4d27a5..08973a4 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> @@ -43,3 +43,6 @@ [LibraryClasses]
>  [Pcd]
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> +
> +[FeaturePcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c 
> b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> index 89050ac..45481b9 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
> @@ -44,6 +44,8 @@ LockBoxLibInitialize (
>  {
>UINTN NumEntries;
>  
> +  ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
> +
>if (PcdGet32 (PcdOvmfLockBoxStorageSize) < sizeof (LOCK_BOX_GLOBAL)) {
>  return RETURN_UNSUPPORTED;
>}
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 1bdc2df..455fcbb 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -407,25 +407,27 @@ InitializeRamRegions (
>}
>  
>if (mBootMode != BOOT_ON_S3_RESUME) {
> -//
> -// Reserve the lock box storage area
> -//
> -// Since this memory range will be used on S3 resume, it must be
> -// reserved as ACPI NVS.
> -//
> -// If S3 is unsupported, then various drivers might still write to the
> -// LockBox area. We ought to prevent DXE from serving allocation requests
> -// such that they would overlap the LockBox storage.
> -//
> -ZeroMem (
> -  (VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> -  (UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
> -  );
> -BuildMemoryAllocationHob (
> -  (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> -  (UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
> -  mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> -  );
> +if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> +  //
> +  // Reserve the lock box storage area
> +  //
> +  // Since this memory range will be used on S3 resume, it must be
> +  // reserved as ACPI NVS.
> +  //
> +  // If S3 is unsupported, then various drivers might still write to the
> +  // LockBox area. We ought to prevent DXE from serving allocation 
> requests
> +  // such that they would overlap the LockBox storage.
> +  //
> +  ZeroMem (
> +(VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> +(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
> +);
> +  BuildMemoryAllocationHob (
> +(EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
> +(UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
> +mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
> +);
> +}
>  
>  if (FeaturePcdGet (PcdSmmSmramRequire)) {
>UINT32 TsegSize;
> 

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

Re: [edk2] [PATCH v4 12/41] OvmfPkg: AcpiS3SaveDxe: don't fake LockBox protocol if SMM_REQUIRE

[Paolo Bonzini]

On 03/11/2015 22:00, Laszlo Ersek wrote:
> In SVN r15306 (git commit d4ba06df), "OvmfPkg: S3 Resume: fake LockBox
> protocol for BootScriptExecutorDxe", we installed a fake LockBox protocol
> in OVMF's AcpiS3SaveDxe clone. While our other AcpiS3SaveDxe
> customizations remain valid (or harmless), said change is invalid when
> OVMF is built with -D SMM_REQUIRE and includes the real protocol provider,
> "MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf".
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf |  3 ++-
>  OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c  | 14 --
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf 
> b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> index 4cc0713..a288b95 100644
> --- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> +++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3SaveDxe.inf
> @@ -59,7 +59,7 @@ [Guids]
>gEfiEndOfDxeEventGroupGuid## CONSUMES  ## Event
>  
>  [Protocols]
> -  gEfiLockBoxProtocolGuid   # PROTOCOL ALWAYS_PRODUCED
> +  gEfiLockBoxProtocolGuid   # PROTOCOL SOMETIMES_PRODUCED
>gEfiLegacyBiosProtocolGuid# PROTOCOL ALWAYS_CONSUMED
>gEfiLegacyRegion2ProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
>gFrameworkEfiMpServiceProtocolGuid# PROTOCOL SOMETIMES_CONSUMED
> @@ -71,6 +71,7 @@ [Pcd]
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize
> ## CONSUMES
>gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3BootScriptStackSize   
> ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> ## CONSUMES
>  
>  [Depex]
>gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
> diff --git a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c 
> b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
> index f20560f..e3ff234 100644
> --- a/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
> +++ b/OvmfPkg/AcpiS3SaveDxe/AcpiS3Save.c
> @@ -538,12 +538,14 @@ InstallEndOfDxeCallback (
>  return EFI_LOAD_ERROR;
>}
>  
> -  Status = gBS->InstallMultipleProtocolInterfaces (
> -  ,
> -  , NULL,
> -  NULL
> -  );
> -  ASSERT_EFI_ERROR (Status);
> +  if (!FeaturePcdGet (PcdSmmSmramRequire)) {
> +Status = gBS->InstallMultipleProtocolInterfaces (
> +,
> +, NULL,
> +NULL
> +);
> +ASSERT_EFI_ERROR (Status);
> +  }
>  
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> 

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

Re: [edk2] [PATCH v4 35/41] OvmfPkg: port CpuS3DataDxe to X64

[Paolo Bonzini]

On 03/11/2015 22:01, Laszlo Ersek wrote:
> From: Paolo Bonzini 
> 
> The descriptor format is different and the assembly source is
> converted to nasm, but otherwise there is no difference.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini 

Missing your SoB.

Paolo

> ---
> 
> Notes:
> v3:
> - new in v3
> 
>  OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf  |   5 +
>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h |  59 +++
>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c| 108 
> 
>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/CpuAsm.nasm   |  58 +++
>  4 files changed, 230 insertions(+)
> 
> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf 
> b/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
> index 2610a63..4bec056 100644
> --- a/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
> @@ -63,6 +63,11 @@ [Sources.Ia32]
>IA32/ArchSpecificDef.h
>IA32/ArchSpecific.c
>  
> +[Sources.X64]
> +  X64/CpuAsm.nasm
> +  X64/ArchSpecificDef.h
> +  X64/ArchSpecific.c
> +
>  [Packages]
>MdePkg/MdePkg.dec
>IntelFrameworkPkg/IntelFrameworkPkg.dec
> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h 
> b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h
> new file mode 100644
> index 000..5ea4cf6
> --- /dev/null
> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h
> @@ -0,0 +1,59 @@
> +/** @file
> +
> +Copyright (C) 2015, Red Hat, Inc.
> +Copyright (c) 2013-2015 Intel Corporation.
> +
> +Redistribution and use in source and binary forms, with or without
> +modification, are permitted provided that the following conditions
> +are met:
> +
> +* Redistributions of source code must retain the above copyright
> +notice, this list of conditions and the following disclaimer.
> +* Redistributions in binary form must reproduce the above copyright
> +notice, this list of conditions and the following disclaimer in
> +the documentation and/or other materials provided with the
> +distribution.
> +* Neither the name of Intel Corporation nor the names of its
> +contributors may be used to endorse or promote products derived
> +from this software without specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +
> +Module Name:
> +
> +  ProcessorDef.h
> +
> +Abstract:
> +
> +  Definition for X64 processor
> +
> +**/
> +
> +#ifndef _PROCESSOR_DEF_H_
> +#define _PROCESSOR_DEF_H_
> +
> +#pragma pack(1)
> +
> +typedef struct {
> +  UINT16  OffsetLow;
> +  UINT16  SegmentSelector;
> +  UINT16  Attributes;
> +  UINT16  OffsetHigh;
> +  UINT32  OffsetUpper;
> +  UINT32  Zero;
> +} INTERRUPT_GATE_DESCRIPTOR;
> +
> +#pragma pack()
> +
> +#endif
> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c 
> b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c
> new file mode 100644
> index 000..544816d
> --- /dev/null
> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c
> @@ -0,0 +1,108 @@
> +/** @file
> +
> +  Memory Operation Functions for IA32 Architecture.
> +
> +  Copyright (C) 2015, Red Hat, Inc.
> +  Copyright (c) 2013-2015 Intel Corporation.
> +
> +  Redistribution and use in source and binary forms, with or without
> +  modification, are permitted provided that the following conditions
> +  are met:
> +
> +  * Redistributions of source code must retain the above copyright
> +  notice, this list of conditions and the following disclaimer.
> +  * Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in
> +  the documentation and/or other materials provided with the
> +  distribution.
> +  * Neither the name of Intel Corporation nor the names of its
> +  contributors may be used to endorse or promote products derived
> +  from this software without specific prior written permission.
> +
> +  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +  A PARTICULAR PURPOSE ARE DISCLAIMED. 

Re: [edk2] [PATCH v2] NetworkPkg:Enable Http Boot over Ipv6 stack

[Gary Ching-Pang Lin]

On Tue, Nov 03, 2015 at 04:13:27PM +0800, Zhang Lubo wrote:
> v2:
> *Rewrite the logic of setting ipv6 gateway in httpboot driver and update some 
> comment.
> 
> Fix a potential bug which can cause assert when setting ipv6 Gateway. If 
> there is no
> router in network environment, it will fail to find out the gateway address 
> which can
> route the message to Http Server Ip . When it check the IP6 route table again 
> 1 seconds
> later, it will assert while the router table structure had been freed last 
> time without
> retrieve new relevant data form ip6 driver.
> 

Hi Lubo,

I applied the patch and it worked as expected except the IP expressed
url. I checked the code and found the function(*) to parse the url couldn't
handle IPv6 and just treat the colon as a separator of the host name and port.
Is this a known issue?

Cheers,

Gary Lin

(*) MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c::NetHttpParseAuthority()

> Cc: Fu Siyuan 
> Cc: Ye Ting 
> CC: Wu Jiaxin 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo 
> ---
>  NetworkPkg/HttpBootDxe/HttpBootClient.c|  234 -
>  NetworkPkg/HttpBootDxe/HttpBootComponentName.c |5 +-
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.c |   12 +-
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.h |   11 +
>  NetworkPkg/HttpBootDxe/HttpBootDhcp6.c |  984 +++
>  NetworkPkg/HttpBootDxe/HttpBootDhcp6.h |  198 
>  NetworkPkg/HttpBootDxe/HttpBootDxe.c   |  903 +++---
>  NetworkPkg/HttpBootDxe/HttpBootDxe.h   |  159 +++-
>  NetworkPkg/HttpBootDxe/HttpBootDxe.inf |9 +
>  NetworkPkg/HttpBootDxe/HttpBootImpl.c  |  109 ++-
>  NetworkPkg/HttpBootDxe/HttpBootImpl.h  |2 +-
>  NetworkPkg/HttpBootDxe/HttpBootSupport.c   |  292 +-
>  NetworkPkg/HttpBootDxe/HttpBootSupport.h   |   70 ++
>  NetworkPkg/HttpDxe/HttpDns.c   |  207 +++-
>  NetworkPkg/HttpDxe/HttpDns.h   |   20 +
>  NetworkPkg/HttpDxe/HttpDriver.c|  806 +++-
>  NetworkPkg/HttpDxe/HttpDriver.h|  143 ++-
>  NetworkPkg/HttpDxe/HttpDxe.inf |5 +
>  NetworkPkg/HttpDxe/HttpImpl.c  |  358 +++
>  NetworkPkg/HttpDxe/HttpImpl.h  |1 -
>  NetworkPkg/HttpDxe/HttpProto.c | 1210 
> 
>  NetworkPkg/HttpDxe/HttpProto.h |  182 +++-
>  22 files changed, 5017 insertions(+), 903 deletions(-)
>  create mode 100644 NetworkPkg/HttpBootDxe/HttpBootDhcp6.c
>  create mode 100644 NetworkPkg/HttpBootDxe/HttpBootDhcp6.h
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 18/41] OvmfPkg: select LocalApicLib instance with x2apic support if SMM_REQUIRE

[Laszlo Ersek]

On 11/04/15 09:48, Paolo Bonzini wrote:
> 
> 
> On 03/11/2015 22:00, Laszlo Ersek wrote:
>> +
>> +!if $(SMM_REQUIRE) == TRUE
>> +  LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>> +!else
>>LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>> +!endif
>> +
> 
> Can we enable BaseXApicX2ApicLib unconditionally?

I think I am technically capable of that :), but should we? We haven't
used BaseXApicX2ApicLib thus far in OVMF, so I can't predict at all if
it could regress stuff or not. If it causes problems with the
SMM_REQUIRE build, so be it, that's new stuff, but I wanted to avoid
such a global change for the traditional build.

I'm not against it, I just don't have experience with BaseXApicX2ApicLib.

Mike, Jeff, Jordan -- what do you think? Why do separate BaseXApicLib
and BaseXApicX2ApicLib instances exist? And why has OvmfPkg always used
the former? If it has only been for simplicity's sake, and
BaseXApicX2ApicLib is otherwise widely used internally at Intel, then I
think Paolo is right, and we should just keep the OVMF DSCs simple.

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

Re: [edk2] [PATCH v4 13/41] OvmfPkg: LockBox: -D SMM_REQUIRE excludes our fake lockbox

[Laszlo Ersek]

On 11/04/15 09:52, Paolo Bonzini wrote:
> 
> 
> On 03/11/2015 22:00, Laszlo Ersek wrote:
>> When the user builds OVMF with -D SMM_REQUIRE, our LockBox implementation
>> must not be used, since it doesn't actually protect data in the LockBox
>> from the runtime guest OS. Add an according assert to
>> LockBoxLibInitialize().
>>
>> Furthermore, since our LockBox must not be selected with -D SMM_REQUIRE,
>> it makes sense to set aside memory for it only if -D SMM_REQUIRE is
>> absent. Modify InitializeRamRegions() accordingly.
>>
>> This patch completes the -D SMM_REQUIRE-related tweaking of the special
>> OVMF memory areas.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf |  3 ++
>>  OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf  |  3 ++
>>  OvmfPkg/Library/LockBoxLib/LockBoxLib.c   |  2 +
>>  OvmfPkg/PlatformPei/MemDetect.c   | 40 ++--
>>  4 files changed, 29 insertions(+), 19 deletions(-)
>>
>> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf 
>> b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>> index 7203d07..81c893e 100644
>> --- a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf
>> @@ -42,3 +42,6 @@ [LibraryClasses]
>>  [Pcd]
>>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>> +
>> +[FeaturePcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf 
>> b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
>> index a4d27a5..08973a4 100644
>> --- a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
>> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
>> @@ -43,3 +43,6 @@ [LibraryClasses]
>>  [Pcd]
>>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase
>>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>> +
>> +[FeaturePcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
>> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c 
>> b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
>> index 89050ac..45481b9 100644
>> --- a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
>> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c
>> @@ -44,6 +44,8 @@ LockBoxLibInitialize (
>>  {
>>UINTN NumEntries;
>>  
>> +  ASSERT (!FeaturePcdGet (PcdSmmSmramRequire));
>> +
>>if (PcdGet32 (PcdOvmfLockBoxStorageSize) < sizeof (LOCK_BOX_GLOBAL)) {
>>  return RETURN_UNSUPPORTED;
>>}
>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c 
>> b/OvmfPkg/PlatformPei/MemDetect.c
>> index 1bdc2df..455fcbb 100644
>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>> @@ -407,25 +407,27 @@ InitializeRamRegions (
>>}
>>  
>>if (mBootMode != BOOT_ON_S3_RESUME) {
>> -//
>> -// Reserve the lock box storage area
>> -//
>> -// Since this memory range will be used on S3 resume, it must be
>> -// reserved as ACPI NVS.
>> -//
>> -// If S3 is unsupported, then various drivers might still write to the
>> -// LockBox area. We ought to prevent DXE from serving allocation 
>> requests
>> -// such that they would overlap the LockBox storage.
>> -//
>> -ZeroMem (
>> -  (VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
>> -  (UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
>> -  );
>> -BuildMemoryAllocationHob (
>> -  (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
>> -  (UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
>> -  mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
>> -  );
>> +if (!FeaturePcdGet (PcdSmmSmramRequire)) {
>> +  //
>> +  // Reserve the lock box storage area
>> +  //
>> +  // Since this memory range will be used on S3 resume, it must be
>> +  // reserved as ACPI NVS.
>> +  //
>> +  // If S3 is unsupported, then various drivers might still write to the
>> +  // LockBox area. We ought to prevent DXE from serving allocation 
>> requests
>> +  // such that they would overlap the LockBox storage.
>> +  //
>> +  ZeroMem (
>> +(VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
>> +(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize)
>> +);
>> +  BuildMemoryAllocationHob (
>> +(EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase),
>> +(UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize),
>> +mS3Supported ? EfiACPIMemoryNVS : EfiBootServicesData
>> +);
>> +}
>>  
>>  if (FeaturePcdGet (PcdSmmSmramRequire)) {
>>UINT32 TsegSize;
>>
> 
> Reviewed-by: Paolo Bonzini 
> 

Thanks, picked this up (and the previous one).
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 35/41] OvmfPkg: port CpuS3DataDxe to X64

[Laszlo Ersek]

On 11/04/15 09:54, Paolo Bonzini wrote:
> 
> 
> On 03/11/2015 22:01, Laszlo Ersek wrote:
>> From: Paolo Bonzini 
>>
>> The descriptor format is different and the assembly source is
>> converted to nasm, but otherwise there is no difference.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Paolo Bonzini 
> 
> Missing your SoB.

I tend not to add my SoB if I forward a patch authored by someone else
verbatim. Ard has already told me this doesn't conform to the practice
followed in Linux (I agree, see Documentation/SubmittingPatches,
11/(c)). Maybe I should change my ways and just append my SoB always...

I don't pass --signoff to git-format-patch indiscriminately any more;
IIRC it has caused problems (duplicate SoBs) in the past.

Instead, I have

[commit]
signoff = true

globally, and for edk2 I also have

[commit]
template = .../tianocore.template

which contains

--


Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
--

I guess in this case I should have applied your patch with "git am
--signoff". I think I'll never be able to remember that. I'll try to see
if I can add

[am]
signoff = true

Hm, actually, now I remember why I should have perhaps added my SoB -- I
did fix up the shift count typo in SetIdtEntry(). I believe I thought
that it didn't deserve a bracketed mention and/or an SoB.

In any case, I'm adding both now. Thanks!
Laszlo

> 
> Paolo
> 
>> ---
>>
>> Notes:
>> v3:
>> - new in v3
>>
>>  OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf  |   5 +
>>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h |  59 +++
>>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c| 108 
>> 
>>  OvmfPkg/QuarkPort/CpuS3DataDxe/X64/CpuAsm.nasm   |  58 +++
>>  4 files changed, 230 insertions(+)
>>
>> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf 
>> b/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
>> index 2610a63..4bec056 100644
>> --- a/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
>> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/CpuS3DataDxe.inf
>> @@ -63,6 +63,11 @@ [Sources.Ia32]
>>IA32/ArchSpecificDef.h
>>IA32/ArchSpecific.c
>>  
>> +[Sources.X64]
>> +  X64/CpuAsm.nasm
>> +  X64/ArchSpecificDef.h
>> +  X64/ArchSpecific.c
>> +
>>  [Packages]
>>MdePkg/MdePkg.dec
>>IntelFrameworkPkg/IntelFrameworkPkg.dec
>> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h 
>> b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h
>> new file mode 100644
>> index 000..5ea4cf6
>> --- /dev/null
>> +++ b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecificDef.h
>> @@ -0,0 +1,59 @@
>> +/** @file
>> +
>> +Copyright (C) 2015, Red Hat, Inc.
>> +Copyright (c) 2013-2015 Intel Corporation.
>> +
>> +Redistribution and use in source and binary forms, with or without
>> +modification, are permitted provided that the following conditions
>> +are met:
>> +
>> +* Redistributions of source code must retain the above copyright
>> +notice, this list of conditions and the following disclaimer.
>> +* Redistributions in binary form must reproduce the above copyright
>> +notice, this list of conditions and the following disclaimer in
>> +the documentation and/or other materials provided with the
>> +distribution.
>> +* Neither the name of Intel Corporation nor the names of its
>> +contributors may be used to endorse or promote products derived
>> +from this software without specific prior written permission.
>> +
>> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> +
>> +
>> +Module Name:
>> +
>> +  ProcessorDef.h
>> +
>> +Abstract:
>> +
>> +  Definition for X64 processor
>> +
>> +**/
>> +
>> +#ifndef _PROCESSOR_DEF_H_
>> +#define _PROCESSOR_DEF_H_
>> +
>> +#pragma pack(1)
>> +
>> +typedef struct {
>> +  UINT16  OffsetLow;
>> +  UINT16  SegmentSelector;
>> +  UINT16  Attributes;
>> +  UINT16  OffsetHigh;
>> +  UINT32  OffsetUpper;
>> +  UINT32  Zero;
>> +} INTERRUPT_GATE_DESCRIPTOR;
>> +
>> +#pragma pack()
>> +
>> +#endif
>> diff --git a/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c 
>> b/OvmfPkg/QuarkPort/CpuS3DataDxe/X64/ArchSpecific.c
>> new file mode 100644