On Tue, Oct 5, 2021 at 4:46 PM Michael Roth michael.roth@amd.com wrote:

Subsequent patches will break some of this code out into file-local helper functions, which will be used by functions like vm_vaddr_alloc(), which currently are defined earlier in the file, so a forward declaration would be needed.

Instead, move it earlier in the file, just above vm_vaddr_alloc() and and friends, which are the main users.

Signed-off-by: Michael Roth michael.roth@amd.com

tools/testing/selftests/kvm/lib/kvm_util.c | 146 ++++++++++----------- 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 10a8ed691c66..92f59adddebe 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1145,6 +1145,79 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid) list_add(&vcpu->list, &vm->vcpus); }

+/*

• • Physical Contiguous Page Allocator
• • Input Args:
• • vm - Virtual Machine
• • num - number of pages
• • paddr_min - Physical address minimum
• • memslot - Memory region to allocate page from
• • Output Args: None
• • Return:
• • Starting physical address
• • Within the VM specified by vm, locates a range of available physical
• • pages at or above paddr_min. If found, the pages are marked as in use
• • 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)
  

+{

•   struct userspace_mem_region *region;
  

•   sparsebit_idx_t pg, base;
  

•   TEST_ASSERT(num > 0, "Must allocate at least one page");
  

•   TEST_ASSERT((paddr_min % vm->page_size) == 0, "Min physical address "
  

•           "not divisible by page size.\n"
  

•           "  paddr_min: 0x%lx page_size: 0x%x",
  

•           paddr_min, vm->page_size);
  

•   region = memslot2region(vm, memslot);
  

•   base = pg = paddr_min >> vm->page_shift;
  

•   do {
  

•           for (; pg < base + num; ++pg) {
  

•                   if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
  

•                           base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
  

•                           break;
  

•                   }
  

•           }
  

•   } while (pg && pg != base + num);
  

•   if (pg == 0) {
  

•           fprintf(stderr, "No guest physical page available, "
  

•                   "paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
  

•                   paddr_min, vm->page_size, memslot);
  

•           fputs("---- vm dump ----\n", stderr);
  

•           vm_dump(stderr, vm, 2);
  

•           abort();
  

•   }
  

•   for (pg = base; pg < base + num; ++pg)
  

•           sparsebit_clear(region->unused_phy_pages, pg);
  

•   return base * vm->page_size;
  

+}

+vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,

•                        uint32_t memslot)
  

+{

•   return vm_phy_pages_alloc(vm, 1, paddr_min, memslot);
  

+}

+/* Arbitrary minimum physical address used for virtual translation tables. */ +#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000

+vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm) +{

•   return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
  

+}

/*

• VM Virtual Address Unused Gap

@@ -2149,79 +2222,6 @@ const char *exit_reason_str(unsigned int exit_reason) return "Unknown"; }

-/*

• • Physical Contiguous Page Allocator
• • Input Args:
• • vm - Virtual Machine
• • num - number of pages
• • paddr_min - Physical address minimum
• • memslot - Memory region to allocate page from
• • Output Args: None
• • Return:
• • Starting physical address
• • Within the VM specified by vm, locates a range of available physical
• • pages at or above paddr_min. If found, the pages are marked as in use
• • 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)
  

-{

•   struct userspace_mem_region *region;
  

•   sparsebit_idx_t pg, base;
  

•   TEST_ASSERT(num > 0, "Must allocate at least one page");
  

•   TEST_ASSERT((paddr_min % vm->page_size) == 0, "Min physical address "
  

•           "not divisible by page size.\n"
  

•           "  paddr_min: 0x%lx page_size: 0x%x",
  

•           paddr_min, vm->page_size);
  

•   region = memslot2region(vm, memslot);
  

•   base = pg = paddr_min >> vm->page_shift;
  

•   do {
  

•           for (; pg < base + num; ++pg) {
  

•                   if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
  

•                           base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
  

•                           break;
  

•                   }
  

•           }
  

•   } while (pg && pg != base + num);
  

•   if (pg == 0) {
  

•           fprintf(stderr, "No guest physical page available, "
  

•                   "paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
  

•                   paddr_min, vm->page_size, memslot);
  

•           fputs("---- vm dump ----\n", stderr);
  

•           vm_dump(stderr, vm, 2);
  

•           abort();
  

•   }
  

•   for (pg = base; pg < base + num; ++pg)
  

•           sparsebit_clear(region->unused_phy_pages, pg);
  

•   return base * vm->page_size;
  

-}

-vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,

•                        uint32_t memslot)
  

-{

•   return vm_phy_pages_alloc(vm, 1, paddr_min, memslot);
  

-}

-/* Arbitrary minimum physical address used for virtual translation tables. */ -#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000

-vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm) -{

•   return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
  

-}

/*

• Address Guest Virtual to Host Virtual

-- 2.25.1

Why move the function implementation? Maybe just adding a declaration at the top of kvm_util.c should suffice.

On Tue, Oct 26, 2021 at 8:52 AM Mingwei Zhang mizhang@google.com wrote:

On Wed, Oct 20, 2021 at 8:47 PM Michael Roth michael.roth@amd.com wrote:

...

On Mon, Oct 18, 2021 at 08:00:00AM -0700, Mingwei Zhang wrote:

...

On Tue, Oct 5, 2021 at 4:46 PM Michael Roth michael.roth@amd.com wrote:

...

Subsequent patches will break some of this code out into file-local helper functions, which will be used by functions like vm_vaddr_alloc(), which currently are defined earlier in the file, so a forward declaration would be needed.

Instead, move it earlier in the file, just above vm_vaddr_alloc() and and friends, which are the main users.

Signed-off-by: Michael Roth michael.roth@amd.com

tools/testing/selftests/kvm/lib/kvm_util.c | 146 ++++++++++----------- 1 file changed, 73 insertions(+), 73 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 10a8ed691c66..92f59adddebe 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1145,6 +1145,79 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid) list_add(&vcpu->list, &vm->vcpus); }

+/*

• • Physical Contiguous Page Allocator
• • Input Args:
• • vm - Virtual Machine
• • num - number of pages
• • paddr_min - Physical address minimum
• • memslot - Memory region to allocate page from
• • Output Args: None
• • Return:
• • Starting physical address
• • Within the VM specified by vm, locates a range of available physical
• • pages at or above paddr_min. If found, the pages are marked as in use
• • 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)
  

+{

•   struct userspace_mem_region *region;
  

•   sparsebit_idx_t pg, base;
  

•   TEST_ASSERT(num > 0, "Must allocate at least one page");
  

•   TEST_ASSERT((paddr_min % vm->page_size) == 0, "Min physical address "
  

•           "not divisible by page size.\n"
  

•           "  paddr_min: 0x%lx page_size: 0x%x",
  

•           paddr_min, vm->page_size);
  

•   region = memslot2region(vm, memslot);
  

•   base = pg = paddr_min >> vm->page_shift;
  

•   do {
  

•           for (; pg < base + num; ++pg) {
  

•                   if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
  

•                           base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
  

•                           break;
  

•                   }
  

•           }
  

•   } while (pg && pg != base + num);
  

•   if (pg == 0) {
  

•           fprintf(stderr, "No guest physical page available, "
  

•                   "paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
  

•                   paddr_min, vm->page_size, memslot);
  

•           fputs("---- vm dump ----\n", stderr);
  

•           vm_dump(stderr, vm, 2);
  

•           abort();
  

•   }
  

•   for (pg = base; pg < base + num; ++pg)
  

•           sparsebit_clear(region->unused_phy_pages, pg);
  

•   return base * vm->page_size;
  

+}

+vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,

•                        uint32_t memslot)
  

+{

•   return vm_phy_pages_alloc(vm, 1, paddr_min, memslot);
  

+}

+/* Arbitrary minimum physical address used for virtual translation tables. */ +#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000

+vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm) +{

•   return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
  

+}

