On Fri, May 10, 2024 at 03:50:26PM +0200, Paolo Bonzini wrote: > On Fri, May 10, 2024 at 3:47 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > + * Since software-protected VMs don't have a notion of a shared vs. > > > + * private that's separate from what KVM is tracking, the above > > > + * KVM_EXIT_MEMORY_FAULT condition wouldn't occur, so avoid the > > > + * special handling for that case for now. > > > > Very technically, it can occur if userspace _just_ modified the attributes. And > > as I've said multiple times, at least for now, I want to avoid special casing > > SW-protected VMs unless it is *absolutely* necessary, because their sole purpose > > is to allow testing flows that are impossible to excercise without SNP/TDX hardware. > > Yep, it is not like they have to be optimized. Ok, I thought there were maybe some future plans to use sw-protected VMs to get some added protections from userspace. But even then there'd probably still be extra considerations for how to handle access tracking so white-listing them probably isn't right anyway. I was also partly tempted to take this route because it would cover this TDX patch as well: https://lore.kernel.org/lkml/91c797997b57056224571e22362321a23947172f.1705965635.git.isaku.yamahata@xxxxxxxxx/ and avoid any weirdness about checking kvm_mem_is_private() without checking mmu_invalidate_seq, but I think those cases all end up resolving themselves eventually and added some comments around that. > > > > + */ > > > + if (kvm_slot_can_be_private(fault->slot) && > > > + !(IS_ENABLED(CONFIG_KVM_SW_PROTECTED_VM) && > > > + vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM)) > > > > Heh, !(x && y) kills me, I misread this like 4 times. > > > > Anyways, I don't like the heuristic. It doesn't tie the restriction back to the > > cause in any reasonable way. Can't this simply be? > > > > if (fault->is_private != kvm_mem_is_private(vcpu->kvm, fault->gfn) > > return false; > > You beat me to it by seconds. And it can also be guarded by a check on > kvm->arch.has_private_mem to avoid the attributes lookup. I re-tested with things implemented this way and everything seems to look good. It's not clear to me whether this would cover the cases the above-mentioned TDX patch handles, but no biggie if that's still needed. The new version of the patch is here: https://github.com/mdroth/linux/commit/39643f9f6da6265d39d633a703c53997985c1208 And I've updated my branches with to replace the old patch and also incorporate Sean's suggestions for patch 22: https://github.com/mdroth/linux/commits/snp-host-v15c3-unsquashed and have them here with things already squashed in/relocated: https://github.com/mdroth/linux/commits/snp-host-v15c3 Thanks for the feedback Sean, Paolo. -Mike > > > Which is much, much more self-explanatory. > > Both more self-explanatory and more correct. > > Paolo > >
