[edk2] Allocating Aligned Pages

[Bhupesh Sharma]

Hi Experts,

We are using the 'AllocateAlignedPages' function (inside 
'MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c')
to allocate 512MB aligned memory chunk in a DXE driver.

However the memory chunk which gets allocated by this API is around 1GB.

I understand that this might be  because of the memory fragmentation, as a 
result of which allocating
the 512MB aligned buffer might result in memory chunk being allocated be 
greater in size than 512MB.

Are we using the right API for the purpose - is there another API available 
which allows alignment to specified
more granularly? 

Here is the 'AllocateAlignedPages' function which calls 
'InternalAllocateAlignedPages' internally.

/**
  Allocates one or more 4KB pages of type EfiBootServicesData at a specified 
alignment.

  Allocates the number of 4KB pages specified by Pages of type 
EfiBootServicesData with an
  alignment specified by Alignment.  The allocated buffer is returned.  If 
Pages is 0, then NULL is
  returned.  If there is not enough memory at the specified alignment remaining 
to satisfy the
  request, then NULL is returned.
  
  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().

  @param  Pages The number of 4 KB pages to allocate.
  @param  Alignment The requested alignment of the allocation.  
Must be a power of two.
If Alignment is zero, then byte alignment is 
used.

  @return A pointer to the allocated buffer or NULL if allocation fails.

**/
VOID *
EFIAPI
AllocateAlignedPages (
  IN UINTN  Pages,
  IN UINTN  Alignment
  )
{
  return InternalAllocateAlignedPages (EfiBootServicesData, Pages, Alignment);
}

...

/**
  Allocates one or more 4KB pages of a certain memory type at a specified 
alignment.

  Allocates the number of 4KB pages specified by Pages of a certain memory type 
with an alignment
  specified by Alignment.  The allocated buffer is returned.  If Pages is 0, 
then NULL is returned.
  If there is not enough memory at the specified alignment remaining to satisfy 
the request, then
  NULL is returned.
  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().

  @param  MemoryTypeThe type of memory to allocate.
  @param  Pages The number of 4 KB pages to allocate.
  @param  Alignment The requested alignment of the allocation.  
Must be a power of two.
If Alignment is zero, then byte alignment is 
used.

  @return A pointer to the allocated buffer or NULL if allocation fails.

**/
VOID *
InternalAllocateAlignedPages (
  IN EFI_MEMORY_TYPE  MemoryType,  
  IN UINTNPages,
  IN UINTNAlignment
  )

...

Thanks for the help.

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