/*

• VM Virtual Address Unused Gap

@@ -2149,79 +2222,6 @@ const char *exit_reason_str(unsigned int exit_reason) return "Unknown"; }

-/*

• • Physical Contiguous Page Allocator
• • Input Args:
• • vm - Virtual Machine
• • num - number of pages
• • paddr_min - Physical address minimum
• • memslot - Memory region to allocate page from
• • Output Args: None
• • Return:
• • Starting physical address
• • Within the VM specified by vm, locates a range of available physical
• • pages at or above paddr_min. If found, the pages are marked as in use
• • 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)
  

-{

•   struct userspace_mem_region *region;
  

•   sparsebit_idx_t pg, base;
  

•   TEST_ASSERT(num > 0, "Must allocate at least one page");
  

•   TEST_ASSERT((paddr_min % vm->page_size) == 0, "Min physical address "
  

•           "not divisible by page size.\n"
  

•           "  paddr_min: 0x%lx page_size: 0x%x",
  

•           paddr_min, vm->page_size);
  

•   region = memslot2region(vm, memslot);
  

•   base = pg = paddr_min >> vm->page_shift;
  

•   do {
  

•           for (; pg < base + num; ++pg) {
  

•                   if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
  

•                           base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
  

•                           break;
  

•                   }
  

•           }
  

•   } while (pg && pg != base + num);
  

•   if (pg == 0) {
  

•           fprintf(stderr, "No guest physical page available, "
  

•                   "paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
  

•                   paddr_min, vm->page_size, memslot);
  

•           fputs("---- vm dump ----\n", stderr);
  

•           vm_dump(stderr, vm, 2);
  

•           abort();
  

•   }
  

•   for (pg = base; pg < base + num; ++pg)
  

•           sparsebit_clear(region->unused_phy_pages, pg);
  

•   return base * vm->page_size;
  

-}

-vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,

•                        uint32_t memslot)
  

-{

•   return vm_phy_pages_alloc(vm, 1, paddr_min, memslot);
  

-}

-/* Arbitrary minimum physical address used for virtual translation tables. */ -#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000

-vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm) -{

•   return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
  

-}

/*

• Address Guest Virtual to Host Virtual

-- 2.25.1

Why move the function implementation? Maybe just adding a declaration at the top of kvm_util.c should suffice.

At least from working on other projects I'd gotten the impression that forward function declarations should be avoided if they can be solved by moving the function above the caller. Certainly don't mind taking your suggestion and dropping this patch if that's not the case here though.

Understood. Yes, I think it would be better to follow your experience then. I was thinking that if you move the code and then potentially git blame on that function might point to you :)

Thanks. -Mingwei

Reviewed-by: Mingwei Zhang mizhang@google.com

Thanks. -Mingwei

On 10/5/21 4:44 PM, Michael Roth wrote:

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.

Signed-off-by: Michael Roth michael.roth@amd.com

.../testing/selftests/kvm/include/kvm_util.h | 6 ++ tools/testing/selftests/kvm/lib/kvm_util.c | 63 +++++++++++++++++-- .../selftests/kvm/lib/kvm_util_internal.h | 10 +++ 3 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 010b59b13917..f417de80596c 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -348,6 +348,12 @@ int vm_create_device(struct kvm_vm *vm, struct kvm_create_device *cd); void assert_on_unhandled_exception(struct kvm_vm *vm, uint32_t vcpuid); +void vm_set_memory_encryption(struct kvm_vm *vm, bool enc_by_default, bool has_enc_bit,

• 	      uint8_t enc_bit);
  

+struct sparsebit *vm_get_encrypted_phy_pages(struct kvm_vm *vm, int slot,

• 			     vm_paddr_t *gpa_start,
  

• 			     uint64_t *size);
  

• /* Common ucalls */ enum { UCALL_NONE,

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 92f59adddebe..c58f930dedd2 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -631,6 +631,7 @@ static void __vm_mem_region_delete(struct kvm_vm *vm, "rc: %i errno: %i", ret, errno); sparsebit_free(&region->unused_phy_pages);

• sparsebit_free(&region->encrypted_phy_pages); ret = munmap(region->mmap_start, region->mmap_size); TEST_ASSERT(ret == 0, "munmap failed, rc: %i errno: %i", ret, errno);

@@ -924,6 +925,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;

@@ -1153,6 +1155,7 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)

• 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

@@ -1164,11 +1167,13 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)

• 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;
• vm_paddr_t gpa;

TEST_ASSERT(num > 0, "Must allocate at least one page"); @@ -1198,10 +1203,25 @@ 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);
  

• }
• gpa = base * vm->page_size;

• return base * vm->page_size;

• if (encrypt && vm->memcrypt.has_enc_bit)

• gpa |= (1ULL << vm->memcrypt.enc_bit);
  

• return gpa;

+}

+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, 1, paddr_min, memslot,

• 		   vm->memcrypt.enc_by_default);
  

  }

vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min, @@ -2146,6 +2166,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);

@@ -2343,3 +2367,34 @@ int vcpu_get_stats_fd(struct kvm_vm *vm, uint32_t vcpuid) return ioctl(vcpu->fd, KVM_GET_STATS_FD, NULL); }

+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;

+}

+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;
• struct sparsebit *encrypted_phy_pages;
• if (!vm->memcrypt.enabled)

• return NULL;
  

• region = memslot2region(vm, slot);
• if (!region)

• return NULL;
  

• encrypted_phy_pages = sparsebit_alloc();
• sparsebit_copy(encrypted_phy_pages, region->encrypted_phy_pages);
• *size = region->region.memory_size;
• *gpa_start = region->region.guest_phys_addr;
• return encrypted_phy_pages;

+} diff --git a/tools/testing/selftests/kvm/lib/kvm_util_internal.h b/tools/testing/selftests/kvm/lib/kvm_util_internal.h index a03febc24ba6..99ccab86115c 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util_internal.h +++ b/tools/testing/selftests/kvm/lib/kvm_util_internal.h @@ -16,6 +16,7 @@ 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;

@@ -44,6 +45,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;

@@ -67,6 +76,7 @@ struct kvm_vm { vm_vaddr_t idt; vm_vaddr_t handlers; uint32_t dirty_ring_size;

• struct vm_memcrypt memcrypt;

For readability, it's probably better to adopt a standard naming convention for structures, members and  functions ?  For example,

encrypted_phy_pages    ->     enc_phy_pages

struct vm_memcrypt  {    ->    struct vm_mem_enc {

struct vm_memcrypt memcrypt    ->    struct vm_mem_enc  mem_enc

vm_get_encrypted_phy_pages()    -> vm_get_enc_phy_pages

}; struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid);

On Thu, Oct 21, 2021 at 05:26:26PM +0200, Paolo Bonzini wrote:

On 06/10/21 01:44, Michael Roth wrote:

...

SEV guests rely on an encyption bit which resides within the range that current code treats as address bits. Guest code will expect these bits to be set appropriately in their page tables, whereas helpers like addr_gpa2hva() will expect these bits to be masked away prior to translation. Add proper handling for these cases.

This is not what you're doing below in addr_gpa2hva, though---or did I misunderstand?

The confusion is warranted, addr_gpa2hva() *doesn't* expect the C bit to be masked in advance so the wording is pretty confusing.

I think I was referring the fact that internally it doesn't need/want the C-bit, in this case it just masks it away as a convenience to callers, as opposed to the other functions modified in the patch that actually make use of it.

It's convenient because page table walkers/mappers make use of addr_gpa2hva() to do things like silently mask away C-bits via when translating PTEs to host addresses. We easily convert those callers from:

addr_gpa2hva(paddr)

to this:

addr_gpa2hva(addr_raw2gpa(paddr))

but now all new code needs to consider whether it might be dealing with C-bits or not prior to deciding to pass it to addr_gpa2hva() (or not really think about it, and add addr_gpa2raw() "just in case"). So since it's always harmless to mask it away silently addr_gpa2hva(), the logic/code seems to benefit a good deal if we indicate clearly that addr_gpa2hva() can accept a 'raw' GPA, and will ignore it completely.

But not a big deal either way if you prefer to keep that explicit. And commit message still needs to be clarified.

I may be wrong due to not actually having written the code, but I'd prefer if most of these APIs worked only if the C bit has already been stripped. In general it's quite unlikely for host code to deal with C=1 pages, so it's worth pointing out explicitly the cases where it does.

I've tried to indicate functions that expect the C-bit by adding the 'raw_' prefix to the gpa/paddr parameters, but as you pointed out with addr_gpa2hva() it's already a bit inconsistent in that regard, and there's a couple cases like virt_map() where I should use the 'raw_' prefix as well that I've missed here.

So that should be addressed, and maybe some additional comments/assertions might be warranted to guard against cases where the C-bit is passed in unexpectedly.

But I should probably re-assess why the C-bit is being passed around in the first place:

- vm_phy_page[s]_alloc() is the main 'source' for 'raw' GPAs with the C-bit set. it determines this based on vm_memcrypt encryption policy, and updates the encryption bitmask as well. - vm_phy_page[s]_alloc() is callable both in kvm_util lib as well as individual tests. - in theory, encoding the C-bit in the returned vm_paddr_t means that vm_phy_page[s]_alloc() callers can pass that directly into virt_map/virt_pg_map() and this will "just work" for both encrypted/non-encrypted guests. - by masking it away in addr_gpa2hva(), existing tests/code flow mostly "just works" as well.

But taking a closer look, in cases where vm_phy_page[s]_alloc() is called directly by tests, like set_memory_region_test, emulator_error_test, and smm_test, that raw GPA is compared to hardcoded non-raw GPAs, so they'd still end up needing fixups to work with the proposed transparent-SEV-mode stuff. And future code would need to be written to account for this, so it doesn't really "just work" after all..

So it's worth considering the alternative approach of *not* encoding the C-bit into GPAs returned by vm_phy_page[s]_alloc(). That would likely involve introducing something like addr_gpa2raw(), which adds in the C-bit according to the encryption bitmap as-needed. If we do that:

- virt_map()/virt_pg_map() still need to accept 'raw' GPAs, since they need to deal with cases where pages are being mapping that weren't allocated by vm_phy_page[s]_alloc(), and so aren't recorded in the bitmap. in those cases it is up to test code to provide the C-bit when needed (e.g. things like separate linear mappings for pa()-like stuff in guest code).

- for cases where vm_phy_page[s]_alloc() determines whether the page is encrypted, addr_gpa2raw() needs to be used to add back the C-bit prior to passing it to virt_map()/virt_pg_map(), both in the library and the test code. vm_vaddr_* allocations would handle all this under the covers as they do now.

So test code would need to consider cases where addr_gpa2raw() needs to be used to set the C-bit (which is basically only when they want to mix usage of the vm_phy_page[s]_alloc with their own mapping of the guest page tables, which doesn't seem to be done in any existing tests anyway).

The library code would need these addr_gpa2raw() hooks in places where it calls virt_*map() internally. Probably just a handful of places though.

Assuming there's no issues with this alternative approach that I may be missing, I'll look at doing it this way for the next spin.

Even in this alternative approach though, having addr_gpa2hva() silently mask away C-bit still seems useful for the reasons above, but again, no strong feelings one way or the other on that.

Paolo

...

@@ -1460,9 +1480,10 @@ void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,

• address providing the memory to the vm physical address is returned.
• A TEST_ASSERT failure occurs if no region containing gpa exists.

*/ -void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa) +void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa_raw) { struct userspace_mem_region *region;

On Wed, Oct 6, 2021 at 1:39 PM Michael Roth michael.roth@amd.com wrote:

Recent kernels have checks to ensure the GPA values in special-purpose registers like CR3 are within the maximum physical address range and don't overlap with anything in the upper/reserved range. In the case of SEV kselftest guests booting directly into 64-bit mode, CR3 needs to be initialized to the GPA of the page table root, with the encryption bit set. The kernel accounts for this encryption bit by removing it from reserved bit range when the guest advertises the bit position via KVM_SET_CPUID*, but kselftests currently call KVM_SET_SREGS as part of vm_vcpu_add_default(), *prior* to vCPU creation, so there's no opportunity to call KVM_SET_CPUID* in advance. As a result, KVM_SET_SREGS will return an error in these cases.

Address this by moving vcpu_set_cpuid() (which calls KVM_SET_CPUID*) ahead of vcpu_setup() (which calls KVM_SET_SREGS).

While there, address a typo in the assertion that triggers when KVM_SET_SREGS fails.

Suggested-by: Sean Christopherson seanjc@google.com Signed-off-by: Michael Roth michael.roth@amd.com

---

tools/testing/selftests/kvm/lib/kvm_util.c | 2 +- tools/testing/selftests/kvm/lib/x86_64/processor.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index ef88fdc7e46b..646cffd86d09 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1906,7 +1906,7 @@ void vcpu_sregs_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs) { int ret = _vcpu_sregs_set(vm, vcpuid, sregs);

•   TEST_ASSERT(ret == 0, "KVM_RUN IOCTL failed, "
  

•   TEST_ASSERT(ret == 0, "KVM_SET_SREGS IOCTL failed, "
            "rc: %i errno: %i", ret, errno);
  

}

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index 0bbd88fe1127..1ab4c20f5d12 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -660,6 +660,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)

    /* Create VCPU */
    vm_vcpu_add(vm, vcpuid);

