Date   

Re: [PATCH v4 1/1] Silicon/Qemu/Sbsa: Add SBSA-wdt entry to GTDT

Graeme Gregory <graeme@...>
 

On Mon, Nov 02, 2020 at 11:51:48AM -0500, Shashi Mallela wrote:
SBSA generic watchdog timer structure entry has been added
to GTDT table as per SBSAv6.0.
This enables acpi detection of wdt in qemu sbsa platform
Reviewed-by: Graeme Gregory <graeme@nuviainc.com>

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Graeme Gregory <graeme@nuviainc.com>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc | 57 ++++++++++++++++++----
1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
index d16778e01a5c..a010b908c434 100644
--- a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
+++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
@@ -24,27 +24,55 @@
#define SYSTEM_TIMER_BASE_ADDRESS 0xFFFFFFFFFFFFFFFF
#endif

-#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
#define GTDT_TIMER_LEVEL_TRIGGERED 0
-#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
#define GTDT_TIMER_ACTIVE_HIGH 0

#define GTDT_GTIMER_FLAGS (GTDT_TIMER_ACTIVE_LOW | GTDT_TIMER_LEVEL_TRIGGERED)

+#define SBSA_PLATFORM_WATCHDOG_COUNT 1
+#define SBSA_PLATFORM_TIMER_COUNT (SBSA_PLATFORM_WATCHDOG_COUNT)
+
+#define SBSAQEMU_WDT_REFRESH_FRAME_BASE 0x50010000
+#define SBSAQEMU_WDT_CONTROL_FRAME_BASE 0x50011000
+#define SBSAQEMU_WDT_IRQ 44
+
+#define GTDT_WDTIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_WDTIMER_LEVEL_TRIGGERED 0
+#define GTDT_WDTIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_WDTIMER_ACTIVE_HIGH 0
+
+#define GTDT_WDTIMER_FLAGS (GTDT_WDTIMER_ACTIVE_HIGH | GTDT_WDTIMER_LEVEL_TRIGGERED)
+
+#define EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT( \
+ RefreshFramePhysicalAddress, ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, WatchdogTimerFlags) \
+ { \
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG, \
+ sizeof(EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE), \
+ EFI_ACPI_RESERVED_WORD, \
+ RefreshFramePhysicalAddress, \
+ ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, \
+ WatchdogTimerFlags \
+ }
+
#pragma pack (1)

typedef struct {
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
- } GENERIC_TIMER_DESCRIPTION_TABLE;
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE Gwdt;
+ } GENERIC_TIMER_DESCRIPTION_TABLES;

#pragma pack ()

- GENERIC_TIMER_DESCRIPTION_TABLE Gtdt = {
+ GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
{
SBSAQEMU_ACPI_HEADER(
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
- GENERIC_TIMER_DESCRIPTION_TABLE,
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
+ GENERIC_TIMER_DESCRIPTION_TABLES,
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
),
SYSTEM_TIMER_BASE_ADDRESS, // UINT64 PhysicalAddress
0, // UINT32 Reserved
@@ -57,9 +85,18 @@
FixedPcdGet32 (PcdArmArchTimerHypIntrNum), // UINT32 NonSecurePL2TimerGSIV
GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL2TimerFlags
0xFFFFFFFFFFFFFFFF, // UINT64 CntReadBasePhysicalAddress
- 0, // UINT32 PlatformTimerCount
- 0
+ SBSA_PLATFORM_TIMER_COUNT, // UINT32 PlatformTimerCount
+ sizeof(EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE),
+ // UINT32 PlatformTimerOffset
+ 0, // UINT32 VirtualPL2TimerGSIV
+ 0 // UINT32 VirtualPL2TimerFlags
},
+ EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
+ SBSAQEMU_WDT_REFRESH_FRAME_BASE,
+ SBSAQEMU_WDT_CONTROL_FRAME_BASE,
+ SBSAQEMU_WDT_IRQ,
+ GTDT_WDTIMER_FLAGS
+ )
};

// Reference the table being generated to prevent the optimizer from removing the
--
2.18.4


Re: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3

Bret Barkelew
 

I second the request for explanation.

 

- Bret

 

From: Laszlo Ersek via groups.io
Sent: Monday, November 2, 2020 10:05 AM
To: Sheng Wei; devel@edk2.groups.io
Cc: Dong, Eric; Ni, Ray; Rahul Kumar; Yao, Jiewen
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3

 

On 11/02/20 05:53, Sheng Wei wrote:
> When the functions called from entrypoint the page table is
> set to mInternalCr3, mInternalIs5LevelPaging reflects
> the page table type pointed by mInternalCr3.
>
> REF: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3015&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=J5khqRr2cyZ%2FFpOXfX4V3juer6n4wvDxTRiQos%2BGJww%3D&amp;reserved=0
>
> Change-Id: I9e44c6385a05930850f5ba60d10ed3e391b628bb

(1) None of the patches should contain such a "Change-Id" line; they are
meaningless for upstream edk2.

They can be removed by the maintainer just before merging, of course.


(2) However, I still have absolutely no idea what the problem is.

Ray provided some great feedback here:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F66617&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=1jszRyYlHbQY5Kl%2F1uDuILrUFyIqFqbGbGmR2KDx5%2BE%3D&amp;reserved=0

In particular:

    Better to explain the case when the functions called from entrypoint
    the page table is set to mInternalCr3, gSmmFeatureEnable5LevelPaging
    reflects the page table type pointed by mInternalCr3.

And this has not been *explained*.

The commit message should include a reminder *why* we sometimes use
"mInternalCr3" (i.e., when it is nonzero), and why we use the CR3
register in other cases. This reminder is not related to the change in
this patch, it should re-state the *original* use case for "mInternalCr3".

And then in the next paragraph, the commit message should explain that,
*IF* we use "mInternalCr3", rather than CR3, *then* accessing CR4 for
5-level paging is wrong -- because in that case, we should save the
5-level paging status similarly (I guess?) to how we save "mInternalCr3".

So, on the surface, the change seems to make sense, but without knowing
why we use "mInternalCr3" in the first place, I find it difficult to
reason about this change.

...

