diff mbox series

[V4,3/8] KVM: selftests: add hooks for managing encrypted guest memory

Message ID 20220829171021.701198-4-pgonda@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: selftests: Add simple SEV test | expand

Commit Message

Peter Gonda Aug. 29, 2022, 5:10 p.m. UTC 
From: Michael Roth <michael.roth@amd.com>

VM implementations that make use of encrypted memory need a way to
configure things like the encryption/shared bit position for page
table handling, the default encryption policy for internal allocations
made by the core library, and a way to fetch the list/bitmap of
encrypted pages to do the actual memory encryption. Add an interface to
configure these parameters. Also introduce a sparsebit map to track
allocations/mappings that should be treated as encrypted, and provide
a way for VM implementations to retrieve it to handle operations
related memory encryption.

Reviewed-by: Mingwei Zhang <mizhang@google.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
---
 .../selftests/kvm/include/kvm_util_base.h     | 17 ++++++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 52 +++++++++++++++++--
 2 files changed, 66 insertions(+), 3 deletions(-)

Comments

Sean Christopherson Oct. 6, 2022, 5:48 p.m. UTC | #1 
On Mon, Aug 29, 2022, Peter Gonda wrote:
> +static vm_paddr_t
> +_vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, vm_paddr_t paddr_min,

Do not wrap before the function name.  Linus has a nice explanation/rant on this[*].
Note to self, add a Vim macro for this...

[*] https://lore.kernel.org/all/CAHk-=wjoLAYG446ZNHfg=GhjSY6nFmuB_wA8fYd5iLBNXjo9Bw@mail.gmail.com

> +		    uint32_t memslot, bool encrypt)
>  {
>  	struct userspace_mem_region *region;
>  	sparsebit_idx_t pg, base;
> @@ -1152,12 +1156,22 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
>  		abort();
>  	}
>  
> -	for (pg = base; pg < base + num; ++pg)
> +	for (pg = base; pg < base + num; ++pg) {
>  		sparsebit_clear(region->unused_phy_pages, pg);
> +		if (encrypt)

prefer s/encrypt/private, and s/encrypted_phy_pages/private_phy_pages.  pKVM
doesn't rely on encryption, and it's not impossible that x86 will someday gain
similar functionality.  And "encrypted" is also technically wrong for SEV and TDX,
as shared memory can also be encrypted with a common key.

> +			sparsebit_set(region->encrypted_phy_pages, pg);
> +	}
>  
>  	return base * vm->page_size;
>  }
>  
> +vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> +			      vm_paddr_t paddr_min, uint32_t memslot)
> +{
> +	return _vm_phy_pages_alloc(vm, num, paddr_min, memslot,
> +				   vm->memcrypt.enc_by_default);

enc_by_default yields a bizarre API.  The behavior depends on whether or not the
VM is protected, and whether or not the VM wants to protect memory by default.

For simplicity, IMO vm_phy_pages_alloc() should allocate memory as private if the
VM supports protected memory, i.e. just have vm->protected or whatever and use
that here.

> +}
> +
>  vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
>  			     uint32_t memslot)
>  {
> @@ -1741,6 +1755,10 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
>  			region->host_mem);
>  		fprintf(stream, "%*sunused_phy_pages: ", indent + 2, "");
>  		sparsebit_dump(stream, region->unused_phy_pages, 0);
> +		if (vm->memcrypt.enabled) {

vm->protected

> +			fprintf(stream, "%*sencrypted_phy_pages: ", indent + 2, "");
> +			sparsebit_dump(stream, region->encrypted_phy_pages, 0);
> +		}
>  	}
>  	fprintf(stream, "%*sMapped Virtual Pages:\n", indent, "");
>  	sparsebit_dump(stream, vm->vpages_mapped, indent + 2);
> @@ -1989,3 +2007,31 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
>  		break;
>  	}
>  }
> +
> +void vm_set_memory_encryption(struct kvm_vm *vm, bool enc_by_default, bool has_enc_bit,
> +			      uint8_t enc_bit)
> +{
> +	vm->memcrypt.enabled = true;
> +	vm->memcrypt.enc_by_default = enc_by_default;
> +	vm->memcrypt.has_enc_bit = has_enc_bit;
> +	vm->memcrypt.enc_bit = enc_bit;
> +}
> +
> +const struct sparsebit *
> +vm_get_encrypted_phy_pages(struct kvm_vm *vm, int slot, vm_paddr_t *gpa_start,
> +			   uint64_t *size)

