On Wed, Jan 18, 2023, Kirill A. Shutemov wrote: > On Wed, Jan 18, 2023 at 01:09:49AM +0000, Sean Christopherson wrote: > > On Mon, Dec 05, 2022, Vishal Annapurve wrote: > > > This series implements selftests targeting the feature floated by Chao via: > > > https://lore.kernel.org/lkml/20221202061347.1070246-10-chao.p.peng@xxxxxxxxxxxxxxx/T/ > > > > > > Below changes aim to test the fd based approach for guest private memory > > > in context of normal (non-confidential) VMs executing on non-confidential > > > platforms. > > > > > > private_mem_test.c file adds selftest to access private memory from the > > > guest via private/shared accesses and checking if the contents can be > > > leaked to/accessed by vmm via shared memory view before/after conversions. > > > > > > Updates in V2: > > > 1) Simplified vcpu run loop implementation API > > > 2) Removed VM creation logic from private mem library > > > > I pushed a rework version of this series to: > > > > git@xxxxxxxxxx:sean-jc/linux.git x86/upm_base_support > > It still has build issue with lockdep enabled that I mentioned before: Yeah, I haven't updated the branch since last Friday, i.e. I haven't fixed the known bugs yet. I pushed the selftests changes at the same as everything else, just didn't get to typing up the emails until yesterday. > https://lore.kernel.org/all/20230116134845.vboraky2nd56szos@xxxxxxxxxxxxxxxxx/ > > And when compiled with lockdep, it triggers a lot of warnings about missed > locks, like this: Ah crud, I knew I was forgetting something. The lockdep assertion can simply be removed, mmu_lock doesn't need to be held to read attributes. I knew the assertion was wrong when I added it, but I wanted to remind myself to take a closer look at the usage of kvm_mem_is_private() and forgot to go back and delete the assertion. The use of kvm_mem_is_private() in kvm_mmu_do_page_fault() is technically unsafe. Similar to getting the pfn, if mmu_lock isn't held, consuming the attributes (private vs. shared) needs MMU notifier protection, i.e. the attributes are safe to read only after mmu_invalidate_seq is snapshot. However, is_private gets rechecked by __kvm_faultin_pfn(), which is protected by the sequence counter, and so the technically unsafe read is verified in the end. The obvious alternative is to make is_private an output of kvm_faultin_pfn(), but that's incorrect for SNP and TDX guests, in which case "is_private" is a property of the fault itself. TL;DR: I'm going to delete the assertion and add a comment in kvm_mmu_do_page_fault() explaining why it's safe to consume kvm_mem_is_private() for "legacy" guests. diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 35a339891aed..da0afe81cf10 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -2310,8 +2310,6 @@ static inline void kvm_account_pgtable_pages(void *virt, int nr) #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES static inline unsigned long kvm_get_memory_attributes(struct kvm *kvm, gfn_t gfn) { - lockdep_assert_held(kvm->mmu_lock); - return xa_to_value(xa_load(&kvm->mem_attr_array, gfn)); }
