Messages in this thread | | | Date | Tue, 5 Sep 2017 19:21:30 +0200 | From | Borislav Petkov <> | Subject | Re: [RFC Part2 PATCH v3 01/26] Documentation/virtual/kvm: Add AMD Secure Encrypted Virtualization (SEV) |
| |
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) --
| |