[edk2] [Patch][edk2-platforms/minnowboard-max-udk2017] Vlv2TbltDevicePkg: Enable signed capsule by default

[Guo, Mang]

Contributed-under: TianoCore Contribution Agreement 1.1

Signed-off-by: Guo Mang 
---
 Vlv2TbltDevicePkg/PlatformPkgConfig.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Vlv2TbltDevicePkg/PlatformPkgConfig.dsc 
b/Vlv2TbltDevicePkg/PlatformPkgConfig.dsc
index f172f8f..b6df2a0 100644
--- a/Vlv2TbltDevicePkg/PlatformPkgConfig.dsc
+++ b/Vlv2TbltDevicePkg/PlatformPkgConfig.dsc
@@ -93,4 +93,4 @@ DEFINE ESRT_ENABLE   = TRUE
 #
  DEFINE SOURCE_DEBUG_ENABLE = FALSE
 
-DEFINE CAPSULE_GENERATE_ENABLE = FALSE
+DEFINE CAPSULE_GENERATE_ENABLE = TRUE
-- 
2.10.1.windows.1

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

Re: [edk2] [PATCH] BaseTools/tools_def AARCH64: enable frame pointers for RELEASE builds

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
>Sent: Saturday, September 16, 2017 3:34 PM
>To: Ard Biesheuvel 
>Cc: edk2-devel@lists.01.org; Gao, Liming ; Zhu,
>Yonghong 
>Subject: Re: [PATCH] BaseTools/tools_def AARCH64: enable frame pointers
>for RELEASE builds
>
>On Fri, Sep 15, 2017 at 04:13:56PM -0700, Ard Biesheuvel wrote:
>> Commit 8f0b62a5dac0 ("BaseTools/tools_def AARCH64: enable frame
>pointers
>> for DEBUG builds") removed the -fomit-frame-pointer switch from the
>> shared CFLAGS between AARCH64 DEBUG and RELEASE build, and move it
>to
>> the RELEASE specific ones, so that DEBUG builds can produce a backtrace
>> when a crash occurs.
>>
>> This is actually a useful thing to have for RELEASE builds as well.
>> AArch64 has 30 general purpose registers, and so the performance hit of
>> having a frame pointer is unlikely to be noticeable, nor are the
>> additional 8 bytes of stack space likely to present a problem.
>>
>> So remove -fomit-frame-pointer altogether this time.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>
>Looks sensible.
>Reviewed-by: Leif Lindholm 
>
>/
>Leif
>
>> ---
>>  BaseTools/Conf/tools_def.template | 10 +-
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/BaseTools/Conf/tools_def.template
>b/BaseTools/Conf/tools_def.template
>> index cbb5024c1bd3..e93c2a0bf1ef 100755
>> --- a/BaseTools/Conf/tools_def.template
>> +++ b/BaseTools/Conf/tools_def.template
>> @@ -4990,7 +4990,7 @@ RELEASE_GCC47_ARM_CC_FLAGS   =
>DEF(GCC47_ARM_CC_FLAGS) -Wno-unused-but-set-v
>>  *_GCC47_AARCH64_CC_XIPFLAGS  =
>DEF(GCC47_AARCH64_CC_XIPFLAGS)
>>
>>DEBUG_GCC47_AARCH64_CC_FLAGS   = DEF(GCC47_AARCH64_CC_FLAGS)
>-O0
>> -RELEASE_GCC47_AARCH64_CC_FLAGS   = DEF(GCC47_AARCH64_CC_FLAGS)
>-Wno-unused-but-set-variable -fomit-frame-pointer
>> +RELEASE_GCC47_AARCH64_CC_FLAGS   =
>DEF(GCC47_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
>>NOOPT_GCC47_AARCH64_CC_FLAGS   = DEF(GCC47_AARCH64_CC_FLAGS)
>-O0
>>
>>
>###
>#
>> @@ -5130,7 +5130,7 @@ RELEASE_GCC48_ARM_CC_FLAGS   =
>DEF(GCC48_ARM_CC_FLAGS) -Wno-unused-but-set-v
>>  *_GCC48_AARCH64_CC_XIPFLAGS  =
>DEF(GCC48_AARCH64_CC_XIPFLAGS)
>>
>>DEBUG_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS)
>-O0
>> -RELEASE_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS)
>-Wno-unused-but-set-variable -fomit-frame-pointer
>> +RELEASE_GCC48_AARCH64_CC_FLAGS   =
>DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable
>>NOOPT_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS)
>-O0
>>
>>
>###
>#
>> @@ -5272,7 +5272,7 @@ RELEASE_GCC49_ARM_CC_FLAGS   =
>DEF(GCC49_ARM_CC_FLAGS) -Wno-unused-but-set-v
>>DEBUG_GCC49_AARCH64_DLINK_FLAGS  =
>DEF(GCC49_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
>>DEBUG_GCC49_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>
>> -RELEASE_GCC49_AARCH64_CC_FLAGS =
>DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-
>unused-const-variable -mcmodel=tiny -fomit-frame-pointer
>> +RELEASE_GCC49_AARCH64_CC_FLAGS =
>DEF(GCC49_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable -Wno-
>unused-const-variable -mcmodel=tiny
>>  RELEASE_GCC49_AARCH64_DLINK_FLAGS  =
>DEF(GCC49_AARCH64_DLINK_FLAGS)
>>
>>NOOPT_GCC49_AARCH64_CC_FLAGS =
>DEF(GCC49_AARCH64_CC_FLAGS) -O0 -mcmodel=small
>> @@ -5428,7 +5428,7 @@ RELEASE_GCC5_ARM_DLINK_FLAGS =
>DEF(GCC5_ARM_DLINK_FLAGS) -flto -Os -L$(WORKS
>>DEBUG_GCC5_AARCH64_DLINK_FLAGS =
>DEF(GCC5_AARCH64_DLINK_FLAGS) -z common-page-size=0x1000
>>DEBUG_GCC5_AARCH64_DLINK_XIPFLAGS = -z common-page-size=0x20
>>
>> -RELEASE_GCC5_AARCH64_CC_FLAGS= DEF(GCC5_AARCH64_CC_FLAGS)
>-flto -Wno-unused-but-set-variable -Wno-unused-const-variable -
>mcmodel=tiny -fomit-frame-pointer
>> +RELEASE_GCC5_AARCH64_CC_FLAGS= DEF(GCC5_AARCH64_CC_FLAGS)
>-flto -Wno-unused-but-set-variable -Wno-unused-const-variable -
>mcmodel=tiny
>>  RELEASE_GCC5_AARCH64_DLINK_FLAGS =
>DEF(GCC5_AARCH64_DLINK_FLAGS) -flto -Os -
>L$(WORKSPACE)/ArmPkg/Library/GccLto -llto-aarch64 -Wl,-plugin-opt=-pass-
>through=-llto-aarch64 -Wno-lto-type-mismatch
>>
>>NOOPT_GCC5_AARCH64_CC_FLAGS= DEF(GCC5_AARCH64_CC_FLAGS) -
>O0 -mcmodel=small
>> @@ -5513,7 +5513,7 @@ RELEASE_CLANG35_ARM_CC_FLAGS =
>DEF(CLANG35_ARM_CC_FLAGS) $(ARCHCC_FLAGS) $(P
>>  *_CLANG35_AARCH64_CC_XIPFLAGS= DEF(GCC_AARCH64_CC_XIPFLAGS)
>>
>>DEBUG_CLANG35_AARCH64_CC_FLAGS =
>DEF(CLANG35_AARCH64_CC_FLAGS) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
>-O0
>> -RELEASE_CLANG35_AARCH64_CC_FLAGS =
>DEF(CLANG35_AARCH64_CC_FLAGS) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
>-Oz 

Re: [edk2] Failed to clear configuration in Ip4Config2 Protocol

[Wu, Jiaxin]

Hi karunakar,

You can verify the DNS device path with HTTP boot feature. After the successful 
HTTP boot DNS parsing,  the device path should be like: 
//../Mac(...)[/Vlan(...)][/Wi-Fi(...)]/IPv4(...)[/Dns(...)]/Uri(...).

That is recommend way for the verification.

Besides, you can also draft the App to call  the DevPathFromTextDns and 
DevPathToTextDns libraries for the more verification.

Thanks,
Jiaxin
  

> -Original Message-
> From: Karunakar P [mailto:karunak...@amiindia.co.in]
> Sent: Monday, September 18, 2017 12:22 PM
> To: Wu, Jiaxin ; 'edk2-devel@lists.01.org'  de...@lists.01.org>; Ye, Ting 
> Subject: RE: [edk2] Failed to clear configuration in Ip4Config2 Protocol
> 
> Hi Jiaxin,
> 
> Thank you very much for your info, Yes it works fine for manual configuration.
> 
> And also could you please provide steps to verify "Add DNS device path
> node" feature.
> 
> Thanks,
> karunakar
> 
> -Original Message-
> From: Wu, Jiaxin [mailto:jiaxin...@intel.com]
> Sent: Monday, September 18, 2017 7:46 AM
> To: Karunakar P; 'edk2-devel@lists.01.org'; Ye, Ting
> Subject: RE: [edk2] Failed to clear configuration in Ip4Config2 Protocol
> 
> Hi Karunakar,
> 
> According the UEFI Spec, the Ip4Config2DataTypeManualAddress,
> Ip4Config2DataTypeGateway and Ip4Config2DataTypeDnsServer
> configuration data are not allowed to set via SetData() if the policy is DHCP.
> So, the clear feature is only for the manual configuration. This is our design
> purpose and also the reason why the feature is not apply to the
> Ip4Config2DataTypeInterfaceInfo/Ip4Config2DataTypePolicy.
> 
> Thanks,
> Jiaxin
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Karunakar P
> > Sent: Friday, September 15, 2017 5:41 PM
> > To: 'edk2-devel@lists.01.org' 
> > Subject: Re: [edk2] Failed to clear configuration in Ip4Config2
> > Protocol
> >
> > Hello All,
> >
> > Could you please anyone provide comment on this?
> >
> > Thank you,
> > karunakar
> >
> > From: Karunakar P
> > Sent: Wednesday, September 13, 2017 7:04 PM
> > To: 'edk2-devel@lists.01.org'
> > Subject: RE: RE: Failed to clear configuration in Ip4Config2 Protocol
> >
> > Hello All,
> >
> > I was trying to verify the feature "Allow SetData to clear
> > configuration in Ip4Config2/Ip6Config Protocol" , But SetData returns
> > with Write Protected Error Status
> >
> > [Steps followed]
> >
> > 1.   I've taken the above feature changes.
> >
> > 2.   I've a UEFI test Application which call to SetData with DataSize 
> > is 0 and
> > Data is NULL
> >
> > Status = Ip4Cfg2->SetData (
> >
> > Ip4Cfg2,
> >
> > Ip4Config2DataTypeManualAddress,
> >
> > 0,
> >
> > 0
> >
> > );
> >
> > 3.   But SetData returns with Write Protected Error Status// 
> > Status =
> > Write Protected
> >
> > 4.Faced the same error for setting Gateway & DnsServer
> >
> > Guess the return is happening from
> > Ip4Config2SetManualAddress() ->
> > ...
> >   if (Instance->Policy != Ip4Config2PolicyStatic) {
> > return EFI_WRITE_PROTECTED;
> >   }
> > ...
> >
> > Could you please help on this whether am I missing anything or
> > anything else need to be done to resolve this?
> >
> > Thanks,
> > karunakar
> >
> >
> > From: Karunakar P
> > Sent: Wednesday, September 13, 2017 7:00 PM
> > To: edk2-devel@lists.01.org
> > Subject: RE: Failed to clear configuration in Ip4Config2 Protocol
> >
> > Hello All,
> >
> > I was trying to verify the feature "Allow SetData to clear
> > configuration in Ip4Config2/Ip6Config Protocol" , But SetData returns
> > with Write Protected Error Status
> >
> > [Steps followed]
> >
> > 1.   I've taken the above feature changes.
> >
> > 2.   I've a UEFI test Application which call to SetData with DataSize 
> > is 0 and
> > Data is NULL
> >
> > Status = Ip4Cfg2->SetData (
> >
> > Ip4Cfg2,
> >
> > Ip4Config2DataTypeManualAddress,
> >
> > 0,
> >
> > 0
> >
> > );
> >
> > 3.   But SetData returns with Write Protected Error Status// 
> > Status =
> > Write Protected
> >
> > 4.Faced the same error for setting Gateway 
> >
> > Guess the error is happening from
> >
> >
> > 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 v2 0/3] UDF partition driver fix

[Ni, Ruiyu]

Paulo,
Could you please paste a "map -r" output on a CDROM which
contains Eltorito volume?
I want to confirm that the result is expected.

Thanks/Ray

> -Original Message-
> From: Paulo Alcantara [mailto:pca...@zytor.com]
> Sent: Sunday, September 17, 2017 9:13 PM
> To: edk2-devel@lists.01.org
> Cc: Paulo Alcantara ; Kinney, Michael D
> ; Gao, Liming ; Laszlo
> Ersek ; Ni, Ruiyu ; Zeng, Star
> ; Yao, Jiewen 
> Subject: [PATCH v2 0/3] UDF partition driver fix
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=707
> 
> Hi,
> 
> This patchset fixes a bug in Partition driver that UDF partitions occupied the
> entire disk space instead of using LVD space only.
> 
> BTW, I've only tested it under OVMF and built it with GCC only. That would
> be appreciable if someone could build with other toolchains and see if this
> doesn't break.
> 
> I used a Windows 10 ISO image with UdfDxe disabled and enabled. The `map
> -r` output seemed OK. No breakage when booting an OS off an ElTorito
> partition from an UDF bridge disk.
> 
> v1->v2:
>  - Followed Laszlo's suggestions to submit a proper patchset. Thanks!
>  - As I'm still waiting for Ruiyu and Star to test this fix, I took
>advantage of it and did some code cleanups :-)
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-partition-fix-v2
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Laszlo Ersek 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Reported-by: Ruiyu Ni 
> Signed-off-by: Paulo Alcantara 
> ---
> 
> Paulo Alcantara (3):
>   MdePkg: Add UDF volume structure definitions
>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c| 323
> +++-
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c |  13 +-
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 515
> 
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c  |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h  |  88 +---
>  MdePkg/Include/IndustryStandard/Udf.h |  63 +++
>  6 files changed, 566 insertions(+), 443 deletions(-)
> 
> --
> 2.11.0

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

Re: [edk2] [PATCH] RFC Inform UEFI memory to Linux

[Ard Biesheuvel]

On 17 September 2017 at 21:07, Udit Kumar  wrote:
> Hi Ard,
>
>> Thanks for the patch. But please consider carefully what you're doing:
>> My point is really that you can just remove lines 124 - 189 instead.
>
> We want, OS to re-use firmware volume area.  Therefore making firmware volume 
> visible.
> You are right, removing lines  124-189 will do the same, and no need of 
> adding splitting area with same
> attribute.
>
> Also, I am not sure, if we can remove this entirely, therefore we posted this 
> as RFC:)
> Code comments says
>
>  // EDK2 does not have the concept of boot firmware copied into DRAM. To 
> avoid the DXE
>  // core to overwrite this area we must mark the region with the attribute 
> non-present
>
> On our architecture, we start boot from DRAM. And we haven't seen any 
> overwrite
> by DXE. This work perfectly well for us.
>
> Do you think, on other platforms it may add a risk of overwrite ?
>

We wondered about that as well. As far as I can tell, a compressed FV
will be decompressed completely into a memory region that is reserved
for it before the DXE core is invoked. This means the outer,
uncompressed FV could technically be overwritten, but in practice, we
never refer back to it in a PrePi build, and we should probably not
even build the FV hob for this volume if we can avoid it.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch V2] BaseTools: Fix a bug to correct SourceFileList

