date:20180615

Re: [edk2] Creating EFI System Partition

2018-06-15 Thread Rod Smith

On 06/15/2018 11:01 AM, Andrew Fish wrote:
> 
>> On Jun 15, 2018, at 6:17 AM, Rod Smith > > wrote:
>> 
>> but AFAIK, any common tool for creating a FAT32 filesystem should
>> work. I generally do it with mkdosfs in Linux, but equivalent tools
>> in macOS, Windows, or the BSDs also work. In practice, FAT16 and
>> FAT12 usually work, too, although the EFI spec does explicitly say
>> at one point that the filesystem should be FAT32, and I know of at
>> least one implementation that gets a little flaky with FAT16, so
>> I'd stick with FAT32.
>> 
> 
> I seem to remember that the FAT32 spec also defined FAT16 and FAT12.
> it also defines when FAT32, FAT16, or FAT12 should be used for
> media.
> 
> If the edk2 FAT driver has an issue with media that conform to the
> FAT32 spec we should fix that. If the issue is non conferment, then
> we need to decide if the fix can be made that will follow the FAT
> Spec. See that CYA was useful after all :).

No, I didn't mean to imply that the EDK2 FAT driver gets flaky with 12-
or 16-bit FAT filesystems. The EFI in question was an early
EFI-over-BIOS implementation on a Gigabyte motherboard from 2011 or
2012. That thing was a horror, and I wrote up my experiences at the time:

http://www.rodsbooks.com/gb-hybrid-efi/

I no longer have that motherboard, so I can't do any more tests with it
or double-check my findings from 2012. From that page, though:

: A FAT-16 ESP, on the other hand, seems problematic. Ubuntu 11.04 (and
: 11.10) in EFI mode creates a dinky FAT-16 ESP, and after my test
: install of Ubuntu 11.04, the board hung on reboot until I reworked
: the ESP as FAT-32. Thus, if you plan to install Ubuntu, or any other
: OS that creates a FAT-16 ESP, be prepared to fix it, preferably
: before the system reboots!

Note that Ubuntu no longer creates a "dinky FAT-16 ESP;" it now creates
a 512MiB FAT-32 ESP. The experience remains a relevant cautionary tale,
though, for anybody who's trying to write an OS installer, particularly
if the system must be installable on some random computer -- systems
from that period are still in use today, so a FAT-16 ESP could cause
problems in the real world. That said, I've not encountered this problem
on any modern (say, 2014 or later) EFI.

-- 
Rod Smith
rodsm...@rodsbooks.com
http://www.rodsbooks.com
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 1/3] Platform/Socionext/DeveloperBox: add support for persistent capsules

2018-06-15 Thread Leif Lindholm

On Thu, Jun 07, 2018 at 01:32:01PM +0200, Ard Biesheuvel wrote:
> Add support for PersistAcrossReset capsules, by setting the associated
> PCD, and by enabling the warm reboot implementation that reenters PEI
> with interrupts, caches and MMU disabled.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 

Reviewed-by: Leif Lindholm 

Feel free to push this one separately once everything is in place to
do so.

> ---
>  Platform/Socionext/DeveloperBox/DeveloperBox.dsc | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc 
> b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> index 300e3fd656a5..1476257acce1 100644
> --- a/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> +++ b/Platform/Socionext/DeveloperBox/DeveloperBox.dsc
> @@ -226,11 +226,8 @@ [PcdsFeatureFlag]
>gEfiMdeModulePkgTokenSpaceGuid.PcdConOutUgaSupport|FALSE
>gEfiMdePkgTokenSpaceGuid.PcdUgaConsumeSupport|FALSE
>  
> -  #
> -  # This requires support in ARM Trusted Firmware and SCP for warm reset,
> -  # so disable for now
> -  #
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSupportUpdateCapsuleReset|FALSE
> +  gArmTokenSpaceGuid.PcdArmReenterPeiForCapsuleWarmReboot|TRUE
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSupportUpdateCapsuleReset|TRUE
>  
># needed for NFIT tables installed by RamDiskDxe
>gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE
> -- 
> 2.17.0
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 0/4] MdeModulePkg ArmPkg: support for persistent capsules and progress reporting

2018-06-15 Thread Ard Biesheuvel

On 13 June 2018 at 10:08, Ard Biesheuvel  wrote:
> This is the delta of code required to implement PersistAcrossReset on ARM
> systems, and to wire up the capsule handling routines in a way that makes
> the new progress reporting code do something meaningful on such platforms.
>
> Changes since v3:
> - let both UpdateCapsule() and QueryCapsuleCapabilities() return 
> EFI_UNSUPPORTED
>   when called at OS runtime on an ARM system
> - reset the system unconditionally after having processed any capsules (#3)
> - re-add Leif's ack (#3)
>
> Changes since v2:
> - move cache handling from CapsulePei to CapsuleRuntimeDxe, and make it ARM 
> only
> - drop patch to change ProcessCapsules() logic in DxeCapsuleLibFmp; instead,
>   the platform BDS code is modified to perform the ProcessCapsuleImage()
>   call directly
>
> Changes since v1:
> - incorporate Star's feedback (#1, #2)
> - add Leif's ack (#4)
>
> Patch #1 ensures that the capsule data which is preserved in DRAM across
> a reboot is written back to main memory before attempting to access it
> with the caches off.
>
> Patch #2 updates DxeCapsuleLibFmp so it does not pass down the progress
> indication callback if its own attempt to invoke it has already failed.
>
> Patch #3 updates ArmPkg's generic PlatformBootManagerLib implementation
> to only call ProcessCapsules() after the [potentially non-trusted]
> console is up and running, to ensure that firmware update progress can
> be reported to the user.
>
> Patch #4 modifies ArmSmcPsciResetSystemLib to emulate a proper warm reboot
> by reentering PEI with interrupts, MMU and caches enabled. This works
> around the lack of an architected warm reboot in most current implementations.
> (The PSCI spec does cover warm reboot, but it was added recently and most
> secure firmware implementations haven't caught up yet)
>
> Ard Biesheuvel (4):
>   MdeModulePkg/CapsuleRuntimeDxe: clean the capsule payload to DRAM
>   MdeModulePkg/DxeCapsuleLibFmp: pass progress callback only if it works
>   ArmPkg/PlatformBootManagerLib: call ProcessCapsules() only once
>   ArmPkg/ArmSmcPsciResetSystemLib: implement fallback for warm reboot
>

Pushed as 488aab257f70..dde2dd64f070

Thanks all

>  ArmPkg/ArmPkg.dec |  4 +
>  .../ArmSmcPsciResetSystemLib.c| 21 -
>  .../ArmSmcPsciResetSystemLib.inf  |  9 ++
>  .../PlatformBootManagerLib/PlatformBm.c   | 86 +--
>  .../PlatformBootManagerLib.inf|  1 +
>  .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.c  | 13 ++-
>  .../CapsuleRuntimeDxe/Arm/CapsuleReset.c  | 77 +
>  .../CapsuleRuntimeDxe/CapsuleReset.c  | 51 +++
>  .../CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf   | 14 ++-
>  .../CapsuleRuntimeDxe/CapsuleService.c| 33 ++-
>  .../CapsuleRuntimeDxe/CapsuleService.h| 73 
>  11 files changed, 321 insertions(+), 61 deletions(-)
>  create mode 100644 
> MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
>  create mode 100644 MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c
>  create mode 100644 MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.h
>
> --
> 2.17.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Leif Lindholm

On Fri, Jun 15, 2018 at 03:26:54PM +, Evan Lloyd wrote:
> Hi Ard, Zeng
> 
> > -Original Message-
> > From: Ard Biesheuvel 
> > Sent: 15 June 2018 15:17
> > To: Sami Mujawar 
> > Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong
> > ; Ruiyu Ni ; Leif Lindholm
> > ; Matteo Carlini ;
> > Stephanie Hughes-Fitt ; Evan Lloyd
> > ; Thomas Abraham ;
> > nd 
> > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
> > space
> > 
> > On 15 June 2018 at 16:13, Sami Mujawar  wrote:
> > > The SATA controller driver crashes while accessing the PCI memory
> > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
> > >
> > > Enable the PCI memory space access to prevent the SATA Controller
> > > driver from crashing.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Sami Mujawar 
> > > ---
> > > The changes can be viewed at
> > >
> > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
> > i
> > > x_v2
> > >
> > > Notes:
> > > v2:
> > > - Improved log message and code documentation based on feedback
> > [SAMI]
> > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE
> > > [ZENG]
> > > - This SATA Controller driver only uses the PCI BAR5 register
> > >   space which is the AHCI Base Address (ABAR). According to the
> > >   'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
> > >   specification, section 2.1.11, 'This register allocates space
> > >   for the HBA memory registers'.
> > >   The section 2.1.10, allows provision for Optional BARs which
> > >   may support either memory or I/O spaces. However, in the context
> > >   of the current SATA controller driver, which only ever access
> > >   the ABAR, enabling I/O memory space is not required.
> > > [SAMI]
> > > - Prefer to use // surrounding comments   
> > > [ZENG]
> > > - Doing this would violate the edk2 coding standard. See EDK2
> > >   Coding Standard Specification, revision 2.20, section 6.2.3.
> > > [SAMI]
> > >
> > 
> > Please stop obsessing about the coding standard. The maintainer was quite
> > clear what he wanted, and in the past, I have also indicated that for the 
> > ARM
> > related packages, I prefer readability and consistency over adherence to a
> > dubious standard.
> 
> [[Evan Lloyd]] I'd like to make some points here:
> 1.It is perfectly reasonable for Sami to explain why he has done
>   something a certain way - Zeng is then at liberty to respond
>   with his preference, but we do not (yet) know what that might
>   be.

Yes we do. Zeng responded with that in the first instance. Sami then
disputed that explicitly stated preference, referring to the coding
standard. There was no lack of clarity in what Zeng wanted - so I
remain unclear on what this is intending to achieve?

> 2."readability and consistency" is the very purpose of any
>   coding standard.  If consistency is valuable, doesn't that
>   apply across modules?  I understand that it may be
>   particularly valuable for maintainers within their modules,
>   but the rest of us benefit from a consistent style -
>   especially when looking outside our normal demesne.

At which point the rule of thumb is: aligning with the immediately
surrounding code always trumps adhering to the specified style.
Which is in itself trumped by the maintainer explicitly stating
another preference. (Like here.)

> 3.Applying a consistent Coding Standard across the whole project
>   is a necessary pre-condition for automated CI checks on new
>   submissions.  I'd like to aim e.g. for an improved
>   patchcheck.py, but that requires consistency across modules,
>   at least for new code.

Such a system will _never_ be fully automatable.

I will _always_ take a 80 < x < 90 line over one that breaks up an
output string or one that reduces readability more than having to
side-scroll does.

That doesn't make it worthless, far from it.

> Note: I am not disputing the dubiosity of the  CCS.  It could be
> improved (a lot).  However it is all we have right now.

Then a more constructive approach is to recognise that not a single
one of the maintainers are appearing to be respecting the horror vacui
rule and submit a patch against
https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification
to have section 6.2.3 deleted.

(Much like I should at some point get around to have 5.4.2.2.2
deleted, or at least conditionalised to only apply for !ARM.)

Regards,

Leif


> Regards,
> Evan
> 
> > 
> > 
> > > v1:
> > > - Fix SATA Controller driver crash
> > > [SAMI]
> ...
> > > --
> > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> > >
> > >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Ard Biesheuvel

On 15 June 2018 at 17:26, Evan Lloyd  wrote:
> Hi Ard, Zeng
>
>> -Original Message-
>> From: Ard Biesheuvel 
>> Sent: 15 June 2018 15:17
>> To: Sami Mujawar 
>> Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong
>> ; Ruiyu Ni ; Leif Lindholm
>> ; Matteo Carlini ;
>> Stephanie Hughes-Fitt ; Evan Lloyd
>> ; Thomas Abraham ;
>> nd 
>> Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
>> space
>>
>> On 15 June 2018 at 16:13, Sami Mujawar  wrote:
>> > The SATA controller driver crashes while accessing the PCI memory
>> > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
>> >
>> > Enable the PCI memory space access to prevent the SATA Controller
>> > driver from crashing.
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.1
>> > Signed-off-by: Sami Mujawar 
>> > ---
>> > The changes can be viewed at
>> >
>> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
>> i
>> > x_v2
>> >
>> > Notes:
>> > v2:
>> > - Improved log message and code documentation based on feedback
>> [SAMI]
>> > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE
>> > [ZENG]
>> > - This SATA Controller driver only uses the PCI BAR5 register
>> >   space which is the AHCI Base Address (ABAR). According to the
>> >   'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
>> >   specification, section 2.1.11, 'This register allocates space
>> >   for the HBA memory registers'.
>> >   The section 2.1.10, allows provision for Optional BARs which
>> >   may support either memory or I/O spaces. However, in the context
>> >   of the current SATA controller driver, which only ever access
>> >   the ABAR, enabling I/O memory space is not required.
>> > [SAMI]
>> > - Prefer to use // surrounding comments   
>> > [ZENG]
>> > - Doing this would violate the edk2 coding standard. See EDK2
>> >   Coding Standard Specification, revision 2.20, section 6.2.3.
>> > [SAMI]
>> >
>>
>> Please stop obsessing about the coding standard. The maintainer was quite
>> clear what he wanted, and in the past, I have also indicated that for the ARM
>> related packages, I prefer readability and consistency over adherence to a
>> dubious standard.
>
> [[Evan Lloyd]] I'd like to make some points here:
> 1.  It is perfectly reasonable for Sami to explain why he has done 
> something a certain way - Zeng is then at liberty to respond with his 
> preference, but we do not (yet) know what that might be.
>

Yes, we do. He already told us his preference.

> 2.  "readability and consistency" is the very purpose of any coding 
> standard.  If consistency is valuable, doesn't that apply across modules?  I 
> understand that it may be particularly valuable for maintainers within their 
> modules, but the rest of us benefit from a consistent style - especially when 
> looking outside our normal demesne.
>
> 3.  Applying a consistent Coding Standard across the whole project is a 
> necessary pre-condition for automated CI checks on new submissions.  I'd like 
> to aim e.g. for an improved patchcheck.py, but that requires consistency 
> across modules, at least for new code.
>
> Note: I am not disputing the dubiosity of the  CCS.  It could be improved (a 
> lot).  However it is all we have right now.
>

OK, so you are arguing that we should adhere to the letter of the CCS
even though there is consensus about its shortcomings and its
detachment from reality? I think that is a waste of everybody's time.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Evan Lloyd

Hi Ard, Zeng

> -Original Message-
> From: Ard Biesheuvel 
> Sent: 15 June 2018 15:17
> To: Sami Mujawar 
> Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric Dong
> ; Ruiyu Ni ; Leif Lindholm
> ; Matteo Carlini ;
> Stephanie Hughes-Fitt ; Evan Lloyd
> ; Thomas Abraham ;
> nd 
> Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem
> space
> 
> On 15 June 2018 at 16:13, Sami Mujawar  wrote:
> > The SATA controller driver crashes while accessing the PCI memory
> > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled.
> >
> > Enable the PCI memory space access to prevent the SATA Controller
> > driver from crashing.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Sami Mujawar 
> > ---
> > The changes can be viewed at
> >
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
> i
> > x_v2
> >
> > Notes:
> > v2:
> > - Improved log message and code documentation based on feedback
> [SAMI]
> > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG]
> > - This SATA Controller driver only uses the PCI BAR5 register
> >   space which is the AHCI Base Address (ABAR). According to the
> >   'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
> >   specification, section 2.1.11, 'This register allocates space
> >   for the HBA memory registers'.
> >   The section 2.1.10, allows provision for Optional BARs which
> >   may support either memory or I/O spaces. However, in the context
> >   of the current SATA controller driver, which only ever access
> >   the ABAR, enabling I/O memory space is not required.[SAMI]
> > - Prefer to use // surrounding comments   [ZENG]
> > - Doing this would violate the edk2 coding standard. See EDK2
> >   Coding Standard Specification, revision 2.20, section 6.2.3.[SAMI]
> >
> 
> Please stop obsessing about the coding standard. The maintainer was quite
> clear what he wanted, and in the past, I have also indicated that for the ARM
> related packages, I prefer readability and consistency over adherence to a
> dubious standard.

[[Evan Lloyd]] I'd like to make some points here:
1.  It is perfectly reasonable for Sami to explain why he has done 
something a certain way - Zeng is then at liberty to respond with his 
preference, but we do not (yet) know what that might be.  

2.  "readability and consistency" is the very purpose of any coding 
standard.  If consistency is valuable, doesn't that apply across modules?  I 
understand that it may be particularly valuable for maintainers within their 
modules, but the rest of us benefit from a consistent style - especially when 
looking outside our normal demesne.

3.  Applying a consistent Coding Standard across the whole project is a 
necessary pre-condition for automated CI checks on new submissions.  I'd like 
to aim e.g. for an improved patchcheck.py, but that requires consistency across 
modules, at least for new code.

Note: I am not disputing the dubiosity of the  CCS.  It could be improved (a 
lot).  However it is all we have right now.

Regards,
Evan

> 
> 
> > v1:
> > - Fix SATA Controller driver crash[SAMI]
...
> > --
> > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> >
> >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Creating EFI System Partition

2018-06-15 Thread Andrew Fish

Sorry Rod, called you Rob. I blame pre coffee emailing. My bad.

Thanks,

Andrew Fish

> On Jun 15, 2018, at 8:01 AM, Andrew Fish  wrote:
> 
> 
> 
>> On Jun 15, 2018, at 6:17 AM, Rod Smith > > wrote:
>> 
>> On 06/14/2018 08:35 PM, Robinson, Herbie wrote:
>>> I have been tasked with implementing EFI boot in our VOS operating
>>> system (in a panic, nobody bothered to tell us ahead of time that
>>> legacy boot was being dropped from the BIOS on our latest hardware).
>>> I think we have a pretty good handle on writing the bootloader, but I
>>> can't find any solid information on creating an empty EFI System
>>> Partition bearing the correct flavor of FAT32 to put the bootloader
>>> in.
>> 
>> The EFI spec has some quirky CYA-type wording about its filesystem,
> 
> Rob,
> 
> Thanks for all this info. 
> 
> The CYA is the UEFI Spec referencing the FAT spec I cross linked. Given a 
> specification needs to be the arbiter of truth some times it is required to 
> be pedantic. In this case the UEFI Spec needed a definition of FAT32, and 
> that is the one and only FAT spec. contributed by Microsoft. 
> 
>> but
>> AFAIK, any common tool for creating a FAT32 filesystem should work. I
>> generally do it with mkdosfs in Linux, but equivalent tools in macOS,
>> Windows, or the BSDs also work. In practice, FAT16 and FAT12 usually
>> work, too, although the EFI spec does explicitly say at one point that
>> the filesystem should be FAT32, and I know of at least one
>> implementation that gets a little flaky with FAT16, so I'd stick with
>> FAT32. An exception is if you need a very small filesystem, as on a
>> bootable optical disc, in which case FAT16 or FAT12 might be required.
>> Also, I've seen reports of problems with filesystems smaller than 512MiB
>> on some EFIs, so I recommend making it at least that large, at least if
>> it will be on a hard disk.
>> 
> 
> I seem to remember that the FAT32 spec also defined FAT16 and FAT12. it also 
> defines when FAT32, FAT16, or FAT12 should be used for media.
> 
> If the edk2 FAT driver has an issue with media that conform to the FAT32 spec 
> we should fix that. If the issue is non conferment, then we need to decide if 
> the fix can be made that will follow the FAT Spec. See that CYA was useful 
> after all :). 
> 
> Thanks,
> 
> Andrew Fish
> 
>> Ideally, the EFI System Partition (ESP) should be identified by the
>> correct partition table type code. On an MBR disk, this is 0xEF; and on
>> a GPT disk, it's C12A7328-F81F-11D2-BA4B-00A0C93EC93B, which is
>> identified in various ways depending on the partitioning software (type
>> EF00 in gdisk; a "boot flag" or "esp flag" set in parted; etc.). GPT is
>> preferable, particularly for installations on hard disks, since some
>> OSes tie the boot mode to the partition table type. (Of course, this
>> might not be an issue for you -- it depends on what you're preparing.)
>> In practice, I'm not aware of any EFI that actually requires the
>> partition type code to be set correctly; every one I've tried will boot
>> fine even if the type code is set to something other than an official
>> ESP type code. That said, I'd set the type code correctly just to be
>> sure it works on an oddball system, and to avoid confusion if/when users
>> look at the disk.
>> 
>> If the boot medium is an optical disc, you'll need a particular variant
>> of an El Torito boot image. If you need help with this, please say so; I
>> can dig up the details, or at least point you to a sample mkisofs command.
>> 
>>> I need to end up with a binary image file (which I can process
>>> into an object module and embed into our OS code for initializing
>>> disks).
>> 
>> GPT places data structures at both the beginning and the end of the
>> disk, which might create some complications, depending on your exact
>> needs. If you need to write an image to a blank disk of unknown size,
>> several options occur to me:
>> 
>> * You can use MBR, which does not rely on data structures at the
>> end of the disk. As noted above, though, that's a little iffy in
>> some cases -- but it might be fine for your case (it's hard to
>> tell without more details).
>> * You can prepare a virtual image of a small disk and write it out
>> to a larger disk, leaving the backup GPT data structures before
>> the end of the disk; then either:
>> * Leave it that way, which will effectively limit the size of the
>>   disk. This might be OK for a USB flash drive.
>> * Use a disk partitioning tool to relocate the backup GPT data
>>   structures to the end of the disk. The sgdisk "-e" option will
>>   do this, for instance. IIRC, parted will do it automatically,
>>   or at least prompt that it be done, if any operation is performed
>>   on the disk.
>> * You can create an image of the FAT32 ESP filesystem ONLY (without
>> partition table), then have your wrapper tool use sgdisk, parted,
>> or some other tool to prepare a disk with GPT and an ESP. You'd
>> then write out

Re: [edk2] Creating EFI System Partition

2018-06-15 Thread Andrew Fish




> On Jun 15, 2018, at 6:17 AM, Rod Smith  wrote:
> 
> On 06/14/2018 08:35 PM, Robinson, Herbie wrote:
>> I have been tasked with implementing EFI boot in our VOS operating
>> system (in a panic, nobody bothered to tell us ahead of time that
>> legacy boot was being dropped from the BIOS on our latest hardware).
>> I think we have a pretty good handle on writing the bootloader, but I
>> can't find any solid information on creating an empty EFI System
>> Partition bearing the correct flavor of FAT32 to put the bootloader
>> in.
> 
> The EFI spec has some quirky CYA-type wording about its filesystem,

Rob,

Thanks for all this info. 

The CYA is the UEFI Spec referencing the FAT spec I cross linked. Given a 
specification needs to be the arbiter of truth some times it is required to be 
pedantic. In this case the UEFI Spec needed a definition of FAT32, and that is 
the one and only FAT spec. contributed by Microsoft. 

> but
> AFAIK, any common tool for creating a FAT32 filesystem should work. I
> generally do it with mkdosfs in Linux, but equivalent tools in macOS,
> Windows, or the BSDs also work. In practice, FAT16 and FAT12 usually
> work, too, although the EFI spec does explicitly say at one point that
> the filesystem should be FAT32, and I know of at least one
> implementation that gets a little flaky with FAT16, so I'd stick with
> FAT32. An exception is if you need a very small filesystem, as on a
> bootable optical disc, in which case FAT16 or FAT12 might be required.
> Also, I've seen reports of problems with filesystems smaller than 512MiB
> on some EFIs, so I recommend making it at least that large, at least if
> it will be on a hard disk.
> 

I seem to remember that the FAT32 spec also defined FAT16 and FAT12. it also 
defines when FAT32, FAT16, or FAT12 should be used for media.

If the edk2 FAT driver has an issue with media that conform to the FAT32 spec 
we should fix that. If the issue is non conferment, then we need to decide if 
the fix can be made that will follow the FAT Spec. See that CYA was useful 
after all :). 

