Re: [PATCH] [edk2-platforms]:Enabling Secure boot feature support on hikey platfrom

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Nov 27, 2017 at 02:02:32PM +0100, Laszlo Ersek wrote:
> On 11/26/17 16:22, Leif Lindholm wrote:
> > (Adding Laszlo to cc based on a single comment I make below.)
> > 
> > On Tue, Nov 21, 2017 at 04:23:36PM +0530, kalyan-nagabhirava wrote:
> 
> >>  [Guids.common]
> >>    gHiKeyTokenSpaceGuid          =  { 0x91148425, 0xcdd2, 0x4830, { 0x8b, 0xd0, 0xc6, 0x1c, 0x6d, 0xea, 0x36, 0x21 } }
> >> +  gHwTokenSpaceGuid             =  { 0x99999999, 0x74c5, 0x4043, { 0xb4, 0x17, 0xa3, 0x22, 0x38, 0x14, 0xce, 0x76 } }
> > 
> > This very much looks like a not properly generated GUID.
> > GUIDs must always be generated using an RFC4122-compliant algorithm.
> > I generally recommend using
> > https://www.guidgenerator.com/online-guid-generator.aspx.
> 
> I just run "uuidgen" in a terminal window.

Yeah, I just prefer pointing to someone that does not require
installing anything, or requires specific operating systems.

> >> +EFI_STATUS
> >> +EFIAPI
> >> +FvbSetAttributes(
> >> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL  *This,
> >> +  IN OUT    EFI_FVB_ATTRIBUTES_2                 *Attributes
> >> +  )
> >> +{
> >> +  DEBUG ((DEBUG_BLKIO, "FvbSetAttributes(0x%X) is not supported\n",*Attributes));
> >> +  return EFI_UNSUPPORTED;
> > 
> > As per my (very) recent comment to Marcin, I do not believe returning
> > EFI_UNSUPPORTED is a valid thing to do here. Which to me suggests the
> > implementation of FvbGetAttributes is also incorrect.
> > 
> > Laszlo - what's your take on this in conjunction with PI 1.6 section
> > 3.4.2? OvmfPkg does something very similar in
> > EmuVariableFvbRuntimeDxe/Fvb.c.
> 
> I guess you are right. The particular OvmfPkg code that you mention is
> likely also spec-breaking.
> 
> FWIW, in the OVMF flash driver that actually uses pflash, namely
> 
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
> 
> the FvbSetVolumeAttributes() function appears both appropriate for the
> spec and generic enough to copy elsewhere.

Yes, that looks good, thanks!

Marcin, Kalyan - please have a look at that implementation for
inspiration.

/
    Leif