[Yonghong Zhu]

We met a case that use two microcode files in the Microcode.inf file,
one is .mcb file, another is .txt file. then it cause build failure
because the SourceFileList include the .txt file's output file, while
this output file is still not be generated, so it cause
GetFileDependency report failure.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 BaseTools/Source/Python/AutoGen/GenMake.py | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py 
b/BaseTools/Source/Python/AutoGen/GenMake.py
index 0f3ddd5..942eb44 100644
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -787,12 +787,19 @@ cleanlib:
 ForceIncludedFile = []
 for File in self._AutoGenObject.AutoGenFileList:
 if File.Ext == '.h':
 ForceIncludedFile.append(File)
 SourceFileList = []
+OutPutFileList = []
 for Target in self._AutoGenObject.IntroTargetList:
 SourceFileList.extend(Target.Inputs)
+OutPutFileList.extend(Target.Outputs)
+
+if OutPutFileList:
+for Item in OutPutFileList:
+if Item in SourceFileList:
+SourceFileList.remove(Item)
 
 self.FileDependency = self.GetFileDependency(
 SourceFileList,
 ForceIncludedFile,
 self._AutoGenObject.IncludePathList + 
self._AutoGenObject.BuildOptionIncPathList
-- 
2.6.1.windows.1

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

Re: [edk2] Failed to clear configuration in Ip4Config2 Protocol

[Karunakar P]

Hi Jiaxin,

Thank you very much for your info, Yes it works fine for manual configuration.

And also could you please provide steps to verify "Add DNS device path node" 
feature. 

Thanks,
karunakar

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com] 
Sent: Monday, September 18, 2017 7:46 AM
To: Karunakar P; 'edk2-devel@lists.01.org'; Ye, Ting
Subject: RE: [edk2] Failed to clear configuration in Ip4Config2 Protocol

Hi Karunakar,

According the UEFI Spec, the Ip4Config2DataTypeManualAddress, 
Ip4Config2DataTypeGateway and Ip4Config2DataTypeDnsServer configuration data 
are not allowed to set via SetData() if the policy is DHCP. So, the clear 
feature is only for the manual configuration. This is our design  purpose and 
also the reason why the feature is not apply to the 
Ip4Config2DataTypeInterfaceInfo/Ip4Config2DataTypePolicy.

Thanks,
Jiaxin


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Karunakar P
> Sent: Friday, September 15, 2017 5:41 PM
> To: 'edk2-devel@lists.01.org' 
> Subject: Re: [edk2] Failed to clear configuration in Ip4Config2 
> Protocol
> 
> Hello All,
> 
> Could you please anyone provide comment on this?
> 
> Thank you,
> karunakar
> 
> From: Karunakar P
> Sent: Wednesday, September 13, 2017 7:04 PM
> To: 'edk2-devel@lists.01.org'
> Subject: RE: RE: Failed to clear configuration in Ip4Config2 Protocol
> 
> Hello All,
> 
> I was trying to verify the feature "Allow SetData to clear 
> configuration in Ip4Config2/Ip6Config Protocol" , But SetData returns 
> with Write Protected Error Status
> 
> [Steps followed]
> 
> 1.   I've taken the above feature changes.
> 
> 2.   I've a UEFI test Application which call to SetData with DataSize is 
> 0 and
> Data is NULL
> 
> Status = Ip4Cfg2->SetData (
> 
> Ip4Cfg2,
> 
> Ip4Config2DataTypeManualAddress,
> 
> 0,
> 
> 0
> 
> );
> 
> 3.   But SetData returns with Write Protected Error Status// 
> Status =
> Write Protected
> 
> 4.Faced the same error for setting Gateway & DnsServer
> 
> Guess the return is happening from
> Ip4Config2SetManualAddress() ->
> ...
>   if (Instance->Policy != Ip4Config2PolicyStatic) {
> return EFI_WRITE_PROTECTED;
>   }
> ...
> 
> Could you please help on this whether am I missing anything or 
> anything else need to be done to resolve this?
> 
> Thanks,
> karunakar
> 
> 
> From: Karunakar P
> Sent: Wednesday, September 13, 2017 7:00 PM
> To: edk2-devel@lists.01.org
> Subject: RE: Failed to clear configuration in Ip4Config2 Protocol
> 
> Hello All,
> 
> I was trying to verify the feature "Allow SetData to clear 
> configuration in Ip4Config2/Ip6Config Protocol" , But SetData returns 
> with Write Protected Error Status
> 
> [Steps followed]
> 
> 1.   I've taken the above feature changes.
> 
> 2.   I've a UEFI test Application which call to SetData with DataSize is 
> 0 and
> Data is NULL
> 
> Status = Ip4Cfg2->SetData (
> 
> Ip4Cfg2,
> 
> Ip4Config2DataTypeManualAddress,
> 
> 0,
> 
> 0
> 
> );
> 
> 3.   But SetData returns with Write Protected Error Status// 
> Status =
> Write Protected
> 
> 4.Faced the same error for setting Gateway 
> 
> Guess the error is happening from
> 
> 
> 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] RFC Inform UEFI memory to Linux

[Udit Kumar]

Hi Ard, 

> Thanks for the patch. But please consider carefully what you're doing:
> My point is really that you can just remove lines 124 - 189 instead.

We want, OS to re-use firmware volume area.  Therefore making firmware volume 
visible. 
You are right, removing lines  124-189 will do the same, and no need of adding 
splitting area with same 
attribute. 

Also, I am not sure, if we can remove this entirely, therefore we posted this 
as RFC:)
Code comments says 

 // EDK2 does not have the concept of boot firmware copied into DRAM. To avoid 
the DXE
 // core to overwrite this area we must mark the region with the attribute 
non-present

On our architecture, we start boot from DRAM. And we haven't seen any overwrite 
by DXE. This work perfectly well for us. 

Do you think, on other platforms it may add a risk of overwrite ? 

Regards
Udit

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Saturday, September 16, 2017 4:25 AM
> To: Leif Lindholm 
> Cc: Meenakshi Aggarwal ; edk2-
> de...@lists.01.org; Udit Kumar 
> Subject: Re: [PATCH] RFC Inform UEFI memory to Linux
> 
> On 15 September 2017 at 03:13, Leif Lindholm 
> wrote:
> > On Fri, Sep 15, 2017 at 08:02:34PM +0530, Meenakshi Aggarwal wrote:
> >> From: Udit Kumar 
> >>
> >> While creating Hob list, ArmPlatformPkg is hiding UEFI memory.
> >> whereas this memory can be used by OS.
> >>
> >> This patch, allows OS to use UEFI code area.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Meenakshi Aggarwal 
> >> Signed-off-by: Udit Kumar 
> >
> > I will let Ard comment on the technical aspect, since you've been
> > discussing this offline.
> >
> > However, there is something broken in your setup: the patch should
> > look like this:
> > ---
> > iff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> > b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> > index 2feb11f21d..d3fa894244 100644
> > --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> > +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> > @@ -150,7 +150,7 @@ MemoryPeim (
> >} else {
> >  // Create the System Memory HOB for the
> >  // firmware with the non-present attribute
> >  BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> > -ResourceAttributes &
> ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
> > +ResourceAttributes,
> >  PcdGet64 (PcdFdBaseAddress),
> >  PcdGet32 (PcdFdSize));
> >
> > @@ -161,7 +161,7 @@ MemoryPeim (
> >  } else {
> >// Create the System Memory HOB for the firmware
> >// with the non-present attribute
> >BuildResourceDescriptorHob (EFI_RESOURCE_SYSTEM_MEMORY,
> > -  ResourceAttributes &
> ~EFI_RESOURCE_ATTRIBUTE_PRESENT,
> > +  ResourceAttributes,
> >PcdGet64 (PcdFdBaseAddress),
> >PcdGet32 (PcdFdSize));
> >
> > ---
> >
> > I am guessing you have inadvertently modified the line terminations
> > from CRLF to LF (the evidence gets stripped out by SMTP).
> > Please resubmit with this addressed.
> >
> 
> Thanks for the patch. But please consider carefully what you're doing:
> in the original code, the entire DRAM region is added, and subsequently split 
> up
> so the part that overlaps with the FD can be registered with different 
> attributes.
> 
> After your patch, we carefully isolate the region that overlaps the FD, and
> subsequently register it with the exact same attributes, which means we didn't
> have to isolate it in the first place.
> 
> My point is really that you can just remove lines 124 - 189 instead.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Check Ipv6 Support for ISCSI attempt page

[Karunakar P]

Hi Ting Ye,

Thanks for your reply.

[Here the bug details]
Bug 710 - IPv6 support condition check for HTTP and ISCSI
https://bugzilla.tianocore.org/show_bug.cgi?id=710

Thanks,
karunakar

-Original Message-
From: Ye, Ting [mailto:ting...@intel.com] 
Sent: Monday, September 18, 2017 8:23 AM
To: Karunakar P; 'edk2-devel@lists.01.org'
Subject: RE: [edk2] Check Ipv6 Support for ISCSI attempt page

Hi Karunakar,

Thanks for capturing this. We agree that it's reasonable to add similar check 
to HTTP and ISCSI. Could you please help to file a bugzilla tracker for this?

Thanks,
Ting Ye

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
Karunakar P
Sent: Friday, September 15, 2017 5:37 PM
To: 'edk2-devel@lists.01.org' 
Subject: Re: [edk2] Check Ipv6 Support for ISCSI attempt page

Hello all,



We have a Broadcom NIC card which doesn't support IPv6 (Ipv6Supported flag is 
0).



1) But the HTTPv6 boot options are shown.

Unlike in PXE (PxeBcCheckIpv6Support()), the HTTP drivers in edk2 do not have a 
condition check whether the card supports IPv6 or not.



2) If the card doesn't support IPv6, then for ISCSI attempt page also, Internet 
Protocol should suppress IPv6



Could you please provide your comments on this?



Thank you

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

[edk2] [PATCH 2/2] UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page

[Jian J Wang]

>From CpuDxe driver perspective, it doesn't update GCD memory attributes 
from current page table setup during its initialization. So the memory
attributes in GCD might not reflect all memory attributes in real world.

Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Michael Kinney 
Suggested-by: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuDxe.c   |  5 +++
 UefiCpuPkg/CpuDxe/CpuDxe.h   |  9 
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 92 
 3 files changed, 106 insertions(+)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index b386f3b677..4e8fa100e0 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -863,6 +863,11 @@ RefreshGcdMemoryAttributes (
 FreePool (MemorySpaceMap);
   }
 
+  //
+  // Update page attributes
+  //
+  RefreshGcdMemoryAttributesFromPaging();
+
   mIsFlushingGCD = FALSE;
 }
 
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 4861abee76..a25b35c6eb 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -52,6 +52,10 @@
EFI_MEMORY_UCE   \
)
 