•   vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
    vcpu_setup(vm, vcpuid);
  
    /* Setup guest general purpose registers */
  

@@ -672,9 +673,6 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code) /* Setup the MP state */ mp_state.mp_state = 0; vcpu_set_mp_state(vm, vcpuid, &mp_state);

•   /* Setup supported CPUIDs */
  

•   vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
  

}

/*

2.25.1

Good catch. Reviewed-by: Nathan Tempelman natet@google.com

On Wed, Oct 6, 2021 at 1:40 PM Michael Roth michael.roth@amd.com wrote:

Add interfaces to allow tests to create/manage SEV guests. The additional state associated with these guests is encapsulated in a new struct sev_vm, which is a light wrapper around struct kvm_vm. These VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() under the covers to configure and sync up with the core kvm_util library on what should/shouldn't be treated as encrypted memory.

Signed-off-by: Michael Roth michael.roth@amd.com

tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/include/x86_64/sev.h | 62 ++++ tools/testing/selftests/kvm/lib/x86_64/sev.c | 303 ++++++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 5832f510a16c..c7a5e1c69e0c 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -35,6 +35,7 @@ endif

LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S +LIBKVM_x86_64 += lib/x86_64/sev.c

Regarding RFC-level feedback: First off, I'm super jazzed with what I'm seeing so far! (While this is my first review, I've been studying the patches up through the SEV boot test, i.e., patch #7). One thing I'm wondering is: the way this is structured is to essentially split the test cases into non-SEV and SEV. I'm wondering how hard it would be to add some flag or environment variable to set up pre-existing tests to run under SEV. Or is this something you all thought about, and decided that it does not make sense?

Looking at how the guest memory is handled, it seems like it's not far off from handling SEV transparently across all test cases. I'd think that we could just default all memory to use the encryption bit, and then have test cases, such as the test case in patch #7, clear the encryption bit for shared pages. However, I think the VM creation would need a bit more refactoring to work with other test cases.

LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c

diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h new file mode 100644 index 000000000000..d2f41b131ecc --- /dev/null +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Helpers used for SEV guests
• • Copyright (C) 2021 Advanced Micro Devices
• */

+#ifndef SELFTEST_KVM_SEV_H +#define SELFTEST_KVM_SEV_H

+#include <stdint.h> +#include <stdbool.h> +#include "kvm_util.h"

+#define SEV_DEV_PATH "/dev/sev"

Not necessary for the initial patches, but eventually, it would be nice to make this configurable. On the machine I was using to test this, the sev device appears at `/mnt/devtmpfs/sev`

+#define SEV_FW_REQ_VER_MAJOR 1 +#define SEV_FW_REQ_VER_MINOR 30

Where does the requirement for this minimum version come from? Maybe add a comment?

Edit: Is this for patches later on in the series that exercise SNP? If so, I think it would be better to add a check like this in the test itself, rather than globally. I happened to test this on a machine with a very old PSP FW, 0.22, and the SEV test added in patch #7 seems to work fine with this ancient PSP FW.

+#define SEV_POLICY_NO_DBG (1UL << 0) +#define SEV_POLICY_ES (1UL << 2)

+#define SEV_GUEST_ASSERT(sync, token, _cond) do { \

•   if (!(_cond))                                   \
  

•           sev_guest_abort(sync, token, 0);        \
  

+} while (0)

+enum {

•   SEV_GSTATE_UNINIT = 0,
  

•   SEV_GSTATE_LUPDATE,
  

•   SEV_GSTATE_LSECRET,
  

•   SEV_GSTATE_RUNNING,
  

+};

+struct sev_sync_data {

•   uint32_t token;
  

•   bool pending;
  

•   bool done;
  

•   bool aborted;
  

•   uint64_t info;
  

+};

nit: This struct could use some comments. For example, `token` is not totally self-explanatory. In general, a comment explaining how this struct is intended to be used by test cases seems useful.

+struct sev_vm;

+void sev_guest_sync(struct sev_sync_data *sync, uint32_t token, uint64_t info); +void sev_guest_done(struct sev_sync_data *sync, uint32_t token, uint64_t info); +void sev_guest_abort(struct sev_sync_data *sync, uint32_t token, uint64_t info);

+void sev_check_guest_sync(struct kvm_run *run, struct sev_sync_data *sync,

•                     uint32_t token);
  

+void sev_check_guest_done(struct kvm_run *run, struct sev_sync_data *sync,

•                     uint32_t token);
  

+void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data); +struct kvm_vm *sev_get_vm(struct sev_vm *sev); +uint8_t sev_get_enc_bit(struct sev_vm *sev);

+struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages); +void sev_vm_free(struct sev_vm *sev); +void sev_vm_launch(struct sev_vm *sev); +void sev_vm_measure(struct sev_vm *sev, uint8_t *measurement); +void sev_vm_launch_finish(struct sev_vm *sev);

+#endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c new file mode 100644 index 000000000000..adda3b396566 --- /dev/null +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -0,0 +1,303 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Helpers used for SEV guests
• • Copyright (C) 2021 Advanced Micro Devices
• */

+#include <stdint.h> +#include <stdbool.h> +#include "kvm_util.h" +#include "linux/psp-sev.h" +#include "processor.h" +#include "sev.h"

