Date: Tue, 5 Sep 2017 19:21:30 +0200
From: Borislav Petkov <bp@suse.de>
To: Brijesh Singh <brijesh.singh@amd.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>, Joerg Roedel <joro@8bytes.org>,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [RFC Part2 PATCH v3 01/26] Documentation/virtual/kvm: Add AMD
 Secure Encrypted Virtualization (SEV)
Message-ID: <20170905172130.24fgl6xsrfovsbsp@pd.tnic>
References: <20170724200303.12197-1-brijesh.singh@amd.com>
 <20170724200303.12197-2-brijesh.singh@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170724200303.12197-2-brijesh.singh@amd.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 16144
Lines: 573

On Mon, Jul 24, 2017 at 03:02:38PM -0500, Brijesh Singh wrote:
> Create a Documentation entry to describe the AMD Secure Encrypted
> Virtualization (SEV) feature.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  .../virtual/kvm/amd-memory-encryption.txt          | 328 +++++++++++++++++++++
>  1 file changed, 328 insertions(+)
>  create mode 100644 Documentation/virtual/kvm/amd-memory-encryption.txt
> 
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.txt b/Documentation/virtual/kvm/amd-memory-encryption.txt
> new file mode 100644
> index 0000000..cffed2d
> --- /dev/null
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.txt

You need to add this new file to Documentation/virtual/kvm/00-INDEX

> @@ -0,0 +1,328 @@
> +Secure Encrypted Virtualization (SEV) is a feature found on AMD processors.
> +
> +SEV is an extension to the AMD-V architecture which supports running virtual
> +machine (VMs) under the control of a hypervisor. When enabled, the memory

"machines"

> +contents of VM will be transparently encrypted with a key unique to the VM.
> +
> +Hypervisor can determine the SEV support through the CPUID instruction. The CPUID
> +function 0x8000001f reports information related to SEV:
> +
> +	0x8000001f[eax]:
> +		Bit[1] 	indicates support for SEV
> +
> +	0x8000001f[ecx]:

0x8000001f[eax]:
		Bit[1]
     ...  [ecx]:
     		Bits[31:0]

looks more compact to me and shows quicker that it is the same CPUID
leaf, just different reg.

While at it, you can do that to
Documentation/x86/amd-memory-encryption.txt too and now that I look at
it, 0x800001f[eax]: is short one 0.

> +		Bits[31:0]  Number of encrypted guest supported simultaneously

						guests

> +
> +If support for SEV is present, MSR 0xc00100010 (MSR_K8_SYSCFG) and MSR

				      0xc0010010

there's one 0 too many in yours. Also, write it 0xc001_0010, with the
4-digit help bar.

> +0xc0000015 (MSR_K7_HWCR_SMMLOCK) can be used to determine if it can be enabled:

Do you mean 0xc0010015 (MSR_K7_HWCR) here? Because the MSR is

#define MSR_K7_HWCR                     0xc0010015

That MSR_K7_HWCR_SMMLOCK is bit 0 in it.

> +
> +	0xc00100010:

that's 9 hex digits

> +		Bit[23]	   0 = memory encryption can be enabled
> +			   0 = memory encryption can not be enabled

			   ^-- one of those needs to be 1b :-)

> +
> +	0xc00010015:

Ditto.

> +		Bit[0]	   0 = memory encryption can not be enabled
> +			   1 = memory encryption can be enabled
> +
> +When SEV support is available, it can be enabled on specific VM during the VMRUN

s/on/in a/

and not "during the VMRUN... " but say "by setting SEV bit ... before
executing VMRUN."

> +instruction by setting SEV bit in VMCB offset 090h:
> +
> +	VMCB offset 090h:

I guess
	VMCB[0x90]

?

> +		Bit[1]	    1 = Enable SEV
> +
> +SEV hardware uses ASIDs to associate memory encryption key with the guest VMs.

		       "...to associate a memory encryption key with a VM."

> +Hence the ASID for the SEV-enabled guests must be from 1 to a maximum value

"Hence, ..."

> +defined through the CPUID function 0x8000001f[ECX].

"defined in the CPUID ... field."

Also, s/ECX/ecx/ as you're using small letters for register names
consistently so far.

> +
> +
> +SEV Key Management
> +------------------
> +
> +The Key management for the SEV guest is handled by a seperate processor known as

WARNING: 'seperate' may be misspelled - perhaps 'separate'?
#74: FILE: Documentation/virtual/kvm/amd-memory-encryption.txt:41:
+The Key management for the SEV guest is handled by a seperate processor known as

Run them all through a spellchecker pls.

> +the AMD Secure Processor (AMD-SP). Firmware running inside the AMD-SP provides a
> +secure key management interface to perform common hypervisor activities such as
> +encrypting bootstrap code, snapshotting, migrating and debugging the guest. For
> +more informaiton, see SEV Key Management spec:
	^^^^^^^^^^^

This looks misspelled too but I caught it and not checkpatch!

Meh, what good is that thing - it can't even catch all typos?! :-\


> +
> +http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf

<--- here you can add an introductory sentence or two:

"KVM implements the following commands to support SEV guests... " and so on.

Also, the userspace API is documented in
Documentation/virtual/kvm/api.txt. Shouldn't those be there too, to have
them in one place?

> +
> +1. KVM_SEV_LAUNCH_START
> +
> +Parameters: struct  kvm_sev_launch_start (in/out)
> +Returns: 0 on success, -negative on error
> +
> +LAUNCH_START command is used to bootstrap a guest by encrypting its memory with

"The KVM_SEV_LAUNCH_START command ... "

> +a new VM Encryption Key (VEK). In order to create guest context, hypervisor should

								   the

You need to start using (in-)definite articles in your sentences - text reads
strange now. Or should I say *the* text reads strange now? :-)

> +provide guest policy, owners public diffie-hellman (PDH) key and session parameters.

	  ^             ^	   	^
	  a		the owner's     Diffie-Hellman

or owners'. There are more occurrences of this below.

> +
> +The guest policy constrains the use and features activated for the lifetime of the
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Please rewrite that - I can only try guessing what it means.

> +launched guest, such as disallowing debugging, enabling key sharing, or turning on
> +other SEV related features.

"SEV-related"

> +
> +The guest owners PDH allows the firmware to establish a cryptographic session with
> +the guest owner to negotiate keys used for attestation.
		  ^

		  in order to


> +
> +The session parameters contains informations such as guest policy MAC, transport

WARNING: 'informations' may be misspelled - perhaps 'information'?
#98: FILE: Documentation/virtual/kvm/amd-memory-encryption.txt:65:
+The session parameters contains informations such as guest policy MAC, transport

also s/contains/contain/

> +integrity key (TIK), transport encryption key (TEK) etc.
> +
> +struct kvm_sev_launch_start {
> +
> +	/* Guest Hanldle, if zero then FW creates a new handle */
> +	__u32 handle;
> +
> +	/* Guest policy */
> +	__u32 policy;
> +
> +	/* Address which contains guest owner's PDH certificate blob */
> +	__u64 dh_cert_address;
> +	__u32 dh_cert_length;
> +
> +	/* Address which contains guest session information blob */
> +	__u64 session_address;
> +	__u32 session_length;
> +};
> +
> +On success, the 'handle' field contain a new handle.

... and on error, a negative value...

> +
> +2. KVM_SEV_LAUNCH_UPDATE_DATA
> +
> +Parameters (in): struct  kvm_sev_launch_update
> +Returns: 0 on success, -negative on error

<--- here you need to explain first what the command does and what could
be used for and then how it does it.

> +LAUNCH_UPDATE_DATA encrypts the memory region using the VEK created during

To avoid confusion, use the "KVM_SEV_"-prefixed defines pls.

> +LAUNCH_START. It also calculates a measurement of the memory region. This
> +measurement can be used as a signature of the memory contents.
> +
> +struct kvm_sev_launch_update {
> +	/* address of the data to be encrypted (must be 16-byte aligned) */
> +	__u64 address;
> +
> +	/* length of the data to be encrypted (must be 16-byte aligned) */
> +	__u32 length;
> +};
> +
> +3. KVM_SEV_LAUNCH_MEASURE
> +
> +Parameters (in): struct  kvm_sev_launch_measure
> +Returns: 0 on success, -negative on error
> +
> +LAUNCH_MEASURE returns the measurement of the memory region encrypted with
> +LAUNCH_UPDATE_DATA. The measurement is keyed with the TIK so that the guest
> +owner can use the measurement to verify the guest was properly launched without
> +tempering.

So this could use a bit more text as it is such an important aspect of
the whole verification of the guest.

> +
> +struct kvm_sev_launch_measure {
> +	/* where to copy the measurement blob */
> +	__u64 address;
> +
> +	/* length of memory region containing measurement */
> +	__u32 length;
> +};
> +
> +If measurement length is too small, the required length is returned in the
> +length field.
> +
> +On success, the measurement is copied to the address.

And how is success signalled to the caller?

> +
> +4. KVM_SEV_LAUNCH_FINISH
> +
> +Returns: 0 on success, -negative on error
> +
> +LAUNCH_FINISH command finalize the SEV guest launch process.

"The KVM_SEV_LAUNCH_FINISH command... "

> +
> +5. KVM_SEV_GUEST_STATUS
> +
> +Parameters (out): struct kvm_sev_guest_status

This is an "out" command, so it should be called
KVM_SEV_GET_GUEST_STATUS. Or is it too late for that?

> +Returns: 0 on success, -negative on error
> +
> +GUEST_STATUS returns the current SEV state the guest is in.
> +
> +struct kvm_sev_guest_status {
> +
> +	/* guest hanldle */
> +	__u32 handle;
> +
> +	/* guest policy */
> +	__u32 policy;
> +
> +	/* guest state  (see below) */
> +	__u8 state;
> +};
> +
> +SEV guest state:
> +
> +enum {
> +	/* guest state is not known */
> +	SEV_STATE_INVALID = 0;

not known or invalid?

> +	/* guest is currently being launched */
> +	SEV_STATE_LAUNCHING.

			   ^--- comma, I guess, instead of full-stop.

> +	/* guest is being launched and ready to accept the ciphertext data */
> +	SEV_STATE_SECRET,
> +	/* guest is fully launched and running */
> +	SEV_STATE_RUNNING,
> +	/* guest is being migrated in from another SEV machine */
> +	SEV_STATE_RECEIVING,
> +	/* guest is getting migrated out another SEV machine */

				    "out to another"

> +	SEV_STATE_SENDING
> +};

Btw, side-comments will make this much more readable:

enum {
        SEV_STATE_INVALID = 0,
        SEV_STATE_LAUNCHING,
        SEV_STATE_SECRET,       /* guest is being launched and ready to accept the ciphertext data */
        SEV_STATE_RUNNING,      /* guest is fully launched and running */
        SEV_STATE_RECEIVING,    /* guest is being migrated in from another SEV machine */
        SEV_STATE_SENDING,      /* guest is getting migrated out to another SEV machine */
};

> +
> +6. KVM_SEV_DBG_DECRYPT
> +
> +DEBUG_DECRYPT command can be used for decrypting a region of guest memory for
> +the SEV guest debug purposes. Note that since decrypting protected memory allows

Now here is "the" wrong. Ditto below.

Also, what is "protected memory"? You mean "guest memory", right?

> +the hypervisor to gain access to guest memory, the guest policy must explicitly
> +allow debugging for this command to work.
> +
> +Parameters (in): struct kvm_sev_dbg
> +Returns: 0 on success, -negative on error
> +
> +struct kvm_sev_dbg {
> +	__u64 src_address;
> +	__u64 dst_address;

Even though obvious, those need comments what they are, just like the
other struct members above. Below need comments too.

> +
> +	/* length of memory region to decrypt */
> +	__u32 length;
> +};
> +
> +7. KVM_SEV_DBG_ENCRYPT
> +
> +DEBUG_ENCRYPT command can be used for injecting the data into guest for the SEV

"The ... " - but you get the idea :)

make that "... can be used for injecting data into a guest for debugging
purposes."

> +guest debug purposes. Note that since injecting the data into protected memory
> +allows the hypervisor to modify the guest memory, the guest policy must explicitly
> +allow debugging for this command to work.

Same issues as above.

> +
> +Parameters (in): struct kvm_sev_dbg
> +Returns: 0 on success, -negative on error
> +
> +struct kvm_sev_dbg {
> +	__u64 src_address;
> +	__u64 dst_address;
> +
> +	/* length of memory region to encrypt */
> +	__u32 length;
> +};
> +
> +8. KVM_SEV_SEND_START
> +
> +Parameters (in): struct kvm_sev_send_start
> +Returns: 0 on success, -negative on error
> +
> +SEND_START command is used to export a SEV guest from one platform to another.

Export or migrate?

> +It can be used for saving a guest to disk to be resumed later, or it can be
> +used to migrate a guest across the network to a receiving platform.

And how do I specify which of those actions needs to happen?

> +
> +struct kvm_sev_send_start {
> +	/* guest policy */
> +	__u32 policy;
> +
> +	/* address which contains receivers PDH key blob */

				 the receiver's

> +	__u64 pdh_cert_address;
> +	__u32 pdh_cert_length;
> +
> +	/* address which contains platform certificate blob */
> +	__u64 plat_cert_address;
> +	__u32 plat_cert_length;
> +
> +	/* address which contains AMD certificate chain */
> +	__u64 amd_cert_address;
> +	__u32 amd_cert_length;
> +
> +	/* where to copy the current session information */
> +	__u64 session_address;
> +	__u32 session_length;
> +};
> +
> +The command uses PDH key to establish a new cryptographic context with the
> +remote platform - the new cryptographic context will be used for re-encrypting
> +the guest memory before sending it to remote platform.
> +
> +If length of the certificate blobs are too small, the required length is

So you wanna say "If the certificate blobs are short, ... "

> +returned in the length field and an error is returned.
> +
> +9. KVM_SEV_SEND_UPDATE_DATA
> +
> +Parameters (in): struct kvm_sev_send_update_data
> +Returns: 0 on success, -negative on error
> +
> +SEND_UPDATE_DATA command is used to re-encrypt the guest memory using the
> +crytographic context established during SEND_START. A fresh IV is generated
> +and written to the packet header field.
> +
> +struct kvm_sev_send_update_data {
> +	/* address which will contain packet header (IV, MAC etc)*/
> +	__u64 hdr_data;
> +	__u32 hdr_length;
> +
> +	/* address of guest memory region containg encrypted data */

"containing" - spellchecker needed.

> +	__u64 guest_address;
> +	__u32 guest_length;
> +
> +	/* address of transport buffer */
> +	__u64 host_address;
> +	__u32 host_length;
> +};
> +
> +If the hdr_length is too small, the required length is returned in the length
> +field and an error is returned.
> +
> +10. KVM_SEV_SEND_FINISH
> +
> +Returns: 0 on success, -negative on error
> +
> +SEND_FINISH command finalize the SEV guest sending process.

"finalizes"

> +
> +11. KVM_SEV_RECEIVE_START
> +
> +Parameters (in): struct kvm_sev_receive_start
> +Returns: 0 on success, -negative on error
> +
> +RECEIVE_START command is used to import a guest from one platform to another.
> +It can be used for restoring a guest from disk, or it can be used to migrate
> +a guest across the network from a sending platform.

Same issues as above.

Also, the explanatory text doesn't say who calls that command. If it is
called, KVM_SEV_RECEIVE_START, so it must be the receiving end but it is
unclear.

> +
> +struct kvm_sev_receive_start {
> +	/* guest handle (if zero then new handle will be created) */
> +	__u32 handle;
> +
> +	/* guest policy */
> +	__u32 policy;
> +
> +	/* Address containing senders PDH certificate blob */
> +	__u64 pdh_cert_address;
> +	__u32 pdh_cert_length;
> +
> +	/* Address containing sender's session information blob */
> +	__u64 session_address;
> +	__u32 session_length;
> +};
> +
> +The RECEIVE_START command creates a new cryptographic context necessary to
> +re-enrypt the guest memory receieved through the RECEIVE_UPDATE command.

"re-encrypt" - typo. Also "received"

Also, what is the RECEIVE_UPDATE command? The
KVM_SEV_RECEIVE_UPDATE_DATA below? See what I mean with ambiguities.

> +
> +12. KVM_SEV_RECEIVE_UPDATE_DATA
> +
> +Parameters (in): struct kvm_sev_receive_update_data
> +Returns: 0 on success, -negative on error
> +
> +RECEIVE_UPDATE_DATA command is used to re-encrypt the guest memory using the
> +crytographic context established during RECEIVE_START.
> +
> +struct kvm_sev_receive_update_data {
> +	/* packet header receieved from the SEND_UPDATE_DATA command */
> +	__u64 hdr_data;
> +	__u32 hdr_length;
> +
> +	/* address of guest memory region */
> +	__u64 guest_address;
> +	__u32 guest_length;
> +
> +	/* address of transport buffer */
> +	__u64 host_address;
> +	__u32 host_length;
> +};
> +
> +13. KVM_SEV_RECEIVE_FINISH
> +
> +Returns: 0 on success, -negative on error
> +
> +RECEIVE_FINISH command finalize the SEV guest receiving process.

Also, "finalizes".

Phew, that took long.

-- 
Regards/Gruss,
    Boris.

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