+#define EFI_MEMORY_PAGETYPE_MASK  (EFI_MEMORY_RP  | \
+   EFI_MEMORY_XP  | \
+   EFI_MEMORY_RO\
+   )
 
 /**
   Flush CPU data cache. If the instruction cache is fully coherent
@@ -261,5 +265,10 @@ SetDataSelectors (
   UINT16 Selector
   );
 
+VOID
+RefreshGcdMemoryAttributesFromPaging (
+  VOID
+  );
+
 #endif
 
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 2c61e7503e..bca4a497ac 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -23,6 +23,8 @@
 #include 
 #include 
 #include 
+
+#include "CpuDxe.h"
 #include "CpuPageTable.h"
 
 ///
@@ -767,6 +769,96 @@ AssignMemoryPageAttributes (
   return Status;
 }
 
+/**
+  Update GCD memory space attributes according to current page table setup.
+**/
+VOID
+RefreshGcdMemoryAttributesFromPaging (
+  VOID
+  )
+{
+  EFI_STATUS  Status;
+  UINTN   NumberOfDescriptors;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR *MemorySpaceMap;
+  PAGE_TABLE_LIB_PAGING_CONTEXT   PagingContext;
+  PAGE_ATTRIBUTE  PageAttribute;
+  UINT64  *PageEntry;
+  UINT64  PageLength;
+  UINT64  MemorySpaceLength;
+  UINT64  Length;
+  UINT64  BaseAddress;
+  UINT64  PageStartAddress;
+  UINT64  Attributes;
+  BOOLEAN DoUpdate;
+  UINTN   Index;
+
+  //
+  // Assuming that memory space map returned is sorted already; otherwise sort
+  // them in the order of lowest address to highest address.
+  //
+  Status = gDS->GetMemorySpaceMap (, );
+  ASSERT_EFI_ERROR (Status);
+
+  GetCurrentPagingContext ();
+
+  BaseAddress = 0;
+  PageLength  = 0;
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+if (MemorySpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
+  continue;
+}
+
+if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
+  //
+  // Current memory space starts at a new page. Resetting PageLength will
+  // trigger a retrieval of page attributes at new address.
+  //
+  PageLength = 0;
+}
+
+// Sync real page attributes to GCD
+BaseAddress   = MemorySpaceMap[Index].BaseAddress;
+MemorySpaceLength = MemorySpaceMap[Index].Length;
+while (MemorySpaceLength > 0) {
+  if (PageLength == 0) {
+PageEntry = GetPageTableEntry (, BaseAddress, 
);
+if (PageEntry == NULL) {
+  break;
+}
+
+//
+// Note current memory space might start in the middle of a page
+//
+PageStartAddress  = (*PageEntry) & 
(UINT64)PageAttributeToMask(PageAttribute);
+PageLength= PageAttributeToLength (PageAttribute) - 
(BaseAddress - PageStartAddress);
+Attributes= GetAttributesFromPageEntry (PageEntry);
+
+if (Attributes != (MemorySpaceMap[Index].Attributes & 
EFI_MEMORY_PAGETYPE_MASK)) {
+  DoUpdate = TRUE;
+  Attributes |= (MemorySpaceMap[Index].Attributes & 
~EFI_MEMORY_PAGETYPE_MASK);
+} else {
+  DoUpdate = FALSE;
+}
+  }
+
+  Length = MIN (PageLength, MemorySpaceLength);
+  if (DoUpdate) {
+gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Attributes);
+

[edk2] [PATCH 1/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

[Jian J Wang]

>From GCD perspective, its SetMemorySpaceAttributes() method doesn't accept
page related attributes. That means users cannot use it to change page
attributes, and have to turn to CPU arch protocol to do it, which is not
be allowed by PI spec.

Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Michael Kinney 
Suggested-by: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 45 -
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index a06f8bb77c..e9d1d5b612 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -40,6 +40,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #define PRESENT_MEMORY_ATTRIBUTES (EFI_RESOURCE_ATTRIBUTE_PRESENT)
 
+#define EXCLUSIVE_MEMORY_ATTRIBUTES   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+   EFI_MEMORY_WT | EFI_MEMORY_WB | \
+   EFI_MEMORY_WP | EFI_MEMORY_UCE)
+
+#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | \
+EFI_MEMORY_RO)
+
 #define INVALID_CPU_ARCH_ATTRIBUTES   0x
 
 //
@@ -654,28 +661,30 @@ ConverToCpuArchAttributes (
   UINT64 Attributes
   )
 {
-  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
-return EFI_MEMORY_UC;
-  }
+  UINT64  CpuArchAttributes;
 
-  if ( (Attributes & EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
-return EFI_MEMORY_WC;
+  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
+  NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
+return INVALID_CPU_ARCH_ATTRIBUTES;
   }
 
-  if ( (Attributes & EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
-return EFI_MEMORY_WT;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WB) == EFI_MEMORY_WB) {
-return EFI_MEMORY_WB;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WP) == EFI_MEMORY_WP) {
-return EFI_MEMORY_WP;
-  }
-
-  return INVALID_CPU_ARCH_ATTRIBUTES;
+  CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
 
+  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
+CpuArchAttributes |= EFI_MEMORY_UC;
+  } else if ( (Attributes & EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
+CpuArchAttributes |= EFI_MEMORY_WC;
+  } else if ( (Attributes & EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
+CpuArchAttributes |= EFI_MEMORY_WT;
+  } else if ( (Attributes & EFI_MEMORY_WB) == EFI_MEMORY_WB) {
+CpuArchAttributes |= EFI_MEMORY_WB;
+  } else if ( (Attributes & EFI_MEMORY_UCE) == EFI_MEMORY_UCE) {
+CpuArchAttributes |= EFI_MEMORY_UCE;
+  } else if ( (Attributes & EFI_MEMORY_WP) == EFI_MEMORY_WP) {
+CpuArchAttributes |= EFI_MEMORY_WP;
+  }
+
+  return CpuArchAttributes;
 }
 
 
-- 
2.14.1.windows.1

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

[edk2] [PATCH 0/2] Fixe out-of-sync issue between GCD and CPU driver

[Jian J Wang]

There're two issues here actually.

>From GCD perspective, its SetMemorySpaceAttributes() method doesn't accept
page related attributes. That means users cannot use it to change page
attributes, and have to turn to CPU arch protocol to do it, which is not
be allowed by PI spec.

>From CpuDxe driver perspective, it doesn't update GCD memory attributes 
from current page table setup during its initialization. So the memory
attributes in GCD might not reflect all memory attributes in real world.

Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Michael Kinney 
Suggested-by: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 

Jian J Wang (2):
  MdeModulePkg/Core: Fix out-of-sync issue in GCD
  UefiCpuPkg/CpuDxe: Fix out-of-sync issue in page attributes

 MdeModulePkg/Core/Dxe/Gcd/Gcd.c  | 45 
 UefiCpuPkg/CpuDxe/CpuDxe.c   |  5 +++
 UefiCpuPkg/CpuDxe/CpuDxe.h   |  9 
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 92 
 4 files changed, 133 insertions(+), 18 deletions(-)

-- 
2.14.1.windows.1

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

Re: [edk2] Check Ipv6 Support for ISCSI attempt page

[Ye, Ting]

Hi Karunakar,

Thanks for capturing this. We agree that it's reasonable to add similar check 
to HTTP and ISCSI. Could you please help to file a bugzilla tracker for this?

Thanks,
Ting Ye

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
Karunakar P
Sent: Friday, September 15, 2017 5:37 PM
To: 'edk2-devel@lists.01.org' 
Subject: Re: [edk2] Check Ipv6 Support for ISCSI attempt page

Hello all,



We have a Broadcom NIC card which doesn't support IPv6 (Ipv6Supported flag is 
0).



1) But the HTTPv6 boot options are shown.

Unlike in PXE (PxeBcCheckIpv6Support()), the HTTP drivers in edk2 do not have a 
condition check whether the card supports IPv6 or not.



2) If the card doesn't support IPv6, then for ISCSI attempt page also, Internet 
Protocol should suppress IPv6



Could you please provide your comments on this?



Thank you

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] Failed to clear configuration in Ip4Config2 Protocol

[Wu, Jiaxin]

Hi Karunakar,

According the UEFI Spec, the Ip4Config2DataTypeManualAddress, 
Ip4Config2DataTypeGateway and Ip4Config2DataTypeDnsServer configuration data 
are not
allowed to set via SetData() if the policy is DHCP. So, the clear feature is 
only for the manual configuration. This is our design  purpose and also the 
reason why the feature is not apply to the 
Ip4Config2DataTypeInterfaceInfo/Ip4Config2DataTypePolicy.

Thanks,
Jiaxin


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Karunakar P
> Sent: Friday, September 15, 2017 5:41 PM
> To: 'edk2-devel@lists.01.org' 
> Subject: Re: [edk2] Failed to clear configuration in Ip4Config2 Protocol
> 
> Hello All,
> 
> Could you please anyone provide comment on this?
> 
> Thank you,
> karunakar
> 
> From: Karunakar P
> Sent: Wednesday, September 13, 2017 7:04 PM
> To: 'edk2-devel@lists.01.org'
> Subject: RE: RE: Failed to clear configuration in Ip4Config2 Protocol
> 
> Hello All,
> 
> I was trying to verify the feature "Allow SetData to clear configuration in
> Ip4Config2/Ip6Config Protocol" , But SetData returns with Write Protected
> Error Status
> 
> [Steps followed]
> 
> 1.   I've taken the above feature changes.
> 
> 2.   I've a UEFI test Application which call to SetData with DataSize is 
> 0 and
> Data is NULL
> 
> Status = Ip4Cfg2->SetData (
> 
> Ip4Cfg2,
> 
> Ip4Config2DataTypeManualAddress,
> 
> 0,
> 
> 0
> 
> );
> 
> 3.   But SetData returns with Write Protected Error Status// 
> Status =
> Write Protected
> 
> 4.Faced the same error for setting Gateway & DnsServer
> 
> Guess the return is happening from
> Ip4Config2SetManualAddress() ->
> ...
>   if (Instance->Policy != Ip4Config2PolicyStatic) {
> return EFI_WRITE_PROTECTED;
>   }
> ...
> 
> Could you please help on this whether am I missing anything or anything else
> need to be done to resolve this?
> 
> Thanks,
> karunakar
> 
> 
> From: Karunakar P
> Sent: Wednesday, September 13, 2017 7:00 PM
> To: edk2-devel@lists.01.org
> Subject: RE: Failed to clear configuration in Ip4Config2 Protocol
> 
> Hello All,
> 
> I was trying to verify the feature "Allow SetData to clear configuration in
> Ip4Config2/Ip6Config Protocol" , But SetData returns with Write Protected
> Error Status
> 
> [Steps followed]
> 
> 1.   I've taken the above feature changes.
> 
> 2.   I've a UEFI test Application which call to SetData with DataSize is 
> 0 and
> Data is NULL
> 
> Status = Ip4Cfg2->SetData (
> 
> Ip4Cfg2,
> 
> Ip4Config2DataTypeManualAddress,
> 
> 0,
> 
> 0
> 
> );
> 
> 3.   But SetData returns with Write Protected Error Status// 
> Status =
> Write Protected
> 
> 4.Faced the same error for setting Gateway 
> 
> Guess the error is happening from
> 
> 
> 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] MdeModulePkg SmbiosMeasurementDxe: Skip measurement for OEM type

[Chiu, Chasel]

Reviewed-by: Chasel Chiu 

-Original Message-
From: Zeng, Star 
Sent: Thursday, September 14, 2017 7:29 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Yao, Jiewen ; Chiu, 
Chasel 
Subject: [PATCH] MdeModulePkg SmbiosMeasurementDxe: Skip measurement for OEM 
type

The generic driver has no way to know whether an OEM type should be filtered or 
not.
This patch is to update the code to skip measurement for OEM type and platform 
code can measure it by self if required.

Cc: Jiewen Yao 
Cc: Chasel Chiu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.c| 54 +-
 .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.inf  |  5 +-
 2 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c 