Bad wrap.

> +{
> +	struct userspace_mem_region *region;
> +
> +	if (!vm->memcrypt.enabled)

This seems rather silly, why not TEST_ASSERT()?

> +		return NULL;
> +
> +	region = memslot2region(vm, slot);
> +	if (!region)

Same here, TEST_ASSERT() seems more appropriate.

Actually, I can't envision a use outside of SEV.  AFAIK, no other architecture
does the whole "launch update" thing.  I.e. just open code this in sev_encrypt().
The more generic API that will be useful for other VM types will be to query if a
specific GPA is private vs. shared.

> +		return NULL;
> +
> +	*size = region->region.memory_size;
> +	*gpa_start = region->region.guest_phys_addr;
> +
> +	return region->encrypted_phy_pages;
> +}
> -- 
> 2.37.2.672.g94769d06f0-goog
>
Peter Gonda Oct. 11, 2022, 5:38 p.m. UTC | #2 
On Thu, Oct 6, 2022 at 11:48 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Aug 29, 2022, Peter Gonda wrote:
> > +static vm_paddr_t
> > +_vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, vm_paddr_t paddr_min,
>
> Do not wrap before the function name.  Linus has a nice explanation/rant on this[*].
> Note to self, add a Vim macro for this...
>
> [*] https://lore.kernel.org/all/CAHk-=wjoLAYG446ZNHfg=GhjSY6nFmuB_wA8fYd5iLBNXjo9Bw@mail.gmail.com
>

Fixed. Thanks. I'll work on a fix to my VIM setup.

> > +                 uint32_t memslot, bool encrypt)
> >  {
> >       struct userspace_mem_region *region;
> >       sparsebit_idx_t pg, base;
> > @@ -1152,12 +1156,22 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> >               abort();
> >       }
> >
> > -     for (pg = base; pg < base + num; ++pg)
> > +     for (pg = base; pg < base + num; ++pg) {
> >               sparsebit_clear(region->unused_phy_pages, pg);
> > +             if (encrypt)
>
> prefer s/encrypt/private, and s/encrypted_phy_pages/private_phy_pages.  pKVM
> doesn't rely on encryption, and it's not impossible that x86 will someday gain
> similar functionality.  And "encrypted" is also technically wrong for SEV and TDX,
> as shared memory can also be encrypted with a common key.

Makes sense. Private or protected sound better.

>
> > +                     sparsebit_set(region->encrypted_phy_pages, pg);
> > +     }
> >
> >       return base * vm->page_size;
> >  }
> >
> > +vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > +                           vm_paddr_t paddr_min, uint32_t memslot)
> > +{
> > +     return _vm_phy_pages_alloc(vm, num, paddr_min, memslot,
> > +                                vm->memcrypt.enc_by_default);
>
> enc_by_default yields a bizarre API.  The behavior depends on whether or not the
> VM is protected, and whether or not the VM wants to protect memory by default.
>
> For simplicity, IMO vm_phy_pages_alloc() should allocate memory as private if the
> VM supports protected memory, i.e. just have vm->protected or whatever and use
> that here.

Removed "enc_by_default" concept.

>
> > +}
> > +
> >  vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
> >                            uint32_t memslot)
> >  {
> > @@ -1741,6 +1755,10 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
> >                       region->host_mem);
> >               fprintf(stream, "%*sunused_phy_pages: ", indent + 2, "");
> >               sparsebit_dump(stream, region->unused_phy_pages, 0);
> > +             if (vm->memcrypt.enabled) {
>
> vm->protected

renamed memcrypt -> protected.

>
> > +                     fprintf(stream, "%*sencrypted_phy_pages: ", indent + 2, "");
> > +                     sparsebit_dump(stream, region->encrypted_phy_pages, 0);
> > +             }
> >       }
> >       fprintf(stream, "%*sMapped Virtual Pages:\n", indent, "");
> >       sparsebit_dump(stream, vm->vpages_mapped, indent + 2);
> > @@ -1989,3 +2007,31 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
> >               break;
> >       }
> >  }
> > +
> > +void vm_set_memory_encryption(struct kvm_vm *vm, bool enc_by_default, bool has_enc_bit,
> > +                           uint8_t enc_bit)
> > +{
> > +     vm->memcrypt.enabled = true;
> > +     vm->memcrypt.enc_by_default = enc_by_default;
> > +     vm->memcrypt.has_enc_bit = has_enc_bit;
> > +     vm->memcrypt.enc_bit = enc_bit;
> > +}
> > +
> > +const struct sparsebit *
> > +vm_get_encrypted_phy_pages(struct kvm_vm *vm, int slot, vm_paddr_t *gpa_start,
> > +                        uint64_t *size)
>
> Bad wrap.

