Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number

[Paulo Alcantara]

On September 13, 2017 2:08:54 AM GMT-03:00, "Zeng, Star"  
wrote:
>I do not understand the context of the code.
>The change is good to fix the build failure, but I want to ask a
>question before I gave Rb. :)
>
>Is it possible ReadFileInfo->FilePosition less than FilePosition?

Nope. When doing my tests, I briefly looked at code how it's used and also 
added an ASSERT() to make sure it is never lesser than FilePosition.

BTW, I *do* know that the code really needs refactoring, documentation, etc. -- 
 I didnt do that before because I believed that it would never get upstream -- 
since its now -- I will look forward to that in my free time.

Its 2:46am here so I should get some sleep :-) Thanks!

Paulo

>
>
>Thanks,
>Star
>-Original Message-
>From: Paulo Alcantara [mailto:pca...@zytor.com] 
>Sent: Wednesday, September 13, 2017 12:45 PM
>To: edk2-devel@lists.01.org
>Cc: Paulo Alcantara ; Zeng, Star
>; Dong, Eric ; Ni, Ruiyu
>; Bi, Dandan 
>Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of
>unsigned number
>
>This patch gets rid of a negative comparison of an UINT64 type (Offset)
>as it'll never evaluate to true.
>
>Cc: Star Zeng 
>Cc: Eric Dong 
>Cc: Ruiyu Ni 
>Cc: Dandan Bi 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Reported-by: Star Zeng 
>Signed-off-by: Paulo Alcantara 
>---
> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
> 1 file changed, 3 deletions(-)
>
>diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>index 7286265373..2039f80289 100644
>--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
>@@ -1082,9 +1082,6 @@ ReadFile (
> 
>if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
>   Offset = ReadFileInfo->FilePosition - FilePosition;
>-  if (Offset < 0) {
>-Offset = -(Offset);
>-  }
> } else {
>   Offset = 0;
> }
>--
>2.11.0

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number

[Zeng, Star]

I do not understand the context of the code.
The change is good to fix the build failure, but I want to ask a question 
before I gave Rb. :)

Is it possible ReadFileInfo->FilePosition less than FilePosition?


Thanks,
Star
-Original Message-
From: Paulo Alcantara [mailto:pca...@zytor.com] 
Sent: Wednesday, September 13, 2017 12:45 PM
To: edk2-devel@lists.01.org
Cc: Paulo Alcantara ; Zeng, Star ; Dong, 
Eric ; Ni, Ruiyu ; Bi, Dandan 

Subject: [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned 
number

This patch gets rid of a negative comparison of an UINT64 type (Offset) as 
it'll never evaluate to true.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Dandan Bi 
Contributed-under: TianoCore Contribution Agreement 1.1
Reported-by: Star Zeng 
Signed-off-by: Paulo Alcantara 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7286265373..2039f80289 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1082,9 +1082,6 @@ ReadFile (
 
 if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
   Offset = ReadFileInfo->FilePosition - FilePosition;
-  if (Offset < 0) {
-Offset = -(Offset);
-  }
 } else {
   Offset = 0;
 }
--
2.11.0

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

[edk2] [PATCH] MdeModulePkg/UdfDxe: Remove negative comparison of unsigned number

[Paulo Alcantara]

This patch gets rid of a negative comparison of an UINT64 type (Offset)
as it'll never evaluate to true.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Dandan Bi 
Contributed-under: TianoCore Contribution Agreement 1.1
Reported-by: Star Zeng 
Signed-off-by: Paulo Alcantara 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7286265373..2039f80289 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1082,9 +1082,6 @@ ReadFile (
 
 if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
   Offset = ReadFileInfo->FilePosition - FilePosition;
-  if (Offset < 0) {
-Offset = -(Offset);
-  }
 } else {
   Offset = 0;
 }
-- 
2.11.0

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

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools

[Paulo Alcantara]

Hi,

On 13/09/2017 00:31, Zeng, Star wrote:

Could you help send the patch quickly? As it breaks some platforms build and 
blocks others' development on that platform, for example Nt32.


Yep. Sorry for the delay. I'll send it shortly.

Thanks!
Paulo




Thanks,
Star
-Original Message-
From: Paulo Alcantara [mailto:pca...@zytor.com]
Sent: Tuesday, September 12, 2017 9:03 PM
To: Zeng, Star ; Bi, Dandan ; 
edk2-devel@lists.01.org
Cc: Dong, Eric ; Ni, Ruiyu ; Gao, Liming 
; Laszlo Ersek (ler...@redhat.com) 
Subject: Re: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build 
failure in VS tools

Hi,

On 9/12/2017 6:39 AM, Zeng, Star wrote:

There is change(type cast to INT64) below in this patch. After check, we found the " if 
(Offset < 0) " should be always false comparison as Offset is UINT64 type.
I have suggested Dandan to remove this change(type case to INT64) at v3 patch 
series https://lists.01.org/pipermail/edk2-devel/2017-September/014523.html.
Could you help check and fix the code appropriately?
 if (Offset < 0) {
-Offset = -(Offset);
+Offset = - (INT64) (Offset);
 }


Oh, nice catch! I'll send a patch that fixes it and do some sanity checks later.

Thank you all! Really appreciate it.

Paulo




Thanks,
Star
-Original Message-
From: Bi, Dandan
Sent: Monday, September 11, 2017 2:17 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric ; Paulo Alcantara
; Ni, Ruiyu ; Zeng, Star

Subject: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix
build failure in VS tools

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
   .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 18 
+-
   1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ea3f5fb..bf33ae4 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -477,11 +477,11 @@ DuplicateFid (
 OUT  UDF_FILE_IDENTIFIER_DESCRIPTOR  **NewFileIdentifierDesc
 )
   {
 *NewFileIdentifierDesc =
   (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
-  GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
+  (UINTN) GetFidDescriptorLength (FileIdentifierDesc),
+ FileIdentifierDesc);
   }
   
   //

   // Duplicate either a given File Entry or a given Extended File Entry.
   //
@@ -814,20 +814,20 @@ GetAedAdsData (
 }
   
 //

 // Allocate buffer to read in AED's data.
 //
-  *Data = AllocatePool (*Length);
+  *Data = AllocatePool ((UINTN) (*Length));
 if (*Data == NULL) {
   return EFI_OUT_OF_RESOURCES;
 }
   
 return DiskIo->ReadDisk (

   DiskIo,
   BlockIo->Media->MediaId,
   Offset,
-*Length,
+(UINTN) (*Length),
   *Data
   );
   }
   
   //

@@ -849,11 +849,11 @@ GrowUpBufferToNextAd (
   *Buffer = AllocatePool (ExtentLength);
   if (*Buffer == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 } else {
-*Buffer = ReallocatePool (Length, Length + ExtentLength, *Buffer);
+*Buffer = ReallocatePool ((UINTN) Length, (UINTN) (Length +
+ ExtentLength), *Buffer);
   if (*Buffer == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 }
   
@@ -938,29 +938,29 @@ ReadFile (

 ReadFileInfo->ReadLength = Length;
   } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
 //
 // Allocate buffer for starting read data.
 //
-  ReadFileInfo->FileData = AllocatePool (Length);
+  ReadFileInfo->FileData = AllocatePool ((UINTN) Length);
 if (ReadFileInfo->FileData == NULL) {
   return EFI_OUT_OF_RESOURCES;
 }
   
 //

 // Read all inline data into ReadFileInfo->FileData
 //
-  CopyMem (ReadFileInfo->FileData, Data, Length);
+  CopyMem (ReadFileInfo->FileData, Data, (UINTN) Length);
 ReadFileInfo->ReadLength = Length;
   } else if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ) {
 //
 // If FilePosition is non-zero, seek file to FilePosition, read
 // FileDataSize bytes and then updates FilePosition.
 //
 CopyMem (
   ReadFileInfo->FileData,
   (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
-ReadFileInfo->FileDataSize
+(UINTN) ReadFileInfo->FileDataSize
   );
   
 ReadFileInfo->FilePosition += ReadFileInfo->FileDataSize;

   } else {
  

Re: [edk2] StartImage() return value an error or return code ?

[David F.]

But what if you're running some unknown app that you don't know the
return code format in the ExitData?  It seems to just read that the
ExitData will be some text followed by some binary data.

On Mon, Sep 11, 2017 at 12:43 AM, Gao, Liming  wrote:
> Per UEFI spec, StartImage() has the optional output ExitData. If ExitData is 
> not NULL, the exit status will be from the image.
>
>>-Original Message-
>>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>David F.
>>Sent: Friday, September 08, 2017 2:53 PM
>>To: edk2-devel@lists.01.org
>>Subject: [edk2] StartImage() return value an error or return code ?
>>
>>Hi,
>>
>>Is there a way to tell if the return code from StartImage() is
>>actually an error from StartImage() vs the exit code of some
>>application (generic application that may return an error code value
>>but not set exitdata).It would seem to conflict with
>>EFI_INVALID_PARAMETER and EFI_SECURITY_VIOLATION  so where actual
>>value came from is unknown?
>>
>>TIA!!
>>___
>>edk2-devel mailing list
>>edk2-devel@lists.01.org
>>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools

[Zeng, Star]

Could you help send the patch quickly? As it breaks some platforms build and 
blocks others' development on that platform, for example Nt32.


Thanks,
Star
-Original Message-
From: Paulo Alcantara [mailto:pca...@zytor.com] 
Sent: Tuesday, September 12, 2017 9:03 PM
To: Zeng, Star ; Bi, Dandan ; 
edk2-devel@lists.01.org
Cc: Dong, Eric ; Ni, Ruiyu ; Gao, 
Liming ; Laszlo Ersek (ler...@redhat.com) 

Subject: Re: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build 
failure in VS tools

Hi,

On 9/12/2017 6:39 AM, Zeng, Star wrote:
> There is change(type cast to INT64) below in this patch. After check, we 
> found the " if (Offset < 0) " should be always false comparison as Offset is 
> UINT64 type.
> I have suggested Dandan to remove this change(type case to INT64) at v3 patch 
> series https://lists.01.org/pipermail/edk2-devel/2017-September/014523.html.
> Could you help check and fix the code appropriately?
> if (Offset < 0) {
> -Offset = -(Offset);
> +Offset = - (INT64) (Offset);
> }

Oh, nice catch! I'll send a patch that fixes it and do some sanity checks later.

Thank you all! Really appreciate it.

Paulo

> 
> 
> Thanks,
> Star
> -Original Message-
> From: Bi, Dandan
> Sent: Monday, September 11, 2017 2:17 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric ; Paulo Alcantara 
> ; Ni, Ruiyu ; Zeng, Star 
> 
> Subject: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix 
> build failure in VS tools
> 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>   .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 18 
> +-
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index ea3f5fb..bf33ae4 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -477,11 +477,11 @@ DuplicateFid (
> OUT  UDF_FILE_IDENTIFIER_DESCRIPTOR  **NewFileIdentifierDesc
> )
>   {
> *NewFileIdentifierDesc =
>   (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
> -  GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
> +  (UINTN) GetFidDescriptorLength (FileIdentifierDesc), 
> + FileIdentifierDesc);
>   }
>   
>   //
>   // Duplicate either a given File Entry or a given Extended File Entry.
>   //
> @@ -814,20 +814,20 @@ GetAedAdsData (
> }
>   
> //
> // Allocate buffer to read in AED's data.
> //
> -  *Data = AllocatePool (*Length);
> +  *Data = AllocatePool ((UINTN) (*Length));
> if (*Data == NULL) {
>   return EFI_OUT_OF_RESOURCES;
> }
>   
> return DiskIo->ReadDisk (
>   DiskIo,
>   BlockIo->Media->MediaId,
>   Offset,
> -*Length,
> +(UINTN) (*Length),
>   *Data
>   );
>   }
>   
>   //
> @@ -849,11 +849,11 @@ GrowUpBufferToNextAd (
>   *Buffer = AllocatePool (ExtentLength);
>   if (*Buffer == NULL) {
> return EFI_OUT_OF_RESOURCES;
>   }
> } else {
> -*Buffer = ReallocatePool (Length, Length + ExtentLength, *Buffer);
> +*Buffer = ReallocatePool ((UINTN) Length, (UINTN) (Length + 
> + ExtentLength), *Buffer);
>   if (*Buffer == NULL) {
> return EFI_OUT_OF_RESOURCES;
>   }
> }
>   
> @@ -938,29 +938,29 @@ ReadFile (
> ReadFileInfo->ReadLength = Length;
>   } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
> //
> // Allocate buffer for starting read data.
> //
> -  ReadFileInfo->FileData = AllocatePool (Length);
> +  ReadFileInfo->FileData = AllocatePool ((UINTN) Length);
> if (ReadFileInfo->FileData == NULL) {
>   return EFI_OUT_OF_RESOURCES;
> }
>   
> //
> // Read all inline data into ReadFileInfo->FileData
> //
> -  CopyMem (ReadFileInfo->FileData, Data, Length);
> +  CopyMem (ReadFileInfo->FileData, Data, (UINTN) Length);
> ReadFileInfo->ReadLength = Length;
>   } else if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ) {
> //
> // If FilePosition is non-zero, seek file to FilePosition, read
> // FileDataSize bytes and then updates FilePosition.
> //
> CopyMem (
>   ReadFileInfo->FileData,
>   (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
> -ReadFileInfo->FileDataSize
> +(UINTN) ReadFileInfo->FileDataSize
>   );
>   
> 

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ni, Ruiyu]

I am going to hold the patch.
Unless some real issue is exposed using current PciBus driver, I will not make 
any change to current implementation.


Thanks/Ray

> -Original Message-
> From: Benjamin Herrenschmidt [mailto:b...@au1.ibm.com]
> Sent: Wednesday, September 13, 2017 7:38 AM
> To: Laszlo Ersek ; Ard Biesheuvel
> 
> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; Dong Wei
> ; Andrew Fish 
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes()
> returns Host address
> 
> On Tue, 2017-09-12 at 18:15 +0200, Laszlo Ersek wrote:
> > > So IIUC, if we were to decide that AddrRangeMin contains the raw BAR
> > > value, and the translation offset that needs to be applied to
> > > produce the CPU address is added to it, we are quite close to the
> > > intent of the definition of QWord, and our PCI I/O code is correct.
> > > Only in this case, we need to fix all users of the protocol (i.e.,
> > > GOP producers)
> >
> > I'd be totally OK with that...
> 
> Which happens to be the approach I took in my original port looking back at
> my code.
> 
> > > Given the low likelihood that this ever worked correctly for cases
> > > where the translation offset != 0, I think that is perhaps the best
> > > course of action.
> >
> > ...as long as the USWG agreed to invert the sense of the fields in the
> > UEFI spec, based on which the GOPs should be updated.
> 
> I think we had that conversation back then ;-) I remember a number of
> inconsistencies in some of the resource fields including existing cases of 
> UEFI
> not quite following the ACPI spec.
> 
> However:
> 
> > In practice this would mean reverting
> >  > mantis_view.php-3Fid-3D1502=DwICaQ=jf_iaSHvJObTbx-
> siA1ZOg=ilASlO
> >
> u_ee2uzSGEkp3JMw=Y8F47uHCO5ZLYtvPhj45KfJDaIOCvYcJVK7HG2h7PC
> M=klaAU
> > FJYXQhUuF3Ad3BNs3HfM_8TpbgsENeLHRlI-vc== >. By now the fix for
> > Mantis#1502 has been in three released versions of the spec (one of
> > the
> > 2.5 Errata, 2.6 and 2.7).
> >
> > I'm fine both ways, as long as code and spec are consistent. From a
> > development perspective though, I think the spec is harder to change
> > than the code, no matter how ugly the code ends up.
> 
> That's a bit annoying. We need to ensure nobody already implemented
> something based on the above and rleased it...
> 
> Note there's another problem in EDK implementation. The
> AddressTranslationOffset field is hijacked in GetProposedResources return
> code iirc (at least back then, this might have changed since).
> 
> I had this patch to the header (and corresponding implementation
> changes) in my tree in
> MdePkg/Include/Protocol/PciHostBridgeResourceAllocation.h:
> 
>  ///
> -/// A UINT64 value that contains the status of a PCI resource requested -///
> in the Configuration parameter returned by GetProposedResources() -///
> The legal values are EFI_RESOURCE_SATISFIED and
> EFI_RESOURCE_NOT_SATISFIED
> +/// If this bit is set, then the PCI Root Bridge doesn't support /// IO
> +space
>  ///
> -typedef UINT64 EFI_RESOURCE_ALLOCATION_STATUS;
> +#define EFI_PCI_HOST_BRIDGE_NO_IO_DECODE   4
> 
>  ///
> -/// The request of this resource type could be fulfilled.  Used in the -///
> Configuration parameter returned by GetProposedResources() to identify -
> /// a PCI resources request that can be satisfied.
> +/// The "Address Translation Offset" field of the ACPI descriptor ///
> +returned by GetProposedResources() can contain either of these ///
> +ranges of *unsigned* values:
> +///
> +///  EFI_RESOURCE_NONEXISTENT : The resource request cannot be
> +satisfied ///
> +///  EFI_RESOURCE_LESS: The resource wasn't satisified but a small
> +/// request could be
>  ///
> -#define EFI_RESOURCE_SATISFIED  0xULL
> +///  Value < EFI_RESOURCE_OFFSET_LIMIT
> +///   : An offset representing the translation from
> +/// PCI addresses to CPU addresses
> +//
> +#define EFI_RESOURCE_NONEXISTENT  0xULL
> +#define EFI_RESOURCE_LESS 0xFFFEULL
> +#define EFI_RESOURCE_OFFSET_LIMIT 0xFFF0ULL
> +
> +// Helper for internal use of PciBusDxe
> +#define EFI_RESOURCE_SATISFIED0
> 
>  ///
> -/// The request of this resource type could not be fulfilled for its -///
> absence in the host bridge resource pool.  Used in the Configuration
> parameter -/// returned by GetProposedResources() to identify a PCI
> resources request that -/// can not be satisfied.
> +/// The request of this resource type could be fulfilled.  Used in the
> +/// Configuration parameter returned by GetProposedResources() to
> +identify /// a PCI resources request that can be satisfied.
>  ///
> -#define EFI_RESOURCE_NOT_SATISFIED  0xULL
> +#define EFI_RESOURCE_IS_SATISFIED(Offset) \
> +  