b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
index bc5e7464e133..4a3e99aefd0f 100644
--- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
+++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c
@@ -1,7 +1,7 @@
 /** @file
   This driver measures SMBIOS table to TPM.
 
-Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.
 This program and the accompanying materials  are licensed and made available 
under the terms and conditions of the BSD License  which accompanies this 
distribution.  The full text of the license may be found at @@ -277,30 +277,38 
@@ FilterSmbiosEntry (
   DEBUG ((EFI_D_INFO, "Smbios Table (Type - %d):\n", ((SMBIOS_STRUCTURE 
*)TableEntry)->Type));
   DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize););
 
-  FilterStruct = GetFilterStructByType (((SMBIOS_STRUCTURE 
*)TableEntry)->Type);
-  if (FilterStruct != NULL) {
-if (FilterStruct->Filter == NULL || FilterStruct->FilterCount == 0) {
-  // zero all table entries, except header
-  ZeroMem ((UINT8 *)TableEntry + sizeof(SMBIOS_STRUCTURE), TableEntrySize 
- sizeof(SMBIOS_STRUCTURE));
-} else {
-  Filter = FilterStruct->Filter;
-  for (Index = 0; Index < FilterStruct->FilterCount; Index++) {
-if (((SMBIOS_STRUCTURE *) TableEntry)->Length >= (Filter[Index].Offset 
+ Filter[Index].Size)) {
-  //
-  // The field is present in the SMBIOS entry.
-  //
-  if ((Filter[Index].Flags & SMBIOS_FILTER_TABLE_FLAG_IS_STRING) != 0) 
{
-CopyMem (, (UINT8 *)TableEntry + Filter[Index].Offset, 
sizeof(StringId));
-if (StringId != 0) {
-  // set ' ' for string field
-  String = GetSmbiosStringById (TableEntry, StringId, );
-  ASSERT (String != NULL);
-  //DEBUG ((EFI_D_INFO,"StrId(0x%x)-%a(%d)\n", StringId, String, 
StringLen));
-  SetMem (String, StringLen, ' ');
+  //
+  // Skip measurement for OEM types.
+  //
+  if (((SMBIOS_STRUCTURE *)TableEntry)->Type >= SMBIOS_OEM_BEGIN) {
+// zero all table fields, except header
+ZeroMem ((UINT8 *)TableEntry + sizeof(SMBIOS_STRUCTURE), 
+ TableEntrySize - sizeof(SMBIOS_STRUCTURE));  } else {
+FilterStruct = GetFilterStructByType (((SMBIOS_STRUCTURE 
*)TableEntry)->Type);
+if (FilterStruct != NULL) {
+  if (FilterStruct->Filter == NULL || FilterStruct->FilterCount == 0) {
+// zero all table fields, except header
+ZeroMem ((UINT8 *)TableEntry + sizeof(SMBIOS_STRUCTURE), 
TableEntrySize - sizeof(SMBIOS_STRUCTURE));
+  } else {
+Filter = FilterStruct->Filter;
+for (Index = 0; Index < FilterStruct->FilterCount; Index++) {
+  if (((SMBIOS_STRUCTURE *) TableEntry)->Length >= 
(Filter[Index].Offset + Filter[Index].Size)) {
+//
+// The field is present in the SMBIOS entry.
+//
+if ((Filter[Index].Flags & SMBIOS_FILTER_TABLE_FLAG_IS_STRING) != 
0) {
+  CopyMem (, (UINT8 *)TableEntry + Filter[Index].Offset, 
sizeof(StringId));
+  if (StringId != 0) {
+// set ' ' for string field
+String = GetSmbiosStringById (TableEntry, StringId, 
);
+ASSERT (String != NULL);
+//DEBUG ((EFI_D_INFO,"StrId(0x%x)-%a(%d)\n", StringId, String, 
StringLen));
+SetMem (String, StringLen, ' ');
+  }
 }
+// zero non-string field
+ZeroMem ((UINT8 *)TableEntry + Filter[Index].Offset, 
+ Filter[Index].Size);
   }
-  // zero non-string field
-  ZeroMem ((UINT8 *)TableEntry + Filter[Index].Offset, 
Filter[Index].Size);
 }
   }
 }
diff --git 
a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.inf 

Re: [edk2] [PATCH 0/3] UDF partition driver fix

[Ni, Ruiyu]

Paulo,
I checked carefully of patch #1, 50% carefully of #2, 20% carefully of #3.
I could only give comments from the EDKII coding style perspective.
I do provide some other comments, but please understand they are from
a person that knows very little about UDF. (I know the concept of Volume.
But just know that there are so many different types of descriptors.)
If my understanding is wrong, please correct me!

Again, thanks for the contribution.

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Paulo Alcantara
> Sent: Sunday, September 17, 2017 5:37 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/3] UDF partition driver fix
> 
> This series fixes an UDF issue with Partition driver as discussed in ML:
> 
> https://lists.01.org/pipermail/edk2-devel/2017-September/014694.html
> 
> Thanks!
> Paulo
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: udf-partition-fix
> 
> Paulo Alcantara (3):
>   MdePkg: Add UDF volume structure definitions
>   MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
>   MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes
> 
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 307 +++-
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c  |  13 +-
>  .../Universal/Disk/UdfDxe/FileSystemOperations.c   | 525 --
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c   |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h   |  88 +---
>  MdePkg/Include/IndustryStandard/Udf.h  |  63 +++
>  6 files changed, 560 insertions(+), 443 deletions(-)
> 
> --
> 2.11.0
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/3] MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes

[Ni, Ruiyu]

Paulo,
With the change in partition driver, I suppose UdfDxe driver
only needs to take care of area covered by the partition descriptor.
But why StartMainVolumeDescriptorSequence() still reads
LVD, TD, and PD?

I thought the UdfDxe driver's logic can be simplified a lot.
There should be no duplicated logic in Partition driver and udf driver.

Can you explain more?

Thanks/Ray

> -Original Message-
> From: Paulo Alcantara [mailto:pca...@zytor.com]
> Sent: Sunday, September 17, 2017 9:13 PM
> To: edk2-devel@lists.01.org
> Cc: Paulo Alcantara ; Dong, Eric ;
> Ni, Ruiyu ; Zeng, Star ; Laszlo
> Ersek 
> Subject: [PATCH v2 3/3] MdeModulePkg/UdfDxe: Rework driver to support
> PartitionDxe changes
> 
> This patch reworks the driver to support Partition driver changes.
> 
> Cc: Eric Dong 
> Cc: Paulo Alcantara 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara 
> ---
>  MdeModulePkg/Universal/Disk/UdfDxe/File.c |  13 +-
>  MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 515
> 
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.c  |   7 -
>  MdeModulePkg/Universal/Disk/UdfDxe/Udf.h  |  88 +---
>  4 files changed, 204 insertions(+), 419 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> index 01361141bb..f2c62967e8 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
> @@ -100,6 +100,7 @@ UdfOpenVolume (
>  >Volume,
>  >Root
>  );
> +  ASSERT_EFI_ERROR (Status);
>if (EFI_ERROR (Status)) {
>  goto Error_Find_Root_Dir;
>}
> @@ -131,7 +132,6 @@ Error_Alloc_Priv_File_Data:
>CleanupFileInformation (>Root);
> 
>  Error_Find_Root_Dir:
> -  CleanupVolumeInformation (>Volume);
> 
>  Error_Read_Udf_Volume:
>  Error_Invalid_Params:
> @@ -528,7 +528,6 @@ UdfClose (
>EFI_TPL OldTpl;
>EFI_STATUS  Status;
>PRIVATE_UDF_FILE_DATA   *PrivFileData;
> -  PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
> 
>OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
> 
> @@ -541,8 +540,6 @@ UdfClose (
> 
>PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
> 
> -  PrivFsData = PRIVATE_UDF_SIMPLE_FS_DATA_FROM_THIS (PrivFileData-
> >SimpleFs);
> -
>if (!PrivFileData->IsRootDirectory) {
>  CleanupFileInformation (>File);
> 
> @@ -551,10 +548,6 @@ UdfClose (
>  }
>}
> 
> -  if (--PrivFsData->OpenFiles == 0) {
> -CleanupVolumeInformation (>Volume);
> -  }
> -
>FreePool ((VOID *)PrivFileData);
> 
>  Exit:
> @@ -787,7 +780,7 @@ UdfGetInfo (
>} else if (CompareGuid (InformationType, )) {
>  String = VolumeLabel;
> 
> -FileSetDesc = PrivFsData->Volume.FileSetDescs[0];
> +FileSetDesc = >Volume.FileSetDesc;
> 
>  OstaCompressed = >LogicalVolumeIdentifier[0];
> 
> @@ -846,7 +839,7 @@ UdfGetInfo (
>  FileSystemInfo->Size= FileSystemInfoLength;
>  FileSystemInfo->ReadOnly= TRUE;
>  FileSystemInfo->BlockSize   =
> -  LV_BLOCK_SIZE (>Volume, UDF_DEFAULT_LV_NUM);
> +  PrivFsData->Volume.LogicalVolDesc.LogicalBlockSize;
>  FileSystemInfo->VolumeSize  = VolumeSize;
>  FileSystemInfo->FreeSpace   = FreeSpaceSize;
> 
> diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> index 4609580b30..63b643e60a 100644
> --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
> @@ -60,154 +60,111 @@ FindAnchorVolumeDescriptorPointer (
> 
>  EFI_STATUS
>  StartMainVolumeDescriptorSequence (
> -  IN   EFI_BLOCK_IO_PROTOCOL *BlockIo,
> -  IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
> -  IN   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
> -  OUT  UDF_VOLUME_INFO   *Volume
> +  IN   EFI_BLOCK_IO_PROTOCOL*BlockIo,
> +  IN   EFI_DISK_IO_PROTOCOL *DiskIo,
> +  OUT  UDF_VOLUME_INFO  *Volume
>)
>  {
> -  EFI_STATUS Status;
> -  UINT32 BlockSize;
> -  UDF_EXTENT_AD  *ExtentAd;
> -  UINT64 StartingLsn;
> -  UINT64 EndingLsn;
> -  VOID   *Buffer;
> -  UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
> -  UDF_PARTITION_DESCRIPTOR   *PartitionDesc;
> -  UINTN  Index;
> -  UINT32 LogicalBlockSize;
> +  EFI_STATUS  Status;
> +  UINT32  BlockSize;
> +  UINT64  BlockOffset;
> +  VOID*Buffer;
> +  UINT32  

Re: [edk2] [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition

[Ni, Ruiyu]

Paulo,
Several comments:
1. Can below logic be removed in PartitionDxe/Udf.c?  
while (!IsDevicePathEnd (DevicePathNode)) {
//
// Do not allow checking for UDF file systems in CDROM "El Torito"
// partitions, and skip duplicate installation of UDF file system child
// nodes.
//
if (DevicePathType (DevicePathNode) == MEDIA_DEVICE_PATH) {
  if (DevicePathSubType (DevicePathNode) == MEDIA_CDROM_DP) {
return EFI_NOT_FOUND;
  }
  if (DevicePathSubType (DevicePathNode) == MEDIA_VENDOR_DP) {
VendorDefinedGuid = (EFI_GUID *)((UINTN)DevicePathNode +
 OFFSET_OF (VENDOR_DEVICE_PATH, Guid));
if (CompareGuid (VendorDefinedGuid, )) {
  return EFI_NOT_FOUND;
}
  }
}
//
// Try next device path node
//
DevicePathNode = NextDevicePathNode (DevicePathNode);
  }

2. Can you add function header comments for the newly added functions?

3. Change the consuming code of IS_UDF_XXX macro after patch #1 is changed.

4. Perhaps to add more checks for these numbers read from the UDF CDROM.
As Jiewen mentioned in another mail, secure code needs to validate all external 
inputs.
We shouldn't assume the UDF CDROM is a well-formatted CDROM.
A hacker may create a UDF CDROM to hack the system by using mechanism like
buffer overflow, integer overflow, etc.


Thanks/Ray

> -Original Message-
> From: Paulo Alcantara [mailto:pca...@zytor.com]
> Sent: Sunday, September 17, 2017 9:13 PM
> To: edk2-devel@lists.01.org
> Cc: Paulo Alcantara ; Dong, Eric ;
> Ni, Ruiyu ; Zeng, Star ; Laszlo
> Ersek 
> Subject: [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF
> logical partition
> 
> Do not use entire block device size for the UDF logical partition, instead
> reserve the appropriate space (LVD space) for it.
> 
> Cc: Eric Dong 
> Cc: Ruiyu Ni 
> Cc: Star Zeng 
> Cc: Laszlo Ersek 
> Reported-by: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara 
> ---
>  MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 323
> ++--
>  1 file changed, 299 insertions(+), 24 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> index 7856b5dfc1..3e84882922 100644
> --- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> +++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
> @@ -84,24 +84,8 @@ FindAnchorVolumeDescriptorPointer (
>return EFI_VOLUME_CORRUPTED;
>  }
> 
> -/**
> -  Check if block device supports a valid UDF file system as specified by OSTA
> -  Universal Disk Format Specification 2.60.
> -
> -  @param[in]   BlockIo  BlockIo interface.
> -  @param[in]   DiskIo   DiskIo interface.
> -
> -  @retval EFI_SUCCESS  UDF file system found.
> -  @retval EFI_UNSUPPORTED  UDF file system not found.
> -  @retval EFI_NO_MEDIA The device has no media.
> -  @retval EFI_DEVICE_ERROR The device reported an error.
> -  @retval EFI_VOLUME_CORRUPTED The file system structures are
> corrupted.
> -  @retval EFI_OUT_OF_RESOURCES The scan was not successful due to lack
> of
> -   resources.
> -
> -**/
>  EFI_STATUS
> -SupportUdfFileSystem (
> +FindUdfVolumeIdentifiers (
>IN EFI_BLOCK_IO_PROTOCOL  *BlockIo,
>IN EFI_DISK_IO_PROTOCOL   *DiskIo
>)
> @@ -111,7 +95,6 @@ SupportUdfFileSystem (
>UINT64EndDiskOffset;
>CDROM_VOLUME_DESCRIPTOR   VolDescriptor;
>CDROM_VOLUME_DESCRIPTOR   TerminatingVolDescriptor;
> -  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  AnchorPoint;
> 
>ZeroMem ((VOID *), sizeof
> (CDROM_VOLUME_DESCRIPTOR));
> 
> @@ -207,12 +190,302 @@ SupportUdfFileSystem (
>  return EFI_UNSUPPORTED;
>}
> 
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +GetPartitionNumber (
> +  IN   UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc,
> +  OUT  UINT16 *PartitionNum
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UDF_LONG_ALLOCATION_DESCRIPTOR  *LongAd;
> +
> +  Status = EFI_SUCCESS;
> +
> +  switch (LV_UDF_REVISION (LogicalVolDesc)) {  case 0x0102:
> +//
> +// UDF 1.20 only supports Type 1 Partition
> +//
> +*PartitionNum = *(UINT16 *)((UINTN)
> >PartitionMaps[4]);
> +break;
> +  case 0x0150:
> +//
> +// Ensure Type 1 Partition map
> +//
> +ASSERT (LogicalVolDesc->PartitionMaps[0] == 1 &&
> +LogicalVolDesc->PartitionMaps[1] == 6);
> +*PartitionNum = *(UINT16 *)((UINTN)
> >PartitionMaps[4]);
> +break;
> +  case 0x0260:
> +LongAd = >LogicalVolumeContentsUse;
> +*PartitionNum = LongAd->ExtentLocation.PartitionReferenceNumber;
> +break;
> 

Re: [edk2] [PATCH v2 1/3] MdePkg: Add UDF volume structure definitions

[Ni, Ruiyu]

Continue reading the patch #2, I think we can change IS_PD to:
#define IS_UDF_PD(Tag) ((Tag)->TagIdentifier == 5)

Using the above way, we can avoid caller to supply an invalid buffer.

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni,
> Ruiyu
> Sent: Monday, September 18, 2017 8:29 AM
> To: Paulo Alcantara ; edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Laszlo Ersek
> ; Gao, Liming 
> Subject: Re: [edk2] [PATCH v2 1/3] MdePkg: Add UDF volume structure
> definitions
> 
> #define _GET_TAG_ID(_Pointer) \
>   (((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)
> 
> #define IS_PD(_Pointer) \
>   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5))
> #define IS_LVD(_Pointer) \
>   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6))
> #define IS_TD(_Pointer) \
>   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))
> 
> #define IS_AVDP(_Pointer) \
>   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))
> 
> Paulo,
> If you take a look at Pci22.h in the same directory, all macros are started
> as "IS_PCI_".
> How about changing the above IS_xxx to IS_UDF_xxx?
> Shall we change AVDP to AVD, following the same naming style as PD, LVD
> and TD?
> How about changing _Pointer to _Tag or DescriptorTag?
> 
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: Paulo Alcantara [mailto:pca...@zytor.com]
> > Sent: Sunday, September 17, 2017 9:13 PM
> > To: edk2-devel@lists.01.org
> > Cc: Paulo Alcantara ; Kinney, Michael D
> > ; Gao, Liming ;
> Laszlo
> > Ersek ; Ni, Ruiyu 
> > Subject: [PATCH v2 1/3] MdePkg: Add UDF volume structure definitions
> >
> > This patch adds a fewe more UDF structures in order to detect Logical
> > Volume and Partition descriptors during Main Volume Descriptor Sequence
> > in Partition driver.
> >
> > Cc: Michael D Kinney 
> > Cc: Liming Gao 
> > Cc: Laszlo Ersek 
> > Cc: Ruiyu Ni 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Paulo Alcantara 
> > ---
> >  MdePkg/Include/IndustryStandard/Udf.h | 63 
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/MdePkg/Include/IndustryStandard/Udf.h
> > b/MdePkg/Include/IndustryStandard/Udf.h
> > index 0febb4bcda..6cc42f8543 100644
> > --- a/MdePkg/Include/IndustryStandard/Udf.h
> > +++ b/MdePkg/Include/IndustryStandard/Udf.h
> > @@ -27,9 +27,19 @@
> >  #define _GET_TAG_ID(_Pointer) \
> >(((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)
> >
> > +#define IS_PD(_Pointer) \
> > +  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5)) #define IS_LVD(_Pointer) \
> > +  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6)) #define IS_TD(_Pointer) \
> > +  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))
> > +
> >  #define IS_AVDP(_Pointer) \
> >((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))
> >
> > +#define LV_UDF_REVISION(_Lv) \
> > +  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
> > +
> >  #pragma pack(1)
> >
> >  typedef struct {
> > @@ -55,6 +65,59 @@ typedef struct {
> >UINT8   Reserved[480];
> >  } UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER;
> >
> > +typedef struct {
> > +  UINT8   CharacterSetType;
> > +  UINT8   CharacterSetInfo[63];
> > +} UDF_CHAR_SPEC;
> > +
> > +typedef struct {
> > +  UINT8   Flags;
> > +  UINT8   Identifier[23];
> > +  UINT8   IdentifierSuffix[8];
> > +} UDF_ENTITY_ID;
> > +
> > +typedef struct {
> > +  UINT32LogicalBlockNumber;
> > +  UINT16PartitionReferenceNumber;
> > +} UDF_LB_ADDR;
> > +
> > +typedef struct {
> > +  UINT32   ExtentLength;
> > +  UDF_LB_ADDR  ExtentLocation;
> > +  UINT8ImplementationUse[6];
> > +} UDF_LONG_ALLOCATION_DESCRIPTOR;
> > +
> > +typedef struct {
> > +  UDF_DESCRIPTOR_TAG  DescriptorTag;
> > +  UINT32  VolumeDescriptorSequenceNumber;
> > +  UDF_CHAR_SPEC   DescriptorCharacterSet;
> > +  UINT8   LogicalVolumeIdentifier[128];
> > +  UINT32  LogicalBlockSize;
> > +  UDF_ENTITY_ID   DomainIdentifier;
> > +  UDF_LONG_ALLOCATION_DESCRIPTOR  LogicalVolumeContentsUse;
> > +  UINT32  MapTableLength;
> > +  UINT32  NumberOfPartitionMaps;
> > +  UDF_ENTITY_ID   ImplementationIdentifier;
> > +  UINT8   ImplementationUse[128];
> > +  UDF_EXTENT_AD   IntegritySequenceExtent;
> > +  UINT8   PartitionMaps[6];
> > +} UDF_LOGICAL_VOLUME_DESCRIPTOR;
> > +
> > +typedef struct {
> > +  UDF_DESCRIPTOR_TAG DescriptorTag;
> > +  UINT32 

Re: [edk2] [PATCH v2 1/3] MdePkg: Add UDF volume structure definitions

[Ni, Ruiyu]

#define _GET_TAG_ID(_Pointer) \
  (((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)

#define IS_PD(_Pointer) \
  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5))
#define IS_LVD(_Pointer) \
  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6))