- According to git-blame, "mInternalCr3" comes from edk2 commit
3eb69b081c68 ("UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86
SMM.", 2019-02-28), and it is related to
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1521&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=0SJTtV%2BOacJuIIrwErqO1z3HVQY3MakyWsyGRwSPOMc%3D&amp;reserved=0>.

- Also according to git-blame, the original "Enable5LevelPaging"
assignment, based on "Cr4.Bits.LA57", comes from commit 4eee0cc7cc0d
("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports",
2019-07-12), related to
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D1946&amp;data=04%7C01%7CBret.Barkelew%40microsoft.com%7C633ad8bea21243a5479c08d87f59df4e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637399371257436398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=gbfshsEySndGIWKsCkbs%2F0%2BV2tA4bPflmH0Y3xcsEVI%3D&amp;reserved=0>.

Commit 3eb69b081c68 (the "CET ShadowStack" feature) *precedes* commit
4eee0cc7cc0d (the 5-level paging feature) in the git commit history.

Therefore the bug was introduced commit 4eee0cc7cc0d -- the 5-level
paging enablement did not take CET ShadowStack support in consideration.

In other words, this patch fixes (or "completes") 5-level paging support
for "CET ShadowStack".

Is that correct?

If so, then *WHY* is none of the expressions "CET ShadowStack" and
"5-level paging" present in any of the subject lines? Why are there no
references to commits 3eb69b081c68 and 4eee0cc7cc0d in the commit
messages? Why does Bug 3015 not reference Bug 1521 and Bug 1946?

After all, if I understand correctly, this patch covers a corner case in
the intersection of two features. Why is that not documented clearly?

I want to see a statement such as "if we are using a shadow stack, then
we need to use a CR4 value that matches the shadow stack, for
determining whether 5-level paging is enabled or not".

Or *something* like this.


> Signed-off-by: Sheng Wei <w.sheng@...>
> Cc: Eric Dong <eric.dong@...>
> Cc: Ray Ni <ray.ni@...>
> Cc: Laszlo Ersek <lersek@...>
> Cc: Rahul Kumar <rahul1.kumar@...>
> Cc: Jiewen Yao <jiewen.yao@...>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         | 10 ++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 42 ++++++++++++++++++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            |  2 ++
>  3 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> index 7fb3a2d9e4..3eb6af62a7 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> @@ -951,6 +951,16 @@ GetPageTableBase (
>    VOID
>    );

> +/**
> +  This function set the internal page table type to 5 level paging or 4 level paging.
> +
> +  @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level paging.
> +**/
> +VOID
> +SetPageTableType (
> +  IN BOOLEAN  Is5LevelPaging
> +  );
> +
>  /**
>    This function sets the attributes for the memory region specified by BaseAddress and
>    Length from their current attributes to the attributes specified by Attributes.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index d67f036aea..91c0fd6587 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>  };

>  UINTN  mInternalCr3;
> +BOOLEAN mInternalIs5LevelPaging = FALSE;

>  /**
>    Set the internal page table base address.
> @@ -65,6 +66,43 @@ GetPageTableBase (
>    return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
>  }

> +/**
> +  This function set the internal page table type to 5 level paging or 4 level paging.
> +
> +  @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level paging.
> +**/
> +VOID
> +SetPageTableType (
> +  IN BOOLEAN  Is5LevelPaging
> +  )
> +{
> +  mInternalIs5LevelPaging = Is5LevelPaging;
> +}
> +
> +/**
> +  Return if the page table is 5 level paging.
> +
> +  @return TRUE  The page table base is 5 level paging.
> +  @return FALSE The page table base is 4 level paging.
> +**/
> +STATIC
> +BOOLEAN
> +Is5LevelPageTableBase (
> +  VOID
> +  )
> +{
> +  IA32_CR4              Cr4;
> +
> +  // If mInternalCr3 is non zero, it will not use the page table from CR3.
> +  // So, return the page level type from mInternalIs5LevelPaging instead of the CR4 LA57 bit.

(3) Invalid comment style. Missing empty "//" lines before and after.

> +  if (mInternalCr3 != 0) {
> +    return mInternalIs5LevelPaging;
> +  }
> +
> +  Cr4.UintN = AsmReadCr4 ();
> +  return (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +}
> +
>  /**
>    Return length according to page attributes.

> @@ -131,7 +169,6 @@ GetPageTableEntry (
>    UINT64                *L3PageTable;
>    UINT64                *L4PageTable;
>    UINT64                *L5PageTable;
> -  IA32_CR4              Cr4;
>    BOOLEAN               Enable5LevelPaging;

>    Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
> @@ -140,8 +177,7 @@ GetPageTableEntry (
>    Index2 = ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK;
>    Index1 = ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK;

> -  Cr4.UintN = AsmReadCr4 ();
> -  Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
> +  Enable5LevelPaging = Is5LevelPageTableBase();

(4) Missing space character before the opening parenthesis.


>    if (sizeof(UINTN) == sizeof(UINT64)) {
>      if (Enable5LevelPaging) {
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index 810985df20..6f2f4adb7d 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -387,6 +387,8 @@ SmmInitPageTable (
>    SetSubEntriesNum (Pml4Entry, 3);
>    PTEntry = Pml4Entry;

> +  SetPageTableType(m5LevelPagingNeeded);

(5) Missing space character before the opening parenthesis.

> +
>    if (m5LevelPagingNeeded) {
>      //
>      // Fill PML5 entry
>

Laszlo





 


Re: [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Correct the Cr3 typo

Laszlo Ersek
 

On 11/02/20 05:53, Sheng Wei wrote:
Change the variable name from mInternalGr3 to mInternalCr3.

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

Change-Id: I6a9df4836d4358405837b1ebbd2d5d4c85e3635f
With the "Change-Id" line removed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Signed-off-by: Sheng Wei <w.sheng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index ebfc46ad45..d67f036aea 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -32,7 +32,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
{Page1G, SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
};

-UINTN mInternalGr3;
+UINTN mInternalCr3;

/**
Set the internal page table base address.
@@ -46,7 +46,7 @@ SetPageTableBase (
IN UINTN Cr3
)
{
- mInternalGr3 = Cr3;
+ mInternalCr3 = Cr3;
}

/**
@@ -59,8 +59,8 @@ GetPageTableBase (
VOID
)
{
- if (mInternalGr3 != 0) {
- return mInternalGr3;
+ if (mInternalCr3 != 0) {
+ return mInternalCr3;
}
return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
}
@@ -252,7 +252,7 @@ ConvertPageEntryAttribute (
if ((Attributes & EFI_MEMORY_RO) != 0) {
if (IsSet) {
NewPageEntry &= ~(UINT64)IA32_PG_RW;
- if (mInternalGr3 != 0) {
+ if (mInternalCr3 != 0) {
// Environment setup
// ReadOnly page need set Dirty bit for shadow stack
NewPageEntry |= IA32_PG_D;


Re: [PATCH v3 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Return level paging type for Internal CR3

Laszlo Ersek
 

On 11/02/20 05:53, Sheng Wei wrote:
When the functions called from entrypoint the page table is
set to mInternalCr3, mInternalIs5LevelPaging reflects
the page table type pointed by mInternalCr3.

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

Change-Id: I9e44c6385a05930850f5ba60d10ed3e391b628bb
(1) None of the patches should contain such a "Change-Id" line; they are
meaningless for upstream edk2.

They can be removed by the maintainer just before merging, of course.


(2) However, I still have absolutely no idea what the problem is.

Ray provided some great feedback here:

https://edk2.groups.io/g/devel/message/66617

In particular:

Better to explain the case when the functions called from entrypoint
the page table is set to mInternalCr3, gSmmFeatureEnable5LevelPaging
reflects the page table type pointed by mInternalCr3.

And this has not been *explained*.

The commit message should include a reminder *why* we sometimes use
"mInternalCr3" (i.e., when it is nonzero), and why we use the CR3
register in other cases. This reminder is not related to the change in
this patch, it should re-state the *original* use case for "mInternalCr3".

And then in the next paragraph, the commit message should explain that,
*IF* we use "mInternalCr3", rather than CR3, *then* accessing CR4 for
5-level paging is wrong -- because in that case, we should save the
5-level paging status similarly (I guess?) to how we save "mInternalCr3".

So, on the surface, the change seems to make sense, but without knowing
why we use "mInternalCr3" in the first place, I find it difficult to
reason about this change.

...

- According to git-blame, "mInternalCr3" comes from edk2 commit
3eb69b081c68 ("UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86
SMM.", 2019-02-28), and it is related to
<https://bugzilla.tianocore.org/show_bug.cgi?id=1521>.

- Also according to git-blame, the original "Enable5LevelPaging"
assignment, based on "Cr4.Bits.LA57", comes from commit 4eee0cc7cc0d
("UefiCpuPkg/PiSmmCpu: Enable 5 level paging when CPU supports",
2019-07-12), related to
<https://bugzilla.tianocore.org/show_bug.cgi?id=1946>.

Commit 3eb69b081c68 (the "CET ShadowStack" feature) *precedes* commit
4eee0cc7cc0d (the 5-level paging feature) in the git commit history.

Therefore the bug was introduced commit 4eee0cc7cc0d -- the 5-level
paging enablement did not take CET ShadowStack support in consideration.

In other words, this patch fixes (or "completes") 5-level paging support
for "CET ShadowStack".

Is that correct?

If so, then *WHY* is none of the expressions "CET ShadowStack" and
"5-level paging" present in any of the subject lines? Why are there no
references to commits 3eb69b081c68 and 4eee0cc7cc0d in the commit
messages? Why does Bug 3015 not reference Bug 1521 and Bug 1946?

After all, if I understand correctly, this patch covers a corner case in
the intersection of two features. Why is that not documented clearly?

I want to see a statement such as "if we are using a shadow stack, then
we need to use a CR4 value that matches the shadow stack, for
determining whether 5-level paging is enabled or not".

Or *something* like this.


Signed-off-by: Sheng Wei <w.sheng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 ++++++
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 42 ++++++++++++++++++++--
UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 2 ++
3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 7fb3a2d9e4..3eb6af62a7 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -951,6 +951,16 @@ GetPageTableBase (
VOID
);

+/**
+ This function set the internal page table type to 5 level paging or 4 level paging.
+
+ @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level paging.
+**/
+VOID
+SetPageTableType (
+ IN BOOLEAN Is5LevelPaging
+ );
+
/**
This function sets the attributes for the memory region specified by BaseAddress and
Length from their current attributes to the attributes specified by Attributes.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index d67f036aea..91c0fd6587 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -33,6 +33,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
};

UINTN mInternalCr3;
+BOOLEAN mInternalIs5LevelPaging = FALSE;

/**
Set the internal page table base address.
@@ -65,6 +66,43 @@ GetPageTableBase (
return (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
}

+/**
+ This function set the internal page table type to 5 level paging or 4 level paging.
+
+ @param Is5LevelPaging TRUE means 5 level paging. FALSE means 4 level paging.
+**/
+VOID
+SetPageTableType (
+ IN BOOLEAN Is5LevelPaging
+ )
+{
+ mInternalIs5LevelPaging = Is5LevelPaging;
+}
+
+/**
+ Return if the page table is 5 level paging.
+
+ @return TRUE The page table base is 5 level paging.
+ @return FALSE The page table base is 4 level paging.
+**/
+STATIC
+BOOLEAN
+Is5LevelPageTableBase (
+ VOID
+ )
+{
+ IA32_CR4 Cr4;
+
+ // If mInternalCr3 is non zero, it will not use the page table from CR3.
+ // So, return the page level type from mInternalIs5LevelPaging instead of the CR4 LA57 bit.
(3) Invalid comment style. Missing empty "//" lines before and after.

+ if (mInternalCr3 != 0) {
+ return mInternalIs5LevelPaging;
+ }
+
+ Cr4.UintN = AsmReadCr4 ();
+ return (BOOLEAN) (Cr4.Bits.LA57 == 1);
+}
+
/**
Return length according to page attributes.

@@ -131,7 +169,6 @@ GetPageTableEntry (
UINT64 *L3PageTable;
UINT64 *L4PageTable;
UINT64 *L5PageTable;
- IA32_CR4 Cr4;
BOOLEAN Enable5LevelPaging;

Index5 = ((UINTN)RShiftU64 (Address, 48)) & PAGING_PAE_INDEX_MASK;
@@ -140,8 +177,7 @@ GetPageTableEntry (
Index2 = ((UINTN)Address >> 21) & PAGING_PAE_INDEX_MASK;
Index1 = ((UINTN)Address >> 12) & PAGING_PAE_INDEX_MASK;

- Cr4.UintN = AsmReadCr4 ();
- Enable5LevelPaging = (BOOLEAN) (Cr4.Bits.LA57 == 1);
+ Enable5LevelPaging = Is5LevelPageTableBase();
(4) Missing space character before the opening parenthesis.


if (sizeof(UINTN) == sizeof(UINT64)) {
if (Enable5LevelPaging) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
index 810985df20..6f2f4adb7d 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
@@ -387,6 +387,8 @@ SmmInitPageTable (
SetSubEntriesNum (Pml4Entry, 3);
PTEntry = Pml4Entry;

+ SetPageTableType(m5LevelPagingNeeded);
(5) Missing space character before the opening parenthesis.

+
if (m5LevelPagingNeeded) {
//
// Fill PML5 entry
Laszlo


Re: 回复: [edk2-devel] [PATCH v3 1/1] MdeModulePkg/Gcd: Check memory allocation when initializing memory

Laszlo Ersek
 

On 10/30/20 01:59, gaoliming wrote:
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

If no other comment, I will merge this patch early of next week.
I'd like to regression-test this with OVMF and ArmVirtQemu, but I need a
bit more time -- I just returned to work, and my email backlog is crushing.

Thanks for your patience.

Laszlo


Re: 回复: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: use pool allocations for ACPI tables

Laszlo Ersek
 

On 10/30/20 15:51, Ard Biesheuvel wrote:
On 10/30/20 2:01 AM, gaoliming wrote:
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Merged #1062 into master.
Sorry for not reacting to this v2 series, I've been away. (I hope that
my automated out-of-office response worked and let you know.) I'm glad
that the patches are now merged.

Thanks
Laszlo


[PATCH v4 1/1] Silicon/Qemu/Sbsa: Add SBSA-wdt entry to GTDT

Shashi Mallela
 

SBSA generic watchdog timer structure entry has been added
to GTDT table as per SBSAv6.0.
This enables acpi detection of wdt in qemu sbsa platform

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Graeme Gregory <graeme@nuviainc.com>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc | 57 ++++++++++++++++++----
1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
index d16778e01a5c..a010b908c434 100644
--- a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
+++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
@@ -24,27 +24,55 @@
#define SYSTEM_TIMER_BASE_ADDRESS 0xFFFFFFFFFFFFFFFF
#endif

-#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
#define GTDT_TIMER_LEVEL_TRIGGERED 0
-#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
#define GTDT_TIMER_ACTIVE_HIGH 0

#define GTDT_GTIMER_FLAGS (GTDT_TIMER_ACTIVE_LOW | GTDT_TIMER_LEVEL_TRIGGERED)

+#define SBSA_PLATFORM_WATCHDOG_COUNT 1
+#define SBSA_PLATFORM_TIMER_COUNT (SBSA_PLATFORM_WATCHDOG_COUNT)
+
+#define SBSAQEMU_WDT_REFRESH_FRAME_BASE 0x50010000
+#define SBSAQEMU_WDT_CONTROL_FRAME_BASE 0x50011000
+#define SBSAQEMU_WDT_IRQ 44
+
+#define GTDT_WDTIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_WDTIMER_LEVEL_TRIGGERED 0
+#define GTDT_WDTIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_WDTIMER_ACTIVE_HIGH 0
+
+#define GTDT_WDTIMER_FLAGS (GTDT_WDTIMER_ACTIVE_HIGH | GTDT_WDTIMER_LEVEL_TRIGGERED)
+
+#define EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT( \
+ RefreshFramePhysicalAddress, ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, WatchdogTimerFlags) \
+ { \
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG, \
+ sizeof(EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE), \
+ EFI_ACPI_RESERVED_WORD, \
+ RefreshFramePhysicalAddress, \
+ ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, \
+ WatchdogTimerFlags \
+ }
+
#pragma pack (1)

typedef struct {
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
- } GENERIC_TIMER_DESCRIPTION_TABLE;
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE Gwdt;
+ } GENERIC_TIMER_DESCRIPTION_TABLES;

#pragma pack ()

- GENERIC_TIMER_DESCRIPTION_TABLE Gtdt = {
+ GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
{
SBSAQEMU_ACPI_HEADER(
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
- GENERIC_TIMER_DESCRIPTION_TABLE,
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
+ GENERIC_TIMER_DESCRIPTION_TABLES,
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
),
SYSTEM_TIMER_BASE_ADDRESS, // UINT64 PhysicalAddress
0, // UINT32 Reserved
@@ -57,9 +85,18 @@
FixedPcdGet32 (PcdArmArchTimerHypIntrNum), // UINT32 NonSecurePL2TimerGSIV
GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL2TimerFlags
0xFFFFFFFFFFFFFFFF, // UINT64 CntReadBasePhysicalAddress
- 0, // UINT32 PlatformTimerCount
- 0
+ SBSA_PLATFORM_TIMER_COUNT, // UINT32 PlatformTimerCount
+ sizeof(EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE),
+ // UINT32 PlatformTimerOffset
+ 0, // UINT32 VirtualPL2TimerGSIV
+ 0 // UINT32 VirtualPL2TimerFlags
},
+ EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
+ SBSAQEMU_WDT_REFRESH_FRAME_BASE,
+ SBSAQEMU_WDT_CONTROL_FRAME_BASE,
+ SBSAQEMU_WDT_IRQ,
+ GTDT_WDTIMER_FLAGS
+ )
};

// Reference the table being generated to prevent the optimizer from removing the
--
2.18.4


[PATCH v4 0/1] Add SBSA-wdt entry to GTDT

Shashi Mallela
 

To enable detection of qemu SBSA generic watchdog timer device,this
patch has been added to create sbsa-wdt entry into the GTDT table
which helps firmware report the presence of SBSA-wdt to the OS.

Changes in v4:
- fixed indentation

Shashi Mallela (1):
Silicon/Qemu/Sbsa: Add SBSA-wdt entry to GTDT

Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc | 57 ++++++++++++++++++----
1 file changed, 47 insertions(+), 10 deletions(-)

--
2.18.4


Re: [PATCH v3 00/11] SEV-ES guest support fixes and cleanup

Laszlo Ersek
 

On 10/29/20 15:17, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@amd.com>

This patch series provides some fixes, updates and cleanup to the SEV-ES
guest support:

- Update the calculation of the qword offset of fields within the GHCB
by removing the hardcoding of the offsets and using the OFFSET_OF ()
and sizeof () functions to calculate the values. Remove unused values
and add values that will be used in later patches.

- Set the SwExitCode, SwExitInfo1, SwExitInfo2 and SwScratch valid bits
in the GHCB ValidBitmap area when these fields are for a VMGEXIT. This
is done by adding two new interfaces to the VmgExitLib library to set
and test the bits of the GHCB ValidBitmap. This reduces code duplication
and keeps access to the ValidBitmap field within the VmgExitLib library.

- Update the Qemu flash drive services support to add SEV-ES support for
erasing blocks.

- Disable interrupts when using the GHCB.

- Use the processor number for setting the AP stack pointer instead of the
APIC ID by calling GetProcessorNumber().

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

---

These patches are based on commit:
6ad819c1abe3 ("FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API")

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>

Changes since v2:
- Don't rename the GHCB_REGISTER enum type.
I've got this queued for review. I'll need some time for getting to it,
as I've just returned after some absence, and everything seems to have
collapsed on my head (as usual).

Laszlo


Changes since v1:
- For the GHCB savearea changes, create a new reserved area name instead
of "renumbering" the reserved areas.
- Rework the ValidBitmap set/test support to be part of the VmgExitLib
library. Create two new interfaces for setting and testing bits in the
GHCB ValidBitmap field and adjust all existing code and the new code in
this series to use these interfaces for the ValidBitmap updates/checks.
- Don't disable interrupts for just the Qemu flash services support, but
rather, cover all users of the GHCB by disabling interrupts in VmgInit()
and restoring them in VmgDone(). This requires changes to those
interaces.

Tom Lendacky (11):
MdePkg: Clean up GHCB field offsets and save area
UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap
bits
OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces
OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events
OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events
UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit
OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES
UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB
UefiCpuPkg/MpInitLib: For SEV-ES guest, set stack based on processor
number

MdePkg/Include/Register/Amd/Ghcb.h | 40 +++---
UefiCpuPkg/Include/Library/VmgExitLib.h | 51 +++++++-
OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 84 ++++++++++++-
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 129 ++++++--------------
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 4 +-
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 6 +-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 5 +-
UefiCpuPkg/Library/MpInitLib/MpLib.c | 14 ++-
UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c | 60 +++++++--
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 6 +
10 files changed, 258 insertions(+), 141 deletions(-)


[PATCH] SecurityPkg/SecurityPkg.dec: add PCD for status of variable integrity

Wang, Jian J
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2594

This patch adds PCD PcdStatusCodeVariableIntegrity used to report the
result of variable integrity check to platform, which should choose
appropriate methods to handle the situation of the compromised variable
or other error conditions.

This patch is part of bz2594 and supposed to be check in the tree in
advance in order to coordinate the development works for bz2594 between
edk2 and platform.

Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Nishant C Mistry <nishant.c.mistry@intel.com>
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
SecurityPkg/SecurityPkg.dec | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/SecurityPkg/SecurityPkg.dec b/SecurityPkg/SecurityPkg.dec
index 1b7d62e802..2e87cb3c31 100644
--- a/SecurityPkg/SecurityPkg.dec
+++ b/SecurityPkg/SecurityPkg.dec
@@ -290,6 +290,11 @@
gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass|0x0303100A=
|UINT32|0x00010030=0D
gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationFail|0x0303100B=
|UINT32|0x00010031=0D
=0D
+ ## Progress Code for variable integrity check result.<BR><BR>=0D
+ # DEFAULT<pass>: (EFI_PERIPHERAL_FIXED_MEDIA | 0)=0D
+ # @Prompt Status Code for variable integiry check result=0D
+ gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeVariableIntegrity|0x01070000|=
UINT32|0x00010032=0D
+=0D
[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]=0D
## Image verification policy for OptionRom. Only following values are va=
lid:<BR><BR>=0D
# NOTE: Do NOT use 0x5 and 0x2 since it violates the UEFI specification=
and has been removed.<BR>=0D
--=20
2.27.0.windows.1


Re: [PATCH V2 1/7] NetworkPkg/Defines: Make iSCSI disable as default

Laszlo Ersek
 

On 10/29/20 03:34, Gao, Zhichao wrote:
Sure. I would do it. I am thinking using Network.dsc.inc instead of others inc's combination. But there may be a question: the default Network.dsc.inc would only cover below build:
Components.IA32, Components.X64, Components.ARM, Components.AARCH64, Components.RISCV64
I am not sure if the above would match ArmVirt and Ovmf's requirements.
Indeed, modifying just "Network.dsc.inc" is insufficient.

"Network.dsc.inc" is convenient when it is applicable, but for some
platforms, it is not flexible enough. That's why we have the separate
DSC include files under NetworkPkg that do not contain the section
headers themselves (such as [LibraryClasses], [Components] etc).

This lets platforms decide *where* they include those snippets.

"Network.dsc.inc" is not used by either ArmVirtPkg or OvmfPkg platforms.
The platform DSC files in those package directories reference
"NetworkDefines.dsc.inc" and "NetworkComponents.dsc.inc" instead.

Thanks,
Laszlo


-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Tuesday, October 27, 2020 6:48 PM
To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
<ard.biesheuvel@arm.com>; Sami Mujawar <sami.mujawar@arm.com>; Leif
Lindholm <leif@nuviainc.com>; Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian
J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>; Jiang, Guomin
<guomin.jiang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
Steele, Kelly <kelly.steele@intel.com>; Sun, Zailiang <zailiang.sun@intel.com>;
Qian, Yi <yi.qian@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Maciej
Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Fu,
Siyuan <siyuan.fu@intel.com>
Subject: Re: [PATCH V2 1/7] NetworkPkg/Defines: Make iSCSI disable as default

Hi Zhichao,

thanks for the CC, I appreciate it. Please see my comments below.

On 10/27/20 03:42, Zhichao Gao wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3003

iSCSI is using the undeprecated function MD5. It is better to make the
default setting secure. If the platforms want to use the iSCSI, they
should enable it in the platforms'
dsc file and be aware they are using an unsafe function.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Kelly Steele <kelly.steele@intel.com>
Cc: Zailiang Sun <zailiang.sun@intel.com>
Cc: Yi Qian <yi.qian@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
NetworkPkg/NetworkDefines.dsc.inc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/NetworkDefines.dsc.inc
b/NetworkPkg/NetworkDefines.dsc.inc
index a442d1b157..18921d81f6 100644
--- a/NetworkPkg/NetworkDefines.dsc.inc
+++ b/NetworkPkg/NetworkDefines.dsc.inc
@@ -17,7 +17,7 @@
# DEFINE NETWORK_TLS_ENABLE = TRUE
# DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE
# DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
-# DEFINE NETWORK_ISCSI_ENABLE = TRUE
+# DEFINE NETWORK_ISCSI_ENABLE = FALSE
# DEFINE NETWORK_VLAN_ENABLE = TRUE
#
# Copyright (c) 2019, Intel Corporation. All rights reserved.<BR> @@
-101,7 +101,7 @@
# Both OpensslLib.inf and OpensslLibCrypto.inf library instance can be used
# since libssl is not required for iSCSI.
#
- DEFINE NETWORK_ISCSI_ENABLE = TRUE
+ DEFINE NETWORK_ISCSI_ENABLE = FALSE
!endif

!if $(NETWORK_ENABLE) == TRUE
I know of people that use iSCSI with the ArmVirtQemu and OVMF platforms.

Please prepend two patches to this series (that is, the v3 series should begin with
these two patches below):

(1) locate "NETWORK_ALLOW_HTTP_CONNECTIONS" in the files:

- ArmVirtPkg/ArmVirtQemu.dsc
- ArmVirtPkg/ArmVirtQemuKernel.dsc

and explicitly enable NETWORK_ISCSI_ENABLE in the same place.

(2) Please do the same for the following files, in a separate patch:

- OvmfPkg/Bhyve/BhyveX64.dsc
- OvmfPkg/OvmfPkgIa32.dsc
- OvmfPkg/OvmfPkgIa32X64.dsc
- OvmfPkg/OvmfPkgX64.dsc
- OvmfPkg/OvmfXen.dsc

Thanks!
Laszlo


Re: [PATCH] BaseTools: Limit command line length.

Laszlo Ersek
 

On 10/28/20 15:57, Feng, Bob C wrote:
Laszlo,

1) I try to provide more information.
This problem happens because of the commit SHA-1: 05217d210e8da37b47d0be58ec363f7af2fa1c18. This commit changes the compilation command from one command compiles one c file to one command compile multiple c files. That change reduces the compilation time for MSVC toolchain but it increases the length of one compilation command line. Since the build tool can autosave the FLAGS and INC to the response file if the command line length larger than --cmd-len, there is no problem for normal build usage.

But for the case in BZ2528, user append additional string to the compilation command, that is adding another tool to launch the compiler.
<path_to_other_tool>/tool.py <path_to_msvc>/cl.exe ...
So the it's easy to exceed the max length of command line and cause build failure.
Understood -- the build tool would not consider the user-originated
prefix. The build tool would decide that the command is safe to start
without using a response file, but the prefix would break that.


This patch is to save the c files in another response file to make the compilation command shorter.
I have two questions regarding this:


(a) wouldn't it be better to calculate the length of the user-originated
prefix (such as the length of "<path_to_other_tool>/tool.py"), and
simply include it in the sum that is then checked against "--cmd-len"?


(b) regarding the present patch, do we use the separate response file
(which stores the C file names) *only* if there are multiple C files
passed to the compiler? (This seems to be confirmed by the commit message.)

I'm asking (b) because, as I understand, the multi-C-file trick is
specific to MSVC in the first place, so this further tweak should *also*
be specific to MSVC. (With GCC, there is never more than 1 C file on a
gcc command line, so the separate response file should never be used.)


2) "--cmd-len" is still functional.
Thanks.

One more comment below, regarding the commit message:



Thanks,
Bob

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Tuesday, October 27, 2020 7:58 PM
To: devel@edk2.groups.io; Liang, MingyueX <mingyuex.liang@intel.com>
Cc: Feng, Bob C <bob.c.feng@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Chen, Christine <yuwei.chen@intel.com>; Ard Biesheuvel (ARM address) <ard.biesheuvel@arm.com>
Subject: Re: [edk2-devel] [PATCH] BaseTools: Limit command line length.

(+Ard)

On 10/23/20 05:00, mliang2x wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2528

Currently, the command line is too long because the CL command is
followed by multiple C files.

Therefore, the number of C files
can be used to determine whether the command line needs to be written
to the file. If the number of C files is greater than one, the command
line is directly written to the file. On the contrary, whether to
write to the file is determined by whether the length of the command
line exceeds the limited length Documents.
I don't understand two expressions here:

(a) "On the contrary".

Is this an attempt to describe the current (= incorrect) behavior?

(b) "limited length Documents".

I have no idea what this means.

Thanks,
Laszlo



Signed-off-by: Mingyue Liang <mingyuex.liang@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/AutoGen/GenMake.py | 45 +++++++++++++++----
.../Source/Python/AutoGen/IncludesAutoGen.py | 13 +++++-
2 files changed, 49 insertions(+), 9 deletions(-)
(1) I've read both BZ#2528 (up to comment 13), and the above commit message too.

I still don't have the slightest idea what the problem is. Please clarify.

(2) How do this patch (and this issue) relate to the "--cmd-len" option?


The bugzilla ticket says the issue is related to MSVC, and that it was exposed by fixing BZ#1672 ("Enable multiple thread for MSVC compiler").
But, at least superficially, the diffstat and the patch body seem to be more generic. What I really care about is that the gcc command lines should not change. It's annoying to look at a build log and see references to "cc_resp" text files, rather than the actual command lines. My build scripts use "--cmd-len=65536" for that reason -- is this patch going to keep that functional?

Thanks,
Laszlo


diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
b/BaseTools/Source/Python/AutoGen/GenMake.py
index 0314d0ea34..0cb97dc18d 100755
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -576,7 +576,8 @@ cleanlib:
EdkLogger.error("build", AUTOGEN_ERROR, "Nothing to build",
ExtraData="[%s]" % str(MyAgo))

- self.ProcessBuildTargetList()
+ self.ProcessBuildTargetList(MyAgo.OutputDir,ToolsDef)
+
self.ParserGenerateFfsCmd()

# Generate macros used to represent input files @@ -866,7
+867,6 @@ cleanlib:
else:
break
SingleCommandLength += len(Str)
-
if SingleCommandLength > GlobalData.gCommandMaxLength:
FlagDict[Tool]['Value'] = True

@@ -890,18 +890,18 @@ cleanlib:
break
else:
break
-
if self._AutoGenObject.ToolChainFamily == 'GCC':
RespDict[Key] = Value.replace('\\', '/')
else:
RespDict[Key] = Value
+
for Target in BuildTargets:
for i, SingleCommand in enumerate(BuildTargets[Target].Commands):
if FlagDict[Flag]['Macro'] in SingleCommand:
BuildTargets[Target].Commands[i] = SingleCommand.replace('$(INC)', '').replace(FlagDict[Flag]['Macro'], RespMacro)
return RespDict

- def ProcessBuildTargetList(self):
+ def ProcessBuildTargetList(self, RespFile, ToolsDef):
#
# Search dependency file list for each source file
#
@@ -1002,6 +1002,7 @@ cleanlib:
self.ObjTargetDict[T.Target.SubDir] = set()
self.ObjTargetDict[T.Target.SubDir].add(NewFile)
for Type in self._AutoGenObject.Targets:
+ resp_file_number = 0
for T in self._AutoGenObject.Targets[Type]:
# Generate related macros if needed
if T.GenFileListMacro and T.FileListMacro not in self.FileListMacros:
@@ -1043,7 +1044,8 @@ cleanlib:
Deps.append("$(%s)" % T.ListFileMacro)

if self._AutoGenObject.BuildRuleFamily == TAB_COMPILER_MSFT and Type == TAB_C_CODE_FILE:
- T, CmdTarget, CmdTargetDict, CmdCppDict = self.ParserCCodeFile(T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict)
+ T, CmdTarget, CmdTargetDict, CmdCppDict = self.ParserCCodeFile(T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict, RespFile, ToolsDef, resp_file_number)
+ resp_file_number += 1
TargetDict = {"target": self.PlaceMacro(T.Target.Path, self.Macros), "cmd": "\n\t".join(T.Commands),"deps": CCodeDeps}
CmdLine = self._BUILD_TARGET_TEMPLATE.Replace(TargetDict).rstrip().replace('\t$(OBJLIST', '$(OBJLIST')
if T.Commands:
@@ -1060,7 +1062,7 @@ cleanlib:
AnnexeTargetDict = {"target": self.PlaceMacro(i.Path, self.Macros), "cmd": "", "deps": self.PlaceMacro(T.Target.Path, self.Macros)}

self.BuildTargetList.append(self._BUILD_TARGET_TEMPLATE.Replace(Annexe
TargetDict))

- def ParserCCodeFile(self, T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict):
+ def ParserCCodeFile(self, T, Type, CmdSumDict, CmdTargetDict, CmdCppDict, DependencyDict, RespFile, ToolsDef, resp_file_number):
if not CmdSumDict:
for item in self._AutoGenObject.Targets[Type]:
CmdSumDict[item.Target.SubDir] = item.Target.BaseName
@@ -1080,6 +1082,7 @@ cleanlib:
CmdCppDict[item.Target.SubDir].append(Path)
if T.Commands:
CommandList = T.Commands[:]
+ SaveFilePath = os.path.join(RespFile, "cc_resp_%s.txt" %
+ resp_file_number)
for Item in CommandList[:]:
SingleCommandList = Item.split()
if len(SingleCommandList) > 0 and self.CheckCCCmd(SingleCommandList):
@@ -1087,19 +1090,45 @@ cleanlib:
if Temp.startswith('/Fo'):
CmdSign = '%s%s' % (Temp.rsplit(TAB_SLASH, 1)[0], TAB_SLASH)
break
- else: continue
+ else:
+ continue
if CmdSign not in list(CmdTargetDict.keys()):
CmdTargetDict[CmdSign] = Item.replace(Temp, CmdSign)
else:
CmdTargetDict[CmdSign] = "%s %s" %
(CmdTargetDict[CmdSign], SingleCommandList[-1])
+
Index = CommandList.index(Item)
CommandList.pop(Index)
if SingleCommandList[-1].endswith("%s%s.c" % (TAB_SLASH, CmdSumDict[CmdSign[3:].rsplit(TAB_SLASH, 1)[0]])):
Cpplist = CmdCppDict[T.Target.SubDir]
Cpplist.insert(0, '$(OBJLIST_%d): ' % list(self.ObjTargetDict.keys()).index(T.Target.SubDir))
- T.Commands[Index] = '%s\n\t%s' % (' \\\n\t'.join(Cpplist), CmdTargetDict[CmdSign])
+ cmdtargetlist = CmdTargetDict[CmdSign].split(" ")
+ # get Source files and Save resp file.
+ c_files = []
+ cmds = []
+ if cmdtargetlist:
+ for item in cmdtargetlist:
+ if item.startswith('$(') or item.startswith('/Fo') or item.startswith('"$('):
+ cmds.append(item)
+ if item.endswith('.c'):
+ c_files.append(item)
+ c_files.insert(0, " ")
+ if len(c_files) > 2:
+ SaveFileOnChange(SaveFilePath," ".join(c_files), False)
+ T.Commands[Index] = '%s\n\t%s $(cc_resp_%s)' % (' \\\n\t'.join(Cpplist), " ".join(cmds), resp_file_number)
+ ToolsDef.append("cc_resp_%s = @%s" %
+ (resp_file_number, SaveFilePath))
+
+ elif len(CmdTargetDict[CmdSign]) > GlobalData.gCommandMaxLength and len(c_files) <=2:
+ SaveFileOnChange(SaveFilePath, " ".join(c_files), False)
+ T.Commands[Index] = '%s\n\t%s $(cc_resp_%s)' % (' \\\n\t'.join(Cpplist), " ".join(cmds), resp_file_number)
+ ToolsDef.append("cc_resp_%s = @%s" %
+ (resp_file_number, SaveFilePath))
+
+ else:
+ T.Commands[Index] = '%s\n\t%s' % ('
+ \\\n\t'.join(Cpplist), CmdTargetDict[CmdSign])
+
else:
T.Commands.pop(Index)
+
return T, CmdSumDict, CmdTargetDict, CmdCppDict

def CheckCCCmd(self, CommandList):
diff --git a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
index 720d93395a..9f61d49b3a 100644
--- a/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/IncludesAutoGen.py
@@ -203,7 +203,18 @@ ${END}
cc_options = line[len(cc_cmd)+2:].split()
else:
cc_options = line[len(cc_cmd):].split()
- SourceFileAbsPathMap = {os.path.basename(item):item for item in cc_options if not item.startswith("/") and os.path.exists(item)}
+
+ for item in cc_options:
+ if not item.startswith("/"):
+ # if item.startswith("@"):
+ if item.endswith(".txt") and item.startswith("@"):
+ with open(item[1:], "r") as file:
+ source_files = file.readlines()[0].split()
+ SourceFileAbsPathMap = {os.path.basename(file):file for file in source_files if os.path.exists(file)}
+ else:
+ if os.path.exists(item):
+
+ SourceFileAbsPathMap.update({os.path.basename(item): item.strip()})
+
if line in SourceFileAbsPathMap:
current_source = line
if current_source not in ModuleDepDict:


Re: [DxeHttpIoLib PATCH V2 1/3] NetworkPkg/Library: Implementation of Http IO Helper Library

Maciej Rabeda
 

On 02-Nov-20 15:14, Abner Chang wrote:

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Monday, November 2, 2020 8:50 PM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3] NetworkPkg/Library:
Implementation of Http IO Helper Library

Hi Abner,

Comments inline.

Thanks,
Maciej

On 29-Oct-20 07:11, Abner Chang wrote:
-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Thursday, October 29, 2020 2:28 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3]
NetworkPkg/Library:
Implementation of Http IO Helper Library

Thanks for patching spelling problems in MdePkg header.
I will send more comments on v3. Added one here regarding chunk
transfer
security.

Thanks,
Maciej

On 26-Oct-20 07:53, Abner Chang wrote:
Thanks for reviewing this Maciej.

Most of comments are addressed. I also submitted a BZ for the wrong
spelling of CHUNK_TRNASFER_*, the patch is already sent.
V3 of this patch set is also sent to the mailing list.

Other feedbacks are in below,

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Saturday, October 24, 2020 12:25 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3]
NetworkPkg/Library:
Implementation of Http IO Helper Library

Hi Abner,

I understand the concept of moving HTTP I/O layer outside
HttpBootDxe
and
into a separate library and I approve it.
However, there are some things that have to
addressed/fixed/explained
here.
See comments within the patch.

On 20-Oct-20 04:38, Abner Chang wrote:
Add HTTP IO helper library which could be sued by HTTP applications
such as HTTP Boot, Redfish HTTP REST EX driver instance and etc.

Signed-off-by: Abner Chang <abner.chang@hpe.com>

Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Nickle Wang <nickle.wang@hpe.com>
---
NetworkPkg/Include/Library/HttpIoLib.h | 328 +++++++
.../Library/DxeHttpIoLib/DxeHttpIoLib.c | 809
++++++++++++++++++
.../Library/DxeHttpIoLib/DxeHttpIoLib.inf | 43 +
.../Library/DxeHttpIoLib/DxeHttpIoLib.uni | 13 +
4 files changed, 1193 insertions(+)
create mode 100644 NetworkPkg/Include/Library/HttpIoLib.h
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
diff --git a/NetworkPkg/Include/Library/HttpIoLib.h
b/NetworkPkg/Include/Library/HttpIoLib.h
new file mode 100644
index 0000000000..8f3804ca42
--- /dev/null
+++ b/NetworkPkg/Include/Library/HttpIoLib.h
@@ -0,0 +1,328 @@
+/** @file
+ HttpIoLib.h.
+
+(C) Copyright 2020 Hewlett-Packard Development Company,
L.P.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef HTTP_IO_LIB_H_
+#define HTTP_IO_LIB_H_
+
+#include <IndustryStandard/Http11.h>
+
+#include <Library/DpcLib.h>
+#include <Library/HttpLib.h>
+#include <Library/NetLib.h>
+
+#define HTTP_IO_MAX_SEND_PAYLOAD 1024
+#define HTTP_IO_CHUNK_SIZE_STRING_LEN 50
+#define HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH
256
+
+///
+/// HTTP_IO_CALLBACK_EVENT
+///
+typedef enum {
+ HttpIoRequest,
+ HttpIoResponse
+} HTTP_IO_CALLBACK_EVENT;
+
+/**
+ HttpIo Callback function which will be invoked when specified
HTTP_IO_CALLBACK_EVENT happened.
+
+ @param[in] EventType Indicate the Event type that occurs in
the
current callback.
+ @param[in] Message HTTP message which will be send to, or
just
received from HTTP server.
+ @param[in] Context The Callback Context pointer.
+
+ @retval EFI_SUCCESS Tells the HttpIo to continue the HTTP
process.
+ @retval Others Tells the HttpIo to abort the current HTTP
process.
+**/
+typedef
+EFI_STATUS
+(EFIAPI * HTTP_IO_CALLBACK) (
+ IN HTTP_IO_CALLBACK_EVENT EventType,
+ IN EFI_HTTP_MESSAGE *Message,
+ IN VOID *Context
+ );
+
+///
+/// A wrapper structure to hold the received HTTP response data.
+///
+typedef struct {
+ EFI_HTTP_RESPONSE_DATA Response;
+ UINTN HeaderCount;
+ EFI_HTTP_HEADER *Headers;
+ UINTN BodyLength;
+ CHAR8 *Body;
+ EFI_STATUS Status;
+} HTTP_IO_RESPONSE_DATA;
+
+///
+/// HTTP_IO configuration data for IPv4 /// typedef struct {
+ EFI_HTTP_VERSION HttpVersion;
+ UINT32 RequestTimeOut; ///< In milliseconds.
+ UINT32 ResponseTimeOut; ///< In milliseconds.
+ BOOLEAN UseDefaultAddress;
+ EFI_IPv4_ADDRESS LocalIp;
+ EFI_IPv4_ADDRESS SubnetMask;
+ UINT16 LocalPort;
+} HTTP4_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO configuration data for IPv6 /// typedef struct {
+ EFI_HTTP_VERSION HttpVersion;
+ UINT32 RequestTimeOut; ///< In milliseconds.
+ BOOLEAN UseDefaultAddress;
+ EFI_IPv6_ADDRESS LocalIp;
+ UINT16 LocalPort;
+} HTTP6_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO configuration
+///
+typedef union {
+ HTTP4_IO_CONFIG_DATA Config4;
+ HTTP6_IO_CONFIG_DATA Config6;
+} HTTP_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO wrapper of the EFI HTTP service.
+///
+typedef struct {
+ UINT8 IpVersion;
+ EFI_HANDLE Image;
+ EFI_HANDLE Controller;
+ EFI_HANDLE Handle;
+
+ EFI_HTTP_PROTOCOL *Http;
+
+ HTTP_IO_CALLBACK Callback;
+ VOID *Context;
+
+ EFI_HTTP_TOKEN ReqToken;
+ EFI_HTTP_MESSAGE ReqMessage;
+ EFI_HTTP_TOKEN RspToken;
+ EFI_HTTP_MESSAGE RspMessage;
+
+ BOOLEAN IsTxDone;
+ BOOLEAN IsRxDone;
+
+ EFI_EVENT TimeoutEvent;
+ UINT32 Timeout;
+} HTTP_IO;
+
+///
+/// Process code of HTTP chunk transfer.
+///
+typedef enum {
+ HttpIoSendChunkNone = 0,
+ HttpIoSendChunkHeaderZeroContent,
+ HttpIoSendChunkContent,
+ HttpIoSendChunkEndChunk,
+ HttpIoSendChunkFinish
+} HTTP_IO_SEND_CHUNK_PROCESS;
+
+///
+/// Process code of HTTP non chunk transfer.
+///
+typedef enum {
+ HttpIoSendNonChunkNone = 0,
+ HttpIoSendNonChunkHeaderZeroContent,
+ HttpIoSendNonChunkContent,
+ HttpIoSendNonChunkFinish
+} HTTP_IO_SEND_NON_CHUNK_PROCESS;
+
+///
+/// Chunk links for HTTP chunked transfer coding.
+///
+typedef struct {
+ LIST_ENTRY NextChunk;
+ UINTN Length;
+ CHAR8 *Data;
+} HTTP_IO_CHUNKS;
+
+/**
+ Notify the callback function when an event is triggered.
+
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotifyDpc (
+ IN VOID *Context
+ );
+
+/**
+ Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK.
+
+ @param[in] Event The event signaled.
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ );
+
+/**
+ Destroy the HTTP_IO and release the resources.
+
+ @param[in] HttpIo The HTTP_IO which wraps the HTTP service
to
be
destroyed.
+
+**/
+VOID
+HttpIoDestroyIo (
+ IN HTTP_IO *HttpIo
+ );
+
+/**
+ Create a HTTP_IO to access the HTTP service. It will create and
+configure
+ a HTTP child handle.
+
+ @param[in] Image The handle of the driver image.
+ @param[in] Controller The handle of the controller.
+ @param[in] IpVersion IP_VERSION_4 or IP_VERSION_6.
+ @param[in] ConfigData The HTTP_IO configuration data.
+ @param[in] Callback Callback function which will be invoked
when
specified
+ HTTP_IO_CALLBACK_EVENT happened.
+ @param[in] Context The Context data which will be passed to
the
Callback function.
+ @param[out] HttpIo The HTTP_IO.
+
+ @retval EFI_SUCCESS The HTTP_IO is created and configured.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_UNSUPPORTED One or more of the control options
are
not
+ supported in the implementation.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval Others Failed to create the HTTP_IO or configure it.
+
+**/
+EFI_STATUS
+HttpIoCreateIo (
+ IN EFI_HANDLE Image,
+ IN EFI_HANDLE Controller,
+ IN UINT8 IpVersion,
+ IN HTTP_IO_CONFIG_DATA *ConfigData,
+ IN HTTP_IO_CALLBACK Callback,
+ IN VOID *Context,
+ OUT HTTP_IO *HttpIo
+ );
+
+/**
+ Synchronously send a HTTP REQUEST message to the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] Request A pointer to storage such data as URL
and
HTTP method.
+ @param[in] HeaderCount Number of HTTP header structures in
Headers list.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[in] BodyLength Length in bytes of the HTTP body.
+ @param[in] Body Body associated with the HTTP request.
+
+ @retval EFI_SUCCESS The HTTP request is transmitted.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoSendRequest (
+ IN HTTP_IO *HttpIo,
+ IN EFI_HTTP_REQUEST_DATA *Request, OPTIONAL
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers, OPTIONAL
+ IN UINTN BodyLength,
+ IN VOID *Body OPTIONAL
+ );
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] RecvMsgHeader TRUE to receive a new HTTP
response
(from message header).
+ FALSE to continue receive the previous response
message.
+ @param[out] ResponseData Point to a wrapper of the received
response data.
+
+ @retval EFI_SUCCESS The HTTP response is received.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoRecvResponse (
+ IN HTTP_IO *HttpIo,
+ IN BOOLEAN RecvMsgHeader,
+ OUT HTTP_IO_RESPONSE_DATA *ResponseData
+ );
+
+/**
+ Get the value of the content length if there is a "Content-Length"
header.
+
+ @param[in] HeaderCount Number of HTTP header structures
in
Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ContentLength Pointer to save the value of the
content
length.
+
+ @retval EFI_SUCCESS Successfully get the content length.
+ @retval EFI_NOT_FOUND No "Content-Length" header in the
Headers.
+
+**/
+EFI_STATUS
+HttpIoGetContentLength (
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT UINTN *ContentLength
+ );
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] HeaderCount Number of headers in Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ChunkListHead A pointer to receivce list head of
chunked
data.
+ Caller has to release memory of ChunkListHead
+ and all list entries.
+ @param[out] ContentLength Total content length
+
+ @retval EFI_SUCCESS The HTTP chunked transfer is received.
+ @retval EFI_NOT_FOUND No chunked transfer coding header
found.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_INVALID_PARAMETER Improper parameters.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoGetChunkedTransferContent (
+ IN HTTP_IO *HttpIo,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT LIST_ENTRY **ChunkListHead,
+ OUT UINTN *ContentLength
+ );
+
+/**
+ Send HTTP request in chunks.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] SendChunkProcess Pointer to current chunk process
status.
+ @param[out] RequestMessage Request to send.
+
+ @retval EFI_SUCCESS Successfully to send chunk data
according
to
SendChunkProcess.
+ @retval Other Other errors.
+
+**/
+EFI_STATUS
+HttpIoSendChunkedTransfer (
+ IN HTTP_IO *HttpIo,
+ IN HTTP_IO_SEND_CHUNK_PROCESS *SendChunkProcess,
+ IN EFI_HTTP_MESSAGE *RequestMessage
+);
+#endif
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
new file mode 100644
index 0000000000..fbce86ea5c
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
@@ -0,0 +1,809 @@
+/** @file
+ Http IO Helper Library.
+
+ (C) Copyright 2020 Hewlett-Packard Development Company,
L.P.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent **/
+
+#include <Uefi.h>
+
+#include <Protocol/Http.h>
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HttpIoLib.h>
+#include <Library/MemoryAllocationLib.h> #include
+<Library/PrintLib.h> #include <Library/UefiBootServicesTableLib.h>
+
+/**
+ Notify the callback function when an event is triggered.
+
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotifyDpc (
+ IN VOID *Context
+ )
+{
+ *((BOOLEAN *) Context) = TRUE;
+}
+
+/**
+ Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK.
+
+ @param[in] Event The event signaled.
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ //
+ // Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK
+ //
+ QueueDpc (TPL_CALLBACK, HttpIoNotifyDpc, Context); }
+
+/**
+ Destroy the HTTP_IO and release the resources.
+
+ @param[in] HttpIo The HTTP_IO which wraps the HTTP service
to
be
destroyed.
+
+**/
+VOID
+HttpIoDestroyIo (
+ IN HTTP_IO *HttpIo
+ )
+{
+ EFI_HTTP_PROTOCOL *Http;
+ EFI_EVENT Event;
+
+ if (HttpIo == NULL) {
+ return;
+ }
+
+ Event = HttpIo->ReqToken.Event;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Event = HttpIo->RspToken.Event;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Event = HttpIo->TimeoutEvent;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Http = HttpIo->Http;
+ if (Http != NULL) {
+ Http->Configure (Http, NULL);
+ gBS->CloseProtocol (
+ HttpIo->Handle,
+ &gEfiHttpProtocolGuid,
+ HttpIo->Image,
+ HttpIo->Controller
+ );
+ }
+
+ NetLibDestroyServiceChild (
+ HttpIo->Controller,
+ HttpIo->Image,
+ &gEfiHttpServiceBindingProtocolGuid,
+ HttpIo->Handle
+ );
+}
+
+/**
+ Create a HTTP_IO to access the HTTP service. It will create and
+configure
+ a HTTP child handle.
+
+ @param[in] Image The handle of the driver image.
+ @param[in] Controller The handle of the controller.
+ @param[in] IpVersion IP_VERSION_4 or IP_VERSION_6.
+ @param[in] ConfigData The HTTP_IO configuration data ,
+ NULL means not to configure the HTTP child.
+ @param[in] Callback Callback function which will be invoked
when
specified
+ HTTP_IO_CALLBACK_EVENT happened.
+ @param[in] Context The Context data which will be passed to
the
Callback function.
+ @param[out] HttpIo The HTTP_IO.
+
+ @retval EFI_SUCCESS The HTTP_IO is created and configured.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_UNSUPPORTED One or more of the control options
are
not
+ supported in the implementation.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval Others Failed to create the HTTP_IO or configure it.
+
+**/
+EFI_STATUS
+HttpIoCreateIo (
+ IN EFI_HANDLE Image,
+ IN EFI_HANDLE Controller,
+ IN UINT8 IpVersion,
+ IN HTTP_IO_CONFIG_DATA *ConfigData, OPTIONAL
+ IN HTTP_IO_CALLBACK Callback,
+ IN VOID *Context,
+ OUT HTTP_IO *HttpIo
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_CONFIG_DATA HttpConfigData;
+ EFI_HTTPv4_ACCESS_POINT Http4AccessPoint;
+ EFI_HTTPv6_ACCESS_POINT Http6AccessPoint;
+ EFI_HTTP_PROTOCOL *Http;
+ EFI_EVENT Event;
+
+ if ((Image == NULL) || (Controller == NULL) || (HttpIo == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (IpVersion != IP_VERSION_4 && IpVersion != IP_VERSION_6) {
+ return EFI_UNSUPPORTED;
+ }
+
+ ZeroMem (HttpIo, sizeof (HTTP_IO)); ZeroMem (&HttpConfigData,
+ sizeof (EFI_HTTP_CONFIG_DATA));
+
+ //
+ // Create the HTTP child instance and get the HTTP protocol.
+ //
+ Status = NetLibCreateServiceChild (
+ Controller,
+ Image,
+ &gEfiHttpServiceBindingProtocolGuid,
+ &HttpIo->Handle
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = gBS->OpenProtocol (
+ HttpIo->Handle,
+ &gEfiHttpProtocolGuid,
+ (VOID **) &Http,
+ Image,
+ Controller,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status) || (Http == NULL)) {
+ goto ON_ERROR;
+ }
+
+ //
+ // Init the configuration data and configure the HTTP child.
+ //
+ HttpIo->Image = Image;
+ HttpIo->Controller = Controller;
+ HttpIo->IpVersion = IpVersion;
+ HttpIo->Http = Http;
+ HttpIo->Callback = Callback;
+ HttpIo->Context = Context;
+
+ if (ConfigData != NULL) {
+ if (HttpIo->IpVersion == IP_VERSION_4) {
+ HttpConfigData.LocalAddressIsIPv6 = FALSE;
+ HttpConfigData.HttpVersion = ConfigData-
Config4.HttpVersion;
+ HttpConfigData.TimeOutMillisec = ConfigData-
Config4.RequestTimeOut;
+
+ Http4AccessPoint.UseDefaultAddress = ConfigData-
Config4.UseDefaultAddress;
+ Http4AccessPoint.LocalPort = ConfigData->Config4.LocalPort;
+ IP4_COPY_ADDRESS (&Http4AccessPoint.LocalAddress,
&ConfigData-
Config4.LocalIp);
+ IP4_COPY_ADDRESS (&Http4AccessPoint.LocalSubnet,
&ConfigData-
Config4.SubnetMask);
+ HttpConfigData.AccessPoint.IPv4Node = &Http4AccessPoint;
+ } else {
+ HttpConfigData.LocalAddressIsIPv6 = TRUE;
+ HttpConfigData.HttpVersion = ConfigData-
Config6.HttpVersion;
+ HttpConfigData.TimeOutMillisec = ConfigData-
Config6.RequestTimeOut;
+
+ Http6AccessPoint.LocalPort = ConfigData->Config6.LocalPort;
+ IP6_COPY_ADDRESS (&Http6AccessPoint.LocalAddress,
&ConfigData-
Config6.LocalIp);
+ HttpConfigData.AccessPoint.IPv6Node = &Http6AccessPoint;
+ }
+
+ Status = Http->Configure (Http, &HttpConfigData);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ }
+
+ //
+ // Create events for variuos asynchronous operations.
+ //
+ Status = gBS->CreateEvent (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ HttpIoNotify,
+ &HttpIo->IsTxDone,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->ReqToken.Event = Event;
+ HttpIo->ReqToken.Message = &HttpIo->ReqMessage;
+
+ Status = gBS->CreateEvent (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ HttpIoNotify,
+ &HttpIo->IsRxDone,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->RspToken.Event = Event;
+ HttpIo->RspToken.Message = &HttpIo->RspMessage;
+
+ //
+ // Create TimeoutEvent for response // Status = gBS->CreateEvent
+ (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->TimeoutEvent = Event;
+ return EFI_SUCCESS;
+
+ON_ERROR:
+ HttpIoDestroyIo (HttpIo);
+
+ return Status;
+}
+
+/**
+ Synchronously send a HTTP REQUEST message to the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] Request A pointer to storage such data as URL
and
HTTP method.
+ @param[in] HeaderCount Number of HTTP header structures in
Headers list.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[in] BodyLength Length in bytes of the HTTP body.
+ @param[in] Body Body associated with the HTTP request.
+
+ @retval EFI_SUCCESS The HTTP request is trasmitted.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoSendRequest (
+ IN HTTP_IO *HttpIo,
+ IN EFI_HTTP_REQUEST_DATA *Request,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ IN UINTN BodyLength,
+ IN VOID *Body
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_PROTOCOL *Http;
+
+ if (HttpIo == NULL || HttpIo->Http == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ HttpIo->ReqToken.Status = EFI_NOT_READY;
+ HttpIo->ReqToken.Message->Data.Request = Request;
+ HttpIo->ReqToken.Message->HeaderCount = HeaderCount;
+ HttpIo->ReqToken.Message->Headers = Headers;
+ HttpIo->ReqToken.Message->BodyLength = BodyLength;
+ HttpIo->ReqToken.Message->Body = Body;
+
+ if (HttpIo->Callback != NULL) {
+ Status = HttpIo->Callback (
+ HttpIoRequest,
+ HttpIo->ReqToken.Message,
+ HttpIo->Context
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ //
+ // Queue the request token to HTTP instances.
+ //
+ Http = HttpIo->Http;
+ HttpIo->IsTxDone = FALSE;
+ Status = Http->Request (
+ Http,
+ &HttpIo->ReqToken
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Poll the network until transmit finish.
+ //
+ while (!HttpIo->IsTxDone) {
+ Http->Poll (Http);
+ }
+
+ return HttpIo->ReqToken.Status;
+}
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] RecvMsgHeader TRUE to receive a new HTTP
response
(from message header).
+ FALSE to continue receive the previous response
message.
+ @param[out] ResponseData Point to a wrapper of the received
response data.
+
+ @retval EFI_SUCCESS The HTTP response is received.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoRecvResponse (
+ IN HTTP_IO *HttpIo,
+ IN BOOLEAN RecvMsgHeader,
+ OUT HTTP_IO_RESPONSE_DATA *ResponseData
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_PROTOCOL *Http;
+
+ if (HttpIo == NULL || HttpIo->Http == NULL || ResponseData ==
NULL)
{
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Create new TimeoutEvent for response // if
+ (HttpIo->TimeoutEvent != NULL) {
+ gBS->CloseEvent (HttpIo->TimeoutEvent);
+ HttpIo->TimeoutEvent = NULL;
+ }
+
+ Status = gBS->CreateEvent (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &HttpIo->TimeoutEvent
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Is it necessary to recreate the event? Wouldn't this be enough to reset
it?
No, we don’t have to recreate this event.
gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
I made some changes in this function.
+
+ //
+ // Start the timer, and wait Timeout seconds to receive the header
packet.
+ //
+ Status = gBS->SetTimer (HttpIo->TimeoutEvent, TimerRelative,
+ HttpIo->Timeout * TICKS_PER_MS); if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Queue the response token to HTTP instances.
+ //
+ HttpIo->RspToken.Status = EFI_NOT_READY; if (RecvMsgHeader) {
+ HttpIo->RspToken.Message->Data.Response =
+ &ResponseData->Response; } else {
+ HttpIo->RspToken.Message->Data.Response = NULL; }
+ HttpIo->RspToken.Message->HeaderCount = 0;
+ HttpIo->RspToken.Message->Headers = NULL;
+ HttpIo->RspToken.Message->BodyLength = ResponseData-
BodyLength;
+ HttpIo->RspToken.Message->Body = ResponseData->Body;
+
+ Http = HttpIo->Http;
+ HttpIo->IsRxDone = FALSE;
+ Status = Http->Response (
+ Http,
+ &HttpIo->RspToken
+ );
+
+ if (EFI_ERROR (Status)) {
+ gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
+ return Status;
+ }
+
+ //
+ // Poll the network until receive finish.
+ //
+ while (!HttpIo->IsRxDone && ((HttpIo->TimeoutEvent == NULL) ||
EFI_ERROR (gBS->CheckEvent (HttpIo->TimeoutEvent)))) {
+ Http->Poll (Http);
+ }
+
+ gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
+
+ if (!HttpIo->IsRxDone) {
+ //
+ // Timeout occurs, cancel the response token.
+ //
+ Http->Cancel (Http, &HttpIo->RspToken);
+
+ Status = EFI_TIMEOUT;
+
+ return Status;
+ } else {
+ HttpIo->IsRxDone = FALSE;
+ }
+
+ if ((HttpIo->Callback != NULL) &&
+ (HttpIo->RspToken.Status == EFI_SUCCESS || HttpIo-
RspToken.Status == EFI_HTTP_ERROR)) {
+ Status = HttpIo->Callback (
+ HttpIoResponse,
+ HttpIo->RspToken.Message,
+ HttpIo->Context
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ //
+ // Store the received data into the wrapper.
+ //
+ ResponseData->Status = HttpIo->RspToken.Status;
+ ResponseData->HeaderCount = HttpIo->RspToken.Message-
HeaderCount;
+ ResponseData->Headers = HttpIo->RspToken.Message->Headers;
+ ResponseData->BodyLength = HttpIo->RspToken.Message-
BodyLength;
+
+ return Status;
+}
+
+/**
+ Get the value of the content length if there is a "Content-Length"
header.
+
+ @param[in] HeaderCount Number of HTTP header structures
in
Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ContentLength Pointer to save the value of the
content
length.
+
+ @retval EFI_SUCCESS Successfully get the content length.
+ @retval EFI_NOT_FOUND No "Content-Length" header in the
Headers.
+
+**/
+EFI_STATUS
+HttpIoGetContentLength (
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT UINTN *ContentLength
+ )
+{
+ EFI_HTTP_HEADER *Header;
+
+ Header = HttpFindHeader (HeaderCount, Headers,
+ HTTP_HEADER_CONTENT_LENGTH); if (Header == NULL) {
+ return EFI_NOT_FOUND;
+ }
+
+ return AsciiStrDecimalToUintnS (Header->FieldValue, (CHAR8 **)
+NULL, ContentLength); }
+/**
+ Send HTTP request in chunks.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] SendChunkProcess Pointer to current chunk process
status.
+ @param[in] RequestMessage Request to send.
+
+ @retval EFI_SUCCESS Successfully to send chunk data
according
to
SendChunkProcess.
+ @retval Other Other errors.
+
+**/
+EFI_STATUS
+HttpIoSendChunkedTransfer (
+ IN HTTP_IO *HttpIo,
+ IN HTTP_IO_SEND_CHUNK_PROCESS *SendChunkProcess,
+ IN EFI_HTTP_MESSAGE *RequestMessage
+)
+{
+ EFI_STATUS Status;
+ EFI_HTTP_HEADER *NewHeaders;
+ EFI_HTTP_HEADER *ContentLengthHeader;
+ UINTN AddNewHeader;
+ UINTN HeaderCount;
+ CHAR8 *MessageBody;
+ UINTN MessageBodyLength;
+ CHAR8 ChunkLengthStr [HTTP_IO_CHUNK_SIZE_STRING_LEN];
+ EFI_HTTP_REQUEST_DATA *SentRequestData;
This is a new function and I already see a difference in the coding
standard.
Please align variable names like in previous functions.
Done

+
+ AddNewHeader = 0;
+ NewHeaders = NULL;
+ MessageBody = NULL;
+ ContentLengthHeader = NULL;
+ MessageBodyLength = 0;
Alignment of assignments.
Done

+
+ switch (*SendChunkProcess) {
+ case HttpIoSendChunkHeaderZeroContent:
Case spacing: 2 spaces. Applies throughout new code.
+ ContentLengthHeader =
+ HttpFindHeader(RequestMessage->HeaderCount,
RequestMessage-
Headers,
+ HTTP_HEADER_CONTENT_LENGTH);
Spacing after function call name. This applies from this point until the
end
of
patch.
Done
+ if (ContentLengthHeader == NULL) {
+ AddNewHeader = 1;
+ }
+
+ NewHeaders = AllocateZeroPool((RequestMessage-
HeaderCount
+
+ AddNewHeader) * sizeof(EFI_HTTP_HEADER));
Lack of NULL pointer test for NewHeaders (out of memory case).
+ CopyMem ((VOID*)NewHeaders, (VOID *)RequestMessage-
Headers,
RequestMessage->HeaderCount * sizeof (EFI_HTTP_HEADER));
+ if (AddNewHeader == 0) {
+ //
+ // Override content-length to Transfer-Encoding.
+ //
+ ContentLengthHeader = HttpFindHeader (RequestMessage-
HeaderCount, NewHeaders, HTTP_HEADER_CONTENT_LENGTH);
+ ContentLengthHeader->FieldName = NULL;
+ ContentLengthHeader->FieldValue = NULL;
+ } else {
+ ContentLengthHeader = NewHeaders + RequestMessage-
HeaderCount;
+ }
+ HttpSetFieldNameAndValue(ContentLengthHeader,
HTTP_HEADER_TRANSFER_ENCODING,
HTTP_HEADER_TRANSFER_ENCODING_CHUNKED);
+ HeaderCount = RequestMessage->HeaderCount +
AddNewHeader;
Move HeaderCount assignment before NewHeaders allocation. Use
HeaderCount as argument for NewHeaders allocation.
+ MessageBodyLength = 0;
+ MessageBody = NULL;
+ SentRequestData = RequestMessage->Data.Request;
+ break;
+
+ case HttpIoSendChunkContent:
+ HeaderCount = 0;
+ NewHeaders = NULL;
+ SentRequestData = NULL;
+ if (RequestMessage->BodyLength >
HTTP_IO_MAX_SEND_PAYLOAD) {
+ MessageBodyLength = HTTP_IO_MAX_SEND_PAYLOAD;
+ } else {
+ MessageBodyLength = RequestMessage->BodyLength;
+ }
+ AsciiSPrint (ChunkLengthStr, HTTP_IO_CHUNK_SIZE_STRING_LEN,
+ "%x%c%c", MessageBodyLength,
CHUNKED_TRNASFER_CODING_CR,
+ CHUNKED_TRNASFER_CODING_LF);
Line too long, break down in accordance to coding standard.
Done

+ MessageBody = AllocatePool (AsciiStrLen (ChunkLengthStr) +
MessageBodyLength + 2);
+ if (MessageBody == NULL) {
+ DEBUG((DEBUG_ERROR, "Not enough memory for chunk
transfer\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (MessageBody, ChunkLengthStr, AsciiStrLen
(ChunkLengthStr));
+ CopyMem (MessageBody + AsciiStrLen (ChunkLengthStr),
RequestMessage->Body, MessageBodyLength);
+ *(MessageBody + AsciiStrLen (ChunkLengthStr) +
MessageBodyLength)
= CHUNKED_TRNASFER_CODING_CR;
+ *(MessageBody + AsciiStrLen (ChunkLengthStr) +
MessageBodyLength
+ 1) = CHUNKED_TRNASFER_CODING_LF;
+ RequestMessage->BodyLength -= MessageBodyLength;
+ RequestMessage->Body = (VOID *)((CHAR8 *)RequestMessage-
Body + MessageBodyLength);
+ MessageBodyLength += (AsciiStrLen (ChunkLengthStr) + 2);
This block could use some spacing and comments to improve
readability.
Done

+ if (RequestMessage->BodyLength == 0) {
+ *SendChunkProcess = HttpIoSendChunkEndChunk;
+ }
+ break;
+
+ case HttpIoSendChunkEndChunk:
+ HeaderCount = 0;
+ NewHeaders = NULL;
+ SentRequestData = NULL;
+ AsciiSPrint (ChunkLengthStr, HTTP_IO_CHUNK_SIZE_STRING_LEN,
"0%c%c%c%c",
+ CHUNKED_TRNASFER_CODING_CR,
CHUNKED_TRNASFER_CODING_LF,
+ CHUNKED_TRNASFER_CODING_CR,
CHUNKED_TRNASFER_CODING_LF
+ );
Please break function calls in accordance with coding standard.
Done
+ MessageBody = AllocatePool (AsciiStrLen(ChunkLengthStr));
+ if (MessageBody == NULL) {
+ DEBUG((DEBUG_ERROR, "Not enough memory for the end
chunk
transfer\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (MessageBody, ChunkLengthStr, AsciiStrLen
(ChunkLengthStr));
+ MessageBodyLength = AsciiStrLen(ChunkLengthStr);
+ *SendChunkProcess = HttpIoSendChunkFinish;
+ break;
+
+ default:
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = HttpIoSendRequest(
+ HttpIo,
+ SentRequestData,
+ HeaderCount,
+ NewHeaders,
+ MessageBodyLength,
+ MessageBody
+ );
+ if (ContentLengthHeader != NULL) {
+ if (ContentLengthHeader->FieldName != NULL) {
+ FreePool(ContentLengthHeader->FieldName);
+ }
+ if (ContentLengthHeader->FieldValue != NULL) {
+ FreePool(ContentLengthHeader->FieldValue);
+ }
+ ContentLengthHeader = NULL;
NULL assignments are not necessary here as we are exiting the
function
and
those variables will not be used.
Right, fixed.
+ }
+ if (NewHeaders != NULL) {
+ FreePool(NewHeaders);
+ NewHeaders = NULL;
+ }
+ if (MessageBody != NULL) {
+ FreePool(MessageBody);
+ MessageBody = NULL;
+ }
+ return Status;
+}
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] HeaderCount Number of headers in Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ChunkListHead A pointer to receivce list head of
chunked
data.
Spelling: receive.
Fixed.

+ Caller has to release memory of ChunkListHead
+ and all list entries.
+ @param[out] ContentLength Total content length
+
+ @retval EFI_SUCCESS The HTTP chunked transfer is received.
+ @retval EFI_NOT_FOUND No chunked transfer coding header
found.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_INVALID_PARAMETER Improper parameters.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoGetChunkedTransferContent (
+ IN HTTP_IO *HttpIo,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT LIST_ENTRY **ChunkListHead,
+ OUT UINTN *ContentLength
+ )
+{
+ EFI_HTTP_HEADER *Header;
+ CHAR8 ChunkSizeAscii[256];
+ EFI_STATUS Status;
+ UINTN Index;
+ HTTP_IO_RESPONSE_DATA ResponseData;
+ UINTN TotalLength;
+ LIST_ENTRY *HttpChunks;
+ HTTP_IO_CHUNKS *ThisChunk;
Variable alignment.
+ LIST_ENTRY *ThisListEntry;
+
+ if (ChunkListHead == NULL || ContentLength == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *ContentLength = 0;
+ Header = HttpFindHeader (HeaderCount, Headers,
+ HTTP_HEADER_TRANSFER_ENCODING); if (Header == NULL) {
+ return EFI_NOT_FOUND;
+ }
+ if (AsciiStrCmp (Header->FieldValue,
+ HTTP_HEADER_TRANSFER_ENCODING_CHUNKED) == 0) {
Please do a negation comparison instead and return. It reduces one
level
of
indentation.
Done
+ //
+ // Loop to get all chunks.
+ //
Please avoid empty single-line comments when starting and ending
comment block.
+ TotalLength = 0;
+ HttpChunks = (LIST_ENTRY *)AllocateZeroPool (sizeof
(LIST_ENTRY));
+ if (HttpChunks == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (HttpChunks);
Add a bit of spacing above to improve readability.
Fixed
+ DEBUG ((DEBUG_INFO, " Chunked transfer\n"));
+ while (TRUE) {
+ ZeroMem((VOID *)&ResponseData,
sizeof(HTTP_IO_RESPONSE_DATA));
+ ResponseData.BodyLength =
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH;
+ ResponseData.Body = ChunkSizeAscii;
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Returning here creates memory leak risk - chunks previously received
will
not
be freed.
Fixed
+ //
+ // Decoding Chunked Transfer Coding.
+ // Only decode chunk-size and last chunk.
+ //
+ DEBUG ((DEBUG_INFO, " Chunk HTTP Response StatusCode -
%d\n",
ResponseData.Response.StatusCode));
+ //
+ // Break if this is last chunk.
+ //
+ if (ChunkSizeAscii [0] ==
CHUNKED_TRNASFER_CODING_LAST_CHUNK)
{
+ Status = EFI_SUCCESS;
+ DEBUG ((DEBUG_INFO, " Last chunk\n"));
+ ThisChunk = (HTTP_IO_CHUNKS *)AllocateZeroPool (sizeof
(HTTP_IO_CHUNKS));
+ if (ThisChunk == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (&ThisChunk->NextChunk);
+ ThisChunk->Length = ResponseData.BodyLength - 1 - 2; // Minus
sizeof '0' and CRLF.
+ ThisChunk->Data = (CHAR8 *)AllocatePool (ThisChunk->Length);
+ if (ThisChunk->Data == NULL) {
+ FreePool ((UINT8 *)ThisChunk);
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ CopyMem ((UINT8 *)ThisChunk->Data, (UINT8
*)ResponseData.Body
+ 1, ThisChunk->Length);
+ TotalLength += ThisChunk->Length;
+ InsertTailList (HttpChunks, &ThisChunk->NextChunk);
+ break;
+ }
+
+ //
+ // Get the chunk length
+ //
+ Index = 0;
+ while ((ChunkSizeAscii [Index] !=
CHUNKED_TRNASFER_CODING_EXTENSION_SAPERATOR) &&
+ (ChunkSizeAscii [Index] !=
+ (CHAR8)CHUNKED_TRNASFER_CODING_CR) &&
Spelling errors in macros above.
Oops. BZ is submitted for this typo in http11.h.
INVALID URI REMOVED
3A__bugzilla.tianocore.org_show-5Fbug.cgi-3Fid-
3D3019&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulks
kz6qU3NYRO03nHp9P7Z5q59A3E&m=f6zs_mrzwCCYwQSl3p4Nbuib7Ln4gVcO
KS74o-Xc524&s=g1WHLCl8Uzm8YKLYknM2Y7T0d4gT7fv_58qhc4Qejj0&e=
Patch is set to the mailing list for review.


+ (Index !=
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH)) {
+ Index ++;
+ };
+ if (Index ==
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH)
{
+ return EFI_NOT_FOUND;
+ }
Again - memory leak.
Fixed

+ ChunkSizeAscii[Index] = 0;
+ AsciiStrHexToUintnS (ChunkSizeAscii, NULL, ContentLength);
+ DEBUG ((DEBUG_INFO, " Length of this chunk %d\n",
*ContentLength));
+ //
+ // Receive the data;
+ //
+ ThisChunk = (HTTP_IO_CHUNKS *)AllocateZeroPool (sizeof
(HTTP_IO_CHUNKS));
+ if (ThisChunk == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ ResponseData.BodyLength = *ContentLength;
+ ResponseData.Body = (CHAR8 *)AllocatePool (*ContentLength);
+ if (ResponseData.Body == NULL) {
+ FreePool (ThisChunk);
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (&ThisChunk->NextChunk);
+ ThisChunk->Length = *ContentLength;
+ ThisChunk->Data = ResponseData.Body;
+ InsertTailList (HttpChunks, &ThisChunk->NextChunk);
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ goto DeleterChunks;
+ }
+ //
+ // Read CRLF
+ //
+ ZeroMem((VOID *)&ResponseData,
sizeof(HTTP_IO_RESPONSE_DATA));
+ ResponseData.BodyLength = 2;
+ ResponseData.Body = ChunkSizeAscii;
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ goto DeleterChunks;
+ }
+
+ TotalLength += *ContentLength;
The following code requires some kind of protection against receiving
an
infinite number of chunks.
Currently I could just not send a last chunk and wait for the platform to
crash
or/and run out of memory :)
Good point! However, I can't think of any good way to prevent from this
happens. This depends on the reliability of content provider and how the
web service handles chunk transfer, right?
From system firmware viewpoint, we would connect to a trusted
content
source for either HTTP boot image and Redfish service resource.
Do you think this concern is valid for the real practice?

Or give it a limited size of payload? Like the size larger than 1G could be
considered as an ill content?
What would be the largest payload that you would like to use
chunk-transfer for?
For the HTTP response, whether to use chunk transfer is at the discretion
of Redfish service. We currently can only tell if the response payload is in
chunk transfer or not using Transfer-Encoding HTTP response header but not
able to predict the size of payload unless we get the first chunk.
In the real practice, I see a large payload like AttributeRegistry is
transferred back in chunk, however, I also see some small size payload is
transferred back in chunk transfer as well.
I see that the chunked transfer is not being currently used in HTTP
Boot, so we should stick to current use case -  Redfish.
Yes, currently only RedfishPkg use HTTP chunk transfer APIs defined in
DxeHttpIoLib. But what do you mean to stick the use case to Redfish only?
Those chunk transfer related APIs could be commonly used by any edk2
HTTP clients need chunk transfer although we only have two use cases (and
only Redfish uses it so far).
I only meant that it would be reasonable to come up with a
chunk-transfer limit that is dictated solely by Redfish use case, since
it is currently the only one around.
In case other SW pieces would like to consume HTTP I/O library for
chunk-transfer and see that the limit is too small, they could propose a
patch to extend it.
Yes, we can do it.
The other approach would be to somehow dynamically assess the limit by
looking at how much free memory is available on the platform.
Can we estimate what would be the largest payload Redfish would like to
push through?
The biggest payload of Redfish resource which sent back to client could be AttaibuteRegistry. One example here is the raw text JSON file of AttaibuteRegistry is 1.2MB, however it is reduced to ~65K using gzip. How big the AttaibuteRegistry payload depends on the platform and also depends on if Redfish service sends back the payload using gzip.
We can just limit it to 10MB for now, that says we break the chunk transfer if TotalLength > 10MB. We can adjust it later if any uses case request it. How do you think?
Since the limit is so small, there is no point in checking it against total amount of memory. For the time being, we should be fine with the limit you have suggested :)

+ };
+ *ContentLength = TotalLength;
+ *ChunkListHead = HttpChunks;
+ DEBUG ((DEBUG_INFO, " Total of lengh of chunks :%d\n",
TotalLength));
+ return EFI_SUCCESS;
+ } else {
+ return EFI_NOT_FOUND;
+ }
+DeleterChunks:;
Name this label ExitDeleteChunks and remove the comma.
+ while (!IsListEmpty (HttpChunks)) {
+ ThisListEntry = GetFirstNode (HttpChunks);
+ RemoveEntryList (ThisListEntry);
+ };
+ return Status;
+}
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
new file mode 100644
index 0000000000..a02b409547
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
@@ -0,0 +1,43 @@
+## @file
+# This library instance provides HTTP IO helper functions.
+#
+# (C) Copyright 2020 Hewlett Packard Enterprise Development
LP<BR>
#
+SPDX-License-Identifier: BSD-2-Clause-Patent # ##
+
+[Defines]
+ INF_VERSION = 0x0001001b
+ BASE_NAME = DxeHttpIoLib
+ MODULE_UNI_FILE = DxeHttpIoLib.uni
+ FILE_GUID = 50B198F8-7986-4F51-A857-CFE4643D59F3
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = HttpIoLib| DXE_CORE DXE_DRIVER
DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION
UEFI_DRIVER
+
+#
+# The following information is for reference only and not required by
the
build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARM64
RISCV64
+#
+
+[Sources]
+ DxeHttpIoLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ NetworkPkg/NetworkPkg.dec
+
+
+[LibraryClasses]
+ BaseLib
+ BaseMemoryLib
+ DebugLib
+ DpcLib
+ MemoryAllocationLib
+ PrintLib
+ UefiBootServicesTableLib
+
+[Protocols]
+ gEfiHttpProtocolGuid ## SOMETIMES_CONSUMES
+
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
new file mode 100644
index 0000000000..d419b1a49d
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
@@ -0,0 +1,13 @@
+// /** @file
+// The library instance provides HTTP IO helper functions.
+//
+// (C) Copyright 2020 Hewlett Packard Enterprise Development
LP<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
+
+
+#string STR_MODULE_ABSTRACT #language en-US "HTTP IO
Helper
Library"
+
+#string STR_MODULE_DESCRIPTION #language en-US "The
library
instance provides HTTP IO helper functions."
+






Re: RFC: Universal Payload Interface

Laszlo Ersek
 

On 10/28/20 04:26, Ni, Ray wrote:
Laszlo,
Thank you for the comments.
Reply inline.

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, October 27, 2020 10:21 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Cc: Zimmer, Vincent <vincent.zimmer@intel.com>; Ma, Maurice
<maurice.ma@intel.com>; Rangarajan, Ravi P <ravi.p.rangarajan@intel.com>;
Dong, Guo <guo.dong@intel.com>; Hau, Tze-ming <tze-ming.hau@intel.com>
Subject: Re: [edk2-devel] RFC: Universal Payload Interface

On 10/23/20 03:18, Ni, Ray wrote:
With the fact that there are many different firmware implementations, we
tried to decouple today's monolithic UEFI firmware binary to two independent
components: bootloader and payload.

(1) "Bootloader" is an extremely loaded word. Regardless of everything
else in this topic, I strongly suggest picking a different name.

We already have "Platform Init" or "PI"; maybe use "Silicon Init" or "SI"?
I am a UEFI guy for many years. So your suggestion "PI" looks very friendly to me.
But I am not sure if the audiences include broader people, like coreboot, SBL developers,
do they like the names.

Personally I am open to any name as long as the concept is not changed: the binary blob
is responsible to initialize the silicon.




(2) What is the *exact* use case (or workflow) that the proposed
interface enables, or improves?
What groups of people (what roles) are supposed to benefit from the
proposed interface?
1. Unified UEFI Payload Binary.
By standardizing the bootloader->payload interface, we keep in mind to move all
platform/silicon specific implementation to bootloader and all the specific info is
passed to payload through the standard interface such that the payload doesn't need
to deal with concrete hardware but just the abstracted info. It gives possibility to create
the unified UEFI payload binary that can run in any platform/silicon (off course, one binary
per one CPU arch). Just like today's UEFI Shell.
It's a huge save on validation side to every company that uses UEFI as boot solution for
hardware.
People may argue maintaining such a binary causes additional overhead. I agree.
But I am optimistic on such direction.
(The code will be still in open source.)

2. Bootloaders
It shows an attitude of EDKII community that it doesn't restrict to use EDKII PEI as the
only acknowledged silicon code execution environment. The standard interface as a promise
allows any compliant bootloader to work with EDKII UEFI Payload.

3. Payloads
I saw different tries to change EDKII DXE environment for different hardware/OS.
It may cause defragmentation of UEFI spec. The standard interface also allows any
compliant payload to be created.
- I don't have anything against this, as long as existent platforms are
not required to adopt the new scheme.

- The above description helps, but it is *still* too generic for me to
understand:

(a) Intel already distributes Firmware Support Packages, which are
supposed to deal with RAM / chipset initialization, AIUI,

(b) the PI spec is not tied to edk2 PEIMs, and I don't see where EDKII
PEI modules are currently the only "acknowledged" silicon init
environment. The edk2 tree itself seems to contain platforms that don't
use the edk2 PEI module set at all, but (IIRC) jump from SEC to DXE. I
believe "ArmPlatformPkg/PrePi" and "ArmVirtPkg/PrePi" are related to this.

(c) Replacing edk2 DXE should already be possible without replacing the
edk2 DXE IPL PEIM -- isn't it enough to change the contents of the boot
firmware volume?

I haven't looked at the above interaces in detail for a very long time
now, but I feel we already have the necessary abstractions in place. So
clearly they are insufficient for *some* workflows / use cases. That's
what I'd like to understand more closely. What are the specific problems
with the edk2 offerings? Can you describe example activities that
vendors or other stakeholders would like to perform today, but they
can't, or at high cost only?

I'm quite "out of the loop" on how firmware is composed *in practice*
for proprietary / binary-only / multi-vendor platforms. With my
background in OVMF, I could be missing your point-of-view entirely.

Which is why I want to say very clearly that I'm not attempting to
"block" this proposal at all -- as long as it causes neither
regressions, nor new requirements, for existent platforms --; I just
think the limitations of the current interfaces / implementation, and
the desired improvements, should be written up precisely. Case studies,
actual projects etc would help.

(I understand that you referenced actual code already, but I don't have
capacity for reviewing code, without an actual use case for OVMF. Hence
my request for a natural language description.)

Thanks
Laszlo


Re: [DxeHttpIoLib PATCH V2 1/3] NetworkPkg/Library: Implementation of Http IO Helper Library

Abner Chang
 

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Monday, November 2, 2020 8:50 PM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3] NetworkPkg/Library:
Implementation of Http IO Helper Library

Hi Abner,

Comments inline.

Thanks,
Maciej

On 29-Oct-20 07:11, Abner Chang wrote:

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Thursday, October 29, 2020 2:28 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3]
NetworkPkg/Library:
Implementation of Http IO Helper Library

Thanks for patching spelling problems in MdePkg header.
I will send more comments on v3. Added one here regarding chunk
transfer
security.

Thanks,
Maciej

On 26-Oct-20 07:53, Abner Chang wrote:
Thanks for reviewing this Maciej.

Most of comments are addressed. I also submitted a BZ for the wrong
spelling of CHUNK_TRNASFER_*, the patch is already sent.
V3 of this patch set is also sent to the mailing list.

Other feedbacks are in below,

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Saturday, October 24, 2020 12:25 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3]
NetworkPkg/Library:
Implementation of Http IO Helper Library

Hi Abner,

I understand the concept of moving HTTP I/O layer outside
HttpBootDxe
and
into a separate library and I approve it.
However, there are some things that have to
addressed/fixed/explained
here.
See comments within the patch.

On 20-Oct-20 04:38, Abner Chang wrote:
Add HTTP IO helper library which could be sued by HTTP applications
such as HTTP Boot, Redfish HTTP REST EX driver instance and etc.

Signed-off-by: Abner Chang <abner.chang@hpe.com>

Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Nickle Wang <nickle.wang@hpe.com>
---
NetworkPkg/Include/Library/HttpIoLib.h | 328 +++++++
.../Library/DxeHttpIoLib/DxeHttpIoLib.c | 809
++++++++++++++++++
.../Library/DxeHttpIoLib/DxeHttpIoLib.inf | 43 +
.../Library/DxeHttpIoLib/DxeHttpIoLib.uni | 13 +
4 files changed, 1193 insertions(+)
create mode 100644 NetworkPkg/Include/Library/HttpIoLib.h
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
diff --git a/NetworkPkg/Include/Library/HttpIoLib.h
b/NetworkPkg/Include/Library/HttpIoLib.h
new file mode 100644
index 0000000000..8f3804ca42
--- /dev/null
+++ b/NetworkPkg/Include/Library/HttpIoLib.h
@@ -0,0 +1,328 @@
+/** @file
+ HttpIoLib.h.
+
+(C) Copyright 2020 Hewlett-Packard Development Company,
L.P.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef HTTP_IO_LIB_H_
+#define HTTP_IO_LIB_H_
+
+#include <IndustryStandard/Http11.h>
+
+#include <Library/DpcLib.h>
+#include <Library/HttpLib.h>
+#include <Library/NetLib.h>
+
+#define HTTP_IO_MAX_SEND_PAYLOAD 1024
+#define HTTP_IO_CHUNK_SIZE_STRING_LEN 50
+#define HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH
256
+
+///
+/// HTTP_IO_CALLBACK_EVENT
+///
+typedef enum {
+ HttpIoRequest,
+ HttpIoResponse
+} HTTP_IO_CALLBACK_EVENT;
+
+/**
+ HttpIo Callback function which will be invoked when specified
HTTP_IO_CALLBACK_EVENT happened.
+
+ @param[in] EventType Indicate the Event type that occurs in
the
current callback.
+ @param[in] Message HTTP message which will be send to, or
just
received from HTTP server.
+ @param[in] Context The Callback Context pointer.
+
+ @retval EFI_SUCCESS Tells the HttpIo to continue the HTTP
process.
+ @retval Others Tells the HttpIo to abort the current HTTP
process.
+**/
+typedef
+EFI_STATUS
+(EFIAPI * HTTP_IO_CALLBACK) (
+ IN HTTP_IO_CALLBACK_EVENT EventType,
+ IN EFI_HTTP_MESSAGE *Message,
+ IN VOID *Context
+ );
+
+///
+/// A wrapper structure to hold the received HTTP response data.
+///
+typedef struct {
+ EFI_HTTP_RESPONSE_DATA Response;
+ UINTN HeaderCount;
+ EFI_HTTP_HEADER *Headers;
+ UINTN BodyLength;
+ CHAR8 *Body;
+ EFI_STATUS Status;
+} HTTP_IO_RESPONSE_DATA;
+
+///
+/// HTTP_IO configuration data for IPv4 /// typedef struct {
+ EFI_HTTP_VERSION HttpVersion;
+ UINT32 RequestTimeOut; ///< In milliseconds.
+ UINT32 ResponseTimeOut; ///< In milliseconds.
+ BOOLEAN UseDefaultAddress;
+ EFI_IPv4_ADDRESS LocalIp;
+ EFI_IPv4_ADDRESS SubnetMask;
+ UINT16 LocalPort;
+} HTTP4_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO configuration data for IPv6 /// typedef struct {
+ EFI_HTTP_VERSION HttpVersion;
+ UINT32 RequestTimeOut; ///< In milliseconds.
+ BOOLEAN UseDefaultAddress;
+ EFI_IPv6_ADDRESS LocalIp;
+ UINT16 LocalPort;
+} HTTP6_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO configuration
+///
+typedef union {
+ HTTP4_IO_CONFIG_DATA Config4;
+ HTTP6_IO_CONFIG_DATA Config6;
+} HTTP_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO wrapper of the EFI HTTP service.
+///
+typedef struct {
+ UINT8 IpVersion;
+ EFI_HANDLE Image;
+ EFI_HANDLE Controller;
+ EFI_HANDLE Handle;
+
+ EFI_HTTP_PROTOCOL *Http;
+
+ HTTP_IO_CALLBACK Callback;
+ VOID *Context;
+
+ EFI_HTTP_TOKEN ReqToken;
+ EFI_HTTP_MESSAGE ReqMessage;
+ EFI_HTTP_TOKEN RspToken;
+ EFI_HTTP_MESSAGE RspMessage;
+
+ BOOLEAN IsTxDone;
+ BOOLEAN IsRxDone;
+
+ EFI_EVENT TimeoutEvent;
+ UINT32 Timeout;
+} HTTP_IO;
+
+///
+/// Process code of HTTP chunk transfer.
+///
+typedef enum {
+ HttpIoSendChunkNone = 0,
+ HttpIoSendChunkHeaderZeroContent,
+ HttpIoSendChunkContent,
+ HttpIoSendChunkEndChunk,
+ HttpIoSendChunkFinish
+} HTTP_IO_SEND_CHUNK_PROCESS;
+
+///
+/// Process code of HTTP non chunk transfer.
+///
+typedef enum {
+ HttpIoSendNonChunkNone = 0,
+ HttpIoSendNonChunkHeaderZeroContent,
+ HttpIoSendNonChunkContent,
+ HttpIoSendNonChunkFinish
+} HTTP_IO_SEND_NON_CHUNK_PROCESS;
+
+///
+/// Chunk links for HTTP chunked transfer coding.
+///
+typedef struct {
+ LIST_ENTRY NextChunk;
+ UINTN Length;
+ CHAR8 *Data;
+} HTTP_IO_CHUNKS;
+
+/**
+ Notify the callback function when an event is triggered.
+
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotifyDpc (
+ IN VOID *Context
+ );
+
+/**
+ Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK.
+
+ @param[in] Event The event signaled.
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ );
+
+/**
+ Destroy the HTTP_IO and release the resources.
+
+ @param[in] HttpIo The HTTP_IO which wraps the HTTP service
to
be
destroyed.
+
+**/
+VOID
+HttpIoDestroyIo (
+ IN HTTP_IO *HttpIo
+ );
+
+/**
+ Create a HTTP_IO to access the HTTP service. It will create and
+configure
+ a HTTP child handle.
+
+ @param[in] Image The handle of the driver image.
+ @param[in] Controller The handle of the controller.
+ @param[in] IpVersion IP_VERSION_4 or IP_VERSION_6.
+ @param[in] ConfigData The HTTP_IO configuration data.
+ @param[in] Callback Callback function which will be invoked
when
specified
+ HTTP_IO_CALLBACK_EVENT happened.
+ @param[in] Context The Context data which will be passed to
the
Callback function.
+ @param[out] HttpIo The HTTP_IO.
+
+ @retval EFI_SUCCESS The HTTP_IO is created and configured.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_UNSUPPORTED One or more of the control options
are
not
+ supported in the implementation.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval Others Failed to create the HTTP_IO or configure it.
+
+**/
+EFI_STATUS
+HttpIoCreateIo (
+ IN EFI_HANDLE Image,
+ IN EFI_HANDLE Controller,
+ IN UINT8 IpVersion,
+ IN HTTP_IO_CONFIG_DATA *ConfigData,
+ IN HTTP_IO_CALLBACK Callback,
+ IN VOID *Context,
+ OUT HTTP_IO *HttpIo
+ );
+
+/**
+ Synchronously send a HTTP REQUEST message to the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] Request A pointer to storage such data as URL
and
HTTP method.
+ @param[in] HeaderCount Number of HTTP header structures in
Headers list.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[in] BodyLength Length in bytes of the HTTP body.
+ @param[in] Body Body associated with the HTTP request.
+
+ @retval EFI_SUCCESS The HTTP request is transmitted.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoSendRequest (
+ IN HTTP_IO *HttpIo,
+ IN EFI_HTTP_REQUEST_DATA *Request, OPTIONAL
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers, OPTIONAL
+ IN UINTN BodyLength,
+ IN VOID *Body OPTIONAL
+ );
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] RecvMsgHeader TRUE to receive a new HTTP
response
(from message header).
+ FALSE to continue receive the previous response
message.
+ @param[out] ResponseData Point to a wrapper of the received
response data.
+
+ @retval EFI_SUCCESS The HTTP response is received.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoRecvResponse (
+ IN HTTP_IO *HttpIo,
+ IN BOOLEAN RecvMsgHeader,
+ OUT HTTP_IO_RESPONSE_DATA *ResponseData
+ );
+
+/**
+ Get the value of the content length if there is a "Content-Length"
header.
+
+ @param[in] HeaderCount Number of HTTP header structures
in
Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ContentLength Pointer to save the value of the
content
length.
+
+ @retval EFI_SUCCESS Successfully get the content length.
+ @retval EFI_NOT_FOUND No "Content-Length" header in the
Headers.
+
+**/
+EFI_STATUS
+HttpIoGetContentLength (
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT UINTN *ContentLength
+ );
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] HeaderCount Number of headers in Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ChunkListHead A pointer to receivce list head of
chunked
data.
+ Caller has to release memory of ChunkListHead
+ and all list entries.
+ @param[out] ContentLength Total content length
+
+ @retval EFI_SUCCESS The HTTP chunked transfer is received.
+ @retval EFI_NOT_FOUND No chunked transfer coding header
found.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_INVALID_PARAMETER Improper parameters.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoGetChunkedTransferContent (
+ IN HTTP_IO *HttpIo,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT LIST_ENTRY **ChunkListHead,
+ OUT UINTN *ContentLength
+ );
+
+/**
+ Send HTTP request in chunks.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] SendChunkProcess Pointer to current chunk process
status.
+ @param[out] RequestMessage Request to send.
+
+ @retval EFI_SUCCESS Successfully to send chunk data
according
to
SendChunkProcess.
+ @retval Other Other errors.
+
+**/
+EFI_STATUS
+HttpIoSendChunkedTransfer (
+ IN HTTP_IO *HttpIo,
+ IN HTTP_IO_SEND_CHUNK_PROCESS *SendChunkProcess,
+ IN EFI_HTTP_MESSAGE *RequestMessage
+);
+#endif
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
new file mode 100644
index 0000000000..fbce86ea5c
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
@@ -0,0 +1,809 @@
+/** @file
+ Http IO Helper Library.
+
+ (C) Copyright 2020 Hewlett-Packard Development Company,
L.P.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent **/
+
+#include <Uefi.h>
+
+#include <Protocol/Http.h>
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HttpIoLib.h>
+#include <Library/MemoryAllocationLib.h> #include
+<Library/PrintLib.h> #include <Library/UefiBootServicesTableLib.h>
+
+/**
+ Notify the callback function when an event is triggered.
+
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotifyDpc (
+ IN VOID *Context
+ )
+{
+ *((BOOLEAN *) Context) = TRUE;
+}
+
+/**
+ Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK.
+
+ @param[in] Event The event signaled.
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ //
+ // Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK
+ //
+ QueueDpc (TPL_CALLBACK, HttpIoNotifyDpc, Context); }
+
+/**
+ Destroy the HTTP_IO and release the resources.
+
+ @param[in] HttpIo The HTTP_IO which wraps the HTTP service
to
be
destroyed.
+
+**/
+VOID
+HttpIoDestroyIo (
+ IN HTTP_IO *HttpIo
+ )
+{
+ EFI_HTTP_PROTOCOL *Http;
+ EFI_EVENT Event;
+
+ if (HttpIo == NULL) {
+ return;
+ }
+
+ Event = HttpIo->ReqToken.Event;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Event = HttpIo->RspToken.Event;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Event = HttpIo->TimeoutEvent;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Http = HttpIo->Http;
+ if (Http != NULL) {
+ Http->Configure (Http, NULL);
+ gBS->CloseProtocol (
+ HttpIo->Handle,
+ &gEfiHttpProtocolGuid,
+ HttpIo->Image,
+ HttpIo->Controller
+ );
+ }
+
+ NetLibDestroyServiceChild (
+ HttpIo->Controller,
+ HttpIo->Image,
+ &gEfiHttpServiceBindingProtocolGuid,
+ HttpIo->Handle
+ );
+}
+
+/**
+ Create a HTTP_IO to access the HTTP service. It will create and
+configure
+ a HTTP child handle.
+
+ @param[in] Image The handle of the driver image.
+ @param[in] Controller The handle of the controller.
+ @param[in] IpVersion IP_VERSION_4 or IP_VERSION_6.
+ @param[in] ConfigData The HTTP_IO configuration data ,
+ NULL means not to configure the HTTP child.
+ @param[in] Callback Callback function which will be invoked
when
specified
+ HTTP_IO_CALLBACK_EVENT happened.
+ @param[in] Context The Context data which will be passed to
the
Callback function.
+ @param[out] HttpIo The HTTP_IO.
+
+ @retval EFI_SUCCESS The HTTP_IO is created and configured.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_UNSUPPORTED One or more of the control options
are
not
+ supported in the implementation.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval Others Failed to create the HTTP_IO or configure it.
+
+**/
+EFI_STATUS
+HttpIoCreateIo (
+ IN EFI_HANDLE Image,
+ IN EFI_HANDLE Controller,
+ IN UINT8 IpVersion,
+ IN HTTP_IO_CONFIG_DATA *ConfigData, OPTIONAL
+ IN HTTP_IO_CALLBACK Callback,
+ IN VOID *Context,
+ OUT HTTP_IO *HttpIo
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_CONFIG_DATA HttpConfigData;
+ EFI_HTTPv4_ACCESS_POINT Http4AccessPoint;
+ EFI_HTTPv6_ACCESS_POINT Http6AccessPoint;
+ EFI_HTTP_PROTOCOL *Http;
+ EFI_EVENT Event;
+
+ if ((Image == NULL) || (Controller == NULL) || (HttpIo == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (IpVersion != IP_VERSION_4 && IpVersion != IP_VERSION_6) {
+ return EFI_UNSUPPORTED;
+ }
+
+ ZeroMem (HttpIo, sizeof (HTTP_IO)); ZeroMem (&HttpConfigData,
+ sizeof (EFI_HTTP_CONFIG_DATA));
+
+ //
+ // Create the HTTP child instance and get the HTTP protocol.
+ //
+ Status = NetLibCreateServiceChild (
+ Controller,
+ Image,
+ &gEfiHttpServiceBindingProtocolGuid,
+ &HttpIo->Handle
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = gBS->OpenProtocol (
+ HttpIo->Handle,
+ &gEfiHttpProtocolGuid,
+ (VOID **) &Http,
+ Image,
+ Controller,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status) || (Http == NULL)) {
+ goto ON_ERROR;
+ }
+
+ //
+ // Init the configuration data and configure the HTTP child.
+ //
+ HttpIo->Image = Image;
+ HttpIo->Controller = Controller;
+ HttpIo->IpVersion = IpVersion;
+ HttpIo->Http = Http;
+ HttpIo->Callback = Callback;
+ HttpIo->Context = Context;
+
+ if (ConfigData != NULL) {
+ if (HttpIo->IpVersion == IP_VERSION_4) {
+ HttpConfigData.LocalAddressIsIPv6 = FALSE;
+ HttpConfigData.HttpVersion = ConfigData-
Config4.HttpVersion;
+ HttpConfigData.TimeOutMillisec = ConfigData-
Config4.RequestTimeOut;
+
+ Http4AccessPoint.UseDefaultAddress = ConfigData-
Config4.UseDefaultAddress;
+ Http4AccessPoint.LocalPort = ConfigData->Config4.LocalPort;
+ IP4_COPY_ADDRESS (&Http4AccessPoint.LocalAddress,
&ConfigData-
Config4.LocalIp);
+ IP4_COPY_ADDRESS (&Http4AccessPoint.LocalSubnet,
&ConfigData-
Config4.SubnetMask);
+ HttpConfigData.AccessPoint.IPv4Node = &Http4AccessPoint;
+ } else {
+ HttpConfigData.LocalAddressIsIPv6 = TRUE;
+ HttpConfigData.HttpVersion = ConfigData-
Config6.HttpVersion;
+ HttpConfigData.TimeOutMillisec = ConfigData-
Config6.RequestTimeOut;
+
+ Http6AccessPoint.LocalPort = ConfigData->Config6.LocalPort;
+ IP6_COPY_ADDRESS (&Http6AccessPoint.LocalAddress,
&ConfigData-
Config6.LocalIp);
+ HttpConfigData.AccessPoint.IPv6Node = &Http6AccessPoint;
+ }
+
+ Status = Http->Configure (Http, &HttpConfigData);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ }
+
+ //
+ // Create events for variuos asynchronous operations.
+ //
+ Status = gBS->CreateEvent (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ HttpIoNotify,
+ &HttpIo->IsTxDone,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->ReqToken.Event = Event;
+ HttpIo->ReqToken.Message = &HttpIo->ReqMessage;
+
+ Status = gBS->CreateEvent (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ HttpIoNotify,
+ &HttpIo->IsRxDone,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->RspToken.Event = Event;
+ HttpIo->RspToken.Message = &HttpIo->RspMessage;
+
+ //
+ // Create TimeoutEvent for response // Status = gBS->CreateEvent
+ (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->TimeoutEvent = Event;
+ return EFI_SUCCESS;
+
+ON_ERROR:
+ HttpIoDestroyIo (HttpIo);
+
+ return Status;
+}
+
+/**
+ Synchronously send a HTTP REQUEST message to the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] Request A pointer to storage such data as URL
and
HTTP method.
+ @param[in] HeaderCount Number of HTTP header structures in
Headers list.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[in] BodyLength Length in bytes of the HTTP body.
+ @param[in] Body Body associated with the HTTP request.
+
+ @retval EFI_SUCCESS The HTTP request is trasmitted.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoSendRequest (
+ IN HTTP_IO *HttpIo,
+ IN EFI_HTTP_REQUEST_DATA *Request,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ IN UINTN BodyLength,
+ IN VOID *Body
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_PROTOCOL *Http;
+
+ if (HttpIo == NULL || HttpIo->Http == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ HttpIo->ReqToken.Status = EFI_NOT_READY;
+ HttpIo->ReqToken.Message->Data.Request = Request;
+ HttpIo->ReqToken.Message->HeaderCount = HeaderCount;
+ HttpIo->ReqToken.Message->Headers = Headers;
+ HttpIo->ReqToken.Message->BodyLength = BodyLength;
+ HttpIo->ReqToken.Message->Body = Body;
+
+ if (HttpIo->Callback != NULL) {
+ Status = HttpIo->Callback (
+ HttpIoRequest,
+ HttpIo->ReqToken.Message,
+ HttpIo->Context
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ //
+ // Queue the request token to HTTP instances.
+ //
+ Http = HttpIo->Http;
+ HttpIo->IsTxDone = FALSE;
+ Status = Http->Request (
+ Http,
+ &HttpIo->ReqToken
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Poll the network until transmit finish.
+ //
+ while (!HttpIo->IsTxDone) {
+ Http->Poll (Http);
+ }
+
+ return HttpIo->ReqToken.Status;
+}
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] RecvMsgHeader TRUE to receive a new HTTP
response
(from message header).
+ FALSE to continue receive the previous response
message.
+ @param[out] ResponseData Point to a wrapper of the received
response data.
+
+ @retval EFI_SUCCESS The HTTP response is received.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoRecvResponse (
+ IN HTTP_IO *HttpIo,
+ IN BOOLEAN RecvMsgHeader,
+ OUT HTTP_IO_RESPONSE_DATA *ResponseData
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_PROTOCOL *Http;
+
+ if (HttpIo == NULL || HttpIo->Http == NULL || ResponseData ==
NULL)
{
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Create new TimeoutEvent for response // if
+ (HttpIo->TimeoutEvent != NULL) {
+ gBS->CloseEvent (HttpIo->TimeoutEvent);
+ HttpIo->TimeoutEvent = NULL;
+ }
+
+ Status = gBS->CreateEvent (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &HttpIo->TimeoutEvent
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Is it necessary to recreate the event? Wouldn't this be enough to reset
it?
No, we don’t have to recreate this event.
gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
I made some changes in this function.
+
+ //
+ // Start the timer, and wait Timeout seconds to receive the header
packet.
+ //
+ Status = gBS->SetTimer (HttpIo->TimeoutEvent, TimerRelative,
+ HttpIo->Timeout * TICKS_PER_MS); if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Queue the response token to HTTP instances.
+ //
+ HttpIo->RspToken.Status = EFI_NOT_READY; if (RecvMsgHeader) {
+ HttpIo->RspToken.Message->Data.Response =
+ &ResponseData->Response; } else {
+ HttpIo->RspToken.Message->Data.Response = NULL; }
+ HttpIo->RspToken.Message->HeaderCount = 0;
+ HttpIo->RspToken.Message->Headers = NULL;
+ HttpIo->RspToken.Message->BodyLength = ResponseData-
BodyLength;
+ HttpIo->RspToken.Message->Body = ResponseData->Body;
+
+ Http = HttpIo->Http;
+ HttpIo->IsRxDone = FALSE;
+ Status = Http->Response (
+ Http,
+ &HttpIo->RspToken
+ );
+
+ if (EFI_ERROR (Status)) {
+ gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
+ return Status;
+ }
+
+ //
+ // Poll the network until receive finish.
+ //
+ while (!HttpIo->IsRxDone && ((HttpIo->TimeoutEvent == NULL) ||
EFI_ERROR (gBS->CheckEvent (HttpIo->TimeoutEvent)))) {
+ Http->Poll (Http);
+ }
+
+ gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
+
+ if (!HttpIo->IsRxDone) {
+ //
+ // Timeout occurs, cancel the response token.
+ //
+ Http->Cancel (Http, &HttpIo->RspToken);
+
+ Status = EFI_TIMEOUT;
+
+ return Status;
+ } else {
+ HttpIo->IsRxDone = FALSE;
+ }
+
+ if ((HttpIo->Callback != NULL) &&
+ (HttpIo->RspToken.Status == EFI_SUCCESS || HttpIo-
RspToken.Status == EFI_HTTP_ERROR)) {
+ Status = HttpIo->Callback (
+ HttpIoResponse,
+ HttpIo->RspToken.Message,
+ HttpIo->Context
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ //
+ // Store the received data into the wrapper.
+ //
+ ResponseData->Status = HttpIo->RspToken.Status;
+ ResponseData->HeaderCount = HttpIo->RspToken.Message-
HeaderCount;
+ ResponseData->Headers = HttpIo->RspToken.Message->Headers;
+ ResponseData->BodyLength = HttpIo->RspToken.Message-
BodyLength;
+
+ return Status;
+}
+
+/**
+ Get the value of the content length if there is a "Content-Length"
header.
+
+ @param[in] HeaderCount Number of HTTP header structures
in
Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ContentLength Pointer to save the value of the
content
length.
+
+ @retval EFI_SUCCESS Successfully get the content length.
+ @retval EFI_NOT_FOUND No "Content-Length" header in the
Headers.
+
+**/
+EFI_STATUS
+HttpIoGetContentLength (
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT UINTN *ContentLength
+ )
+{
+ EFI_HTTP_HEADER *Header;
+
+ Header = HttpFindHeader (HeaderCount, Headers,
+ HTTP_HEADER_CONTENT_LENGTH); if (Header == NULL) {
+ return EFI_NOT_FOUND;
+ }
+
+ return AsciiStrDecimalToUintnS (Header->FieldValue, (CHAR8 **)
+NULL, ContentLength); }
+/**
+ Send HTTP request in chunks.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] SendChunkProcess Pointer to current chunk process
status.
+ @param[in] RequestMessage Request to send.
+
+ @retval EFI_SUCCESS Successfully to send chunk data
according
to
SendChunkProcess.
+ @retval Other Other errors.
+
+**/
+EFI_STATUS
+HttpIoSendChunkedTransfer (
+ IN HTTP_IO *HttpIo,
+ IN HTTP_IO_SEND_CHUNK_PROCESS *SendChunkProcess,
+ IN EFI_HTTP_MESSAGE *RequestMessage
+)
+{
+ EFI_STATUS Status;
+ EFI_HTTP_HEADER *NewHeaders;
+ EFI_HTTP_HEADER *ContentLengthHeader;
+ UINTN AddNewHeader;
+ UINTN HeaderCount;
+ CHAR8 *MessageBody;
+ UINTN MessageBodyLength;
+ CHAR8 ChunkLengthStr [HTTP_IO_CHUNK_SIZE_STRING_LEN];
+ EFI_HTTP_REQUEST_DATA *SentRequestData;
This is a new function and I already see a difference in the coding
standard.
Please align variable names like in previous functions.
Done

+
+ AddNewHeader = 0;
+ NewHeaders = NULL;
+ MessageBody = NULL;
+ ContentLengthHeader = NULL;
+ MessageBodyLength = 0;
Alignment of assignments.
Done

+
+ switch (*SendChunkProcess) {
+ case HttpIoSendChunkHeaderZeroContent:
Case spacing: 2 spaces. Applies throughout new code.
+ ContentLengthHeader =
+ HttpFindHeader(RequestMessage->HeaderCount,
RequestMessage-
Headers,
+ HTTP_HEADER_CONTENT_LENGTH);
Spacing after function call name. This applies from this point until the
end
of
patch.
Done
+ if (ContentLengthHeader == NULL) {
+ AddNewHeader = 1;
+ }
+
+ NewHeaders = AllocateZeroPool((RequestMessage-
HeaderCount
+
+ AddNewHeader) * sizeof(EFI_HTTP_HEADER));
Lack of NULL pointer test for NewHeaders (out of memory case).
+ CopyMem ((VOID*)NewHeaders, (VOID *)RequestMessage-
Headers,
RequestMessage->HeaderCount * sizeof (EFI_HTTP_HEADER));
+ if (AddNewHeader == 0) {
+ //
+ // Override content-length to Transfer-Encoding.
+ //
+ ContentLengthHeader = HttpFindHeader (RequestMessage-
HeaderCount, NewHeaders, HTTP_HEADER_CONTENT_LENGTH);
+ ContentLengthHeader->FieldName = NULL;
+ ContentLengthHeader->FieldValue = NULL;
+ } else {
+ ContentLengthHeader = NewHeaders + RequestMessage-
HeaderCount;
+ }
+ HttpSetFieldNameAndValue(ContentLengthHeader,
HTTP_HEADER_TRANSFER_ENCODING,
HTTP_HEADER_TRANSFER_ENCODING_CHUNKED);
+ HeaderCount = RequestMessage->HeaderCount +
AddNewHeader;
Move HeaderCount assignment before NewHeaders allocation. Use
HeaderCount as argument for NewHeaders allocation.
+ MessageBodyLength = 0;
+ MessageBody = NULL;
+ SentRequestData = RequestMessage->Data.Request;
+ break;
+
+ case HttpIoSendChunkContent:
+ HeaderCount = 0;
+ NewHeaders = NULL;
+ SentRequestData = NULL;
+ if (RequestMessage->BodyLength >
HTTP_IO_MAX_SEND_PAYLOAD) {
+ MessageBodyLength = HTTP_IO_MAX_SEND_PAYLOAD;
+ } else {
+ MessageBodyLength = RequestMessage->BodyLength;
+ }
+ AsciiSPrint (ChunkLengthStr, HTTP_IO_CHUNK_SIZE_STRING_LEN,
+ "%x%c%c", MessageBodyLength,
CHUNKED_TRNASFER_CODING_CR,
+ CHUNKED_TRNASFER_CODING_LF);
Line too long, break down in accordance to coding standard.
Done

+ MessageBody = AllocatePool (AsciiStrLen (ChunkLengthStr) +
MessageBodyLength + 2);
+ if (MessageBody == NULL) {
+ DEBUG((DEBUG_ERROR, "Not enough memory for chunk
transfer\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (MessageBody, ChunkLengthStr, AsciiStrLen
(ChunkLengthStr));
+ CopyMem (MessageBody + AsciiStrLen (ChunkLengthStr),
RequestMessage->Body, MessageBodyLength);
+ *(MessageBody + AsciiStrLen (ChunkLengthStr) +
MessageBodyLength)
= CHUNKED_TRNASFER_CODING_CR;
+ *(MessageBody + AsciiStrLen (ChunkLengthStr) +
MessageBodyLength
+ 1) = CHUNKED_TRNASFER_CODING_LF;
+ RequestMessage->BodyLength -= MessageBodyLength;
+ RequestMessage->Body = (VOID *)((CHAR8 *)RequestMessage-
Body + MessageBodyLength);
+ MessageBodyLength += (AsciiStrLen (ChunkLengthStr) + 2);
This block could use some spacing and comments to improve
readability.
Done

+ if (RequestMessage->BodyLength == 0) {
+ *SendChunkProcess = HttpIoSendChunkEndChunk;
+ }
+ break;
+
+ case HttpIoSendChunkEndChunk:
+ HeaderCount = 0;
+ NewHeaders = NULL;
+ SentRequestData = NULL;
+ AsciiSPrint (ChunkLengthStr, HTTP_IO_CHUNK_SIZE_STRING_LEN,
"0%c%c%c%c",
+ CHUNKED_TRNASFER_CODING_CR,
CHUNKED_TRNASFER_CODING_LF,
+ CHUNKED_TRNASFER_CODING_CR,
CHUNKED_TRNASFER_CODING_LF
+ );
Please break function calls in accordance with coding standard.
Done
+ MessageBody = AllocatePool (AsciiStrLen(ChunkLengthStr));
+ if (MessageBody == NULL) {
+ DEBUG((DEBUG_ERROR, "Not enough memory for the end
chunk
transfer\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (MessageBody, ChunkLengthStr, AsciiStrLen
(ChunkLengthStr));
+ MessageBodyLength = AsciiStrLen(ChunkLengthStr);
+ *SendChunkProcess = HttpIoSendChunkFinish;
+ break;
+
+ default:
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = HttpIoSendRequest(
+ HttpIo,
+ SentRequestData,
+ HeaderCount,
+ NewHeaders,
+ MessageBodyLength,
+ MessageBody
+ );
+ if (ContentLengthHeader != NULL) {
+ if (ContentLengthHeader->FieldName != NULL) {
+ FreePool(ContentLengthHeader->FieldName);
+ }
+ if (ContentLengthHeader->FieldValue != NULL) {
+ FreePool(ContentLengthHeader->FieldValue);
+ }
+ ContentLengthHeader = NULL;
NULL assignments are not necessary here as we are exiting the
function
and
those variables will not be used.
Right, fixed.
+ }
+ if (NewHeaders != NULL) {
+ FreePool(NewHeaders);
+ NewHeaders = NULL;
+ }
+ if (MessageBody != NULL) {
+ FreePool(MessageBody);
+ MessageBody = NULL;
+ }
+ return Status;
+}
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] HeaderCount Number of headers in Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ChunkListHead A pointer to receivce list head of
chunked
data.
Spelling: receive.
Fixed.

+ Caller has to release memory of ChunkListHead
+ and all list entries.
+ @param[out] ContentLength Total content length
+
+ @retval EFI_SUCCESS The HTTP chunked transfer is received.
+ @retval EFI_NOT_FOUND No chunked transfer coding header
found.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_INVALID_PARAMETER Improper parameters.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoGetChunkedTransferContent (
+ IN HTTP_IO *HttpIo,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT LIST_ENTRY **ChunkListHead,
+ OUT UINTN *ContentLength
+ )
+{
+ EFI_HTTP_HEADER *Header;
+ CHAR8 ChunkSizeAscii[256];
+ EFI_STATUS Status;
+ UINTN Index;
+ HTTP_IO_RESPONSE_DATA ResponseData;
+ UINTN TotalLength;
+ LIST_ENTRY *HttpChunks;
+ HTTP_IO_CHUNKS *ThisChunk;
Variable alignment.
+ LIST_ENTRY *ThisListEntry;
+
+ if (ChunkListHead == NULL || ContentLength == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *ContentLength = 0;
+ Header = HttpFindHeader (HeaderCount, Headers,
+ HTTP_HEADER_TRANSFER_ENCODING); if (Header == NULL) {
+ return EFI_NOT_FOUND;
+ }
+ if (AsciiStrCmp (Header->FieldValue,
+ HTTP_HEADER_TRANSFER_ENCODING_CHUNKED) == 0) {
Please do a negation comparison instead and return. It reduces one
level
of
indentation.
Done
+ //
+ // Loop to get all chunks.
+ //
Please avoid empty single-line comments when starting and ending
comment block.
+ TotalLength = 0;
+ HttpChunks = (LIST_ENTRY *)AllocateZeroPool (sizeof
(LIST_ENTRY));
+ if (HttpChunks == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (HttpChunks);
Add a bit of spacing above to improve readability.
Fixed
+ DEBUG ((DEBUG_INFO, " Chunked transfer\n"));
+ while (TRUE) {
+ ZeroMem((VOID *)&ResponseData,
sizeof(HTTP_IO_RESPONSE_DATA));
+ ResponseData.BodyLength =
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH;
+ ResponseData.Body = ChunkSizeAscii;
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Returning here creates memory leak risk - chunks previously received
will
not
be freed.
Fixed
+ //
+ // Decoding Chunked Transfer Coding.
+ // Only decode chunk-size and last chunk.
+ //
+ DEBUG ((DEBUG_INFO, " Chunk HTTP Response StatusCode -
%d\n",
ResponseData.Response.StatusCode));
+ //
+ // Break if this is last chunk.
+ //
+ if (ChunkSizeAscii [0] ==
CHUNKED_TRNASFER_CODING_LAST_CHUNK)
{
+ Status = EFI_SUCCESS;
+ DEBUG ((DEBUG_INFO, " Last chunk\n"));
+ ThisChunk = (HTTP_IO_CHUNKS *)AllocateZeroPool (sizeof
(HTTP_IO_CHUNKS));
+ if (ThisChunk == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (&ThisChunk->NextChunk);
+ ThisChunk->Length = ResponseData.BodyLength - 1 - 2; // Minus
sizeof '0' and CRLF.
+ ThisChunk->Data = (CHAR8 *)AllocatePool (ThisChunk->Length);
+ if (ThisChunk->Data == NULL) {
+ FreePool ((UINT8 *)ThisChunk);
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ CopyMem ((UINT8 *)ThisChunk->Data, (UINT8
*)ResponseData.Body
+ 1, ThisChunk->Length);
+ TotalLength += ThisChunk->Length;
+ InsertTailList (HttpChunks, &ThisChunk->NextChunk);
+ break;
+ }
+
+ //
+ // Get the chunk length
+ //
+ Index = 0;
+ while ((ChunkSizeAscii [Index] !=
CHUNKED_TRNASFER_CODING_EXTENSION_SAPERATOR) &&
+ (ChunkSizeAscii [Index] !=
+ (CHAR8)CHUNKED_TRNASFER_CODING_CR) &&
Spelling errors in macros above.
Oops. BZ is submitted for this typo in http11.h.
INVALID URI REMOVED
3A__bugzilla.tianocore.org_show-5Fbug.cgi-3Fid-
3D3019&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulks
kz6qU3NYRO03nHp9P7Z5q59A3E&m=f6zs_mrzwCCYwQSl3p4Nbuib7Ln4gVcO
KS74o-Xc524&s=g1WHLCl8Uzm8YKLYknM2Y7T0d4gT7fv_58qhc4Qejj0&e=
Patch is set to the mailing list for review.


+ (Index !=
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH)) {
+ Index ++;
+ };
+ if (Index ==
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH)
{
+ return EFI_NOT_FOUND;
+ }
Again - memory leak.
Fixed

+ ChunkSizeAscii[Index] = 0;
+ AsciiStrHexToUintnS (ChunkSizeAscii, NULL, ContentLength);
+ DEBUG ((DEBUG_INFO, " Length of this chunk %d\n",
*ContentLength));
+ //
+ // Receive the data;
+ //
+ ThisChunk = (HTTP_IO_CHUNKS *)AllocateZeroPool (sizeof
(HTTP_IO_CHUNKS));
+ if (ThisChunk == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ ResponseData.BodyLength = *ContentLength;
+ ResponseData.Body = (CHAR8 *)AllocatePool (*ContentLength);
+ if (ResponseData.Body == NULL) {
+ FreePool (ThisChunk);
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (&ThisChunk->NextChunk);
+ ThisChunk->Length = *ContentLength;
+ ThisChunk->Data = ResponseData.Body;
+ InsertTailList (HttpChunks, &ThisChunk->NextChunk);
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ goto DeleterChunks;
+ }
+ //
+ // Read CRLF
+ //
+ ZeroMem((VOID *)&ResponseData,
sizeof(HTTP_IO_RESPONSE_DATA));
+ ResponseData.BodyLength = 2;
+ ResponseData.Body = ChunkSizeAscii;
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ goto DeleterChunks;
+ }
+
+ TotalLength += *ContentLength;
The following code requires some kind of protection against receiving
an
infinite number of chunks.
Currently I could just not send a last chunk and wait for the platform to
crash
or/and run out of memory :)
Good point! However, I can't think of any good way to prevent from this
happens. This depends on the reliability of content provider and how the
web service handles chunk transfer, right?
From system firmware viewpoint, we would connect to a trusted
content
source for either HTTP boot image and Redfish service resource.
Do you think this concern is valid for the real practice?

Or give it a limited size of payload? Like the size larger than 1G could be
considered as an ill content?
What would be the largest payload that you would like to use
chunk-transfer for?
For the HTTP response, whether to use chunk transfer is at the discretion
of Redfish service. We currently can only tell if the response payload is in
chunk transfer or not using Transfer-Encoding HTTP response header but not
able to predict the size of payload unless we get the first chunk.
In the real practice, I see a large payload like AttributeRegistry is
transferred back in chunk, however, I also see some small size payload is
transferred back in chunk transfer as well.
I see that the chunked transfer is not being currently used in HTTP
Boot, so we should stick to current use case -  Redfish.
Yes, currently only RedfishPkg use HTTP chunk transfer APIs defined in
DxeHttpIoLib. But what do you mean to stick the use case to Redfish only?
Those chunk transfer related APIs could be commonly used by any edk2
HTTP clients need chunk transfer although we only have two use cases (and
only Redfish uses it so far).
I only meant that it would be reasonable to come up with a
chunk-transfer limit that is dictated solely by Redfish use case, since
it is currently the only one around.
In case other SW pieces would like to consume HTTP I/O library for
chunk-transfer and see that the limit is too small, they could propose a
patch to extend it.
Yes, we can do it.
The other approach would be to somehow dynamically assess the limit by
looking at how much free memory is available on the platform.
Can we estimate what would be the largest payload Redfish would like to
push through?
The biggest payload of Redfish resource which sent back to client could be AttaibuteRegistry. One example here is the raw text JSON file of AttaibuteRegistry is 1.2MB, however it is reduced to ~65K using gzip. How big the AttaibuteRegistry payload depends on the platform and also depends on if Redfish service sends back the payload using gzip.
We can just limit it to 10MB for now, that says we break the chunk transfer if TotalLength > 10MB. We can adjust it later if any uses case request it. How do you think?


+ };
+ *ContentLength = TotalLength;
+ *ChunkListHead = HttpChunks;
+ DEBUG ((DEBUG_INFO, " Total of lengh of chunks :%d\n",
TotalLength));
+ return EFI_SUCCESS;
+ } else {
+ return EFI_NOT_FOUND;
+ }
+DeleterChunks:;
Name this label ExitDeleteChunks and remove the comma.
+ while (!IsListEmpty (HttpChunks)) {
+ ThisListEntry = GetFirstNode (HttpChunks);
+ RemoveEntryList (ThisListEntry);
+ };
+ return Status;
+}
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
new file mode 100644
index 0000000000..a02b409547
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
@@ -0,0 +1,43 @@
+## @file
+# This library instance provides HTTP IO helper functions.
+#
+# (C) Copyright 2020 Hewlett Packard Enterprise Development
LP<BR>
#
+SPDX-License-Identifier: BSD-2-Clause-Patent # ##
+
+[Defines]
+ INF_VERSION = 0x0001001b
+ BASE_NAME = DxeHttpIoLib
+ MODULE_UNI_FILE = DxeHttpIoLib.uni
+ FILE_GUID = 50B198F8-7986-4F51-A857-CFE4643D59F3
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = HttpIoLib| DXE_CORE DXE_DRIVER
DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION
UEFI_DRIVER
+
+#
+# The following information is for reference only and not required by
the
build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARM64
RISCV64
+#
+
+[Sources]
+ DxeHttpIoLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ NetworkPkg/NetworkPkg.dec
+
+
+[LibraryClasses]
+ BaseLib
+ BaseMemoryLib
+ DebugLib
+ DpcLib
+ MemoryAllocationLib
+ PrintLib
+ UefiBootServicesTableLib
+
+[Protocols]
+ gEfiHttpProtocolGuid ## SOMETIMES_CONSUMES
+
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
new file mode 100644
index 0000000000..d419b1a49d
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
@@ -0,0 +1,13 @@
+// /** @file
+// The library instance provides HTTP IO helper functions.
+//
+// (C) Copyright 2020 Hewlett Packard Enterprise Development
LP<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
+
+
+#string STR_MODULE_ABSTRACT #language en-US "HTTP IO
Helper
Library"
+
+#string STR_MODULE_DESCRIPTION #language en-US "The
library
instance provides HTTP IO helper functions."
+






Re: [PATCH v3 1/1] Silicon/Qemu/Sbsa: Add SBSA-wdt entry to GTDT

Graeme Gregory <graeme@...>
 

On Thu, Oct 29, 2020 at 04:21:21PM -0400, Shashi Mallela wrote:
SBSA generic watchdog timer structure entry has been added
to GTDT table as per SBSAv6.0.
This enables acpi detection of wdt in qemu sbsa platform
This version seems has reverteed to v1 re-indenting the whole file?

Please base update on v2 where you did not do this.

Thanks

Graeme

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Graeme Gregory <graeme@nuviainc.com>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc | 87 ++++++++++++++------
1 file changed, 62 insertions(+), 25 deletions(-)

diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
index d16778e01a5c..1713c203766e 100644
--- a/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
+++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Gtdt.aslc
@@ -24,42 +24,79 @@
#define SYSTEM_TIMER_BASE_ADDRESS 0xFFFFFFFFFFFFFFFF
#endif

-#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_TIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_MODE
#define GTDT_TIMER_LEVEL_TRIGGERED 0
-#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_5_0_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_TIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_TIMER_FLAG_TIMER_INTERRUPT_POLARITY
#define GTDT_TIMER_ACTIVE_HIGH 0

#define GTDT_GTIMER_FLAGS (GTDT_TIMER_ACTIVE_LOW | GTDT_TIMER_LEVEL_TRIGGERED)

+#define SBSA_PLATFORM_WATCHDOG_COUNT 1
+#define SBSA_PLATFORM_TIMER_COUNT (SBSA_PLATFORM_WATCHDOG_COUNT)
+
+#define SBSAQEMU_WDT_REFRESH_FRAME_BASE 0x50010000
+#define SBSAQEMU_WDT_CONTROL_FRAME_BASE 0x50011000
+#define SBSAQEMU_WDT_IRQ 44
+
+#define GTDT_WDTIMER_EDGE_TRIGGERED EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_MODE
+#define GTDT_WDTIMER_LEVEL_TRIGGERED 0
+#define GTDT_WDTIMER_ACTIVE_LOW EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_FLAG_TIMER_INTERRUPT_POLARITY
+#define GTDT_WDTIMER_ACTIVE_HIGH 0
+
+#define GTDT_WDTIMER_FLAGS (GTDT_WDTIMER_ACTIVE_HIGH | GTDT_WDTIMER_LEVEL_TRIGGERED)
+
+#define EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT( \
+ RefreshFramePhysicalAddress, ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, WatchdogTimerFlags) \
+ { \
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG, \
+ sizeof(EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE), \
+ EFI_ACPI_RESERVED_WORD, \
+ RefreshFramePhysicalAddress, \
+ ControlFramePhysicalAddress, \
+ WatchdogTimerGSIV, \
+ WatchdogTimerFlags \
+ }
+
#pragma pack (1)

typedef struct {
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
- } GENERIC_TIMER_DESCRIPTION_TABLE;
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE Gtdt;
+ EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG_STRUCTURE Gwdt;
+ } GENERIC_TIMER_DESCRIPTION_TABLES;

#pragma pack ()

- GENERIC_TIMER_DESCRIPTION_TABLE Gtdt = {
- {
- SBSAQEMU_ACPI_HEADER(
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
- GENERIC_TIMER_DESCRIPTION_TABLE,
- EFI_ACPI_5_1_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
- ),
- SYSTEM_TIMER_BASE_ADDRESS, // UINT64 PhysicalAddress
- 0, // UINT32 Reserved
- FixedPcdGet32 (PcdArmArchTimerSecIntrNum), // UINT32 SecurePL1TimerGSIV
- GTDT_GTIMER_FLAGS, // UINT32 SecurePL1TimerFlags
- FixedPcdGet32 (PcdArmArchTimerIntrNum), // UINT32 NonSecurePL1TimerGSIV
- GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL1TimerFlags
- FixedPcdGet32 (PcdArmArchTimerVirtIntrNum), // UINT32 VirtualTimerGSIV
- GTDT_GTIMER_FLAGS, // UINT32 VirtualTimerFlags
- FixedPcdGet32 (PcdArmArchTimerHypIntrNum), // UINT32 NonSecurePL2TimerGSIV
- GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL2TimerFlags
- 0xFFFFFFFFFFFFFFFF, // UINT64 CntReadBasePhysicalAddress
- 0, // UINT32 PlatformTimerCount
- 0
- },
+ GENERIC_TIMER_DESCRIPTION_TABLES Gtdt = {
+ {
+ SBSAQEMU_ACPI_HEADER(
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
+ GENERIC_TIMER_DESCRIPTION_TABLES,
+ EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION
+ ),
+ SYSTEM_TIMER_BASE_ADDRESS, // UINT64 PhysicalAddress
+ 0, // UINT32 Reserved
+ FixedPcdGet32 (PcdArmArchTimerSecIntrNum), // UINT32 SecurePL1TimerGSIV
+ GTDT_GTIMER_FLAGS, // UINT32 SecurePL1TimerFlags
+ FixedPcdGet32 (PcdArmArchTimerIntrNum), // UINT32 NonSecurePL1TimerGSIV
+ GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL1TimerFlags
+ FixedPcdGet32 (PcdArmArchTimerVirtIntrNum), // UINT32 VirtualTimerGSIV
+ GTDT_GTIMER_FLAGS, // UINT32 VirtualTimerFlags
+ FixedPcdGet32 (PcdArmArchTimerHypIntrNum), // UINT32 NonSecurePL2TimerGSIV
+ GTDT_GTIMER_FLAGS, // UINT32 NonSecurePL2TimerFlags
+ 0xFFFFFFFFFFFFFFFF, // UINT64 CntReadBasePhysicalAddress
+ SBSA_PLATFORM_TIMER_COUNT, // UINT32 PlatformTimerCount
+ sizeof(EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE),
+ // UINT32 PlatformTimerOffset
+ 0, // UINT32 VirtualPL2TimerGSIV
+ 0 // UINT32 VirtualPL2TimerFlags
+ },
+ EFI_ACPI_6_3_SBSA_GENERIC_WATCHDOG_STRUCTURE_INIT(
+ SBSAQEMU_WDT_REFRESH_FRAME_BASE,
+ SBSAQEMU_WDT_CONTROL_FRAME_BASE,
+ SBSAQEMU_WDT_IRQ,
+ GTDT_WDTIMER_FLAGS
+ )
};

// Reference the table being generated to prevent the optimizer from removing the
--
2.18.4


Re: [DxeHttpIoLib PATCH V2 1/3] NetworkPkg/Library: Implementation of Http IO Helper Library

Maciej Rabeda
 

Hi Abner,

Comments inline.

Thanks,
Maciej

On 29-Oct-20 07:11, Abner Chang wrote:

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Thursday, October 29, 2020 2:28 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3] NetworkPkg/Library:
Implementation of Http IO Helper Library

Thanks for patching spelling problems in MdePkg header.
I will send more comments on v3. Added one here regarding chunk transfer
security.

Thanks,
Maciej

On 26-Oct-20 07:53, Abner Chang wrote:
Thanks for reviewing this Maciej.

Most of comments are addressed. I also submitted a BZ for the wrong
spelling of CHUNK_TRNASFER_*, the patch is already sent.
V3 of this patch set is also sent to the mailing list.

Other feedbacks are in below,

-----Original Message-----
From: Rabeda, Maciej [mailto:maciej.rabeda@linux.intel.com]
Sent: Saturday, October 24, 2020 12:25 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
<abner.chang@hpe.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
Wang, Nickle (HPS SW) <nickle.wang@hpe.com>
Subject: Re: [edk2-devel] [DxeHttpIoLib PATCH V2 1/3]
NetworkPkg/Library:
Implementation of Http IO Helper Library

Hi Abner,

I understand the concept of moving HTTP I/O layer outside HttpBootDxe
and
into a separate library and I approve it.
However, there are some things that have to addressed/fixed/explained
here.
See comments within the patch.

On 20-Oct-20 04:38, Abner Chang wrote:
Add HTTP IO helper library which could be sued by HTTP applications
such as HTTP Boot, Redfish HTTP REST EX driver instance and etc.

Signed-off-by: Abner Chang <abner.chang@hpe.com>

Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Nickle Wang <nickle.wang@hpe.com>
---
NetworkPkg/Include/Library/HttpIoLib.h | 328 +++++++
.../Library/DxeHttpIoLib/DxeHttpIoLib.c | 809
++++++++++++++++++
.../Library/DxeHttpIoLib/DxeHttpIoLib.inf | 43 +
.../Library/DxeHttpIoLib/DxeHttpIoLib.uni | 13 +
4 files changed, 1193 insertions(+)
create mode 100644 NetworkPkg/Include/Library/HttpIoLib.h
create mode 100644 NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
create mode 100644
NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
diff --git a/NetworkPkg/Include/Library/HttpIoLib.h
b/NetworkPkg/Include/Library/HttpIoLib.h
new file mode 100644
index 0000000000..8f3804ca42
--- /dev/null
+++ b/NetworkPkg/Include/Library/HttpIoLib.h
@@ -0,0 +1,328 @@
+/** @file
+ HttpIoLib.h.
+
+(C) Copyright 2020 Hewlett-Packard Development Company, L.P.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef HTTP_IO_LIB_H_
+#define HTTP_IO_LIB_H_
+
+#include <IndustryStandard/Http11.h>
+
+#include <Library/DpcLib.h>
+#include <Library/HttpLib.h>
+#include <Library/NetLib.h>
+
+#define HTTP_IO_MAX_SEND_PAYLOAD 1024
+#define HTTP_IO_CHUNK_SIZE_STRING_LEN 50
+#define HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH 256
+
+///
+/// HTTP_IO_CALLBACK_EVENT
+///
+typedef enum {
+ HttpIoRequest,
+ HttpIoResponse
+} HTTP_IO_CALLBACK_EVENT;
+
+/**
+ HttpIo Callback function which will be invoked when specified
HTTP_IO_CALLBACK_EVENT happened.
+
+ @param[in] EventType Indicate the Event type that occurs in the
current callback.
+ @param[in] Message HTTP message which will be send to, or just
received from HTTP server.
+ @param[in] Context The Callback Context pointer.
+
+ @retval EFI_SUCCESS Tells the HttpIo to continue the HTTP
process.
+ @retval Others Tells the HttpIo to abort the current HTTP
process.
+**/
+typedef
+EFI_STATUS
+(EFIAPI * HTTP_IO_CALLBACK) (
+ IN HTTP_IO_CALLBACK_EVENT EventType,
+ IN EFI_HTTP_MESSAGE *Message,
+ IN VOID *Context
+ );
+
+///
+/// A wrapper structure to hold the received HTTP response data.
+///
+typedef struct {
+ EFI_HTTP_RESPONSE_DATA Response;
+ UINTN HeaderCount;
+ EFI_HTTP_HEADER *Headers;
+ UINTN BodyLength;
+ CHAR8 *Body;
+ EFI_STATUS Status;
+} HTTP_IO_RESPONSE_DATA;
+
+///
+/// HTTP_IO configuration data for IPv4 /// typedef struct {
+ EFI_HTTP_VERSION HttpVersion;
+ UINT32 RequestTimeOut; ///< In milliseconds.
+ UINT32 ResponseTimeOut; ///< In milliseconds.
+ BOOLEAN UseDefaultAddress;
+ EFI_IPv4_ADDRESS LocalIp;
+ EFI_IPv4_ADDRESS SubnetMask;
+ UINT16 LocalPort;
+} HTTP4_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO configuration data for IPv6 /// typedef struct {
+ EFI_HTTP_VERSION HttpVersion;
+ UINT32 RequestTimeOut; ///< In milliseconds.
+ BOOLEAN UseDefaultAddress;
+ EFI_IPv6_ADDRESS LocalIp;
+ UINT16 LocalPort;
+} HTTP6_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO configuration
+///
+typedef union {
+ HTTP4_IO_CONFIG_DATA Config4;
+ HTTP6_IO_CONFIG_DATA Config6;
+} HTTP_IO_CONFIG_DATA;
+
+///
+/// HTTP_IO wrapper of the EFI HTTP service.
+///
+typedef struct {
+ UINT8 IpVersion;
+ EFI_HANDLE Image;
+ EFI_HANDLE Controller;
+ EFI_HANDLE Handle;
+
+ EFI_HTTP_PROTOCOL *Http;
+
+ HTTP_IO_CALLBACK Callback;
+ VOID *Context;
+
+ EFI_HTTP_TOKEN ReqToken;
+ EFI_HTTP_MESSAGE ReqMessage;
+ EFI_HTTP_TOKEN RspToken;
+ EFI_HTTP_MESSAGE RspMessage;
+
+ BOOLEAN IsTxDone;
+ BOOLEAN IsRxDone;
+
+ EFI_EVENT TimeoutEvent;
+ UINT32 Timeout;
+} HTTP_IO;
+
+///
+/// Process code of HTTP chunk transfer.
+///
+typedef enum {
+ HttpIoSendChunkNone = 0,
+ HttpIoSendChunkHeaderZeroContent,
+ HttpIoSendChunkContent,
+ HttpIoSendChunkEndChunk,
+ HttpIoSendChunkFinish
+} HTTP_IO_SEND_CHUNK_PROCESS;
+
+///
+/// Process code of HTTP non chunk transfer.
+///
+typedef enum {
+ HttpIoSendNonChunkNone = 0,
+ HttpIoSendNonChunkHeaderZeroContent,
+ HttpIoSendNonChunkContent,
+ HttpIoSendNonChunkFinish
+} HTTP_IO_SEND_NON_CHUNK_PROCESS;
+
+///
+/// Chunk links for HTTP chunked transfer coding.
+///
+typedef struct {
+ LIST_ENTRY NextChunk;
+ UINTN Length;
+ CHAR8 *Data;
+} HTTP_IO_CHUNKS;
+
+/**
+ Notify the callback function when an event is triggered.
+
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotifyDpc (
+ IN VOID *Context
+ );
+
+/**
+ Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK.
+
+ @param[in] Event The event signaled.
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ );
+
+/**
+ Destroy the HTTP_IO and release the resources.
+
+ @param[in] HttpIo The HTTP_IO which wraps the HTTP service to
be
destroyed.
+
+**/
+VOID
+HttpIoDestroyIo (
+ IN HTTP_IO *HttpIo
+ );
+
+/**
+ Create a HTTP_IO to access the HTTP service. It will create and
+configure
+ a HTTP child handle.
+
+ @param[in] Image The handle of the driver image.
+ @param[in] Controller The handle of the controller.
+ @param[in] IpVersion IP_VERSION_4 or IP_VERSION_6.
+ @param[in] ConfigData The HTTP_IO configuration data.
+ @param[in] Callback Callback function which will be invoked when
specified
+ HTTP_IO_CALLBACK_EVENT happened.
+ @param[in] Context The Context data which will be passed to the
Callback function.
+ @param[out] HttpIo The HTTP_IO.
+
+ @retval EFI_SUCCESS The HTTP_IO is created and configured.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_UNSUPPORTED One or more of the control options
are
not
+ supported in the implementation.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval Others Failed to create the HTTP_IO or configure it.
+
+**/
+EFI_STATUS
+HttpIoCreateIo (
+ IN EFI_HANDLE Image,
+ IN EFI_HANDLE Controller,
+ IN UINT8 IpVersion,
+ IN HTTP_IO_CONFIG_DATA *ConfigData,
+ IN HTTP_IO_CALLBACK Callback,
+ IN VOID *Context,
+ OUT HTTP_IO *HttpIo
+ );
+
+/**
+ Synchronously send a HTTP REQUEST message to the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] Request A pointer to storage such data as URL and
HTTP method.
+ @param[in] HeaderCount Number of HTTP header structures in
Headers list.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[in] BodyLength Length in bytes of the HTTP body.
+ @param[in] Body Body associated with the HTTP request.
+
+ @retval EFI_SUCCESS The HTTP request is transmitted.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoSendRequest (
+ IN HTTP_IO *HttpIo,
+ IN EFI_HTTP_REQUEST_DATA *Request, OPTIONAL
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers, OPTIONAL
+ IN UINTN BodyLength,
+ IN VOID *Body OPTIONAL
+ );
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] RecvMsgHeader TRUE to receive a new HTTP response
(from message header).
+ FALSE to continue receive the previous response
message.
+ @param[out] ResponseData Point to a wrapper of the received
response data.
+
+ @retval EFI_SUCCESS The HTTP response is received.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoRecvResponse (
+ IN HTTP_IO *HttpIo,
+ IN BOOLEAN RecvMsgHeader,
+ OUT HTTP_IO_RESPONSE_DATA *ResponseData
+ );
+
+/**
+ Get the value of the content length if there is a "Content-Length"
header.
+
+ @param[in] HeaderCount Number of HTTP header structures in
Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ContentLength Pointer to save the value of the
content
length.
+
+ @retval EFI_SUCCESS Successfully get the content length.
+ @retval EFI_NOT_FOUND No "Content-Length" header in the
Headers.
+
+**/
+EFI_STATUS
+HttpIoGetContentLength (
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT UINTN *ContentLength
+ );
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] HeaderCount Number of headers in Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ChunkListHead A pointer to receivce list head of
chunked
data.
+ Caller has to release memory of ChunkListHead
+ and all list entries.
+ @param[out] ContentLength Total content length
+
+ @retval EFI_SUCCESS The HTTP chunked transfer is received.
+ @retval EFI_NOT_FOUND No chunked transfer coding header
found.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_INVALID_PARAMETER Improper parameters.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoGetChunkedTransferContent (
+ IN HTTP_IO *HttpIo,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT LIST_ENTRY **ChunkListHead,
+ OUT UINTN *ContentLength
+ );
+
+/**
+ Send HTTP request in chunks.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] SendChunkProcess Pointer to current chunk process
status.
+ @param[out] RequestMessage Request to send.
+
+ @retval EFI_SUCCESS Successfully to send chunk data according
to
SendChunkProcess.
+ @retval Other Other errors.
+
+**/
+EFI_STATUS
+HttpIoSendChunkedTransfer (
+ IN HTTP_IO *HttpIo,
+ IN HTTP_IO_SEND_CHUNK_PROCESS *SendChunkProcess,
+ IN EFI_HTTP_MESSAGE *RequestMessage
+);
+#endif
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
new file mode 100644
index 0000000000..fbce86ea5c
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.c
@@ -0,0 +1,809 @@
+/** @file
+ Http IO Helper Library.
+
+ (C) Copyright 2020 Hewlett-Packard Development Company, L.P.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent **/
+
+#include <Uefi.h>
+
+#include <Protocol/Http.h>
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/HttpIoLib.h>
+#include <Library/MemoryAllocationLib.h> #include
+<Library/PrintLib.h> #include <Library/UefiBootServicesTableLib.h>
+
+/**
+ Notify the callback function when an event is triggered.
+
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotifyDpc (
+ IN VOID *Context
+ )
+{
+ *((BOOLEAN *) Context) = TRUE;
+}
+
+/**
+ Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK.
+
+ @param[in] Event The event signaled.
+ @param[in] Context The opaque parameter to the function.
+
+**/
+VOID
+EFIAPI
+HttpIoNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ //
+ // Request HttpIoNotifyDpc as a DPC at TPL_CALLBACK
+ //
+ QueueDpc (TPL_CALLBACK, HttpIoNotifyDpc, Context); }
+
+/**
+ Destroy the HTTP_IO and release the resources.
+
+ @param[in] HttpIo The HTTP_IO which wraps the HTTP service to
be
destroyed.
+
+**/
+VOID
+HttpIoDestroyIo (
+ IN HTTP_IO *HttpIo
+ )
+{
+ EFI_HTTP_PROTOCOL *Http;
+ EFI_EVENT Event;
+
+ if (HttpIo == NULL) {
+ return;
+ }
+
+ Event = HttpIo->ReqToken.Event;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Event = HttpIo->RspToken.Event;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Event = HttpIo->TimeoutEvent;
+ if (Event != NULL) {
+ gBS->CloseEvent (Event);
+ }
+
+ Http = HttpIo->Http;
+ if (Http != NULL) {
+ Http->Configure (Http, NULL);
+ gBS->CloseProtocol (
+ HttpIo->Handle,
+ &gEfiHttpProtocolGuid,
+ HttpIo->Image,
+ HttpIo->Controller
+ );
+ }
+
+ NetLibDestroyServiceChild (
+ HttpIo->Controller,
+ HttpIo->Image,
+ &gEfiHttpServiceBindingProtocolGuid,
+ HttpIo->Handle
+ );
+}
+
+/**
+ Create a HTTP_IO to access the HTTP service. It will create and
+configure
+ a HTTP child handle.
+
+ @param[in] Image The handle of the driver image.
+ @param[in] Controller The handle of the controller.
+ @param[in] IpVersion IP_VERSION_4 or IP_VERSION_6.
+ @param[in] ConfigData The HTTP_IO configuration data ,
+ NULL means not to configure the HTTP child.
+ @param[in] Callback Callback function which will be invoked when
specified
+ HTTP_IO_CALLBACK_EVENT happened.
+ @param[in] Context The Context data which will be passed to the
Callback function.
+ @param[out] HttpIo The HTTP_IO.
+
+ @retval EFI_SUCCESS The HTTP_IO is created and configured.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_UNSUPPORTED One or more of the control options
are
not
+ supported in the implementation.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval Others Failed to create the HTTP_IO or configure it.
+
+**/
+EFI_STATUS
+HttpIoCreateIo (
+ IN EFI_HANDLE Image,
+ IN EFI_HANDLE Controller,
+ IN UINT8 IpVersion,
+ IN HTTP_IO_CONFIG_DATA *ConfigData, OPTIONAL
+ IN HTTP_IO_CALLBACK Callback,
+ IN VOID *Context,
+ OUT HTTP_IO *HttpIo
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_CONFIG_DATA HttpConfigData;
+ EFI_HTTPv4_ACCESS_POINT Http4AccessPoint;
+ EFI_HTTPv6_ACCESS_POINT Http6AccessPoint;
+ EFI_HTTP_PROTOCOL *Http;
+ EFI_EVENT Event;
+
+ if ((Image == NULL) || (Controller == NULL) || (HttpIo == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (IpVersion != IP_VERSION_4 && IpVersion != IP_VERSION_6) {
+ return EFI_UNSUPPORTED;
+ }
+
+ ZeroMem (HttpIo, sizeof (HTTP_IO)); ZeroMem (&HttpConfigData,
+ sizeof (EFI_HTTP_CONFIG_DATA));
+
+ //
+ // Create the HTTP child instance and get the HTTP protocol.
+ //
+ Status = NetLibCreateServiceChild (
+ Controller,
+ Image,
+ &gEfiHttpServiceBindingProtocolGuid,
+ &HttpIo->Handle
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ Status = gBS->OpenProtocol (
+ HttpIo->Handle,
+ &gEfiHttpProtocolGuid,
+ (VOID **) &Http,
+ Image,
+ Controller,
+ EFI_OPEN_PROTOCOL_BY_DRIVER
+ );
+ if (EFI_ERROR (Status) || (Http == NULL)) {
+ goto ON_ERROR;
+ }
+
+ //
+ // Init the configuration data and configure the HTTP child.
+ //
+ HttpIo->Image = Image;
+ HttpIo->Controller = Controller;
+ HttpIo->IpVersion = IpVersion;
+ HttpIo->Http = Http;
+ HttpIo->Callback = Callback;
+ HttpIo->Context = Context;
+
+ if (ConfigData != NULL) {
+ if (HttpIo->IpVersion == IP_VERSION_4) {
+ HttpConfigData.LocalAddressIsIPv6 = FALSE;
+ HttpConfigData.HttpVersion = ConfigData->Config4.HttpVersion;
+ HttpConfigData.TimeOutMillisec = ConfigData-
Config4.RequestTimeOut;
+
+ Http4AccessPoint.UseDefaultAddress = ConfigData-
Config4.UseDefaultAddress;
+ Http4AccessPoint.LocalPort = ConfigData->Config4.LocalPort;
+ IP4_COPY_ADDRESS (&Http4AccessPoint.LocalAddress,
&ConfigData-
Config4.LocalIp);
+ IP4_COPY_ADDRESS (&Http4AccessPoint.LocalSubnet, &ConfigData-
Config4.SubnetMask);
+ HttpConfigData.AccessPoint.IPv4Node = &Http4AccessPoint;
+ } else {
+ HttpConfigData.LocalAddressIsIPv6 = TRUE;
+ HttpConfigData.HttpVersion = ConfigData->Config6.HttpVersion;
+ HttpConfigData.TimeOutMillisec = ConfigData-
Config6.RequestTimeOut;
+
+ Http6AccessPoint.LocalPort = ConfigData->Config6.LocalPort;
+ IP6_COPY_ADDRESS (&Http6AccessPoint.LocalAddress,
&ConfigData-
Config6.LocalIp);
+ HttpConfigData.AccessPoint.IPv6Node = &Http6AccessPoint;
+ }
+
+ Status = Http->Configure (Http, &HttpConfigData);
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ }
+
+ //
+ // Create events for variuos asynchronous operations.
+ //
+ Status = gBS->CreateEvent (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ HttpIoNotify,
+ &HttpIo->IsTxDone,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->ReqToken.Event = Event;
+ HttpIo->ReqToken.Message = &HttpIo->ReqMessage;
+
+ Status = gBS->CreateEvent (
+ EVT_NOTIFY_SIGNAL,
+ TPL_NOTIFY,
+ HttpIoNotify,
+ &HttpIo->IsRxDone,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->RspToken.Event = Event;
+ HttpIo->RspToken.Message = &HttpIo->RspMessage;
+
+ //
+ // Create TimeoutEvent for response // Status = gBS->CreateEvent
+ (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &Event
+ );
+ if (EFI_ERROR (Status)) {
+ goto ON_ERROR;
+ }
+ HttpIo->TimeoutEvent = Event;
+ return EFI_SUCCESS;
+
+ON_ERROR:
+ HttpIoDestroyIo (HttpIo);
+
+ return Status;
+}
+
+/**
+ Synchronously send a HTTP REQUEST message to the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] Request A pointer to storage such data as URL and
HTTP method.
+ @param[in] HeaderCount Number of HTTP header structures in
Headers list.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[in] BodyLength Length in bytes of the HTTP body.
+ @param[in] Body Body associated with the HTTP request.
+
+ @retval EFI_SUCCESS The HTTP request is trasmitted.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoSendRequest (
+ IN HTTP_IO *HttpIo,
+ IN EFI_HTTP_REQUEST_DATA *Request,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ IN UINTN BodyLength,
+ IN VOID *Body
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_PROTOCOL *Http;
+
+ if (HttpIo == NULL || HttpIo->Http == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ HttpIo->ReqToken.Status = EFI_NOT_READY;
+ HttpIo->ReqToken.Message->Data.Request = Request;
+ HttpIo->ReqToken.Message->HeaderCount = HeaderCount;
+ HttpIo->ReqToken.Message->Headers = Headers;
+ HttpIo->ReqToken.Message->BodyLength = BodyLength;
+ HttpIo->ReqToken.Message->Body = Body;
+
+ if (HttpIo->Callback != NULL) {
+ Status = HttpIo->Callback (
+ HttpIoRequest,
+ HttpIo->ReqToken.Message,
+ HttpIo->Context
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ //
+ // Queue the request token to HTTP instances.
+ //
+ Http = HttpIo->Http;
+ HttpIo->IsTxDone = FALSE;
+ Status = Http->Request (
+ Http,
+ &HttpIo->ReqToken
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Poll the network until transmit finish.
+ //
+ while (!HttpIo->IsTxDone) {
+ Http->Poll (Http);
+ }
+
+ return HttpIo->ReqToken.Status;
+}
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] RecvMsgHeader TRUE to receive a new HTTP response
(from message header).
+ FALSE to continue receive the previous response
message.
+ @param[out] ResponseData Point to a wrapper of the received
response data.
+
+ @retval EFI_SUCCESS The HTTP response is received.
+ @retval EFI_INVALID_PARAMETER One or more parameters are
invalid.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_DEVICE_ERROR An unexpected network or system
error
occurred.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoRecvResponse (
+ IN HTTP_IO *HttpIo,
+ IN BOOLEAN RecvMsgHeader,
+ OUT HTTP_IO_RESPONSE_DATA *ResponseData
+ )
+{
+ EFI_STATUS Status;
+ EFI_HTTP_PROTOCOL *Http;
+
+ if (HttpIo == NULL || HttpIo->Http == NULL || ResponseData == NULL)
{
+ return EFI_INVALID_PARAMETER;
+ }
+
+ //
+ // Create new TimeoutEvent for response // if
+ (HttpIo->TimeoutEvent != NULL) {
+ gBS->CloseEvent (HttpIo->TimeoutEvent);
+ HttpIo->TimeoutEvent = NULL;
+ }
+
+ Status = gBS->CreateEvent (
+ EVT_TIMER,
+ TPL_CALLBACK,
+ NULL,
+ NULL,
+ &HttpIo->TimeoutEvent
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Is it necessary to recreate the event? Wouldn't this be enough to reset it?
No, we don’t have to recreate this event.
gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
I made some changes in this function.
+
+ //
+ // Start the timer, and wait Timeout seconds to receive the header
packet.
+ //
+ Status = gBS->SetTimer (HttpIo->TimeoutEvent, TimerRelative,
+ HttpIo->Timeout * TICKS_PER_MS); if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ //
+ // Queue the response token to HTTP instances.
+ //
+ HttpIo->RspToken.Status = EFI_NOT_READY; if (RecvMsgHeader) {
+ HttpIo->RspToken.Message->Data.Response =
+ &ResponseData->Response; } else {
+ HttpIo->RspToken.Message->Data.Response = NULL; }
+ HttpIo->RspToken.Message->HeaderCount = 0;
+ HttpIo->RspToken.Message->Headers = NULL;
+ HttpIo->RspToken.Message->BodyLength = ResponseData-
BodyLength;
+ HttpIo->RspToken.Message->Body = ResponseData->Body;
+
+ Http = HttpIo->Http;
+ HttpIo->IsRxDone = FALSE;
+ Status = Http->Response (
+ Http,
+ &HttpIo->RspToken
+ );
+
+ if (EFI_ERROR (Status)) {
+ gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
+ return Status;
+ }
+
+ //
+ // Poll the network until receive finish.
+ //
+ while (!HttpIo->IsRxDone && ((HttpIo->TimeoutEvent == NULL) ||
EFI_ERROR (gBS->CheckEvent (HttpIo->TimeoutEvent)))) {
+ Http->Poll (Http);
+ }
+
+ gBS->SetTimer (HttpIo->TimeoutEvent, TimerCancel, 0);
+
+ if (!HttpIo->IsRxDone) {
+ //
+ // Timeout occurs, cancel the response token.
+ //
+ Http->Cancel (Http, &HttpIo->RspToken);
+
+ Status = EFI_TIMEOUT;
+
+ return Status;
+ } else {
+ HttpIo->IsRxDone = FALSE;
+ }
+
+ if ((HttpIo->Callback != NULL) &&
+ (HttpIo->RspToken.Status == EFI_SUCCESS || HttpIo-
RspToken.Status == EFI_HTTP_ERROR)) {
+ Status = HttpIo->Callback (
+ HttpIoResponse,
+ HttpIo->RspToken.Message,
+ HttpIo->Context
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+ }
+
+ //
+ // Store the received data into the wrapper.
+ //
+ ResponseData->Status = HttpIo->RspToken.Status;
+ ResponseData->HeaderCount = HttpIo->RspToken.Message-
HeaderCount;
+ ResponseData->Headers = HttpIo->RspToken.Message->Headers;
+ ResponseData->BodyLength = HttpIo->RspToken.Message-
BodyLength;
+
+ return Status;
+}
+
+/**
+ Get the value of the content length if there is a "Content-Length"
header.
+
+ @param[in] HeaderCount Number of HTTP header structures in
Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ContentLength Pointer to save the value of the
content
length.
+
+ @retval EFI_SUCCESS Successfully get the content length.
+ @retval EFI_NOT_FOUND No "Content-Length" header in the
Headers.
+
+**/
+EFI_STATUS
+HttpIoGetContentLength (
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT UINTN *ContentLength
+ )
+{
+ EFI_HTTP_HEADER *Header;
+
+ Header = HttpFindHeader (HeaderCount, Headers,
+ HTTP_HEADER_CONTENT_LENGTH); if (Header == NULL) {
+ return EFI_NOT_FOUND;
+ }
+
+ return AsciiStrDecimalToUintnS (Header->FieldValue, (CHAR8 **)
+NULL, ContentLength); }
+/**
+ Send HTTP request in chunks.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] SendChunkProcess Pointer to current chunk process
status.
+ @param[in] RequestMessage Request to send.
+
+ @retval EFI_SUCCESS Successfully to send chunk data according
to
SendChunkProcess.
+ @retval Other Other errors.
+
+**/
+EFI_STATUS
+HttpIoSendChunkedTransfer (
+ IN HTTP_IO *HttpIo,
+ IN HTTP_IO_SEND_CHUNK_PROCESS *SendChunkProcess,
+ IN EFI_HTTP_MESSAGE *RequestMessage
+)
+{
+ EFI_STATUS Status;
+ EFI_HTTP_HEADER *NewHeaders;
+ EFI_HTTP_HEADER *ContentLengthHeader;
+ UINTN AddNewHeader;
+ UINTN HeaderCount;
+ CHAR8 *MessageBody;
+ UINTN MessageBodyLength;
+ CHAR8 ChunkLengthStr [HTTP_IO_CHUNK_SIZE_STRING_LEN];
+ EFI_HTTP_REQUEST_DATA *SentRequestData;
This is a new function and I already see a difference in the coding standard.
Please align variable names like in previous functions.
Done

+
+ AddNewHeader = 0;
+ NewHeaders = NULL;
+ MessageBody = NULL;
+ ContentLengthHeader = NULL;
+ MessageBodyLength = 0;
Alignment of assignments.
Done

+
+ switch (*SendChunkProcess) {
+ case HttpIoSendChunkHeaderZeroContent:
Case spacing: 2 spaces. Applies throughout new code.
+ ContentLengthHeader =
+ HttpFindHeader(RequestMessage->HeaderCount, RequestMessage-
Headers,
+ HTTP_HEADER_CONTENT_LENGTH);
Spacing after function call name. This applies from this point until the end
of
patch.
Done
+ if (ContentLengthHeader == NULL) {
+ AddNewHeader = 1;
+ }
+
+ NewHeaders = AllocateZeroPool((RequestMessage->HeaderCount
+
+ AddNewHeader) * sizeof(EFI_HTTP_HEADER));
Lack of NULL pointer test for NewHeaders (out of memory case).
+ CopyMem ((VOID*)NewHeaders, (VOID *)RequestMessage-
Headers,
RequestMessage->HeaderCount * sizeof (EFI_HTTP_HEADER));
+ if (AddNewHeader == 0) {
+ //
+ // Override content-length to Transfer-Encoding.
+ //
+ ContentLengthHeader = HttpFindHeader (RequestMessage-
HeaderCount, NewHeaders, HTTP_HEADER_CONTENT_LENGTH);
+ ContentLengthHeader->FieldName = NULL;
+ ContentLengthHeader->FieldValue = NULL;
+ } else {
+ ContentLengthHeader = NewHeaders + RequestMessage-
HeaderCount;
+ }
+ HttpSetFieldNameAndValue(ContentLengthHeader,
HTTP_HEADER_TRANSFER_ENCODING,
HTTP_HEADER_TRANSFER_ENCODING_CHUNKED);
+ HeaderCount = RequestMessage->HeaderCount + AddNewHeader;
Move HeaderCount assignment before NewHeaders allocation. Use
HeaderCount as argument for NewHeaders allocation.
+ MessageBodyLength = 0;
+ MessageBody = NULL;
+ SentRequestData = RequestMessage->Data.Request;
+ break;
+
+ case HttpIoSendChunkContent:
+ HeaderCount = 0;
+ NewHeaders = NULL;
+ SentRequestData = NULL;
+ if (RequestMessage->BodyLength >
HTTP_IO_MAX_SEND_PAYLOAD) {
+ MessageBodyLength = HTTP_IO_MAX_SEND_PAYLOAD;
+ } else {
+ MessageBodyLength = RequestMessage->BodyLength;
+ }
+ AsciiSPrint (ChunkLengthStr, HTTP_IO_CHUNK_SIZE_STRING_LEN,
+ "%x%c%c", MessageBodyLength, CHUNKED_TRNASFER_CODING_CR,
+ CHUNKED_TRNASFER_CODING_LF);
Line too long, break down in accordance to coding standard.
Done

+ MessageBody = AllocatePool (AsciiStrLen (ChunkLengthStr) +
MessageBodyLength + 2);
+ if (MessageBody == NULL) {
+ DEBUG((DEBUG_ERROR, "Not enough memory for chunk
transfer\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (MessageBody, ChunkLengthStr, AsciiStrLen
(ChunkLengthStr));
+ CopyMem (MessageBody + AsciiStrLen (ChunkLengthStr),
RequestMessage->Body, MessageBodyLength);
+ *(MessageBody + AsciiStrLen (ChunkLengthStr) +
MessageBodyLength)
= CHUNKED_TRNASFER_CODING_CR;
+ *(MessageBody + AsciiStrLen (ChunkLengthStr) +
MessageBodyLength
+ 1) = CHUNKED_TRNASFER_CODING_LF;
+ RequestMessage->BodyLength -= MessageBodyLength;
+ RequestMessage->Body = (VOID *)((CHAR8 *)RequestMessage-
Body + MessageBodyLength);
+ MessageBodyLength += (AsciiStrLen (ChunkLengthStr) + 2);
This block could use some spacing and comments to improve readability.
Done

+ if (RequestMessage->BodyLength == 0) {
+ *SendChunkProcess = HttpIoSendChunkEndChunk;
+ }
+ break;
+
+ case HttpIoSendChunkEndChunk:
+ HeaderCount = 0;
+ NewHeaders = NULL;
+ SentRequestData = NULL;
+ AsciiSPrint (ChunkLengthStr, HTTP_IO_CHUNK_SIZE_STRING_LEN,
"0%c%c%c%c",
+ CHUNKED_TRNASFER_CODING_CR,
CHUNKED_TRNASFER_CODING_LF,
+ CHUNKED_TRNASFER_CODING_CR,
CHUNKED_TRNASFER_CODING_LF
+ );
Please break function calls in accordance with coding standard.
Done
+ MessageBody = AllocatePool (AsciiStrLen(ChunkLengthStr));
+ if (MessageBody == NULL) {
+ DEBUG((DEBUG_ERROR, "Not enough memory for the end chunk
transfer\n"));
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (MessageBody, ChunkLengthStr, AsciiStrLen
(ChunkLengthStr));
+ MessageBodyLength = AsciiStrLen(ChunkLengthStr);
+ *SendChunkProcess = HttpIoSendChunkFinish;
+ break;
+
+ default:
+ return EFI_INVALID_PARAMETER;
+ }
+
+ Status = HttpIoSendRequest(
+ HttpIo,
+ SentRequestData,
+ HeaderCount,
+ NewHeaders,
+ MessageBodyLength,
+ MessageBody
+ );
+ if (ContentLengthHeader != NULL) {
+ if (ContentLengthHeader->FieldName != NULL) {
+ FreePool(ContentLengthHeader->FieldName);
+ }
+ if (ContentLengthHeader->FieldValue != NULL) {
+ FreePool(ContentLengthHeader->FieldValue);
+ }
+ ContentLengthHeader = NULL;
NULL assignments are not necessary here as we are exiting the function
and
those variables will not be used.
Right, fixed.
+ }
+ if (NewHeaders != NULL) {
+ FreePool(NewHeaders);
+ NewHeaders = NULL;
+ }
+ if (MessageBody != NULL) {
+ FreePool(MessageBody);
+ MessageBody = NULL;
+ }
+ return Status;
+}
+
+/**
+ Synchronously receive a HTTP RESPONSE message from the server.
+
+ @param[in] HttpIo The HttpIo wrapping the HTTP service.
+ @param[in] HeaderCount Number of headers in Headers.
+ @param[in] Headers Array containing list of HTTP headers.
+ @param[out] ChunkListHead A pointer to receivce list head of
chunked
data.
Spelling: receive.
Fixed.

+ Caller has to release memory of ChunkListHead
+ and all list entries.
+ @param[out] ContentLength Total content length
+
+ @retval EFI_SUCCESS The HTTP chunked transfer is received.
+ @retval EFI_NOT_FOUND No chunked transfer coding header
found.
+ @retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
+ @retval EFI_INVALID_PARAMETER Improper parameters.
+ @retval Others Other errors as indicated.
+
+**/
+EFI_STATUS
+HttpIoGetChunkedTransferContent (
+ IN HTTP_IO *HttpIo,
+ IN UINTN HeaderCount,
+ IN EFI_HTTP_HEADER *Headers,
+ OUT LIST_ENTRY **ChunkListHead,
+ OUT UINTN *ContentLength
+ )
+{
+ EFI_HTTP_HEADER *Header;
+ CHAR8 ChunkSizeAscii[256];
+ EFI_STATUS Status;
+ UINTN Index;
+ HTTP_IO_RESPONSE_DATA ResponseData;
+ UINTN TotalLength;
+ LIST_ENTRY *HttpChunks;
+ HTTP_IO_CHUNKS *ThisChunk;
Variable alignment.
+ LIST_ENTRY *ThisListEntry;
+
+ if (ChunkListHead == NULL || ContentLength == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ *ContentLength = 0;
+ Header = HttpFindHeader (HeaderCount, Headers,
+ HTTP_HEADER_TRANSFER_ENCODING); if (Header == NULL) {
+ return EFI_NOT_FOUND;
+ }
+ if (AsciiStrCmp (Header->FieldValue,
+ HTTP_HEADER_TRANSFER_ENCODING_CHUNKED) == 0) {
Please do a negation comparison instead and return. It reduces one level
of
indentation.
Done
+ //
+ // Loop to get all chunks.
+ //
Please avoid empty single-line comments when starting and ending
comment block.
+ TotalLength = 0;
+ HttpChunks = (LIST_ENTRY *)AllocateZeroPool (sizeof (LIST_ENTRY));
+ if (HttpChunks == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (HttpChunks);
Add a bit of spacing above to improve readability.
Fixed
+ DEBUG ((DEBUG_INFO, " Chunked transfer\n"));
+ while (TRUE) {
+ ZeroMem((VOID *)&ResponseData,
sizeof(HTTP_IO_RESPONSE_DATA));
+ ResponseData.BodyLength =
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH;
+ ResponseData.Body = ChunkSizeAscii;
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
Returning here creates memory leak risk - chunks previously received will
not
be freed.
Fixed
+ //
+ // Decoding Chunked Transfer Coding.
+ // Only decode chunk-size and last chunk.
+ //
+ DEBUG ((DEBUG_INFO, " Chunk HTTP Response StatusCode -
%d\n",
ResponseData.Response.StatusCode));
+ //
+ // Break if this is last chunk.
+ //
+ if (ChunkSizeAscii [0] ==
CHUNKED_TRNASFER_CODING_LAST_CHUNK)
{
+ Status = EFI_SUCCESS;
+ DEBUG ((DEBUG_INFO, " Last chunk\n"));
+ ThisChunk = (HTTP_IO_CHUNKS *)AllocateZeroPool (sizeof
(HTTP_IO_CHUNKS));
+ if (ThisChunk == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (&ThisChunk->NextChunk);
+ ThisChunk->Length = ResponseData.BodyLength - 1 - 2; // Minus
sizeof '0' and CRLF.
+ ThisChunk->Data = (CHAR8 *)AllocatePool (ThisChunk->Length);
+ if (ThisChunk->Data == NULL) {
+ FreePool ((UINT8 *)ThisChunk);
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ CopyMem ((UINT8 *)ThisChunk->Data, (UINT8
*)ResponseData.Body
+ 1, ThisChunk->Length);
+ TotalLength += ThisChunk->Length;
+ InsertTailList (HttpChunks, &ThisChunk->NextChunk);
+ break;
+ }
+
+ //
+ // Get the chunk length
+ //
+ Index = 0;
+ while ((ChunkSizeAscii [Index] !=
CHUNKED_TRNASFER_CODING_EXTENSION_SAPERATOR) &&
+ (ChunkSizeAscii [Index] !=
+ (CHAR8)CHUNKED_TRNASFER_CODING_CR) &&
Spelling errors in macros above.
Oops. BZ is submitted for this typo in http11.h.
INVALID URI REMOVED
3A__bugzilla.tianocore.org_show-5Fbug.cgi-3Fid-
3D3019&d=DwIDaQ&c=C5b8zRQO1miGmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulks
kz6qU3NYRO03nHp9P7Z5q59A3E&m=f6zs_mrzwCCYwQSl3p4Nbuib7Ln4gVcO
KS74o-Xc524&s=g1WHLCl8Uzm8YKLYknM2Y7T0d4gT7fv_58qhc4Qejj0&e=
Patch is set to the mailing list for review.


+ (Index !=
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH)) {
+ Index ++;
+ };
+ if (Index ==
HTTP_IO_CHUNKED_TRANSFER_CODING_DATA_LENGTH)
{
+ return EFI_NOT_FOUND;
+ }
Again - memory leak.
Fixed

+ ChunkSizeAscii[Index] = 0;
+ AsciiStrHexToUintnS (ChunkSizeAscii, NULL, ContentLength);
+ DEBUG ((DEBUG_INFO, " Length of this chunk %d\n",
*ContentLength));
+ //
+ // Receive the data;
+ //
+ ThisChunk = (HTTP_IO_CHUNKS *)AllocateZeroPool (sizeof
(HTTP_IO_CHUNKS));
+ if (ThisChunk == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ ResponseData.BodyLength = *ContentLength;
+ ResponseData.Body = (CHAR8 *)AllocatePool (*ContentLength);
+ if (ResponseData.Body == NULL) {
+ FreePool (ThisChunk);
+ Status = EFI_OUT_OF_RESOURCES;
+ goto DeleterChunks;
+ }
+ InitializeListHead (&ThisChunk->NextChunk);
+ ThisChunk->Length = *ContentLength;
+ ThisChunk->Data = ResponseData.Body;
+ InsertTailList (HttpChunks, &ThisChunk->NextChunk);
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ goto DeleterChunks;
+ }
+ //
+ // Read CRLF
+ //
+ ZeroMem((VOID *)&ResponseData,
sizeof(HTTP_IO_RESPONSE_DATA));
+ ResponseData.BodyLength = 2;
+ ResponseData.Body = ChunkSizeAscii;
+ Status = HttpIoRecvResponse (
+ HttpIo,
+ FALSE,
+ &ResponseData
+ );
+ if (EFI_ERROR (Status)) {
+ goto DeleterChunks;
+ }
+
+ TotalLength += *ContentLength;
The following code requires some kind of protection against receiving an
infinite number of chunks.
Currently I could just not send a last chunk and wait for the platform to
crash
or/and run out of memory :)
Good point! However, I can't think of any good way to prevent from this
happens. This depends on the reliability of content provider and how the
web service handles chunk transfer, right?
From system firmware viewpoint, we would connect to a trusted content
source for either HTTP boot image and Redfish service resource.
Do you think this concern is valid for the real practice?

Or give it a limited size of payload? Like the size larger than 1G could be
considered as an ill content?
What would be the largest payload that you would like to use
chunk-transfer for?
For the HTTP response, whether to use chunk transfer is at the discretion of Redfish service. We currently can only tell if the response payload is in chunk transfer or not using Transfer-Encoding HTTP response header but not able to predict the size of payload unless we get the first chunk.
In the real practice, I see a large payload like AttributeRegistry is transferred back in chunk, however, I also see some small size payload is transferred back in chunk transfer as well.
I see that the chunked transfer is not being currently used in HTTP
Boot, so we should stick to current use case -  Redfish.
Yes, currently only RedfishPkg use HTTP chunk transfer APIs defined in DxeHttpIoLib. But what do you mean to stick the use case to Redfish only? Those chunk transfer related APIs could be commonly used by any edk2 HTTP clients need chunk transfer although we only have two use cases (and only Redfish uses it so far).
I only meant that it would be reasonable to come up with a chunk-transfer limit that is dictated solely by Redfish use case, since it is currently the only one around.
In case other SW pieces would like to consume HTTP I/O library for chunk-transfer and see that the limit is too small, they could propose a patch to extend it.
The other approach would be to somehow dynamically assess the limit by looking at how much free memory is available on the platform.
Can we estimate what would be the largest payload Redfish would like to push through?

+ };
+ *ContentLength = TotalLength;
+ *ChunkListHead = HttpChunks;
+ DEBUG ((DEBUG_INFO, " Total of lengh of chunks :%d\n",
TotalLength));
+ return EFI_SUCCESS;
+ } else {
+ return EFI_NOT_FOUND;
+ }
+DeleterChunks:;
Name this label ExitDeleteChunks and remove the comma.
+ while (!IsListEmpty (HttpChunks)) {
+ ThisListEntry = GetFirstNode (HttpChunks);
+ RemoveEntryList (ThisListEntry);
+ };
+ return Status;
+}
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
new file mode 100644
index 0000000000..a02b409547
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.inf
@@ -0,0 +1,43 @@
+## @file
+# This library instance provides HTTP IO helper functions.
+#
+# (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
#
+SPDX-License-Identifier: BSD-2-Clause-Patent # ##
+
+[Defines]
+ INF_VERSION = 0x0001001b
+ BASE_NAME = DxeHttpIoLib
+ MODULE_UNI_FILE = DxeHttpIoLib.uni
+ FILE_GUID = 50B198F8-7986-4F51-A857-CFE4643D59F3
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = HttpIoLib| DXE_CORE DXE_DRIVER
DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION
UEFI_DRIVER
+
+#
+# The following information is for reference only and not required by
the
build tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 EBC ARM AARM64 RISCV64
+#
+
+[Sources]
+ DxeHttpIoLib.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ NetworkPkg/NetworkPkg.dec
+
+
+[LibraryClasses]
+ BaseLib
+ BaseMemoryLib
+ DebugLib
+ DpcLib
+ MemoryAllocationLib
+ PrintLib
+ UefiBootServicesTableLib
+
+[Protocols]
+ gEfiHttpProtocolGuid ## SOMETIMES_CONSUMES
+
diff --git a/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
new file mode 100644
index 0000000000..d419b1a49d
--- /dev/null
+++ b/NetworkPkg/Library/DxeHttpIoLib/DxeHttpIoLib.uni
@@ -0,0 +1,13 @@
+// /** @file
+// The library instance provides HTTP IO helper functions.
+//
+// (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent // // **/
+
+
+#string STR_MODULE_ABSTRACT #language en-US "HTTP IO
Helper
Library"
+
+#string STR_MODULE_DESCRIPTION #language en-US "The library
instance provides HTTP IO helper functions."
+





[PATCH 2/2] uefi-sct/SctPkg: Correct BBTestEraseBlocks behavior (EFI_BLOCK_IO_PROTOCOL)

Chen, ArvinX
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3022=0D
=0D
The storage device erase behavior may have two possibilities:=0D
1.Write all data into "0"=0D
2.Write all data into "1"=0D
but now tool behavior can only check case 1 (Write all data into "0"),=0D
so we need add the other case into SCT tool to correct the test behavior.=0D
=0D
Cc: G Edhaya Chandran <Edhaya.Chandran@arm.com>=0D
Cc: Eric Jin <eric.jin@intel.com>=0D
Signed-off-by: ArvinX Chen <arvinx.chen@intel.com>=0D
---=0D
.../BlackBoxTest/EraseBlockBBTestFunction.c | 55 +++++++++++++++----=0D
1 file changed, 43 insertions(+), 12 deletions(-)=0D
=0D
diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBox=
Test/EraseBlockBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protoco=
l/EraseBlock/BlackBoxTest/EraseBlockBBTestFunction.c=0D
index cbf43e1d..dbbb70c6 100644=0D
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBoxTest/Er=
aseBlockBBTestFunction.c=0D
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/EraseBlock/BlackBoxTest/Er=
aseBlockBBTestFunction.c=0D
@@ -71,6 +71,7 @@ BBTestEraseBlocksFunctionTest (=0D
UINT64 Index;=0D
UINTN Index1;=0D
UINTN Remainder;=0D
+ UINT64 EraseCounter;=0D
=0D
EFI_ERASE_BLOCK_TOKEN Token;=0D
EFI_BLOCK_IO2_TOKEN BlockIo2Token;=0D
@@ -223,26 +224,41 @@ BBTestEraseBlocksFunctionTest (=0D
// Read the data with 0, the first/last block should not be erased=
=0D
ReadStatus =3D BlockIo->ReadBlocks (BlockIo, MediaId, Lba, BufferS=
ize, (VOID*)Buffer2);=0D
if (ReadStatus =3D=3D EFI_SUCCESS) {=0D
- for (Index1 =3D 0; Index1 < BlockSize; Index1++) {=0D
- if (Buffer2[Index1] !=3D 0) {=0D
+ for (Index1 =3D 0, EraseCounter =3D 0; Index1 < BlockSize; Index=
1++) {=0D
+ if (Buffer2[Index1] !=3D 0x00 && Buffer2[Index1] !=3D 0xFF) {=
=0D
IsZero1 =3D FALSE;=0D
break;=0D
+ } else if (Buffer2[Index1] =3D=3D 0x00) {=0D
+ EraseCounter++;=0D
}=0D
}=0D
+ if (EraseCounter!=3D0 && EraseCounter!=3DBlockSize) {=0D
+ IsZero1 =3D FALSE;=0D
+ }=0D
=0D
- for (Index1 =3D BlockSize; Index1 < BufferSize - BlockSize; Inde=
x1++) {=0D
- if (Buffer2[Index1] !=3D 0) {=0D
+ for (Index1 =3D BlockSize, EraseCounter =3D 0; Index1 < BufferSi=
ze - BlockSize; Index1++) {=0D
+ if (Buffer2[Index1] !=3D 0x00 && Buffer2[Index1] !=3D 0xFF) {=
=0D
IsZero2 =3D FALSE;=0D
break;=0D
+ } else if (Buffer2[Index1] =3D=3D 0x00) {=0D
+ EraseCounter++;=0D
}=0D
}=0D
+ if (EraseCounter!=3D0 && EraseCounter!=3D(BufferSize - (BlockSiz=
e*2))) {=0D
+ IsZero2 =3D FALSE;=0D
+ }=0D
=0D
- for (Index1 =3D BufferSize - BlockSize; Index1 < BufferSize; Ind=
ex1++) {=0D
- if (Buffer2[Index1] !=3D 0) {=0D
+ for (Index1 =3D BufferSize - BlockSize, EraseCounter =3D 0; Inde=
x1 < BufferSize; Index1++) {=0D
+ if (Buffer2[Index1] !=3D 0x00 && Buffer2[Index1] !=3D 0xFF) {=
=0D
IsZero3 =3D FALSE;=0D
break;=0D
+ } else if (Buffer2[Index1] =3D=3D 0x00) {=0D
+ EraseCounter++;=0D
}=0D
}=0D
+ if (EraseCounter!=3D0 && EraseCounter!=3DBlockSize) {=0D
+ IsZero3 =3D FALSE;=0D
+ }=0D
=0D
if ((EraseStatus =3D=3D EFI_SUCCESS) && (IsZero1 =3D=3D FALSE) &=
& (IsZero2 =3D=3D TRUE) && ((IsZero3 =3D=3D FALSE)))=0D
AssertionType =3D EFI_TEST_ASSERTION_PASSED;=0D
@@ -492,26 +508,41 @@ BlockIo2:=0D
// Read the data with 0, the first/last block should not be erased=
=0D
ReadStatus =3D BlockIo2->ReadBlocksEx (BlockIo2, MediaId, Lba, &Bl=
ockIo2Token, BufferSize, (VOID*)Buffer2);=0D
if (ReadStatus =3D=3D EFI_SUCCESS) {=0D
- for (Index1 =3D 0; Index1 < BlockSize; Index1++) {=0D
- if (Buffer2[Index1] !=3D 0) {=0D
+ for (Index1 =3D 0, EraseCounter =3D 0; Index1 < BlockSize; Index=
1++) {=0D
+ if (Buffer2[Index1] !=3D 0x00 && Buffer2[Index1] !=3D 0xFF) {=
=0D
IsZero1 =3D FALSE;=0D
break;=0D
+ } else if (Buffer2[Index1] =3D=3D 0x00) {=0D
+ EraseCounter++;=0D
}=0D
}=0D
+ if (EraseCounter!=3D0 && EraseCounter!=3DBlockSize) {=0D
+ IsZero1 =3D FALSE;=0D
+ }=0D
=0D
- for (Index1 =3D BlockSize; Index1 < BufferSize - BlockSize; Inde=
x1++) {=0D
- if (Buffer2[Index1] !=3D 0) {=0D
+ for (Index1 =3D BlockSize, EraseCounter =3D 0; Index1 < BufferSi=
ze - BlockSize; Index1++) {=0D
+ if (Buffer2[Index1] !=3D 0x00 && Buffer2[Index1] !=3D 0xFF) {=
=0D
IsZero2 =3D FALSE;=0D
break;=0D
+ } else if (Buffer2[Index1] =3D=3D 0x00) {=0D
+ EraseCounter++;=0D
}=0D
}=0D
+ if (EraseCounter!=3D0 && EraseCounter!=3D(BufferSize - (BlockSiz=
e*2))) {=0D
+ IsZero2 =3D FALSE;=0D
+ }=0D
=0D
- for (Index1 =3D BufferSize - BlockSize; Index1 < BufferSize; Ind=
ex1++) {=0D
- if (Buffer2[Index1] !=3D 0) {=0D
+ for (Index1 =3D BufferSize - BlockSize, EraseCounter =3D 0; Inde=
x1 < BufferSize; Index1++) {=0D
+ if (Buffer2[Index1] !=3D 0x00 && Buffer2[Index1] !=3D 0xFF) {=
=0D
IsZero3 =3D FALSE;=0D
break;=0D
+ } else if (Buffer2[Index1] =3D=3D 0x00) {=0D
+ EraseCounter++;=0D
}=0D
}=0D
+ if (EraseCounter!=3D0 && EraseCounter!=3DBlockSize) {=0D
+ IsZero3 =3D FALSE;=0D
+ }=0D
=0D
if ((EraseStatus =3D=3D EFI_SUCCESS) && (IsZero1 =3D=3D FALSE) &=
& (IsZero2 =3D=3D TRUE) && ((IsZero3 =3D=3D FALSE)))=0D
AssertionType =3D EFI_TEST_ASSERTION_PASSED;=0D
-- =0D
2.26.2.windows.1=0D
=0D


[PATCH 2/2] uefi-sct/SctPkg: Correct check image test behavior

Chen, ArvinX
 

Now, in our SCT test case "CheckImage" behavior have some problems.Once=0D
tool need to check the "EFI_SECURITY_VIOLATION" have correct return from=0D
"EFI_FIRMWARE_MANAGEMENT_PROTOCOL->CheckImage" function, the function wil=0D
because tool behavior probability return EFI_BUFFER_TOO_SMALL,so we should=
=0D
gave it a correct header info to make the test item can process correctly.=
=0D
=0D
Cc: ArvinX Chen <arvinx.chen@intel.com>=0D
Cc: G Edhaya Chandran <Edhaya.Chandran@arm.com>=0D
Cc: Eric Jin <eric.jin@intel.com>=0D
Cc: Wei6 Xu <wei6.xu@intel.com>=0D
Signed-off-by: ArvinX Chen <arvinx.chen@intel.com>=0D
---=0D
.../FirmwareManagementBBTestConformance.c | 11 ++++++++-=0D
.../FirmwareManagement/BlackBoxTest/Guid.c | 1 +=0D
.../FirmwareManagement/BlackBoxTest/Guid.h | 5 ++++=0D
.../SctPkg/UEFI/Protocol/FirmwareManagement.h | 23 +++++++++++++++++++=0D
4 files changed, 39 insertions(+), 1 deletion(-)=0D
=0D
diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/=
BlackBoxTest/FirmwareManagementBBTestConformance.c b/uefi-sct/SctPkg/TestCa=
se/UEFI/EFI/Protocol/FirmwareManagement/BlackBoxTest/FirmwareManagementBBTe=
stConformance.c=0D
index 720326d0..7c6c709b 100644=0D
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/BlackBo=
xTest/FirmwareManagementBBTestConformance.c=0D
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/BlackBo=
xTest/FirmwareManagementBBTestConformance.c=0D
@@ -2901,7 +2901,8 @@ BBTestCheckImageConformanceTestCheckpoint2 (=0D
UINTN i;=0D
EFI_FIRMWARE_IMAGE_DESCRIPTOR *p;=0D
UINTN FunctionTested;=0D
-=0D
+ EFI_FIRMWARE_IMAGE_AUTHENTICATION *EFIA;=0D
+ EFI_GUID gEfiCertPkcs7Guid;=0D
//=0D
// Init=0D
//=0D
@@ -2909,6 +2910,7 @@ BBTestCheckImageConformanceTestCheckpoint2 (=0D
Status =3D EFI_SUCCESS;=0D
AssertionType =3D EFI_TEST_ASSERTION_PASSED;=0D
TestGuid =3D gFirmwareManagementBBTestConformanceAssertionGuid012;=0D
+ gEfiCertPkcs7Guid =3D gFirmwareManagementBBTestConformanceSupportGuid005=
;=0D
ResultMessageLabel =3D L"CheckImage, conformance checkpoint #2";=0D
=0D
BufferImageInfo =3D NULL;=0D
@@ -3020,6 +3022,13 @@ BBTestCheckImageConformanceTestCheckpoint2 (=0D
ResultMessageData =3D L"test case initialization failure.";=0D
goto Exit;=0D
}=0D
+=0D
+ EFIA =3D Image;=0D
+ EFIA->AuthInfo.Hdr.dwLength =3D sizeof(WIN_CERTIFICATE_UEFI_GU=
ID)+0x10;=0D
+ EFIA->AuthInfo.Hdr.wRevision =3D 0x0200;=0D
+ EFIA->AuthInfo.Hdr.wCertificateType =3D WIN_CERT_TYPE_EFI_GUID;=0D
+ for (i=3D0; i<sizeof(EFI_GUID); ((UINT8*)&EFIA->AuthInfo.CertType)[i]=
=3D((UINT8*)&gEfiCertPkcs7Guid)[i], i++);=0D
+=0D
BufferImage =3D Image;=0D
FunctionTested++;=0D
Status =3D FirmwareManagement->CheckImage ( =0D
diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/=
BlackBoxTest/Guid.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareMa=
nagement/BlackBoxTest/Guid.c=0D
index 91cf1ba6..cd541496 100644=0D
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/BlackBo=
xTest/Guid.c=0D
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/BlackBo=
xTest/Guid.c=0D
@@ -43,6 +43,7 @@ EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid0=
01=3DEFI_TEST_FIRMWAREMAN=0D
EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid002=3DEFI_TEST_FIR=
MWAREMANAGEMENTBBTESTCONFORMANCE_SUPPORT_002_GUID;=0D
EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid003=3DEFI_TEST_FIR=
MWAREMANAGEMENTBBTESTCONFORMANCE_SUPPORT_003_GUID;=0D
EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid004=3DEFI_TEST_FIR=
MWAREMANAGEMENTBBTESTCONFORMANCE_SUPPORT_004_GUID;=0D
+EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid005=3DEFI_TEST_FIR=
MWAREMANAGEMENTBBTESTCONFORMANCE_SUPPORT_005_GUID;=0D
=0D
EFI_GUID gFirmwareManagementBBTestConformanceAssertionGuid001=3DEFI_TEST_F=
IRMWAREMANAGEMENTBBTESTCONFORMANCE_ASSERTION_001_GUID;=0D
EFI_GUID gFirmwareManagementBBTestConformanceAssertionGuid002=3DEFI_TEST_F=
IRMWAREMANAGEMENTBBTESTCONFORMANCE_ASSERTION_002_GUID;=0D
diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/=
BlackBoxTest/Guid.h b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareMa=
nagement/BlackBoxTest/Guid.h=0D
index b5277f7e..b045021e 100644=0D
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/BlackBo=
xTest/Guid.h=0D
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/FirmwareManagement/BlackBo=
xTest/Guid.h=0D
@@ -67,6 +67,11 @@ extern EFI_GUID gFirmwareManagementBBTestConformanceSupp=
ortGuid003;=0D
=0D
extern EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid004;=0D
=0D
+#define EFI_TEST_FIRMWAREMANAGEMENTBBTESTCONFORMANCE_SUPPORT_005_GUID \=0D
+{ 0x4aafd29d, 0x68df, 0x49ee, {0x8a, 0xa9, 0x34, 0x7d, 0x37, 0x56, 0x65, 0=
xa7 }}=0D
+=0D
+extern EFI_GUID gFirmwareManagementBBTestConformanceSupportGuid005;=0D
+=0D
// ***********************************************************************=
*****=0D
// Conformance - Assertion=0D
// ***********************************************************************=
*****=0D
diff --git a/uefi-sct/SctPkg/UEFI/Protocol/FirmwareManagement.h b/uefi-sct/=
SctPkg/UEFI/Protocol/FirmwareManagement.h=0D
index b8876a96..c35ed3f4 100644=0D
--- a/uefi-sct/SctPkg/UEFI/Protocol/FirmwareManagement.h=0D
+++ b/uefi-sct/SctPkg/UEFI/Protocol/FirmwareManagement.h=0D
@@ -58,6 +58,22 @@ UINT64 AttributesSetting;=0D
UINT64 Compatibilities;=0D
} EFI_FIRMWARE_IMAGE_DESCRIPTOR;=0D
=0D
+typedef struct {=0D
+ ///=0D
+ /// It is included in the signature of AuthInfo. It is used to ensure fr=
eshness/no replay.=0D
+ /// It is incremented during each firmware image operation.=0D
+ ///=0D
+ UINT64 MonotonicCount;=0D
+ ///=0D
+ /// Provides the authorization for the firmware image operations. It is =
a signature across=0D
+ /// the image data and the Monotonic Count value. Caller uses the privat=
e key that is=0D
+ /// associated with a public key that has been provisioned via the key e=
xchange.=0D
+ /// Because this is defined as a signature, WIN_CERTIFICATE_UEFI_GUID.Ce=
rtType must=0D
+ /// be EFI_CERT_TYPE_PKCS7_GUID.=0D
+ ///=0D
+ WIN_CERTIFICATE_UEFI_GUID AuthInfo;=0D
+} EFI_FIRMWARE_IMAGE_AUTHENTICATION;=0D
+=0D
//=0D
// Image Attribute Definitions=0D
//=0D
@@ -79,6 +95,13 @@ UINT64 Compatibilities;=0D
=0D
#define EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION 1=0D
=0D
+//=0D
+// _WIN_CERTIFICATE.wCertificateType=0D
+//=0D
+#define WIN_CERT_TYPE_PKCS_SIGNED_DATA 0x0002=0D
+#define WIN_CERT_TYPE_EFI_PKCS115 0x0EF0=0D
+#define WIN_CERT_TYPE_EFI_GUID 0x0EF1=0D
+=0D
/*++=0D
//=0D
// Image Attribute Authentication Required=0D
-- =0D
2.26.2.windows.1=0D
=0D


Re: [Patch] BaseTools: Remove the dependency on the build intermediate file

Bob Feng
 

Liming,

Yes, it's also for other source files for AcpiTable, like *.asl.

Thanks,
Bob

-----Original Message-----
From: gaoliming <gaoliming@byosoft.com.cn>
Sent: Monday, November 2, 2020 9:33 AM
To: Feng, Bob C <bob.c.feng@intel.com>; devel@edk2.groups.io
Cc: Chen, Christine <yuwei.chen@intel.com>
Subject: 回复: [Patch] BaseTools: Remove the dependency on the build intermediate file

Bob:
So, this incremental issue happens when the source file name is changed.
Is it only for *.aslc file, or also for other file, such as *.asl?

Thanks
Liming
-----邮件原件-----
发件人: Bob Feng <bob.c.feng@intel.com>
发送时间: 2020年10月26日 20:28
收件人: devel@edk2.groups.io
抄送: Liming Gao <gaoliming@byosoft.com.cn>; Yuwei Chen
<yuwei.chen@intel.com>
主题: [Patch] BaseTools: Remove the dependency on the build intermediate
file

When generating compressed section, the build tool rely on the build
intermediate files, which were generated in last build, to get the
file list. This method will cause the incremental build to generate
incorrect build result. To reproduce this incremental build error, you
can do:
1. build Ovmf
2. change the module OvmfPkg\AcpiTables a source file Facp.aslc name
from Facp.aslc to Facpxxx.aslc.
3. change the Facp.aslc file name in [sources] section of
AcpiTables.inf 4. incremental build Ovmf

you will see the in AcpiTables module Makefile, the corresponding
Facp.acpi file is not changed.

This patch is to make the build always get file list from the INF.

Signed-off-by: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Yuwei Chen <yuwei.chen@intel.com>
---
BaseTools/Source/Python/GenFds/Section.py | 15 ---------------
1 file changed, 15 deletions(-)

diff --git a/BaseTools/Source/Python/GenFds/Section.py
b/BaseTools/Source/Python/GenFds/Section.py
index 2acb70f412..0382b2a759 100644
--- a/BaseTools/Source/Python/GenFds/Section.py
+++ b/BaseTools/Source/Python/GenFds/Section.py
@@ -139,25 +139,10 @@ class Section (SectionClassObject):
if File.Ext == Suffix:
FileList.append(File.Path)

if (not IsMakefile and Suffix is not None and
os.path.exists(FfsInf.EfiOutputPath)) or (IsMakefile and Suffix is not
None):
- #
- # Get Makefile path and time stamp
- #
- MakefileDir = FfsInf.EfiOutputPath[:-len('OUTPUT')]
- Makefile = os.path.join(MakefileDir, 'Makefile')
- if not os.path.exists(Makefile):
- Makefile = os.path.join(MakefileDir, 'GNUmakefile')
- if os.path.exists(Makefile):
- # Update to search files with suffix in all sub-dirs.
- Tuple = os.walk(FfsInf.EfiOutputPath)
- for Dirpath, Dirnames, Filenames in Tuple:
- for F in Filenames:
- if os.path.splitext(F)[1] == Suffix:
- FullName = os.path.join(Dirpath, F)
- if os.path.getmtime(FullName) >
os.path.getmtime(Makefile):
- FileList.append(FullName)
if not FileList:
SuffixMap = FfsInf.GetFinalTargetSuffixMap()
if Suffix in SuffixMap:
FileList.extend(SuffixMap[Suffix])

--
2.20.1.windows.1