Re: [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

["Duran, Leo" <leo.duran@amd.com>]

Lazlo, et al,
Please reply below.
Leo.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 08, 2017 2:39 AM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: Feng Tian <feng.tian@intel.com>; Singh, Brijesh
> <brijesh.singh@amd.com>; Star Zeng <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> PcdPteMemoryEncryptionAddressOrMask
> 
> On 02/07/17 20:53, Leo Duran wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> >
> > This dynamic PCD holds the address mask for page table entries when
> > memory encryption is enabled on AMD processors supporting the Secure
> > Encrypted Virtualization (SEV) feature.
> >
> > Cc: Feng Tian <feng.tian@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Leo Duran <leo.duran@amd.com>
> > ---
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  5 ++++-
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 18
> ++++++++++--------
> >  MdeModulePkg/MdeModulePkg.dec                    |  8 ++++++++
> >  3 files changed, 22 insertions(+), 9 deletions(-)
> 
> [snip]
> 
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index 273cd7e..207384f 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -6,6 +6,8 @@
> >  # Copyright (c) 2007 - 2017, Intel Corporation. All rights
> > reserved.<BR>  # Copyright (c) 2016, Linaro Ltd. All rights
> > reserved.<BR>  # (C) Copyright 2016 Hewlett Packard Enterprise
> > Development LP<BR>
> > +# Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
> >  # This program and the accompanying materials are licensed and made
> > available under  # the terms and conditions of the BSD License that
> accompanies this distribution.
> >  # The full text of the license may be found at @@ -1738,5 +1740,11 @@
> > [PcdsDynamic, PcdsDynamicEx]
> >    # @Prompt If there is any test key used by the platform.
> >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed|FALSE|BOOLEAN|0x0
> 0030003
> >
> > +  ## This dynamic PCD holds the address mask for page table entries
> > + when memory encryption is  #  enabled on AMD processors supporting
> the Secure Encrypted Virtualization (SEV) feature.
> > +  #  This mask should be applied when creating 1:1 virtual to physical
> mapping tables.
> > +  #
> > +
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask|0
> > + x0|UINT64|0x00030004
> > +
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    MdeModulePkgExtra.uni
> >
> 
> I think this PCD should be added to the following section of the DEC file:
> 
>   [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> 
> Although OVMF will use the PCD as a dynamic one, I see no reason why the
> DEC file should prohibit setting the PCD as a fixed one.
> 
> Looking at other PCDs in the
> 
>   [PcdsDynamic, PcdsDynamicEx]
> 
> section, most of those seem to stand for data that are impossible to
> determine at build time. Is the new PCD impossible to determine at build
> time, for any platform at all?
> 
[Duran, Leo] 
The PCD should be determined at runtime using CPUID.
However, I suppose you may 'know' about your platform at build time.

So to your point, I think it's reasonable to allow fixed, etc.

> ... Of course, if it can be proved that any given platform will either
> (a) set this new PCD dynamically, or
> (b) not even *include* the PCD -- with a dynamic default value -- in
>     its platform DSC file,
> then this patch can work too.
> 
> Thanks
> Laszlo