#define IS_TD(_Pointer) \
  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))

#define IS_AVDP(_Pointer) \
  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))

Paulo,
If you take a look at Pci22.h in the same directory, all macros are started
as "IS_PCI_".
How about changing the above IS_xxx to IS_UDF_xxx?
Shall we change AVDP to AVD, following the same naming style as PD, LVD and TD?
How about changing _Pointer to _Tag or DescriptorTag?


Thanks/Ray

> -Original Message-
> From: Paulo Alcantara [mailto:pca...@zytor.com]
> Sent: Sunday, September 17, 2017 9:13 PM
> To: edk2-devel@lists.01.org
> Cc: Paulo Alcantara ; Kinney, Michael D
> ; Gao, Liming ; Laszlo
> Ersek ; Ni, Ruiyu 
> Subject: [PATCH v2 1/3] MdePkg: Add UDF volume structure definitions
> 
> This patch adds a fewe more UDF structures in order to detect Logical
> Volume and Partition descriptors during Main Volume Descriptor Sequence
> in Partition driver.
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Laszlo Ersek 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara 
> ---
>  MdePkg/Include/IndustryStandard/Udf.h | 63 
>  1 file changed, 63 insertions(+)
> 
> diff --git a/MdePkg/Include/IndustryStandard/Udf.h
> b/MdePkg/Include/IndustryStandard/Udf.h
> index 0febb4bcda..6cc42f8543 100644
> --- a/MdePkg/Include/IndustryStandard/Udf.h
> +++ b/MdePkg/Include/IndustryStandard/Udf.h
> @@ -27,9 +27,19 @@
>  #define _GET_TAG_ID(_Pointer) \
>(((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)
> 
> +#define IS_PD(_Pointer) \
> +  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5)) #define IS_LVD(_Pointer) \
> +  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6)) #define IS_TD(_Pointer) \
> +  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))
> +
>  #define IS_AVDP(_Pointer) \
>((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))
> 
> +#define LV_UDF_REVISION(_Lv) \
> +  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
> +
>  #pragma pack(1)
> 
>  typedef struct {
> @@ -55,6 +65,59 @@ typedef struct {
>UINT8   Reserved[480];
>  } UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER;
> 
> +typedef struct {
> +  UINT8   CharacterSetType;
> +  UINT8   CharacterSetInfo[63];
> +} UDF_CHAR_SPEC;
> +
> +typedef struct {
> +  UINT8   Flags;
> +  UINT8   Identifier[23];
> +  UINT8   IdentifierSuffix[8];
> +} UDF_ENTITY_ID;
> +
> +typedef struct {
> +  UINT32LogicalBlockNumber;
> +  UINT16PartitionReferenceNumber;
> +} UDF_LB_ADDR;
> +
> +typedef struct {
> +  UINT32   ExtentLength;
> +  UDF_LB_ADDR  ExtentLocation;
> +  UINT8ImplementationUse[6];
> +} UDF_LONG_ALLOCATION_DESCRIPTOR;
> +
> +typedef struct {
> +  UDF_DESCRIPTOR_TAG  DescriptorTag;
> +  UINT32  VolumeDescriptorSequenceNumber;
> +  UDF_CHAR_SPEC   DescriptorCharacterSet;
> +  UINT8   LogicalVolumeIdentifier[128];
> +  UINT32  LogicalBlockSize;
> +  UDF_ENTITY_ID   DomainIdentifier;
> +  UDF_LONG_ALLOCATION_DESCRIPTOR  LogicalVolumeContentsUse;
> +  UINT32  MapTableLength;
> +  UINT32  NumberOfPartitionMaps;
> +  UDF_ENTITY_ID   ImplementationIdentifier;
> +  UINT8   ImplementationUse[128];
> +  UDF_EXTENT_AD   IntegritySequenceExtent;
> +  UINT8   PartitionMaps[6];
> +} UDF_LOGICAL_VOLUME_DESCRIPTOR;
> +
> +typedef struct {
> +  UDF_DESCRIPTOR_TAG DescriptorTag;
> +  UINT32 VolumeDescriptorSequenceNumber;
> +  UINT16 PartitionFlags;
> +  UINT16 PartitionNumber;
> +  UDF_ENTITY_ID  PartitionContents;
> +  UINT8  PartitionContentsUse[128];
> +  UINT32 AccessType;
> +  UINT32 PartitionStartingLocation;
> +  UINT32 PartitionLength;
> +  UDF_ENTITY_ID  ImplementationIdentifier;
> +  UINT8  ImplementationUse[128];
> +  UINT8  Reserved[156];
> +} UDF_PARTITION_DESCRIPTOR;
> +
>  #pragma pack()
> 
>  #endif
> --
> 2.11.0

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

Re: [edk2] [PATCH 0/3] UDF partition driver fix

[Paulo Alcantara]

Hi Jiewen,

On 16/09/2017 20:52, Yao, Jiewen wrote:

Thank you Paulo, to provide a fix for this driver.


No problem! I'm trying to do the best I can in my very short time.



I do not have comment for this specific patch. I would defer the review 
work to Star and Ruiyu.


OK.



I do have some general question for the new UDF support and I would like 
to know more detail about the quality level.


As we are seeing some issues in the new UDF driver, would you please 
share what test you have done for the UDF support? (Not only for this 
patch, but also for the UDF partition and UDF file system which are 
already checked in)


I tested it with a Windows 10 Enterprise ISO image (UDF bridge disk 
image). With it, I could test if ElTorito boot still worked, as well as 
if I could list directory and files, print out file contents, etc. in 
its UDF file system.


I also used a 32GiB USB stick by formatting it with 'mkudffs -b 512 
--media-type=hd /dev/sdX' and copied some files to it and performed some 
basic file operations like listening and reading files/symlinks. I built 
a Linux kernel with EFI stub support inside the UDF file system -- it 
booted fine, however the kernel wasn't able to mount the rootfs being an 
UDF file system.


I tried some Fedora images I have but they aren't UDF bridge disk images 
(e.g. only ISO9660 + ElTorito), so couldn't test with them.


There's also a great tool I used to validate the ISO images and see if 
they are complaint with UDF specification: "Philips UDF Conformance 
Tool". You may find it at 
https://www.lscdweb.com/registered/udf_verifier.html




I ask this specially, because UDF support (partition and file system) 
adds a brand new group of external input for the UEFI BIOS. For a long 
time, we are monitoring all the external input.


Yeah, you're right.



Per our security model, the external input means the input by an end 
user. The known external includes but not limited to UEFI image (OROM/OS 
Loader), Capsule Image, Recovery Image, file system, partition, network 
packet, variable, etc.


Good to know!



UDF is a new one, because with the new UDF support, now a malicious user 
may insert a mal-format UDF CDROM to the system and try to break the 
system. As such, we need evaluate it.


Sure!



To be specific, would you please share:

1)Which UDF spec these 2 drivers (FS and partition) are following?


I'm following the UDF 2.60 specification.



I have seen you mentioned the header file follows “(revisions 1.02 
through 2.60)”. But I am not sure about the driver.


I found you mentioned:

Originally the driver was written to support UDF file systems as

specified by OSTA Universal Disk Format Specification 2.60. However,

some Windows 10 Enterprise ISO (UDF bridge) images that I tested

supported a revision of 1.02 thus I had to rework the driver a little

bit to support such revision as well.

Do you mean you only support 1.02 and 2.6 in driver, or you support 1.02 
through 2.6?


2)Which UDF function is supported? And more important, which UDF 
function is NOT supported?


The partition + file system drivers should support only 2.60. However, 
as you can see in this patchset, there is a GetPartitionNumber() in 
PartitionDxe, which checks for the UDF revision (1.02 through 2.60) in 
order to retrieve the correct partition number and Partition Descriptor.


For instance, the Windows 10 Enterprise ISO image, by running it with 
the "Philips UDF Conformance Tool", it says "Final UDF Revision range: 
1.02 only", but partition + driver works fine with it because of that 
revision check in GetPartitionNumber() in PartitionDxe.


Anyways, it's UDF 2.60 revision, officially.



I have seen “Compliance” section in UDF spec, and it lists some optional 
feature, such as multi-volume, multi-partition, multisession, file name 
translation, backward read, backward write, etc. >


I also have interest to know the support level of current existing 
CDROM, and existing UDF driver in OS (such as Windows, or Linux). How 
many optional feature are implemented?


None optional feature has been implemented. Only the mandatory ones.

I will list some known limitations that I could remember:

- Partition types other than 1, will not be supported.

- Extended Allocation Descriptors are not supported. However, the spec 
 mentions: "Only Short and Long Allocation Descriptors shall be 
recorded". IIRC, the 'mkudffs' tool in Linux, it may create Extended 
Allocation Descriptors in directories with thousands of files, and the 
UDF conformance tool reports that the UDF file system created by the 
tool is complaint with UDF revision 2.60.


- Only one Logical Volume and Partition Descriptor is supported. See "2. 
Basic Restrictions & Requirements" in UDF 2.60 specification.

- No write support.

There may be other unsupported features I forgot to mention. Sorry. I 
also need to read the unfriendly ECMA-167 and UDF specs again and check 
the remaining unsupported features.




I ask this, 

Re: [edk2] [PATCH 0/3] UDF partition driver fix

[Yao, Jiewen]

Thank you Laszlo. You analysis is great!

Thank you
Yao Jiewen

From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
Ersek
Sent: Sunday, September 17, 2017 6:07 PM
To: Yao, Jiewen ; Paulo Alcantara 
Cc: Ni, Ruiyu ; Dong, Eric ; 
edk2-devel@lists.01.org; Gao, Liming ; Kinney, Michael D 
; Zeng, Star 
Subject: Re: [edk2] [PATCH 0/3] UDF partition driver fix

Hi Jiewen,

I agree these are important questions; even earlier I noticed the
following remark in "PartitionDxe.inf":