Thanks,

Andrew Fish

> Ideally, the EFI System Partition (ESP) should be identified by the
> correct partition table type code. On an MBR disk, this is 0xEF; and on
> a GPT disk, it's C12A7328-F81F-11D2-BA4B-00A0C93EC93B, which is
> identified in various ways depending on the partitioning software (type
> EF00 in gdisk; a "boot flag" or "esp flag" set in parted; etc.). GPT is
> preferable, particularly for installations on hard disks, since some
> OSes tie the boot mode to the partition table type. (Of course, this
> might not be an issue for you -- it depends on what you're preparing.)
> In practice, I'm not aware of any EFI that actually requires the
> partition type code to be set correctly; every one I've tried will boot
> fine even if the type code is set to something other than an official
> ESP type code. That said, I'd set the type code correctly just to be
> sure it works on an oddball system, and to avoid confusion if/when users
> look at the disk.
> 
> If the boot medium is an optical disc, you'll need a particular variant
> of an El Torito boot image. If you need help with this, please say so; I
> can dig up the details, or at least point you to a sample mkisofs command.
> 
>> I need to end up with a binary image file (which I can process
>> into an object module and embed into our OS code for initializing
>> disks).
> 
> GPT places data structures at both the beginning and the end of the
> disk, which might create some complications, depending on your exact
> needs. If you need to write an image to a blank disk of unknown size,
> several options occur to me:
> 
> * You can use MBR, which does not rely on data structures at the
>  end of the disk. As noted above, though, that's a little iffy in
>  some cases -- but it might be fine for your case (it's hard to
>  tell without more details).
> * You can prepare a virtual image of a small disk and write it out
>  to a larger disk, leaving the backup GPT data structures before
>  the end of the disk; then either:
>  * Leave it that way, which will effectively limit the size of the
>disk. This might be OK for a USB flash drive.
>  * Use a disk partitioning tool to relocate the backup GPT data
>structures to the end of the disk. The sgdisk "-e" option will
>do this, for instance. IIRC, parted will do it automatically,
>or at least prompt that it be done, if any operation is performed
>on the disk.
> * You can create an image of the FAT32 ESP filesystem ONLY (without
>  partition table), then have your wrapper tool use sgdisk, parted,
>  or some other tool to prepare a disk with GPT and an ESP. You'd
>  then write out the image to the ESP on the disk you've just
>  partitioned.
> 
> Caveat/full disclosure: I mostly use Linux, so I've referenced Linux
> commands. I'm also the author of GPT fdisk (gdisk, sgdisk, and cgdisk);
> but of course, other tools on many platforms can do what you need.
>

Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Ard Biesheuvel

On 15 June 2018 at 16:13, Sami Mujawar  wrote:
> The SATA controller driver crashes while accessing the
> PCI memory [AHCI Base Registers (ABAR)], as the PCI memory
> space is not enabled.
>
> Enable the PCI memory space access to prevent the SATA
> Controller driver from crashing.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar 
> ---
> The changes can be viewed at 
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2
>
> Notes:
> v2:
> - Improved log message and code documentation based on feedback   [SAMI]
> - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG]
> - This SATA Controller driver only uses the PCI BAR5 register
>   space which is the AHCI Base Address (ABAR). According to the
>   'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
>   specification, section 2.1.11, 'This register allocates space
>   for the HBA memory registers'.
>   The section 2.1.10, allows provision for Optional BARs which
>   may support either memory or I/O spaces. However, in the context
>   of the current SATA controller driver, which only ever access
>   the ABAR, enabling I/O memory space is not required.[SAMI]
> - Prefer to use // surrounding comments   [ZENG]
> - Doing this would violate the edk2 coding standard. See EDK2
>   Coding Standard Specification, revision 2.20, section 6.2.3.[SAMI]
>

Please stop obsessing about the coding standard. The maintainer was
quite clear what he wanted, and in the past, I have also indicated
that for the ARM related packages, I prefer readability and
consistency over adherence to a dubious standard.