+#define PAGE_SHIFT 12 +#define PAGE_SIZE (1UL << PAGE_SHIFT)

+struct sev_vm {

•   struct kvm_vm *vm;
  

•   int fd;
  

•   int enc_bit;
  

•   uint32_t sev_policy;
  

+};

+/* Helpers for coordinating between guests and test harness. */

+void sev_guest_sync(struct sev_sync_data *sync, uint32_t token, uint64_t info) +{

•   sync->token = token;
  

•   sync->info = info;
  

•   sync->pending = true;
  

•   asm volatile("hlt" : : : "memory");
  

+}

+void sev_guest_done(struct sev_sync_data *sync, uint32_t token, uint64_t info) +{

•   while (true) {
  

•           sync->done = true;
  

•           sev_guest_sync(sync, token, info);
  

•   }
  

+}

+void sev_guest_abort(struct sev_sync_data *sync, uint32_t token, uint64_t info) +{

•   while (true) {
  

•           sync->aborted = true;
  

•           sev_guest_sync(sync, token, info);
  

•   }
  

+}

Maybe this should have some logic to make sure it only gets executed once instead of a while loop. Something like:

void sev_guest_done(struct sev_sync_data *sync, uint32_t token, uint64_t info) { static bool abort = false;

SEV_GUEST_ASSERT(sync, 0xDEADBEEF, !abort); abort = true;

sync->done = true; sev_guest_sync(sync, token, info); }

+void sev_check_guest_sync(struct kvm_run *run, struct sev_sync_data *sync,

•                     uint32_t token)
  

+{

•   TEST_ASSERT(run->exit_reason == KVM_EXIT_HLT,
  

•               "unexpected exit reason: %u (%s)",
  

•               run->exit_reason, exit_reason_str(run->exit_reason));
  

•   TEST_ASSERT(sync->token == token,
  

•               "unexpected guest token, expected %d, got: %d", token,
  

•               sync->token);
  

•   TEST_ASSERT(!sync->done, "unexpected guest state");
  

•   TEST_ASSERT(!sync->aborted, "unexpected guest state");
  

•   sync->pending = false;
  

Check that `pending` is `true` before setting to `false`?

+}

+void sev_check_guest_done(struct kvm_run *run, struct sev_sync_data *sync,

•                     uint32_t token)
  

+{

•   TEST_ASSERT(run->exit_reason == KVM_EXIT_HLT,
  

•               "unexpected exit reason: %u (%s)",
  

•               run->exit_reason, exit_reason_str(run->exit_reason));
  

•   TEST_ASSERT(sync->token == token,
  

•               "unexpected guest token, expected %d, got: %d", token,
  

•               sync->token);
  

•   TEST_ASSERT(sync->done, "unexpected guest state");
  

•   TEST_ASSERT(!sync->aborted, "unexpected guest state");
  

•   sync->pending = false;
  

+}

nit: This function is nearly identical to `sev_check_guest_sync()`, other than the ASSERT for `sync->done`. Might be worth splitting out a common static helper or using a single function for both cases, and distinguishing between them with a function parameter.

+/* Common SEV helpers/accessors. */

+struct kvm_vm *sev_get_vm(struct sev_vm *sev) +{

•   return sev->vm;
  

+}

+uint8_t sev_get_enc_bit(struct sev_vm *sev) +{

•   return sev->enc_bit;
  

+}

+void sev_ioctl(int sev_fd, int cmd, void *data) +{

•   int ret;
  

•   struct sev_issue_cmd arg;
  

•   arg.cmd = cmd;
  

•   arg.data = (unsigned long)data;
  

nit: Should the cast be `(__u64)`, rather than `(unsigned long)`? This is how `data` is defined in `struct sev_issue_cmd`, and also how the `data` field is cast in `sev_ioctl`, below.

•   ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg);
  

•   TEST_ASSERT(ret == 0,
  

•               "SEV ioctl %d failed, error: %d, fw_error: %d",
  

•               cmd, ret, arg.error);
  

+}

+void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data) +{

•   struct kvm_sev_cmd arg = {0};
  

•   int ret;
  

•   arg.id = cmd;
  

•   arg.sev_fd = sev->fd;
  

•   arg.data = (__u64)data;
  

•   ret = ioctl(vm_get_fd(sev->vm), KVM_MEMORY_ENCRYPT_OP, &arg);
  

•   TEST_ASSERT(ret == 0,
  

•               "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d",
  

•               cmd, ret, errno, strerror(errno), arg.error);
  

nit: Technically, the `KVM_MEMORY_ENCRYPT_OP ` ioctl failed. The failure message should reflect this. Maybe something like:

"SEV KVM_MEMORY_ENCRYPT_OP ioctl w/ cmd: %d failed, ..."

+}

+/* Local helpers. */

+static void +sev_register_user_range(struct sev_vm *sev, void *hva, uint64_t size) +{

•   struct kvm_enc_region range = {0};
  

•   int ret;
  

•   pr_debug("register_user_range: hva: %p, size: %lu\n", hva, size);
  

•   range.addr = (__u64)hva;
  

•   range.size = size;
  

•   ret = ioctl(vm_get_fd(sev->vm), KVM_MEMORY_ENCRYPT_REG_REGION, &range);
  

•   TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno);
  

+}

I don't see any code to call `KVM_MEMORY_ENCRYPT_UNREG_REGION` in this code. Shouldn't we be calling it to unpin the memory when the test is done?

+static void +sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size) +{

•   struct kvm_sev_launch_update_data ksev_update_data = {0};
  

•   pr_debug("encrypt_phy_range: addr: 0x%lx, size: %lu\n", gpa, size);
  

•   ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa);
  

•   ksev_update_data.len = size;
  

•   kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data);
  

+}

+static void sev_encrypt(struct sev_vm *sev) +{

•   struct sparsebit *enc_phy_pages;
  

•   struct kvm_vm *vm = sev->vm;
  

•   sparsebit_idx_t pg = 0;
  

•   vm_paddr_t gpa_start;
  

•   uint64_t memory_size;
  

•   /* Only memslot 0 supported for now. */
  

•   enc_phy_pages = vm_get_encrypted_phy_pages(sev->vm, 0, &gpa_start, &memory_size);
  

•   TEST_ASSERT(enc_phy_pages, "Unable to retrieve encrypted pages bitmap");
  

•   while (pg < (memory_size / vm_get_page_size(vm))) {
  

•           sparsebit_idx_t pg_cnt;
  

•           if (sparsebit_is_clear(enc_phy_pages, pg)) {
  

•                   pg = sparsebit_next_set(enc_phy_pages, pg);
  

•                   if (!pg)
  

•                           break;
  

•           }
  

•           pg_cnt = sparsebit_next_clear(enc_phy_pages, pg) - pg;
  

•           if (pg_cnt <= 0)
  

•                   pg_cnt = 1;
  

The `pg_cnt <= 0` case doesn't seem correct. First off, I'd just git rid of the `<`, because `pg_cnt` is unsigned. Second, the comment header for `sparsebit_next_clear()` says that a return value of `0` indicates that no bits following `prev` are cleared. Thus, in this case, I'd think that `pg_cnt` should be set to `memory_size - pg`. So maybe something like:

end = sparsebit_next_clear(enc_phy_pages, pg); if (end == 0) end = memory_size; pg_cnt = end - pg;

•           sev_encrypt_phy_range(sev,
  

•                                 gpa_start + pg * vm_get_page_size(vm),
  

•                                 pg_cnt * vm_get_page_size(vm));
  

•           pg += pg_cnt;
  

•   }
  

•   sparsebit_free(&enc_phy_pages);
  

+}

+/* SEV VM implementation. */

+static struct sev_vm *sev_common_create(struct kvm_vm *vm) +{

•   struct sev_user_data_status sev_status = {0};
  

•   uint32_t eax, ebx, ecx, edx;
  

•   struct sev_vm *sev;
  

•   int sev_fd;
  

•   sev_fd = open(SEV_DEV_PATH, O_RDWR);
  

•   if (sev_fd < 0) {
  

•           pr_info("Failed to open SEV device, path: %s, error: %d, skipping test.\n",
  

•                   SEV_DEV_PATH, sev_fd);
  

•           return NULL;
  

•   }
  

•   sev_ioctl(sev_fd, SEV_PLATFORM_STATUS, &sev_status);
  

•   if (!(sev_status.api_major > SEV_FW_REQ_VER_MAJOR ||
  

•         (sev_status.api_major == SEV_FW_REQ_VER_MAJOR &&
  

•          sev_status.api_minor >= SEV_FW_REQ_VER_MINOR))) {
  

•           pr_info("SEV FW version too old. Have API %d.%d (build: %d), need %d.%d, skipping test.\n",
  

•                   sev_status.api_major, sev_status.api_minor, sev_status.build,
  

•                   SEV_FW_REQ_VER_MAJOR, SEV_FW_REQ_VER_MINOR);
  

nit: If we fail here, we leak `sev_fd`. Should we call `close(sev_fd)` here?

•           return NULL;
  

•   }
  

•   sev = calloc(1, sizeof(*sev));
  

•   sev->fd = sev_fd;
  

•   sev->vm = vm;
  

•   /* Get encryption bit via CPUID. */
  

•   eax = 0x8000001f;
  

•   ecx = 0;
  

•   cpuid(&eax, &ebx, &ecx, &edx);
  

•   sev->enc_bit = ebx & 0x3F;
  

•   return sev;
  

+}

