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@xxxxxxx>
> ---
> .../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)
--