> v1:
> - Fix SATA Controller driver crash[SAMI]
>
>  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 
> +++-
>  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
>  2 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c 
> b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> index 
> a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8ff885e48e4318
>  100644
> --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> @@ -2,6 +2,7 @@
>This driver module produces IDE_CONTROLLER_INIT protocol for Sata 
> Controllers.
>
>Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2018, ARM Ltd. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
>which accompanies this distribution.  The full text of the license may be 
> found at
> @@ -364,6 +365,7 @@ SataControllerStart (
>EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
>UINT32Data32;
>UINTN TotalCount;
> +  UINT64PciAttributes;
>
>DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
>
> @@ -406,6 +408,61 @@ SataControllerStart (
>Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
>Private->IdeInit.SetTiming  = IdeInitSetTiming;
>Private->IdeInit.EnumAll= SATA_ENUMER_ALL;
> +  Private->PciAttributesChanged   = FALSE;
> +
> +  // Save original PCI attributes
> +  Status = PciIo->Attributes (
> +PciIo,
> +EfiPciIoAttributeOperationGet,
> +0,
> +>OriginalPciAttributes
> +);
> +  if (EFI_ERROR (Status)) {
> +  goto Done;
> +  }
> +
> +  DEBUG ((
> +EFI_D_INFO,
> +"Original PCI Attributes = 0x%llx\n",
> +Private->OriginalPciAttributes
> +));
> +
> +  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +Status = PciIo->Attributes (
> +  PciIo,
> +  EfiPciIoAttributeOperationSupported,
> +  0,
> +  
> +  );
> +if (EFI_ERROR (Status)) {
> +  goto Done;
> +}
> +
> +DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", 
> PciAttributes));
> +
> +if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +  DEBUG ((
> +EFI_D_ERROR,
> +"Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
> +));
> +  Status = EFI_UNSUPPORTED;
> +  goto Done;
> +}
> +
> +PciAttributes = Private->OriginalPciAttributes | 
> EFI_PCI_IO_ATTRIBUTE_MEMORY;
> +
> +DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
> +Status = PciIo->Attributes (
> +  PciIo,
> +  EfiPciIoAttributeOperationEnable,
> +  PciAttributes,
> +  NULL
> +

[edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Sami Mujawar

The SATA controller driver crashes while accessing the
PCI memory [AHCI Base Registers (ABAR)], as the PCI memory
space is not enabled.

Enable the PCI memory space access to prevent the SATA
Controller driver from crashing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
---
The changes can be viewed at 
https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2

Notes:
v2:
- Improved log message and code documentation based on feedback   [SAMI]
- Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE[ZENG]
- This SATA Controller driver only uses the PCI BAR5 register
  space which is the AHCI Base Address (ABAR). According to the
  'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
  specification, section 2.1.11, 'This register allocates space
  for the HBA memory registers'.
  The section 2.1.10, allows provision for Optional BARs which
  may support either memory or I/O spaces. However, in the context
  of the current SATA controller driver, which only ever access
  the ABAR, enabling I/O memory space is not required.[SAMI]
- Prefer to use // surrounding comments   [ZENG]
- Doing this would violate the edk2 coding standard. See EDK2
  Coding Standard Specification, revision 2.20, section 6.2.3.[SAMI]

v1:
- Fix SATA Controller driver crash[SAMI]

 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 
+++-
 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c 
b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index 
a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8ff885e48e4318
 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -2,6 +2,7 @@
   This driver module produces IDE_CONTROLLER_INIT protocol for Sata 
Controllers.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2018, ARM Ltd. All rights reserved.
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -364,6 +365,7 @@ SataControllerStart (
   EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
   UINT32Data32;
   UINTN TotalCount;
+  UINT64PciAttributes;
 
   DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
 
@@ -406,6 +408,61 @@ SataControllerStart (
   Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
   Private->IdeInit.SetTiming  = IdeInitSetTiming;
   Private->IdeInit.EnumAll= SATA_ENUMER_ALL;
+  Private->PciAttributesChanged   = FALSE;
+
+  // Save original PCI attributes
+  Status = PciIo->Attributes (
+PciIo,
+EfiPciIoAttributeOperationGet,
+0,
+>OriginalPciAttributes
+);
+  if (EFI_ERROR (Status)) {
+  goto Done;
+  }
+
+  DEBUG ((
+EFI_D_INFO,
+"Original PCI Attributes = 0x%llx\n",
+Private->OriginalPciAttributes
+));
+
+  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+Status = PciIo->Attributes (
+  PciIo,
+  EfiPciIoAttributeOperationSupported,
+  0,
+  
+  );
+if (EFI_ERROR (Status)) {
+  goto Done;
+}
+
+DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", PciAttributes));
+
+if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+  DEBUG ((
+EFI_D_ERROR,
+"Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
+));
+  Status = EFI_UNSUPPORTED;
+  goto Done;
+}
+
+PciAttributes = Private->OriginalPciAttributes | 
EFI_PCI_IO_ATTRIBUTE_MEMORY;
+
+DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
+Status = PciIo->Attributes (
+  PciIo,
+  EfiPciIoAttributeOperationEnable,
+  PciAttributes,
+  NULL
+  );
+if (EFI_ERROR (Status)) {
+  goto Done;
+}
+Private->PciAttributesChanged = TRUE;
+  }
 
   Status = PciIo->Pci.Read (
 PciIo,
@@ -414,7 +471,10 @@ SataControllerStart (
 sizeof (PciData.Hdr.ClassCode),
 PciData.Hdr.ClassCode
 );
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+ASSERT (FALSE);
+goto Done;
+  }
 
   if (IS_PCI_IDE ()) {
 Private->IdeInit.ChannelCount =

[edk2] [PATCH v1 0/2] ArmPkg/ArmScmiDxe: Fix a bug uncovered with the new SCP firmware

2018-06-15 Thread Girish Pathak

This series fixes a bug in the ArmScmiDxe which was revealed while
testing with the new SCP firmware. 

The changes can be seen at 
https://github.com/girishpathak/edk2/tree/281_scmi_fix_v1 

Girish Pathak (2):
  ArmPkg/ArmScmiDxe: Fix ASSERT error in SCMI DXE
  ArmPkg/ArmScmiDxe: Dynamically allocate buffer for protocol ids

 ArmPkg/Drivers/ArmScmiDxe/Scmi.c|  5 --
 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c | 62 
 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h |  9 +--
 3 files changed, 43 insertions(+), 33 deletions(-)

-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


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

[edk2] [PATCH v1 1/2] ArmPkg/ArmScmiDxe: Fix ASSERT error in SCMI DXE

2018-06-15 Thread Girish Pathak

This change fixes a bug in the SCMI DXE which is observed with the
upcoming release of the SCP firmware.

The PROTOCOL_ID_MASK (0xF) which is used to generate an index in
the ProtocolInitFxns is wrong because protocol ids can be
anywhere in 0x10 - 15 or 0x80 - FF range. This mask generates
the same index for two different protocols e.g. for protocol ids
0x10 and 0x90, which causes duplicate initialization of a protocol
resulting in a failure.

This change removes the use of PROTOCOL_ID_MASK and instead
uses a list of protocol ids and their initialization functions
to identify a supported protocol and initialize it.

Change-Id: Ib004294d2bec853045ed6e811d123832fba555cd
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Girish Pathak 
---
 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c | 35 ++--
 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h |  8 +++--
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c 
b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
index 
2920c6f6f33c5bb8ac00c903a0b199ba5f06f4de..cc68cbc922fcc06bff8f7e0aa8b6bf64a9932874
 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
+++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
@@ -29,13 +29,10 @@
 #include "ScmiDxe.h"
 #include "ScmiPrivate.h"
 
-STATIC CONST SCMI_PROTOCOL_INIT_TABLE ProtocolInitFxns[MAX_PROTOCOLS] = {
-  { ScmiBaseProtocolInit },
-  { NULL },
-  { NULL },
-  { ScmiPerformanceProtocolInit },
-  { ScmiClockProtocolInit },
-  { NULL }
+STATIC CONST SCMI_PROTOCOL_ENTRY Protocols[] = {
+  { SCMI_PROTOCOL_ID_BASE, ScmiBaseProtocolInit },
+  { SCMI_PROTOCOL_ID_PERFORMANCE, ScmiPerformanceProtocolInit },
+  { SCMI_PROTOCOL_ID_CLOCK, ScmiClockProtocolInit }
 };
 
 /** ARM SCMI driver entry point function.
@@ -65,14 +62,14 @@ ArmScmiDxeEntryPoint (
   UINT32  Version;
   UINT32  Index;
   UINT32  NumProtocols;
-  UINT32  ProtocolNo;
+  UINT32  ProtocolIndex = 0;
   UINT8   SupportedList[MAX_PROTOCOLS];
   UINT32  SupportedListSize = sizeof (SupportedList);
 
-  ProtocolNo = SCMI_PROTOCOL_ID_BASE & PROTOCOL_ID_MASK;
-
   // Every SCMI implementation must implement the base protocol.
-  Status = ProtocolInitFxns[ProtocolNo].Init ();
+  ASSERT (Protocols[ProtocolIndex].Id == SCMI_PROTOCOL_ID_BASE);
+
+  Status = Protocols[ProtocolIndex].InitFxn ();
   if (EFI_ERROR (Status)) {
 ASSERT (FALSE);
 return Status;
@@ -123,13 +120,15 @@ ArmScmiDxeEntryPoint (
   }
 
   // Install supported protocol on ImageHandle.
-  for (Index = 0; Index < NumProtocols; Index++) {
-ProtocolNo = SupportedList[Index] & PROTOCOL_ID_MASK;
-if (ProtocolInitFxns[ProtocolNo].Init != NULL) {
-  Status = ProtocolInitFxns[ProtocolNo].Init ();
-  if (EFI_ERROR (Status)) {
-ASSERT (FALSE);
-return Status;
+  while (++ProtocolIndex < ARRAY_SIZE (Protocols)) {
+for (Index = 0; Index < NumProtocols; Index++) {
+  if (Protocols[ProtocolIndex].Id == SupportedList[Index]) {
+Status = Protocols[ProtocolIndex].InitFxn ();
+if (EFI_ERROR (Status)) {
+  ASSERT (FALSE);
+  return Status;
+}
+break;
   }
 }
   }
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h 
b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h
index 
29cdde173659c701116b021a3c437a92b473e4e5..5815e1e78074067760b6f618e248526ee25e59c8
 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h
+++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h
@@ -17,8 +17,9 @@
 #ifndef SCMI_DXE_H_
 #define SCMI_DXE_H_
 
+#include "ScmiPrivate.h"
+
 #define MAX_PROTOCOLS6
-#define PROTOCOL_ID_MASK 0xF
 #define MAX_VENDOR_LEN   SCMI_MAX_STR_LEN
 
 /** Pointer to protocol initialization function.
@@ -35,7 +36,8 @@ EFI_STATUS
   );
 
 typedef struct {
-  SCMI_PROTOCOL_INIT_FXN Init;
-} SCMI_PROTOCOL_INIT_TABLE;
+  SCMI_PROTOCOL_ID Id;// Protocol Id.
+  SCMI_PROTOCOL_INIT_FXN InitFxn; // Protocol init function.
+} SCMI_PROTOCOL_ENTRY;
 
 #endif /* SCMI_DXE_H_ */
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


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

[edk2] [PATCH v1 2/2] ArmPkg/ArmScmiDxe: Dynamically allocate buffer for protocol ids

2018-06-15 Thread Girish Pathak

Dynamically allocate the buffer to receive the SCMI protocol list.
This makes MAX_PROTOCOLS redundant, so it is removed.
It also fixes one minor code alignment issue and removes an unused
macro PROTOCOL_MASK.

Change-Id: Idc5880d4fb7b5c674f5fb7dce1198a7cad0303a7
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Girish Pathak 
---
 ArmPkg/Drivers/ArmScmiDxe/Scmi.c|  5 
 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c | 27 +++-
 ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h |  1 -
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/ArmPkg/Drivers/ArmScmiDxe/Scmi.c b/ArmPkg/Drivers/ArmScmiDxe/Scmi.c
index 
1e279f69cf615428dbb6477b8ac7de3258628df3..d247d3a932fe9f197460a95e9afa88681742e4b4
 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/Scmi.c
+++ b/ArmPkg/Drivers/ArmScmiDxe/Scmi.c
@@ -22,11 +22,6 @@
 
 #include "ScmiPrivate.h"
 
-// SCMI Specification 1.0
-#define  MAX_PROTOCOLS   6
-
-#define  PROTOCOL_MASK 0xF
-
 // Arbitrary timeout value 20ms.
 #define  RESPONSE_TIMEOUT  2
 
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c 
b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
index 
cc68cbc922fcc06bff8f7e0aa8b6bf64a9932874..e7b9feca5e1b565fc031385bfe2f2dc0ca53aab0
 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
+++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c
@@ -63,8 +63,8 @@ ArmScmiDxeEntryPoint (
   UINT32  Index;
   UINT32  NumProtocols;
   UINT32  ProtocolIndex = 0;
-  UINT8   SupportedList[MAX_PROTOCOLS];
-  UINT32  SupportedListSize = sizeof (SupportedList);
+  UINT8   *SupportedList;
+  UINT32  SupportedListSize;
 
   // Every SCMI implementation must implement the base protocol.
   ASSERT (Protocols[ProtocolIndex].Id == SCMI_PROTOCOL_ID_BASE);
@@ -108,13 +108,26 @@ ArmScmiDxeEntryPoint (
 
   ASSERT (NumProtocols != 0);
 
+  SupportedListSize = (NumProtocols * sizeof (*SupportedList));
+
+  Status = gBS->AllocatePool (
+  EfiBootServicesData,
+  SupportedListSize,
+  (VOID**)
+  );
+  if (EFI_ERROR (Status)) {
+ASSERT (FALSE);
+return Status;
+  }
+
   // Get the list of protocols supported by SCP firmware on the platform.
   Status = BaseProtocol->DiscoverListProtocols (
- BaseProtocol,
- ,
- SupportedList
- );
+   BaseProtocol,
+   ,
+   SupportedList
+   );
   if (EFI_ERROR (Status)) {
+gBS->FreePool (SupportedList);
 ASSERT (FALSE);
 return Status;
   }
@@ -133,5 +146,7 @@ ArmScmiDxeEntryPoint (
 }
   }
 
+  gBS->FreePool (SupportedList);
+
   return EFI_SUCCESS;
 }
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h 
b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h
index 
5815e1e78074067760b6f618e248526ee25e59c8..b50a52a01d47efbbccec8c97bf44041c47ff8b38
 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h
+++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h
@@ -19,7 +19,6 @@
 
 #include "ScmiPrivate.h"
 
-#define MAX_PROTOCOLS6
 #define MAX_VENDOR_LEN   SCMI_MAX_STR_LEN
 
 /** Pointer to protocol initialization function.
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'


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

Re: [edk2] [PATCH v4 1/4] MdeModulePkg/CapsuleRuntimeDxe: clean the capsule payload to DRAM

2018-06-15 Thread Yao, Jiewen

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Friday, June 15, 2018 3:52 AM
> To: Zeng, Star 
> Cc: edk2-devel@lists.01.org; leif.lindh...@linaro.org; Yao, Jiewen
> ; Kinney, Michael D 
> Subject: Re: [PATCH v4 1/4] MdeModulePkg/CapsuleRuntimeDxe: clean the
> capsule payload to DRAM
> 
> On 14 June 2018 at 02:54, Zeng, Star  wrote:
> > With 'EFIAPI' removed from IsPersistAcrossResetCapsuleSupported and
> CapsuleCacheWriteBack definitions, Reviewed-by: Star Zeng
> .
> >
> > You can wait a little more time in case Jiewen/Mike has comments.
> >
> 
> Thank you Star.
> 
> I will push these by the end of today unless anyone objects.
> 
> 
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Wednesday, June 13, 2018 4:09 PM
> > To: edk2-devel@lists.01.org
> > Cc: leif.lindh...@linaro.org; Zeng, Star ; Yao, Jiewen
> ; Kinney, Michael D ; Ard
> Biesheuvel 
> > Subject: [PATCH v4 1/4] MdeModulePkg/CapsuleRuntimeDxe: clean the
> capsule payload to DRAM
> >
> > When capsule updates are staged for processing after a warm reboot, they are
> copied into memory with the MMU and caches enabled. When the capsule PEI
> gets around to coalescing the capsule, the MMU and caches may still be 
> disabled,
> and so on architectures where uncached accesses are incoherent with the caches
> (such as ARM and AARCH64), we need to ensure that the data passed into
> UpdateCapsule() is written back to main memory before performing the warm
> reboot.
> >
> > Unfortunately, on ARM, the only type of cache maintenance instructions that
> are suitable for this purpose operate on virtual addresses only, and given 
> that the
> UpdateCapsule() prototype includes the physical address of a linked list of
> scatter/gather data structures that are mapped at an address that is unknown 
> to
> the firmware (and may not even be mapped at all when UpdateCapsule() is
> invoked), we can only perform this cache maintenance at boot time. 
> Fortunately,
> both Windows and Linux only invoke UpdateCapsule() before calling
> ExitBootServices(), so this is not a problem in practice.
> >
> > In the future, we may propose adding a secure firmware service that permits
> performing the cache maintenance at OS runtime, in which case this code may
> be enhanced to call that service if available. For now, we just fail any
> UpdateCapsule() calls performed at OS runtime on ARM.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel 
> > ---
> >  MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c| 77
> 
> >  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c| 51
> +
> >  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf | 14
> +++-
> >  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c  | 33
> ++---
> >  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.h  | 73
> +++
> >  5 files changed, 219 insertions(+), 29 deletions(-)
> >
> > diff --git a/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> > new file mode 100644
> > index ..7e0ca06ce7d0
> > --- /dev/null
> > +++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> > @@ -0,0 +1,77 @@
> > + /** @file
> > +  ARM implementation of architecture specific routines related to
> > + PersistAcrossReset capsules
> > +
> > +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.
> > +
> > +  This program and the accompanying materials are licensed and made
> > + available  under the terms and conditions of the BSD License which
> > + accompanies this  distribution.  The full text of the license may be
> > + found at  http://opensource.org/licenses/bsd-license.php
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> BASIS,
> > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#include "CapsuleService.h"
> > +
> > +#include 
> > +
> > +/**
> > +  Whether the platform supports capsules that persist across reset.
> > +Note that
> > +  some platforms only support such capsules at boot time.
> > +
> > +  @return TRUE  if a PersistAcrossReset capsule may be passed to
> UpdateCapsule()
> > +at this time
> > +  FALSE otherwise
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +IsPersistAcrossResetCapsuleSupported (
> > +  VOID
> > +  )
> > +{
> > +  //
> > +  // ARM requires the capsule payload to be cleaned to the point of
> > +coherency
> > +  // (PoC), but only permits doing so using cache maintenance
> > +instructions that
> > +  // operate on virtual addresses. Since at runtime, we don't know the
> > +virtual
> > +  // addresses of the data structures that make up the scatter/gather
> > +list, we
> > +  // cannot perform the maintenance, and all we can do is

Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Sami Mujawar

Hi Zeng,

Please find my response marked [SAMI] below.

Regards,

Sami Mujawar

-Original Message-
From: Zeng, Star  
Sent: 15 June 2018 10:42 AM
To: Sami Mujawar ; edk2-devel@lists.01.org
Cc: ruiyu...@intel.com; nd ; Stephanie Hughes-Fitt 
; eric.d...@intel.com; 
ard.biesheu...@linaro.org; leif.lindh...@linaro.org; star.z...@intel.com
Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem 
space

Generally, the patch is good to me.
Some comments below.

On 2018/6/14 19:38, Sami Mujawar wrote:
> The SATA controller driver crashes while accessing the PCI memory, as 
> the PCI memory space is not enabled.

The code "accessing the PCI memory" you mentioned here is the AhciReadReg in 
the following code block, right?
[SAMI] Yes.
> 
> Enable the PCI memory space access to prevent the SATA Controller 
> driver from crashing.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar 
> ---
> The changes can be seen at 
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fi
> x_v1
> 
> Notes:
>  v1:
>  - Fix SATA Controller driver crash[SAMI]
> 
>   MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 
> +++-
>   MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
>   2 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c 
> b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> index 
> a6d55c15571728eb3fd572003f383ba7c86635ae..21cc101d693f5adfd9d43f0c21a0
> 96eb59ba73b1 100644
> --- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> +++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
> @@ -2,6 +2,7 @@
> This driver module produces IDE_CONTROLLER_INIT protocol for Sata 
> Controllers.
>   
> Copyright (c) 2011 - 2016, Intel Corporation. All rights 
> reserved.
> +  Copyright (c) 2018, ARM Ltd. All rights reserved.
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD 
> License
> which accompanies this distribution.  The full text of the license 
> may be found at @@ -364,6 +365,7 @@ SataControllerStart (
> EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
> UINT32Data32;
> UINTN TotalCount;
> +  UINT64PciAttributes;
>   
> DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
>   
> @@ -406,6 +408,61 @@ SataControllerStart (
> Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
> Private->IdeInit.SetTiming  = IdeInitSetTiming;
> Private->IdeInit.EnumAll= SATA_ENUMER_ALL;
> +  Private->PciAttributesChanged   = FALSE;
> +
> +  // Save original PCI attributes
> +  Status = PciIo->Attributes (
> +PciIo,
> +EfiPciIoAttributeOperationGet,
> +0,
> +>OriginalPciAttributes
> +);
> +  if (EFI_ERROR (Status)) {
> +  goto Done;
> +  }

Good to me.

> +
> +  DEBUG ((
> +EFI_D_INFO,
> +"PCI Attributes = 0x%llx\n",

How about using "Original PCI Attributes = 0x%llx\n"?
[SAMI] I will submit a patch with this fixed.

> +Private->OriginalPciAttributes
> +));
> +
> +  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +Status = PciIo->Attributes (
> +  PciIo,
> +  EfiPciIoAttributeOperationSupported,
> +  0,
> +  
> +  );
> +if (EFI_ERROR (Status)) {
> +  goto Done;
> +}
> +
> +DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", 
> + PciAttributes));
> +
> +if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
> +  DEBUG ((
> +EFI_D_ERROR,
> +"Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
> +));
> +  Status = EFI_UNSUPPORTED;
> +  goto Done;
> +}
> +
> +PciAttributes = Private->OriginalPciAttributes | 
> + EFI_PCI_IO_ATTRIBUTE_MEMORY;
> +
> +DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
> +Status = PciIo->Attributes (
> +  PciIo,
> +  EfiPciIoAttributeOperationEnable,
> +  PciAttributes,
> +  NULL
> +  );
> +if (EFI_ERROR (Status)) {
> +  goto Done;
> +}

It is the case for enabling memory space, but there may be case to need IO 
space enabling. I suggest to use the code block (same with other device 
drivers).

   Status = PciIo->Attributes (
 PciIo,
 EfiPciIoAttributeOperationSupported,
 0,
 
 );
   if (!EFI_ERROR (Status)) {
 Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
 Status = PciIo->Attributes (

Re: [edk2] Creating EFI System Partition

2018-06-15 Thread Rod Smith

On 06/14/2018 08:35 PM, Robinson, Herbie wrote:
> I have been tasked with implementing EFI boot in our VOS operating
> system (in a panic, nobody bothered to tell us ahead of time that
> legacy boot was being dropped from the BIOS on our latest hardware).
> I think we have a pretty good handle on writing the bootloader, but I
> can't find any solid information on creating an empty EFI System
> Partition bearing the correct flavor of FAT32 to put the bootloader
> in.

The EFI spec has some quirky CYA-type wording about its filesystem, but
AFAIK, any common tool for creating a FAT32 filesystem should work. I
generally do it with mkdosfs in Linux, but equivalent tools in macOS,
Windows, or the BSDs also work. In practice, FAT16 and FAT12 usually
work, too, although the EFI spec does explicitly say at one point that
the filesystem should be FAT32, and I know of at least one
implementation that gets a little flaky with FAT16, so I'd stick with
FAT32. An exception is if you need a very small filesystem, as on a
bootable optical disc, in which case FAT16 or FAT12 might be required.
Also, I've seen reports of problems with filesystems smaller than 512MiB
on some EFIs, so I recommend making it at least that large, at least if
it will be on a hard disk.

Ideally, the EFI System Partition (ESP) should be identified by the
correct partition table type code. On an MBR disk, this is 0xEF; and on
a GPT disk, it's C12A7328-F81F-11D2-BA4B-00A0C93EC93B, which is
identified in various ways depending on the partitioning software (type
EF00 in gdisk; a "boot flag" or "esp flag" set in parted; etc.). GPT is
preferable, particularly for installations on hard disks, since some
OSes tie the boot mode to the partition table type. (Of course, this
might not be an issue for you -- it depends on what you're preparing.)
In practice, I'm not aware of any EFI that actually requires the
partition type code to be set correctly; every one I've tried will boot
fine even if the type code is set to something other than an official
ESP type code. That said, I'd set the type code correctly just to be
sure it works on an oddball system, and to avoid confusion if/when users
look at the disk.

If the boot medium is an optical disc, you'll need a particular variant
of an El Torito boot image. If you need help with this, please say so; I
can dig up the details, or at least point you to a sample mkisofs command.

> I need to end up with a binary image file (which I can process
> into an object module and embed into our OS code for initializing
> disks).

GPT places data structures at both the beginning and the end of the
disk, which might create some complications, depending on your exact
needs. If you need to write an image to a blank disk of unknown size,
several options occur to me:

* You can use MBR, which does not rely on data structures at the
  end of the disk. As noted above, though, that's a little iffy in
  some cases -- but it might be fine for your case (it's hard to
  tell without more details).
* You can prepare a virtual image of a small disk and write it out
  to a larger disk, leaving the backup GPT data structures before
  the end of the disk; then either:
  * Leave it that way, which will effectively limit the size of the
disk. This might be OK for a USB flash drive.
  * Use a disk partitioning tool to relocate the backup GPT data
structures to the end of the disk. The sgdisk "-e" option will
do this, for instance. IIRC, parted will do it automatically,
or at least prompt that it be done, if any operation is performed
on the disk.
* You can create an image of the FAT32 ESP filesystem ONLY (without
  partition table), then have your wrapper tool use sgdisk, parted,
  or some other tool to prepare a disk with GPT and an ESP. You'd
  then write out the image to the ESP on the disk you've just
  partitioned.

Caveat/full disclosure: I mostly use Linux, so I've referenced Linux
commands. I'm also the author of GPT fdisk (gdisk, sgdisk, and cgdisk);
but of course, other tools on many platforms can do what you need.

> I found a thread on submitting a "GPT" EFI shell application on this
> list, but it seems to have trailed off to nowhere about two years
> ago.
> 
> Did that end up somewhere that I haven't found?
> 
> Herbie Robinson Software Architect Stratus Technologies |
> www.stratus.com 5 Mill and Main Place, Suite 500 | Maynard, MA 01754 
> T: +1-978-461-7531 | E: herbie.robin...@stratus.com [Stratus
> Technologies]
> 
> ___ edk2-devel mailing
> list edk2-devel@lists.01.org 
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

-- 
Rod Smith
rodsm...@rodsbooks.com
http://www.rodsbooks.com
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms v2 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

2018-06-15 Thread Ard Biesheuvel

On 15 June 2018 at 14:00, Chandni Cherukuri  wrote:
> Use the platform ID and config ID values from the platform ID
> HOB to choose the right set of ACPI tables to be installed.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 
> +---
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
>  Platform/ARM/SgiPkg/SgiPlatform.dsc |  1 +
>  Platform/ARM/SgiPkg/SgiPlatform.fdf |  1 +
>  4 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> index edaae5b..f077a2c 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -14,6 +14,8 @@
>
>  #include 
>  #include 
> +#include 
> +#include 
>
>  EFI_STATUS
>  InitVirtioBlockIo (
> @@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
>)
>  {
>EFI_STATUS  Status;
> +  VOID*PlatformIdHob;
> +  SGI_PLATFORM_DESCRIPTOR *HobData;
> +  UINT32  ConfigId;
> +  UINT32  PartNum;
>
> -  Status = LocateAndInstallAcpiFromFv ();
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
> __FUNCTION__));
> -return Status;
> +  PlatformIdHob = GetFirstGuidHob ();
> +  if (PlatformIdHob == NULL) {
> +DEBUG ((DEBUG_ERROR, "Platform ID HOB is NULL\n"));
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
> +
> +  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
> +  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
> +
> +  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
> +Status = LocateAndInstallAcpiFromFv ();
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
> __FUNCTION__));
> +  return Status;
> +}
> +  } else {
> +DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
> +return EFI_UNSUPPORTED;
>}
>


> -  Status = EFI_REQUEST_UNLOAD_IMAGE;
>if (FeaturePcdGet (PcdVirtioSupported)) {
>  Status = InitVirtioBlockIo (ImageHandle);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
>  __FUNCTION__));
>  }
> +  } else {
> +Status = EFI_REQUEST_UNLOAD_IMAGE;
>}
>

This is an unrelated (and unnecessary) change.


>return Status;
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index 51ad22f..b6b8209 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -30,10 +30,12 @@
>
>  [LibraryClasses]
>AcpiLib
> +  HobLib
>UefiDriverEntryPoint
>VirtioMmioDeviceLib
>
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>gSgi575AcpiTablesiFileGuid
>
>  [FeaturePcd]
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
> b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index a56175e..7b8e051 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -199,6 +199,7 @@
>  
>
> NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>}
> +  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
>
>#
># DXE
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf 
> b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> index 17cdf48..0e5739e 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
> @@ -220,6 +220,7 @@ READ_LOCK_STATUS   = TRUE
>INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
>INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
>INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> +  INF Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
>
>FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
>  SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED 
> = TRUE {
> --
> 2.7.4
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms v2 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread Ard Biesheuvel

On 15 June 2018 at 14:00, Chandni Cherukuri  wrote:
> On SGI platforms, the trusted firmware executes prior to
> the SEC phase. It supplies to the SEC phase a pointer to
> a fdt, that contains platform specific information such as
> part number and config number of the SGI platform. The
> platform code that executes during the SEC phase creates a
> PPI that allows access to other PEIMs to obtain a copy of the
> fdt pointer.
>
> Further, during the PEI phase, a Platform ID PEIM installs a
> Platform ID HOB. The Platform ID HOB can be used by DXE
> drivers to identify the platform it is executing on.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h   |  23 
>  Platform/ARM/SgiPkg/Include/SgiPlatform.h |  13 +++
>  Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S  |  13 ++-
>  Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c |  10 ++
>  Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |   3 +
>  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |  40 +++
>  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  | 112 
> 
>  Platform/ARM/SgiPkg/SgiPlatform.dec   |   5 +
>  8 files changed, 215 insertions(+), 4 deletions(-)
>
> diff --git a/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h 
> b/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
> new file mode 100644
> index 000..5ffc69d
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
> @@ -0,0 +1,23 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution. The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#ifndef  SGI_PLATFORMID_PPI_
> +#define  SGI_PLATFORMID_PPI_
> +
> +//HwConfig DT structure
> +typedef struct {
> +  UINT64  *HwConfigDtAddr;
> +} SGI_HW_CONFIG_INFO_PPI;
> +
> +#endif
> diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h 
> b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> index 00ca7e9..1454018 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> @@ -68,4 +68,17 @@
>  #define SGI_SYSPH_SYS_REG_FLASH   0x4C
>  #define SGI_SYSPH_SYS_REG_FLASH_RWEN  0x1
>
> +// SGI575_VERSION values
> +#define SGI575_CONF_NUM   0x3
> +#define SGI575_PART_NUM   0x783
> +
> +#define SGI_CONFIG_MASK   0x0F
> +#define SGI_CONFIG_SHIFT  0x1C
> +#define SGI_PART_NUM_MASK 0xFFF
> +
> +// ARM platform description data.
> +typedef struct {
> +  UINTN  PlatformId;
> +} SGI_PLATFORM_DESCRIPTOR;
> +
>  #endif // __SGI_PLATFORM_H__
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> index dab6c77..3662266 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> @@ -22,15 +22,20 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> +GCC_ASM_IMPORT(HwConfigDtBlob)
>
>  //
>  // First platform specific function to be called in the PEI phase
>  //
> -// This function is actually the first function called by the PrePi
> -// or PrePeiCore modules. It allows to retrieve arguments passed to
> -// the UEFI firmware through the CPU registers.
> -//
> +// The trusted firmware passed the hw config DT blob in x1 register.
> +// Keep a copy of it in a global variable.
> +//VOID
> +//ArmPlatformPeiBootAction (
> +//  VOID
> +//  );
>  ASM_PFX(ArmPlatformPeiBootAction):
> +  adr  x10, HwConfigDtBlob
> +  str  x1, [x10]
>ret
>
>  //UINTN
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> index ea3201a..c9ddbfb 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> @@ -15,6 +15,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +UINT64 HwConfigDtBlob;
> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>
>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>{
> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>IN  UINTN MpId
>)
>  {
> +  mHwConfigDtInfoPpi.HwConfigDtAddr = 
>return

Re: [edk2] [PATCH edk2-platforms v2 0/2] ARM Dynamic Configuration

2018-06-15 Thread Leif Lindholm

On Fri, Jun 15, 2018 at 05:30:00PM +0530, Chandni Cherukuri wrote:
> On SGI platforms, the trusted firmware executes prior to the SEC
> phase. It supplies to the SEC phase, a pointer to a HW CONFIG fdt
> in the x1 which contains platform specific information such as part
> number and config number of the SGI platform. 
> 
> The HW CONFIG FDT would look as,
> /dts-v1/;
> / {
> /* compatible string */
> compatible = "arm,sgi-isys3";
> 
>   /* platform ID node */
>   system-id {
>platform-id = <0x03000783>;
>   }
> };
> 
> In the very first step of the assembly code which executes in SEC phase
> the fdt pointer is stored in a global variable from the register. A PPI 
> is created during the SEC phase which stores the global variable.
> 
> During PEI phase, a Platform ID PEIM is installed which accessess the PPI
> and retrieves the platform information using FDT helper functions and a
> PlatformId HOB is created and populated with the information.
>  
> During DXE phase the drivers can access the Platform ID HOB to get the
> platform information and perform platform specific functions based on the
> platform.

It's helpful for subsequent revisions to keep a revision history in
cover letter.

Changes since v1:
- ...
- ...
- ...

Kept around and added to for subsequent revisions.

However, you've addressed all of my comments on v1, so for my part -
for the series:
Reviewed-by: Leif Lindholm 

But we want to hear back from Ard as well.

/
Leif

> Chandni Cherukuri (2):
>   Platform/ARM/Sgi: Install a Platform ID HOB
>   Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID
> 
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c |  33 +-
>  Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf   |   2 +
>  Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h   |  23 
>  Platform/ARM/SgiPkg/Include/SgiPlatform.h |  13 +++
>  Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S  |  13 ++-
>  Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c |  10 ++
>  Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |   3 +
>  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |  40 +++
>  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  | 112 
> 
>  Platform/ARM/SgiPkg/SgiPlatform.dec   |   5 +
>  Platform/ARM/SgiPkg/SgiPlatform.dsc   |   1 +
>  Platform/ARM/SgiPkg/SgiPlatform.fdf   |   1 +
>  12 files changed, 247 insertions(+), 9 deletions(-)
>  create mode 100644 Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
>  create mode 100644 
> Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
>  create mode 100644 
> Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c
> 
> -- 
> 2.7.4
> 
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread chandni cherukuri

On Fri, Jun 15, 2018 at 3:48 PM, Leif Lindholm  wrote:
>
> Just some minor formatting points in addition to the comment on Ard's
> reply..
>
> On Fri, Jun 15, 2018 at 11:25:31AM +0530, Chandni Cherukuri wrote:
> > On SGI platforms, the trusted firmware executes prior to
> > the SEC phase. It supplies to the SEC phase a pointer to
> > a fdt, that contains platform specific information such as
> > part number and config number of the SGI platform. The
> > platform code that executes during the SEC phase creates a
> > PPI that allows access to other PEIMs to obtain a copy of the
> > fdt pointer.
> >
> > Further, during the PEI phase, a Platform ID PEIM installs a
> > Platform ID HOB. The Platform ID HOB can be used by DXE
> > drivers to identify the platform it is executing on.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Chandni Cherukuri 
> > ---
> >  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S|  15 ++-
> >  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
> >  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
> >  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 
> > +
> >  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf|  40 
> >  Platform/ARM/SgiPkg/SgiPlatform.dec|   5 +
> >  6 files changed, 177 insertions(+), 4 deletions(-)
> >  create mode 100644 
> > Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> >  create mode 100644 
> > Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> >
> > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S 
> > b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> > index dab6c77..80b93ea 100644
> > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> > @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
> >  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
> >  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
> >  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> > +GCC_ASM_EXPORT(HwConfigDtBlob)
> > +
> > +HwConfigDtBlob: .long 0
> >
> >  //
> >  // First platform specific function to be called in the PEI phase
> >  //
> > -// This function is actually the first function called by the PrePi
> > -// or PrePeiCore modules. It allows to retrieve arguments passed to
> > -// the UEFI firmware through the CPU registers.
> > -//
> > +// The trusted firmware passed the hw config DT blob in x1 register.
> > +// Keep a copy of it in a global variable.
> > +//VOID
> > +//ArmPlatformPeiBootAction (
> > +//  VOID
> > +//  );
> >  ASM_PFX(ArmPlatformPeiBootAction):
> > +  ldr  x10, =HwConfigDtBlob
> > +  str  x1, [x10]
> >ret
> >
> >  //UINTN
> > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
> > b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> > index ea3201a..54860e0 100644
> > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> > @@ -15,6 +15,10 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +
> > +extern UINT64 HwConfigDtBlob;
> > +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
> >
> >  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
> >{
> > @@ -36,6 +40,7 @@ ArmPlatformInitialize (
> >IN  UINTN MpId
> >)
> >  {
> > +  mHwConfigDtInfoPpi.HwConfigDtAddr = 
> >return RETURN_SUCCESS;
> >  }
> >
> > @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
> >  EFI_PEI_PPI_DESCRIPTOR_PPI,
> >  ,
> >  
> > +  },
> > +  {
> > +EFI_PEI_PPI_DESCRIPTOR_PPI,
> > +,
> > +
> >}
> >  };
> >
> > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf 
> > b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> > index 42e14d5..5d4bf72 100644
> > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> > @@ -61,7 +61,10 @@
> >gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> >
> >  [Guids]
> > +  gArmSgiPlatformIdDescriptorGuid
> >gEfiHobListGuid  ## CONSUMES  ## SystemTable
> > +  gFdtTableGuid
> >
> >  [Ppis]
> >gArmMpCoreInfoPpiGuid
> > +  gHwConfigDtInfoPpiGuid
> > diff --git 
> > a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c 
> > b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> > new file mode 100644
> > index 000..f6446e6
> > --- /dev/null
> > +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> > @@ -0,0 +1,108 @@
> > +/** @file
> > +*
> > +*  Copyright (c) 2018, ARM Limited. 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
> > +*

[edk2] FMP Capsule vs PMCI PLDM FW Update binaries

2018-06-15 Thread TVKR

Hi,

The DMTF group is coming up with a new way to update FW on PCI devices via
BMC - DSP0267. The method expects the FW binary to carry a pre-defined
header that makes it easier for BMC to perform the necessary actions.
Currently some of the OEM/ODMs are already releasing FW payloads that can
be flashed via FMP. Additionally under certain circumstances they may also
release capsules targeting FMP for firmware updates. Now, if they need to
support this new FW update method via BMC will they end up releasing
separate binaries to do the same update - one that is compliant with PLDM
spec and the other compliant with FMP capsule? Is there a way to avoid
this? Or are the OEM/ODM expected to release bulky FW packages that contain
multiple binaries to update the same type of FW.

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

[edk2] [PATCH edk2-platforms v2 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

2018-06-15 Thread Chandni Cherukuri

Use the platform ID and config ID values from the platform ID
HOB to choose the right set of ACPI tables to be installed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chandni Cherukuri 
---
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 
+---
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
 Platform/ARM/SgiPkg/SgiPlatform.dsc |  1 +
 Platform/ARM/SgiPkg/SgiPlatform.fdf |  1 +
 4 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c 
b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
index edaae5b..f077a2c 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
@@ -14,6 +14,8 @@
 
 #include 
 #include 
+#include 
+#include 
 
 EFI_STATUS
 InitVirtioBlockIo (
@@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
   )
 {
   EFI_STATUS  Status;
+  VOID*PlatformIdHob;
+  SGI_PLATFORM_DESCRIPTOR *HobData;
+  UINT32  ConfigId;
+  UINT32  PartNum;
 
-  Status = LocateAndInstallAcpiFromFv ();
-  if (EFI_ERROR (Status)) {
-DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", __FUNCTION__));
-return Status;
+  PlatformIdHob = GetFirstGuidHob ();
+  if (PlatformIdHob == NULL) {
+DEBUG ((DEBUG_ERROR, "Platform ID HOB is NULL\n"));
+return EFI_INVALID_PARAMETER;
+  }
+
+  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
+
+  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
+  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
+
+  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
+Status = LocateAndInstallAcpiFromFv ();
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
__FUNCTION__));
+  return Status;
+}
+  } else {
+DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
+return EFI_UNSUPPORTED;
   }
 
-  Status = EFI_REQUEST_UNLOAD_IMAGE;
   if (FeaturePcdGet (PcdVirtioSupported)) {
 Status = InitVirtioBlockIo (ImageHandle);
 if (EFI_ERROR (Status)) {
   DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
 __FUNCTION__));
 }
+  } else {
+Status = EFI_REQUEST_UNLOAD_IMAGE;
   }
 
   return Status;
diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
index 51ad22f..b6b8209 100644
--- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
+++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
@@ -30,10 +30,12 @@
 
 [LibraryClasses]
   AcpiLib
+  HobLib
   UefiDriverEntryPoint
   VirtioMmioDeviceLib
 
 [Guids]
+  gArmSgiPlatformIdDescriptorGuid
   gSgi575AcpiTablesiFileGuid
 
 [FeaturePcd]
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
b/Platform/ARM/SgiPkg/SgiPlatform.dsc
index a56175e..7b8e051 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
@@ -199,6 +199,7 @@
 
   
NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
   }
+  Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
 
   #
   # DXE
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf 
b/Platform/ARM/SgiPkg/SgiPlatform.fdf
index 17cdf48..0e5739e 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
+++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
@@ -220,6 +220,7 @@ READ_LOCK_STATUS   = TRUE
   INF MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf
   INF MdeModulePkg/Universal/PCD/Pei/Pcd.inf
   INF MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
+  INF Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
 
   FILE FV_IMAGE = 9E21FD93-9C72-4c15-8C4B-E77F1DB2D792 {
 SECTION GUIDED EE4E5898-3914-4259-9D6E-DC7BD79403CF PROCESSING_REQUIRED = 
TRUE {
-- 
2.7.4

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

[edk2] [PATCH edk2-platforms v2 0/2] ARM Dynamic Configuration

2018-06-15 Thread Chandni Cherukuri

On SGI platforms, the trusted firmware executes prior to the SEC
phase. It supplies to the SEC phase, a pointer to a HW CONFIG fdt
in the x1 which contains platform specific information such as part
number and config number of the SGI platform. 

The HW CONFIG FDT would look as,
/dts-v1/;
/ {
/* compatible string */
compatible = "arm,sgi-isys3";

/* platform ID node */
system-id {
 platform-id = <0x03000783>;
}
};