+static void sev_common_free(struct sev_vm *sev) +{

•   close(sev->fd);
  

•   free(sev);
  

+}

+struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages) +{

•   struct sev_vm *sev;
  

•   struct kvm_vm *vm;
  

•   /* Need to handle memslots after init, and after setting memcrypt. */
  

•   vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
  

•   sev = sev_common_create(vm);
  

•   if (!sev)
  

•           return NULL;
  

•   sev->sev_policy = policy;
  

•   kvm_sev_ioctl(sev, KVM_SEV_INIT, NULL);
  

•   vm_set_memory_encryption(vm, true, true, sev->enc_bit);
  

•   vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, npages, 0);
  

•   sev_register_user_range(sev, addr_gpa2hva(vm, 0), npages * vm_get_page_size(vm));
  

•   pr_info("SEV guest created, policy: 0x%x, size: %lu KB\n",
  

•           sev->sev_policy, npages * vm_get_page_size(vm) / 1024);
  

•   return sev;
  

+}

+void sev_vm_free(struct sev_vm *sev) +{

•   kvm_vm_free(sev->vm);
  

•   sev_common_free(sev);
  

+}

+void sev_vm_launch(struct sev_vm *sev) +{

•   struct kvm_sev_launch_start ksev_launch_start = {0};
  

•   struct kvm_sev_guest_status ksev_status = {0};
  

•   ksev_launch_start.policy = sev->sev_policy;
  

•   kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_START, &ksev_launch_start);
  

•   kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status);
  

•   TEST_ASSERT(ksev_status.policy == sev->sev_policy, "Incorrect guest policy.");
  

•   TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE,
  

•               "Unexpected guest state: %d", ksev_status.state);
  

•   sev_encrypt(sev);
  

+}

+void sev_vm_measure(struct sev_vm *sev, uint8_t *measurement) +{

•   struct kvm_sev_launch_measure ksev_launch_measure = {0};
  

•   struct kvm_sev_guest_status ksev_guest_status = {0};
  

•   ksev_launch_measure.len = 256;
  

•   ksev_launch_measure.uaddr = (__u64)measurement;
  

•   kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_MEASURE, &ksev_launch_measure);
  

•   /* Measurement causes a state transition, check that. */
  

•   kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_guest_status);
  

•   TEST_ASSERT(ksev_guest_status.state == SEV_GSTATE_LSECRET,
  

•               "Unexpected guest state: %d", ksev_guest_status.state);
  

+}

+void sev_vm_launch_finish(struct sev_vm *sev) +{

•   struct kvm_sev_guest_status ksev_status = {0};
  

•   kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status);
  

•   TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE ||
  

•               ksev_status.state == SEV_GSTATE_LSECRET,
  

•               "Unexpected guest state: %d", ksev_status.state);
  

•   kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_FINISH, NULL);
  

•   kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status);
  

•   TEST_ASSERT(ksev_status.state == SEV_GSTATE_RUNNING,
  

•               "Unexpected guest state: %d", ksev_status.state);
  

+}

2.25.1

This is phenomenal. I'll try to send feedback on the other patches in the first batch of 7 sooner rather than later. I haven't looked past patch #7 yet. Let me know what you think about my first comment, on whether we can get all pre-existing tests to run under SEV, or that's a bad idea. I probably haven't given it as much thought as you have.

On Mon, Oct 11, 2021 at 08:15:37PM -0500, Michael Roth wrote:

On Sun, Oct 10, 2021 at 08:17:00PM -0700, Marc Orr wrote:

...

On Wed, Oct 6, 2021 at 1:40 PM Michael Roth michael.roth@amd.com wrote:

...

Add interfaces to allow tests to create/manage SEV guests. The additional state associated with these guests is encapsulated in a new struct sev_vm, which is a light wrapper around struct kvm_vm. These VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() under the covers to configure and sync up with the core kvm_util library on what should/shouldn't be treated as encrypted memory.

Signed-off-by: Michael Roth michael.roth@amd.com

tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/include/x86_64/sev.h | 62 ++++ tools/testing/selftests/kvm/lib/x86_64/sev.c | 303 ++++++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 5832f510a16c..c7a5e1c69e0c 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -35,6 +35,7 @@ endif

LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S +LIBKVM_x86_64 += lib/x86_64/sev.c

Regarding RFC-level feedback: First off, I'm super jazzed with what I'm seeing so far! (While this is my first review, I've been studying the patches up through the SEV boot test, i.e., patch #7). One thing I'm wondering is: the way this is structured is to essentially split the test cases into non-SEV and SEV. I'm wondering how hard it would be to add some flag or environment variable to set up pre-existing tests to run under SEV. Or is this something you all thought about, and decided that it does not make sense?

Looking at how the guest memory is handled, it seems like it's not far off from handling SEV transparently across all test cases. I'd think that we could just default all memory to use the encryption bit, and then have test cases, such as the test case in patch #7, clear the encryption bit for shared pages. However, I think the VM creation would need a bit more refactoring to work with other test cases.

I think it's possible, but there's a few missing pieces:

1. As you indicated, existing tests which rely on vm_create(), vm_create_default(), vm_create_default_with_vcpus(), etc. would either need to be updated with whatever new interface provides this 'use-sev' flag, or it would need to happen underneath the covers based on said environment variable/global/etc. There's also the question of where to hook in the sev_vm_launch_start() hooks. Maybe the first time a vcpu_run() is issued? Or maybe some explict call each test will need to be updated to call just prior to initial execution.

2. Many of the existing tests use the GUESY_SYNC/ucall stuff to handle synchronization between host userspace and guest kernel, which relies on guests issuing PIO instructions to particular port addresses to cause an exit back to host userspace, with various parameters passed via register arguments.

   • For SEV this would almost work as-is, but some tests might rely on things like memory addresses being passed in this way so would need to audit the code and mark that memory as shared where needed.

   • For SEV-ES/SEV-SNP, there's a bit more work since:

     • The registers will not be accessible through the existing KVM_GET_REGS mechanism. It may be possible to set some flag/hook to set/access arguments through some other mechanism like a shared buffer for certain VM types though.

     • Additionally, the #VC handler only supports CPUID currently, and leverages that fact to avoid doing any significant instruction decoding. Instead the SEV tests use HLT instructions to handle exits to host userspace, which may not work for some tests. So unless there's some other mechanism that SEV/non-SEV tests could utilize rather that PIO, the #VC handler would need to support PIO, which would be nice to have either way, but would likely involve pulling in the intruction decoder library used in the kernel, or some subset/re-implementation of it at least.

3. Similar to SEV-ES/SEV-SNP requirements for 1), tests which generate PIO/MMIO and other NAE events would need appropriate support for those events in the #VC handler. Nice-to-have either way, but not sure atm how much it would be to implement all of that. Also any tests relying on things like KVM_GET_REGS/KVM_GET_SREGS are non-starters.

One more I should mention:

4) After encryption, the page table is no longer usable for translations by stuff like addr_gva2gpa(), so tests would either need to be audited/updated to do these translations upfront and only rely on cached/stored values thereafter, or perhaps a "shadow" copy could be maintained by kvm_util so the translations will continue to work after encryption.

On 10/6/21 1:37 PM, Michael Roth wrote:

Add interfaces to allow tests to create/manage SEV guests. The additional state associated with these guests is encapsulated in a new struct sev_vm, which is a light wrapper around struct kvm_vm. These VMs will use vm_set_memory_encryption() and vm_get_encrypted_phy_pages() under the covers to configure and sync up with the core kvm_util library on what should/shouldn't be treated as encrypted memory.

Signed-off-by: Michael Roth michael.roth@amd.com

tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/include/x86_64/sev.h | 62 ++++ tools/testing/selftests/kvm/lib/x86_64/sev.c | 303 ++++++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 tools/testing/selftests/kvm/include/x86_64/sev.h create mode 100644 tools/testing/selftests/kvm/lib/x86_64/sev.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 5832f510a16c..c7a5e1c69e0c 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -35,6 +35,7 @@ endif LIBKVM = lib/assert.c lib/elf.c lib/io.c lib/kvm_util.c lib/rbtree.c lib/sparsebit.c lib/test_util.c lib/guest_modes.c lib/perf_test_util.c LIBKVM_x86_64 = lib/x86_64/apic.c lib/x86_64/processor.c lib/x86_64/vmx.c lib/x86_64/svm.c lib/x86_64/ucall.c lib/x86_64/handlers.S +LIBKVM_x86_64 += lib/x86_64/sev.c LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c lib/aarch64/handlers.S LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c diff --git a/tools/testing/selftests/kvm/include/x86_64/sev.h b/tools/testing/selftests/kvm/include/x86_64/sev.h new file mode 100644 index 000000000000..d2f41b131ecc --- /dev/null +++ b/tools/testing/selftests/kvm/include/x86_64/sev.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/*

• • Helpers used for SEV guests
• • Copyright (C) 2021 Advanced Micro Devices
• */

+#ifndef SELFTEST_KVM_SEV_H +#define SELFTEST_KVM_SEV_H

+#include <stdint.h> +#include <stdbool.h> +#include "kvm_util.h"

+#define SEV_DEV_PATH "/dev/sev" +#define SEV_FW_REQ_VER_MAJOR 1 +#define SEV_FW_REQ_VER_MINOR 30

+#define SEV_POLICY_NO_DBG (1UL << 0) +#define SEV_POLICY_ES (1UL << 2)

+#define SEV_GUEST_ASSERT(sync, token, _cond) do { \

• if (!(_cond)) \

• sev_guest_abort(sync, token, 0);	\
  

+} while (0)

+enum {

• SEV_GSTATE_UNINIT = 0,
• SEV_GSTATE_LUPDATE,
• SEV_GSTATE_LSECRET,
• SEV_GSTATE_RUNNING,

+};

+struct sev_sync_data {

• uint32_t token;
• bool pending;
• bool done;
• bool aborted;
• uint64_t info;

+};

+struct sev_vm;

+void sev_guest_sync(struct sev_sync_data *sync, uint32_t token, uint64_t info); +void sev_guest_done(struct sev_sync_data *sync, uint32_t token, uint64_t info); +void sev_guest_abort(struct sev_sync_data *sync, uint32_t token, uint64_t info);

+void sev_check_guest_sync(struct kvm_run *run, struct sev_sync_data *sync,

• 	  uint32_t token);
  

+void sev_check_guest_done(struct kvm_run *run, struct sev_sync_data *sync,

• 	  uint32_t token);
  

+void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data); +struct kvm_vm *sev_get_vm(struct sev_vm *sev); +uint8_t sev_get_enc_bit(struct sev_vm *sev);

+struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages); +void sev_vm_free(struct sev_vm *sev); +void sev_vm_launch(struct sev_vm *sev); +void sev_vm_measure(struct sev_vm *sev, uint8_t *measurement); +void sev_vm_launch_finish(struct sev_vm *sev);

+#endif /* SELFTEST_KVM_SEV_H */ diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c b/tools/testing/selftests/kvm/lib/x86_64/sev.c new file mode 100644 index 000000000000..adda3b396566 --- /dev/null +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c @@ -0,0 +1,303 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Helpers used for SEV guests
• • Copyright (C) 2021 Advanced Micro Devices
• */

+#include <stdint.h> +#include <stdbool.h> +#include "kvm_util.h" +#include "linux/psp-sev.h" +#include "processor.h" +#include "sev.h"

+#define PAGE_SHIFT 12 +#define PAGE_SIZE (1UL << PAGE_SHIFT)

+struct sev_vm {

• struct kvm_vm *vm;
• int fd;
• int enc_bit;
• uint32_t sev_policy;

+};

+/* Helpers for coordinating between guests and test harness. */

+void sev_guest_sync(struct sev_sync_data *sync, uint32_t token, uint64_t info) +{

• sync->token = token;
• sync->info = info;
• sync->pending = true;
• asm volatile("hlt" : : : "memory");

+}

+void sev_guest_done(struct sev_sync_data *sync, uint32_t token, uint64_t info) +{

• while (true) {

• sync->done = true;
  

• sev_guest_sync(sync, token, info);
  

• }

+}

+void sev_guest_abort(struct sev_sync_data *sync, uint32_t token, uint64_t info) +{

• while (true) {

• sync->aborted = true;
  

• sev_guest_sync(sync, token, info);
  

• }

+}

+void sev_check_guest_sync(struct kvm_run *run, struct sev_sync_data *sync,

• 	  uint32_t token)
  

+{

• TEST_ASSERT(run->exit_reason == KVM_EXIT_HLT,

•     "unexpected exit reason: %u (%s)",
  

•     run->exit_reason, exit_reason_str(run->exit_reason));
  

• TEST_ASSERT(sync->token == token,

•     "unexpected guest token, expected %d, got: %d", token,
  

•     sync->token);
  

• TEST_ASSERT(!sync->done, "unexpected guest state");
• TEST_ASSERT(!sync->aborted, "unexpected guest state");
• sync->pending = false;

+}

+void sev_check_guest_done(struct kvm_run *run, struct sev_sync_data *sync,

• 	  uint32_t token)
  

+{

• TEST_ASSERT(run->exit_reason == KVM_EXIT_HLT,

•     "unexpected exit reason: %u (%s)",
  

•     run->exit_reason, exit_reason_str(run->exit_reason));
  

• TEST_ASSERT(sync->token == token,

•     "unexpected guest token, expected %d, got: %d", token,
  

•     sync->token);
  

• TEST_ASSERT(sync->done, "unexpected guest state");
• TEST_ASSERT(!sync->aborted, "unexpected guest state");
• sync->pending = false;

+}

+/* Common SEV helpers/accessors. */

+struct kvm_vm *sev_get_vm(struct sev_vm *sev) +{

• return sev->vm;

+}

+uint8_t sev_get_enc_bit(struct sev_vm *sev) +{

• return sev->enc_bit;

+}

+void sev_ioctl(int sev_fd, int cmd, void *data) +{

• int ret;
• struct sev_issue_cmd arg;
• arg.cmd = cmd;
• arg.data = (unsigned long)data;
• ret = ioctl(sev_fd, SEV_ISSUE_CMD, &arg);
• TEST_ASSERT(ret == 0,

•     "SEV ioctl %d failed, error: %d, fw_error: %d",
  

•     cmd, ret, arg.error);
  

+}

+void kvm_sev_ioctl(struct sev_vm *sev, int cmd, void *data) +{

• struct kvm_sev_cmd arg = {0};
• int ret;
• arg.id = cmd;
• arg.sev_fd = sev->fd;
• arg.data = (__u64)data;
• ret = ioctl(vm_get_fd(sev->vm), KVM_MEMORY_ENCRYPT_OP, &arg);
• TEST_ASSERT(ret == 0,

•     "SEV KVM ioctl %d failed, rc: %i errno: %i (%s), fw_error: %d",
  

•     cmd, ret, errno, strerror(errno), arg.error);
  

+}

+/* Local helpers. */

+static void +sev_register_user_range(struct sev_vm *sev, void *hva, uint64_t size)

Since 'region' is used in every naming, it's better to call it sev_register_user_region or sev_register_userspace_region for the sake of consistency.

+{

• struct kvm_enc_region range = {0};
• int ret;
• pr_debug("register_user_range: hva: %p, size: %lu\n", hva, size);
• range.addr = (__u64)hva;
• range.size = size;
• ret = ioctl(vm_get_fd(sev->vm), KVM_MEMORY_ENCRYPT_REG_REGION, &range);
• TEST_ASSERT(ret == 0, "failed to register user range, errno: %i\n", errno);

+}

+static void +sev_encrypt_phy_range(struct sev_vm *sev, vm_paddr_t gpa, uint64_t size) +{

• struct kvm_sev_launch_update_data ksev_update_data = {0};
• pr_debug("encrypt_phy_range: addr: 0x%lx, size: %lu\n", gpa, size);
• ksev_update_data.uaddr = (__u64)addr_gpa2hva(sev->vm, gpa);
• ksev_update_data.len = size;
• kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_UPDATE_DATA, &ksev_update_data);

+}

+static void sev_encrypt(struct sev_vm *sev) +{

• struct sparsebit *enc_phy_pages;
• struct kvm_vm *vm = sev->vm;
• sparsebit_idx_t pg = 0;
• vm_paddr_t gpa_start;
• uint64_t memory_size;
• /* Only memslot 0 supported for now. */
• enc_phy_pages = vm_get_encrypted_phy_pages(sev->vm, 0, &gpa_start, &memory_size);

vm_get_encrypted_phy_pages() allocates a duplicate sparsebit. I am wondering if it is possible for the function to just give a pointer to 'region->encrypted_phy_pages' so the allocation and freeing of memory can be avoided.

• TEST_ASSERT(enc_phy_pages, "Unable to retrieve encrypted pages bitmap");
• while (pg < (memory_size / vm_get_page_size(vm))) {

• sparsebit_idx_t pg_cnt;
  

• if (sparsebit_is_clear(enc_phy_pages, pg)) {
  

• 	pg = sparsebit_next_set(enc_phy_pages, pg);
  

• 	if (!pg)
  

• 		break;
  

• }
  

• pg_cnt = sparsebit_next_clear(enc_phy_pages, pg) - pg;
  

• if (pg_cnt <= 0)
  

• 	pg_cnt = 1;
  

• sev_encrypt_phy_range(sev,
  

• 		      gpa_start + pg * vm_get_page_size(vm),
  

• 		      pg_cnt * vm_get_page_size(vm));
  

• pg += pg_cnt;
  

• }
• sparsebit_free(&enc_phy_pages);

+}

