Re: [PATCH v5 01/32] x86: Documentation for AMD Secure Memory Encryption (SME)

[Tom Lendacky]

On 4/19/2017 4:02 AM, Borislav Petkov wrote:
> Always have a verb in the Subject to form a "do this" or "do that"
> sentence to better explain what the patch does:
>
> "Subject: [PATCH v5 01/32] x86: Add documentation for AMD Secure Memory Encryption (SME)"

Will do.

Btw, I tried to update all the subjects and descriptions to be
more descriptive but I'm sure there is still room for improvement
so keep the comments on them coming.

> On Tue, Apr 18, 2017 at 04:16:25PM -0500, Tom Lendacky wrote:
> > Create a Documentation entry to describe the AMD Secure Memory
> > Encryption (SME) feature and add documentation for the mem_encrypt=
> > kernel parameter.
> >
> > Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |   11 ++++
> >  Documentation/x86/amd-memory-encryption.txt     |   60 +++++++++++++++++++++++
> >  2 files changed, 71 insertions(+)
> >  create mode 100644 Documentation/x86/amd-memory-encryption.txt
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 3dd6d5d..84c5787 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2165,6 +2165,17 @@
> >  			memory contents and reserves bad memory
> >  			regions that are detected.
> >
> > +	mem_encrypt=	[X86-64] AMD Secure Memory Encryption (SME) control
> > +			Valid arguments: on, off
> > +			Default (depends on kernel configuration option):
> > +			  on  (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=y)
> > +			  off (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=n)
> > +			mem_encrypt=on:		Activate SME
> > +			mem_encrypt=off:	Do not activate SME
> > +
> > +			Refer to Documentation/x86/amd-memory-encryption.txt
> > +			for details on when memory encryption can be activated.
> > +
> >  	mem_sleep_default=	[SUSPEND] Default system suspend mode:
> >  			s2idle  - Suspend-To-Idle
> >  			shallow - Power-On Suspend or equivalent (if supported)
> > diff --git a/Documentation/x86/amd-memory-encryption.txt b/Documentation/x86/amd-memory-encryption.txt
> > new file mode 100644
> > index 0000000..0b72ff2
> > --- /dev/null
> > +++ b/Documentation/x86/amd-memory-encryption.txt
> > @@ -0,0 +1,60 @@
> > +Secure Memory Encryption (SME) is a feature found on AMD processors.
> > +
> > +SME provides the ability to mark individual pages of memory as encrypted using
> > +the standard x86 page tables.  A page that is marked encrypted will be
> > +automatically decrypted when read from DRAM and encrypted when written to
> > +DRAM.  SME can therefore be used to protect the contents of DRAM from physical
> > +attacks on the system.
> > +
> > +A page is encrypted when a page table entry has the encryption bit set (see
> > +below on how to determine its position).  The encryption bit can be specified
> > +in the cr3 register, allowing the PGD table to be encrypted. Each successive
>
> I missed that the last time: do you mean here, "The encryption bit can
> be specified in the %cr3 register allowing for the page table hierarchy
> itself to be encrypted."?
>
> > +level of page tables can also be encrypted.
>
> Right, judging by the next sentence, it looks like it.

Correct. I like the hierarchy term so I'll add that to the text.

Note, just because the bit is set in %cr3 doesn't mean the full
hierarchy is encrypted. Each level in the hierarchy needs to have the
encryption bit set. So, theoretically, you could have the encryption
bit set in %cr3 so that the PGD is encrypted, but not set the encryption
bit in the PGD entry for a PUD and so the PUD pointed to by that entry
would not be encrypted.

Thanks,
Tom

> The rest looks and reads really nice to me, so feel free to add:
>
> Reviewed-by: Borislav Petkov <bp@xxxxxxx>
>
> after addressing those minor nitpicks on your next submission.
>
> Thanks.