lkml.org 
[lkml]   [2017]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC Part2 PATCH v3 22/26] KVM: SVM: Pin guest memory when SEV is active
    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@amd.com>
    > ---
    > 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)
    --

    \
     
     \ /
      Last update: 2017-09-14 19:28    [W:3.601 / U:0.324 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site