+/* SEV VM implementation. */

+static struct sev_vm *sev_common_create(struct kvm_vm *vm)

Can there be a better name like, sev_vm_alloc because it's essentially allocating and initializing a sev_vm data structure.

+{

• struct sev_user_data_status sev_status = {0};
• uint32_t eax, ebx, ecx, edx;
• struct sev_vm *sev;
• int sev_fd;
• sev_fd = open(SEV_DEV_PATH, O_RDWR);
• if (sev_fd < 0) {

• pr_info("Failed to open SEV device, path: %s, error: %d, skipping test.\n",
  

• 	SEV_DEV_PATH, sev_fd);
  

• return NULL;
  

• }
• sev_ioctl(sev_fd, SEV_PLATFORM_STATUS, &sev_status);
• if (!(sev_status.api_major > SEV_FW_REQ_VER_MAJOR ||

•      (sev_status.api_major == SEV_FW_REQ_VER_MAJOR &&
  

•       sev_status.api_minor >= SEV_FW_REQ_VER_MINOR))) {
  

• pr_info("SEV FW version too old. Have API %d.%d (build: %d), need %d.%d, skipping test.\n",
  

• 	sev_status.api_major, sev_status.api_minor, sev_status.build,
  

• 	SEV_FW_REQ_VER_MAJOR, SEV_FW_REQ_VER_MINOR);
  

• return NULL;
  

• }
• sev = calloc(1, sizeof(*sev));
• sev->fd = sev_fd;
• sev->vm = vm;
• /* Get encryption bit via CPUID. */
• eax = 0x8000001f;
• ecx = 0;
• cpuid(&eax, &ebx, &ecx, &edx);
• sev->enc_bit = ebx & 0x3F;
• return sev;

+}

+static void sev_common_free(struct sev_vm *sev) +{

• close(sev->fd);
• free(sev);

+}

+struct sev_vm *sev_vm_create(uint32_t policy, uint64_t npages) +{

• struct sev_vm *sev;
• struct kvm_vm *vm;
• /* Need to handle memslots after init, and after setting memcrypt. */
• vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
• sev = sev_common_create(vm);
• if (!sev)

• return NULL;
  

• sev->sev_policy = policy;
• kvm_sev_ioctl(sev, KVM_SEV_INIT, NULL);
• vm_set_memory_encryption(vm, true, true, sev->enc_bit);
• vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, npages, 0);
• sev_register_user_range(sev, addr_gpa2hva(vm, 0), npages * vm_get_page_size(vm));
• pr_info("SEV guest created, policy: 0x%x, size: %lu KB\n",

• sev->sev_policy, npages * vm_get_page_size(vm) / 1024);
  

• return sev;

+}

+void sev_vm_free(struct sev_vm *sev) +{

• kvm_vm_free(sev->vm);
• sev_common_free(sev);

+}

+void sev_vm_launch(struct sev_vm *sev) +{

• struct kvm_sev_launch_start ksev_launch_start = {0};
• struct kvm_sev_guest_status ksev_status = {0};
• ksev_launch_start.policy = sev->sev_policy;
• kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_START, &ksev_launch_start);
• kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status);
• TEST_ASSERT(ksev_status.policy == sev->sev_policy, "Incorrect guest policy.");
• TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE,

•     "Unexpected guest state: %d", ksev_status.state);
  

• sev_encrypt(sev);

+}

+void sev_vm_measure(struct sev_vm *sev, uint8_t *measurement) +{

• struct kvm_sev_launch_measure ksev_launch_measure = {0};
• struct kvm_sev_guest_status ksev_guest_status = {0};
• ksev_launch_measure.len = 256;
• ksev_launch_measure.uaddr = (__u64)measurement;
• kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_MEASURE, &ksev_launch_measure);
• /* Measurement causes a state transition, check that. */
• kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_guest_status);
• TEST_ASSERT(ksev_guest_status.state == SEV_GSTATE_LSECRET,

•     "Unexpected guest state: %d", ksev_guest_status.state);
  

+}

+void sev_vm_launch_finish(struct sev_vm *sev) +{

• struct kvm_sev_guest_status ksev_status = {0};
• kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status);
• TEST_ASSERT(ksev_status.state == SEV_GSTATE_LUPDATE ||

•     ksev_status.state == SEV_GSTATE_LSECRET,
  

•     "Unexpected guest state: %d", ksev_status.state);
  

• kvm_sev_ioctl(sev, KVM_SEV_LAUNCH_FINISH, NULL);
• kvm_sev_ioctl(sev, KVM_SEV_GUEST_STATUS, &ksev_status);
• TEST_ASSERT(ksev_status.state == SEV_GSTATE_RUNNING,

•     "Unexpected guest state: %d", ksev_status.state);
  

+}

On Fri, Oct 15, 2021 at 07:55:53PM -0700, Krish Sadhukhan wrote:

On 10/6/21 1:37 PM, Michael Roth wrote:

...

A common aspect of booting SEV guests is checking related CPUID/MSR bits and accessing shared/private memory. Add a basic test to cover this.

This test will be expanded to cover basic boot of SEV-ES and SEV-SNP in subsequent patches.

Signed-off-by: Michael Roth michael.roth@amd.com

tools/testing/selftests/kvm/.gitignore | 1 + tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/x86_64/sev_all_boot_test.c | 252 ++++++++++++++++++ 3 files changed, 254 insertions(+) create mode 100644 tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore index 0709af0144c8..824f100bec2a 100644 --- a/tools/testing/selftests/kvm/.gitignore +++ b/tools/testing/selftests/kvm/.gitignore @@ -38,6 +38,7 @@ /x86_64/xen_vmcall_test /x86_64/xss_msr_test /x86_64/vmx_pmu_msrs_test +/x86_64/sev_all_boot_test /access_tracking_perf_test /demand_paging_test /dirty_log_test diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index c7a5e1c69e0c..aa8901bdbd22 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -72,6 +72,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test TEST_GEN_PROGS_x86_64 += x86_64/vmx_pmu_msrs_test TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test +TEST_GEN_PROGS_x86_64 += x86_64/sev_all_boot_test TEST_GEN_PROGS_x86_64 += access_tracking_perf_test TEST_GEN_PROGS_x86_64 += demand_paging_test TEST_GEN_PROGS_x86_64 += dirty_log_test diff --git a/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c new file mode 100644 index 000000000000..8df7143ac17d --- /dev/null +++ b/tools/testing/selftests/kvm/x86_64/sev_all_boot_test.c @@ -0,0 +1,252 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Basic SEV boot tests.
• • Copyright (C) 2021 Advanced Micro Devices
• */

+#define _GNU_SOURCE /* for program_invocation_short_name */ +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <sys/ioctl.h>

+#include "test_util.h"

+#include "kvm_util.h" +#include "processor.h" +#include "svm_util.h" +#include "linux/psp-sev.h" +#include "sev.h"

+#define VCPU_ID 2 +#define PAGE_SIZE 4096 +#define PAGE_STRIDE 32

+#define SHARED_PAGES 8192 +#define SHARED_VADDR_MIN 0x1000000

+#define PRIVATE_PAGES 2048 +#define PRIVATE_VADDR_MIN (SHARED_VADDR_MIN + SHARED_PAGES * PAGE_SIZE)

+#define TOTAL_PAGES (512 + SHARED_PAGES + PRIVATE_PAGES)

+static void fill_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val) +{

• int i, j;
• for (i = 0; i < pages; i++)

• for (j = 0; j < PAGE_SIZE; j += stride)
  

• 	buf[i * PAGE_SIZE + j] = val;
  

+}

+static bool check_buf(uint8_t *buf, size_t pages, size_t stride, uint8_t val) +{

• int i, j;
• for (i = 0; i < pages; i++)

• for (j = 0; j < PAGE_SIZE; j += stride)
  

• 	if (buf[i * PAGE_SIZE + j] != val)
  

• 		return false;
  

• return true;

+}

+static void guest_test_start(struct sev_sync_data *sync) +{

• /* Initial guest check-in. */
• sev_guest_sync(sync, 1, 0);

+}

