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

[Tom Lendacky <thomas.lendacky@amd.com>]

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@amd.com>
>> ---
>>  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@suse.de>
>
> after addressing those minor nitpicks on your next submission.
>
> Thanks.
>