In the very first step of the assembly code which executes in SEC phase
the fdt pointer is stored in a global variable from the register. A PPI 
is created during the SEC phase which stores the global variable.

During PEI phase, a Platform ID PEIM is installed which accessess the PPI
and retrieves the platform information using FDT helper functions and a
PlatformId HOB is created and populated with the information.
 
During DXE phase the drivers can access the Platform ID HOB to get the
platform information and perform platform specific functions based on the
platform.

Chandni Cherukuri (2):
  Platform/ARM/Sgi: Install a Platform ID HOB
  Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c |  33 +-
 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf   |   2 +
 Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h   |  23 
 Platform/ARM/SgiPkg/Include/SgiPlatform.h |  13 +++
 Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S  |  13 ++-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c |  10 ++
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |   3 +
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |  40 +++
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  | 112 

 Platform/ARM/SgiPkg/SgiPlatform.dec   |   5 +
 Platform/ARM/SgiPkg/SgiPlatform.dsc   |   1 +
 Platform/ARM/SgiPkg/SgiPlatform.fdf   |   1 +
 12 files changed, 247 insertions(+), 9 deletions(-)
 create mode 100644 Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
 create mode 100644 
Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf
 create mode 100644 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c

-- 
2.7.4


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

[edk2] [PATCH edk2-platforms v2 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread Chandni Cherukuri

On SGI platforms, the trusted firmware executes prior to
the SEC phase. It supplies to the SEC phase a pointer to
a fdt, that contains platform specific information such as
part number and config number of the SGI platform. The
platform code that executes during the SEC phase creates a
PPI that allows access to other PEIMs to obtain a copy of the
fdt pointer.

Further, during the PEI phase, a Platform ID PEIM installs a
Platform ID HOB. The Platform ID HOB can be used by DXE
drivers to identify the platform it is executing on.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chandni Cherukuri 
---
 Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h   |  23 
 Platform/ARM/SgiPkg/Include/SgiPlatform.h |  13 +++
 Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S  |  13 ++-
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c |  10 ++
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf   |   3 +
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPei.inf |  40 +++
 Platform/ARM/SgiPkg/Library/SgiPlatformPei/SgiPlatformPeim.c  | 112 

 Platform/ARM/SgiPkg/SgiPlatform.dec   |   5 +
 8 files changed, 215 insertions(+), 4 deletions(-)

diff --git a/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h 
b/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
new file mode 100644
index 000..5ffc69d
--- /dev/null
+++ b/Platform/ARM/SgiPkg/Include/Ppi/SgiPlatformId.h
@@ -0,0 +1,23 @@
+/** @file
+*
+*  Copyright (c) 2018, ARM Limited. All rights reserved.
+*
+*  This program and the accompanying materials are licensed and made available
+*  under the terms and conditions of the BSD License which accompanies this
+*  distribution. The full text of the license may be found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+*
+**/
+
+#ifndef  SGI_PLATFORMID_PPI_
+#define  SGI_PLATFORMID_PPI_
+
+//HwConfig DT structure
+typedef struct {
+  UINT64  *HwConfigDtAddr;
+} SGI_HW_CONFIG_INFO_PPI;
+
+#endif
diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h 
b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
index 00ca7e9..1454018 100644
--- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
+++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
@@ -68,4 +68,17 @@
 #define SGI_SYSPH_SYS_REG_FLASH   0x4C
 #define SGI_SYSPH_SYS_REG_FLASH_RWEN  0x1
 
+// SGI575_VERSION values
+#define SGI575_CONF_NUM   0x3
+#define SGI575_PART_NUM   0x783
+
+#define SGI_CONFIG_MASK   0x0F
+#define SGI_CONFIG_SHIFT  0x1C
+#define SGI_PART_NUM_MASK 0xFFF
+
+// ARM platform description data.
+typedef struct {
+  UINTN  PlatformId;
+} SGI_PLATFORM_DESCRIPTOR;
+
 #endif // __SGI_PLATFORM_H__
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S 
b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
index dab6c77..3662266 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
@@ -22,15 +22,20 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
 GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
 GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
 GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
+GCC_ASM_IMPORT(HwConfigDtBlob)
 
 //
 // First platform specific function to be called in the PEI phase
 //
-// This function is actually the first function called by the PrePi
-// or PrePeiCore modules. It allows to retrieve arguments passed to
-// the UEFI firmware through the CPU registers.
-//
+// The trusted firmware passed the hw config DT blob in x1 register.
+// Keep a copy of it in a global variable.
+//VOID
+//ArmPlatformPeiBootAction (
+//  VOID
+//  );
 ASM_PFX(ArmPlatformPeiBootAction):
+  adr  x10, HwConfigDtBlob
+  str  x1, [x10]
   ret
 
 //UINTN
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
index ea3201a..c9ddbfb 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
@@ -15,6 +15,10 @@
 #include 
 #include 
 #include 
+#include 
+
+UINT64 HwConfigDtBlob;
+STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
 
 STATIC ARM_CORE_INFO mCoreInfoTable[] = {
   {
@@ -36,6 +40,7 @@ ArmPlatformInitialize (
   IN  UINTN MpId
   )
 {
+  mHwConfigDtInfoPpi.HwConfigDtAddr = 
   return RETURN_SUCCESS;
 }
 
@@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
 EFI_PEI_PPI_DESCRIPTOR_PPI,
 ,
 
+  },
+  {
+EFI_PEI_PPI_DESCRIPTOR_PPI,
+,
+
   }
 };
 
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf

Re: [edk2] [PATCH v3 2/2] ArmPlatformPkg: Include PL011UartClock Lib

2018-06-15 Thread Ard Biesheuvel

On 12 June 2018 at 22:14, Udit Kumar  wrote:
> This patch gets PL011 baud rate clock from
> pl011 uart clock lib instead of Pcd.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Udit Kumar 

Reviewed-by: Ard Biesheuvel 

Pushed as 112c6c22376267a79f4a4ac0c4263bf24a548d81

> ---
>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c   | 5 +++--
>  ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c 
> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> index 6aa8063..212991d 100644
> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.c
> @@ -19,6 +19,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -48,7 +49,7 @@ SerialPortInitialize (
>
>return PL011UartInitializePort (
> (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
> -   FixedPcdGet32 (PL011UartClkInHz),
> +   PL011UartClockGetFreq(),
> ,
> ,
> ,
> @@ -156,7 +157,7 @@ SerialPortSetAttributes (
>  {
>return PL011UartInitializePort (
> (UINTN)FixedPcdGet64 (PcdSerialRegisterBase),
> -   FixedPcdGet32 (PL011UartClkInHz),
> +   PL011UartClockGetFreq(),
> BaudRate,
> ReceiveFifoDepth,
> Parity,
> diff --git a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf 
> b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
> index 3683e06..5ce5b2f 100644
> --- a/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
> +++ b/ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
> @@ -26,6 +26,7 @@
>PL011SerialPortLib.c
>
>  [LibraryClasses]
> +  PL011UartClockLib
>PL011UartLib
>PcdLib
>
> --
> 1.9.1
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 1/4] MdeModulePkg/CapsuleRuntimeDxe: clean the capsule payload to DRAM

2018-06-15 Thread Ard Biesheuvel

On 14 June 2018 at 02:54, Zeng, Star  wrote:
> With 'EFIAPI' removed from IsPersistAcrossResetCapsuleSupported and 
> CapsuleCacheWriteBack definitions, Reviewed-by: Star Zeng 
> .
>
> You can wait a little more time in case Jiewen/Mike has comments.
>

Thank you Star.

I will push these by the end of today unless anyone objects.


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Wednesday, June 13, 2018 4:09 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindh...@linaro.org; Zeng, Star ; Yao, Jiewen 
> ; Kinney, Michael D ; Ard 
> Biesheuvel 
> Subject: [PATCH v4 1/4] MdeModulePkg/CapsuleRuntimeDxe: clean the capsule 
> payload to DRAM
>
> When capsule updates are staged for processing after a warm reboot, they are 
> copied into memory with the MMU and caches enabled. When the capsule PEI gets 
> around to coalescing the capsule, the MMU and caches may still be disabled, 
> and so on architectures where uncached accesses are incoherent with the 
> caches (such as ARM and AARCH64), we need to ensure that the data passed into 
> UpdateCapsule() is written back to main memory before performing the warm 
> reboot.
>
> Unfortunately, on ARM, the only type of cache maintenance instructions that 
> are suitable for this purpose operate on virtual addresses only, and given 
> that the UpdateCapsule() prototype includes the physical address of a linked 
> list of scatter/gather data structures that are mapped at an address that is 
> unknown to the firmware (and may not even be mapped at all when 
> UpdateCapsule() is invoked), we can only perform this cache maintenance at 
> boot time. Fortunately, both Windows and Linux only invoke UpdateCapsule() 
> before calling ExitBootServices(), so this is not a problem in practice.
>
> In the future, we may propose adding a secure firmware service that permits 
> performing the cache maintenance at OS runtime, in which case this code may 
> be enhanced to call that service if available. For now, we just fail any 
> UpdateCapsule() calls performed at OS runtime on ARM.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c| 77 
> 
>  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleReset.c| 51 
> +
>  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleRuntimeDxe.inf | 14 +++-
>  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c  | 33 ++---
>  MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.h  | 73 
> +++
>  5 files changed, 219 insertions(+), 29 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c 
> b/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> new file mode 100644
> index ..7e0ca06ce7d0
> --- /dev/null
> +++ b/MdeModulePkg/Universal/CapsuleRuntimeDxe/Arm/CapsuleReset.c
> @@ -0,0 +1,77 @@
> + /** @file
> +  ARM implementation of architecture specific routines related to
> + PersistAcrossReset capsules
> +
> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.
> +
> +  This program and the accompanying materials are licensed and made
> + available  under the terms and conditions of the BSD License which
> + accompanies this  distribution.  The full text of the license may be
> + found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#include "CapsuleService.h"
> +
> +#include 
> +
> +/**
> +  Whether the platform supports capsules that persist across reset.
> +Note that
> +  some platforms only support such capsules at boot time.
> +
> +  @return TRUE  if a PersistAcrossReset capsule may be passed to 
> UpdateCapsule()
> +at this time
> +  FALSE otherwise
> +**/
> +BOOLEAN
> +EFIAPI
> +IsPersistAcrossResetCapsuleSupported (
> +  VOID
> +  )
> +{
> +  //
> +  // ARM requires the capsule payload to be cleaned to the point of
> +coherency
> +  // (PoC), but only permits doing so using cache maintenance
> +instructions that
> +  // operate on virtual addresses. Since at runtime, we don't know the
> +virtual
> +  // addresses of the data structures that make up the scatter/gather
> +list, we
> +  // cannot perform the maintenance, and all we can do is give up.
> +  //
> +  return FeaturePcdGet (PcdSupportUpdateCapsuleReset) && !EfiAtRuntime
> +(); }
> +
> +/**
> +  Writes Back a range of data cache lines covering a set of capsules in 
> memory.
> +
> +  Writes Back the data cache lines specified by ScatterGatherList.
> +
> +  @param  ScatterGatherList Physical address of the data structure that
> +describes a set of capsules in memory
> +
> +**/
> +VOID
> +EFIAPI
> +CapsuleCacheWriteBack (
> +  IN  EFI_PHYSICAL_ADDRESSScatterGatherList
> +

Re: [edk2] [PATCH edk2-platforms v2 0/2]DeveloperBox: prepare for expanding the capsule payload

2018-06-15 Thread Ard Biesheuvel

On 13 June 2018 at 20:56, Leif Lindholm  wrote:
> On Wed, Jun 13, 2018 at 06:28:24PM +0200, Ard Biesheuvel wrote:
>> We intend to start distributing firmware update capsules that include the SCP
>> firmware partition. In order to allow for more flexibility regarding whether
>> a capsule contains this piece or not, update the flash access routines and
>> the flash partition descriptions so we can update any part of the first
>> 4 MB, consisting of the CM3 bootstrap code, emergency flasher, pseudo
>> EEPROM, SCP firmware, ARM-TF and UEFI.
>>
>> Note that this means we can drop the 'TEMPORARY' patch I proposed a while
>> ago to hack around this limitation.
>>
>> Changes since v1:
>> - use more parentheses in if () condition expression (#1)
>> - use symbolic constants for code and env NOR regions (#2)
>
> For the series:
> Reviewed-by: Leif Lindholm 
>

Thanks

Pushed as 1cd14dea5fc1..fff2e99a5ee6

>
>> Ard Biesheuvel (2):
>>   Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check
>>   Silicon/NorFlashSynQuacerLib: describe entire firmware region as FV
>>
>>  .../NorFlashSynQuacerLib/NorFlashSynQuacer.c  | 18 --
>>  .../SynQuacerPlatformFlashAccessLib.c | 56 +--
>>  2 files changed, 39 insertions(+), 35 deletions(-)
>>
>> --
>> 2.17.1
>>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

2018-06-15 Thread Leif Lindholm

On Fri, Jun 15, 2018 at 11:25:32AM +0530, Chandni Cherukuri wrote:
> Use the platform ID and config ID values from the platform ID
> HOB to choose the right set of ACPI tables to be installed.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 
> ++
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
>  Platform/ARM/SgiPkg/Include/SgiPlatform.h  | 24 ++--
>  Platform/ARM/SgiPkg/SgiPlatform.dsc|  1 +
>  Platform/ARM/SgiPkg/SgiPlatform.fdf|  1 +
>  5 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> index edaae5b..d0d5808 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -14,6 +14,8 @@
>  
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  EFI_STATUS
>  InitVirtioBlockIo (
> @@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
>)
>  {
>EFI_STATUS  Status;
> +  VOID*PlatformIdHob;
> +  SGI_PLATFORM_DESCRIPTOR *HobData;
> +  UINT32  ConfigId;
> +  UINT32  PartNum;
>  
> -  Status = LocateAndInstallAcpiFromFv ();
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
> __FUNCTION__));
> -return Status;
> +  PlatformIdHob = GetFirstGuidHob ();
> +  if (PlatformIdHob == NULL) {
> +DEBUG ((DEBUG_ERROR, "Platform ID Hob is NULL\n"));

HOB

> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
> +
> +  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
> +  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
> +
> +  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
> +Status = LocateAndInstallAcpiFromFv ();
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
> __FUNCTION__));
> +  return Status;
> +}
> +  } else {
> +DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
> +return EFI_UNSUPPORTED;
>}
>  
> -  Status = EFI_REQUEST_UNLOAD_IMAGE;
>if (FeaturePcdGet (PcdVirtioSupported)) {
>  Status = InitVirtioBlockIo (ImageHandle);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
>  __FUNCTION__));
>  }
> +  } else {
> +Status = EFI_REQUEST_UNLOAD_IMAGE;
>}
>  
>return Status;
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index 51ad22f..b6b8209 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -30,10 +30,12 @@
>  
>  [LibraryClasses]
>AcpiLib
> +  HobLib
>UefiDriverEntryPoint
>VirtioMmioDeviceLib
>  
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>gSgi575AcpiTablesiFileGuid
>  
>  [FeaturePcd]
> diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h 
> b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> index 00ca7e9..756954c 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> @@ -12,8 +12,8 @@
>  *
>  **/
>  
> -#ifndef __SGI_PLATFORM_H__
> -#define __SGI_PLATFORM_H__
> +#ifndef SGI_PLATFORM_H_
> +#define SGI_PLATFORM_H_

Non-functional change. Please submit as separate change or not at all.

/
Leif

>  
>  
> /***
>  // Platform Memory Map
> @@ -68,4 +68,22 @@
>  #define SGI_SYSPH_SYS_REG_FLASH   0x4C
>  #define SGI_SYSPH_SYS_REG_FLASH_RWEN  0x1
>  
> -#endif // __SGI_PLATFORM_H__
> +// SGI575_VERSION values
> +#define SGI575_CONF_NUM   0x3
> +#define SGI575_PART_NUM   0x783
> +
> +#define SGI_CONFIG_MASK   0x0F
> +#define SGI_CONFIG_SHIFT  0x1C
> +#define SGI_PART_NUM_MASK 0xFFF
> +
> +//HwConfig DT structure^M
> +typedef struct {
> +  UINT64  *HwConfigDtAddr;
> +} SGI_HW_CONFIG_INFO_PPI;
> +
> +// ARM platform description data.
> +typedef struct {
> +  UINTN  PlatformId;
> +} SGI_PLATFORM_DESCRIPTOR;
> +
> +#endif // SGI_PLATFORM_H_
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
> b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index a56175e..f571897 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -199,6 +199,7 @@
>  
>
> NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>}
> +

Re: [edk2] [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread Leif Lindholm

Just some minor formatting points in addition to the comment on Ard's
reply..

On Fri, Jun 15, 2018 at 11:25:31AM +0530, Chandni Cherukuri wrote:
> On SGI platforms, the trusted firmware executes prior to
> the SEC phase. It supplies to the SEC phase a pointer to
> a fdt, that contains platform specific information such as
> part number and config number of the SGI platform. The
> platform code that executes during the SEC phase creates a
> PPI that allows access to other PEIMs to obtain a copy of the
> fdt pointer.
> 
> Further, during the PEI phase, a Platform ID PEIM installs a
> Platform ID HOB. The Platform ID HOB can be used by DXE
> drivers to identify the platform it is executing on.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S|  15 ++-
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
>  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 
> +
>  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf|  40 
>  Platform/ARM/SgiPkg/SgiPlatform.dec|   5 +
>  6 files changed, 177 insertions(+), 4 deletions(-)
>  create mode 100644 
> Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>  create mode 100644 
> Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> 
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> index dab6c77..80b93ea 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> +GCC_ASM_EXPORT(HwConfigDtBlob)
> +
> +HwConfigDtBlob: .long 0
>  
>  //
>  // First platform specific function to be called in the PEI phase
>  //
> -// This function is actually the first function called by the PrePi
> -// or PrePeiCore modules. It allows to retrieve arguments passed to
> -// the UEFI firmware through the CPU registers.
> -//
> +// The trusted firmware passed the hw config DT blob in x1 register.
> +// Keep a copy of it in a global variable.
> +//VOID
> +//ArmPlatformPeiBootAction (
> +//  VOID
> +//  );
>  ASM_PFX(ArmPlatformPeiBootAction):
> +  ldr  x10, =HwConfigDtBlob
> +  str  x1, [x10]
>ret
>  
>  //UINTN
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> index ea3201a..54860e0 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> @@ -15,6 +15,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +extern UINT64 HwConfigDtBlob;
> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>  
>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>{
> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>IN  UINTN MpId
>)
>  {
> +  mHwConfigDtInfoPpi.HwConfigDtAddr = 
>return RETURN_SUCCESS;
>  }
>  
> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>  EFI_PEI_PPI_DESCRIPTOR_PPI,
>  ,
>  
> +  },
> +  {
> +EFI_PEI_PPI_DESCRIPTOR_PPI,
> +,
> +
>}
>  };
>  
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> index 42e14d5..5d4bf72 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> @@ -61,7 +61,10 @@
>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>  
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>gEfiHobListGuid  ## CONSUMES  ## SystemTable
> +  gFdtTableGuid
>  
>  [Ppis]
>gArmMpCoreInfoPpiGuid
> +  gHwConfigDtInfoPpiGuid
> diff --git 
> a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c 
> b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> new file mode 100644
> index 000..f6446e6
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> @@ -0,0 +1,108 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> +
>

Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space

2018-06-15 Thread Zeng, Star


Generally, the patch is good to me.
Some comments below.

On 2018/6/14 19:38, Sami Mujawar wrote:

The SATA controller driver crashes while accessing the
PCI memory, as the PCI memory space is not enabled.


The code "accessing the PCI memory" you mentioned here is
the AhciReadReg in the following code block, right?



Enable the PCI memory space access to prevent the SATA
Controller driver from crashing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar 
---
The changes can be seen at 
https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v1

Notes:
 v1:
 - Fix SATA Controller driver crash[SAMI]

  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 
+++-
  MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
  2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c 
b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index 
a6d55c15571728eb3fd572003f383ba7c86635ae..21cc101d693f5adfd9d43f0c21a096eb59ba73b1
 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -2,6 +2,7 @@
This driver module produces IDE_CONTROLLER_INIT protocol for Sata 
Controllers.
  
Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.

+  Copyright (c) 2018, ARM Ltd. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD 
License
which accompanies this distribution.  The full text of the license may be 
found at
@@ -364,6 +365,7 @@ SataControllerStart (
EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
UINT32Data32;
UINTN TotalCount;
+  UINT64PciAttributes;
  
DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
  
@@ -406,6 +408,61 @@ SataControllerStart (

Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
Private->IdeInit.SetTiming  = IdeInitSetTiming;
Private->IdeInit.EnumAll= SATA_ENUMER_ALL;
+  Private->PciAttributesChanged   = FALSE;
+
+  // Save original PCI attributes
+  Status = PciIo->Attributes (
+PciIo,
+EfiPciIoAttributeOperationGet,
+0,
+>OriginalPciAttributes
+);
+  if (EFI_ERROR (Status)) {
+  goto Done;
+  }


Good to me.


+
+  DEBUG ((
+EFI_D_INFO,
+"PCI Attributes = 0x%llx\n",


How about using "Original PCI Attributes = 0x%llx\n"?


+Private->OriginalPciAttributes
+));
+
+  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+Status = PciIo->Attributes (
+  PciIo,
+  EfiPciIoAttributeOperationSupported,
+  0,
+  
+  );
+if (EFI_ERROR (Status)) {
+  goto Done;
+}
+
+DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", PciAttributes));
+
+if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+  DEBUG ((
+EFI_D_ERROR,
+"Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
+));
+  Status = EFI_UNSUPPORTED;
+  goto Done;
+}
+
+PciAttributes = Private->OriginalPciAttributes | 
EFI_PCI_IO_ATTRIBUTE_MEMORY;
+
+DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
+Status = PciIo->Attributes (
+  PciIo,
+  EfiPciIoAttributeOperationEnable,
+  PciAttributes,
+  NULL
+  );
+if (EFI_ERROR (Status)) {
+  goto Done;
+}


It is the case for enabling memory space, but there may be case to need 
IO space enabling. I suggest to use the code block (same with other 
device drivers).


  Status = PciIo->Attributes (
PciIo,
EfiPciIoAttributeOperationSupported,
0,

);
  if (!EFI_ERROR (Status)) {
Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
Status = PciIo->Attributes (
  PciIo,
  EfiPciIoAttributeOperationEnable,
  Supports,
  NULL
  );
  }


+Private->PciAttributesChanged = TRUE;
+  }
  
Status = PciIo->Pci.Read (

  PciIo,
@@ -414,7 +471,10 @@ SataControllerStart (
  sizeof (PciData.Hdr.ClassCode),
  PciData.Hdr.ClassCode
  );
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+ASSERT (FALSE);
+goto Done;
+  }
  
if (IS_PCI_IDE ()) {

  Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
@@ -481,6 +541,15 @@ Done:
if

Re: [edk2] [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread Leif Lindholm

On Fri, Jun 15, 2018 at 08:19:35AM +0200, Ard Biesheuvel wrote:
> On 15 June 2018 at 08:16, Ard Biesheuvel  wrote:
> >> diff --git 
> >> a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf 
> >> b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> >> new file mode 100644
> >> index 000..502d6ac
> >> --- /dev/null
> >> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
> >> @@ -0,0 +1,40 @@
> >> +#
> >> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> >> +#
> >> +#  This program and the accompanying materials are licensed and made 
> >> available
> >> +#  under the terms and conditions of the BSD License which accompanies 
> >> this
> >> +#  distribution.  The full text of the license may be found at
> >> +#  http://opensource.org/licenses/bsd-license.php
> >> +#
> >> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> >> IMPLIED.
> >> +#
> >> +
> >> +[Defines]
> >> +  INF_VERSION= 0x0001001A
> >> +  BASE_NAME  = SgiPlatformIdPeiLib
> 
> I forgot: this is a module not a library. So it should be called
> SgiPlatformIdPei or SgiPlatformIdPeim not ...Lib

A grep through the edk2 tree suggests that:
- the directory is usually called *Pei
- .inf/.uni usually *Pei.*
- entry point and source files *Peim*

/
Leif

> >> +  FILE_GUID  = a9936f3e-6a1b-11e8-8ce0-fffe69b86863
> >> +  MODULE_TYPE= PEIM
> >> +  VERSION_STRING = 1.0
> >> +  ENTRY_POINT= SgiPlatformIdPeim
> >> +
> >> +[Packages]
> >> +  EmbeddedPkg/EmbeddedPkg.dec
> >> +  MdePkg/MdePkg.dec
> >> +  Platform/ARM/SgiPkg/SgiPlatform.dec
> >> +
> >> +[LibraryClasses]
> >> +  FdtLib
> >> +  PeimEntryPoint
> >> +
> >> +[Sources]
> >> +  SgiPlatformPeiLib.c
> >> +
> >> +[Guids]
> >> +  gArmSgiPlatformIdDescriptorGuid
> >> +
> >> +[Ppis]
> >> +  gHwConfigDtInfoPpiGuid
> >> +
> >> +[Depex]
> >> +  TRUE
> >> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec 
> >> b/Platform/ARM/SgiPkg/SgiPlatform.dec
> >> index d995937..ea9f6c5 100644
> >> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec
> >> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec
> >> @@ -29,9 +29,14 @@
> >>Include# Root include for the package
> >>
> >>  [Guids.common]
> >> +  # ARM Sgi Platform ID descriptor
> >> +  gArmSgiPlatformIdDescriptorGuid = { 0xf56f152a, 0xad2a, 0x11e6, { 0xb1, 
> >> 0xa7, 0x00, 0x50, 0x56, 0x3c, 0x44, 0xcc } }
> >>gArmSgiTokenSpaceGuid  = { 0x577d6941, 0xaea1, 0x40b4, { 0x90, 
> >> 0x93, 0x2a, 0x86, 0x61, 0x72, 0x5a, 0x57 } }
> >>gSgi575AcpiTablesiFileGuid = { 0xc712719a, 0x0aaf, 0x438c, { 0x9c, 
> >> 0xdd, 0x35, 0xab, 0x4d, 0x60, 0x20, 0x7d } }
> >>
> >>  [PcdsFeatureFlag.common]
> >># Set this PCD to TRUE to enable virtio support.
> >>gArmSgiTokenSpaceGuid.PcdVirtioSupported|TRUE|BOOLEAN|0x0001
> >> +
> >> +[Ppis]
> >> +  gHwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 
> >> 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } }
> >> --
> >> 2.7.4
> >>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] FDF Spec: clean up to support for FILE statement

2018-06-15 Thread Yonghong Zhu

Update the format in [FV], [Capsule] and [Rule] section.

Cc: Liming Gao 
Cc: Michael Kinney 
Cc: Kevin W Shaw 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 2_fdf_design_discussion/26_[capsule]_sections.md  |  4 ++--
 3_edk_ii_fdf_file_format/36_[fv]_sections.md  |  8 
 3_edk_ii_fdf_file_format/37_[capsule]_sections.md | 10 +-
 3_edk_ii_fdf_file_format/39_[rule]_sections.md| 15 ++-
 README.md |  1 +
 5 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/2_fdf_design_discussion/26_[capsule]_sections.md 
b/2_fdf_design_discussion/26_[capsule]_sections.md
index ba93268..2e29612 100644
--- a/2_fdf_design_discussion/26_[capsule]_sections.md
+++ b/2_fdf_design_discussion/26_[capsule]_sections.md
@@ -1,9 +1,9 @@

[edk2] [PATCH 3/4] MdeModulePkg: Add GUID for recovery capsule on NVM Express devices

2018-06-15 Thread Hao Wu

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Include/Guid/RecoveryDevice.h | 9 +
 MdeModulePkg/MdeModulePkg.dec  | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/MdeModulePkg/Include/Guid/RecoveryDevice.h 
b/MdeModulePkg/Include/Guid/RecoveryDevice.h
index b79c51c4d1..2d2cea68ec 100644
--- a/MdeModulePkg/Include/Guid/RecoveryDevice.h
+++ b/MdeModulePkg/Include/Guid/RecoveryDevice.h
@@ -52,9 +52,18 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 0x0ffbce19, 0x324c, 0x4690, {0xa0, 0x09, 0x98, 0xc6, 0xae, 0x2e, 0xb1, 
0x86 } \
   }
 
+///
+/// The Global ID used to identify a recovery capsule that was loaded from NVM 
Express device.
+///
+#define RECOVERY_ON_FAT_NVME_DISK_GUID \
+  { \
+0xc770a27f, 0x956a, 0x497a, {0x85, 0x48, 0xe0, 0x61, 0x97, 0x58, 0x8b, 
0xf6 } \
+  }
+
 extern EFI_GUID gRecoveryOnDataCdGuid;
 extern EFI_GUID gRecoveryOnFatFloppyDiskGuid;
 extern EFI_GUID gRecoveryOnFatIdeDiskGuid;
 extern EFI_GUID gRecoveryOnFatUsbDiskGuid;
+extern EFI_GUID gRecoveryOnFatNvmeDiskGuid;
 
 #endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 8d7c97ee91..492b67d251 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -300,6 +300,9 @@
   ## Include/Guid/RecoveryDevice.h
   gRecoveryOnDataCdGuid  = { 0x5CAC0099, 0x0DC9, 0x48E5, { 0x80, 
0x68, 0xBB, 0x95, 0xF5, 0x40, 0x0A, 0x9F }}
 
+  ## Include/Guid/RecoveryDevice.h
+  gRecoveryOnFatNvmeDiskGuid = { 0xC770A27F, 0x956A, 0x497A, { 0x85, 
0x48, 0xE0, 0x61, 0x97, 0x58, 0x8B, 0xF6 }}
+
   ## Include/Guid/SmmLockBox.h
   gEfiSmmLockBoxCommunicationGuid= { 0x2a3cfebd, 0x27e8, 0x4d0a, { 0x8b, 
0x79, 0xd6, 0x88, 0xc2, 0xa3, 0xe1, 0xc0 }}
 
-- 
2.12.0.windows.1

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

[edk2] [PATCH 0/4] Add PEI BlockIo support for NVM Express devices

2018-06-15 Thread Hao Wu

The series is also available at:
https://github.com/hwu25/edk2/tree/nvme_pei_blockio

The series will add the PEI BlockIo support for NVM Express devices.

A new EDKII PEI NVME host controller PPI will be introduced. It will
provide the caller with the MMIO BAR address and the device path
information of the NVM Express host controllers within system.

The NvmExpressPei driver is added to comsume the EDKII PEI NVME host
controller PPI and produce the BlockIo PPIs for those NVME devices.

Also, FatPei driver has been updated to support the recovery from NVME
devices.

Tests done for the patch:
(NOTE: All tests are performed with 1 NVME device attached, and 2 NVME
devices attached. And all tests are performed with Vtd enabled and
disabled on platform.)

1. Use the BlockIo PPIs to read and verify the data on NVME devices.
2. Recovery from NVME device test on real platform.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Jiewen Yao 

Hao Wu (4):
  MdeModulePkg: Add definitions for EDKII PEI NVME host controller PPI
  MdeModulePkg/NvmExpressPei: Add the NVME device PEI BlockIo support
  MdeModulePkg: Add GUID for recovery capsule on NVM Express devices
  FatPkg/FatPei: Add the recognition of recovery capsule on NVME device

 FatPkg/FatPei/FatLiteApi.c |   6 +-
 FatPkg/FatPei/FatPei.inf   |   3 +-
 MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c| 249 +++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c | 368 ++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h | 265 +++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf   |  70 ++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.uni   |  21 +
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiBlockIo.c  | 531 ++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiBlockIo.h  | 266 +++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiExtra.uni  |  19 +
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.c  | 748 

 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.h  | 166 +
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 628 

 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.h | 107 +++
 MdeModulePkg/Include/Guid/RecoveryDevice.h |   9 +
 MdeModulePkg/Include/Ppi/NvmExpressHostController.h|  97 +++
 MdeModulePkg/MdeModulePkg.dec  |   6 +
 MdeModulePkg/MdeModulePkg.dsc  |   1 +
 18 files changed, 3558 insertions(+), 2 deletions(-)
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.uni
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiBlockIo.c
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiBlockIo.h
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiExtra.uni
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.c
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.h
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c
 create mode 100644 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.h
 create mode 100644 MdeModulePkg/Include/Ppi/NvmExpressHostController.h

-- 
2.12.0.windows.1

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

[edk2] [PATCH 2/4] MdeModulePkg/NvmExpressPei: Add the NVME device PEI BlockIo support

2018-06-15 Thread Hao Wu

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

This commit adds the PEI BlockIo support for NVM Express devices.

The driver will consume the EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI for NVM
Express host controllers within the system. And then produces the
BlockIo(2) PPIs for each controller.

The implementation of this driver is currently based on the NVM Express 1.1
Specification, which is available at:
http://nvmexpress.org/resources/specifications/

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c| 249 +++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.c | 368 ++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.h | 265 +++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.inf   |  70 ++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPei.uni   |  21 +
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiBlockIo.c  | 531 ++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiBlockIo.h  | 266 +++
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiExtra.uni  |  19 +
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.c  | 748 

 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiHci.h  | 166 +
 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.c | 628 

 MdeModulePkg/Bus/Pci/NvmExpressPei/NvmExpressPeiPassThru.h | 107 +++
 MdeModulePkg/MdeModulePkg.dsc  |   1 +
 13 files changed, 3439 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c 
b/MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c
new file mode 100644
index 00..51b48d38dd
--- /dev/null
+++ b/MdeModulePkg/Bus/Pci/NvmExpressPei/DmaMem.c
@@ -0,0 +1,249 @@
+/** @file
+  The DMA memory help function.
+
+  Copyright (c) 2018, 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
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include "NvmExpressPei.h"
+
+EDKII_IOMMU_PPI  *mIoMmu;
+
+/**
+  Provides the controller-specific addresses required to access system memory 
from a
+  DMA bus master.
+
+  @param  Operation Indicates if the bus master is going to read 
or write to system memory.
+  @param  HostAddress   The system memory address to map to the PCI 
controller.
+  @param  NumberOfBytes On input the number of bytes to map. On output 
the number of bytes
+that were mapped.
+  @param  DeviceAddress The resulting map address for the bus master 
PCI controller to use to
+access the hosts HostAddress.
+  @param  Mapping   A resulting value to pass to Unmap().
+
+  @retval EFI_SUCCESS   The range was mapped for the returned 
NumberOfBytes.
+  @retval EFI_UNSUPPORTED   The HostAddress cannot be mapped as a common 
buffer.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The request could not be completed due to a 
lack of resources.
+  @retval EFI_DEVICE_ERROR  The system hardware could not map the 
requested address.
+
+**/
+EFI_STATUS
+IoMmuMap (
+  IN  EDKII_IOMMU_OPERATION Operation,
+  IN VOID   *HostAddress,
+  IN  OUT UINTN *NumberOfBytes,
+  OUT EFI_PHYSICAL_ADDRESS  *DeviceAddress,
+  OUT VOID  **Mapping
+  )
+{
+  EFI_STATUS  Status;
+  UINT64  Attribute;
+
+  if (mIoMmu != NULL) {
+Status = mIoMmu->Map (
+   mIoMmu,
+   Operation,
+   HostAddress,
+   NumberOfBytes,
+   DeviceAddress,
+   Mapping
+   );
+if (EFI_ERROR (Status)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+switch (Operation) {
+case EdkiiIoMmuOperationBusMasterRead:
+case EdkiiIoMmuOperationBusMasterRead64:
+  Attribute = EDKII_IOMMU_ACCESS_READ;
+  break;
+case EdkiiIoMmuOperationBusMasterWrite:
+case EdkiiIoMmuOperationBusMasterWrite64:
+  Attribute = EDKII_IOMMU_ACCESS_WRITE;
+  break;
+case EdkiiIoMmuOperationBusMasterCommonBuffer:
+case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+  Attribute = EDKII_IOMMU_ACCESS_READ | EDKII_IOMMU_ACCESS_WRITE;
+  break;
+default:
+  ASSERT(FALSE);
+  return EFI_INVALID_PARAMETER;
+}
+Status = mIoMmu->SetAttribute (
+   mIoMmu,
+   *Mapping,
+   Attribute
+

[edk2] [PATCH 4/4] FatPkg/FatPei: Add the recognition of recovery capsule on NVME device

2018-06-15 Thread Hao Wu

The driver now can recognize the BlockIo2 PPI for NVM Express devices.
And support identifying the recovery capsule on those devices.

Cc: Ruiyu Ni 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 FatPkg/FatPei/FatLiteApi.c | 6 +-
 FatPkg/FatPei/FatPei.inf   | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/FatPkg/FatPei/FatLiteApi.c b/FatPkg/FatPei/FatLiteApi.c
index e302657132..b455390610 100644
--- a/FatPkg/FatPei/FatLiteApi.c
+++ b/FatPkg/FatPei/FatLiteApi.c
@@ -1,7 +1,7 @@
 /** @file
   FAT recovery PEIM entry point, Ppi Functions and FAT Api functions.
 
-Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2018, 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
@@ -485,6 +485,10 @@ GetRecoveryCapsuleInfo (
 CopyGuid (CapsuleType, );
 break;
 
+  case MSG_NVME_NAMESPACE_DP:
+CopyGuid (CapsuleType, );
+break;
+
   default:
 break;
   }
diff --git a/FatPkg/FatPei/FatPei.inf b/FatPkg/FatPei/FatPei.inf
index 273f72da2f..00b08df2b9 100644
--- a/FatPkg/FatPei/FatPei.inf
+++ b/FatPkg/FatPei/FatPei.inf
@@ -1,7 +1,7 @@
 ## @file
 #Lite Fat driver only used in Pei Phase.
 #
-#  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.
+#  Copyright (c) 2006 - 2018, 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
@@ -58,6 +58,7 @@
   gRecoveryOnFatUsbDiskGuid   ## SOMETIMES_CONSUMES   ## 
UNDEFINED
   gRecoveryOnFatIdeDiskGuid   ## SOMETIMES_CONSUMES   ## 
UNDEFINED
   gRecoveryOnFatFloppyDiskGuid## SOMETIMES_CONSUMES   ## 
UNDEFINED
+  gRecoveryOnFatNvmeDiskGuid  ## SOMETIMES_CONSUMES   ## 
UNDEFINED
 
 
 [Ppis]
-- 
2.12.0.windows.1

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

[edk2] [PATCH 1/4] MdeModulePkg: Add definitions for EDKII PEI NVME host controller PPI

2018-06-15 Thread Hao Wu

Introduces the below PPI:

struct EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI {
  EDKII_NVM_EXPRESS_HC_GET_MMIO_BAR   GetNvmeHcMmioBar;
  EDKII_NVM_EXPRESS_HC_GET_DEVICE_PATHGetNvmeHcDevicePath;
};

The GetNvmeHcMmioBar service will provide the caller with the MMIO BAR
address of each NVMe HC within the system;

The GetNvmeHcDevicePath service will provide the caller with the device
path information of each NVMe HC.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Include/Ppi/NvmExpressHostController.h | 97 
 MdeModulePkg/MdeModulePkg.dec   |  3 +
 2 files changed, 100 insertions(+)

diff --git a/MdeModulePkg/Include/Ppi/NvmExpressHostController.h 
b/MdeModulePkg/Include/Ppi/NvmExpressHostController.h
new file mode 100644
index 00..de9ae4a59c
--- /dev/null
+++ b/MdeModulePkg/Include/Ppi/NvmExpressHostController.h
@@ -0,0 +1,97 @@
+/** @file
+
+  Copyright (c) 2018, 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
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef _EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI_H_
+#define _EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI_H_
+
+#include 
+
+///
+/// Global ID for the EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI.
+///
+#define EDKII_NVME_EXPRESS_HOST_CONTROLLER_PPI_GUID \
+  { \
+0xcae3aa63, 0x676f, 0x4da3, { 0xbd, 0x50, 0x6c, 0xc5, 0xed, 0xde, 0x9a, 
0xad } \
+  }
+
+//
+// Forward declaration for the EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI.
+//
+typedef struct _EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI  
EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI;
+
+/**
+  Get the MMIO base address of NVM Express host controller.
+
+  @param[in]  PeiServices  Describes the list of possible PEI Services.
+  @param[in]  This The PPI instance pointer.
+  @param[in]  ControllerId The ID of the NVM Express host controller.
+  @param[out] MmioBar  The MMIO base address of the controller.
+
+  @retval EFI_SUCCESS  The operation succeeds.
+  @retval EFI_INVALID_PARAMETERThe parameters are invalid.
+  @retval EFI_NOT_FOUNDThe specified NVM Express host controller 
not
+   found.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_NVM_EXPRESS_HC_GET_MMIO_BAR) (
+  IN  EFI_PEI_SERVICES **PeiServices,
+  IN  EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI*This,
+  IN  UINT8ControllerId,
+  OUT UINTN*MmioBar
+  );
+
+/**
+  Get the device path of NVM Express host controller.
+
+  @param[in]  PeiServices  Describes the list of possible PEI Services.
+  @param[in]  This The PPI instance pointer.
+  @param[in]  ControllerId The ID of the NVM Express host controller.
+  @param[out] DevicePathLenth  The length of the device path in bytes 
specified
+   by DevicePath.
+  @param[out] DevicePath   The device path of NVM Express host 
controller.
+   The caller is responsible for freeing it.
+   This field re-uses EFI Device Path Protocol 
as
+   defined by Section 10.2 EFI Device Path 
Protocol
+   of UEFI 2.7 Specification.
+
+  @retval EFI_SUCCESS  The operation succeeds.
+  @retval EFI_INVALID_PARAMETERThe parameters are invalid.
+  @retval EFI_NOT_FOUNDThe specified NVM Express host controller 
not
+   found.
+  @retval EFI_OUT_OF_RESOURCES The operation fails due to lack of 
resources.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_NVM_EXPRESS_HC_GET_DEVICE_PATH) (
+  IN  EFI_PEI_SERVICES **PeiServices,
+  IN  EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI*This,
+  IN  UINT8ControllerId,
+  OUT UINTN*DevicePathLenth,
+  OUT EFI_DEVICE_PATH_PROTOCOL **DevicePath
+  );
+
+//
+// This PPI contains a set of services to interact with the NVM Express host
+// controller.
+//
+struct _EDKII_NVM_EXPRESS_HOST_CONTROLLER_PPI {
+  EDKII_NVM_EXPRESS_HC_GET_MMIO_BAR   GetNvmeHcMmioBar;
+  EDKII_NVM_EXPRESS_HC_GET_DEVICE_PATHGetNvmeHcDevicePath;
+};
+
+extern EFI_GUID gEdkiiPeiNvmExpressHostControllerPpiGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 3802b6e0b8..8d7c97ee91 100644
---

Re: [edk2] [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread Ard Biesheuvel

On 15 June 2018 at 08:16, Ard Biesheuvel  wrote:
> Hello Chandni,
>
> On 15 June 2018 at 07:55, Chandni Cherukuri  wrote:
>> On SGI platforms, the trusted firmware executes prior to
>> the SEC phase. It supplies to the SEC phase a pointer to
>> a fdt, that contains platform specific information such as
>> part number and config number of the SGI platform. The
>> platform code that executes during the SEC phase creates a
>> PPI that allows access to other PEIMs to obtain a copy of the
>> fdt pointer.
>>
>> Further, during the PEI phase, a Platform ID PEIM installs a
>> Platform ID HOB. The Platform ID HOB can be used by DXE
>> drivers to identify the platform it is executing on.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Chandni Cherukuri 
>> ---
>>  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S|  15 ++-
>>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
>>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
>>  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 
>> +
>>  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf|  40 
>>  Platform/ARM/SgiPkg/SgiPlatform.dec|   5 +
>>  6 files changed, 177 insertions(+), 4 deletions(-)
>>  create mode 100644 
>> Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>>  create mode 100644 
>> Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>>
>
> Please use '--stat=250 --stat-graph-width=20' when using git
> format-patch so that we get to see the entire path names in the
> diffstat
>
>> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S 
>> b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
>> index dab6c77..80b93ea 100644
>> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
>> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
>> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
>> +GCC_ASM_EXPORT(HwConfigDtBlob)
>> +
>> +HwConfigDtBlob: .long 0
>>
>
> Please move this definition to PlatformLib.c
>
>>  //
>>  // First platform specific function to be called in the PEI phase
>>  //
>> -// This function is actually the first function called by the PrePi
>> -// or PrePeiCore modules. It allows to retrieve arguments passed to
>> -// the UEFI firmware through the CPU registers.
>> -//
>> +// The trusted firmware passed the hw config DT blob in x1 register.
>> +// Keep a copy of it in a global variable.
>> +//VOID
>> +//ArmPlatformPeiBootAction (
>> +//  VOID
>> +//  );
>>  ASM_PFX(ArmPlatformPeiBootAction):
>> +  ldr  x10, =HwConfigDtBlob
>
> Please use adr to take the address of HwConfigDtBlob
>
>> +  str  x1, [x10]
>>ret
>>
>>  //UINTN
>> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
>> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
>> index ea3201a..54860e0 100644
>> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
>> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
>> @@ -15,6 +15,10 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +
>> +extern UINT64 HwConfigDtBlob;
>> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>>
>
> Where is this PPI declared? A PPI is like a protocol, it has its own
> declaration in a header file under Include/Ppi in the package
>
>>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>>{
>> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>>IN  UINTN MpId
>>)
>>  {
>> +  mHwConfigDtInfoPpi.HwConfigDtAddr = 
>>return RETURN_SUCCESS;
>>  }
>>
>> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>>  EFI_PEI_PPI_DESCRIPTOR_PPI,
>>  ,
>>  
>> +  },
>> +  {
>> +EFI_PEI_PPI_DESCRIPTOR_PPI,
>> +,
>> +
>>}
>>  };
>>
>> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf 
>> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
>> index 42e14d5..5d4bf72 100644
>> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
>> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
>> @@ -61,7 +61,10 @@
>>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>
>>  [Guids]
>> +  gArmSgiPlatformIdDescriptorGuid
>>gEfiHobListGuid  ## CONSUMES  ## SystemTable
>> +  gFdtTableGuid
>>
>>  [Ppis]
>>gArmMpCoreInfoPpiGuid
>> +  gHwConfigDtInfoPpiGuid
>> diff --git 
>> a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c 
>> b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>> new file mode 100644
>> index 000..f6446e6
>> --- /dev/null
>> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>> @@ -0,0 +1,108 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
>> +*
>> +*  This program and the accompanying materials are licensed and made 
>> available

Re: [edk2] [PATCH 2/2] Platform/ARM/Sgi: Pick ACPI tables to install based on platform ID

2018-06-15 Thread Ard Biesheuvel

Hello Chandni,

On 15 June 2018 at 07:55, Chandni Cherukuri  wrote:
> Use the platform ID and config ID values from the platform ID
> HOB to choose the right set of ACPI tables to be installed.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 33 
> ++
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf |  2 ++
>  Platform/ARM/SgiPkg/Include/SgiPlatform.h  | 24 ++--
>  Platform/ARM/SgiPkg/SgiPlatform.dsc|  1 +
>  Platform/ARM/SgiPkg/SgiPlatform.fdf|  1 +
>  5 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> index edaae5b..d0d5808 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -14,6 +14,8 @@
>
>  #include 
>  #include 
> +#include 
> +#include 
>
>  EFI_STATUS
>  InitVirtioBlockIo (
> @@ -28,20 +30,41 @@ ArmSgiPkgEntryPoint (
>)
>  {
>EFI_STATUS  Status;
> +  VOID*PlatformIdHob;
> +  SGI_PLATFORM_DESCRIPTOR *HobData;
> +  UINT32  ConfigId;
> +  UINT32  PartNum;
>
> -  Status = LocateAndInstallAcpiFromFv ();
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
> __FUNCTION__));
> -return Status;
> +  PlatformIdHob = GetFirstGuidHob ();
> +  if (PlatformIdHob == NULL) {
> +DEBUG ((DEBUG_ERROR, "Platform ID Hob is NULL\n"));
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  HobData = (SGI_PLATFORM_DESCRIPTOR *)(GET_GUID_HOB_DATA (PlatformIdHob));
> +
> +  PartNum = (HobData->PlatformId & SGI_PART_NUM_MASK);
> +  ConfigId = ((HobData->PlatformId >> SGI_CONFIG_SHIFT) & SGI_CONFIG_MASK);
> +
> +  if ((PartNum == SGI575_PART_NUM) && (ConfigId == SGI575_CONF_NUM)) {
> +Status = LocateAndInstallAcpiFromFv ();
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR, "%a: Failed to install ACPI tables\n", 
> __FUNCTION__));
> +  return Status;
> +}
> +  } else {
> +DEBUG ((DEBUG_ERROR, "PlatformDxe: Unsupported Platform Id\n"));
> +return EFI_UNSUPPORTED;
>}
>
> -  Status = EFI_REQUEST_UNLOAD_IMAGE;
>if (FeaturePcdGet (PcdVirtioSupported)) {
>  Status = InitVirtioBlockIo (ImageHandle);
>  if (EFI_ERROR (Status)) {
>DEBUG ((DEBUG_ERROR, "%a: Failed to install Virtio Block device\n",
>  __FUNCTION__));
>  }
> +  } else {
> +Status = EFI_REQUEST_UNLOAD_IMAGE;
>}
>
>return Status;
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf 
> b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> index 51ad22f..b6b8209 100644
> --- a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -30,10 +30,12 @@
>
>  [LibraryClasses]
>AcpiLib
> +  HobLib
>UefiDriverEntryPoint
>VirtioMmioDeviceLib
>
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>gSgi575AcpiTablesiFileGuid
>
>  [FeaturePcd]
> diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h 
> b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> index 00ca7e9..756954c 100644
> --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h
> @@ -12,8 +12,8 @@
>  *
>  **/
>
> -#ifndef __SGI_PLATFORM_H__
> -#define __SGI_PLATFORM_H__
> +#ifndef SGI_PLATFORM_H_
> +#define SGI_PLATFORM_H_
>
>  
> /***
>  // Platform Memory Map
> @@ -68,4 +68,22 @@
>  #define SGI_SYSPH_SYS_REG_FLASH   0x4C
>  #define SGI_SYSPH_SYS_REG_FLASH_RWEN  0x1
>
> -#endif // __SGI_PLATFORM_H__
> +// SGI575_VERSION values
> +#define SGI575_CONF_NUM   0x3
> +#define SGI575_PART_NUM   0x783
> +
> +#define SGI_CONFIG_MASK   0x0F
> +#define SGI_CONFIG_SHIFT  0x1C
> +#define SGI_PART_NUM_MASK 0xFFF
> +
> +//HwConfig DT structure^M

^M ?

> +typedef struct {
> +  UINT64  *HwConfigDtAddr;
> +} SGI_HW_CONFIG_INFO_PPI;
> +

Please declare this PPI properly

> +// ARM platform description data.
> +typedef struct {
> +  UINTN  PlatformId;
> +} SGI_PLATFORM_DESCRIPTOR;
> +
> +#endif // SGI_PLATFORM_H_
> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
> b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> index a56175e..f571897 100644
> --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
> @@ -199,6 +199,7 @@
>  
>
> NULL|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
>}
> +  Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>
>#
># DXE
> diff --git

Re: [edk2] [PATCH 1/2] Platform/ARM/Sgi: Install a Platform ID HOB

2018-06-15 Thread Ard Biesheuvel

Hello Chandni,

On 15 June 2018 at 07:55, Chandni Cherukuri  wrote:
> On SGI platforms, the trusted firmware executes prior to
> the SEC phase. It supplies to the SEC phase a pointer to
> a fdt, that contains platform specific information such as
> part number and config number of the SGI platform. The
> platform code that executes during the SEC phase creates a
> PPI that allows access to other PEIMs to obtain a copy of the
> fdt pointer.
>
> Further, during the PEI phase, a Platform ID PEIM installs a
> Platform ID HOB. The Platform ID HOB can be used by DXE
> drivers to identify the platform it is executing on.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chandni Cherukuri 
> ---
>  .../SgiPkg/Library/PlatformLib/AArch64/Helper.S|  15 ++-
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.c   |  10 ++
>  .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   3 +
>  .../Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c  | 108 
> +
>  .../SgiPlatformPeiLib/SgiPlatformPeiLib.inf|  40 
>  Platform/ARM/SgiPkg/SgiPlatform.dec|   5 +
>  6 files changed, 177 insertions(+), 4 deletions(-)
>  create mode 100644 
> Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
>  create mode 100644 
> Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.inf
>

Please use '--stat=250 --stat-graph-width=20' when using git
format-patch so that we get to see the entire path names in the
diffstat

> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> index dab6c77..80b93ea 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/AArch64/Helper.S
> @@ -22,15 +22,22 @@ GCC_ASM_EXPORT(ArmPlatformPeiBootAction)
>  GCC_ASM_EXPORT(ArmPlatformGetCorePosition)
>  GCC_ASM_EXPORT(ArmPlatformGetPrimaryCoreMpId)
>  GCC_ASM_EXPORT(ArmPlatformIsPrimaryCore)
> +GCC_ASM_EXPORT(HwConfigDtBlob)
> +
> +HwConfigDtBlob: .long 0
>

Please move this definition to PlatformLib.c

>  //
>  // First platform specific function to be called in the PEI phase
>  //
> -// This function is actually the first function called by the PrePi
> -// or PrePeiCore modules. It allows to retrieve arguments passed to
> -// the UEFI firmware through the CPU registers.
> -//
> +// The trusted firmware passed the hw config DT blob in x1 register.
> +// Keep a copy of it in a global variable.
> +//VOID
> +//ArmPlatformPeiBootAction (
> +//  VOID
> +//  );
>  ASM_PFX(ArmPlatformPeiBootAction):
> +  ldr  x10, =HwConfigDtBlob

Please use adr to take the address of HwConfigDtBlob

> +  str  x1, [x10]
>ret
>
>  //UINTN
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> index ea3201a..54860e0 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.c
> @@ -15,6 +15,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +extern UINT64 HwConfigDtBlob;
> +STATIC SGI_HW_CONFIG_INFO_PPI mHwConfigDtInfoPpi;
>

Where is this PPI declared? A PPI is like a protocol, it has its own
declaration in a header file under Include/Ppi in the package

>  STATIC ARM_CORE_INFO mCoreInfoTable[] = {
>{
> @@ -36,6 +40,7 @@ ArmPlatformInitialize (
>IN  UINTN MpId
>)
>  {
> +  mHwConfigDtInfoPpi.HwConfigDtAddr = 
>return RETURN_SUCCESS;
>  }
>
> @@ -59,6 +64,11 @@ EFI_PEI_PPI_DESCRIPTOR gPlatformPpiTable[] = {
>  EFI_PEI_PPI_DESCRIPTOR_PPI,
>  ,
>  
> +  },
> +  {
> +EFI_PEI_PPI_DESCRIPTOR_PPI,
> +,
> +
>}
>  };
>
> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf 
> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> index 42e14d5..5d4bf72 100644
> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
> @@ -61,7 +61,10 @@
>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>
>  [Guids]
> +  gArmSgiPlatformIdDescriptorGuid
>gEfiHobListGuid  ## CONSUMES  ## SystemTable
> +  gFdtTableGuid
>
>  [Ppis]
>gArmMpCoreInfoPpiGuid
> +  gHwConfigDtInfoPpiGuid
> diff --git 
> a/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c 
> b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> new file mode 100644
> index 000..f6446e6
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Library/SgiPlatformPeiLib/SgiPlatformPeiLib.c
> @@ -0,0 +1,108 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made 
> available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
>

40 matches

Mail list logo