+static void check_test_start(struct kvm_vm *vm, struct sev_sync_data *sync) +{

• struct kvm_run *run;
• run = vcpu_state(vm, VCPU_ID);
• vcpu_run(vm, VCPU_ID);
• /* Initial guest check-in. */
• sev_check_guest_sync(run, sync, 1);

+}

+static void +guest_test_common(struct sev_sync_data *sync, uint8_t *shared_buf, uint8_t *private_buf) +{

• bool success;
• /* Initial check-in for common. */
• sev_guest_sync(sync, 100, 0);
• /* Ensure initial shared pages are intact. */
• success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
• SEV_GUEST_ASSERT(sync, 103, success);
• /* Ensure initial private pages are intact/encrypted. */
• success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
• SEV_GUEST_ASSERT(sync, 104, success);
• /* Ensure host userspace can't read newly-written encrypted data. */
• fill_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
• sev_guest_sync(sync, 200, 0);
• /* Ensure guest can read newly-written shared data from host. */
• success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
• SEV_GUEST_ASSERT(sync, 201, success);
• /* Ensure host can read newly-written shared data from guest. */
• fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
• sev_guest_sync(sync, 300, 0);

+}

+static void +check_test_common(struct kvm_vm *vm, struct sev_sync_data *sync,

•   uint8_t *shared_buf, uint8_t *private_buf)
  

+{

• struct kvm_run *run = vcpu_state(vm, VCPU_ID);
• bool success;
• /* Initial guest check-in. */
• vcpu_run(vm, VCPU_ID);
• sev_check_guest_sync(run, sync, 100);
• /* Ensure initial private pages are intact/encrypted. */
• success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);
• TEST_ASSERT(!success, "Initial guest memory not encrypted!");
• vcpu_run(vm, VCPU_ID);
• sev_check_guest_sync(run, sync, 200);
• /* Ensure host userspace can't read newly-written encrypted data. */
• success = check_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x43);
• TEST_ASSERT(!success, "Modified guest memory not encrypted!");
• /* Ensure guest can read newly-written shared data from host. */
• fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x44);
• vcpu_run(vm, VCPU_ID);
• sev_check_guest_sync(run, sync, 300);
• /* Ensure host can read newly-written shared data from guest. */
• success = check_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x45);
• TEST_ASSERT(success, "Host can't read shared guest memory!");

+}

+static void +guest_test_done(struct sev_sync_data *sync) +{

• sev_guest_done(sync, 10000, 0);

+}

+static void +check_test_done(struct kvm_vm *vm, struct sev_sync_data *sync) +{

• struct kvm_run *run = vcpu_state(vm, VCPU_ID);
• vcpu_run(vm, VCPU_ID);
• sev_check_guest_done(run, sync, 10000);

+}

+static void __attribute__((__flatten__)) +guest_sev_code(struct sev_sync_data *sync, uint8_t *shared_buf, uint8_t *private_buf) +{

• uint32_t eax, ebx, ecx, edx;
• uint64_t sev_status;
• guest_test_start(sync);
• /* Check SEV CPUID bit. */
• eax = 0x8000001f;
• ecx = 0;
• cpuid(&eax, &ebx, &ecx, &edx);
• SEV_GUEST_ASSERT(sync, 2, eax & (1 << 1));
• /* Check SEV MSR bit. */
• sev_status = rdmsr(MSR_AMD64_SEV);
• SEV_GUEST_ASSERT(sync, 3, (sev_status & 0x1) == 1);

Is there any need to do the cpuid and MSR tests every time the guest code is executed ?

It seems like a good sanity check that KVM is setting the expected bits (via KVM_GET_SUPPORTED_CPUID) for SEV guests. This is also a fairly common example of the sort of things a guest would do during boot to detect/initialize SEV-related functionality.

It becomes a little more useful for the SEV-ES/SEV-SNP tests, where cpuid instructions cause a #VC exception, which in turn leads to a VMGExit and exercises the host KVM's handling of host-guest communiction via GHCB MSR/GHCB page.

...

• guest_test_common(sync, shared_buf, private_buf);
• guest_test_done(sync);

+}

+static void +setup_test_common(struct sev_vm *sev, void *guest_code, vm_vaddr_t *sync_vaddr,

•   vm_vaddr_t *shared_vaddr, vm_vaddr_t *private_vaddr)
  

+{

• struct kvm_vm *vm = sev_get_vm(sev);
• uint8_t *shared_buf, *private_buf;
• /* Set up VCPU and initial guest kernel. */
• vm_vcpu_add_default(vm, VCPU_ID, guest_code);
• kvm_vm_elf_load(vm, program_invocation_name);
• /* Set up shared sync buffer. */
• *sync_vaddr = vm_vaddr_alloc_shared(vm, PAGE_SIZE, 0);
• /* Set up buffer for reserved shared memory. */
• *shared_vaddr = vm_vaddr_alloc_shared(vm, SHARED_PAGES * PAGE_SIZE,

• 			      SHARED_VADDR_MIN);
  

• shared_buf = addr_gva2hva(vm, *shared_vaddr);
• fill_buf(shared_buf, SHARED_PAGES, PAGE_STRIDE, 0x41);
• /* Set up buffer for reserved private memory. */
• *private_vaddr = vm_vaddr_alloc(vm, PRIVATE_PAGES * PAGE_SIZE,

• 			PRIVATE_VADDR_MIN);
  

• private_buf = addr_gva2hva(vm, *private_vaddr);
• fill_buf(private_buf, PRIVATE_PAGES, PAGE_STRIDE, 0x42);

+}

+static void test_sev(void *guest_code, uint64_t policy) +{

• vm_vaddr_t sync_vaddr, shared_vaddr, private_vaddr;
• uint8_t *shared_buf, *private_buf;
• struct sev_sync_data *sync;
• uint8_t measurement[512];
• struct sev_vm *sev;
• struct kvm_vm *vm;
• int i;
• sev = sev_vm_create(policy, TOTAL_PAGES);
• if (!sev)

• return;
  

• vm = sev_get_vm(sev);
• setup_test_common(sev, guest_code, &sync_vaddr, &shared_vaddr, &private_vaddr);
• /* Set up guest params. */
• vcpu_args_set(vm, VCPU_ID, 4, sync_vaddr, shared_vaddr, private_vaddr);
• sync = addr_gva2hva(vm, sync_vaddr);
• shared_buf = addr_gva2hva(vm, shared_vaddr);
• private_buf = addr_gva2hva(vm, private_vaddr);
• /* Allocations/setup done. Encrypt initial guest payload. */
• sev_vm_launch(sev);
• /* Dump the initial measurement. A test to actually verify it would be nice. */
• sev_vm_measure(sev, measurement);
• pr_info("guest measurement: ");
• for (i = 0; i < 32; ++i)

• pr_info("%02x", measurement[i]);
  

• pr_info("\n");
• sev_vm_launch_finish(sev);

Since the above section of this function is actually setup code and is not running the test yet, it is best placed in a setup function. My suggestion is that you place the above section into a function called setup_test_common() and within that function you further separate out the SEV-related setup into a function called setup_test_sev() or something similar. Then call the top-level setup function from within main().

That makes sense. I'll try to rework this according to your suggestions.

...

• /* Guest is ready to run. Do the tests. */
• check_test_start(vm, sync);
• check_test_common(vm, sync, shared_buf, private_buf);
• check_test_done(vm, sync);

These function names can be better. These functions are not just checking the result, they are actually running the guest code. So, may be, calling them 'test_start, test_common and test_done are better. I would just

Will do.

collapse them and place their code in test_sev() only if you separate the out the setup code as I suggested above.

Hmm, I did it that way because in the guest_{sev,sev_es,sev_snp}_code routines there are some type-specific checks that happen before guest_test_done(), so it made sense to have that in a separate routine, and seemed more readable to then also have check_test_done() separate to pair with it. But I'll see if I can rework that a bit.

...

• sev_vm_free(sev);

+}

+int main(int argc, char *argv[]) +{

• /* SEV tests */
• test_sev(guest_sev_code, SEV_POLICY_NO_DBG);
• test_sev(guest_sev_code, 0);
• return 0;

+}

On 06/10/21 22:37, Michael Roth wrote:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c1a2dd0024b2..d724fa185bef 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10400,8 +10400,7 @@ static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs, vcpu->arch.cr2 = sregs->cr2; *mmu_reset_needed |= kvm_read_cr3(vcpu) != sregs->cr3;

• vcpu->arch.cr3 = sregs->cr3;
• kvm_register_mark_available(vcpu, VCPU_EXREG_CR3);

• static_call(kvm_x86_set_cr3)(vcpu, sregs->cr3);

kvm_set_cr8(vcpu, sregs->cr8);

Can you instead make the hook "post_set_sregs", and keep these two lines in __set_sregs_common?

Thanks,

Paolo