#  Caution: This module requires additional review when modified.
#  This driver will have external input - disk partition.
#  This external input must be validated carefully to avoid security issue like
#  buffer overflow, integer overflow.

I'll let Paulo answer your questions.

I'll just comment on one thing because your specific question refers to
code that I recommended.

On 09/17/17 01:52, Yao, Jiewen wrote:

> 4)   The last but not least important, which negative test
> (security test) has been done?
> Have you run some fuzzing test?
> Have you tried a mal-format UDF disc? For example, with bad offset or
> length?
> Have you test the integer overflow / buffer over flow handing in the
> code?
>
> NOTE: we should not use ASSERT to handle such error.
> When I review the code, I found below:
>
> Status = ReadFileData (
>   BlockIo,
>   DiskIo,
>   Volume,
>   Parent,
>   PrivFileData->FileSize,
>   >FilePosition,
>   Buffer,
>   
>   );
> ASSERT (BufferSizeUint64 <= MAX_UINTN);
> *BufferSize = (UINTN)BufferSizeUint64;
>
> I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
> Can a malicious user construct a bad UDF and make BufferSizeUint64 >
> MAX_UINTN?
> Does it might happen? Or never happen?
>
> We documented Appendix B - EDKII code review top 5 in
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
> 3 of them are matched in these partition and file system drivers. I
> quote below:
> ===
> If the code consumes input from an untrusted source or region,
> Ensure that the input is rigorously and adequately validated.
> Verify buffer overflow is handled. Avoid integer overflow.
> Try to use subtraction instead of addition and division instead of
> multiplication.
> Verify that ASSERT is used properly.
> ASSERT is used to catch conditions that should never happen. If
> something might happen, use error handling instead. We can use both
> ASSERT and error handling to facilitate debugging - ASSERT allows for
> earlier detection and isolation of several classes of issues.
> [McConnell]
> ===

You can find the discussion about the code above here:

8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com">http://mid.mail-archive.com/8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com

I can describe it in more detail here:

The UdfRead() function, which implements EFI_FILE_PROTOCOL.Read(), takes
an IN OUT parameter called BufferSize:

typedef
EFI_STATUS
(EFIAPI *EFI_FILE_READ)(
  IN EFI_FILE_PROTOCOL*This,
  IN OUT UINTN*BufferSize,
  OUT VOID*Buffer
  );

BufferSize points to an UINTN variable. On input BufferSize says how
much data the caller is trying to read, and on output it tells the
caller how much data was actually read.

In the UdfRead() function, we pass the pointer to a helper function
called ReadFileData(). ReadFileData() takes a similar IN OUT BufferSize
parameter, but in ReadFileData() the pointer is to UINT64, not UINTN:

EFI_STATUS
ReadFileData (
  IN  EFI_BLOCK_IO_PROTOCOL  *BlockIo,
  IN  EFI_DISK_IO_PROTOCOL   *DiskIo,
  IN  UDF_VOLUME_INFO*Volume,
  IN  UDF_FILE_INFO  *File,
  IN  UINT64 FileSize,
  IN OUT  UINT64 *FilePosition,
  IN OUT  VOID   *Buffer,
  IN OUT  UINT64 *BufferSize
  )

Therefore we cannot pass the original pointer directly, because in
32-bit builds, ReadFileData() would access 64 bits through the pointer,
even though the caller of UdfRead() originally allocated only 32 bits
for the outermost BufferSize variable.

Therefore, in UdfRead(), we have a local variable

  UINT64  BufferSizeUint64;

and we use it like this:

BufferSizeUint64 = *BufferSize;

Status = ReadFileData (
  BlockIo,
  DiskIo,
  Volume,
  Parent,
  PrivFileData->FileSize,
  >FilePosition,
  Buffer,
  
  );
ASSERT (BufferSizeUint64 <= MAX_UINTN);
*BufferSize = (UINTN)BufferSizeUint64;

First we set the helper variable to *BufferSize, from the caller. In
64-bit builds, UINTN is UINT64, hence this is a UINT64-to-UINT64
assignment. In 32-bit builds, UINTN is UINT32, 

[edk2] [PATCH v2 2/3] MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition

[Paulo Alcantara]

Do not use entire block device size for the UDF logical partition,
instead reserve the appropriate space (LVD space) for it.

Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Reported-by: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara 
---
 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c | 323 ++--
 1 file changed, 299 insertions(+), 24 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c 
b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
index 7856b5dfc1..3e84882922 100644
--- a/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
+++ b/MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c
@@ -84,24 +84,8 @@ FindAnchorVolumeDescriptorPointer (
   return EFI_VOLUME_CORRUPTED;
 }
 
-/**
-  Check if block device supports a valid UDF file system as specified by OSTA
-  Universal Disk Format Specification 2.60.
-
-  @param[in]   BlockIo  BlockIo interface.
-  @param[in]   DiskIo   DiskIo interface.
-
-  @retval EFI_SUCCESS  UDF file system found.
-  @retval EFI_UNSUPPORTED  UDF file system not found.
-  @retval EFI_NO_MEDIA The device has no media.
-  @retval EFI_DEVICE_ERROR The device reported an error.
-  @retval EFI_VOLUME_CORRUPTED The file system structures are corrupted.
-  @retval EFI_OUT_OF_RESOURCES The scan was not successful due to lack of
-   resources.
-
-**/
 EFI_STATUS