Re: [edk2] [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, September 13, 2017 6:26 AM
> To: edk2-devel-01 
> Cc: Ard Biesheuvel ; Dong, Eric
> ; Paulo Alcantara ; Ni, Ruiyu
> ; Zeng, Star 
> Subject: [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in
> ICB.Flags[2:0]
> 
> The ECMA-167 standard (3rd Edition, June 1997) reserves values 4 through 7
> in the ICB.Flags[2:0] bit-field for future standardization; see "14.6 ICB 
> Tag" /
> "14.6.8 Flags (RBP 18)".
> 
> https://www.ecma-international.org/publications/standards/Ecma-167.htm
> 
> The
> 
>   switch (RecordingFlags)
> 
> statement in the ReadFile() function handles all the standard values, using
> the constants of the UDF_FE_RECORDING_FLAGS enum type. However, the
> reserved values are not caught with a "default" case label, which both breaks
> the edk2 Coding Style Spec, and leaves the Status variable un-initialized,
> before we return Status under the Done label.
> 
> Set Status to EFI_UNSUPPORTED if we encounter a reserved value.
> 
> This issue was reported by Ard's and Gerd's CI systems independently
> (through build failures with GCC48/GCC49, DEBUG/RELEASE targets).
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Reported-by: Ard Biesheuvel 
> Reported-by: Gerd Hoffmann 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8
> 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 72862653738e..096fbb4452cb 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -1150,6 +1150,14 @@ ReadFile (
>  ASSERT (FALSE);
>  Status = EFI_UNSUPPORTED;
>  break;
> +
> +  default:
> +//
> +// A flag value reserved by the ECMA-167 standard (3rd Edition - June
> +// 1997); 14.6 ICB Tag; 14.6.8 Flags (RBP 18); was found.
> +//
> +Status = EFI_UNSUPPORTED;
> +break;
>}
> 
>  Done:
> --
> 2.14.1.3.gb7cf6e02401b
> 

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

[edk2] Spi/MvSpiDxe likely NullPtrException

[methavanitpong.pipat]

Hi,

MvSpiDxe.c:MvSpiTransfer() attempts load data into DataInPtr, which is
a possibly NULL pointer. 


https://github.com/tianocore/edk2-platforms/blob/master/Platform/Marvell/Drivers/Spi/MvSpiDxe.c#L228
/ SNIP /
for (Iterator = 0; Iterator < SPI_TIMEOUT; Iterator++) {
  if (MmioRead32 (SpiRegBase + SPI_INT_CAUSE_REG)) {
*DataInPtr = MmioRead32 (SpiRegBase + SPI_DATA_IN_REG);


One example that DataInPtr == NULL is in MvSpiDxe.c:MvReadWrite() in 
the first MvSpiTransfer() call. 


https://github.com/tianocore/edk2-platforms/blob/master/Platform/Marvell/Drivers/Spi/MvSpiDxe.c#L276
/ SNIP /
Status = MvSpiTransfer (This, Slave, CmdSize, Cmd, NULL, 
SPI_TRANSFER_BEGIN);
if (EFI_ERROR (Status)) {
  Print (L"Spi Transfer Error\n");
  return EFI_DEVICE_ERROR;
}

Status = MvSpiTransfer (This, Slave, DataSize, DataOut, DataIn, 
SPI_TRANSFER_END);
if (EFI_ERROR (Status)) {
  Print (L"Spi Transfer Error\n");
  return EFI_DEVICE_ERROR;
}


It should store data from SPI_DATA_IN_REG in a temporary variable first, 
then passes the data into DataInPtr if DataInPtr != NULL. 
--
Pipat Methavanitpong
Software Developer, S-Project 3
Socionext Inc.

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

[edk2] [PATCH 0/2] MdeModulePkg: UDF fixes and cleanups, round 2

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: udf_fixes_cleanups_round2

Once these patches are sufficiently reviewed, please don't wait for me
to commit them.

Further UdfDxe issues should be please reported in the TianoCore
Bugzilla.

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 

Thanks
Laszlo

Laszlo Ersek (2):
  MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]
  MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13 +
 1 file changed, 13 insertions(+)

-- 
2.14.1.3.gb7cf6e02401b

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

[edk2] [PATCH 1/2] MdeModulePkg/UdfDxe: reject reserved values in ICB.Flags[2:0]

[Laszlo Ersek]

The ECMA-167 standard (3rd Edition, June 1997) reserves values 4 through 7
in the ICB.Flags[2:0] bit-field for future standardization; see "14.6 ICB
Tag" / "14.6.8 Flags (RBP 18)".

https://www.ecma-international.org/publications/standards/Ecma-167.htm

The

  switch (RecordingFlags)

statement in the ReadFile() function handles all the standard values,
using the constants of the UDF_FE_RECORDING_FLAGS enum type. However, the
reserved values are not caught with a "default" case label, which both
breaks the edk2 Coding Style Spec, and leaves the Status variable
un-initialized, before we return Status under the Done label.

Set Status to EFI_UNSUPPORTED if we encounter a reserved value.

This issue was reported by Ard's and Gerd's CI systems independently
(through build failures with GCC48/GCC49, DEBUG/RELEASE targets).

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Reported-by: Ard Biesheuvel 
Reported-by: Gerd Hoffmann 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 72862653738e..096fbb4452cb 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -1150,6 +1150,14 @@ ReadFile (
 ASSERT (FALSE);
 Status = EFI_UNSUPPORTED;
 break;
+
+  default:
+//
+// A flag value reserved by the ECMA-167 standard (3rd Edition - June
+// 1997); 14.6 ICB Tag; 14.6.8 Flags (RBP 18); was found.
+//
+Status = EFI_UNSUPPORTED;
+break;
   }
 
 Done:
-- 
2.14.1.3.gb7cf6e02401b


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

[edk2] [PATCH 2/2] MdeModulePkg/UdfDxe: suppress incorrect compiler warning in ReadFile()

[Laszlo Ersek]

When building the driver for DEBUG/RELEASE, GCC48/GCC49 warn about
ReadFile() possibly using "BytesLeft" without initializing it first.

This is not the case. The reads of "BytesLeft" are only reachable if
(ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ). But, in that case, we
also set "BytesLeft" to "ReadFileInfo->FileDataSize", near the top of the
function.

Assign "BytesLeft" zero at the top, and add a comment that conforms to the
pending Coding Style Spec feature request at
.

This issue was reported by Ard's and Gerd's CI systems independently.

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Reported-by: Ard Biesheuvel 
Reported-by: Gerd Hoffmann 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 096fbb4452cb..392494b2eb3f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -893,6 +893,11 @@ ReadFile (
   LogicalBlockSize  = LV_BLOCK_SIZE (Volume, UDF_DEFAULT_LV_NUM);
   DoFreeAed = FALSE;
 
+  //
+  // set BytesLeft to suppress incorrect compiler/analyzer warnings
+  //
+  BytesLeft = 0;
+
   switch (ReadFileInfo->Flags) {
   case READ_FILE_GET_FILESIZE:
   case READ_FILE_ALLOCATE_AND_READ:
-- 
2.14.1.3.gb7cf6e02401b

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

Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Laszlo Ersek]

On 09/12/17 17:38, Ard Biesheuvel wrote:
> On 12 September 2017 at 03:14, Laszlo Ersek  wrote:
>> On 09/10/17 02:12, Laszlo Ersek wrote:
>>> Repo:   https://github.com/lersek/edk2.git
>>> Branch: udf_fixes_cleanups
>>>
>>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>>
>>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>>> patch #4, respectively.
>>>
>>> Cc: Ard Biesheuvel 
>>> Cc: Eric Dong 
>>> Cc: Paulo Alcantara 
>>> Cc: Ruiyu Ni 
>>> Cc: Star Zeng 
>>>
>>> Thanks
>>> Laszlo
>>>
>>> Laszlo Ersek (5):
>>>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>>> req
>>>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>>> succeeds
>>>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>>> ZeroMem()
>>>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>>>   MdeModulePkg/PartitionDxe: remove always false comparison
>>>
>>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c| 9 +++--
>>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 --
>>>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +
>>>  3 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>
>> Thanks all for the feedback, pushed as commit range
>> c05cae55ebd8..b4e5807d2492.
>>
>> (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
>> to touch the code on such an urgent push, relative to the posted and
>> tested version.)
>>
> 
> Rather unexpectedly, the build is still broken on my CI system
> 
> This time, it is GCC 4.8 that complains about uninitialized variables,
> most likely false positives again (apologies for the letter soup)
> 
> :
> In function 'ReadFile':
> :876:27:
> error: 'Status' may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>EFI_STATUS  Status;
>^

This is actually a bug in the code.

In order to explain that, I have to elaborate.

The UDF standard from the OSTA is introduced like this:

The OSTA Universal Disk Format (UDF ® ) specification defines a
subset of the standard ECMA 167 3 rd edition. The primary goal of
the OSTA UDF is to maximize data interchange and minimize the cost
and complexity of implementing ECMA 167.

Which (in retrospect...) means that we have to download ECMA 167 at once:

https://www.ecma-international.org/publications/standards/Ecma-167.htm

OK, so let's look at the code.

First, the ReadFile() function is very hard to read. It is long (~300
lines), but what is actually problematic about it is that it uses "goto"
statements for control flow that is *not* related to error handling.
This makes it painful to track, and it happens to violate the edk2
coding style spec as well:

https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html#57-c-programming

5.7.3.8 Goto Statements should not be used (in general)

In almost all cases, it is possible to write the code so that a goto
is not needed. If a goto is used, be ready to defend it during
review.

It is common to use goto for error handling and thus, exiting a
routine in an error case is the only legal use of a goto. A goto
allows the error exit code to be contained in one place in the
routine. This one place reduces software life cycle maintenance
issues, as there can be one copy of error cleanup code per routine.

   [...]

Second, in this case it actually pays off to read the function top-down.

- The first switch statement (on "ReadFileInfo->Flags") does not set
Status, but it also doesn't read it or jump to a read site.

- The second switch statement has four case labels (RecordingFlags):

  - INLINE_DATA: it sets Status to EFI_SUCCESS
(thanks to my recent patch)

  - LONG_ADS_SEQUENCE, SHORT_ADS_SEQUENCE: Status is set very
soon, when we unconditionally call GetAllocationDescriptor().

  - EXTENDED_ADS_SEQUENCE: we ASSERT(FALSE), but also set Status to
EFI_UNSUPPORTED

Then, assuming we didn't jump to any of the error labels
(Error_Read_Disk_Blk, Error_Alloc_Buffer_To_Next_Ad, Error_Get_Aed), we
proceed to the Done label, and return Status.

(BTW, I checked all the paths to those error labels, and those *are* fine.)

So how is it possible that we reach Done, and return Status, without
having set Status? It is possible by taking *none* of the branches in
the second switch statement (RecordingFlags). The switch statement does

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Laszlo Ersek]

On 09/12/17 17:49, Ard Biesheuvel wrote:
> On 12 September 2017 at 01:40, Laszlo Ersek  wrote:
>> On 09/12/17 08:44, Ard Biesheuvel wrote:
>>> On 12 September 2017 at 06:01, Ni, Ruiyu  wrote:
 Laszlo,
 Your understanding is: DeviceAddress = HostAddress + 
 AddressTranslationOffset
 But my patch assumes: HostAddress = DeviceAddress + 
 AddressTranslationOffset

 They are totally different. If I follow your understanding, the patch is 
 wrong!
 Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply 
 to the
 Starting address of a BAR to convert it to a PCI address" very clearly, I 
 quoted
 the statement from ACPI spec.
 Your understanding to "apply to" is "add", my understanding is "minus".

>>>
>>> Even though we are stretching the ACPI definition of a QWord
>>> descriptor beyond its original meaning, I don't think there is a lot
>>> of ambiguity here, to be honest. The AddrRangeMin field contains the
>>> address on the secondary side of a bridge, and the primary value can
>>> be obtained by 'applying' the ATO. In my opinion, applying a (positive
>>> or negative) offset implies addition, not subtraction, as subtraction
>>> involves negating the second addend before applying it. And the
>>> secondary side of the host bridge is clearly the PCI side.
>>
>> Wait, now I'm even more confused.
>>
>> (1) Up-thread you wrote, "AddrRangeMin is indeed already defined to be a
>> host address [...]".
>>
> 
> Yes.
> 
>> (2) Here you write, "the secondary side of the host bridge is clearly
>> the PCI side [...] The AddrRangeMin field contains the address on the
>> secondary side of a bridge". --> This means that AddrRangeMin is a PCI
>> address.
>>
> 
> Right. Now *I* am even more confused.
> 
>> Thus, to me these statements appear to conflict.
>>
> 
> Yes they do, apologies.
> 
>>> It does mean the offset field is signed, though.
>>>
>>> So I don't agree with the conclusion that no clarification is
>>> required. We need to make sure the spec is crystal clear in this
>>> regard. But I do agree with the change, I think it is the only
>>> solution that makes sense.
>>
>> My understanding of "Table 121. QWORD Address Space Descriptor" is:
>>
>> - AddrRangeMin --> host address.
>>
>> - ATO --> the UINT64 value that the *caller* of GetBarAttributes() has
>>   to add, in UINT64 modular arithmetic, to AddrRangeMin, to calculate
>>   the PCI address, after GetBarAttributes() returns.
>>
>> Now, if I understand the *patch* correctly,
>>
>> - the current (pre-patch) code returns a PCI address in
>>   "Descriptor->AddrRangeMin", which is wrong,
>>
>> - in addition, we already have the ATO, in
>>   "Descriptor->AddrTranslationOffset", that we have to add to the PCI
>>   address, to end up with a host address.
>>
>> If that's the case, then I think the patch is good, but it is
>> incomplete. Namely,
>>
>> - To return a host address to the caller in "Descriptor->AddrRangeMin",
>>   we add the ATO to it, fetched from the Root Bridge IO protocol. Great.
>>
>> - However, think of what happens when the caller wants to recompute the
>>   PCI address! According to the UEFI spec, the ATO that the caller gets
>>   in the QWORD descriptor has to be *added* to AddrRangeMin. This means
>>   that, the client code would ultimately result in:
>>
>>   ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + OriginalATO
>>
>> This makes no sense. In order to end up with the original PCI address,
>> the client side ATO must be the modular UINT64 *negative* of the
>> original ATO, so that they ultimately cancel out on the client side,
>> like this:
>>
>>   ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + ClientSideATO
>>== (OriginalPciAddress + OriginalATO) + (-OriginalATO)
>>== OriginalPciAddress
>>
>> Therefore, I think that the patch must, *in addition*, negate the ATO
>> before returning, like this:
>>
>> +  Descriptor->AddrRangeMin += Descriptor->AddrTranslationOffset;
>> +  Descriptor->AddrTranslationOffset = 
>> (-Descriptor->AddrTranslationOffset);
>>
> 
> Ugh. I think you're right. But now, I am no longer convinced
> AddrRangeMin should contain the host address, given that we are
> inverting the sense of both the AddrRangeMin field and the translation
> offset.
> 
> So IIUC, if we were to decide that AddrRangeMin contains the raw BAR
> value, and the translation offset that needs to be applied to produce
> the CPU address is added to it, we are quite close to the intent of
> the definition of QWord, and our PCI I/O code is correct. Only in this
> case, we need to fix all users of the protocol (i.e., GOP producers)

I'd be totally OK with that...

> Given the low likelihood that this ever worked correctly for cases
> where the translation offset != 0, I think that is perhaps the best
> course of action.

...as long as the USWG agreed to invert the sense of 

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ard Biesheuvel]

On 12 September 2017 at 01:40, Laszlo Ersek  wrote:
> On 09/12/17 08:44, Ard Biesheuvel wrote:
>> On 12 September 2017 at 06:01, Ni, Ruiyu  wrote:
>>> Laszlo,
>>> Your understanding is: DeviceAddress = HostAddress + 
>>> AddressTranslationOffset
>>> But my patch assumes: HostAddress = DeviceAddress + AddressTranslationOffset
>>>
>>> They are totally different. If I follow your understanding, the patch is 
>>> wrong!
>>> Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply 
>>> to the
>>> Starting address of a BAR to convert it to a PCI address" very clearly, I 
>>> quoted
>>> the statement from ACPI spec.
>>> Your understanding to "apply to" is "add", my understanding is "minus".
>>>
>>
>> Even though we are stretching the ACPI definition of a QWord
>> descriptor beyond its original meaning, I don't think there is a lot
>> of ambiguity here, to be honest. The AddrRangeMin field contains the
>> address on the secondary side of a bridge, and the primary value can
>> be obtained by 'applying' the ATO. In my opinion, applying a (positive
>> or negative) offset implies addition, not subtraction, as subtraction
>> involves negating the second addend before applying it. And the
>> secondary side of the host bridge is clearly the PCI side.
>
> Wait, now I'm even more confused.
>
> (1) Up-thread you wrote, "AddrRangeMin is indeed already defined to be a
> host address [...]".
>

Yes.

> (2) Here you write, "the secondary side of the host bridge is clearly
> the PCI side [...] The AddrRangeMin field contains the address on the
> secondary side of a bridge". --> This means that AddrRangeMin is a PCI
> address.
>

Right. Now *I* am even more confused.

> Thus, to me these statements appear to conflict.
>

Yes they do, apologies.

>> It does mean the offset field is signed, though.
>>
>> So I don't agree with the conclusion that no clarification is
>> required. We need to make sure the spec is crystal clear in this
>> regard. But I do agree with the change, I think it is the only
>> solution that makes sense.
>
> My understanding of "Table 121. QWORD Address Space Descriptor" is:
>
> - AddrRangeMin --> host address.
>
> - ATO --> the UINT64 value that the *caller* of GetBarAttributes() has
>   to add, in UINT64 modular arithmetic, to AddrRangeMin, to calculate
>   the PCI address, after GetBarAttributes() returns.
>
> Now, if I understand the *patch* correctly,
>
> - the current (pre-patch) code returns a PCI address in
>   "Descriptor->AddrRangeMin", which is wrong,
>
> - in addition, we already have the ATO, in
>   "Descriptor->AddrTranslationOffset", that we have to add to the PCI
>   address, to end up with a host address.
>
> If that's the case, then I think the patch is good, but it is
> incomplete. Namely,
>
> - To return a host address to the caller in "Descriptor->AddrRangeMin",
>   we add the ATO to it, fetched from the Root Bridge IO protocol. Great.
>
> - However, think of what happens when the caller wants to recompute the
>   PCI address! According to the UEFI spec, the ATO that the caller gets
>   in the QWORD descriptor has to be *added* to AddrRangeMin. This means
>   that, the client code would ultimately result in:
>
>   ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + OriginalATO
>
> This makes no sense. In order to end up with the original PCI address,
> the client side ATO must be the modular UINT64 *negative* of the
> original ATO, so that they ultimately cancel out on the client side,
> like this:
>
>   ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + ClientSideATO
>== (OriginalPciAddress + OriginalATO) + (-OriginalATO)
>== OriginalPciAddress
>
> Therefore, I think that the patch must, *in addition*, negate the ATO
> before returning, like this:
>
> +  Descriptor->AddrRangeMin += Descriptor->AddrTranslationOffset;
> +  Descriptor->AddrTranslationOffset = 
> (-Descriptor->AddrTranslationOffset);
>

Ugh. I think you're right. But now, I am no longer convinced
AddrRangeMin should contain the host address, given that we are
inverting the sense of both the AddrRangeMin field and the translation
offset.

So IIUC, if we were to decide that AddrRangeMin contains the raw BAR
value, and the translation offset that needs to be applied to produce
the CPU address is added to it, we are quite close to the intent of
the definition of QWord, and our PCI I/O code is correct. Only in this
case, we need to fix all users of the protocol (i.e., GOP producers)

Given the low likelihood that this ever worked correctly for cases
where the translation offset != 0, I think that is perhaps the best
course of action.

Apologies for adding to the confusion.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/5] MdeModulePkg: UDF fixes and cleanups

[Ard Biesheuvel]

On 12 September 2017 at 03:14, Laszlo Ersek  wrote:
> On 09/10/17 02:12, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: udf_fixes_cleanups
>>
>> Patches #2, #3 and #4 are needed (and enough) for me to build OVMF for
>> IA32 and X64 with clang-3.8, after the UDF introduction.
>>
>> Patches #1 and #5 are cleanups that I felt fit before patch #2 and after
>> patch #4, respectively.
>>
>> Cc: Ard Biesheuvel 
>> Cc: Eric Dong 
>> Cc: Paulo Alcantara 
>> Cc: Ruiyu Ni 
>> Cc: Star Zeng 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (5):
>>   MdeModulePkg/UdfDxe: ASSERT() valid ReadFileInfo Flags for INLINE_DATA
>> req
>>   MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req
>> succeeds
>>   MdeModulePkg/UdfDxe: replace zero-init of local variables with
>> ZeroMem()
>>   MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators
>>   MdeModulePkg/PartitionDxe: remove always false comparison
>>
>>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c| 9 +++--
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 --
>>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 5 +
>>  3 files changed, 16 insertions(+), 4 deletions(-)
>>
>
> Thanks all for the feedback, pushed as commit range
> c05cae55ebd8..b4e5807d2492.
>
> (I didn't change the sizeof / sizeof() stuff -- for one, I didn't want
> to touch the code on such an urgent push, relative to the posted and
> tested version.)
>

Rather unexpectedly, the build is still broken on my CI system

This time, it is GCC 4.8 that complains about uninitialized variables,
most likely false positives again (apologies for the letter soup)

:
In function 'ReadFile':
:876:27:
error: 'Status' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   EFI_STATUS  Status;
   ^
"/home/buildslave/srv/toolchain/arm-tc-14.04/bin/arm-linux-gnueabihf-gcc"
-mthumb -mcpu=cortex-a15
-I
-g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror
-Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian
-mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections
-fdata-sections -fomit-frame-pointer -Wno-address -mthumb
-mfloat-abi=soft -fno-pic -fno-pie -fstack-protector
-mword-relocations -Wno-unused-but-set-variable -DMDEPKG_NDEBUG
-DDISABLE_NEW_DEPRECATED_INTERFACES -c -o

-I
-I
-I
-I
-I
-I
-I

:887:27:
error: 'BytesLeft' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
   UINT64  BytesLeft;
   ^
cc1: all warnings being treated as errors
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools

[Paulo Alcantara]

Hi,

On 9/12/2017 6:39 AM, Zeng, Star wrote:

There is change(type cast to INT64) below in this patch. After check, we found the " if 
(Offset < 0) " should be always false comparison as Offset is UINT64 type.
I have suggested Dandan to remove this change(type case to INT64) at v3 patch 
series https://lists.01.org/pipermail/edk2-devel/2017-September/014523.html.
Could you help check and fix the code appropriately?
if (Offset < 0) {
-Offset = -(Offset);
+Offset = - (INT64) (Offset);
}


Oh, nice catch! I'll send a patch that fixes it and do some sanity 
checks later.


Thank you all! Really appreciate it.

Paulo




Thanks,
Star
-Original Message-
From: Bi, Dandan
Sent: Monday, September 11, 2017 2:17 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric ; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure 
in VS tools

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 18 +-
  1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ea3f5fb..bf33ae4 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -477,11 +477,11 @@ DuplicateFid (
OUT  UDF_FILE_IDENTIFIER_DESCRIPTOR  **NewFileIdentifierDesc
)
  {
*NewFileIdentifierDesc =
  (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
-  GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
+  (UINTN) GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
  }
  
  //

  // Duplicate either a given File Entry or a given Extended File Entry.
  //
@@ -814,20 +814,20 @@ GetAedAdsData (
}
  
//

// Allocate buffer to read in AED's data.
//
-  *Data = AllocatePool (*Length);
+  *Data = AllocatePool ((UINTN) (*Length));
if (*Data == NULL) {
  return EFI_OUT_OF_RESOURCES;
}
  
return DiskIo->ReadDisk (

  DiskIo,
  BlockIo->Media->MediaId,
  Offset,
-*Length,
+(UINTN) (*Length),
  *Data
  );
  }
  
  //

@@ -849,11 +849,11 @@ GrowUpBufferToNextAd (
  *Buffer = AllocatePool (ExtentLength);
  if (*Buffer == NULL) {
return EFI_OUT_OF_RESOURCES;
  }
} else {
-*Buffer = ReallocatePool (Length, Length + ExtentLength, *Buffer);
+*Buffer = ReallocatePool ((UINTN) Length, (UINTN) (Length + ExtentLength), 
*Buffer);
  if (*Buffer == NULL) {
return EFI_OUT_OF_RESOURCES;
  }
}
  
@@ -938,29 +938,29 @@ ReadFile (

ReadFileInfo->ReadLength = Length;
  } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
//
// Allocate buffer for starting read data.
//
-  ReadFileInfo->FileData = AllocatePool (Length);
+  ReadFileInfo->FileData = AllocatePool ((UINTN) Length);
if (ReadFileInfo->FileData == NULL) {
  return EFI_OUT_OF_RESOURCES;
}
  
//

// Read all inline data into ReadFileInfo->FileData
//
-  CopyMem (ReadFileInfo->FileData, Data, Length);
+  CopyMem (ReadFileInfo->FileData, Data, (UINTN) Length);
ReadFileInfo->ReadLength = Length;
  } else if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ) {
//
// If FilePosition is non-zero, seek file to FilePosition, read
// FileDataSize bytes and then updates FilePosition.
//
CopyMem (
  ReadFileInfo->FileData,
  (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
-ReadFileInfo->FileDataSize
+(UINTN) ReadFileInfo->FileDataSize
  );
  
ReadFileInfo->FilePosition += ReadFileInfo->FileDataSize;

  } else {
ASSERT (FALSE);
@@ -1081,11 +1081,11 @@ ReadFile (
  }
  
  if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {

Offset = ReadFileInfo->FilePosition - FilePosition;
if (Offset < 0) {
-Offset = -(Offset);
+Offset = - (INT64) (Offset);
}
  } else {
Offset = 0;
  }
  
@@ -1109,11 +1109,11 @@ ReadFile (

  //
  Status = DiskIo->ReadDisk (
DiskIo,
BlockIo->Media->MediaId,
Offset + MultU64x32 (Lsn, LogicalBlockSize),
-  DataLength,
+  (UINTN) DataLength,
(VOID *)((UINT8 *)ReadFileInfo->FileData +
 DataOffset)
);
  if (EFI_ERROR (Status)) {
goto 

Re: [edk2] [PATCH] MdeModulePkg/UdfDxe: Fix NULL pointer dereference

[Laszlo Ersek]

On 09/12/17 03:30, Paulo Alcantara wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=704
> 
> For root directory, the FID (File Identifier Descriptor) pointer is
> accessible through PRIVATE_UDF_FILE_DATA.Root, whereas non-root
> directory and regular files, their FIDs are accessible through
> PRIVATE_UDF_FILE_DATA.File.
> 
> In UdfSetPosition(), the FID was retrieved through
> PRIVATE_UDF_FILE_DATA.File, hence when calling it with a root directory,
> PRIVATE_UDF_FILE_DATA.File.FileIdentifierDescriptor would be NULL and
> then dereferenced.
> 
> This patch fixes the NULL pointer dereference by calling _FILE() to
> transparently return the correct UDF_FILE_INFO * which points to a valid
> FID descriptor of a specific file.
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Ruiyu Ni 
> Cc: Laszlo Ersek 
> Cc: Steven Shi 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Reported-by: Steven Shi 
> Signed-off-by: Paulo Alcantara 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c 
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 8b9339567f..a1eb2196df 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -690,7 +690,8 @@ UdfSetPosition (
>  
>PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
>  
> -  FileIdentifierDesc = PrivFileData->File.FileIdentifierDesc;
> +  FileIdentifierDesc = _FILE (PrivFileData)->FileIdentifierDesc;
> +  ASSERT (FileIdentifierDesc != NULL);
>if (IS_FID_DIRECTORY_FILE (FileIdentifierDesc)) {
>  //
>  // If the file handle is a directory, the _only_ position that may be 
> set is
> 

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [PATCH 0/3] ArmVirtPkg: add HTTP_BOOT_ENABLE and UsbMassStorageDxe

[Laszlo Ersek]

On 09/12/17 08:48, Ard Biesheuvel wrote:
> On 11 September 2017 at 22:08, Laszlo Ersek  wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: armvirt_http_usbstor
>>
>> I was... ugh... experimenting with "stuff" over the weekend, and
>> realized these were missing.
>>
>> Cc: Ard Biesheuvel 
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (3):
>>   ArmVirtPkg: don't build the network stack uselessly for Xen
>>   ArmVirtPkg/ArmVirtQemu: port HTTP_BOOT_ENABLE from OvmfPkg
>>   ArmVirtPkg/ArmVirtQemu: include UsbMassStorageDxe
>>
>>  ArmVirtPkg/ArmVirt.dsc.inc   | 15 --
>>  ArmVirtPkg/ArmVirtQemu.dsc   | 31 
>>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 31 
>>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  7 +
>>  4 files changed, 69 insertions(+), 15 deletions(-)
>>
> 
> Reviewed-by: Ard Biesheuvel 

Thank you! Commit range b4e5807d2492..62ef40c4959e.

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

Re: [edk2] [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver for SPI NOR flash

[methavanitpong.pipat]

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Tuesday, September 12, 2017 5:39 PM
> To: Methavanitpong, Pipat/メタワニットポン ピパット
> 
> Cc: edk2-devel@lists.01.org; masahisa.koj...@linaro.org;
> masami.hirama...@linaro.org; ard.biesheu...@linaro.org
> Subject: Re: [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver
> for SPI NOR flash
> 
> On Tue, Sep 12, 2017 at 01:41:34AM +,
> methavanitpong.pi...@socionext.com wrote:
> > > -Original Message-
> > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > Sent: Tuesday, September 12, 2017 4:13 AM
> > > To: Ard Biesheuvel 
> > > Cc: edk2-devel@lists.01.org; Methavanitpong, Pipat/メタワニットポン ピ
> パット
> > > ; masahisa.koj...@linaro.org;
> > > masami.hirama...@linaro.org
> > > Subject: Re: [PATCH edk2-platforms 13/14] Silicon/Socionext: add
> > > driver for SPI NOR flash
> > >
> > > On Fri, Sep 08, 2017 at 07:23:14PM +0100, Ard Biesheuvel wrote:
> > > > This imports the driver sources provided by Socionext for the
> > > > FIP006 SPI NOR flash device found on Synquacer SoCs. It has been
> > > > slightly tweaked to bring it up to date with the changes made on
> > > > the EDK2 side since it was forked.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Ard Biesheuvel 
> 
> > [Methavanitpong, Pipat/メタワニットポン ピパット]
> > Hi Leif,
> >
> > 1. Hex magic
> >
> > There are 2 hex magic in this driver
> >
> >   1. FIP006 command sequencer decode command
> >
> >  FIP006 in command sequencer mode detects accesses to its memory
> region.
> >  An access to this region is translated into commands according to
> >  FIP006_REG_CS_RD and FIP006_REG_CS_WR for read and write
> accordingly.
> >
> >  A single access can be translated up to 8 1-byte commands, as the
> >  registers have 8 slots each. Each command is transmitted
> consecutively
> >  starting from their base addresses.
> >
> >  Each slot in the registers are decoded as
> >
> >  1. Immediate value - CSDC(imm, ..., ..., DEC=0)
> >  2. Addr[7:0] - CSDC(0x00, ..., ..., DEC=1)
> >  3. Addr[15:8] - CSDC(0x01, ..., ..., DEC=1)
> >  4. Addr[23:16] - CSDC(0x02, ..., ..., DEC=1)
> >  5. Addr[31:24] - CSDC(0x03, ..., ..., DEC=1)
> >  6. HiZ for 1 byte - CSDC(0x04, ..., ..., DEC=1)
> >  7. Upper 4-bit immediate value + 4-bit HiZ - CSDC((imm << 4) |
> 0x05, ..., ..., DEC=1)
> >  8. Command termination - CSDC(0x07, ..., ..., DEC=1)
> >
> >   2. Micron N25Q flash command
> >
> >  In order to send a command to a flash device, FIP006's
> FIP006_REG_CS_RD
> >  and FIP006_REG_CS_WR must be set properly. NorFlashSetHostCommand()
> >  sets these registers according to an instance's command definition
> table.
> >
> >  For example; NorFlashSetHostCommand (Instance, 0x13) corresponds to
> a
> >  read access with 4-byte addressing read serial nor flash command
> (0x13).
> >  The result FIP006_REG_CS_RD is the following:
> >
> >  FIP006_REG_CS_RD[0] = CSDC(0x13, 0, 1-bit lane, DEC=0)
> >  FIP006_REG_CS_RD[1] = CSDC(0x03, 0, 1-bit lane, DEC=1)
> >  FIP006_REG_CS_RD[2] = CSDC(0x02, 0, 1-bit lane, DEC=1)
> >  FIP006_REG_CS_RD[3] = CSDC(0x01, 0, 1-bit lane, DEC=1)
> >  FIP006_REG_CS_RD[4] = CSDC(0x00, 0, 1-bit lane, DEC=1)
> >  FIP006_REG_CS_RD[5] = CSDC(0x07, 0, 1-bit lane, DEC=1)
> >  FIP006_REG_CS_RD[6] = CSDC(0x07, 0, 1-bit lane, DEC=1)
> >  FIP006_REG_CS_RD[7] = CSDC(0x07, 0, 1-bit lane, DEC=1)
> 
> Thank you for the explanation.
> However, the code needs to be be readable by someone who does not have the
> instruction set for each given component fresh in their mind.
> 
> That means that direct numeral expressions should be avoided wherever they
> are not mathematically obvious (and where you consider them obvious,
> consider whether everyone else would agree without already knowing what
> the code is intended to do).
> 
> They should be replaced by #defines or enums as appropriate.
> 
> > 2. Bit field declaration
> >
> > About the bitfield struct you mentioned in "Fip006Reg.h", What should
> > be a good way to declare it?
> 
> The problem with bitfields is that they are not very well defined in the C
> language, and hence do not tend to be very portable.
> 
> Tedious as it may seem, #defines with shifts and masks are usually the way
> to go.

Thank you for your comment, Leif. 
I will see what I can do with Ard. 

> 
> Regards,
> 
> Leif

--
Pipat Methavanitpong
Software Developer, S-Project 3
Socionext Inc.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] Partition Driver started multiple times.

[Ramesh R .]

Hi,

Is there any reason why are we starting the partition driver again for the same 
partition if it's already started. 

  if (Status == EFI_ALREADY_STARTED) {
return EFI_SUCCESS;
  }

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

Re: [edk2] [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Laszlo Ersek]

On 09/12/17 11:38, Ni, Ruiyu wrote:
> Star,
> Sizeof is an operator, not a function, like + or -. Not having () is ok.

Ugh, just seeing this now :) So what should I do now?

If Star agrees, I would prefer *not* to add the parens. If Star insists,
I can add them.

Thanks
Laszlo

>> -Original Message-
>> From: Zeng, Star
>> Sent: Tuesday, September 12, 2017 4:55 PM
>> To: Laszlo Ersek ; edk2-devel-01 > de...@lists.01.org>
>> Cc: Ard Biesheuvel ; Dong, Eric
>> ; Paulo Alcantara ; Ni, Ruiyu
>> ; Zeng, Star 
>> Subject: RE: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
>> variables with ZeroMem()
>>
>> Reviewed-by: Star Zeng 
>>
>> BTW: How about to use "sizeof ()" instead of "sizeof"?
>>
>>
>> Thanks,
>> Star
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Sunday, September 10, 2017 8:13 AM
>> To: edk2-devel-01 
>> Cc: Ard Biesheuvel ; Dong, Eric
>> ; Paulo Alcantara ; Ni, Ruiyu
>> ; Zeng, Star 
>> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
>> variables with ZeroMem()
>>
>> In edk2, initialization of local variables is forbidden, both for stylistic 
>> reasons
>> and because such initialization may generate calls to compiler intrinsics.
>>
>> For the following initialization in UdfRead():
>>
>>   CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
>>
>> clang-3.8 generates a memset() call, when building UdfDxe for IA32, which
>> then fails to link.
>>
>> Replace the initialization with ZeroMem().
>>
>> Do the same to "FilePath" in UdfOpen().
>>
>> Cc: Ard Biesheuvel 
>> Cc: Eric Dong 
>> Cc: Paulo Alcantara 
>> Cc: Ruiyu Ni 
>> Cc: Star Zeng 
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek 
>> ---
>>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> index 8b9339567f8e..e7159ff861f7 100644
>> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
>> @@ -174,15 +174,16 @@ UdfOpen (
>>  {
>>EFI_TPL OldTpl;
>>EFI_STATUS  Status;
>>PRIVATE_UDF_FILE_DATA   *PrivFileData;
>>PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
>> -  CHAR16  FilePath[UDF_PATH_LENGTH] = { 0 };
>> +  CHAR16  FilePath[UDF_PATH_LENGTH];
>>UDF_FILE_INFO   File;
>>PRIVATE_UDF_FILE_DATA   *NewPrivFileData;
>>CHAR16  *TempFileName;
>>
>> +  ZeroMem (FilePath, sizeof FilePath);
>>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>if (This == NULL || NewHandle == NULL || FileName == NULL) {
>>  Status = EFI_INVALID_PARAMETER;
>>  goto Error_Invalid_Params;
>> @@ -322,14 +323,15 @@ UdfRead (
>>EFI_BLOCK_IO_PROTOCOL   *BlockIo;
>>EFI_DISK_IO_PROTOCOL*DiskIo;
>>UDF_FILE_INFO   FoundFile;
>>UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>>VOID*NewFileEntryData;
>> -  CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
>> +  CHAR16  FileName[UDF_FILENAME_LENGTH];
>>UINT64  FileSize;
>>UINT64  BufferSizeUint64;
>>
>> +  ZeroMem (FileName, sizeof FileName);
>>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>>
>>if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>>   Buffer == NULL)) {
>>  Status = EFI_INVALID_PARAMETER;
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
> 

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

Re: [edk2] [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Laszlo Ersek]

On 09/12/17 10:55, Zeng, Star wrote:
> Reviewed-by: Star Zeng 
> 
> BTW: How about to use "sizeof ()" instead of "sizeof"?

"sizeof" is a unary operator. The parentheses are mandatory when sizeof
is used with a type name, but when the operand of sizeof is an
expression, the parentheses frequently make the wrong impression.

Consider for example

  UINTN  Var1, Var2, Var3;
  STRUCTURE_TYPE VarStruct;

  Var1 = 5;
  Var2 = sizeof Var3 + Var1;// [1]

Most people dislike the assignment to Var2, and will rewrite it as:

  Var2 = sizeof (Var3) + Var1;  // [2]

However, this is totally irrelevant, and does not reflect the binding
strengths between the "sizeof" and "+" operators. In order to reflect
that relationship correctly, the following parentheses should be added:

  Var2 = (sizeof Var3) + Var1;  // [3]

None of [2] or [3] make any difference relative to [1], of course, in
behavior.

I just dislike [2] because it *suggests* that putting Var3 in parens
"protects" Var1 from falling under "sizeof".

That's not the case. "sizeof" is not a function, it is a unary operator
like "*", "&", "!", etc.

If we want to add parens for emphasizing the actual operator binding,
then [3] is the way to go. For example, if "Var3" was a pointer, you
wouldn't write

  *(Var3) + Var1

in order to emphasize that "*" takes precedence over "+";  you would write

  (*Var3) + Var1


TL;DR: "sizeof" is a unary, prefix operator, not a function designator.

... In order not to delay this any longer, I will add the parens before
I push.

But, we should all know that those parens don't mean what most people
think they mean :)

Laszlo


> 
> 
> Thanks,
> Star
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 
> Cc: Ard Biesheuvel ; Dong, Eric 
> ; Paulo Alcantara ; Ni, Ruiyu 
> ; Zeng, Star 
> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local 
> variables with ZeroMem()
> 
> In edk2, initialization of local variables is forbidden, both for stylistic 
> reasons and because such initialization may generate calls to compiler 
> intrinsics.
> 
> For the following initialization in UdfRead():
> 
>   CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
> 
> clang-3.8 generates a memset() call, when building UdfDxe for IA32, which 
> then fails to link.
> 
> Replace the initialization with ZeroMem().
> 
> Do the same to "FilePath" in UdfOpen().
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c 
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 8b9339567f8e..e7159ff861f7 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -174,15 +174,16 @@ UdfOpen (
>  {
>EFI_TPL OldTpl;
>EFI_STATUS  Status;
>PRIVATE_UDF_FILE_DATA   *PrivFileData;
>PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
> -  CHAR16  FilePath[UDF_PATH_LENGTH] = { 0 };
> +  CHAR16  FilePath[UDF_PATH_LENGTH];
>UDF_FILE_INFO   File;
>PRIVATE_UDF_FILE_DATA   *NewPrivFileData;
>CHAR16  *TempFileName;
>  
> +  ZeroMem (FilePath, sizeof FilePath);
>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>  
>if (This == NULL || NewHandle == NULL || FileName == NULL) {
>  Status = EFI_INVALID_PARAMETER;
>  goto Error_Invalid_Params;
> @@ -322,14 +323,15 @@ UdfRead (
>EFI_BLOCK_IO_PROTOCOL   *BlockIo;
>EFI_DISK_IO_PROTOCOL*DiskIo;
>UDF_FILE_INFO   FoundFile;
>UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>VOID*NewFileEntryData;
> -  CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
> +  CHAR16  FileName[UDF_FILENAME_LENGTH];
>UINT64  FileSize;
>UINT64  BufferSizeUint64;
>  
> +  ZeroMem (FileName, sizeof FileName);
>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
>  
>if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>   Buffer == NULL)) {
>  Status = EFI_INVALID_PARAMETER;
> --
> 2.14.1.3.gb7cf6e02401b
> 
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org

Re: [edk2] [PATCH v3 1/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: Bi, Dandan
> Sent: Tuesday, September 12, 2017 4:56 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric ; Paulo Alcantara ;
> Ni, Ruiyu ; Zeng, Star 
> Subject: [PATCH v3 1/3] MdeModulePkg/UdfDxe: Add type cast to fix build
> failure in VS tools
> 
> V3: Remove one unnecessay type cast in patch 1.
> Codes:
> if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
>   Offset = ReadFileInfo->FilePosition - FilePosition;
>   if (Offset < 0) {
> Offset = -(Offset)
>   }
> ...
> }
> offset is UINT64 can not < 0, so the code logic may have some issue.
> and Offset = -(Offset) may build failure in some circumstance.
> previously type case Offset to INT64 to fix build block. Now remove the type
> cast. Then can to check the code logic later.
> 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c | 16 
> 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 7d7f722..5c5b5e3 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -470,11 +470,11 @@ DuplicateFid (
>OUT  UDF_FILE_IDENTIFIER_DESCRIPTOR  **NewFileIdentifierDesc
>)
>  {
>*NewFileIdentifierDesc =
>  (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
> -  GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
> +  (UINTN) GetFidDescriptorLength (FileIdentifierDesc),
> + FileIdentifierDesc);
>  }
> 
>  //
>  // Duplicate either a given File Entry or a given Extended File Entry.
>  //
> @@ -807,20 +807,20 @@ GetAedAdsData (
>}
> 
>//
>// Allocate buffer to read in AED's data.
>//
> -  *Data = AllocatePool (*Length);
> +  *Data = AllocatePool ((UINTN) (*Length));
>if (*Data == NULL) {
>  return EFI_OUT_OF_RESOURCES;
>}
> 
>return DiskIo->ReadDisk (
>  DiskIo,
>  BlockIo->Media->MediaId,
>  Offset,
> -*Length,
> +(UINTN) (*Length),
>  *Data
>  );
>  }
> 
>  //
> @@ -842,11 +842,11 @@ GrowUpBufferToNextAd (
>  *Buffer = AllocatePool (ExtentLength);
>  if (*Buffer == NULL) {
>return EFI_OUT_OF_RESOURCES;
>  }
>} else {
> -*Buffer = ReallocatePool (Length, Length + ExtentLength, *Buffer);
> +*Buffer = ReallocatePool ((UINTN) Length, (UINTN) (Length +
> + ExtentLength), *Buffer);
>  if (*Buffer == NULL) {
>return EFI_OUT_OF_RESOURCES;
>  }
>}
> 
> @@ -931,29 +931,29 @@ ReadFile (
>ReadFileInfo->ReadLength = Length;
>  } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
>//
>// Allocate buffer for starting read data.
>//
> -  ReadFileInfo->FileData = AllocatePool (Length);
> +  ReadFileInfo->FileData = AllocatePool ((UINTN) Length);
>if (ReadFileInfo->FileData == NULL) {
>  return EFI_OUT_OF_RESOURCES;
>}
> 
>//
>// Read all inline data into ReadFileInfo->FileData
>//
> -  CopyMem (ReadFileInfo->FileData, Data, Length);
> +  CopyMem (ReadFileInfo->FileData, Data, (UINTN) Length);
>ReadFileInfo->ReadLength = Length;
>  } else if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ) {
>//
>// If FilePosition is non-zero, seek file to FilePosition, read
>// FileDataSize bytes and then updates FilePosition.
>//
>CopyMem (
>  ReadFileInfo->FileData,
>  (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
> -ReadFileInfo->FileDataSize
> +(UINTN) ReadFileInfo->FileDataSize
>  );
> 
>ReadFileInfo->FilePosition += ReadFileInfo->FileDataSize;
>  }
> 
> @@ -1097,11 +1097,11 @@ ReadFile (
>  //
>  Status = DiskIo->ReadDisk (
>DiskIo,
>BlockIo->Media->MediaId,
>Offset + MultU64x32 (Lsn, LogicalBlockSize),
> -  DataLength,
> +  (UINTN) DataLength,
>(VOID *)((UINT8 *)ReadFileInfo->FileData +
> DataOffset)
>);
>  if (EFI_ERROR (Status)) {
>goto Error_Read_Disk_Blk;
> --
> 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 v3 2/3] MdeModulePkg/UdfDxe: Initialize the array after declaration

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: Bi, Dandan
> Sent: Tuesday, September 12, 2017 4:56 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric ; Paulo Alcantara ;
> Ni, Ruiyu ; Zeng, Star 
> Subject: [PATCH v3 2/3] MdeModulePkg/UdfDxe: Initialize the array after
> declaration
> 
> Initialize the array DescriptorLBAs[] after declaration to fix non-constant
> aggregate initializer warning in VS tool chains.
> 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13
> ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 5c5b5e3..904262a 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -20,15 +20,22 @@ FindAnchorVolumeDescriptorPointer (
>IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
>OUT  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint
>)
>  {
>EFI_STATUS  Status;
> -  UINT32  BlockSize = BlockIo->Media->BlockSize;
> -  EFI_LBA EndLBA = BlockIo->Media->LastBlock;
> -  EFI_LBA DescriptorLBAs[] = { 256, EndLBA - 256, EndLBA, 512 };
> +  UINT32  BlockSize;
> +  EFI_LBA EndLBA;
> +  EFI_LBA DescriptorLBAs[4];
>UINTN   Index;
> 
> +  BlockSize = BlockIo->Media->BlockSize;  EndLBA =
> + BlockIo->Media->LastBlock;  DescriptorLBAs[0] = 256;
> + DescriptorLBAs[1] = EndLBA - 256;  DescriptorLBAs[2] = EndLBA;
> + DescriptorLBAs[3] = 512;
> +
>for (Index = 0; Index < ARRAY_SIZE (DescriptorLBAs); Index++) {
>  Status = DiskIo->ReadDisk (
>DiskIo,
>BlockIo->Media->MediaId,
>MultU64x32 (DescriptorLBAs[Index], BlockSize),
> --
> 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 v3 3/3] MdeModulePkg/PartitionDxe: Initialize the array after declaration

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Dandan Bi
> Sent: Tuesday, September 12, 2017 4:56 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Dong, Eric ; Zeng,
> Star 
> Subject: [edk2] [PATCH v3 3/3] MdeModulePkg/PartitionDxe: Initialize the
> array after declaration
> 
> Initialize the array DescriptorLBAs[] after declaration to fix non-constant
> aggregate initializer warning in VS tool chains.
> 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi 
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index c1d4480..3174ab2 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -46,15 +46,22 @@ FindAnchorVolumeDescriptorPointer (
>IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
>OUT  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint
>)
>  {
>EFI_STATUS  Status;
> -  UINT32  BlockSize = BlockIo->Media->BlockSize;
> -  EFI_LBA EndLBA = BlockIo->Media->LastBlock;
> -  EFI_LBA DescriptorLBAs[] = { 256, EndLBA - 256, EndLBA, 512 };
> +  UINT32  BlockSize;
> +  EFI_LBA EndLBA;
> +  EFI_LBA DescriptorLBAs[4];
>UINTN   Index;
> 
> +  BlockSize = BlockIo->Media->BlockSize;  EndLBA =
> + BlockIo->Media->LastBlock;  DescriptorLBAs[0] = 256;
> + DescriptorLBAs[1] = EndLBA - 256;  DescriptorLBAs[2] = EndLBA;
> + DescriptorLBAs[3] = 512;
> +
>for (Index = 0; Index < ARRAY_SIZE (DescriptorLBAs); Index++) {
>  Status = DiskIo->ReadDisk (
>DiskIo,
>BlockIo->Media->MediaId,
>MultU64x32 (DescriptorLBAs[Index], BlockSize),
> --
> 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] [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Ni, Ruiyu]

Star,
Sizeof is an operator, not a function, like + or -. Not having () is ok.

Thanks/Ray

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, September 12, 2017 4:55 PM
> To: Laszlo Ersek ; edk2-devel-01  de...@lists.01.org>
> Cc: Ard Biesheuvel ; Dong, Eric
> ; Paulo Alcantara ; Ni, Ruiyu
> ; Zeng, Star 
> Subject: RE: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
> variables with ZeroMem()
> 
> Reviewed-by: Star Zeng 
> 
> BTW: How about to use "sizeof ()" instead of "sizeof"?
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 
> Cc: Ard Biesheuvel ; Dong, Eric
> ; Paulo Alcantara ; Ni, Ruiyu
> ; Zeng, Star 
> Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local
> variables with ZeroMem()
> 
> In edk2, initialization of local variables is forbidden, both for stylistic 
> reasons
> and because such initialization may generate calls to compiler intrinsics.
> 
> For the following initialization in UdfRead():
> 
>   CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
> 
> clang-3.8 generates a memset() call, when building UdfDxe for IA32, which
> then fails to link.
> 
> Replace the initialization with ZeroMem().
> 
> Do the same to "FilePath" in UdfOpen().
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 8b9339567f8e..e7159ff861f7 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -174,15 +174,16 @@ UdfOpen (
>  {
>EFI_TPL OldTpl;
>EFI_STATUS  Status;
>PRIVATE_UDF_FILE_DATA   *PrivFileData;
>PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
> -  CHAR16  FilePath[UDF_PATH_LENGTH] = { 0 };
> +  CHAR16  FilePath[UDF_PATH_LENGTH];
>UDF_FILE_INFO   File;
>PRIVATE_UDF_FILE_DATA   *NewPrivFileData;
>CHAR16  *TempFileName;
> 
> +  ZeroMem (FilePath, sizeof FilePath);
>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> 
>if (This == NULL || NewHandle == NULL || FileName == NULL) {
>  Status = EFI_INVALID_PARAMETER;
>  goto Error_Invalid_Params;
> @@ -322,14 +323,15 @@ UdfRead (
>EFI_BLOCK_IO_PROTOCOL   *BlockIo;
>EFI_DISK_IO_PROTOCOL*DiskIo;
>UDF_FILE_INFO   FoundFile;
>UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
>VOID*NewFileEntryData;
> -  CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
> +  CHAR16  FileName[UDF_FILENAME_LENGTH];
>UINT64  FileSize;
>UINT64  BufferSizeUint64;
> 
> +  ZeroMem (FileName, sizeof FileName);
>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> 
>if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
>   Buffer == NULL)) {
>  Status = EFI_INVALID_PARAMETER;
> --
> 2.14.1.3.gb7cf6e02401b
> 

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

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools

[Zeng, Star]

Hi Paulo,

There is change(type cast to INT64) below in this patch. After check, we found 
the " if (Offset < 0) " should be always false comparison as Offset is UINT64 
type.
I have suggested Dandan to remove this change(type case to INT64) at v3 patch 
series https://lists.01.org/pipermail/edk2-devel/2017-September/014523.html.
Could you help check and fix the code appropriately?
   if (Offset < 0) {
-Offset = -(Offset);
+Offset = - (INT64) (Offset);
   }


Thanks,
Star
-Original Message-
From: Bi, Dandan 
Sent: Monday, September 11, 2017 2:17 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric ; Paulo Alcantara ; Ni, 
Ruiyu ; Zeng, Star 
Subject: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure 
in VS tools

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index ea3f5fb..bf33ae4 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -477,11 +477,11 @@ DuplicateFid (
   OUT  UDF_FILE_IDENTIFIER_DESCRIPTOR  **NewFileIdentifierDesc
   )
 {
   *NewFileIdentifierDesc =
 (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
-  GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
+  (UINTN) GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
 }
 
 //
 // Duplicate either a given File Entry or a given Extended File Entry.
 //
@@ -814,20 +814,20 @@ GetAedAdsData (
   }
 
   //
   // Allocate buffer to read in AED's data.
   //
-  *Data = AllocatePool (*Length);
+  *Data = AllocatePool ((UINTN) (*Length));
   if (*Data == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
   return DiskIo->ReadDisk (
 DiskIo,
 BlockIo->Media->MediaId,
 Offset,
-*Length,
+(UINTN) (*Length),
 *Data
 );
 }
 
 //
@@ -849,11 +849,11 @@ GrowUpBufferToNextAd (
 *Buffer = AllocatePool (ExtentLength);
 if (*Buffer == NULL) {
   return EFI_OUT_OF_RESOURCES;
 }
   } else {
-*Buffer = ReallocatePool (Length, Length + ExtentLength, *Buffer);
+*Buffer = ReallocatePool ((UINTN) Length, (UINTN) (Length + ExtentLength), 
*Buffer);
 if (*Buffer == NULL) {
   return EFI_OUT_OF_RESOURCES;
 }
   }
 
@@ -938,29 +938,29 @@ ReadFile (
   ReadFileInfo->ReadLength = Length;
 } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
   //
   // Allocate buffer for starting read data.
   //
-  ReadFileInfo->FileData = AllocatePool (Length);
+  ReadFileInfo->FileData = AllocatePool ((UINTN) Length);
   if (ReadFileInfo->FileData == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
   //
   // Read all inline data into ReadFileInfo->FileData
   //
-  CopyMem (ReadFileInfo->FileData, Data, Length);
+  CopyMem (ReadFileInfo->FileData, Data, (UINTN) Length);
   ReadFileInfo->ReadLength = Length;
 } else if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ) {
   //
   // If FilePosition is non-zero, seek file to FilePosition, read
   // FileDataSize bytes and then updates FilePosition.
   //
   CopyMem (
 ReadFileInfo->FileData,
 (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
-ReadFileInfo->FileDataSize
+(UINTN) ReadFileInfo->FileDataSize
 );
 
   ReadFileInfo->FilePosition += ReadFileInfo->FileDataSize;
 } else {
   ASSERT (FALSE);
@@ -1081,11 +1081,11 @@ ReadFile (
 }
 
 if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
   Offset = ReadFileInfo->FilePosition - FilePosition;
   if (Offset < 0) {
-Offset = -(Offset);
+Offset = - (INT64) (Offset);
   }
 } else {
   Offset = 0;
 }
 
@@ -1109,11 +1109,11 @@ ReadFile (
 //
 Status = DiskIo->ReadDisk (
   DiskIo,
   BlockIo->Media->MediaId,
   Offset + MultU64x32 (Lsn, LogicalBlockSize),
-  DataLength,
+  (UINTN) DataLength,
   (VOID *)((UINT8 *)ReadFileInfo->FileData +
DataOffset)
   );
 if (EFI_ERROR (Status)) {
   goto Error_Read_Disk_Blk;
-- 
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 v3 0/3] Fix VS tool chain build failure

[Zeng, Star]

Reviewed-by: Star Zeng  and pushed the patches at 
https://github.com/tianocore/edk2/compare/1f4807074005...c05cae55ebd8.
I also picked up the Reviewed-by of Paulo at 
https://lists.01.org/pipermail/edk2-devel/2017-September/014490.html when 
pushing the patches.
I also fixed some typos in [PATCH v3 3/3], for example, "offset" -> "Offset", 
"block" -> "break" and "case" -> "cast".


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dandan Bi
Sent: Tuesday, September 12, 2017 4:56 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH v3 0/3] Fix VS tool chain build failure

V3: Remove one unnecessay type cast in patch 1.

Dandan Bi (3):
  MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools
  MdeModulePkg/UdfDxe: Initialize the array after declaration
  MdeModulePkg/PartitionDxe: Initialize the array after declaration

 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 13 +++---
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 29 ++
 2 files changed, 28 insertions(+), 14 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] [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if INLINE_DATA req succeeds

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH 2/5] MdeModulePkg/UdfDxe: don't return unset Status if 
INLINE_DATA req succeeds

Ard reports that clang-3.8 correctly flags the following issue in the
ReadFile() function:

If "RecordingFlags" is INLINE_DATA, then there are three paths through the code 
where we mean to return success, but forget to set Status
accordingly:

(1) when "ReadFileInfo->Flags" is READ_FILE_GET_FILESIZE, or

(2) when "ReadFileInfo->Flags" is READ_FILE_ALLOCATE_AND_READ and
AllocatePool() succeeds, or

(3) when "ReadFileInfo->Flags" is READ_FILE_SEEK_AND_READ.

Set "Status" to EFI_SUCCESS when we are done processing the INLINE_DATA 
request, i.e., when we reach the corresponding "break" statament under the 
INLINE_DATA case label.

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Reported-by: Ard Biesheuvel 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index a2ca65e5dfe8..0de9c71c250f 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -958,11 +958,13 @@ ReadFile (
 } else {
   ASSERT (FALSE);
   return EFI_INVALID_PARAMETER;
 }
 
+Status = EFI_SUCCESS;
 break;
+
   case LONG_ADS_SEQUENCE:
   case SHORT_ADS_SEQUENCE:
 //
 // This FE/EFE contains a run of Allocation Descriptors. Get data + size
 // for start reading them out.
--
2.14.1.3.gb7cf6e02401b


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

[edk2] [PATCH v3 3/3] MdeModulePkg/PartitionDxe: Initialize the array after declaration

[Dandan Bi]

Initialize the array DescriptorLBAs[] after declaration to fix
non-constant aggregate initializer warning in VS tool chains.

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c 
b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index c1d4480..3174ab2 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -46,15 +46,22 @@ FindAnchorVolumeDescriptorPointer (
   IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
   OUT  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint
   )
 {
   EFI_STATUS  Status;
-  UINT32  BlockSize = BlockIo->Media->BlockSize;
-  EFI_LBA EndLBA = BlockIo->Media->LastBlock;
-  EFI_LBA DescriptorLBAs[] = { 256, EndLBA - 256, EndLBA, 512 };
+  UINT32  BlockSize;
+  EFI_LBA EndLBA;
+  EFI_LBA DescriptorLBAs[4];
   UINTN   Index;
 
+  BlockSize = BlockIo->Media->BlockSize;
+  EndLBA = BlockIo->Media->LastBlock;
+  DescriptorLBAs[0] = 256;
+  DescriptorLBAs[1] = EndLBA - 256;
+  DescriptorLBAs[2] = EndLBA;
+  DescriptorLBAs[3] = 512;
+
   for (Index = 0; Index < ARRAY_SIZE (DescriptorLBAs); Index++) {
 Status = DiskIo->ReadDisk (
   DiskIo,
   BlockIo->Media->MediaId,
   MultU64x32 (DescriptorLBAs[Index], BlockSize),
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH v3 2/3] MdeModulePkg/UdfDxe: Initialize the array after declaration

[Dandan Bi]

Initialize the array DescriptorLBAs[] after declaration to fix
non-constant aggregate initializer warning in VS tool chains.

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 5c5b5e3..904262a 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -20,15 +20,22 @@ FindAnchorVolumeDescriptorPointer (
   IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
   OUT  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint
   )
 {
   EFI_STATUS  Status;
-  UINT32  BlockSize = BlockIo->Media->BlockSize;
-  EFI_LBA EndLBA = BlockIo->Media->LastBlock;
-  EFI_LBA DescriptorLBAs[] = { 256, EndLBA - 256, EndLBA, 512 };
+  UINT32  BlockSize;
+  EFI_LBA EndLBA;
+  EFI_LBA DescriptorLBAs[4];
   UINTN   Index;
 
+  BlockSize = BlockIo->Media->BlockSize;
+  EndLBA = BlockIo->Media->LastBlock;
+  DescriptorLBAs[0] = 256;
+  DescriptorLBAs[1] = EndLBA - 256;
+  DescriptorLBAs[2] = EndLBA;
+  DescriptorLBAs[3] = 512;
+
   for (Index = 0; Index < ARRAY_SIZE (DescriptorLBAs); Index++) {
 Status = DiskIo->ReadDisk (
   DiskIo,
   BlockIo->Media->MediaId,
   MultU64x32 (DescriptorLBAs[Index], BlockSize),
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH v3 1/3] MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools

[Dandan Bi]

V3: Remove one unnecessay type cast in patch 1.
Codes:
if (FilePosition + ExtentLength > ReadFileInfo->FilePosition) {
  Offset = ReadFileInfo->FilePosition - FilePosition;
  if (Offset < 0) {
Offset = -(Offset)
  }
...
}
offset is UINT64 can not < 0, so the code logic may have some issue.
and Offset = -(Offset) may build failure in some circumstance.
previously type case Offset to INT64 to fix build block. Now remove
the type cast. Then can to check the code logic later.

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Dandan Bi 
---
 .../Universal/Disk/UdfDxe/FileSystemOperations.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 7d7f722..5c5b5e3 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -470,11 +470,11 @@ DuplicateFid (
   OUT  UDF_FILE_IDENTIFIER_DESCRIPTOR  **NewFileIdentifierDesc
   )
 {
   *NewFileIdentifierDesc =
 (UDF_FILE_IDENTIFIER_DESCRIPTOR *)AllocateCopyPool (
-  GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
+  (UINTN) GetFidDescriptorLength (FileIdentifierDesc), FileIdentifierDesc);
 }
 
 //
 // Duplicate either a given File Entry or a given Extended File Entry.
 //
@@ -807,20 +807,20 @@ GetAedAdsData (
   }
 
   //
   // Allocate buffer to read in AED's data.
   //
-  *Data = AllocatePool (*Length);
+  *Data = AllocatePool ((UINTN) (*Length));
   if (*Data == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
   return DiskIo->ReadDisk (
 DiskIo,
 BlockIo->Media->MediaId,
 Offset,
-*Length,
+(UINTN) (*Length),
 *Data
 );
 }
 
 //
@@ -842,11 +842,11 @@ GrowUpBufferToNextAd (
 *Buffer = AllocatePool (ExtentLength);
 if (*Buffer == NULL) {
   return EFI_OUT_OF_RESOURCES;
 }
   } else {
-*Buffer = ReallocatePool (Length, Length + ExtentLength, *Buffer);
+*Buffer = ReallocatePool ((UINTN) Length, (UINTN) (Length + ExtentLength), 
*Buffer);
 if (*Buffer == NULL) {
   return EFI_OUT_OF_RESOURCES;
 }
   }
 
@@ -931,29 +931,29 @@ ReadFile (
   ReadFileInfo->ReadLength = Length;
 } else if (ReadFileInfo->Flags == READ_FILE_ALLOCATE_AND_READ) {
   //
   // Allocate buffer for starting read data.
   //
-  ReadFileInfo->FileData = AllocatePool (Length);
+  ReadFileInfo->FileData = AllocatePool ((UINTN) Length);
   if (ReadFileInfo->FileData == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
 
   //
   // Read all inline data into ReadFileInfo->FileData
   //
-  CopyMem (ReadFileInfo->FileData, Data, Length);
+  CopyMem (ReadFileInfo->FileData, Data, (UINTN) Length);
   ReadFileInfo->ReadLength = Length;
 } else if (ReadFileInfo->Flags == READ_FILE_SEEK_AND_READ) {
   //
   // If FilePosition is non-zero, seek file to FilePosition, read
   // FileDataSize bytes and then updates FilePosition.
   //
   CopyMem (
 ReadFileInfo->FileData,
 (VOID *)((UINT8 *)Data + ReadFileInfo->FilePosition),
-ReadFileInfo->FileDataSize
+(UINTN) ReadFileInfo->FileDataSize
 );
 
   ReadFileInfo->FilePosition += ReadFileInfo->FileDataSize;
 }
 
@@ -1097,11 +1097,11 @@ ReadFile (
 //
 Status = DiskIo->ReadDisk (
   DiskIo,
   BlockIo->Media->MediaId,
   Offset + MultU64x32 (Lsn, LogicalBlockSize),
-  DataLength,
+  (UINTN) DataLength,
   (VOID *)((UINT8 *)ReadFileInfo->FileData +
DataOffset)
   );
 if (EFI_ERROR (Status)) {
   goto Error_Read_Disk_Blk;
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH v3 0/3] Fix VS tool chain build failure

[Dandan Bi]

V3: Remove one unnecessay type cast in patch 1.

Dandan Bi (3):
  MdeModulePkg/UdfDxe: Add type cast to fix build failure in VS tools
  MdeModulePkg/UdfDxe: Initialize the array after declaration
  MdeModulePkg/PartitionDxe: Initialize the array after declaration

 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 13 +++---
 .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 29 ++
 2 files changed, 28 insertions(+), 14 deletions(-)

-- 
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 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables with ZeroMem()

[Zeng, Star]

Reviewed-by: Star Zeng 

BTW: How about to use "sizeof ()" instead of "sizeof"?


Thanks,
Star
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH 3/5] MdeModulePkg/UdfDxe: replace zero-init of local variables 
with ZeroMem()

In edk2, initialization of local variables is forbidden, both for stylistic 
reasons and because such initialization may generate calls to compiler 
intrinsics.

For the following initialization in UdfRead():

  CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };

clang-3.8 generates a memset() call, when building UdfDxe for IA32, which then 
fails to link.

Replace the initialization with ZeroMem().

Do the same to "FilePath" in UdfOpen().

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 8b9339567f8e..e7159ff861f7 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -174,15 +174,16 @@ UdfOpen (
 {
   EFI_TPL OldTpl;
   EFI_STATUS  Status;
   PRIVATE_UDF_FILE_DATA   *PrivFileData;
   PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
-  CHAR16  FilePath[UDF_PATH_LENGTH] = { 0 };
+  CHAR16  FilePath[UDF_PATH_LENGTH];
   UDF_FILE_INFO   File;
   PRIVATE_UDF_FILE_DATA   *NewPrivFileData;
   CHAR16  *TempFileName;
 
+  ZeroMem (FilePath, sizeof FilePath);
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (This == NULL || NewHandle == NULL || FileName == NULL) {
 Status = EFI_INVALID_PARAMETER;
 goto Error_Invalid_Params;
@@ -322,14 +323,15 @@ UdfRead (
   EFI_BLOCK_IO_PROTOCOL   *BlockIo;
   EFI_DISK_IO_PROTOCOL*DiskIo;
   UDF_FILE_INFO   FoundFile;
   UDF_FILE_IDENTIFIER_DESCRIPTOR  *NewFileIdentifierDesc;
   VOID*NewFileEntryData;
-  CHAR16  FileName[UDF_FILENAME_LENGTH] = { 0 };
+  CHAR16  FileName[UDF_FILENAME_LENGTH];
   UINT64  FileSize;
   UINT64  BufferSizeUint64;
 
+  ZeroMem (FileName, sizeof FileName);
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
   if (This == NULL || BufferSize == NULL || (*BufferSize != 0 &&
  Buffer == NULL)) {
 Status = EFI_INVALID_PARAMETER;
--
2.14.1.3.gb7cf6e02401b


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

Re: [edk2] [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison

[Zeng, Star]

Reviewed-by: Star Zeng 

-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Sunday, September 10, 2017 8:13 AM
To: edk2-devel-01 
Cc: Ard Biesheuvel ; Dong, Eric 
; Paulo Alcantara ; Ni, Ruiyu 
; Zeng, Star 
Subject: [PATCH 5/5] MdeModulePkg/PartitionDxe: remove always false comparison

In the expression

  (RemainderByMediaBlockSize != 0 ||
   Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE)

the second expression is only evaluated if the first expression is false.

If the first expression is false, i.e.,

  RemainderByMediaBlockSize == 0

then UDF_LOGICAL_SECTOR_SIZE is a whole multiple of "Media->BlockSize", which 
implies

  UDF_LOGICAL_SECTOR_SIZE >= Media->BlockSize.

Therefore whenever

  Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE

is evaluated, it is false.

The expression

  ((expression) || FALSE)

is equivalent to

  (expression).

Cc: Ard Biesheuvel 
Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c 
b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index c491ef25f47e..3347b48421a8 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -252,12 +252,11 @@ PartitionInstallUdfChildHandles (
   DivU64x32Remainder (
 UDF_LOGICAL_SECTOR_SIZE,   // Dividend
 Media->BlockSize,  // Divisor
  // Remainder
 );
-  if (RemainderByMediaBlockSize != 0 ||
-  Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
+  if (RemainderByMediaBlockSize != 0) {
 return EFI_NOT_FOUND;
   }
 
   DevicePathNode = DevicePath;
   while (!IsDevicePathEnd (DevicePathNode)) {
--
2.14.1.3.gb7cf6e02401b

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

Re: [edk2] Question on Iscsi Behavior

[Karunakar P]

Hi Ting Ye,

Thank you very much for your info.

Thanks,
karunakar

-Original Message-
From: Ye, Ting [mailto:ting...@intel.com] 
Sent: Tuesday, September 12, 2017 1:49 PM
To: Karunakar P; edk2-devel@lists.01.org
Subject: RE: [edk2] Question on Iscsi Behavior

Hi Karunakar,

In "Enabled" mode, only connected attempts will be recorded in iBFT while in 
"Enabled for MPIO", all configured attempts will be recorded in iBFT.
In "Enabled for MPIO" mode, the first configured attempt (according to attempt 
order displayed) is preferred. If the first attempt is valid it will not 
connect other attempts. While in "Enabled" mode, all configured attempts will 
be tried.

Thanks,
Ting Ye 

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
Karunakar P
Sent: Monday, September 11, 2017 6:38 PM
To: edk2-devel@lists.01.org
Subject: Re: [edk2] Question on Iscsi Behavior

Hello All,

Could you please help in clarifying below question on ISCSI behavior.

2 ISCSI Attempts are added with different configuration

Case 1:
Attempt 1--Enabled(ISCSI Mode)--IP4-DHCP ---> In Valid Attempt (In 
Valid Target Details) Attempt 2--Enabled(ISCSI Mode)--IP4-DHCP ---> 
Valid Attempt
Result:-
-> Initiator IP Assigned for Attempt1 and Attempt2(2 times DHCP process
->done) ISCSI connection success with Attempt2 and same Details was 
->found in Shell

Case 2:
Attempt 1--Enabled for MPIO(ISCSI Mode)--IP4-DHCP ---> In Valid 
Attempt (In Valid Target Details) Attempt 2--Enabled for MPIO(ISCSI 
Mode)--IP4-DHCP ---> Valid Attempt
Result:-
-> Initiator IP Assigned for Attempt1 and Attempt2(2 times DHCP process
->done) ISCSI connection success with Attempt2 and same Details was 
->found in Shell

Note: Same behavior seen with adding attempts for 2 different MAC addresses


1.What is the difference between ISCSI Mode Enable & Enabled for MPIO ?
Because irrespective of ISCSI Mode, Attempt2 will be processed if Attempt1 is 
In Valid


Thanks,
karunakar
___
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 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Zeng, Star]

Reviewed-by: Star Zeng 

I am not sure whether the other patches in this series has been reviewed or not.
Since this patch is fixing build break, I think we can have this patch pushed 
first after reviewed. And really appreciate that. :)

Thanks,
Star
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com] 
Sent: Tuesday, September 12, 2017 3:59 PM
To: Bi, Dandan ; edk2-devel-01 
Cc: Ni, Ruiyu ; Dong, Eric ; Zeng, 
Star ; Ard Biesheuvel 
Subject: Re: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit 
values with C operators

On 09/12/17 07:41, Bi, Dandan wrote:
> Hi Laszlo,
> 
> When do you plan to push this patch? IA32 build is blocked for this issue now.

I was ready to push the series yesterday; I just hoped I'd get review feedback 
from MdeModulePkg maintainers as well, and/or from Ray, in one or two days.

These are strongly localized changes that require no knowledge of the UDF 
driver. (I don't have that knowledge myself, to begin with.) At least an 
Acked-by would be nice.

If someone from Intel tells me I can push this with the R-b's that are 
currently on the list, I'm totally game.

Thanks!
Laszlo

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Laszlo Ersek
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 
> Cc: Ni, Ruiyu ; Dong, Eric ; 
> Zeng, Star ; Ard Biesheuvel 
> 
> Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 
> 64-bit values with C operators
> 
> In edk2, the division and shifting of 64-bit values are forbidden with 
> C-language operators, because the compiler may generate intrinsic calls for 
> them.
> 
> For example, clang-3.8 emits a call to "__umoddi3" for
> 
>   UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize
> 
> in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, 
> which then fails to link.
> 
> UDF_LOGICAL_SECTOR_SIZE has type UINT64, while EFI_BLOCK_IO_MEDIA.BlockSize 
> has type UINT32(). Replace the % operator with a DivU64x32Remainder() call.
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c 
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index c1d44809bfd2..c491ef25f47e 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
>IN  EFI_BLOCK_IO_PROTOCOL*BlockIo,
>IN  EFI_BLOCK_IO2_PROTOCOL   *BlockIo2,
>IN  EFI_DEVICE_PATH_PROTOCOL *DevicePath
>)
>  {
> +  UINT32   RemainderByMediaBlockSize;
>EFI_STATUS   Status;
>EFI_BLOCK_IO_MEDIA   *Media;
>EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
>EFI_GUID *VendorDefinedGuid;
>EFI_GUID UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
> @@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
>Media = BlockIo->Media;
>  
>//
>// Check if UDF logical block size is multiple of underlying device block 
> size
>//
> -  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
> +  DivU64x32Remainder (
> +UDF_LOGICAL_SECTOR_SIZE,   // Dividend
> +Media->BlockSize,  // Divisor
> + // Remainder
> +);
> +  if (RemainderByMediaBlockSize != 0 ||
>Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
>  return EFI_NOT_FOUND;
>}
>  
>DevicePathNode = DevicePath;
> --
> 2.14.1.3.gb7cf6e02401b
> 
> 
> ___
> 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/PciBusDxe: GetBarAttributes() returns Host address

[Laszlo Ersek]

On 09/12/17 08:44, Ard Biesheuvel wrote:
> On 12 September 2017 at 06:01, Ni, Ruiyu  wrote:
>> Laszlo,
>> Your understanding is: DeviceAddress = HostAddress + AddressTranslationOffset
>> But my patch assumes: HostAddress = DeviceAddress + AddressTranslationOffset
>>
>> They are totally different. If I follow your understanding, the patch is 
>> wrong!
>> Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply to 
>> the
>> Starting address of a BAR to convert it to a PCI address" very clearly, I 
>> quoted
>> the statement from ACPI spec.
>> Your understanding to "apply to" is "add", my understanding is "minus".
>>
>
> Even though we are stretching the ACPI definition of a QWord
> descriptor beyond its original meaning, I don't think there is a lot
> of ambiguity here, to be honest. The AddrRangeMin field contains the
> address on the secondary side of a bridge, and the primary value can
> be obtained by 'applying' the ATO. In my opinion, applying a (positive
> or negative) offset implies addition, not subtraction, as subtraction
> involves negating the second addend before applying it. And the
> secondary side of the host bridge is clearly the PCI side.

Wait, now I'm even more confused.

(1) Up-thread you wrote, "AddrRangeMin is indeed already defined to be a
host address [...]".

(2) Here you write, "the secondary side of the host bridge is clearly
the PCI side [...] The AddrRangeMin field contains the address on the
secondary side of a bridge". --> This means that AddrRangeMin is a PCI
address.

Thus, to me these statements appear to conflict.

> It does mean the offset field is signed, though.
>
> So I don't agree with the conclusion that no clarification is
> required. We need to make sure the spec is crystal clear in this
> regard. But I do agree with the change, I think it is the only
> solution that makes sense.

My understanding of "Table 121. QWORD Address Space Descriptor" is:

- AddrRangeMin --> host address.

- ATO --> the UINT64 value that the *caller* of GetBarAttributes() has
  to add, in UINT64 modular arithmetic, to AddrRangeMin, to calculate
  the PCI address, after GetBarAttributes() returns.

Now, if I understand the *patch* correctly,

- the current (pre-patch) code returns a PCI address in
  "Descriptor->AddrRangeMin", which is wrong,

- in addition, we already have the ATO, in
  "Descriptor->AddrTranslationOffset", that we have to add to the PCI
  address, to end up with a host address.

If that's the case, then I think the patch is good, but it is
incomplete. Namely,

- To return a host address to the caller in "Descriptor->AddrRangeMin",
  we add the ATO to it, fetched from the Root Bridge IO protocol. Great.

- However, think of what happens when the caller wants to recompute the
  PCI address! According to the UEFI spec, the ATO that the caller gets
  in the QWORD descriptor has to be *added* to AddrRangeMin. This means
  that, the client code would ultimately result in:

  ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + OriginalATO

This makes no sense. In order to end up with the original PCI address,
the client side ATO must be the modular UINT64 *negative* of the
original ATO, so that they ultimately cancel out on the client side,
like this:

  ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + ClientSideATO
   == (OriginalPciAddress + OriginalATO) + (-OriginalATO)
   == OriginalPciAddress

Therefore, I think that the patch must, *in addition*, negate the ATO
before returning, like this:

+  Descriptor->AddrRangeMin += Descriptor->AddrTranslationOffset;
+  Descriptor->AddrTranslationOffset = (-Descriptor->AddrTranslationOffset);

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

Re: [edk2] [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver for SPI NOR flash

[Leif Lindholm]

On Tue, Sep 12, 2017 at 01:41:34AM +, methavanitpong.pi...@socionext.com 
wrote:
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: Tuesday, September 12, 2017 4:13 AM
> > To: Ard Biesheuvel 
> > Cc: edk2-devel@lists.01.org; Methavanitpong, Pipat/メタワニットポン ピパット
> > ; masahisa.koj...@linaro.org;
> > masami.hirama...@linaro.org
> > Subject: Re: [PATCH edk2-platforms 13/14] Silicon/Socionext: add driver
> > for SPI NOR flash
> > 
> > On Fri, Sep 08, 2017 at 07:23:14PM +0100, Ard Biesheuvel wrote:
> > > This imports the driver sources provided by Socionext for the FIP006
> > > SPI NOR flash device found on Synquacer SoCs. It has been slightly
> > > tweaked to bring it up to date with the changes made on the EDK2 side
> > > since it was forked.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel 

> [Methavanitpong, Pipat/メタワニットポン ピパット] 
> Hi Leif, 
> 
> 1. Hex magic
> 
> There are 2 hex magic in this driver
> 
>   1. FIP006 command sequencer decode command
> 
>  FIP006 in command sequencer mode detects accesses to its memory region. 
>  An access to this region is translated into commands according to 
>  FIP006_REG_CS_RD and FIP006_REG_CS_WR for read and write accordingly. 
> 
>  A single access can be translated up to 8 1-byte commands, as the 
>  registers have 8 slots each. Each command is transmitted consecutively
>  starting from their base addresses. 
> 
>  Each slot in the registers are decoded as
> 
>  1. Immediate value - CSDC(imm, ..., ..., DEC=0)
>  2. Addr[7:0] - CSDC(0x00, ..., ..., DEC=1)
>  3. Addr[15:8] - CSDC(0x01, ..., ..., DEC=1)
>  4. Addr[23:16] - CSDC(0x02, ..., ..., DEC=1)
>  5. Addr[31:24] - CSDC(0x03, ..., ..., DEC=1)
>  6. HiZ for 1 byte - CSDC(0x04, ..., ..., DEC=1)
>  7. Upper 4-bit immediate value + 4-bit HiZ - CSDC((imm << 4) | 0x05, 
> ..., ..., DEC=1)
>  8. Command termination - CSDC(0x07, ..., ..., DEC=1)
> 
>   2. Micron N25Q flash command
> 
>  In order to send a command to a flash device, FIP006's FIP006_REG_CS_RD 
>  and FIP006_REG_CS_WR must be set properly. NorFlashSetHostCommand() 
>  sets these registers according to an instance's command definition 
> table. 
> 
>  For example; NorFlashSetHostCommand (Instance, 0x13) corresponds to a 
>  read access with 4-byte addressing read serial nor flash command (0x13). 
>  The result FIP006_REG_CS_RD is the following:
> 
>  FIP006_REG_CS_RD[0] = CSDC(0x13, 0, 1-bit lane, DEC=0)
>  FIP006_REG_CS_RD[1] = CSDC(0x03, 0, 1-bit lane, DEC=1)
>  FIP006_REG_CS_RD[2] = CSDC(0x02, 0, 1-bit lane, DEC=1)
>  FIP006_REG_CS_RD[3] = CSDC(0x01, 0, 1-bit lane, DEC=1)
>  FIP006_REG_CS_RD[4] = CSDC(0x00, 0, 1-bit lane, DEC=1)
>  FIP006_REG_CS_RD[5] = CSDC(0x07, 0, 1-bit lane, DEC=1)
>  FIP006_REG_CS_RD[6] = CSDC(0x07, 0, 1-bit lane, DEC=1)
>  FIP006_REG_CS_RD[7] = CSDC(0x07, 0, 1-bit lane, DEC=1)

Thank you for the explanation.
However, the code needs to be be readable by someone who does not have
the instruction set for each given component fresh in their mind.

That means that direct numeral expressions should be avoided wherever
they are not mathematically obvious (and where you consider them
obvious, consider whether everyone else would agree without already
knowing what the code is intended to do).

They should be replaced by #defines or enums as appropriate.

> 2. Bit field declaration
> 
> About the bitfield struct you mentioned in "Fip006Reg.h", 
> What should be a good way to declare it?

The problem with bitfields is that they are not very well defined in
the C language, and hence do not tend to be very portable.

Tedious as it may seem, #defines with shifts and masks are usually the
way to go.

Regards,

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

Re: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Ni, Ruiyu]

Reviewed-by: Ruiyu Ni 

Thanks/Ray

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, September 12, 2017 3:59 PM
> To: Bi, Dandan ; edk2-devel-01  de...@lists.01.org>
> Cc: Ni, Ruiyu ; Dong, Eric ; Zeng,
> Star ; Ard Biesheuvel 
> Subject: Re: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide
> 64-bit values with C operators
> 
> On 09/12/17 07:41, Bi, Dandan wrote:
> > Hi Laszlo,
> >
> > When do you plan to push this patch? IA32 build is blocked for this issue
> now.
> 
> I was ready to push the series yesterday; I just hoped I'd get review
> feedback from MdeModulePkg maintainers as well, and/or from Ray, in one
> or two days.
> 
> These are strongly localized changes that require no knowledge of the UDF
> driver. (I don't have that knowledge myself, to begin with.) At least an
> Acked-by would be nice.
> 
> If someone from Intel tells me I can push this with the R-b's that are 
> currently
> on the list, I'm totally game.
> 
> Thanks!
> Laszlo
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Laszlo Ersek
> > Sent: Sunday, September 10, 2017 8:13 AM
> > To: edk2-devel-01 
> > Cc: Ni, Ruiyu ; Dong, Eric ;
> > Zeng, Star ; Ard Biesheuvel
> > 
> > Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide
> > 64-bit values with C operators
> >
> > In edk2, the division and shifting of 64-bit values are forbidden with C-
> language operators, because the compiler may generate intrinsic calls for
> them.
> >
> > For example, clang-3.8 emits a call to "__umoddi3" for
> >
> >   UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize
> >
> > in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, 
> > which
> then fails to link.
> >
> > UDF_LOGICAL_SECTOR_SIZE has type UINT64, while
> EFI_BLOCK_IO_MEDIA.BlockSize has type UINT32(). Replace the % operator
> with a DivU64x32Remainder() call.
> >
> > Cc: Ard Biesheuvel 
> > Cc: Eric Dong 
> > Cc: Paulo Alcantara 
> > Cc: Ruiyu Ni 
> > Cc: Star Zeng 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Laszlo Ersek 
> > ---
> >  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > index c1d44809bfd2..c491ef25f47e 100644
> > --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> > @@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
> >IN  EFI_BLOCK_IO_PROTOCOL*BlockIo,
> >IN  EFI_BLOCK_IO2_PROTOCOL   *BlockIo2,
> >IN  EFI_DEVICE_PATH_PROTOCOL *DevicePath
> >)
> >  {
> > +  UINT32   RemainderByMediaBlockSize;
> >EFI_STATUS   Status;
> >EFI_BLOCK_IO_MEDIA   *Media;
> >EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
> >EFI_GUID *VendorDefinedGuid;
> >EFI_GUID UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
> > @@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
> >Media = BlockIo->Media;
> >
> >//
> >// Check if UDF logical block size is multiple of underlying device 
> > block size
> >//
> > -  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
> > +  DivU64x32Remainder (
> > +UDF_LOGICAL_SECTOR_SIZE,   // Dividend
> > +Media->BlockSize,  // Divisor
> > + // Remainder
> > +);
> > +  if (RemainderByMediaBlockSize != 0 ||
> >Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
> >  return EFI_NOT_FOUND;
> >}
> >
> >DevicePathNode = DevicePath;
> > --
> > 2.14.1.3.gb7cf6e02401b
> >
> >
> > ___
> > 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 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit values with C operators

[Laszlo Ersek]

On 09/12/17 07:41, Bi, Dandan wrote:
> Hi Laszlo,
> 
> When do you plan to push this patch? IA32 build is blocked for this issue now.

I was ready to push the series yesterday; I just hoped I'd get review
feedback from MdeModulePkg maintainers as well, and/or from Ray, in one
or two days.

These are strongly localized changes that require no knowledge of the
UDF driver. (I don't have that knowledge myself, to begin with.) At
least an Acked-by would be nice.

If someone from Intel tells me I can push this with the R-b's that are
currently on the list, I'm totally game.

Thanks!
Laszlo

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Sunday, September 10, 2017 8:13 AM
> To: edk2-devel-01 
> Cc: Ni, Ruiyu ; Dong, Eric ; Zeng, 
> Star ; Ard Biesheuvel 
> Subject: [edk2] [PATCH 4/5] MdeModulePkg/PartitionDxe: don't divide 64-bit 
> values with C operators
> 
> In edk2, the division and shifting of 64-bit values are forbidden with 
> C-language operators, because the compiler may generate intrinsic calls for 
> them.
> 
> For example, clang-3.8 emits a call to "__umoddi3" for
> 
>   UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize
> 
> in PartitionInstallUdfChildHandles(), if PartitionDxe is built for IA32, 
> which then fails to link.
> 
> UDF_LOGICAL_SECTOR_SIZE has type UINT64, while EFI_BLOCK_IO_MEDIA.BlockSize 
> has type UINT32(). Replace the % operator with a DivU64x32Remainder() call.
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c 
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index c1d44809bfd2..c491ef25f47e 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -234,10 +234,11 @@ PartitionInstallUdfChildHandles (
>IN  EFI_BLOCK_IO_PROTOCOL*BlockIo,
>IN  EFI_BLOCK_IO2_PROTOCOL   *BlockIo2,
>IN  EFI_DEVICE_PATH_PROTOCOL *DevicePath
>)
>  {
> +  UINT32   RemainderByMediaBlockSize;
>EFI_STATUS   Status;
>EFI_BLOCK_IO_MEDIA   *Media;
>EFI_DEVICE_PATH_PROTOCOL *DevicePathNode;
>EFI_GUID *VendorDefinedGuid;
>EFI_GUID UdfDevPathGuid = EFI_UDF_DEVICE_PATH_GUID;
> @@ -246,11 +247,16 @@ PartitionInstallUdfChildHandles (
>Media = BlockIo->Media;
>  
>//
>// Check if UDF logical block size is multiple of underlying device block 
> size
>//
> -  if ((UDF_LOGICAL_SECTOR_SIZE % Media->BlockSize) != 0 ||
> +  DivU64x32Remainder (
> +UDF_LOGICAL_SECTOR_SIZE,   // Dividend
> +Media->BlockSize,  // Divisor
> + // Remainder
> +);
> +  if (RemainderByMediaBlockSize != 0 ||
>Media->BlockSize > UDF_LOGICAL_SECTOR_SIZE) {
>  return EFI_NOT_FOUND;
>}
>  
>DevicePathNode = DevicePath;
> --
> 2.14.1.3.gb7cf6e02401b
> 
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

Re: [edk2] [Patch] BaseTools: add support for BIOS build with binary cache

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Monday, September 04, 2017 4:44 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming 
>Subject: [Patch] BaseTools: add support for BIOS build with binary cache
>
>Add three new options:
>--hash enables hash-based caching during build process. when --hash is
>enabled, build tool will base on the module hash value to do the
>incremental build, without --hash, build tool will base on the
>timestamp to do the incremental build. --hash option use md5 method to
>get every hash value, DSC/FDF, tools_def.txt, build_rule.txt and build
>command are calculated as global hash value, Package DEC and its
>include header files are calculated as package hash value, Module
>source files and its INF file are calculated as module hash value.
>Library hash value will combine the global hash value and its dependent
>package hash value. Driver hash value will combine the global hash
>value, its dependent package hash value and its linked library hash
>value.
>When --hash and --bindest are specified, build tool will copy the
>generated binary files for each module into the directory specified by
>bindest at the build phase. Bindest caches all generated binary files.
>When --hash and --binsource are specified, build tool will try to get
>the binary files from the binary source directory at the build phase.
>If the cached binary has the same hash value, it will be directly used.
>Otherwise, build tool will compile the source files and generate the
>binary files.
>
>Cc: Liming Gao 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> BaseTools/Source/Python/AutoGen/AutoGen.py   | 161
>+--
> BaseTools/Source/Python/Common/GlobalData.py |   7 ++
> BaseTools/Source/Python/build/build.py   |  30 +
> 3 files changed, 189 insertions(+), 9 deletions(-)
>
>diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py
>b/BaseTools/Source/Python/AutoGen/AutoGen.py
>index 70e2e62..246bfec 100644
>--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
>+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
>@@ -41,10 +41,11 @@ import Common.VpdInfoFile as VpdInfoFile
> from GenPcdDb import CreatePcdDatabaseCode
> from Workspace.MetaFileCommentParser import UsageList
> from Common.MultipleWorkspace import MultipleWorkspace as mws
> import InfSectionParser
> import datetime
>+import hashlib
>
> ## Regular expression for splitting Dependency Expression string into tokens
> gDepexTokenPattern = re.compile("(\(|\)|\w+| \S+\.inf)")
>
> #
>@@ -263,10 +264,14 @@ class WorkspaceAutoGen(AutoGen):
> self.FdfFile= FlashDefinitionFile
> self.FdTargetList   = Fds
> self.FvTargetList   = Fvs
> self.CapTargetList  = Caps
> self.AutoGenObjectList = []
>+self._BuildDir  = None
>+self._FvDir = None
>+self._MakeFileDir   = None
>+self._BuildCommand  = None
>
> # there's many relative directory operations, so ...
> os.chdir(self.WorkspaceDir)
>
> #
>@@ -642,10 +647,18 @@ class WorkspaceAutoGen(AutoGen):
> #
> Pa.CollectPlatformDynamicPcds()
> Pa.CollectFixedAtBuildPcds()
> self.AutoGenObjectList.append(Pa)
>
>+#
>+# Generate Package level hash value
>+#
>+GlobalData.gPackageHash[Arch] = {}
>+if GlobalData.gUseHashCache:
>+for Pkg in Pkgs:
>+self._GenPkgLevelHash(Pkg)
>+
> #
> # Check PCDs token value conflict in each DEC file.
> #
> self._CheckAllPcdsTokenValueConflict()
>
>@@ -655,15 +668,10 @@ class WorkspaceAutoGen(AutoGen):
> self._CheckPcdDefineAndType()
>
> # if self.FdfFile:
> # self._CheckDuplicateInFV(Fdf)
>
>-self._BuildDir = None
>-self._FvDir = None
>-self._MakeFileDir = None
>-self._BuildCommand = None
>-
> #
> # Create BuildOptions Macro & PCD metafile, also add the Active 
> Platform
>and FDF file.
> #
> content = 'gCommandLineDefines: '
> content += str(GlobalData.gCommandLineDefines)
>@@ -675,10 +683,14 @@ class WorkspaceAutoGen(AutoGen):
> content += str(self.Platform)
> content += os.linesep
> if self.FdfFile:
> content += 'Flash Image Definition: '
> content += str(self.FdfFile)
>+content += os.linesep
>+if GlobalData.gBinCacheDest:
>+content += 'Cache of .efi location: '
>+content += str(GlobalData.gBinCacheDest)
> SaveFileOnChange(os.path.join(self.BuildDir, 'BuildOptions'), content,
>False)
>
> #
> # Create PcdToken Number file for Dynamic/DynamicEx Pcd.
> #
>@@ -704,10 +716,22 @@ class WorkspaceAutoGen(AutoGen):
>  

Re: [edk2] [Patch] Build spec: add description for build with binary cache

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Monday, September 04, 2017 4:43 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch] Build spec: add description for build with binary cache
>
>fixes:https://bugzilla.tianocore.org/show_bug.cgi?id=689
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> .../82_auto-generation_process.md| 20 
> README.md|  1 +
> appendix_d_buildexe_command/d4_usage.md  | 19
>+++
> 3 files changed, 36 insertions(+), 4 deletions(-)
>
>diff --git a/8_pre-build_autogen_stage/82_auto-generation_process.md
>b/8_pre-build_autogen_stage/82_auto-generation_process.md
>index 671a7d5..f1d7158 100644
>--- a/8_pre-build_autogen_stage/82_auto-generation_process.md
>+++ b/8_pre-build_autogen_stage/82_auto-generation_process.md
>@@ -1031,10 +1031,30 @@ maximum command line length.  The default
>value is 4096.
> **Note:** The following `FLAGS` options are included in the response file:
> `PP_FLAGS`, `CC_FLAGS`, `VFRPP_FLAGS`, `APP_FLAGS`, `ASLPP_FLAGS`,
>`ASLCC_FLAGS`,
> and `ASM_FLAGS`.
> **
>
>+ 8.2.4.15 Build with Binary Cache
>+
>+**build** tool provides three new options for binary cache feature.
>+--hash enables hash-based caching during build process. when --hash is
>enabled,
>+build tool will base on the module hash value to do the incremental build,
>without
>+--hash, build tool will base on the timestamp to do the incremental build. --
>hash
>+option use md5 method to get every hash value, DSC/FDF, tools_def.txt,
>build_rule.txt
>+and build command are calculated as global hash value, Package DEC and its
>include
>+header files are calculated as package hash value, Module source files and its
>INF
>+file are calculated as module hash value. Library hash value will combine the
>global
>+hash value and its dependent package hash value. Driver hash value will
>combine the
>+global hash value, its dependent package hash value and its linked library
>hash value.
>+When --hash and --bindest are specified, build tool will copy the generated
>binary
>+files for each module into the directory specified by bindest at the build
>phase.
>+Bindest caches all generated binary files.
>+When --hash and --binsource are specified, build tool will try to get the
>binary
>+files from the binary source directory at the build phase. If the cached 
>binary
>has
>+the same hash value, it will be directly used. Otherwise, build tool will
>compile the
>+source files and generate the binary files.
>+
> ### 8.2.5 Post processing
>
> Once all files are parsed, the build tools will do following work for each EDK
> II module:
>
>diff --git a/README.md b/README.md
>index 52abb6a..ca59a35 100644
>--- a/README.md
>+++ b/README.md
>@@ -215,5 +215,6 @@ Copyright (c) 2008-2017, Intel Corporation. All rights
>reserved.
> || [#523](https://bugzilla.tianocore.org/show_bug.cgi?id=523) 
> Build
>spec: add EBNF for the --pcd syntax in the Section D.4
>|   |
> || [#517](https://bugzilla.tianocore.org/show_bug.cgi?id=517) 
> Build
>spec: chapter 5.2.2 Guided Tools add description for Pkcs7Sign tool and
>BrotliCompress tool
>|   |
> || [#481](https://bugzilla.tianocore.org/show_bug.cgi?id=481) 
> Build
>Spec: add clarification for not used Pcd that build tool will not do additional
>checks on its value
>|   |
> || [#518](https://bugzilla.tianocore.org/show_bug.cgi?id=518) 
> Build
>Spec: Update Precedence of PCD Values
>|   |
> || [#669](https://bugzilla.tianocore.org/show_bug.cgi?id=669) 
> Build
>Spec: Add multi-arg support to PREBUILD/POSTBUILD
>|   |
>+|| [#689](https://bugzilla.tianocore.org/show_bug.cgi?id=689) 
>Build
>spec: add description for build with binary cache
>|   |
>diff --git a/appendix_d_buildexe_command/d4_usage.md
>b/appendix_d_buildexe_command/d4_usage.md
>index b71f2d0..6a91c3a 100644
>--- a/appendix_d_buildexe_command/d4_usage.md
>+++ b/appendix_d_buildexe_command/d4_usage.md
>@@ -32,19 +32,20 @@
> ## D.4 Usage
>
> ```ini
> Usage: build.exe [options]
> [all|fds|genc|genmake|clean|cleanall|cleanlib|modules|libraries|run]
>-Copyright (c) 2007 - 2014, Intel Corporation All rights reserved.
>+Copyright (c) 2007 - 2017, Intel Corporation All rights reserved.
>
> Options:
>   --version show program's version number and exit
>   -h, --helpshow this help message and exit
>   -a TARGETARCH, --arch=TARGETARCH
>-ARCHS is one of list: 

Re: [edk2] Fwd: StartImage with Secure Boot on Self-Signed App

[Gao, Liming]

You can load and start the image based on PeCoffLib APIs in BasePeCoffLib 
instead of LoadImage() and StartImage() service. 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>David F.
>Sent: Friday, September 08, 2017 11:34 PM
>To: Gary Lin 
>Cc: edk2-devel@lists.01.org
>Subject: Re: [edk2] Fwd: StartImage with Secure Boot on Self-Signed App
>
>Actually, even a StartImageEx() would be fine with parameter to allow options.
>
>On Thu, Sep 7, 2017 at 7:51 PM, David F.  wrote:
>> Thanks, looking forward, can the people on the board dealing with the
>> specification please consider revising EFI_LOADED_IMAGE_PROTOCOL to
>> include a new "Flags" field and one of the bits allows StartImage to
>> start the image even if LoadImage reported a EFI_SECURITY_VIOLATION
>> was reported.  defined bit name could be #define
>> EFI_LOADED_IMAGE_PROTOCOL_FLAG_SELF_VALIDATED
>0x0001ULL.
>>  This provides a clean interface for applications without having to
>> hack StartImage() with a potential conflict with future changes to the
>> internal firmware.
>>
>>
>> On Thu, Sep 7, 2017 at 7:11 PM, Gary Lin  wrote:
>>> On Thu, Sep 07, 2017 at 01:00:03PM -0700, David F. wrote:
 Hello,

 What is the proper way to allow running another app that is verified
 with a self-signed certificate?

 Example, App1 is signed with one that allows secure boot booting (in
 firmware) and has a public key embedded in the signed code, App2 is
 verified by App1 and so is allowed to run, but because the key is not
 in secure boot firmware, StartImage will not run it (although
 LoadImage did what it needed to do and already reported the security
 violation potential).   Do we have to roll our own StartImage?  or is
 something already in place?  I can't rely on changing an internal
 private structure field to allow StartImage to work since each
 firmware platform may change the way it all works, looking for the
 proper method as designed.

>>> The major linux distros are using shim(*) to verify the bootloaders and
>>> kernels signed by ourselves, and shim implements its own StartImage.
>>>
>>> If your application is going to be deployed to the newer UEFI, instead
>>> of using the built-in openssl, you can try EFI_PKCS7_VERIFY_PROTOCOL to
>>> verify the UEFI images. It will make your application much slimmer and
>>> easier to maintain.
>>>
>>> Cheers,
>>>
>>> Gary Lin
>>>
>>> (*) https://github.com/rhboot/shim
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/3] ArmVirtPkg: add HTTP_BOOT_ENABLE and UsbMassStorageDxe

[Ard Biesheuvel]

On 11 September 2017 at 22:08, Laszlo Ersek  wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: armvirt_http_usbstor
>
> I was... ugh... experimenting with "stuff" over the weekend, and
> realized these were missing.
>
> Cc: Ard Biesheuvel 
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
>   ArmVirtPkg: don't build the network stack uselessly for Xen
>   ArmVirtPkg/ArmVirtQemu: port HTTP_BOOT_ENABLE from OvmfPkg
>   ArmVirtPkg/ArmVirtQemu: include UsbMassStorageDxe
>
>  ArmVirtPkg/ArmVirt.dsc.inc   | 15 --
>  ArmVirtPkg/ArmVirtQemu.dsc   | 31 
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 31 
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  7 +
>  4 files changed, 69 insertions(+), 15 deletions(-)
>

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

Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ard Biesheuvel]

On 12 September 2017 at 06:01, Ni, Ruiyu  wrote:
> Laszlo,
> Your understanding is: DeviceAddress = HostAddress + AddressTranslationOffset
> But my patch assumes: HostAddress = DeviceAddress + AddressTranslationOffset
>
> They are totally different. If I follow your understanding, the patch is 
> wrong!
> Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply to 
> the
> Starting address of a BAR to convert it to a PCI address" very clearly, I 
> quoted
> the statement from ACPI spec.
> Your understanding to "apply to" is "add", my understanding is "minus".
>

Even though we are stretching the ACPI definition of a QWord
descriptor beyond its original meaning, I don't think there is a lot
of ambiguity here, to be honest. The AddrRangeMin field contains the
address on the secondary side of a bridge, and the primary value can
be obtained by 'applying' the ATO. In my opinion, applying a (positive
or negative) offset implies addition, not subtraction, as subtraction
involves negating the second addend before applying it. And the
secondary side of the host bridge is clearly the PCI side. It does
mean the offset field is signed, though.

So I don't agree with the conclusion that no clarification is
required. We need to make sure the spec is crystal clear in this
regard. But I do agree with the change, I think it is the only
solution that makes sense.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel