From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752623AbdIERVn (ORCPT ); Tue, 5 Sep 2017 13:21:43 -0400 Received: from mx2.suse.de ([195.135.220.15]:42217 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752539AbdIERVk (ORCPT ); Tue, 5 Sep 2017 13:21:40 -0400 Date: Tue, 5 Sep 2017 19:21:30 +0200 From: Borislav Petkov To: Brijesh Singh Cc: linux-kernel@vger.kernel.org, x86@kernel.org, kvm@vger.kernel.org, Thomas Gleixner , Joerg Roedel , "Michael S . Tsirkin" , Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= , Tom Lendacky 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > .../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) --