-SupportUdfFileSystem (
+FindUdfVolumeIdentifiers (
   IN EFI_BLOCK_IO_PROTOCOL  *BlockIo,
   IN EFI_DISK_IO_PROTOCOL   *DiskIo
   )
@@ -111,7 +95,6 @@ SupportUdfFileSystem (
   UINT64EndDiskOffset;
   CDROM_VOLUME_DESCRIPTOR   VolDescriptor;
   CDROM_VOLUME_DESCRIPTOR   TerminatingVolDescriptor;
-  UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  AnchorPoint;
 
   ZeroMem ((VOID *), sizeof 
(CDROM_VOLUME_DESCRIPTOR));
 
@@ -207,12 +190,302 @@ SupportUdfFileSystem (
 return EFI_UNSUPPORTED;
   }
 
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+GetPartitionNumber (
+  IN   UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc,
+  OUT  UINT16 *PartitionNum
+  )
+{
+  EFI_STATUS  Status;
+  UDF_LONG_ALLOCATION_DESCRIPTOR  *LongAd;
+
+  Status = EFI_SUCCESS;
+
+  switch (LV_UDF_REVISION (LogicalVolDesc)) {
+  case 0x0102:
+//
+// UDF 1.20 only supports Type 1 Partition
+//
+*PartitionNum = *(UINT16 *)((UINTN)>PartitionMaps[4]);
+break;
+  case 0x0150:
+//
+// Ensure Type 1 Partition map
+//
+ASSERT (LogicalVolDesc->PartitionMaps[0] == 1 &&
+LogicalVolDesc->PartitionMaps[1] == 6);
+*PartitionNum = *(UINT16 *)((UINTN)>PartitionMaps[4]);
+break;
+  case 0x0260:
+LongAd = >LogicalVolumeContentsUse;
+*PartitionNum = LongAd->ExtentLocation.PartitionReferenceNumber;
+break;
+  default:
+//
+// Unhandled UDF revision
+//
+Status = EFI_VOLUME_CORRUPTED;
+break;
+  }
+
+  return Status;
+}
+
+EFI_STATUS
+FindLogicalVolumeLocation (
+  IN   EFI_BLOCK_IO_PROTOCOL *BlockIo,
+  IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
+  IN   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
+  OUT  UINT64*MainVdsStartLsn,
+  OUT  UINT64*LogicalVolEndLsn
+  )
+{
+  EFI_STATUS Status;
+  UINT32 BlockSize;
+  EFI_LBALastBlock;
+  UDF_EXTENT_AD  *ExtentAd;
+  UINT64 StartingLsn;
+  UINT64 EndingLsn;
+  VOID   *Buffer;
+  UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
+  UDF_PARTITION_DESCRIPTOR   *PartitionDesc;
+  UINT64 GuardMainVdsStartLsn;
+  UINT16 PartitionNum;
+
+  BlockSize = BlockIo->Media->BlockSize;
+  LastBlock = BlockIo->Media->LastBlock;
+  ExtentAd  = >MainVolumeDescriptorSequenceExtent;
+  StartingLsn   = (UINT64)ExtentAd->ExtentLocation;
+  EndingLsn =
+StartingLsn + DivU64x32 ((UINT64)ExtentAd->ExtentLength, BlockSize);
+
+  LogicalVolDesc= NULL;
+  PartitionDesc = NULL;
+  GuardMainVdsStartLsn  = StartingLsn;
+
+  //
+  // Allocate buffer for reading disk blocks
+  //
+  Buffer = AllocateZeroPool (BlockSize);
+  if (Buffer == NULL) {
+return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = EFI_VOLUME_CORRUPTED;
+
+  //
+  // As per UDF 2.60 specification:
+  //
+  // There shall be exactly one prevailing Logical Volume Descriptor
+  // recorded per Volume Set.
+  //
+  // Start Main Volume Descriptor Sequence and find Logical Volume Descriptor
+  //
+  while (StartingLsn <= EndingLsn) {
+//
+// Read disk block
+//
+Status = DiskIo->ReadDisk (
+  

[edk2] [PATCH v2 0/3] UDF partition driver fix

[Paulo Alcantara]

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

Hi,

This patchset fixes a bug in Partition driver that UDF partitions
occupied the entire disk space instead of using LVD space only.

BTW, I've only tested it under OVMF and built it with GCC only. That
would be appreciable if someone could build with other toolchains and
see if this doesn't break.

I used a Windows 10 ISO image with UdfDxe disabled and enabled. The
`map -r` output seemed OK. No breakage when booting an OS off an
ElTorito partition from an UDF bridge disk.

v1->v2:
 - Followed Laszlo's suggestions to submit a proper patchset. Thanks!
 - As I'm still waiting for Ruiyu and Star to test this fix, I took
   advantage of it and did some code cleanups :-)

Repo:   https://github.com/pcacjr/edk2.git
Branch: udf-partition-fix-v2

Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Reported-by: Ruiyu Ni 
Signed-off-by: Paulo Alcantara 
---

Paulo Alcantara (3):
  MdePkg: Add UDF volume structure definitions
  MdeModulePkg/PartitionDxe: Fix creation of UDF logical partition
  MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes

 MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c| 323 +++-
 MdeModulePkg/Universal/Disk/UdfDxe/File.c |  13 +-
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 515 

 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c  |   7 -
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h  |  88 +---
 MdePkg/Include/IndustryStandard/Udf.h |  63 +++
 6 files changed, 566 insertions(+), 443 deletions(-)

-- 
2.11.0

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

[edk2] [PATCH v2 3/3] MdeModulePkg/UdfDxe: Rework driver to support PartitionDxe changes

[Paulo Alcantara]

This patch reworks the driver to support Partition driver changes.

Cc: Eric Dong 
Cc: Paulo Alcantara 
Cc: Ruiyu Ni 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara 
---
 MdeModulePkg/Universal/Disk/UdfDxe/File.c |  13 +-
 MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 515 

 MdeModulePkg/Universal/Disk/UdfDxe/Udf.c  |   7 -
 MdeModulePkg/Universal/Disk/UdfDxe/Udf.h  |  88 +---
 4 files changed, 204 insertions(+), 419 deletions(-)

diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/File.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
index 01361141bb..f2c62967e8 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/File.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/File.c
@@ -100,6 +100,7 @@ UdfOpenVolume (
 >Volume,
 >Root
 );
+  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
 goto Error_Find_Root_Dir;
   }
@@ -131,7 +132,6 @@ Error_Alloc_Priv_File_Data:
   CleanupFileInformation (>Root);
 
 Error_Find_Root_Dir:
-  CleanupVolumeInformation (>Volume);
 
 Error_Read_Udf_Volume:
 Error_Invalid_Params:
@@ -528,7 +528,6 @@ UdfClose (
   EFI_TPL OldTpl;
   EFI_STATUS  Status;
   PRIVATE_UDF_FILE_DATA   *PrivFileData;
-  PRIVATE_UDF_SIMPLE_FS_DATA  *PrivFsData;
 
   OldTpl = gBS->RaiseTPL (TPL_CALLBACK);
 
@@ -541,8 +540,6 @@ UdfClose (
 
   PrivFileData = PRIVATE_UDF_FILE_DATA_FROM_THIS (This);
 
-  PrivFsData = PRIVATE_UDF_SIMPLE_FS_DATA_FROM_THIS (PrivFileData->SimpleFs);
-
   if (!PrivFileData->IsRootDirectory) {
 CleanupFileInformation (>File);
 
@@ -551,10 +548,6 @@ UdfClose (
 }
   }
 
-  if (--PrivFsData->OpenFiles == 0) {
-CleanupVolumeInformation (>Volume);
-  }
-
   FreePool ((VOID *)PrivFileData);
 
 Exit:
@@ -787,7 +780,7 @@ UdfGetInfo (
   } else if (CompareGuid (InformationType, )) {
 String = VolumeLabel;
 
-FileSetDesc = PrivFsData->Volume.FileSetDescs[0];
+FileSetDesc = >Volume.FileSetDesc;
 
 OstaCompressed = >LogicalVolumeIdentifier[0];
 
@@ -846,7 +839,7 @@ UdfGetInfo (
 FileSystemInfo->Size= FileSystemInfoLength;
 FileSystemInfo->ReadOnly= TRUE;
 FileSystemInfo->BlockSize   =
-  LV_BLOCK_SIZE (>Volume, UDF_DEFAULT_LV_NUM);
+  PrivFsData->Volume.LogicalVolDesc.LogicalBlockSize;
 FileSystemInfo->VolumeSize  = VolumeSize;
 FileSystemInfo->FreeSpace   = FreeSpaceSize;
 
diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c 
b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
index 4609580b30..63b643e60a 100644
--- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
+++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c
@@ -60,154 +60,111 @@ FindAnchorVolumeDescriptorPointer (
 
 EFI_STATUS
 StartMainVolumeDescriptorSequence (
-  IN   EFI_BLOCK_IO_PROTOCOL *BlockIo,
-  IN   EFI_DISK_IO_PROTOCOL  *DiskIo,
-  IN   UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER  *AnchorPoint,
-  OUT  UDF_VOLUME_INFO   *Volume
+  IN   EFI_BLOCK_IO_PROTOCOL*BlockIo,
+  IN   EFI_DISK_IO_PROTOCOL *DiskIo,
+  OUT  UDF_VOLUME_INFO  *Volume
   )
 {
-  EFI_STATUS Status;
-  UINT32 BlockSize;
-  UDF_EXTENT_AD  *ExtentAd;
-  UINT64 StartingLsn;
-  UINT64 EndingLsn;
-  VOID   *Buffer;
-  UDF_LOGICAL_VOLUME_DESCRIPTOR  *LogicalVolDesc;
-  UDF_PARTITION_DESCRIPTOR   *PartitionDesc;
-  UINTN  Index;
-  UINT32 LogicalBlockSize;
+  EFI_STATUS  Status;
+  UINT32  BlockSize;
+  UINT64  BlockOffset;
+  VOID*Buffer;
+  UINT32  LogicalBlockSize;
+
+  BlockSize = BlockIo->Media->BlockSize;
 
   //
-  // We've already found an ADVP on the volume. It contains the extent
-  // (MainVolumeDescriptorSequenceExtent) where the Main Volume Descriptor
-  // Sequence starts. Therefore, we'll look for Logical Volume Descriptors and
-  // Partitions Descriptors and save them in memory, accordingly.
-  //
-  // Note also that each descriptor will be aligned on a block size (BlockSize)
-  // boundary, so we need to read one block at a time.
+  // Allocate buffer for reading disk blocks
   //
-  BlockSize= BlockIo->Media->BlockSize;
-  ExtentAd = >MainVolumeDescriptorSequenceExtent;
-  StartingLsn  = (UINT64)ExtentAd->ExtentLocation;
-  EndingLsn= StartingLsn + DivU64x32 (
- (UINT64)ExtentAd->ExtentLength,
- BlockSize
- );
-
-  Volume->LogicalVolDescs =
-(UDF_LOGICAL_VOLUME_DESCRIPTOR **)AllocateZeroPool 
(ExtentAd->ExtentLength);
-  if 

[edk2] [PATCH v2 1/3] MdePkg: Add UDF volume structure definitions

[Paulo Alcantara]

This patch adds a fewe more UDF structures in order to detect Logical
Volume and Partition descriptors during Main Volume Descriptor Sequence
in Partition driver.

Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Paulo Alcantara 
---
 MdePkg/Include/IndustryStandard/Udf.h | 63 
 1 file changed, 63 insertions(+)

diff --git a/MdePkg/Include/IndustryStandard/Udf.h 
b/MdePkg/Include/IndustryStandard/Udf.h
index 0febb4bcda..6cc42f8543 100644
--- a/MdePkg/Include/IndustryStandard/Udf.h
+++ b/MdePkg/Include/IndustryStandard/Udf.h
@@ -27,9 +27,19 @@
 #define _GET_TAG_ID(_Pointer) \
   (((UDF_DESCRIPTOR_TAG *)(_Pointer))->TagIdentifier)
 
+#define IS_PD(_Pointer) \
+  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 5))
+#define IS_LVD(_Pointer) \
+  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 6))
+#define IS_TD(_Pointer) \
+  ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 8))
+
 #define IS_AVDP(_Pointer) \
   ((BOOLEAN)(_GET_TAG_ID (_Pointer) == 2))
 
+#define LV_UDF_REVISION(_Lv) \
+  *(UINT16 *)(UINTN)(_Lv)->DomainIdentifier.IdentifierSuffix
+
 #pragma pack(1)
 
 typedef struct {
@@ -55,6 +65,59 @@ typedef struct {
   UINT8   Reserved[480];
 } UDF_ANCHOR_VOLUME_DESCRIPTOR_POINTER;
 
+typedef struct {
+  UINT8   CharacterSetType;
+  UINT8   CharacterSetInfo[63];
+} UDF_CHAR_SPEC;
+
+typedef struct {
+  UINT8   Flags;
+  UINT8   Identifier[23];
+  UINT8   IdentifierSuffix[8];
+} UDF_ENTITY_ID;
+
+typedef struct {
+  UINT32LogicalBlockNumber;
+  UINT16PartitionReferenceNumber;
+} UDF_LB_ADDR;
+
+typedef struct {
+  UINT32   ExtentLength;
+  UDF_LB_ADDR  ExtentLocation;
+  UINT8ImplementationUse[6];
+} UDF_LONG_ALLOCATION_DESCRIPTOR;
+
+typedef struct {
+  UDF_DESCRIPTOR_TAG  DescriptorTag;
+  UINT32  VolumeDescriptorSequenceNumber;
+  UDF_CHAR_SPEC   DescriptorCharacterSet;
+  UINT8   LogicalVolumeIdentifier[128];
+  UINT32  LogicalBlockSize;
+  UDF_ENTITY_ID   DomainIdentifier;
+  UDF_LONG_ALLOCATION_DESCRIPTOR  LogicalVolumeContentsUse;
+  UINT32  MapTableLength;
+  UINT32  NumberOfPartitionMaps;
+  UDF_ENTITY_ID   ImplementationIdentifier;
+  UINT8   ImplementationUse[128];
+  UDF_EXTENT_AD   IntegritySequenceExtent;
+  UINT8   PartitionMaps[6];
+} UDF_LOGICAL_VOLUME_DESCRIPTOR;
+
+typedef struct {
+  UDF_DESCRIPTOR_TAG DescriptorTag;
+  UINT32 VolumeDescriptorSequenceNumber;
+  UINT16 PartitionFlags;
+  UINT16 PartitionNumber;
+  UDF_ENTITY_ID  PartitionContents;
+  UINT8  PartitionContentsUse[128];
+  UINT32 AccessType;
+  UINT32 PartitionStartingLocation;
+  UINT32 PartitionLength;
+  UDF_ENTITY_ID  ImplementationIdentifier;
+  UINT8  ImplementationUse[128];
+  UINT8  Reserved[156];
+} UDF_PARTITION_DESCRIPTOR;
+
 #pragma pack()
 
 #endif
-- 
2.11.0

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

Re: [edk2] [PATCH 0/3] UDF partition driver fix

[Laszlo Ersek]

Hi Jiewen,

I agree these are important questions; even earlier I noticed the
following remark in "PartitionDxe.inf":

#  Caution: This module requires additional review when modified.
#  This driver will have external input - disk partition.
#  This external input must be validated carefully to avoid security issue like
#  buffer overflow, integer overflow.

I'll let Paulo answer your questions.

I'll just comment on one thing because your specific question refers to
code that I recommended.

On 09/17/17 01:52, Yao, Jiewen wrote:

> 4)   The last but not least important, which negative test
> (security test) has been done?
> Have you run some fuzzing test?
> Have you tried a mal-format UDF disc? For example, with bad offset or
> length?
> Have you test the integer overflow / buffer over flow handing in the
> code?
>
> NOTE: we should not use ASSERT to handle such error.
> When I review the code, I found below:
>
> Status = ReadFileData (
>   BlockIo,
>   DiskIo,
>   Volume,
>   Parent,
>   PrivFileData->FileSize,
>   >FilePosition,
>   Buffer,
>   
>   );
> ASSERT (BufferSizeUint64 <= MAX_UINTN);
> *BufferSize = (UINTN)BufferSizeUint64;
>
> I am not sure if we can use ASSERT (BufferSizeUint64 <= MAX_UINTN);
> Can a malicious user construct a bad UDF and make BufferSizeUint64 >
> MAX_UINTN?
> Does it might happen? Or never happen?
>
> We documented Appendix B - EDKII code review top 5 in
> https://github.com/tianocore-docs/Docs/raw/master/White_Papers/A_Tour_Beyond_BIOS_Security_Design_Guide_in_EDK_II.pdf
> 3 of them are matched in these partition and file system drivers. I
> quote below:
> ===
> If the code consumes input from an untrusted source or region,
> Ensure that the input is rigorously and adequately validated.
> Verify buffer overflow is handled. Avoid integer overflow.
> Try to use subtraction instead of addition and division instead of
> multiplication.
> Verify that ASSERT is used properly.
> ASSERT is used to catch conditions that should never happen. If
> something might happen, use error handling instead. We can use both
> ASSERT and error handling to facilitate debugging - ASSERT allows for
> earlier detection and isolation of several classes of issues.
> [McConnell]
> ===

You can find the discussion about the code above here:

8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com">http://mid.mail-archive.com/8ca69ac9-4f5a-3aa9-f150-844b0aeeb898@redhat.com

I can describe it in more detail here:

The UdfRead() function, which implements EFI_FILE_PROTOCOL.Read(), takes
an IN OUT parameter called BufferSize:

typedef
EFI_STATUS
(EFIAPI *EFI_FILE_READ)(
  IN EFI_FILE_PROTOCOL*This,
  IN OUT UINTN*BufferSize,
  OUT VOID*Buffer
  );

BufferSize points to an UINTN variable. On input BufferSize says how
much data the caller is trying to read, and on output it tells the
caller how much data was actually read.

In the UdfRead() function, we pass the pointer to a helper function
called ReadFileData(). ReadFileData() takes a similar IN OUT BufferSize
parameter, but in ReadFileData() the pointer is to UINT64, not UINTN:

EFI_STATUS
ReadFileData (
  IN  EFI_BLOCK_IO_PROTOCOL  *BlockIo,
  IN  EFI_DISK_IO_PROTOCOL   *DiskIo,
  IN  UDF_VOLUME_INFO*Volume,
  IN  UDF_FILE_INFO  *File,
  IN  UINT64 FileSize,
  IN OUT  UINT64 *FilePosition,
  IN OUT  VOID   *Buffer,
  IN OUT  UINT64 *BufferSize
  )

Therefore we cannot pass the original pointer directly, because in
32-bit builds, ReadFileData() would access 64 bits through the pointer,
even though the caller of UdfRead() originally allocated only 32 bits
for the outermost BufferSize variable.

Therefore, in UdfRead(), we have a local variable

  UINT64  BufferSizeUint64;

and we use it like this:

BufferSizeUint64 = *BufferSize;

Status = ReadFileData (
  BlockIo,
  DiskIo,
  Volume,
  Parent,
  PrivFileData->FileSize,
  >FilePosition,
  Buffer,
  
  );
ASSERT (BufferSizeUint64 <= MAX_UINTN);
*BufferSize = (UINTN)BufferSizeUint64;

First we set the helper variable to *BufferSize, from the caller. In
64-bit builds, UINTN is UINT64, hence this is a UINT64-to-UINT64
assignment. In 32-bit builds, UINTN is UINT32, hence this is a
UINT32-to-UINT64 assignment.

Then we call ReadFileData() with a pointer to the helper variable. This
is safe because the helper variable has enough room (64 bits) so that
ReadFileData() will not access data beyond it.

Finally, we must put back the result from BufferSizeUint64, to the
caller's (*BufferSize) object. In 64-bit builds, this is a
UINT64-to-UINT64 assignment, so that is safe. However, in 32-bit builds,
this is a UINT64-to-UINT32 assignment, and we must prevent the value
from being truncated.

The insight 

[edk2] [PATCH 1/3] IntelSiliconPkg/VTdInfoPpi: Let it follow DMAR table.

[Jiewen Yao]

We notice that there is real usage in PEI to show
the graphic output. As such we need report RMRR table
in PEI to let VTdPmrPei driver skip the IGD UMA region.

Now the VTD_INFO PPI uses the same DMAR data structure.

Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao 
---
 IntelSiliconPkg/Include/Ppi/VtdInfo.h | 26 +++-
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/IntelSiliconPkg/Include/Ppi/VtdInfo.h 
b/IntelSiliconPkg/Include/Ppi/VtdInfo.h
index e8be63f..a0a6f9c 100644
--- a/IntelSiliconPkg/Include/Ppi/VtdInfo.h
+++ b/IntelSiliconPkg/Include/Ppi/VtdInfo.h
@@ -17,22 +17,26 @@
 #ifndef __VTD_INFO_PPI_H__
 #define __VTD_INFO_PPI_H__
 
+#include 
+#include 
+
 #define EDKII_VTD_INFO_PPI_GUID \
 { \
   0x8a59fcb3, 0xf191, 0x400c, { 0x97, 0x67, 0x67, 0xaf, 0x2b, 0x25, 0x68, 
0x4a } \
 }
 
-typedef struct _EDKII_VTD_INFO_PPI  EDKII_VTD_INFO_PPI;
-
-#define EDKII_VTD_INFO_PPI_REVISION 0x0001
-
-struct _EDKII_VTD_INFO_PPI {
-  UINT64  Revision;
-  UINT8   HostAddressWidth;
-  UINT8   Reserved[3];
-  UINT32  VTdEngineCount;
-  UINT64  VTdEngineAddress[1];
-};
+//
+// VTD info PPI just use same data structure as DMAR table.
+//
+// The reported information must include what is needed in PEI phase, e.g.
+//   the VTd engine (such as DRHD)
+//   the reserved DMA address in PEI for eary graphic (such as RMRR for 
graphic UMA)
+//
+// The reported information can be and might be a subset of full DMAR table, 
e.g.
+//   if some data is not avaiable (such as ANDD),
+//   if some data is not needed (such as RMRR for legacy USB).
+//
+typedef EFI_ACPI_DMAR_HEADER EDKII_VTD_INFO_PPI;
 
 extern EFI_GUID gEdkiiVTdInfoPpiGuid;
 
-- 
2.7.4.windows.1

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

[edk2] [PATCH 0/3] IntelSiliconPkg/InteVTdPei: Add RMRR support in PEI

[Jiewen Yao]

We notice that there is real usage in PEI to show
the graphic output.
The Integrated Graphic Device is blocked by current
IntelVTdPei because the DMA buffer is fully controlled
by VTd PEIM. The UMA is not allowed.

In DXE phase, the UMA is reported via RMRR table.

As such, we need similar way in PEI to let VTd PEI
get the RMRR information.

This series patch resolves this problem.

We also updated sample driver to show how to get the RMRR information.

Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao 

Jiewen Yao (3):
  IntelSiliconPkg/VTdInfoPpi: Let it follow DMAR table.
  IntelSiliconPkg/IntelVTdPmrPei: Parse RMRR table.
  IntelSiliconPkg/VTdInfoSample: Add RMRR table.

 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmr.c   
   |  52 +-
 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c
   | 581 +++-
 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.h
   |  20 +-
 
IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c 
  | 156 +-
 
IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
 |   3 +
 IntelSiliconPkg/Include/Ppi/VtdInfo.h  
   |  26 +-
 6 files changed, 788 insertions(+), 50 deletions(-)

-- 
2.7.4.windows.1

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

[edk2] [PATCH 3/3] IntelSiliconPkg/VTdInfoSample: Add RMRR table.

[Jiewen Yao]

Let system report RMRR table for the platform support
PEI graphic.

Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao 
---
 
IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c 
  | 156 ++--
 
IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.inf
 |   3 +
 2 files changed, 149 insertions(+), 10 deletions(-)

diff --git 
a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
 
b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
index c79398f..423e2f1 100644
--- 
a/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
+++ 
b/IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/PlatformVTdInfoSamplePei.c
@@ -18,21 +18,111 @@
 
 #include 
 #include 
+#include 
+#include 
+
+#define R_SA_MCHBAR   (0x48)
+#define R_SA_GGC  (0x50)
+#define N_SKL_SA_GGC_GGMS_OFFSET  (0x6)
+#define B_SKL_SA_GGC_GGMS_MASK(0xc0)
+#define N_SKL_SA_GGC_GMS_OFFSET   (0x8)
+#define B_SKL_SA_GGC_GMS_MASK (0xff00)
+#define V_SKL_SA_GGC_GGMS_8MB 3
+#define R_SA_TOLUD(0xbc)
+
+#define R_SA_MCHBAR_VTD1_OFFSET  0x5400  ///< HW UNIT for IGD
+#define R_SA_MCHBAR_VTD2_OFFSET  0x5410  ///< HW UNIT for all other - PEG, 
USB, SATA etc
 
 typedef struct {
-  UINT64  Revision;
-  UINT8   HostAddressWidth;
-  UINT8   Reserved[3];
-  UINT32  VTdEngineCount;
-  UINT64  VTdEngineAddress[2];
+  EFI_ACPI_DMAR_HEADER DmarHeader;
+  //
+  // VTd engine 1 - integrated graphic
+  //
+  EFI_ACPI_DMAR_DRHD_HEADERDrhd1;
+  EFI_ACPI_DMAR_DEVICE_SCOPE_STRUCTURE_HEADER  Drhd11;
+  EFI_ACPI_DMAR_PCI_PATH   Drhd111;
+  //
+  // VTd engine 2 - all rest
+  //
+  EFI_ACPI_DMAR_DRHD_HEADERDrhd2;
+  //
+  // RMRR 1 - integrated graphic
+  //
+  EFI_ACPI_DMAR_RMRR_HEADERRmrr1;
+  EFI_ACPI_DMAR_DEVICE_SCOPE_STRUCTURE_HEADER  Rmrr11;
+  EFI_ACPI_DMAR_PCI_PATH   Rmrr111;
 } MY_VTD_INFO_PPI;
 
 MY_VTD_INFO_PPI  mPlatformVTdSample = {
-  EDKII_VTD_INFO_PPI_REVISION,
-  0x26,
-  {0},
-  2,
-  {0xFED9, 0xFED91000},
+  { // DmarHeader
+{ // Header
+  EFI_ACPI_4_0_DMA_REMAPPING_TABLE_SIGNATURE,
+  sizeof(MY_VTD_INFO_PPI),
+  EFI_ACPI_DMAR_REVISION,
+},
+0x26, // HostAddressWidth
+  },
+
+  { // Drhd1
+{ // Header
+  EFI_ACPI_DMAR_TYPE_DRHD,
+  sizeof(EFI_ACPI_DMAR_DRHD_HEADER) +
+sizeof(EFI_ACPI_DMAR_DEVICE_SCOPE_STRUCTURE_HEADER) +
+sizeof(EFI_ACPI_DMAR_PCI_PATH)
+},
+0, // Flags
+0, // Reserved
+0, // SegmentNumber
+0xFED9 // RegisterBaseAddress -- TO BE PATCHED
+  },
+  { // Drhd11
+EFI_ACPI_DEVICE_SCOPE_ENTRY_TYPE_PCI_ENDPOINT,
+sizeof(EFI_ACPI_DMAR_DEVICE_SCOPE_STRUCTURE_HEADER) +
+  sizeof(EFI_ACPI_DMAR_PCI_PATH),
+0, // Reserved2
+0, // EnumerationId
+0  // StartBusNumber
+  },
+  { // Drhd111
+2,  // Device
+0   // Function
+  },
+
+  { // Drhd2
+{ // Header
+  EFI_ACPI_DMAR_TYPE_DRHD,
+  sizeof(EFI_ACPI_DMAR_DRHD_HEADER)
+},
+EFI_ACPI_DMAR_DRHD_FLAGS_INCLUDE_PCI_ALL, // Flags
+0, // Reserved
+0, // SegmentNumber
+0xFED91000 // RegisterBaseAddress -- TO BE PATCHED
+  },
+
+  { // Rmrr1
+{ // Header
+  EFI_ACPI_DMAR_TYPE_RMRR,
+  sizeof(EFI_ACPI_DMAR_RMRR_HEADER) +
+sizeof(EFI_ACPI_DMAR_DEVICE_SCOPE_STRUCTURE_HEADER) +
+sizeof(EFI_ACPI_DMAR_PCI_PATH)
+},
+{0}, // Reserved
+0, // SegmentNumber
+0x0, // ReservedMemoryRegionBaseAddress -- TO BE PATCHED
+0x0 // ReservedMemoryRegionLimitAddress -- TO BE PATCHED
+  },
+  { // Rmrr11
+EFI_ACPI_DEVICE_SCOPE_ENTRY_TYPE_PCI_ENDPOINT,
+sizeof(EFI_ACPI_DMAR_DEVICE_SCOPE_STRUCTURE_HEADER) +
+  sizeof(EFI_ACPI_DMAR_PCI_PATH),
+0, // Reserved2
+0, // EnumerationId
+0  // StartBusNumber
+  },
+  { // Rmrr111
+2,  // Device
+0   // Function
+  },
 };
 
 EFI_PEI_PPI_DESCRIPTOR mPlatformVTdInfoSampleDesc = {
@@ -42,6 +132,50 @@ EFI_PEI_PPI_DESCRIPTOR mPlatformVTdInfoSampleDesc = {
 };
 
 /**
+  Patch Graphic UMA address in RMRR and base address.
+**/
+VOID
+PatchDmar (
+  VOID
+  )
+{
+  UINT32  MchBar;
+  UINT16  IgdMode;
+  UINT16  GttMode;
+  UINT32  IgdMemSize;
+  UINT32  GttMemSize;
+
+  ///
+  /// Calculate IGD memsize
+  ///
+  IgdMode = ((PciRead16 (PCI_LIB_ADDRESS(0, 0, 0, R_SA_GGC)) & 
B_SKL_SA_GGC_GMS_MASK) >> N_SKL_SA_GGC_GMS_OFFSET) & 0xFF;
+  if (IgdMode < 0xF0) {
+IgdMemSize = IgdMode * 32 * (1024) * (1024);
+  } else {
+IgdMemSize = 4 * (IgdMode - 0xF0 + 1) * (1024) * 

[edk2] [PATCH 2/3] IntelSiliconPkg/IntelVTdPmrPei: Parse RMRR table.

[Jiewen Yao]

In order to support PEI graphic, we let VTdPmrPei driver
parse DMAR table RMRR entry and allow the UMA access.

If a system has no PEI IGD, no RMRR is needed. The behavior
is unchanged.

If a system has PEI IGD, it must report RMRR in PEI phase.
The PeiVTdPrm will program the IGD VTd engine to skip the
RMRR region, and program the reset PCI VTd engine to skip
the another DMA buffer allocated in PEI phase for other
device driver.

Cc: Star Zeng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao 
---
 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmr.c|  52 +-
 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.c | 581 
+++-
 IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmrPei.h |  20 +-
 3 files changed, 624 insertions(+), 29 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmr.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmr.c
index ef08e29..be841aa 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmr.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdPmrPei/IntelVTdPmr.c
@@ -22,7 +22,7 @@
 
 #include "IntelVTdPmrPei.h"
 
-extern EDKII_VTD_INFO_PPI*mVTdInfoPpi;
+extern VTD_INFO*mVTdInfo;
 
 /**
   Get protected low memory alignment.
@@ -60,7 +60,7 @@ GetPhmrAlignment (
   UINT64Data64;
   UINT8 HostAddressWidth;
 
-  HostAddressWidth = mVTdInfoPpi->HostAddressWidth;
+  HostAddressWidth = mVTdInfo->HostAddressWidth;
 
   MmioWrite64 (VtdUnitBaseAddress + R_PMEN_HIGH_BASE_REG, 0x);
   Data64 = MmioRead64 (VtdUnitBaseAddress + R_PMEN_HIGH_BASE_REG);
@@ -73,11 +73,13 @@ GetPhmrAlignment (
 /**
   Get protected low memory alignment.
 
+  @param EngineMask The mask of the VTd engine to be accessed.
+
   @return protected low memory alignment.
 **/
 UINT32
 GetLowMemoryAlignment (
-  VOID
+  IN UINT64EngineMask
   )
 {
   UINTN Index;
@@ -85,8 +87,11 @@ GetLowMemoryAlignment (
   UINT32FinalAlignment;
 
   FinalAlignment = 0;
-  for (Index = 0; Index < mVTdInfoPpi->VTdEngineCount; Index++) {
-Alignment = GetPlmrAlignment ((UINTN)mVTdInfoPpi->VTdEngineAddress[Index]);
+  for (Index = 0; Index < mVTdInfo->VTdEngineCount; Index++) {
+if ((EngineMask & LShiftU64(1, Index)) == 0) {
+  continue;
+}
+Alignment = GetPlmrAlignment ((UINTN)mVTdInfo->VTdEngineAddress[Index]);
 if (FinalAlignment < Alignment) {
   FinalAlignment = Alignment;
 }
@@ -97,11 +102,13 @@ GetLowMemoryAlignment (
 /**
   Get protected high memory alignment.
 
+  @param EngineMask The mask of the VTd engine to be accessed.
+
   @return protected high memory alignment.
 **/
 UINT64
 GetHighMemoryAlignment (
-  VOID
+  IN UINT64EngineMask
   )
 {
   UINTN Index;
@@ -109,8 +116,11 @@ GetHighMemoryAlignment (
   UINT64FinalAlignment;
 
   FinalAlignment = 0;
-  for (Index = 0; Index < mVTdInfoPpi->VTdEngineCount; Index++) {
-Alignment = GetPhmrAlignment ((UINTN)mVTdInfoPpi->VTdEngineAddress[Index]);
+  for (Index = 0; Index < mVTdInfo->VTdEngineCount; Index++) {
+if ((EngineMask & LShiftU64(1, Index)) == 0) {
+  continue;
+}
+Alignment = GetPhmrAlignment ((UINTN)mVTdInfo->VTdEngineAddress[Index]);
 if (FinalAlignment < Alignment) {
   FinalAlignment = Alignment;
 }
@@ -246,6 +256,7 @@ SetPmrRegion (
 /**
   Set DMA protected region.
 
+  @param EngineMask The mask of the VTd engine to be accessed.
   @param LowMemoryBase  The protected low memory region base.
   @param LowMemoryLengthThe protected low memory region length.
   @param HighMemoryBase The protected high memory region base.
@@ -256,6 +267,7 @@ SetPmrRegion (
 **/
 EFI_STATUS
 SetDmaProtectedRange (
+  IN UINT64EngineMask,
   IN UINT32LowMemoryBase,
   IN UINT32LowMemoryLength,
   IN UINT64HighMemoryBase,
@@ -265,12 +277,15 @@ SetDmaProtectedRange (
   UINTN   Index;
   EFI_STATUS  Status;
 
-  DEBUG ((DEBUG_INFO, "SetDmaProtectedRange - [0x%x, 0x%x] [0x%lx, 0x%lx]\n", 
LowMemoryBase, LowMemoryLength, HighMemoryBase, HighMemoryLength));
+  DEBUG ((DEBUG_INFO, "SetDmaProtectedRange(0x%lx) - [0x%x, 0x%x] [0x%lx, 
0x%lx]\n", EngineMask, LowMemoryBase, LowMemoryLength, HighMemoryBase, 
HighMemoryLength));
 
-  for (Index = 0; Index < mVTdInfoPpi->VTdEngineCount; Index++) {
-DisablePmr ((UINTN)mVTdInfoPpi->VTdEngineAddress[Index]);
+  for (Index = 0; Index < mVTdInfo->VTdEngineCount; Index++) {
+if ((EngineMask & LShiftU64(1, Index)) == 0) {
+  continue;
+}
+DisablePmr ((UINTN)mVTdInfo->VTdEngineAddress[Index]);
 Status = SetPmrRegion (
-   (UINTN)mVTdInfoPpi->VTdEngineAddress[Index],
+   (UINTN)mVTdInfo->VTdEngineAddress[Index],
LowMemoryBase,
LowMemoryLength,
HighMemoryBase,
@@ -279,7 +294,7 @@