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 <[email protected]> > --- > 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) --
