On Thu, Oct 6, 2022 at 11:48 AM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxxxxxx > 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 > >
