Hi Sean,
On 10/14/2024 5:58 PM, Sean Christopherson wrote:
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
From: Michael Roth michael.roth@amd.com
For SEV and SNP, currently __vm_phy_pages_alloc() handles setting the region->protected_phy_pages bitmap to mark that the region needs to be encrypted/measured into the initial guest state prior to
Nothing needs to be measured, no? (because there's no attestation)
Right.
finalizing/starting the guest. It also marks what GPAs need to be mapped as encrypted in the initial guest page table.
...
static inline void vm_mem_set_private(struct kvm_vm *vm, uint64_t gpa, uint64_t size) diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index bbf90ad224da..d44a37aebcec 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -1991,6 +1991,43 @@ const char *exit_reason_str(unsigned int exit_reason) return "Unknown"; } +/*
- Set what guest GFNs need to be encrypted prior to finalizing a CoCo VM.
- Input Args:
- vm - Virtual Machine
- memslot - Memory region to allocate page from
- paddr - Start of physical address to mark as encrypted
- num - number of pages
- Output Args: None
- Return: None
- Generally __vm_phy_pages_alloc() will handle this automatically, but
- for cases where the test handles managing the physical allocation and
- mapping directly this interface should be used to mark physical pages
- that are intended to be encrypted as part of the initial guest state.
- This will also affect whether virt_map()/virt_pg_map() will map the
- page as encrypted or not in the initial guest page table.
- If the initial guest state has already been finalized, then setting
- it as encrypted will essentially be a noop since nothing more can be
- encrypted into the initial guest state at that point.
- */
+void vm_mem_set_protected(struct kvm_vm *vm, uint32_t memslot,
vm_paddr_t paddr, size_t num)+{
- struct userspace_mem_region *region;
- sparsebit_idx_t pg, base;
- base = paddr >> vm->page_shift;
- region = memslot2region(vm, memslot);
Please no, doing a memslot lookup in a helper like this is only going to encourage proliferation of bad code. vm_mem_add() really should be able to mark the region as protected.
E.g. practically speaking, the only code that will be able to use this helper is code that is marking the entire memslot as protection. And ability to _clear_ the protected_phy_pages bit is conspicuously missing.
From my understanding, vm_mem_add() only allocates the protected sparsebits but does not populate them. For that maybe a better path would be to go through, vm_phy_pages_alloc() - something similar to how the set_memory_region_test.c does? I think we avoided doing that because vm_phys_pages_alloc() takes a paddr_min rather than guaranteeing the specific paddr and even then would need the similar virt_map() logic to stay. If this is a cleaner approach though, I'm happy to redo this code that way.
Thanks! Pratik
- for (pg = base; pg < base + num; ++pg)
sparsebit_set(region->protected_phy_pages, pg);+}