Re: [RFC Part1 PATCH v3 01/17] Documentation/x86: Add AMD Secure Encrypted Virtualization (SEV) descrption

[Borislav Petkov <bp-l3A5Bk7waGM@public.gmane.org>]

On Mon, Jul 24, 2017 at 02:07:41PM -0500, Brijesh Singh wrote:

Subject: Re: [RFC Part1 PATCH v3 01/17] Documentation/x86: Add AMD Secure Encrypted Virtualization (SEV) descrption
													 ^^^^^^^^^^

Please introduce a spellchecker into your workflow.

> Update amd-memory-encryption document describing the AMD Secure Encrypted

"Update the AMD memory encryption document...

The patch has the proper URL already.

> Virtualization (SEV) feature.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh-5C7GfCeVMHo@public.gmane.org>
> ---
>  Documentation/x86/amd-memory-encryption.txt | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/x86/amd-memory-encryption.txt b/Documentation/x86/amd-memory-encryption.txt
> index f512ab7..747df07 100644
> --- a/Documentation/x86/amd-memory-encryption.txt
> +++ b/Documentation/x86/amd-memory-encryption.txt
> @@ -1,4 +1,5 @@
> -Secure Memory Encryption (SME) is a feature found on AMD processors.
> +Secure Memory Encryption (SME) and Secure Encrypted Virtualization (SEV) are
> +features 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
> @@ -6,6 +7,12 @@ 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.
>  
> +SEV enables running encrypted virtual machine (VMs) in which the code and data

					 machines

> +of the virtual machine are secured so that decrypted version is available only

... of the guest VM ...		  ... so that a decrypted ...

> +within the VM itself. SEV guest VMs have concept of private and shared memory.

				 	have *the* concept - you need to use
					definite and indefinite articles in your
					text.

> +Private memory is encrypted with the guest-specific key, while shared memory
> +may be encrypted with hypervisor key.

And here you explain that the hypervisor key is the same key which we
use in SME. So that people can make the connection.

> +
>  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 also be
>  specified in the cr3 register, allowing the PGD table to be encrypted. Each
> @@ -19,11 +26,20 @@ so that the PGD is encrypted, but not set the encryption bit in the PGD entry
>  for a PUD which results in the PUD pointed to by that entry to not be
>  encrypted.
>  
> -Support for SME can be determined through the CPUID instruction. The CPUID
> -function 0x8000001f reports information related to SME:
> +When SEV is enabled, certain type of memory (namely insruction pages and guest

When SEV is enabled, instruction pages and guest page tables are ...

> +page tables) are always treated as private. Due to security reasons all DMA

security reasons??

> +operations inside the guest must be performed on shared memory. Since the
> +memory encryption bit is only controllable by the guest OS when it is operating

		     ... is controlled ...

> +in 64-bit or 32-bit PAE mode, in all other modes the SEV hardware forces memory

								... forces the memory ...

> +encryption bit to 1.
> +
> +Support for SME and SEV can be determined through the CPUID instruction. The
> +CPUID function 0x8000001f reports information related to SME:
>  
>  	0x8000001f[eax]:
>  		Bit[0] indicates support for SME
> +	0x800001f[eax]:

There's a 0 missing and you don't really need it as it is already above.

> +		Bit[1] indicates support for SEV
>  	0x8000001f[ebx]:
>  		Bits[5:0]  pagetable bit number used to activate memory
>  			   encryption
> @@ -39,6 +55,13 @@ determine if SME is enabled and/or to enable memory encryption:
>  		Bit[23]   0 = memory encryption features are disabled
>  			  1 = memory encryption features are enabled
>  
> +If SEV is supported, MSR 0xc0010131 (MSR_F17H_SEV) can be used to determine if

If this MSR is going to be part of the architecture - and I really think
it is - then call it MSR_AMD64_SEV.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--