On Mon, Jul 24, 2017 at 03:02:59PM -0500, Brijesh Singh wrote:
> The SEV memory encryption engine uses a tweak such that two identical
> plaintexts at different location will have a different ciphertexts.
plaintexts or plaintext pages? also, s/a //
> So swapping or moving ciphertexts of two pages will not result in
> plaintexts being swapped. Relocating (or migrating) a physical backing
s/a //
> pages for SEV guest will require some additional steps. The current SEV
"for a SEV guest"
> key management spec does not provide commands to swap or migrate (move)
> ciphertexts. For now, we pin the guest memory registered through
> KVM_MEMORY_ENCRYPT_REGISTER_RAM ioctl.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/svm.c | 113 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 114 insertions(+)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 150177e..a91aadf 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -747,6 +747,7 @@ struct kvm_sev_info {
> unsigned int handle; /* firmware handle */
> unsigned int asid; /* asid for this guest */
> int sev_fd; /* SEV device fd */
> + struct list_head ram_list; /* list of registered ram */
regions_list I guess.
> };
>
> struct kvm_arch {
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 75dcaa9..cdb1cf3 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -333,8 +333,19 @@ static int sev_asid_new(void);
> static void sev_asid_free(int asid);
> static void sev_deactivate_handle(struct kvm *kvm, int *error);
> static void sev_decommission_handle(struct kvm *kvm, int *error);
> +static void sev_unpin_memory(struct page **pages, unsigned long npages);
Unneeded.
> +
> #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
>
> +struct kvm_sev_pin_ram {
sev_pinned_region
> + struct list_head list;
> + unsigned long npages;
> + struct page **pages;
> + struct kvm_memory_encrypt_ram userspace;
That member would need a comment what it is.
> +};
> +
> +static void __mem_encrypt_unregister_ram(struct kvm_sev_pin_ram *ram);
Move code so that you don't need that one.
> +
> static bool svm_sev_enabled(void)
> {
> return !!max_sev_asid;
> @@ -385,6 +396,11 @@ static inline void sev_set_fd(struct kvm *kvm, int fd)
> to_sev_info(kvm)->sev_fd = fd;
> }
>
> +static inline struct list_head *sev_get_ram_list(struct kvm *kvm)
> +{
> + return &to_sev_info(kvm)->ram_list;
> +}
> +
> static inline void mark_all_dirty(struct vmcb *vmcb)
> {
> vmcb->control.clean = 0;
> @@ -1566,10 +1582,24 @@ static void sev_firmware_uninit(void)
> static void sev_vm_destroy(struct kvm *kvm)
> {
> int state, error;
> + struct list_head *pos, *q;
> + struct kvm_sev_pin_ram *ram;
> + struct list_head *head = sev_get_ram_list(kvm);
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
>
> if (!sev_guest(kvm))
> return;
>
> + /*
> + * if userspace was terminated before unregistering the memory region
> + * then lets unpin all the registered memory.
> + */
> + if (!list_empty(head)) {
> + list_for_each_safe(pos, q, head) {
> + ram = list_entry(pos, struct kvm_sev_pin_ram, list);
> + __mem_encrypt_unregister_ram(ram);
You don't need the local "ram" varible here:
__mem_encrypt_unregister_ram(list_entry(pos, struct kvm_sev_pin_ram, list));
> + }
> + }
> +
> /* release the firmware resources for this guest */
> if (sev_get_handle(kvm)) {
> sev_deactivate_handle(kvm, &error);
> @@ -5640,6 +5670,7 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> sev_set_active(kvm);
> sev_set_asid(kvm, asid);
> sev_set_fd(kvm, argp->sev_fd);
> + INIT_LIST_HEAD(sev_get_ram_list(kvm));
> ret = 0;
> e_err:
> fdput(f);
> @@ -6437,6 +6468,86 @@ static int svm_memory_encryption_op(struct kvm *kvm, void __user *argp)
> return r;
> }
>
> +static int mem_encrypt_register_ram(struct kvm *kvm,
> + struct kvm_memory_encrypt_ram *ram)
> +{
Please call that arg "regions" or so. "ram" is strange. In the other
functions too.
> + struct list_head *head = sev_get_ram_list(kvm);
> + struct kvm_sev_pin_ram *pin_ram;
> +
> + if (!sev_guest(kvm))
> + return -ENOTTY;
> +
> + pin_ram = kzalloc(sizeof(*pin_ram), GFP_KERNEL);
> + if (!pin_ram)
> + return -ENOMEM;
> +
> + pin_ram->pages = sev_pin_memory(ram->address, ram->size,
> + &pin_ram->npages, 1);
Let it stick out.
> + if (!pin_ram->pages)
> + goto e_free;
> +
> + /*
> + * Guest may change the memory encryption attribute from C=0 -> C=1
"The guest"
> + * for this memory range. Lets make sure caches are flushed to ensure
> + * that guest data gets written into memory with correct C-bit.
> + */
> + sev_clflush_pages(pin_ram->pages, pin_ram->npages);
> +
> + pin_ram->userspace.address = ram->address;
> + pin_ram->userspace.size = ram->size;
> + list_add_tail(&pin_ram->list, head);
> + return 0;
<---- newline here.
> +e_free:
> + kfree(pin_ram);
> + return 1;
> +}
> +
> +static struct kvm_sev_pin_ram *sev_find_pinned_ram(struct kvm *kvm,
> + struct kvm_memory_encrypt_ram *ram)
So this function signature is almost impossible to read: you have "kvm"
"sev" "pin" "ram" and those long structure names.
Now look how something like this:
static struct regions_list *
sev_find_pinned_memory(struct kvm *kvm, struct enc_range *range)
tells you exactly what the function does.
> +{
> + struct list_head *head = sev_get_ram_list(kvm);
> + struct kvm_sev_pin_ram *i;
> +
> + list_for_each_entry(i, head, list) {
> + if (i->userspace.address == ram->address &&
> + i->userspace.size == ram->size)
if (i->usr.addr == reg->addr &&
i->usr.size == reg->size)
reads much better to me.
> + return i;
> + }
> +
> + return NULL;
> +}
> +
> +static void __mem_encrypt_unregister_ram(struct kvm_sev_pin_ram *ram)
> +{
> + /*
> + * Guest may have changed the memory encryption attribute from
"The guest"
> + * C=0 -> C=1. Lets make sure caches are flushed to ensure in data
Both comments talk about the 0 -> 1 case for the C-bit. What about the
reverse: 1->0? Do we not flush there or we don't have cases where a
guest doesn't decrypt its memory?
> + * gets written into memory with correct C-bit.
> + */
> + sev_clflush_pages(ram->pages, ram->npages);
> +
> + sev_unpin_memory(ram->pages, ram->npages);
> + list_del(&ram->list);
> + kfree(ram);
> +}
> +
> +static int mem_encrypt_unregister_ram(struct kvm *kvm,
> + struct kvm_memory_encrypt_ram *ram)
> +{
> + struct kvm_sev_pin_ram *pinned_ram;
> +
> + if (!sev_guest(kvm))
> + return -ENOTTY;
> +
> + pinned_ram = sev_find_pinned_ram(kvm, ram);
> + if (!pinned_ram)
> + return -EINVAL;
> +
> + __mem_encrypt_unregister_ram(pinned_ram);
> +
> + return 0;
> +}
> +
> static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
> .cpu_has_kvm_support = has_svm,
> .disabled_by_bios = is_disabled,
> @@ -6551,6 +6662,8 @@ static struct kvm_x86_ops svm_x86_ops __ro_after_init = {
> .setup_mce = svm_setup_mce,
>
> .memory_encryption_op = svm_memory_encryption_op,
> + .memory_encryption_register_ram = mem_encrypt_register_ram,
> + .memory_encryption_unregister_ram = mem_encrypt_unregister_ram,
Names are too long. mem_encrypt_reg_memory or so I guess. In general,
choose a prefix and stick with it. mem_enc, mem_encrypt, mem_crypt...
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