Re: [edk2] [PATCH v3 1/2] MdeModulePkg: Define a general function to create DNS QName

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jiaxin Wu
> Sent: Wednesday, February 17, 2016 2:52 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan 
> Subject: [edk2] [PATCH v3 1/2] MdeModulePkg: Define a general function to
> create DNS QName
> 
> v3:
> * Fix potential overflow issue.
> 
> This patch is used to define a general function to create
> DNS QName.
> QName is a domain name represented as a sequence
> of labels, where each label consists of a length octet
> followed by that number of octets. The domain name terminates
> with the zero length octet for the null label of the root.
> 
> Cc: Hegde Nagaraj P 
> Cc: El-Haj-Mahmoud Samer 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu 
> ---
>  MdeModulePkg/Include/Library/NetLib.h  | 22 ++
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 69
> +-
>  2 files changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h
> b/MdeModulePkg/Include/Library/NetLib.h
> index e4456fa..b871a85 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -35,10 +35,12 @@ typedef UINT16  TCP_PORTNO;
>  #define  EFI_IP_PROTO_UDP  0x11
>  #define  EFI_IP_PROTO_TCP  0x06
>  #define  EFI_IP_PROTO_ICMP 0x01
>  #define  IP4_PROTO_IGMP0x02
>  #define  IP6_ICMP  58
> +#define  DNS_MAX_NAME_SIZE 255
> +#define  DNS_MAX_MESSAGE_SIZE  512
> 
>  //
>  // The address classification
>  //
>  #define  IP4_ADDR_CLASSA   1
> @@ -2154,6 +2156,26 @@ EFI_STATUS
>  EFIAPI
>  NetLibGetSystemGuid (
>OUT EFI_GUID  *SystemGuid
>);
> 
> +/**
> +  Create Dns QName according the queried domain name.
> +  QName is a domain name represented as a sequence of labels,
> +  where each label consists of a length octet followed by that
> +  number of octets. The QName terminates with the zero
> +  length octet for the null label of the root. Caller should
> +  take responsibility to free the buffer in returned pointer.
> +
> +  @param  DomainNameThe pointer to the queried domain name string.
> +
> +  @retval NULL  Failed to fill QName.
> +  @return   QName filled successfully.
> +
> +**/
> +CHAR8 *
> +EFIAPI
> +NetLibCreateDnsQName (
> +  IN  CHAR16  *DomainName
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index e112d45..ebc3e12 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -1,9 +1,9 @@
>  /** @file
>Network library.
> 
> -Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
>  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php
> @@ -3324,5 +3324,72 @@ NetLibGetSystemGuid (
>}
>  } while (TRUE);
>} while (Smbios.Raw < SmbiosEnd.Raw);
>return EFI_NOT_FOUND;
>  }
> +
> +/**
> +  Create Dns QName according the queried domain name.
> +  QName is a domain name represented as a sequence of labels,
> +  where each label consists of a length octet followed by that
> +  number of octets. The QName terminates with the zero
> +  length octet for the null label of the root. Caller should
> +  take responsibility to free the buffer in returned pointer.
> +
> +  @param  DomainNameThe pointer to the queried domain name string.
> +
> +  @retval NULL  Failed to fill QName.
> +  @return   QName filled successfully.
> +
> +**/
> +CHAR8 *
> +EFIAPI
> +NetLibCreateDnsQName (
> +  IN  CHAR16  *DomainName
> +  )
> +{
> +  CHAR8 *QueryName;
> +  UINTN QueryNameSize;
> +  CHAR8 *Header;
> +  CHAR8 *Tail;
> +  UINTN Len;
> +  UINTN Index;
> +
> +  QueryName = NULL;
> +  QueryNameSize = 0;
> +  Header= NULL;
> +  Tail  = NULL;
> +
> +  //
> +  // One byte for first label length, one byte for terminated length zero.
> +  //
> +  QueryNameSize = StrLen (DomainName) + 2;
> +
> +  if (QueryNameSize > DNS_MAX_NAME_SIZE) {
> +return NULL;
> +  }
> +
> +  QueryName = AllocateZeroPool (QueryNameSize);
> +  if (QueryName == NULL) {
> +return NULL;
> +  }
> +
> +  Header = QueryName;
> +  Tail = 

[edk2] [PATCH v2 1/2] MdeModulePkg: Define a general function to create DNS QName

[Jiaxin Wu]

v2:
* Correct function description.
* Correct max QName size.
* Add max domain name length check.

This patch is used to define a general function to create
DNS QName.
QName is a domain name represented as a sequence
of labels, where each label consists of a length octet
followed by that number of octets. The domain name terminates
with the zero length octet for the null label of the root.

Cc: Hegde Nagaraj P 
Cc: El-Haj-Mahmoud Samer 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 MdeModulePkg/Include/Library/NetLib.h  | 22 +++
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 62 +-
 2 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Include/Library/NetLib.h 
b/MdeModulePkg/Include/Library/NetLib.h
index e4456fa..b871a85 100644
--- a/MdeModulePkg/Include/Library/NetLib.h
+++ b/MdeModulePkg/Include/Library/NetLib.h
@@ -35,10 +35,12 @@ typedef UINT16  TCP_PORTNO;
 #define  EFI_IP_PROTO_UDP  0x11
 #define  EFI_IP_PROTO_TCP  0x06
 #define  EFI_IP_PROTO_ICMP 0x01
 #define  IP4_PROTO_IGMP0x02
 #define  IP6_ICMP  58
+#define  DNS_MAX_NAME_SIZE 255
+#define  DNS_MAX_MESSAGE_SIZE  512
 
 //
 // The address classification
 //
 #define  IP4_ADDR_CLASSA   1
@@ -2154,6 +2156,26 @@ EFI_STATUS
 EFIAPI
 NetLibGetSystemGuid (
   OUT EFI_GUID  *SystemGuid
   );
 
+/**
+  Create Dns QName according the queried domain name. 
+  QName is a domain name represented as a sequence of labels, 
+  where each label consists of a length octet followed by that 
+  number of octets. The QName terminates with the zero 
+  length octet for the null label of the root. Caller should 
+  take responsibility to free the buffer in returned pointer.
+
+  @param  DomainNameThe pointer to the queried domain name string.  
+
+  @retval NULL  Failed to fill QName.
+  @return   QName filled successfully.
+  
+**/ 
+CHAR8 *
+EFIAPI
+NetLibCreateDnsQName (
+  IN  CHAR16  *DomainName
+  );
+
 #endif
diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c 
b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
index e112d45..390afef 100644
--- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
+++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
@@ -1,9 +1,9 @@
 /** @file
   Network library.
 
-Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
 (C) Copyright 2015 Hewlett Packard Enterprise Development LP
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
 http://opensource.org/licenses/bsd-license.php
@@ -3324,5 +3324,65 @@ NetLibGetSystemGuid (
   }
 } while (TRUE);
   } while (Smbios.Raw < SmbiosEnd.Raw);
   return EFI_NOT_FOUND;
 }
+
+/**
+  Create Dns QName according the queried domain name. 
+  QName is a domain name represented as a sequence of labels, 
+  where each label consists of a length octet followed by that 
+  number of octets. The QName terminates with the zero 
+  length octet for the null label of the root. Caller should 
+  take responsibility to free the buffer in returned pointer.
+
+  @param  DomainNameThe pointer to the queried domain name string.  
+
+  @retval NULL  Failed to fill QName.
+  @return   QName filled successfully.
+  
+**/ 
+CHAR8 *
+EFIAPI
+NetLibCreateDnsQName (
+  IN  CHAR16  *DomainName
+  )
+{
+  CHAR8 *QueryName;
+  CHAR8 *Header;
+  CHAR8 *Tail;
+  UINTN Len;
+  UINTN Index;
+
+  QueryName  = NULL;
+  Header = NULL;
+  Tail   = NULL;
+
+  if (StrLen (DomainName) > DNS_MAX_NAME_SIZE) {
+return NULL;
+  }
+
+  QueryName = AllocateZeroPool (DNS_MAX_NAME_SIZE);
+  if (QueryName == NULL) {
+return NULL;
+  }
+  
+  Header = QueryName;
+  Tail = Header + 1;
+  Len = 0;
+  for (Index = 0; DomainName[Index] != 0; Index++) {
+*Tail = (CHAR8) DomainName[Index];
+if (*Tail == '.') {
+  *Header = (CHAR8) Len;
+  Header = Tail;
+  Tail ++;
+  Len = 0;
+} else {
+  Tail++;
+  Len++;
+}
+  }
+  *Header = (CHAR8) Len;
+  *Tail = 0;
+
+  return QueryName;
+}
\ No newline at end of file
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH v2 0/2] Expose one function defined in DnsDxe to NetLib

[Jiaxin Wu]

v2:
* Correct function description.
* Correct max QName size.
* Add max domain name length check.
* Update to use DNS_MAX_MESSAGE_SIZE.

The series of patches are used to expose one function defined in DnsDxe to 
NetLib.

Cc: Hegde Nagaraj P 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 

Jiaxin Wu (2):
  MdeModulePkg: Define a general function to create DNS QName
  NetworkPkg: Replace the internal function with exposed one

 MdeModulePkg/Include/Library/NetLib.h  | 22 +++
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 62 -
 NetworkPkg/DnsDxe/DnsImpl.c| 63 +++---
 NetworkPkg/DnsDxe/DnsImpl.h| 19 -
 NetworkPkg/DnsDxe/DnsProtocol.c|  4 +-
 5 files changed, 91 insertions(+), 79 deletions(-)

-- 
1.9.5.msysgit.1

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

[edk2] [PATCH v2 2/2] NetworkPkg: Replace the internal function with exposed one

[Jiaxin Wu]

v2:
* Update to use DNS_MAX_MESSAGE_SIZE.

This patch is used to replace the internal function with
the exposed one defined in NetLib.h.

Cc: Hegde Nagaraj P 
Cc: El-Haj-Mahmoud Samer 
Cc: Ye Ting 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
 NetworkPkg/DnsDxe/DnsImpl.c | 63 -
 NetworkPkg/DnsDxe/DnsImpl.h | 19 -
 NetworkPkg/DnsDxe/DnsProtocol.c |  4 +--
 3 files changed, 8 insertions(+), 78 deletions(-)

diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
index 617e623..1918441 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.c
+++ b/NetworkPkg/DnsDxe/DnsImpl.c
@@ -1036,65 +1036,10 @@ AddDns6ServerIp (
   
   return EFI_SUCCESS;
 }
 
 /**
-  Fill QName for IP querying. QName is a domain name represented as 
-  a sequence of labels, where each label consists of a length octet 
-  followed by that number of octets. The domain name terminates with 
-  the zero length octet for the null label of the root. Caller should 
-  take responsibility to the buffer in QName.
-
-  @param  HostName  Queried HostName
-
-  @retval NULL  Failed to fill QName.
-  @return   QName filled successfully.
-  
-**/ 
-CHAR8 *
-EFIAPI
-DnsFillinQNameForQueryIp (
-  IN  CHAR16  *HostName
-  )
-{
-  CHAR8 *QueryName;
-  CHAR8 *Header;
-  CHAR8 *Tail;
-  UINTN Len;
-  UINTN Index;
-
-  QueryName  = NULL;
-  Header = NULL;
-  Tail   = NULL;
-
-  QueryName = AllocateZeroPool (DNS_DEFAULT_BLKSIZE);
-  if (QueryName == NULL) {
-return NULL;
-  }
-  
-  Header = QueryName;
-  Tail = Header + 1;
-  Len = 0;
-  for (Index = 0; HostName[Index] != 0; Index++) {
-*Tail = (CHAR8) HostName[Index];
-if (*Tail == '.') {
-  *Header = (CHAR8) Len;
-  Header = Tail;
-  Tail ++;
-  Len = 0;
-} else {
-  Tail++;
-  Len++;
-}
-  }
-  *Header = (CHAR8) Len;
-  *Tail = 0;
-
-  return QueryName;
-}
-
-/**
   Find out whether the response is valid or invalid.
 
   @param  TokensMap   All DNS transmittal Tokens entry.  
   @param  Identification  Identification for queried packet.  
   @param  TypeType for queried packet.
@@ -1804,12 +1749,16 @@ ConstructDNSQuery (
   )
 {
   NET_FRAGMENTFrag;
   DNS_HEADER  *DnsHeader;
   DNS_QUERY_SECTION   *DnsQuery;
-  
-  Frag.Bulk = AllocatePool (DNS_DEFAULT_BLKSIZE * sizeof (UINT8));
+
+  //
+  // Messages carried by UDP are restricted to 512 bytes (not counting the IP
+  // or UDP headers).
+  //
+  Frag.Bulk = AllocatePool (DNS_MAX_MESSAGE_SIZE * sizeof (UINT8));
   if (Frag.Bulk == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
   //
diff --git a/NetworkPkg/DnsDxe/DnsImpl.h b/NetworkPkg/DnsDxe/DnsImpl.h
index 8cd73e7..0ef8255 100644
--- a/NetworkPkg/DnsDxe/DnsImpl.h
+++ b/NetworkPkg/DnsDxe/DnsImpl.h
@@ -85,11 +85,10 @@ extern EFI_DNS6_PROTOCOL mDns6Protocol;
 #define DNS_STATE_CONFIGED   1
 #define DNS_STATE_DESTROY2
 
 #define DNS_DEFAULT_TIMEOUT  2
 #define DNS_DEFAULT_RETRY3
-#define DNS_DEFAULT_BLKSIZE  512
 
 #define DNS_TIME_TO_GETMAP   5
 
 #pragma pack(1)
 
@@ -555,28 +554,10 @@ AddDns6ServerIp (
   IN LIST_ENTRY*Dns6ServerList,
   IN EFI_IPv6_ADDRESS   ServerIp
   );
 
 /**
-  Fill QName for IP querying. QName is a domain name represented as 
-  a sequence of labels, where each label consists of a length octet 
-  followed by that number of octets. The domain name terminates with 
-  the zero length octet for the null label of the root.
-
-  @param  HostName  Queried HostName
-
-  @retval NULL  Failed to fill QName.
-  @return   QName filled successfully.
-  
-**/ 
-CHAR8 *
-EFIAPI
-DnsFillinQNameForQueryIp (
-  IN  CHAR16  *HostName
-  );
-
-/**
   Find out whether the response is valid or invalid.
 
   @param  TokensMap   All DNS transmittal Tokens entry.  
   @param  Identification  Identification for queried packet.  
   @param  TypeType for queried packet.
diff --git a/NetworkPkg/DnsDxe/DnsProtocol.c b/NetworkPkg/DnsDxe/DnsProtocol.c
index f572b8b..11009fd 100644
--- a/NetworkPkg/DnsDxe/DnsProtocol.c
+++ b/NetworkPkg/DnsDxe/DnsProtocol.c
@@ -452,11 +452,11 @@ Dns4HostNameToIp (
   TokenEntry->Token = Token;
 
   //
   // Construct QName.
   //
-  QueryName = DnsFillinQNameForQueryIp (TokenEntry->QueryHostName);
+  QueryName = NetLibCreateDnsQName (TokenEntry->QueryHostName);
   if (QueryName == NULL) {
 Status = EFI_OUT_OF_RESOURCES;
 goto ON_EXIT;
   }
   
@@ -1262,11 +1262,11 @@ Dns6HostNameToIp (
 
 
   //
   // Construct QName.
   //
-  QueryName = DnsFillinQNameForQueryIp (TokenEntry->QueryHostName);
+  QueryName = NetLibCreateDnsQName 

Re: [edk2] [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

[Wu, Jiaxin]

That's fine, thanks Siyuan.

-Original Message-
From: Fu, Siyuan 
Sent: Wednesday, February 17, 2016 12:17 PM
To: Wu, Jiaxin ; edk2-devel@lists.01.org
Cc: Hegde Nagaraj P ; Ye, Ting 
Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

For public library interface, we should check the input parameters and return 
error code or NULL pointer if it's invalid, instead of use assert.

> -Original Message-
> From: Wu, Jiaxin
> Sent: Wednesday, February 17, 2016 12:09 PM
> To: Fu, Siyuan ; edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting 
> 
> Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create 
> DNS QName
> 
> Thanks Siyuan,
> 
> I don't have strong opinion about the name DNS_MAX_BLKSIZE or 
> DNS_MAX_MESSAGE_SIZE.
> 
> For comments 1 and 2, I agree the changes.
> 
> For comments 3 and 4, according RFC1035 descriptions as below:
> <<
> Various objects and parameters in the DNS have size limits. They are 
> listed below. Some could be easily changed, others are more fundamental.
> 1. labels 63 octets or less
> 2. names255 octets or less
> 3. TTL  positive values of a signed 32 bit number.
> 4. UDP messages  512 octets or less
> ...
> To simplify implementations, the total length of a domain name (i.e., 
> label octets and label length octets) is restricted to 255 octets or less.
> >>
> So, since the domain name is restricted to 255 octets or less, after 
> preprocessing, the total length must less than 512 octets or less. But 
> you are right, we'd better add a 'Assert' to assert the DomainName 
> length is restricted to 255 octets or less.
> 
> Thanks.
> Jiaxin
> 
> 
> -Original Message-
> From: Fu, Siyuan
> Sent: Wednesday, February 17, 2016 11:24 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting 
> 
> Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create 
> DNS QName
> 
> BTW, the DNS_MAX_BLKSIZE better to be DNS_MAX_MESSAGE_SIZE, according 
> to the words in RFC.
> 
> > -Original Message-
> > From: Fu, Siyuan
> > Sent: Wednesday, February 17, 2016 11:20 AM
> > To: Wu, Jiaxin ; edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P ; Ye, Ting 
> > 
> > Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create 
> > DNS QName
> >
> > Hi, Jiaxin
> >
> > Comments as below.
> > 1. In the function description of NetLibCreateDnsQName(), whether 
> > below sentence
> > The domain name terminates with the zero...
> > Should be
> > The QName terminates with the zero...
> >
> > The
> > Caller should take responsibility to the buffer in QName.
> > Should be
> > Caller should take responsibility to free the buffer in returned 
> > pointer.
> >
> > 2. The @param mismatch with the parameter name DomainName.
> >
> > 3. There will be buffer overflow if the StrSize(DomainName) > 
> > DNS_MAX_BLKSIZE in NetLibCreateDnsQName().
> >
> > 4. What the spec requirement if a label is large than 255, which 
> > exceed the max value of a UINT8?
> >
> > Best Regards
> > Siyuan
> >
> >
> >
> >
> > > -Original Message-
> > > From: Wu, Jiaxin
> > > Sent: Friday, January 22, 2016 1:54 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Hegde Nagaraj P ; Ye, Ting 
> > > ; Fu, Siyuan 
> > > Subject: [Patch 1/2] MdeModulePkg: Define one function to create 
> > > DNS QName
> > >
> > > This patch is used to define a general function to create DNS QName.
> > > QName is a domain name represented as a sequence of labels, where 
> > > each label consists of a length octet followed by that number of 
> > > octets. The domain name terminates with the zero length octet for 
> > > the null label of the root.
> > >
> > > Cc: Hegde Nagaraj P 
> > > Cc: Ye Ting 
> > > Cc: Fu Siyuan 
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Jiaxin Wu 
> > > ---
> > >  MdeModulePkg/Include/Library/NetLib.h  | 21 +++
> > >  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 58
> > > +-
> > >  2 files changed, 78 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Include/Library/NetLib.h
> > > b/MdeModulePkg/Include/Library/NetLib.h
> > > index e4456fa..a257815 100644
> > > --- a/MdeModulePkg/Include/Library/NetLib.h
> > > +++ b/MdeModulePkg/Include/Library/NetLib.h
> > > @@ -35,10 +35,11 @@ typedef UINT16  TCP_PORTNO;
> > >  #define  EFI_IP_PROTO_UDP  0x11
> > >  #define  EFI_IP_PROTO_TCP  0x06
> > >  #define  EFI_IP_PROTO_ICMP 0x01
> > >  #define  

Re: [edk2] [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

[Fu, Siyuan]

For public library interface, we should check the input parameters and return 
error code or NULL pointer if it's invalid, instead of use assert.

> -Original Message-
> From: Wu, Jiaxin
> Sent: Wednesday, February 17, 2016 12:09 PM
> To: Fu, Siyuan ; edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting
> 
> Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create DNS
> QName
> 
> Thanks Siyuan,
> 
> I don't have strong opinion about the name DNS_MAX_BLKSIZE or
> DNS_MAX_MESSAGE_SIZE.
> 
> For comments 1 and 2, I agree the changes.
> 
> For comments 3 and 4, according RFC1035 descriptions as below:
> <<
> Various objects and parameters in the DNS have size limits. They are listed
> below. Some could be easily changed, others are more fundamental.
> 1. labels 63 octets or less
> 2. names255 octets or less
> 3. TTL  positive values of a signed 32 bit number.
> 4. UDP messages  512 octets or less
> ...
> To simplify implementations, the total length of a domain name (i.e., label
> octets and label length octets) is restricted to 255 octets or less.
> >>
> So, since the domain name is restricted to 255 octets or less, after
> preprocessing, the total length must less than 512 octets or less. But you are
> right, we'd better add a 'Assert' to assert the DomainName length is
> restricted to 255 octets or less.
> 
> Thanks.
> Jiaxin
> 
> 
> -Original Message-
> From: Fu, Siyuan
> Sent: Wednesday, February 17, 2016 11:24 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting
> 
> Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create DNS
> QName
> 
> BTW, the DNS_MAX_BLKSIZE better to be DNS_MAX_MESSAGE_SIZE,
> according to the words in RFC.
> 
> > -Original Message-
> > From: Fu, Siyuan
> > Sent: Wednesday, February 17, 2016 11:20 AM
> > To: Wu, Jiaxin ; edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P ; Ye, Ting
> > 
> > Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create
> > DNS QName
> >
> > Hi, Jiaxin
> >
> > Comments as below.
> > 1. In the function description of NetLibCreateDnsQName(), whether
> > below sentence
> > The domain name terminates with the zero...
> > Should be
> > The QName terminates with the zero...
> >
> > The
> > Caller should take responsibility to the buffer in QName.
> > Should be
> > Caller should take responsibility to free the buffer in returned 
> > pointer.
> >
> > 2. The @param mismatch with the parameter name DomainName.
> >
> > 3. There will be buffer overflow if the StrSize(DomainName) >
> > DNS_MAX_BLKSIZE in NetLibCreateDnsQName().
> >
> > 4. What the spec requirement if a label is large than 255, which
> > exceed the max value of a UINT8?
> >
> > Best Regards
> > Siyuan
> >
> >
> >
> >
> > > -Original Message-
> > > From: Wu, Jiaxin
> > > Sent: Friday, January 22, 2016 1:54 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Hegde Nagaraj P ; Ye, Ting
> > > ; Fu, Siyuan 
> > > Subject: [Patch 1/2] MdeModulePkg: Define one function to create DNS
> > > QName
> > >
> > > This patch is used to define a general function to create DNS QName.
> > > QName is a domain name represented as a sequence of labels, where
> > > each label consists of a length octet followed by that number of
> > > octets. The domain name terminates with the zero length octet for
> > > the null label of the root.
> > >
> > > Cc: Hegde Nagaraj P 
> > > Cc: Ye Ting 
> > > Cc: Fu Siyuan 
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Jiaxin Wu 
> > > ---
> > >  MdeModulePkg/Include/Library/NetLib.h  | 21 +++
> > >  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 58
> > > +-
> > >  2 files changed, 78 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/MdeModulePkg/Include/Library/NetLib.h
> > > b/MdeModulePkg/Include/Library/NetLib.h
> > > index e4456fa..a257815 100644
> > > --- a/MdeModulePkg/Include/Library/NetLib.h
> > > +++ b/MdeModulePkg/Include/Library/NetLib.h
> > > @@ -35,10 +35,11 @@ typedef UINT16  TCP_PORTNO;
> > >  #define  EFI_IP_PROTO_UDP  0x11
> > >  #define  EFI_IP_PROTO_TCP  0x06
> > >  #define  EFI_IP_PROTO_ICMP 0x01
> > >  #define  IP4_PROTO_IGMP0x02
> > >  #define  IP6_ICMP  58
> > > +#define  DNS_MAX_BLKSIZE   512
> > >
> > >  //
> > >  // The address classification
> > >  //
> > >  #define  IP4_ADDR_CLASSA   1
> > > @@ -2154,6 +2155,26 @@ EFI_STATUS
> > >  EFIAPI
> > >  NetLibGetSystemGuid (
> > >OUT EFI_GUID  *SystemGuid
> > >);
> 

Re: [edk2] [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

[Wu, Jiaxin]

Thanks Siyuan,

I don't have strong opinion about the name DNS_MAX_BLKSIZE or 
DNS_MAX_MESSAGE_SIZE.

For comments 1 and 2, I agree the changes.

For comments 3 and 4, according RFC1035 descriptions as below:
<<
Various objects and parameters in the DNS have size limits. They are listed 
below. Some could be easily changed, others are more fundamental.
1. labels 63 octets or less
2. names255 octets or less
3. TTL  positive values of a signed 32 bit number.
4. UDP messages  512 octets or less
...
To simplify implementations, the total length of a domain name (i.e., label 
octets and label length octets) is restricted to 255 octets or less.
>>
So, since the domain name is restricted to 255 octets or less, after 
preprocessing, the total length must less than 512 octets or less. But you are 
right, we'd better add a 'Assert' to assert the DomainName length is restricted 
to 255 octets or less.

Thanks.
Jiaxin
 

-Original Message-
From: Fu, Siyuan 
Sent: Wednesday, February 17, 2016 11:24 AM
To: Wu, Jiaxin ; edk2-devel@lists.01.org
Cc: Hegde Nagaraj P ; Ye, Ting 
Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

BTW, the DNS_MAX_BLKSIZE better to be DNS_MAX_MESSAGE_SIZE, according to the 
words in RFC.

> -Original Message-
> From: Fu, Siyuan
> Sent: Wednesday, February 17, 2016 11:20 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting 
> 
> Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create 
> DNS QName
> 
> Hi, Jiaxin
> 
> Comments as below.
> 1. In the function description of NetLibCreateDnsQName(), whether 
> below sentence
>   The domain name terminates with the zero...
> Should be
>   The QName terminates with the zero...
> 
> The
>   Caller should take responsibility to the buffer in QName.
> Should be
>   Caller should take responsibility to free the buffer in returned 
> pointer.
> 
> 2. The @param mismatch with the parameter name DomainName.
> 
> 3. There will be buffer overflow if the StrSize(DomainName) > 
> DNS_MAX_BLKSIZE in NetLibCreateDnsQName().
> 
> 4. What the spec requirement if a label is large than 255, which 
> exceed the max value of a UINT8?
> 
> Best Regards
> Siyuan
> 
> 
> 
> 
> > -Original Message-
> > From: Wu, Jiaxin
> > Sent: Friday, January 22, 2016 1:54 AM
> > To: edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P ; Ye, Ting 
> > ; Fu, Siyuan 
> > Subject: [Patch 1/2] MdeModulePkg: Define one function to create DNS 
> > QName
> >
> > This patch is used to define a general function to create DNS QName.
> > QName is a domain name represented as a sequence of labels, where 
> > each label consists of a length octet followed by that number of 
> > octets. The domain name terminates with the zero length octet for 
> > the null label of the root.
> >
> > Cc: Hegde Nagaraj P 
> > Cc: Ye Ting 
> > Cc: Fu Siyuan 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu 
> > ---
> >  MdeModulePkg/Include/Library/NetLib.h  | 21 +++
> >  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 58
> > +-
> >  2 files changed, 78 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Include/Library/NetLib.h
> > b/MdeModulePkg/Include/Library/NetLib.h
> > index e4456fa..a257815 100644
> > --- a/MdeModulePkg/Include/Library/NetLib.h
> > +++ b/MdeModulePkg/Include/Library/NetLib.h
> > @@ -35,10 +35,11 @@ typedef UINT16  TCP_PORTNO;
> >  #define  EFI_IP_PROTO_UDP  0x11
> >  #define  EFI_IP_PROTO_TCP  0x06
> >  #define  EFI_IP_PROTO_ICMP 0x01
> >  #define  IP4_PROTO_IGMP0x02
> >  #define  IP6_ICMP  58
> > +#define  DNS_MAX_BLKSIZE   512
> >
> >  //
> >  // The address classification
> >  //
> >  #define  IP4_ADDR_CLASSA   1
> > @@ -2154,6 +2155,26 @@ EFI_STATUS
> >  EFIAPI
> >  NetLibGetSystemGuid (
> >OUT EFI_GUID  *SystemGuid
> >);
> >
> > +/**
> > +  Create Dns QName according the queried domain name.
> > +  QName is a domain name represented as a sequence of labels,
> > +  where each label consists of a length octet followed by that
> > +  number of octets. The domain name terminates with the zero
> > +  length octet for the null label of the root. Caller should
> > +  take responsibility to the buffer in QName.
> > +
> > +  @param  StringThe pointer to the queried Ascii string.
> > +
> > +  @retval NULL  Failed to fill QName.
> > +  @return   QName filled successfully.
> > +
> > +**/
> > +CHAR8 *
> > +EFIAPI
> > +NetLibCreateDnsQName (
> > +  IN  CHAR16  *DomainName
> > +  

Re: [edk2] [Patch 2/2] NetworkPkg: Replace the internal function with exposed one

[Fu, Siyuan]

Reviewed-by: Fu Siyuan 

> -Original Message-
> From: Wu, Jiaxin
> Sent: Friday, January 22, 2016 1:54 AM
> To: edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting
> ; Fu, Siyuan 
> Subject: [Patch 2/2] NetworkPkg: Replace the internal function with exposed
> one
> 
> This patch is used to replace the internal function with
> the exposed one defined in NetLib.h.
> 
> Cc: Hegde Nagaraj P 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu 
> ---
>  NetworkPkg/DnsDxe/DnsImpl.c | 63 
> -
>  NetworkPkg/DnsDxe/DnsImpl.h | 19 -
>  NetworkPkg/DnsDxe/DnsProtocol.c |  6 ++--
>  3 files changed, 9 insertions(+), 79 deletions(-)
> 
> diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c
> index 71dacce..77e6496 100644
> --- a/NetworkPkg/DnsDxe/DnsImpl.c
> +++ b/NetworkPkg/DnsDxe/DnsImpl.c
> @@ -1012,65 +1012,10 @@ AddDns6ServerIp (
> 
>return EFI_SUCCESS;
>  }
> 
>  /**
> -  Fill QName for IP querying. QName is a domain name represented as
> -  a sequence of labels, where each label consists of a length octet
> -  followed by that number of octets. The domain name terminates with
> -  the zero length octet for the null label of the root. Caller should
> -  take responsibility to the buffer in QName.
> -
> -  @param  HostName  Queried HostName
> -
> -  @retval NULL  Failed to fill QName.
> -  @return   QName filled successfully.
> -
> -**/
> -CHAR8 *
> -EFIAPI
> -DnsFillinQNameForQueryIp (
> -  IN  CHAR16  *HostName
> -  )
> -{
> -  CHAR8 *QueryName;
> -  CHAR8 *Header;
> -  CHAR8 *Tail;
> -  UINTN Len;
> -  UINTN Index;
> -
> -  QueryName  = NULL;
> -  Header = NULL;
> -  Tail   = NULL;
> -
> -  QueryName = AllocateZeroPool (DNS_DEFAULT_BLKSIZE);
> -  if (QueryName == NULL) {
> -return NULL;
> -  }
> -
> -  Header = QueryName;
> -  Tail = Header + 1;
> -  Len = 0;
> -  for (Index = 0; HostName[Index] != 0; Index++) {
> -*Tail = (CHAR8) HostName[Index];
> -if (*Tail == '.') {
> -  *Header = (CHAR8) Len;
> -  Header = Tail;
> -  Tail ++;
> -  Len = 0;
> -} else {
> -  Tail++;
> -  Len++;
> -}
> -  }
> -  *Header = (CHAR8) Len;
> -  *Tail = 0;
> -
> -  return QueryName;
> -}
> -
> -/**
>Find out whether the response is valid or invalid.
> 
>@param  TokensMap   All DNS transmittal Tokens entry.
>@param  Identification  Identification for queried packet.
>@param  TypeType for queried packet.
> @@ -1780,12 +1725,16 @@ ConstructDNSQuery (
>)
>  {
>NET_FRAGMENTFrag;
>DNS_HEADER  *DnsHeader;
>DNS_QUERY_SECTION   *DnsQuery;
> -
> -  Frag.Bulk = AllocatePool (DNS_DEFAULT_BLKSIZE * sizeof (UINT8));
> +
> +  //
> +  // Messages carried by UDP are restricted to 512 bytes (not counting the IP
> +  // or UDP headers).
> +  //
> +  Frag.Bulk = AllocatePool (DNS_MAX_BLKSIZE * sizeof (UINT8));
>if (Frag.Bulk == NULL) {
>  return EFI_OUT_OF_RESOURCES;
>}
> 
>//
> diff --git a/NetworkPkg/DnsDxe/DnsImpl.h b/NetworkPkg/DnsDxe/DnsImpl.h
> index 72b85cb..e286064 100644
> --- a/NetworkPkg/DnsDxe/DnsImpl.h
> +++ b/NetworkPkg/DnsDxe/DnsImpl.h
> @@ -85,11 +85,10 @@ extern EFI_DNS6_PROTOCOL mDns6Protocol;
>  #define DNS_STATE_CONFIGED   1
>  #define DNS_STATE_DESTROY2
> 
>  #define DNS_DEFAULT_TIMEOUT  2
>  #define DNS_DEFAULT_RETRY3
> -#define DNS_DEFAULT_BLKSIZE  512
> 
>  #define DNS_TIME_TO_GETMAP   5
> 
>  #pragma pack(1)
> 
> @@ -555,28 +554,10 @@ AddDns6ServerIp (
>IN LIST_ENTRY*Dns6ServerList,
>IN EFI_IPv6_ADDRESS   ServerIp
>);
> 
>  /**
> -  Fill QName for IP querying. QName is a domain name represented as
> -  a sequence of labels, where each label consists of a length octet
> -  followed by that number of octets. The domain name terminates with
> -  the zero length octet for the null label of the root.
> -
> -  @param  HostName  Queried HostName
> -
> -  @retval NULL  Failed to fill QName.
> -  @return   QName filled successfully.
> -
> -**/
> -CHAR8 *
> -EFIAPI
> -DnsFillinQNameForQueryIp (
> -  IN  CHAR16  *HostName
> -  );
> -
> -/**
>Find out whether the response is valid or invalid.
> 
>@param  TokensMap   All DNS transmittal Tokens entry.
>@param  Identification  Identification for queried packet.
>@param  TypeType for queried packet.
> diff --git a/NetworkPkg/DnsDxe/DnsProtocol.c
> b/NetworkPkg/DnsDxe/DnsProtocol.c
> index a3f3de9..3093535 100644
> --- a/NetworkPkg/DnsDxe/DnsProtocol.c
> +++ 

Re: [edk2] [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

[Fu, Siyuan]

BTW, the DNS_MAX_BLKSIZE better to be DNS_MAX_MESSAGE_SIZE, according to the 
words in RFC.

> -Original Message-
> From: Fu, Siyuan
> Sent: Wednesday, February 17, 2016 11:20 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting
> 
> Subject: RE: [Patch 1/2] MdeModulePkg: Define one function to create DNS
> QName
> 
> Hi, Jiaxin
> 
> Comments as below.
> 1. In the function description of NetLibCreateDnsQName(), whether below
> sentence
>   The domain name terminates with the zero...
> Should be
>   The QName terminates with the zero...
> 
> The
>   Caller should take responsibility to the buffer in QName.
> Should be
>   Caller should take responsibility to free the buffer in returned 
> pointer.
> 
> 2. The @param mismatch with the parameter name DomainName.
> 
> 3. There will be buffer overflow if the StrSize(DomainName) >
> DNS_MAX_BLKSIZE in NetLibCreateDnsQName().
> 
> 4. What the spec requirement if a label is large than 255, which exceed the
> max value of a UINT8?
> 
> Best Regards
> Siyuan
> 
> 
> 
> 
> > -Original Message-
> > From: Wu, Jiaxin
> > Sent: Friday, January 22, 2016 1:54 AM
> > To: edk2-devel@lists.01.org
> > Cc: Hegde Nagaraj P ; Ye, Ting
> > ; Fu, Siyuan 
> > Subject: [Patch 1/2] MdeModulePkg: Define one function to create DNS
> > QName
> >
> > This patch is used to define a general function to create
> > DNS QName.
> > QName is a domain name represented as a sequence
> > of labels, where each label consists of a length octet
> > followed by that number of octets. The domain name terminates
> > with the zero length octet for the null label of the root.
> >
> > Cc: Hegde Nagaraj P 
> > Cc: Ye Ting 
> > Cc: Fu Siyuan 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu 
> > ---
> >  MdeModulePkg/Include/Library/NetLib.h  | 21 +++
> >  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 58
> > +-
> >  2 files changed, 78 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Include/Library/NetLib.h
> > b/MdeModulePkg/Include/Library/NetLib.h
> > index e4456fa..a257815 100644
> > --- a/MdeModulePkg/Include/Library/NetLib.h
> > +++ b/MdeModulePkg/Include/Library/NetLib.h
> > @@ -35,10 +35,11 @@ typedef UINT16  TCP_PORTNO;
> >  #define  EFI_IP_PROTO_UDP  0x11
> >  #define  EFI_IP_PROTO_TCP  0x06
> >  #define  EFI_IP_PROTO_ICMP 0x01
> >  #define  IP4_PROTO_IGMP0x02
> >  #define  IP6_ICMP  58
> > +#define  DNS_MAX_BLKSIZE   512
> >
> >  //
> >  // The address classification
> >  //
> >  #define  IP4_ADDR_CLASSA   1
> > @@ -2154,6 +2155,26 @@ EFI_STATUS
> >  EFIAPI
> >  NetLibGetSystemGuid (
> >OUT EFI_GUID  *SystemGuid
> >);
> >
> > +/**
> > +  Create Dns QName according the queried domain name.
> > +  QName is a domain name represented as a sequence of labels,
> > +  where each label consists of a length octet followed by that
> > +  number of octets. The domain name terminates with the zero
> > +  length octet for the null label of the root. Caller should
> > +  take responsibility to the buffer in QName.
> > +
> > +  @param  StringThe pointer to the queried Ascii string.
> > +
> > +  @retval NULL  Failed to fill QName.
> > +  @return   QName filled successfully.
> > +
> > +**/
> > +CHAR8 *
> > +EFIAPI
> > +NetLibCreateDnsQName (
> > +  IN  CHAR16  *DomainName
> > +  );
> > +
> >  #endif
> > diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > index e112d45..dd67a1c 100644
> > --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > @@ -1,9 +1,9 @@
> >  /** @file
> >Network library.
> >
> > -Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
> > +Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> >  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
> >  This program and the accompanying materials
> >  are licensed and made available under the terms and conditions of the
> BSD
> > License
> >  which accompanies this distribution.  The full text of the license may be
> > found at
> >  http://opensource.org/licenses/bsd-license.php
> > @@ -3324,5 +3324,61 @@ NetLibGetSystemGuid (
> >}
> >  } while (TRUE);
> >} while (Smbios.Raw < SmbiosEnd.Raw);
> >return EFI_NOT_FOUND;
> >  }
> > +
> > +/**
> > +  Create Dns QName according the queried domain name.
> > +  QName is a domain name represented as a sequence of labels,
> > +  where each label consists of a length octet followed by that
> > +  number of octets. The domain name terminates with the zero
> > +  length octet for the null 

Re: [edk2] [Patch 1/2] MdeModulePkg: Define one function to create DNS QName

[Fu, Siyuan]

Hi, Jiaxin

Comments as below.
1. In the function description of NetLibCreateDnsQName(), whether below sentence
The domain name terminates with the zero...
Should be
The QName terminates with the zero...

The
Caller should take responsibility to the buffer in QName.
Should be 
Caller should take responsibility to free the buffer in returned 
pointer.

2. The @param mismatch with the parameter name DomainName. 

3. There will be buffer overflow if the StrSize(DomainName) > DNS_MAX_BLKSIZE 
in NetLibCreateDnsQName().

4. What the spec requirement if a label is large than 255, which exceed the max 
value of a UINT8?

Best Regards
Siyuan




> -Original Message-
> From: Wu, Jiaxin
> Sent: Friday, January 22, 2016 1:54 AM
> To: edk2-devel@lists.01.org
> Cc: Hegde Nagaraj P ; Ye, Ting
> ; Fu, Siyuan 
> Subject: [Patch 1/2] MdeModulePkg: Define one function to create DNS
> QName
> 
> This patch is used to define a general function to create
> DNS QName.
> QName is a domain name represented as a sequence
> of labels, where each label consists of a length octet
> followed by that number of octets. The domain name terminates
> with the zero length octet for the null label of the root.
> 
> Cc: Hegde Nagaraj P 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu 
> ---
>  MdeModulePkg/Include/Library/NetLib.h  | 21 +++
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 58
> +-
>  2 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h
> b/MdeModulePkg/Include/Library/NetLib.h
> index e4456fa..a257815 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -35,10 +35,11 @@ typedef UINT16  TCP_PORTNO;
>  #define  EFI_IP_PROTO_UDP  0x11
>  #define  EFI_IP_PROTO_TCP  0x06
>  #define  EFI_IP_PROTO_ICMP 0x01
>  #define  IP4_PROTO_IGMP0x02
>  #define  IP6_ICMP  58
> +#define  DNS_MAX_BLKSIZE   512
> 
>  //
>  // The address classification
>  //
>  #define  IP4_ADDR_CLASSA   1
> @@ -2154,6 +2155,26 @@ EFI_STATUS
>  EFIAPI
>  NetLibGetSystemGuid (
>OUT EFI_GUID  *SystemGuid
>);
> 
> +/**
> +  Create Dns QName according the queried domain name.
> +  QName is a domain name represented as a sequence of labels,
> +  where each label consists of a length octet followed by that
> +  number of octets. The domain name terminates with the zero
> +  length octet for the null label of the root. Caller should
> +  take responsibility to the buffer in QName.
> +
> +  @param  StringThe pointer to the queried Ascii string.
> +
> +  @retval NULL  Failed to fill QName.
> +  @return   QName filled successfully.
> +
> +**/
> +CHAR8 *
> +EFIAPI
> +NetLibCreateDnsQName (
> +  IN  CHAR16  *DomainName
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index e112d45..dd67a1c 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -1,9 +1,9 @@
>  /** @file
>Network library.
> 
> -Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
>  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
>  which accompanies this distribution.  The full text of the license may be
> found at
>  http://opensource.org/licenses/bsd-license.php
> @@ -3324,5 +3324,61 @@ NetLibGetSystemGuid (
>}
>  } while (TRUE);
>} while (Smbios.Raw < SmbiosEnd.Raw);
>return EFI_NOT_FOUND;
>  }
> +
> +/**
> +  Create Dns QName according the queried domain name.
> +  QName is a domain name represented as a sequence of labels,
> +  where each label consists of a length octet followed by that
> +  number of octets. The domain name terminates with the zero
> +  length octet for the null label of the root. Caller should
> +  take responsibility to the buffer in QName.
> +
> +  @param  StringThe pointer to the queried Ascii string.
> +
> +  @retval NULL  Failed to fill QName.
> +  @return   QName filled successfully.
> +
> +**/
> +CHAR8 *
> +EFIAPI
> +NetLibCreateDnsQName (
> +  IN  CHAR16  *DomainName
> +  )
> +{
> +  CHAR8 *QueryName;
> +  CHAR8 *Header;
> +  CHAR8 *Tail;
> +  UINTN Len;
> +  UINTN Index;
> +
> +  QueryName  = NULL;
> +  Header = NULL;
> +  Tail   = NULL;
> +
> +  QueryName = AllocateZeroPool 

Re: [edk2] Using PCD to set default policy for IPv4/IPv6

[Wu, Jiaxin]

Mike,
Thanks your comments.
First, I agree other platform modules can also change the police by using the 
set operation.
Second for the complete design, the proposed for the new PCDs will only be 
consumed by Ip4Dxe/Ip6Dxe modules. The PCDs are used by these two modules to 
determine the default policy, the current Ip4Dxe/Ip6Dxe driver will  make 
decision which more configuration information needed to be also configured. So, 
I think this a complete design to cover the IPv4/Ipv6 configuration protocol.

Thanks.
Jiaxin

From: Kinney, Michael D
Sent: Wednesday, February 17, 2016 6:10 AM
To: Wu, Jiaxin ; Subramanian, Sriram (EG Servers Platform 
SW) ; El-Haj-Mahmoud, Samer ; 
Hegde, Nagaraj P ; Zimmer, Vincent 
; Li, Ruth ; Ye, Ting 
; Fu, Siyuan ; edk2-devel@lists.01.org; 
Kinney, Michael D 
Subject: RE: Using PCD to set default policy for IPv4/IPv6

Jiaxin,

This proposal looks incomplete to me.  If the configuration policy is set to 
Ip4Config2PolicyStatic or Ip6ConfigPolicyManual, then it seems like a lot more 
configuration information would be required for the static or manual policies.

Can you also provide details on the complete design.  What modules are you 
proposing would use these new PCDs?  These configuration protocols provide 
get/set operations.  Why can't a platform module use the set operation to set 
static/manual policy along with the additional set operations to completely 
configure static/manual policy?

Thanks,

Mike

From: Wu, Jiaxin
Sent: Monday, February 15, 2016 7:37 PM
To: Subramanian, Sriram (EG Servers Platform SW) 
>; El-Haj-Mahmoud, Samer 
>; Hegde, 
Nagaraj P >; Zimmer, 
Vincent >; Kinney, 
Michael D >; Li, 
Ruth >; Ye, Ting 
>; Fu, Siyuan 
>; 
edk2-devel@lists.01.org
Subject: Using PCD to set default policy for IPv4/IPv6

Hi all,

Below is the descriptions about the default policy for IPv4/IPv6 in latest UEFI 
Spec (Version 2.6).

<< P1472: The EFI_IP4_CONFIG2_POLICY defines the general configuration policy 
the EFI IPv4 Configuration II Protocol supports. The default policy for a newly 
detected communication device is beyond the scope of this document. An 
implementation might leave it to platform to choose the default policy.>>

<< P1510: The EFI_IP6_CONFIG_POLICY defines the general configuration policy 
the EFI IPv6 Configuration Protocol supports. The default policy for a newly 
detected communication device is beyond the scope of this document. An 
implementation might leave it to platform to choose the default policy.>>

So, I propose to introduce PCD to leave it to platform to choose the default 
policy. That's meaningful to make the source code consistent with UEFI Spec. 
Detailed see below:

## Ip4Config2 Policy Type configuration.
# 01 = The configuration policy is Ip4Config2PolicyStatic
# 02 = The configuration policy is Ip4Config2PolicyDhcp
# @Prompt Type Value of Ip4Config2 Policy.
gEfiMdeModulePkgTokenSpaceGuid.PcdIp4Config2PolicyType|2|UINT8|0x1002

## Ip6Config Policy Type configuration.
# 01 = The configuration policy is Ip6ConfigPolicyManual
# 02 = The configuration policy is Ip6ConfigPolicyAutomatic
# @Prompt Type Value of Ip6Config Policy.
gEfiNetworkPkgTokenSpaceGuid.PcdIp6ConfigPolicyType|2|UINT8|0x1002

Do you have any opinion? If there is no objection, I will create the 
corresponding patches to fix it.

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

Re: [edk2] Using PCD to set default policy for IPv4/IPv6

[Kinney, Michael D]

Jiaxin,

This proposal looks incomplete to me.  If the configuration policy is set to 
Ip4Config2PolicyStatic or Ip6ConfigPolicyManual, then it seems like a lot more 
configuration information would be required for the static or manual policies.

Can you also provide details on the complete design.  What modules are you 
proposing would use these new PCDs?  These configuration protocols provide 
get/set operations.  Why can't a platform module use the set operation to set 
static/manual policy along with the additional set operations to completely 
configure static/manual policy?

Thanks,

Mike

From: Wu, Jiaxin
Sent: Monday, February 15, 2016 7:37 PM
To: Subramanian, Sriram (EG Servers Platform SW) ; 
El-Haj-Mahmoud, Samer ; Hegde, Nagaraj P 
; Zimmer, Vincent ; Kinney, 
Michael D ; Li, Ruth ; Ye, Ting 
; Fu, Siyuan ; edk2-devel@lists.01.org
Subject: Using PCD to set default policy for IPv4/IPv6

Hi all,

Below is the descriptions about the default policy for IPv4/IPv6 in latest UEFI 
Spec (Version 2.6).

<< P1472: The EFI_IP4_CONFIG2_POLICY defines the general configuration policy 
the EFI IPv4 Configuration II Protocol supports. The default policy for a newly 
detected communication device is beyond the scope of this document. An 
implementation might leave it to platform to choose the default policy.>>

<< P1510: The EFI_IP6_CONFIG_POLICY defines the general configuration policy 
the EFI IPv6 Configuration Protocol supports. The default policy for a newly 
detected communication device is beyond the scope of this document. An 
implementation might leave it to platform to choose the default policy.>>

So, I propose to introduce PCD to leave it to platform to choose the default 
policy. That's meaningful to make the source code consistent with UEFI Spec. 
Detailed see below:

## Ip4Config2 Policy Type configuration.
# 01 = The configuration policy is Ip4Config2PolicyStatic
# 02 = The configuration policy is Ip4Config2PolicyDhcp
# @Prompt Type Value of Ip4Config2 Policy.
gEfiMdeModulePkgTokenSpaceGuid.PcdIp4Config2PolicyType|2|UINT8|0x1002

## Ip6Config Policy Type configuration.
# 01 = The configuration policy is Ip6ConfigPolicyManual
# 02 = The configuration policy is Ip6ConfigPolicyAutomatic
# @Prompt Type Value of Ip6Config Policy.
gEfiNetworkPkgTokenSpaceGuid.PcdIp6ConfigPolicyType|2|UINT8|0x1002

Do you have any opinion? If there is no objection, I will create the 
corresponding patches to fix it.

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

Re: [edk2] [PATCH] BaseTools: LzmaCompress: eliminate _maxMode and bogus indentation

[Jordan Justen]

On 2016-02-15 10:47:28, Laszlo Ersek wrote:
> The "_maxMode" variable doesn't exist in edk2's variant of LzmaCompress,
> but the way one of the old uses of the variable is commented out (i.e.,
> together with the enclosing "if" statement) triggers the
> "misleading-indentation" warning that is new in gcc-6.0, for the block of
> code that originally depended on the "if" statement. Gcc believes
> (mistakenly) that the programmer believes (mistakenly) that the block
> depends on (repIndex == 0) higher up.
> 
> Remove the commented out uses of "_maxMode", and unindent the block in
> question.
> 
> This patch is best viewed with "git show -b".
> 
> Cc: Cole Robinson 
> Cc: Yonghong Zhu 
> Cc: Liming Gao 
> Reported-by: Cole Robinson 
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1307439
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c | 81 
> -
>  1 file changed, 40 insertions(+), 41 deletions(-)
> 
> diff --git a/BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c 
> b/BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c
> index 9b2dd16ffa48..1eb9898b5291 100644
> --- a/BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c
> +++ b/BaseTools/Source/C/LzmaCompress/Sdk/C/LzmaEnc.c
> @@ -1367,52 +1367,51 @@ static UInt32 GetOptimum(CLzmaEnc *p, UInt32 
> position, UInt32 *backRes)
>if (repIndex == 0)
>  startLen = lenTest + 1;
>  
> -  /* if (_maxMode) */

This also seems to exist as-is in the LZMA SDK code. So, it is not an
EDK II deviation.

One of the things I tried to do with incorporating the LZMA SDK was
minimize any changes to LZMA SDK files. This would hopefully make it
easier to merge in newer versions of the SDK. (Which, of course, we've
never done.)

Nevertheless, what about just making this change instead:

-  /* if (_maxMode) */
+  if (1 /* _maxMode */)

-Jordan

> +  {
> +UInt32 lenTest2 = lenTest + 1;
> +UInt32 limit = lenTest2 + p->numFastBytes;
> +UInt32 nextRepMatchPrice;
> +if (limit > numAvailFull)
> +  limit = numAvailFull;
> +for (; lenTest2 < limit && data[lenTest2] == data2[lenTest2]; 
> lenTest2++);
> +lenTest2 -= lenTest + 1;
> +if (lenTest2 >= 2)
>  {
> -  UInt32 lenTest2 = lenTest + 1;
> -  UInt32 limit = lenTest2 + p->numFastBytes;
> -  UInt32 nextRepMatchPrice;
> -  if (limit > numAvailFull)
> -limit = numAvailFull;
> -  for (; lenTest2 < limit && data[lenTest2] == data2[lenTest2]; 
> lenTest2++);
> -  lenTest2 -= lenTest + 1;
> -  if (lenTest2 >= 2)
> +  UInt32 state2 = kRepNextStates[state];
> +  UInt32 posStateNext = (position + lenTest) & p->pbMask;
> +  UInt32 curAndLenCharPrice =
> +  price + p->repLenEnc.prices[posState][lenTest - 2] +
> +  GET_PRICE_0(p->isMatch[state2][posStateNext]) +
> +  LitEnc_GetPriceMatched(LIT_PROBS(position + lenTest, 
> data[lenTest - 1]),
> +  data[lenTest], data2[lenTest], p->ProbPrices);
> +  state2 = kLiteralNextStates[state2];
> +  posStateNext = (position + lenTest + 1) & p->pbMask;
> +  nextRepMatchPrice = curAndLenCharPrice +
> +  GET_PRICE_1(p->isMatch[state2][posStateNext]) +
> +  GET_PRICE_1(p->isRep[state2]);
> +
> +  /* for (; lenTest2 >= 2; lenTest2--) */
>{
> -UInt32 state2 = kRepNextStates[state];
> -UInt32 posStateNext = (position + lenTest) & p->pbMask;
> -UInt32 curAndLenCharPrice =
> -price + p->repLenEnc.prices[posState][lenTest - 2] +
> -GET_PRICE_0(p->isMatch[state2][posStateNext]) +
> -LitEnc_GetPriceMatched(LIT_PROBS(position + lenTest, 
> data[lenTest - 1]),
> -data[lenTest], data2[lenTest], p->ProbPrices);
> -state2 = kLiteralNextStates[state2];
> -posStateNext = (position + lenTest + 1) & p->pbMask;
> -nextRepMatchPrice = curAndLenCharPrice +
> -GET_PRICE_1(p->isMatch[state2][posStateNext]) +
> -GET_PRICE_1(p->isRep[state2]);
> -
> -/* for (; lenTest2 >= 2; lenTest2--) */
> +UInt32 curAndLenPrice;
> +COptimal *opt;
> +UInt32 offset = cur + lenTest + 1 + lenTest2;
> +while (lenEnd < offset)
> +  p->opt[++lenEnd].price = kInfinityPrice;
> +curAndLenPrice = nextRepMatchPrice + GetRepPrice(p, 0, lenTest2, 
> state2, posStateNext);
> +opt = >opt[offset];
> +if (curAndLenPrice < opt->price)
>  {
> -  UInt32 curAndLenPrice;
> -  COptimal *opt;
> -  UInt32 offset = 

[edk2] [PATCH v1 1/1] StdLib/BsdSocketLib: Fix minor memory leak.

[Daryl McDaniel]

Fixes a minor memory leak in function res_mkupdrec() by
freeing rrecp on error return.

The error return is triggered by one of two conditions:
  1.  rrecp is NULL (calloc failed)
  2.  strdup(dname) returns NULL

Previously, the function just returned NULL.  This patch adds a call to
free rrecp before returning NULL.  Since the free() function will properly
do nothing when called with a NULL parameter, it is not necessary to
separate the two tests into separate if clauses.

Reported-by: Colin King 

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daryl McDaniel 
---
 StdLib/BsdSocketLib/res_mkupdate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/StdLib/BsdSocketLib/res_mkupdate.c 
b/StdLib/BsdSocketLib/res_mkupdate.c
index d81d7d6f1518..db8540ab4b85 100644
--- a/StdLib/BsdSocketLib/res_mkupdate.c
+++ b/StdLib/BsdSocketLib/res_mkupdate.c
@@ -438,8 +438,10 @@ res_mkupdrec(int section, const char *dname,
  u_int class, u_int type, u_long ttl) {
 ns_updrec *rrecp = (ns_updrec *)calloc(1, sizeof(ns_updrec));

-if (!rrecp || !(rrecp->r_dname = strdup(dname)))
+if (!rrecp || !(rrecp->r_dname = strdup(dname))) {
+free(rrecp);
 return (NULL);
+}
 rrecp->r_class = (u_int16_t)class;
 rrecp->r_type = (u_int16_t)type;
 rrecp->r_ttl = (u_int32_t)ttl;
--
1.9.5.msysgit.1

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

Re: [edk2] [patch] MdeModulePkg: Make HII configuration settings available to OS runtime

[El-Haj-Mahmoud, Samer]

+1

I also would add there may be some HII strings that are hidden from user 
interfaces, and reflect settings for field service or troubleshooting, and that 
a mass export to the OS may expose these settings to OS runtime code and 
possibly applications.



-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Andrew 
Fish
Sent: Tuesday, February 16, 2016 10:37 AM
To: Brian J. Johnson 
Cc: Dandan Bi ; edk2-devel@lists.01.org; Eric Dong 
; Liming Gao 
Subject: Re: [edk2] [patch] MdeModulePkg: Make HII configuration settings 
available to OS runtime


> On Feb 16, 2016, at 8:33 AM, Brian J. Johnson  wrote:
> 
> On 02/16/2016 12:58 AM, Dandan Bi wrote:
>> This feature is aimed to allow OS make use of the HII database during 
>> runtime. In this case, the contents of the HII Database is exported 
>> to a buffer. The pointer to the buffer is placed in the EFI System 
>> Configuration Table, where it can be retrieved by an OS application.
>> 
>> Export the configuration data and contents of HiiDatabase when driver 
>> add package, update package and remove package.
>> For string and image may also need to update the contents of 
>> HiiDatabase when NewString/SetString, NewImage/SetImage.
>> 
>> Cc: Liming Gao 
>> Cc: Eric Dong 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Dandan Bi  ...
> 
> Please make this behavior selectable via a PCD.  HII operations are very 
> expensive, especially on simulators.  I don't want to pay the export time 
> every time a package is added or string is changed.  Also, platforms should 
> be able to decide if they want to offer this data to the OS.
> 

+1

I would want to opt out to NOT take the memory away from the OS if the platform 
did not care about the feature.

Thanks,

Andrew Fish

> Why not just export the data once, using a "ready to boot" event hook?
> 
> Thanks,
> --
> 
>   Brian J. Johnson
> 
> 
> 
>  My statements are my own, are not authorized by SGI, and do not  
> necessarily represent SGI's positions.
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

Re: [edk2] [PATCH] MdeModulePkg, AtaBusDxe: Bounce buffer IO operations if unaligned

[Laszlo Ersek]

On 02/16/16 17:36, Jeremy Linton wrote:
> 
> (trimming)
> 
> |Not wishing to influence the discussion, just out of curiosity: Jeremy
> |mentions "numerous other BlockIo protocol providers in edk2 bounce IO
> |operations rather than simply allowing them to fail" -- can we see some
> |examples? I wonder if, upon seeing that code, we could use "git blame"
> |to find out *why* those workarounds had been introduced.
> 
> It's the same case, grub2 fails to honor the alignment requirements.
> 
> OvmfPkg/XenPvBlkDxe/BlockIo.c
> 
> I thought when I originally posted that I had found a couple more
> cases, but then when talking to Leif about it a while back, that was
> the one case I found.

//
// Grub2 does not appear to respect IoAlign of 512, so reallocate the
// buffer here.
//

That's not an example that makes me especially happy. In fact, now that
I know about it, I think I'll only tolerate it in OvmfPkg because it's
related to Xen. (And because it's already there.)

I'm actually pretty disappointed. The above XenPvBlkDxe code was
committed to edk2 in October 2014 (5de8a35c62406), but grub2 has the bug
to *this day*. If you grep the grub2 code at 25492a0f047c for
"io_align", the only hit is in the structure definition, in
"include/grub/efi/api.h".

Apparently, noone has bothered in the past ~1.5 years to report and fix
the bug in grub2, despite the bugfix looking quite manageable.

Also, I don't think there is a recent change in edk2 that can be
pinpointed as "breaking" grub2.

So this example sort of makes me want to influence the discussion, after
all.

> Either way, the problem will eventually get solved in grub, but that
> doesn't solve the problem of trying to boot existing OS's.

Can you please explain the expected lifecycle of the firmware side
bugfix? Assuming the patch gets applied, edk2 becomes able to work
around the grub2 bug. How are users, for whom the OS installer (or the
prebuilt image) fails to boot on their hardware, due to the grub2 bug,
benefit from the edk2 bugfix? Are they expected to rebuild, and/or
reflash, their firmware?

Anyway, what worries me most is, once we add this kludge to edk2, we'll
never know when it's safe to remove it. We'll be stuck with it forever.
I think that's a bad prospect for the project that is supposed to be the
reference implementation for UEFI. (Side note: I hope my argument
resembles Ard's argument against the properties table closely enough! ;))

edk2 does include workarounds -- for hardware bugs. OS installers and
prebuilt images are not hardware though. They can be refreshed. I think
grub_efidisk_readwrite() in grub2 genuinely needs a fix, and that fix
should be backported as far as a given distro cares.

Beyond that, I think this patch should go into downstreams of edk2 that
care about supporting very old distributions whose install media (or
prebuilt images) cannot be refreshed with a fixed grub2.

(Side note: I'm acutely aware of the fact that OVMF depends on some
quirks in KVM, namely the MTRR emulation. That's a different story
however: (a) we *did* try to fix it -- but failed, because it is rooted
in physical hardware, and extremely hard to debug and reason about --,
and (b) there was a very localizable, bisectable *change* in KVM that
ultimately broke the user experience with OVMF, and had to be quirked
for OVMF's sake. Neither of these traits seem to hold for the grub2 bug.
Compat kludges have not been created equal.)

Can you carry this patch in your downstream edk2 project?

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

Re: [edk2] [PATCH] EmbeddedPkg: Add ACPI6.0 GIC Definitions

[Supreeth Venkatesh]

I will test it on Juno and update the patch.

Thanks,
Supreeth

-Original Message-
From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
Sent: Monday, February 15, 2016 11:34 AM
To: Supreeth Venkatesh
Cc: edk2-devel@lists.01.org; ard.biesheu...@linaro.org; 
graeme.greg...@linaro.org
Subject: Re: [edk2] [PATCH] EmbeddedPkg: Add ACPI6.0 GIC Definitions

Hi Supreeth,

On Wed, Feb 10, 2016 at 02:29:42PM -0600, Supreeth Venkatesh wrote:
> Add ACPI6.0 macros for GIC Distributor Initialization, GICC Structure
> Initialization and GIC Redistributor Initialization.

So, I have no objection to the below, but there is no public platform making 
use of it, so no way to test it using public code.
I would prefer to not push it until that changes.
Would it be much effort to update Juno to all ACPI 6?

> Contributed-under: TianoCore Contribution Agreement 1.0
> Reported-by: Dennis Chen 
> Signed-off-by: Supreeth Venkatesh 
> Tested-by: Supreeth Venkatesh 

No need to add the Tested-by on your own patches.
That is implied unless explicitly stated otherwise.

> ---
>  EmbeddedPkg/Include/Library/AcpiLib.h | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/EmbeddedPkg/Include/Library/AcpiLib.h
> b/EmbeddedPkg/Include/Library/AcpiLib.h
> index 42710fd..e88e57f 100644
> --- a/EmbeddedPkg/Include/Library/AcpiLib.h
> +++ b/EmbeddedPkg/Include/Library/AcpiLib.h
> @@ -38,6 +38,12 @@
>  GicDistHwId, GicDistBase, GicDistVector, EFI_ACPI_RESERVED_DWORD \
>}
>
> +#define EFI_ACPI_6_0_GIC_DISTRIBUTOR_INIT(GicDistHwId, GicDistBase,
> +GicDistVector, GicVersion) \
> +  { \
> +EFI_ACPI_6_0_GICD, sizeof (EFI_ACPI_6_0_GIC_DISTRIBUTOR_STRUCTURE), 
> EFI_ACPI_RESERVED_WORD, \
> +GicDistHwId, GicDistBase, GicDistVector, GicVersion \
> +  }
> +
>  // Note the parking protocol is configured by UEFI if required
> #define EFI_ACPI_5_0_GIC_STRUCTURE_INIT(GicId, AcpiCpuId, Flags, PmuIrq, 
> GicBase) \
>{ \
> @@ -54,12 +60,26 @@
>  GsivId, GicRBase, Mpidr  
> \
>}
>
> +// Note the parking protocol is configured by UEFI if required
> +#define EFI_ACPI_6_0_GICC_STRUCTURE_INIT(GicId, AcpiCpuUid, Mpidr, Flags, 
> PmuIrq,\
> +GicBase, GicVBase, GicHBase, GsivId, GicRBase)   
> \
> +  {  
> \
> +EFI_ACPI_6_0_GIC, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE), 
> EFI_ACPI_RESERVED_WORD,   \
> +GicId, AcpiCpuUid, Flags, 0, PmuIrq, 0, GicBase, GicVBase, GicHBase, 
> \
> +GsivId, GicRBase, Mpidr  
> \
> +  }
> +
>  #define EFI_ACPI_6_0_GIC_MSI_FRAME_INIT(GicMsiFrameId, PhysicalBaseAddress, 
> Flags, SPICount, SPIBase) \
>{ \
>  EFI_ACPI_6_0_GIC_MSI_FRAME, sizeof 
> (EFI_ACPI_6_0_GIC_MSI_FRAME_STRUCTURE), EFI_ACPI_RESERVED_WORD, \
>  GicMsiFrameId, PhysicalBaseAddress, Flags, SPICount, SPIBase \
>}
>
> +#define EFI_ACPI_6_0_GIC_REDISTRIBUTOR_INIT(RedisRegionAddr,
> +RedisDiscLength) \
> +  { \
> +EFI_ACPI_6_0_GICR, sizeof (EFI_ACPI_6_0_GICR_STRUCTURE), 0,
> +RedisRegionAddr, RedisDiscLength \
> +  }
> +
>  //
>  // SBSA Generic Watchdog
>  //
> --
> 2.6.3
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.

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

Re: [edk2] [PATCH] MdeModulePkg, AtaBusDxe: Bounce buffer IO operations if unaligned

[Jeremy Linton]

(trimming)

|Not wishing to influence the discussion, just out of curiosity: Jeremy
|mentions "numerous other BlockIo protocol providers in edk2 bounce IO
|operations rather than simply allowing them to fail" -- can we see some
|examples? I wonder if, upon seeing that code, we could use "git blame"
|to find out *why* those workarounds had been introduced.

It's the same case, grub2 fails to honor the alignment requirements.

OvmfPkg/XenPvBlkDxe/BlockIo.c

I thought when I originally posted that I had found a couple more cases, but 
then when talking to Leif about it a while back, that was the one case I found. 
 Either way, the problem will eventually get solved in grub, but that doesn't 
solve the problem of trying to boot existing OS's. 



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

Re: [edk2] [patch] MdeModulePkg: Make HII configuration settings available to OS runtime

[Andrew Fish]

> On Feb 16, 2016, at 8:33 AM, Brian J. Johnson  wrote:
> 
> On 02/16/2016 12:58 AM, Dandan Bi wrote:
>> This feature is aimed to allow OS make use of the HII database
>> during runtime. In this case, the contents of the HII Database
>> is exported to a buffer. The pointer to the buffer is placed
>> in the EFI System Configuration Table, where it can be retrieved
>> by an OS application.
>> 
>> Export the configuration data and contents of HiiDatabase when
>> driver add package, update package and remove package.
>> For string and image may also need to update the contents of
>> HiiDatabase when NewString/SetString, NewImage/SetImage.
>> 
>> Cc: Liming Gao 
>> Cc: Eric Dong 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Dandan Bi 
>> ...
> 
> Please make this behavior selectable via a PCD.  HII operations are very 
> expensive, especially on simulators.  I don't want to pay the export time 
> every time a package is added or string is changed.  Also, platforms should 
> be able to decide if they want to offer this data to the OS.
> 

+1

I would want to opt out to NOT take the memory away from the OS if the platform 
did not care about the feature.

Thanks,

Andrew Fish

> Why not just export the data once, using a "ready to boot" event hook?
> 
> Thanks,
> -- 
> 
>   Brian J. Johnson
> 
> 
> 
>  My statements are my own, are not authorized by SGI, and do not
>  necessarily represent SGI’s positions.
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

Re: [edk2] [patch] MdeModulePkg: Make HII configuration settings available to OS runtime

[Brian J. Johnson]

On 02/16/2016 12:58 AM, Dandan Bi wrote:

This feature is aimed to allow OS make use of the HII database
during runtime. In this case, the contents of the HII Database
is exported to a buffer. The pointer to the buffer is placed
in the EFI System Configuration Table, where it can be retrieved
by an OS application.

Export the configuration data and contents of HiiDatabase when
driver add package, update package and remove package.
For string and image may also need to update the contents of
HiiDatabase when NewString/SetString, NewImage/SetImage.

Cc: Liming Gao 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
...


Please make this behavior selectable via a PCD.  HII operations are very 
expensive, especially on simulators.  I don't want to pay the export 
time every time a package is added or string is changed.  Also, 
platforms should be able to decide if they want to offer this data to 
the OS.


Why not just export the data once, using a "ready to boot" event hook?

Thanks,
--

Brian J. Johnson



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

Re: [edk2] [PATCH v2 1/2] ShellPkg/UefiDpLib: Refine the code of locating all handles in DpTrace.c.

[El-Haj-Mahmoud, Samer]

Series Reviewed -By: Samer El-Haj-Mahmoud 



-Original Message-
From: Shia, Cinnamon 
Sent: Monday, February 15, 2016 3:07 AM
To: edk2-devel@lists.01.org
Cc: El-Haj-Mahmoud, Samer ; Shia, Cinnamon 

Subject: [PATCH v2 1/2] ShellPkg/UefiDpLib: Refine the code of locating all 
handles in DpTrace.c.

Replace gBS->LocateHandle with gBS->LocateHandleBuffer

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Cinnamon Shia 
---
 ShellPkg/Library/UefiDpLib/DpTrace.c | 40 
 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/ShellPkg/Library/UefiDpLib/DpTrace.c 
b/ShellPkg/Library/UefiDpLib/DpTrace.c
index d17d514..22b83d0 100644
--- a/ShellPkg/Library/UefiDpLib/DpTrace.c
+++ b/ShellPkg/Library/UefiDpLib/DpTrace.c
@@ -2,7 +2,7 @@
   Trace reporting for the Dp utility.
 
   Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.
-  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
+  (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at @@ -153,8 +153,7 @@ DumpAllTrace(
   UINTN TIndex;
 
   EFI_HANDLE*HandleBuffer;
-  UINTN Size;
-  EFI_HANDLETempHandle;
+  UINTN HandleCount;
   EFI_STATUSStatus;
   EFI_STRINGStringPtrUnknown;
 
@@ -166,17 +165,7 @@ DumpAllTrace(
 
   // Get Handle information
   //
-  Size = 0;
-  HandleBuffer = 
-  Status  = gBS->LocateHandle (AllHandles, NULL, NULL, , );
-  if (Status == EFI_BUFFER_TOO_SMALL) {
-HandleBuffer = AllocatePool (Size);
-ASSERT (HandleBuffer != NULL);
-if (HandleBuffer == NULL) {
-  return;
-}
-Status  = gBS->LocateHandle (AllHandles, NULL, NULL, , HandleBuffer);
-  }
+  Status  = gBS->LocateHandleBuffer (AllHandles, NULL, NULL, 
+ , );
   if (EFI_ERROR (Status)) {
 ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DP_HANDLES_ERROR), 
gDpHiiHandle, Status);
   }
@@ -232,7 +221,7 @@ DumpAllTrace(
   AsciiStrToUnicodeStr (Measurement.Token, mUnicodeToken);
   if (Measurement.Handle != NULL) {
 // See if the Handle is in the HandleBuffer
-for (TIndex = 0; TIndex < (Size / sizeof(HandleBuffer[0])); TIndex++) {
+for (TIndex = 0; TIndex < HandleCount; TIndex++) {
   if (Measurement.Handle == HandleBuffer[TIndex]) {
 DpGetNameFromHandle (HandleBuffer[TIndex]);
 break;
@@ -270,7 +259,7 @@ DumpAllTrace(
   }
 }
   }
-  if (HandleBuffer != ) {
+  if (HandleBuffer != NULL) {
 FreePool (HandleBuffer);
   }
   SHELL_FREE_NON_NULL (IncFlag);
@@ -536,8 +525,7 @@ ProcessHandles(
   UINTN Index;
   UINTN LogEntryKey;
   UINTN Count;
-  UINTN Size;
-  EFI_HANDLETempHandle;
+  UINTN HandleCount;
   EFI_STATUSStatus;
   EFI_STRINGStringPtrUnknown;
 
@@ -548,17 +536,7 @@ ProcessHandles(
   FreePool (StringPtr);
   FreePool (StringPtrUnknown);
 
-  Size = 0;
-  HandleBuffer = 
-  Status  = gBS->LocateHandle (AllHandles, NULL, NULL, , );
-  if (Status == EFI_BUFFER_TOO_SMALL) {
-HandleBuffer = AllocatePool (Size);
-ASSERT (HandleBuffer != NULL);
-if (HandleBuffer == NULL) {
-  return Status;
-}
-Status  = gBS->LocateHandle (AllHandles, NULL, NULL, , HandleBuffer);
-  }
+  Status = gBS->LocateHandleBuffer (AllHandles, NULL, NULL, 
+ , );
   if (EFI_ERROR (Status)) {
 ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DP_HANDLES_ERROR), 
gDpHiiHandle, Status);
   }
@@ -598,7 +576,7 @@ ProcessHandles(
   mGaugeString[0] = 0;// Empty driver name by default
   AsciiStrToUnicodeStr (Measurement.Token, mUnicodeToken);
   // See if the Handle is in the HandleBuffer
-  for (Index = 0; Index < (Size / sizeof(HandleBuffer[0])); Index++) {
+  for (Index = 0; Index < HandleCount; Index++) {
 if (Measurement.Handle == HandleBuffer[Index]) {
   DpGetNameFromHandle (HandleBuffer[Index]); // Name is put into 
mGaugeString
   break;
@@ -630,7 +608,7 @@ ProcessHandles(
   }
 }
   }
-  if (HandleBuffer != ) {
+  if (HandleBuffer != NULL) {
 FreePool (HandleBuffer);
   }
   return Status;
--
2.7.0.windows.2

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

[edk2] [patch 0/2] Add new HII action type EFI_BROWSER_ACTION_SUBMITTED

[Dandan Bi]

The following two patches mainly add the new HII action 
type EFI_BROWSER_ACTION_SUBMITTED to notify HII driver 
when its question values are submitted.

Cc: Liming Gao 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 

Dandan Bi (2):
  MdePkg: Add new HII action type EFI_BROWSER_ACTION_SUBMITTED
  MdeModulePkg: Add new HII action type EFI_BROWSER_ACTION_SUBMITTED

 MdeModulePkg/Universal/SetupBrowserDxe/Setup.c | 170 -
 MdePkg/Include/Protocol/HiiConfigAccess.h  |   3 +-
 2 files changed, 138 insertions(+), 35 deletions(-)

-- 
1.9.5.msysgit.1

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

[edk2] [patch 2/2] MdeModulePkg: Add new HII action type EFI_BROWSER_ACTION_SUBMITTED

[Dandan Bi]

Add new HII action type EFI_BROWSER_ACTION_SUBMITTED
to notify HII driver when its question values are submitted.

Cc: Liming Gao 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/Universal/SetupBrowserDxe/Setup.c | 170 -
 1 file changed, 136 insertions(+), 34 deletions(-)

diff --git a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c 
b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
index 89869ed..f9e7e80 100644
--- a/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
+++ b/MdeModulePkg/Universal/SetupBrowserDxe/Setup.c
@@ -2447,10 +2447,123 @@ SendDiscardInfoToDriver (
  );
   }
 }
 
 /**
+  When submit the question value, call the callback function with Submitted 
type
+  to inform the hii driver. And update the reset and reconnect flag.
+
+  @param  FormSetFormSet data structure.
+  @param  Form   Form data structure.
+
+**/
+VOID
+SubmitCallbackAndFlageUpdateForForm (
+  IN FORM_BROWSER_FORMSET *FormSet,
+  IN FORM_BROWSER_FORM*Form
+  )
+{
+  LIST_ENTRY  *Link;
+  FORM_BROWSER_STATEMENT  *Question;
+  EFI_IFR_TYPE_VALUE  *TypeValue;
+  EFI_BROWSER_ACTION_REQUEST  ActionRequest;
+
+  if (FormSet->ConfigAccess == NULL) {
+return;
+  }
+
+  Link = GetFirstNode (>StatementListHead);
+  while (!IsNull (>StatementListHead, Link)) {
+Question = FORM_BROWSER_STATEMENT_FROM_LINK (Link);
+Link = GetNextNode (>StatementListHead, Link);
+
+if (!Question->ValueChanged) {
+  continue;
+}
+
+//
+//Compare the buffer and editbuffer data to see whether the data has been 
saved.
+//
+IsQuestionValueChanged(FormSet, Form, Question, GetSetValueWithBothBuffer);
+
+if (Question->ValueChanged) {
+  continue;
+}
+
+//
+// Only the changed data has been saved, then need to set the reset flag.
+//
+if ((Question->QuestionFlags & EFI_IFR_FLAG_RESET_REQUIRED) != 0) {
+  gResetRequired = TRUE;
+}
+
+if ((Question->QuestionFlags & EFI_IFR_FLAG_RECONNECT_REQUIRED) != 0) {
+  gFlagReconnect = TRUE;
+}
+
+if (Question->Storage == NULL || Question->Storage->Type == 
EFI_HII_VARSTORE_EFI_VARIABLE) {
+  continue;
+}
+
+if ((Question->QuestionFlags & EFI_IFR_FLAG_CALLBACK) != 
EFI_IFR_FLAG_CALLBACK) {
+   continue;
+}
+
+if (Question->Operand == EFI_IFR_PASSWORD_OP) {
+   continue;
+}
+
+if (Question->HiiValue.Type == EFI_IFR_TYPE_BUFFER) {
+  TypeValue = (EFI_IFR_TYPE_VALUE *) Question->BufferValue;
+} else {
+  TypeValue = >HiiValue.Value;
+}
+
+ActionRequest = EFI_BROWSER_ACTION_REQUEST_NONE;
+FormSet->ConfigAccess->Callback (
+ FormSet->ConfigAccess,
+ EFI_BROWSER_ACTION_SUBMITTED,
+ Question->QuestionId,
+ Question->HiiValue.Type,
+ TypeValue,
+ 
+ );
+  }
+}
+
+/**
+  When value set Success, call the submit callback function,
+  and update the reset and reconnect flag.
+
+  @param  FormSetFormSet data structure.
+  @param  Form   Form data structure.
+
+**/
+VOID
+SubmitCallbackAndFlageUpdate (
+  IN FORM_BROWSER_FORMSET *FormSet,
+  IN FORM_BROWSER_FORM*Form
+  )
+{
+  FORM_BROWSER_FORM   *CurrentForm;
+  LIST_ENTRY  *Link;
+
+  if (Form != NULL) {
+SubmitCallbackAndFlageUpdateForForm(FormSet, Form);
+return;
+  }
+
+  Link = GetFirstNode (>FormListHead);
+  while (!IsNull (>FormListHead, Link)) {
+CurrentForm = FORM_BROWSER_FORM_FROM_LINK (Link);
+Link = GetNextNode (>FormListHead, Link);
+
+SubmitCallbackAndFlageUpdateForForm(FormSet, CurrentForm);
+  }
+}
+
+/**
   Validate the HiiHandle.
 
   @param  HiiHandle  The input HiiHandle which need to validate.
 
   @retval TRUE   The handle is validate.
@@ -2516,92 +2629,71 @@ ValidateFormSet (
   }
 
   return Find;
 }
 /**
-  Check whether need to enable the reset flag in form level.
-  Also clean all ValueChanged flag in question.
+  Clean all ValueChanged flag in question.
 
   @param  SetFlagWhether need to set the Reset Flag.
   @param  FormSetFormSet data structure.
   @param  Form   Form data structure.
 
 **/
 VOID
 UpdateFlagForForm (
-  IN BOOLEAN  SetFlag,
   IN FORM_BROWSER_FORMSET *FormSet,
   IN FORM_BROWSER_FORM*Form
   )
 {
   LIST_ENTRY  *Link;
   FORM_BROWSER_STATEMENT  *Question;
-  BOOLEAN OldValue;
 
   Link = GetFirstNode (>StatementListHead);
   while (!IsNull (>StatementListHead, Link)) {
 Question = 

[edk2] [patch 1/2] MdePkg: Add new HII action type EFI_BROWSER_ACTION_SUBMITTED

[Dandan Bi]

Base on the UEFI2.6, Add the new HII action type
EFI_BROWSER_ACTION_SUBMITTED to notify HII driver
when its question values are submitted.

Cc: Liming Gao 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdePkg/Include/Protocol/HiiConfigAccess.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/HiiConfigAccess.h 
b/MdePkg/Include/Protocol/HiiConfigAccess.h
index ec8f694..6cd28e5 100644
--- a/MdePkg/Include/Protocol/HiiConfigAccess.h
+++ b/MdePkg/Include/Protocol/HiiConfigAccess.h
@@ -3,11 +3,11 @@
   The EFI HII results processing protocol invokes this type of protocol 
   when it needs to forward results to a driver's configuration handler. 
   This protocol is published by drivers providing and requesting 
   configuration data from HII. It may only be invoked by HII.
   
-Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
 This program and the accompanying materials are licensed and made available 
under 
 the terms and conditions of the BSD License that accompanies this 
distribution.  
 The full text of the license may be found at
 http://opensource.org/licenses/bsd-license.php.
  
 
@@ -32,10 +32,11 @@ typedef UINTN EFI_BROWSER_ACTION;
 #define EFI_BROWSER_ACTION_CHANGING   0
 #define EFI_BROWSER_ACTION_CHANGED1
 #define EFI_BROWSER_ACTION_RETRIEVE   2
 #define EFI_BROWSER_ACTION_FORM_OPEN  3
 #define EFI_BROWSER_ACTION_FORM_CLOSE 4
+#define EFI_BROWSER_ACTION_SUBMITTED  5
 #define EFI_BROWSER_ACTION_DEFAULT_STANDARD  0x1000
 #define EFI_BROWSER_ACTION_DEFAULT_MANUFACTURING 0x1001
 #define EFI_BROWSER_ACTION_DEFAULT_SAFE  0x1002
 #define EFI_BROWSER_ACTION_DEFAULT_PLATFORM  0x2000
 #define EFI_BROWSER_ACTION_DEFAULT_HARDWARE  0x3000
-- 
1.9.5.msysgit.1

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

Re: [edk2] SATA 3.0 AHCI host controller codebase

[Laszlo Ersek]

On 02/16/16 10:01, Shaveta Leekha wrote:
> Resending it
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Tuesday, February 16, 2016 2:02 PM
> To: Shaveta Leekha ; Bhupesh Sharma 
> ; Leif Lindholm 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
> 
> On 02/16/16 09:06, Shaveta Leekha wrote:
>> Hi Laszlo,
>>
>> Please find my replies and few more doubts in-lined.
> 
> I cannot visually distinguish your comments from mine. Can you please resend 
> your email with a mailer that supports sane quoting, or else mark each one of 
> the paragraphs you are adding with [Shaveta]?
> 
> Thanks
> Laszlo
> 
>>
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Monday, February 15, 2016 4:42 PM
>> To: Shaveta Leekha ; Bhupesh Sharma 
>> ; Leif Lindholm 
>> Cc: edk2-devel@lists.01.org 
>> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
>>
>> On 02/15/16 06:52, Shaveta Leekha wrote:
>>>
>>>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Monday, February 08, 2016 1:20 PM
>>> To: Bhupesh Sharma ; Leif Lindholm 
>>> ; Shaveta Leekha 
>>> Cc: edk2-devel@lists.01.org 
>>> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
>>>
>>> On 02/06/16 09:38, Bhupesh Sharma wrote:
> From: Laszlo Ersek
> Sent: Friday, February 05, 2016 2:27 AM
>
> On 02/03/16 22:39, Leif Lindholm wrote:
>> Hi Shaveta,
>>
>> On Tue, Feb 02, 2016 at 07:05:03AM +, Shaveta Leekha wrote:
>>> Is there some SATA 3.0 AHCI driver implementation in UEFI / EDK code?
>>> The one I need to write for our platform is not PCI based.
>>>
>>> I have seen few implementations in EDK2:
>>> DuetPkg/SataControllerDxe/SataController.c
>>> OvmfPkg/SataControllerDxe/SataController.c
>>> But in all of them SATA connectivity is via PCI Express switch.
>>>
>>> Kindly point me to some non-PCI based "SATA 3.0 AHCI driver code"
>>> for UEFI, if there is any such code.
>>
>> I have seen several such platforms implementing a "dummy" PCI 
>> Express driver, simply translating PCI accesses to memory mapped 
>> ones, and still making use of the MdeModulePkg/Bus/Ata/ libraries.
>>
>> You can see examples of PCI emulation in 
>> Omap35xxPkg/PciEmulation/PciEmulation.c
>> and
>> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/PciEmulation.c
>>
>> As a side note, we really should have a common mmio-PCI emulation 
>> library in core EDK2.
>
> Actually the driver stack rests upon the platform-specific PCI host 
> bridge/root bridge driver (a DXE_DRIVER, i.e., one that doesn't 
> bind other handles according to the UEFI Driver Model, but produces 
> the Root Bridge IO protocols, and the Host Bridge Resource 
> Allocation protocol(s), out of "thin air", in its entry point function).
>
> The PCI Bus driver (producing PciIo protocol instances) is 
> platform- independent, and needs the previously mentioned driver. 
> From PciIo upwards, it's all generic. (Well, I'll mention that 
> there is no
> platform- independent SataControllerDxe in edk2. I don't recall the 
> reason -- apologies --, but a reason *was* given for it.)
>
> Ray recently implemented a generic PCI host bridge / root bridge 
> driver, in "MdeModulePkg/Bus/Pci/PciHostBridgeDxe". That driver delegates:
>
> - some of the platform specifics to the (also new) PciHostBridgeLib
>   class,
> - PCI config space access to the PciSegmentLib class (there are, or can
>   be made, library instances that provide 0xCF8/0xCFC, or MMCONFIG/ECAM
>   based config space access),
> - IO port access to the CPU driver that provides the
>   EFI_CPU_IO2_PROTOCOL.
>
> I believe that for a "single root bridge" system, this structure is 
> generic enough.
>
> (Of course, nothing prevents a system from implementing PciIo 
> independently, which "ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe"
> seems to do.)

 Thanks Laszlo and Leif.

 So, AFAIU there are two approaches available on implementing the 
 SATA (AHCI based) host controller on ARM based SoCs:

 1. Either, we can use the PCIe emulation layers to emulation a SATA 
 controller sitting as a PciIO device on top of the PCIe emulation 
 layer. So, eventually we will be using the ATA Bus layer inside 
 MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
 to get the AHCI-ATA bus services.

 2. Or, we can implement the AHCI based SATA controller as a block 
 

[edk2] Why is the tools_def.txt file parsed 47 times on my fds build?

[Andrew Fish]

I turned on profiling in Python:
BaseTools/BinWrappers/PosixLike/RunToolFromSource:
#!/usr/bin/env bash
#python `dirname $0`/RunToolFromSource.py `basename $0` $*
PYTHONPATH="`dirname $0`/../../Source/Python" \
python -m cProfile -s time "`dirname $0`/../../Source/Python"/`basename 
$0`/`basename $0`.py $*


The slowest item in the working thread was ToolDefClassObject. LoadToolDefFile()

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   478.9320.190   10.9860.234 
ToolDefClassObject.py:60(LoadToolDefFile)
  40674781.3370.0001.3370.000 {method 'split' of 'str' objects}
285191.0640.0001.0640.000 {method 'execute' of 
'sqlite3.Cursor' objects}
   7333780.8260.0001.1610.000 StringIO.py:208(write)
   710.6130.0090.6130.009 {method 'items' of 'dict' objects}
   230.4990.022   11.9050.518 
GuidSection.py:245(__FindExtendTool__)


I see it gets called at the beginning of the build from InitBuild() and that 
makes sense. If I do a genmake build LoadToolDefFile() only gets called a 
single time. 

But it looks like __FindExtendTool__ is also making calls:
 ToolDefinition = 
ToolDefClassObject.ToolDefDict(GenFdsGlobalVariable.ConfDir).ToolsDefTxtDictionary

So it looks to me like class GuidSection() is parsing the tools_def.txt for 
every GUID'ed section that has a GUID defined tool? How come this information 
is not cached? 

Does anyone see a quick fix?

Thanks,

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

Re: [edk2] [patch 0/2] Refine function comments in Keyword Handler Protocol

[Gao, Liming]

Reviewed-by: Liming Gao 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dandan Bi
Sent: Monday, February 15, 2016 2:56 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric; Gao, Liming
Subject: [edk2] [patch 0/2] Refine function comments in Keyword Handler Protocol

The follow two patches mainly to refine the function comments in
EFI Configuration Keyword Handler Protocol.

Cc: Liming Gao 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 

Dandan Bi (2):
  MdeModulePkg: Refine function comments in Keyword Handler Protocol
  MdePkg: Refine the function comments in Keyword Handler Protocol

 MdeModulePkg/Universal/HiiDatabaseDxe/ConfigKeywordHandler.c | 12 +++-
 MdeModulePkg/Universal/HiiDatabaseDxe/HiiDatabase.h  | 12 +++-
 MdePkg/Include/Protocol/HiiConfigKeyword.h   | 12 +++-
 3 files changed, 21 insertions(+), 15 deletions(-)

-- 
1.9.5.msysgit.1

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

Re: [edk2] SATA 3.0 AHCI host controller codebase

[Shaveta Leekha]

Resending it

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Tuesday, February 16, 2016 2:02 PM
To: Shaveta Leekha ; Bhupesh Sharma 
; Leif Lindholm 
Cc: edk2-devel@lists.01.org 
Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase

On 02/16/16 09:06, Shaveta Leekha wrote:
> Hi Laszlo,
> 
> Please find my replies and few more doubts in-lined.

I cannot visually distinguish your comments from mine. Can you please resend 
your email with a mailer that supports sane quoting, or else mark each one of 
the paragraphs you are adding with [Shaveta]?

Thanks
Laszlo

> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Monday, February 15, 2016 4:42 PM
> To: Shaveta Leekha ; Bhupesh Sharma 
> ; Leif Lindholm 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
> 
> On 02/15/16 06:52, Shaveta Leekha wrote:
>>
>>
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Monday, February 08, 2016 1:20 PM
>> To: Bhupesh Sharma ; Leif Lindholm 
>> ; Shaveta Leekha 
>> Cc: edk2-devel@lists.01.org 
>> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
>>
>> On 02/06/16 09:38, Bhupesh Sharma wrote:
 From: Laszlo Ersek
 Sent: Friday, February 05, 2016 2:27 AM

 On 02/03/16 22:39, Leif Lindholm wrote:
> Hi Shaveta,
>
> On Tue, Feb 02, 2016 at 07:05:03AM +, Shaveta Leekha wrote:
>> Is there some SATA 3.0 AHCI driver implementation in UEFI / EDK code?
>> The one I need to write for our platform is not PCI based.
>>
>> I have seen few implementations in EDK2:
>> DuetPkg/SataControllerDxe/SataController.c
>> OvmfPkg/SataControllerDxe/SataController.c
>> But in all of them SATA connectivity is via PCI Express switch.
>>
>> Kindly point me to some non-PCI based "SATA 3.0 AHCI driver code"
>> for UEFI, if there is any such code.
>
> I have seen several such platforms implementing a "dummy" PCI 
> Express driver, simply translating PCI accesses to memory mapped 
> ones, and still making use of the MdeModulePkg/Bus/Ata/ libraries.
>
> You can see examples of PCI emulation in 
> Omap35xxPkg/PciEmulation/PciEmulation.c
> and
> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/PciEmulation.c
>
> As a side note, we really should have a common mmio-PCI emulation 
> library in core EDK2.

 Actually the driver stack rests upon the platform-specific PCI host 
 bridge/root bridge driver (a DXE_DRIVER, i.e., one that doesn't 
 bind other handles according to the UEFI Driver Model, but produces 
 the Root Bridge IO protocols, and the Host Bridge Resource 
 Allocation protocol(s), out of "thin air", in its entry point function).

 The PCI Bus driver (producing PciIo protocol instances) is 
 platform- independent, and needs the previously mentioned driver. 
 From PciIo upwards, it's all generic. (Well, I'll mention that 
 there is no
 platform- independent SataControllerDxe in edk2. I don't recall the 
 reason -- apologies --, but a reason *was* given for it.)

 Ray recently implemented a generic PCI host bridge / root bridge 
 driver, in "MdeModulePkg/Bus/Pci/PciHostBridgeDxe". That driver delegates:

 - some of the platform specifics to the (also new) PciHostBridgeLib
   class,
 - PCI config space access to the PciSegmentLib class (there are, or can
   be made, library instances that provide 0xCF8/0xCFC, or MMCONFIG/ECAM
   based config space access),
 - IO port access to the CPU driver that provides the
   EFI_CPU_IO2_PROTOCOL.

 I believe that for a "single root bridge" system, this structure is 
 generic enough.

 (Of course, nothing prevents a system from implementing PciIo 
 independently, which "ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe"
 seems to do.)
>>>
>>> Thanks Laszlo and Leif.
>>>
>>> So, AFAIU there are two approaches available on implementing the 
>>> SATA (AHCI based) host controller on ARM based SoCs:
>>>
>>> 1. Either, we can use the PCIe emulation layers to emulation a SATA 
>>> controller sitting as a PciIO device on top of the PCIe emulation 
>>> layer. So, eventually we will be using the ATA Bus layer inside 
>>> MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
>>> to get the AHCI-ATA bus services.
>>>
>>> 2. Or, we can implement the AHCI based SATA controller as a block 
>>> device (exposing the BLOCK IO protocol) and implemented on the lines 
>>> of a SD/MMC card like driver. The BLOCK IO protocol can then be used by a 
>>> FatPkg like layer to 

Re: [edk2] [PATCH] ArmPlatformPkg: delete Juno ACPI tables

[Ryan Harkin]

On 16 February 2016 at 08:52, Ard Biesheuvel  wrote:
> On 15 February 2016 at 18:20, Leif Lindholm  wrote:
>> Juno is now managed in OpenPlatformPkg, including the ACPI
>> tables - so delete this unmaintained copy.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Leif Lindholm 
>
> Acked-by: Ard Biesheuvel 
Acked-by: Ryan Harkin 


>
>> ---
>>  .../ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl  | 190 
>> 
>>  .../ArmJunoPkg/AcpiTables/AcpiTables.inf   |  52 --
>>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Dsdt.asl  | 194 
>> -
>>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Facs.aslc |  62 ---
>>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Fadt.aslc |  99 ---
>>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Gtdt.aslc | 103 ---
>>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Madt.aslc | 131 --
>>  7 files changed, 831 deletions(-)
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiTables.inf
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Dsdt.asl
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Facs.aslc
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Fadt.aslc
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Gtdt.aslc
>>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Madt.aslc
>>
>> diff --git a/ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl 
>> b/ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl
>> deleted file mode 100644
>> index 800d2cb..000
>> --- a/ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl
>> +++ /dev/null
>> @@ -1,190 +0,0 @@
>> -/** @file
>> -  Differentiated System Description Table Fields (SSDT)
>> -
>> -  Copyright (c) 2014-2015, ARM Ltd. All rights reserved.
>> -This program and the accompanying materials
>> -  are licensed and made available under the terms and conditions of the BSD 
>> License
>> -  which accompanies this distribution.  The full text of the license may be 
>> found at
>> -  http://opensource.org/licenses/bsd-license.php
>> -
>> -  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> -  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>> IMPLIED.
>> -
>> -**/
>> -
>> -#include "ArmPlatform.h"
>> -
>> -/*
>> -  See Reference [1] 6.2.12
>> -  "There are two ways that _PRT can be used. ...
>> -  In the second model, the PCI interrupts are hardwired to specific 
>> interrupt
>> -  inputs on the interrupt controller and are not configurable. In this case,
>> -  the Source field in _PRT does not reference a device, but instead contains
>> -  the value zero, and the Source Index field contains the global system
>> -  interrupt to which the PCI interrupt is hardwired."
>> -*/
>> -#define PRT_ENTRY(Address, Pin, Interrupt)  
>>  \
>> -  Package (4) { 
>>   \
>> -Address,/* uses the same format as _ADR */  
>>   \
>> -Pin,/* The PCI pin number of the device (0-INTA, 
>> 1-INTB, 2-INTC, 3-INTD). */  \
>> -Zero,   /* allocated from the global interrupt pool. */ 
>>   \
>> -Interrupt   /* global system interrupt number */
>>   \
>> -  }
>> -
>> -/*
>> -  See Reference [1] 6.1.1
>> -  "High word–Device #, Low word–Function #. (for example, device 3, 
>> function 2 is
>> -   0x00030002). To refer to all the functions on a device #, use a function 
>> number of )."
>> -*/
>> -#define ROOT_PRT_ENTRY(Pin, Interrupt)   PRT_ENTRY(0x, Pin, 
>> Interrupt)
>> -// Device 0 for Bridge.
>> -
>> -
>> -DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "ARM-JUNO", 
>> EFI_ACPI_ARM_OEM_REVISION) {
>> -  Scope(_SB) {
>> -   //
>> -   // PCI Root Complex
>> -   //
>> -   Device(PCI0)
>> -{
>> -   Name(_HID, EISAID("PNP0A08")) // PCI Express Root Bridge
>> -   Name(_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge
>> -   Name(_SEG, Zero) // PCI Segment Group number
>> -   Name(_BBN, Zero) // PCI Base Bus Number
>> -   Name(_CCA, 1)// Initially mark the PCI coherent (for 
>> JunoR1)
>> -
>> -// Root Complex 0
>> -Device (RP0) {
>> -Name(_ADR, 0xF000)// Dev 0, Func 0
>> -}
>> -
>> -   // PCI Routing Table
>> -   Name(_PRT, Package() {
>> -   ROOT_PRT_ENTRY(0, 168),   // INTA
>> -   ROOT_PRT_ENTRY(1, 

Re: [edk2] [PATCH] ArmPlatformPkg: delete Juno ACPI tables

[Ard Biesheuvel]

On 15 February 2016 at 18:20, Leif Lindholm  wrote:
> Juno is now managed in OpenPlatformPkg, including the ACPI
> tables - so delete this unmaintained copy.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leif Lindholm 

Acked-by: Ard Biesheuvel 

> ---
>  .../ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl  | 190 
>  .../ArmJunoPkg/AcpiTables/AcpiTables.inf   |  52 --
>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Dsdt.asl  | 194 
> -
>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Facs.aslc |  62 ---
>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Fadt.aslc |  99 ---
>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Gtdt.aslc | 103 ---
>  ArmPlatformPkg/ArmJunoPkg/AcpiTables/Madt.aslc | 131 --
>  7 files changed, 831 deletions(-)
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiTables.inf
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Dsdt.asl
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Facs.aslc
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Fadt.aslc
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Gtdt.aslc
>  delete mode 100644 ArmPlatformPkg/ArmJunoPkg/AcpiTables/Madt.aslc
>
> diff --git a/ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl 
> b/ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl
> deleted file mode 100644
> index 800d2cb..000
> --- a/ArmPlatformPkg/ArmJunoPkg/AcpiTables/AcpiSsdtRootPci.asl
> +++ /dev/null
> @@ -1,190 +0,0 @@
> -/** @file
> -  Differentiated System Description Table Fields (SSDT)
> -
> -  Copyright (c) 2014-2015, ARM Ltd. All rights reserved.
> -This program and the accompanying materials
> -  are licensed and made available under the terms and conditions of the BSD 
> License
> -  which accompanies this distribution.  The full text of the license may be 
> found at
> -  http://opensource.org/licenses/bsd-license.php
> -
> -  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> -
> -**/
> -
> -#include "ArmPlatform.h"
> -
> -/*
> -  See Reference [1] 6.2.12
> -  "There are two ways that _PRT can be used. ...
> -  In the second model, the PCI interrupts are hardwired to specific interrupt
> -  inputs on the interrupt controller and are not configurable. In this case,
> -  the Source field in _PRT does not reference a device, but instead contains
> -  the value zero, and the Source Index field contains the global system
> -  interrupt to which the PCI interrupt is hardwired."
> -*/
> -#define PRT_ENTRY(Address, Pin, Interrupt)   
> \
> -  Package (4) {  
>  \
> -Address,/* uses the same format as _ADR */   
>  \
> -Pin,/* The PCI pin number of the device (0-INTA, 1-INTB, 
> 2-INTC, 3-INTD). */  \
> -Zero,   /* allocated from the global interrupt pool. */  
>  \
> -Interrupt   /* global system interrupt number */ 
>  \
> -  }
> -
> -/*
> -  See Reference [1] 6.1.1
> -  "High word–Device #, Low word–Function #. (for example, device 3, function 
> 2 is
> -   0x00030002). To refer to all the functions on a device #, use a function 
> number of )."
> -*/
> -#define ROOT_PRT_ENTRY(Pin, Interrupt)   PRT_ENTRY(0x, Pin, 
> Interrupt)
> -// Device 0 for Bridge.
> -
> -
> -DefinitionBlock("SsdtPci.aml", "SSDT", 1, "ARMLTD", "ARM-JUNO", 
> EFI_ACPI_ARM_OEM_REVISION) {
> -  Scope(_SB) {
> -   //
> -   // PCI Root Complex
> -   //
> -   Device(PCI0)
> -{
> -   Name(_HID, EISAID("PNP0A08")) // PCI Express Root Bridge
> -   Name(_CID, EISAID("PNP0A03")) // Compatible PCI Root Bridge
> -   Name(_SEG, Zero) // PCI Segment Group number
> -   Name(_BBN, Zero) // PCI Base Bus Number
> -   Name(_CCA, 1)// Initially mark the PCI coherent (for 
> JunoR1)
> -
> -// Root Complex 0
> -Device (RP0) {
> -Name(_ADR, 0xF000)// Dev 0, Func 0
> -}
> -
> -   // PCI Routing Table
> -   Name(_PRT, Package() {
> -   ROOT_PRT_ENTRY(0, 168),   // INTA
> -   ROOT_PRT_ENTRY(1, 169),   // INTB
> -   ROOT_PRT_ENTRY(2, 170),   // INTC
> -   ROOT_PRT_ENTRY(3, 171),   // INTD
> -   })
> -// Root complex resources
> -   Method (_CRS, 0, Serialized) {
> -  

Re: [edk2] SATA 3.0 AHCI host controller codebase

[Laszlo Ersek]

On 02/16/16 09:06, Shaveta Leekha wrote:
> Hi Laszlo,
> 
> Please find my replies and few more doubts in-lined.

I cannot visually distinguish your comments from mine. Can you please
resend your email with a mailer that supports sane quoting, or else mark
each one of the paragraphs you are adding with [Shaveta]?

Thanks
Laszlo

> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Monday, February 15, 2016 4:42 PM
> To: Shaveta Leekha ; Bhupesh Sharma 
> ; Leif Lindholm 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
> 
> On 02/15/16 06:52, Shaveta Leekha wrote:
>>
>>
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Monday, February 08, 2016 1:20 PM
>> To: Bhupesh Sharma ; Leif Lindholm 
>> ; Shaveta Leekha 
>> Cc: edk2-devel@lists.01.org 
>> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
>>
>> On 02/06/16 09:38, Bhupesh Sharma wrote:
 From: Laszlo Ersek
 Sent: Friday, February 05, 2016 2:27 AM

 On 02/03/16 22:39, Leif Lindholm wrote:
> Hi Shaveta,
>
> On Tue, Feb 02, 2016 at 07:05:03AM +, Shaveta Leekha wrote:
>> Is there some SATA 3.0 AHCI driver implementation in UEFI / EDK code?
>> The one I need to write for our platform is not PCI based.
>>
>> I have seen few implementations in EDK2:
>> DuetPkg/SataControllerDxe/SataController.c
>> OvmfPkg/SataControllerDxe/SataController.c
>> But in all of them SATA connectivity is via PCI Express switch.
>>
>> Kindly point me to some non-PCI based "SATA 3.0 AHCI driver code"
>> for UEFI, if there is any such code.
>
> I have seen several such platforms implementing a "dummy" PCI 
> Express driver, simply translating PCI accesses to memory mapped 
> ones, and still making use of the MdeModulePkg/Bus/Ata/ libraries.
>
> You can see examples of PCI emulation in 
> Omap35xxPkg/PciEmulation/PciEmulation.c
> and
> ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/PciEmulation.c
>
> As a side note, we really should have a common mmio-PCI emulation 
> library in core EDK2.

 Actually the driver stack rests upon the platform-specific PCI host 
 bridge/root bridge driver (a DXE_DRIVER, i.e., one that doesn't bind 
 other handles according to the UEFI Driver Model, but produces the 
 Root Bridge IO protocols, and the Host Bridge Resource Allocation 
 protocol(s), out of "thin air", in its entry point function).

 The PCI Bus driver (producing PciIo protocol instances) is platform- 
 independent, and needs the previously mentioned driver. From PciIo 
 upwards, it's all generic. (Well, I'll mention that there is no
 platform- independent SataControllerDxe in edk2. I don't recall the 
 reason -- apologies --, but a reason *was* given for it.)

 Ray recently implemented a generic PCI host bridge / root bridge 
 driver, in "MdeModulePkg/Bus/Pci/PciHostBridgeDxe". That driver delegates:

 - some of the platform specifics to the (also new) PciHostBridgeLib
   class,
 - PCI config space access to the PciSegmentLib class (there are, or can
   be made, library instances that provide 0xCF8/0xCFC, or MMCONFIG/ECAM
   based config space access),
 - IO port access to the CPU driver that provides the
   EFI_CPU_IO2_PROTOCOL.

 I believe that for a "single root bridge" system, this structure is 
 generic enough.

 (Of course, nothing prevents a system from implementing PciIo 
 independently, which "ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe"
 seems to do.)
>>>
>>> Thanks Laszlo and Leif.
>>>
>>> So, AFAIU there are two approaches available on implementing the SATA 
>>> (AHCI based) host controller on ARM based SoCs:
>>>
>>> 1. Either, we can use the PCIe emulation layers to emulation a SATA 
>>> controller sitting as a PciIO device on top of the PCIe emulation 
>>> layer. So, eventually we will be using the ATA Bus layer inside 
>>> MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
>>> to get the AHCI-ATA bus services.
>>>
>>> 2. Or, we can implement the AHCI based SATA controller as a block 
>>> device (exposing the BLOCK IO protocol) and implemented on the lines 
>>> of a SD/MMC card like driver. The BLOCK IO protocol can then be used by a 
>>> FatPkg like layer to provide a support for FAT32 filesystem over the SATA 
>>> driver.
>>>
>>> Both the above approaches have their own merits and demerits. While 
>>> approach 1 might introduce extra memory allocation/housekeeping overheads 
>>> for PCIe emulation layer, approach 2 might not be compatible to all UEFI 
>>> shell applications that want to use a underlying SATA HDD support.
>>>
>>> 

Re: [edk2] Memory sharing between PEI and DXE phases

[Laszlo Ersek]

On 02/16/16 07:26, Bhupesh Sharma wrote:
> Hi Experts,
> 
> I have a doubt regarding memory sharing between PEI and DXE phases.
> 
> Let's say I have a PEI library 'NorLib.c' and a DXE driver 'NorDxeDriver.c', 
> where
> the DXE driver uses some APIs of the PEI Library to obtain information and 
> provide
> functionalities to the upper layers.
> 
> Now, if I allocate some memory chunk (let's say for a pointer) in the PEI 
> phase,
> how can I use the same in the DXE phase (as normally the PEI pointer contents 
> become
> INVALID in the DXE phase).
> 
> Are there standard HOB mechanisms available for the same?
> 
> Thanks for the help.

In the PEI phase, use the MemoryAllocationLib API's to allocate memory.
The library class header is:

  MdePkg/Include/Library/MemoryAllocationLib.h

and the library instance you should resolve the class to is

  MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf

The memory release functions will not ASSERT(), but they are no-ops. You
can't free memory you allocate during PEI.

For each pool (i.e., non-pages) allocation, there is a size limit of a
bit less than 64KB. The reason is that such allocations are internally
handled with HOBs. If you need more memory, call AllocatePages().

The memory you allocate with AllocatePages() during PEI will survive
in-place into DXE, and it will be correctly reflected in the UEFI memmap
as well. HOBs will be migrated though, so you can't reference them from
DXE *by address*.

So, you have two options:
- If the data is small, create a vendor GUID HOB with the data in it, in
PEI. In DXE, look up the HOB by GUID.
- If the data is large, use AllocatePages() -- or one of its friends --
in PEI, then stash the address in a small vendor GUID HOB.

For the second option, a dynamic PCD could be considered as well, for
storing the base address of the allocated pages. However, storing the
address in a dynamic PCD is in a sense a bit more restricted than
storing the address in a vendor GUID HOB. For example, if you'd like to
access the allocated area in a library instance that gets linked into
the DXE core, then the dynamic PCD is a no-go.

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

Re: [edk2] SATA 3.0 AHCI host controller codebase

[Shaveta Leekha]

Hi Laszlo,

Please find my replies and few more doubts in-lined.

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Monday, February 15, 2016 4:42 PM
To: Shaveta Leekha ; Bhupesh Sharma 
; Leif Lindholm 
Cc: edk2-devel@lists.01.org 
Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase

On 02/15/16 06:52, Shaveta Leekha wrote:
> 
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Monday, February 08, 2016 1:20 PM
> To: Bhupesh Sharma ; Leif Lindholm 
> ; Shaveta Leekha 
> Cc: edk2-devel@lists.01.org 
> Subject: Re: [edk2] SATA 3.0 AHCI host controller codebase
> 
> On 02/06/16 09:38, Bhupesh Sharma wrote:
>>> From: Laszlo Ersek
>>> Sent: Friday, February 05, 2016 2:27 AM
>>>
>>> On 02/03/16 22:39, Leif Lindholm wrote:
 Hi Shaveta,

 On Tue, Feb 02, 2016 at 07:05:03AM +, Shaveta Leekha wrote:
> Is there some SATA 3.0 AHCI driver implementation in UEFI / EDK code?
> The one I need to write for our platform is not PCI based.
>
> I have seen few implementations in EDK2:
> DuetPkg/SataControllerDxe/SataController.c
> OvmfPkg/SataControllerDxe/SataController.c
> But in all of them SATA connectivity is via PCI Express switch.
>
> Kindly point me to some non-PCI based "SATA 3.0 AHCI driver code"
> for UEFI, if there is any such code.

 I have seen several such platforms implementing a "dummy" PCI 
 Express driver, simply translating PCI accesses to memory mapped 
 ones, and still making use of the MdeModulePkg/Bus/Ata/ libraries.

 You can see examples of PCI emulation in 
 Omap35xxPkg/PciEmulation/PciEmulation.c
 and
 ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/PciEmulation.c

 As a side note, we really should have a common mmio-PCI emulation 
 library in core EDK2.
>>>
>>> Actually the driver stack rests upon the platform-specific PCI host 
>>> bridge/root bridge driver (a DXE_DRIVER, i.e., one that doesn't bind 
>>> other handles according to the UEFI Driver Model, but produces the 
>>> Root Bridge IO protocols, and the Host Bridge Resource Allocation 
>>> protocol(s), out of "thin air", in its entry point function).
>>>
>>> The PCI Bus driver (producing PciIo protocol instances) is platform- 
>>> independent, and needs the previously mentioned driver. From PciIo 
>>> upwards, it's all generic. (Well, I'll mention that there is no
>>> platform- independent SataControllerDxe in edk2. I don't recall the 
>>> reason -- apologies --, but a reason *was* given for it.)
>>>
>>> Ray recently implemented a generic PCI host bridge / root bridge 
>>> driver, in "MdeModulePkg/Bus/Pci/PciHostBridgeDxe". That driver delegates:
>>>
>>> - some of the platform specifics to the (also new) PciHostBridgeLib
>>>   class,
>>> - PCI config space access to the PciSegmentLib class (there are, or can
>>>   be made, library instances that provide 0xCF8/0xCFC, or MMCONFIG/ECAM
>>>   based config space access),
>>> - IO port access to the CPU driver that provides the
>>>   EFI_CPU_IO2_PROTOCOL.
>>>
>>> I believe that for a "single root bridge" system, this structure is 
>>> generic enough.
>>>
>>> (Of course, nothing prevents a system from implementing PciIo 
>>> independently, which "ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe"
>>> seems to do.)
>>
>> Thanks Laszlo and Leif.
>>
>> So, AFAIU there are two approaches available on implementing the SATA 
>> (AHCI based) host controller on ARM based SoCs:
>>
>> 1. Either, we can use the PCIe emulation layers to emulation a SATA 
>> controller sitting as a PciIO device on top of the PCIe emulation 
>> layer. So, eventually we will be using the ATA Bus layer inside 
>> MdeModulePkg/Bus/Ata/AtaBusDxe/AtaBusDxe.inf
>> to get the AHCI-ATA bus services.
>>
>> 2. Or, we can implement the AHCI based SATA controller as a block 
>> device (exposing the BLOCK IO protocol) and implemented on the lines 
>> of a SD/MMC card like driver. The BLOCK IO protocol can then be used by a 
>> FatPkg like layer to provide a support for FAT32 filesystem over the SATA 
>> driver.
>>
>> Both the above approaches have their own merits and demerits. While 
>> approach 1 might introduce extra memory allocation/housekeeping overheads 
>> for PCIe emulation layer, approach 2 might not be compatible to all UEFI 
>> shell applications that want to use a underlying SATA HDD support.
>>
>> So, this brings forward the question, as to which UEFI shell applications 
>> (e.g. GRUB2) can work with approach 2 and which cannot.
>> Are there any other obvious disadvantages to approach 2?
>>
>> Please share your thoughts.
> 
> I don't know many UEFI applications, but the few I know, should be happy with 
> the BlockIo + above protocols. At least I can't name any obvious