Fixed.

>
> > +{
> > +     struct userspace_mem_region *region;
> > +
> > +     if (!vm->memcrypt.enabled)
>
> This seems rather silly, why not TEST_ASSERT()?
>
> > +             return NULL;
> > +
> > +     region = memslot2region(vm, slot);
> > +     if (!region)
>
> Same here, TEST_ASSERT() seems more appropriate.
>
> Actually, I can't envision a use outside of SEV.  AFAIK, no other architecture
> does the whole "launch update" thing.  I.e. just open code this in sev_encrypt().
> The more generic API that will be useful for other VM types will be to query if a
> specific GPA is private vs. shared.

Good point. I'll move this code into that sev_encrypt() flow and
remove this function completely.

>
> > +             return NULL;
> > +
> > +     *size = region->region.memory_size;
> > +     *gpa_start = region->region.guest_phys_addr;
> > +
> > +     return region->encrypted_phy_pages;
> > +}
> > --
> > 2.37.2.672.g94769d06f0-goog
> >
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 59d52b58a1a6..5ecde5ad4c2f 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -33,6 +33,7 @@  typedef uint64_t vm_vaddr_t; /* Virtual Machine (Guest) virtual address */
 struct userspace_mem_region {
 	struct kvm_userspace_memory_region region;
 	struct sparsebit *unused_phy_pages;
+	struct sparsebit *encrypted_phy_pages;
 	int fd;
 	off_t offset;
 	void *host_mem;
@@ -65,6 +66,14 @@  struct userspace_mem_regions {
 	DECLARE_HASHTABLE(slot_hash, 9);
 };
 
+/* Memory encryption policy/configuration. */
+struct vm_memcrypt {
+	bool enabled;
+	int8_t enc_by_default;
+	bool has_enc_bit;
+	int8_t enc_bit;
+};
+
 struct kvm_vm {
 	int mode;
 	unsigned long type;
@@ -89,6 +98,7 @@  struct kvm_vm {
 	vm_vaddr_t idt;
 	vm_vaddr_t handlers;
 	uint32_t dirty_ring_size;
+	struct vm_memcrypt memcrypt;
 
 	/* Cache of information for binary stats interface */
 	int stats_fd;
@@ -849,4 +859,11 @@  static inline int __vm_disable_nx_huge_pages(struct kvm_vm *vm)
 	return __vm_enable_cap(vm, KVM_CAP_VM_DISABLE_NX_HUGE_PAGES, 0);
 }
 
+void vm_set_memory_encryption(struct kvm_vm *vm, bool enc_by_default, bool has_enc_bit,
+			      uint8_t enc_bit);
+
+const struct sparsebit *vm_get_encrypted_phy_pages(struct kvm_vm *vm, int slot,
+						   vm_paddr_t *gpa_start,
+						   uint64_t *size);
+
 #endif /* SELFTEST_KVM_UTIL_BASE_H */
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 06559994711e..53b9a509c1d5 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -553,6 +553,7 @@  static void __vm_mem_region_delete(struct kvm_vm *vm,
 	vm_ioctl(vm, KVM_SET_USER_MEMORY_REGION, &region->region);
 
 	sparsebit_free(&region->unused_phy_pages);
+	sparsebit_free(&region->encrypted_phy_pages);
 	ret = munmap(region->mmap_start, region->mmap_size);
 	TEST_ASSERT(!ret, __KVM_SYSCALL_ERROR("munmap()", ret));
 
@@ -893,6 +894,7 @@  void vm_userspace_mem_region_add(struct kvm_vm *vm,
 	}
 
 	region->unused_phy_pages = sparsebit_alloc();
+	region->encrypted_phy_pages = sparsebit_alloc();
 	sparsebit_set_num(region->unused_phy_pages,
 		guest_paddr >> vm->page_shift, npages);
 	region->region.slot = slot;
@@ -1108,6 +1110,7 @@  struct kvm_vcpu *__vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
  *   num - number of pages
  *   paddr_min - Physical address minimum
  *   memslot - Memory region to allocate page from
+ *   encrypt - Whether to treat the pages as encrypted
  *
  * Output Args: None
  *
@@ -1119,8 +1122,9 @@  struct kvm_vcpu *__vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpu_id)
  * and their base address is returned. A TEST_ASSERT failure occurs if
  * not enough pages are available at or above paddr_min.
  */
-vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
-			      vm_paddr_t paddr_min, uint32_t memslot)
+static vm_paddr_t
+_vm_phy_pages_alloc(struct kvm_vm *vm, size_t num, vm_paddr_t paddr_min,
+		    uint32_t memslot, bool encrypt)
 {
 	struct userspace_mem_region *region;
 	sparsebit_idx_t pg, base;
@@ -1152,12 +1156,22 @@  vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
 		abort();
 	}
 
-	for (pg = base; pg < base + num; ++pg)
+	for (pg = base; pg < base + num; ++pg) {
 		sparsebit_clear(region->unused_phy_pages, pg);
+		if (encrypt)
+			sparsebit_set(region->encrypted_phy_pages, pg);
+	}
 
 	return base * vm->page_size;
 }
 
+vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
+			      vm_paddr_t paddr_min, uint32_t memslot)
+{
+	return _vm_phy_pages_alloc(vm, num, paddr_min, memslot,
+				   vm->memcrypt.enc_by_default);
+}
+
 vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
 			     uint32_t memslot)
 {
@@ -1741,6 +1755,10 @@  void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
 			region->host_mem);
 		fprintf(stream, "%*sunused_phy_pages: ", indent + 2, "");
 		sparsebit_dump(stream, region->unused_phy_pages, 0);
+		if (vm->memcrypt.enabled) {
+			fprintf(stream, "%*sencrypted_phy_pages: ", indent + 2, "");
+			sparsebit_dump(stream, region->encrypted_phy_pages, 0);
+		}
 	}
 	fprintf(stream, "%*sMapped Virtual Pages:\n", indent, "");
 	sparsebit_dump(stream, vm->vpages_mapped, indent + 2);
@@ -1989,3 +2007,31 @@  void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data,
 		break;
 	}
 }
+
+void vm_set_memory_encryption(struct kvm_vm *vm, bool enc_by_default, bool has_enc_bit,
+			      uint8_t enc_bit)
+{
+	vm->memcrypt.enabled = true;
+	vm->memcrypt.enc_by_default = enc_by_default;
+	vm->memcrypt.has_enc_bit = has_enc_bit;
+	vm->memcrypt.enc_bit = enc_bit;
+}
+
+const struct sparsebit *
+vm_get_encrypted_phy_pages(struct kvm_vm *vm, int slot, vm_paddr_t *gpa_start,
+			   uint64_t *size)
+{
+	struct userspace_mem_region *region;
+
+	if (!vm->memcrypt.enabled)
+		return NULL;
+
+	region = memslot2region(vm, slot);
+	if (!region)
+		return NULL;
+
+	*size = region->region.memory_size;
+	*gpa_start = region->region.guest_phys_addr;
+
+	return region->encrypted_phy_pages;
+}