On Tue, Aug 15, 2023, Anish Moorthy wrote: > On Fri, Aug 11, 2023 at 3:12 PM Anish Moorthy <amoorthy@xxxxxxxxxx> wrote: > > > > On Wed, Jun 14, 2023 at 10:35 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > > > +inline void kvm_populate_efault_info(struct kvm_vcpu *vcpu, > > > > > > Tagging a globally visible, non-static function as "inline" is odd, to say the > > > least. > > > > I think my eyes glaze over whenever I read the words "translation > > unit" (my brain certainly does) so I'll have to take your word for it. > > IIRC last time I tried to mark this function "static" the compiler > > yelled at me, so removing the "inline" it is. > > > >... > > > On Mon, Aug 14, 2023 at 5:43 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > Can you point me at your branch? That should be easy to resolve, but it's all > > but impossible to figure out what's going wrong without being able to see the > > full code. > > Sure: https://github.com/anlsh/linux/tree/suffd-kvm-staticinline. > Don't worry about this unless you're bored though: I only called out > my change because I wanted to make sure the final signature was fine. > If you say it should be static inline then I can take a more concerted > stab at learning/figuring out what's going on here. That branch builds (and looks) just fine on gcc-12 and clang-14. Maybe you have stale objects in your build directory? Or maybe PEBKAC? > > > Btw, do you actually know the size of the union in the run struct? I > > > started checking it but stopped when I realized that it includes > > > arch-dependent structs. > > > > 256 bytes, though how much of that is actually free for the "speculative" idea... > > > > /* Fix the size of the union. */ > > char padding[256]; > > > > Well fudge. PPC's KVM_EXIT_OSI actually uses all 256 bytes. And KVM_EXIT_SYSTEM_EVENT > > is closer to the limit than I'd like > > > > On the other hand, despite burning 2048 bytes for kvm_sync_regs, all of kvm_run > > is only 2352 bytes, i.e. we have plenty of room in the 4KiB page. So we could > > throw the "speculative" exits in a completely different union. But that would > > be cumbersome for userspace. > > Haha, well it's a good thing we checked. What about an extra union > would be cumbersome for userspace though? From an API perspective it > doesn't seem like splitting the current struct or adding an extra one > would be all too different- is it something about needing to recompile > things due to the struct size change? I was thinking that we couldn't have two anonymous unions, and so userspace (and KVM) would need to do something like run->exit2.memory_fault.gpa instead of run->memory_fault.gpa but the names just need to be unique, e.g. the below compiles just fine. So unless someone has a better idea, using a separate union for exits that might be clobbered seems like the way to go. diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 5bdda75bfd10..fc3701d835d6 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -3289,6 +3289,9 @@ static int kvm_handle_error_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fa return RET_PF_RETRY; } + vcpu->run->memory_fault.flags = 0; + vcpu->run->memory_fault.gpa = fault->gfn << PAGE_SHIFT; + vcpu->run->memory_fault.len = PAGE_SIZE; return -EFAULT; } diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index f089ab290978..1a8ccd5f949a 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -531,6 +531,18 @@ struct kvm_run { struct kvm_sync_regs regs; char padding[SYNC_REGS_SIZE_BYTES]; } s; + + /* Anonymous union for exits #2. */ + union { + /* KVM_EXIT_MEMORY_FAULT */ + struct { + __u64 flags; + __u64 gpa; + __u64 len; /* in bytes */ + } memory_fault; + + char padding2[256]; + }; }; /* for KVM_REGISTER_COALESCED_MMIO / KVM_UNREGISTER_COALESCED_MMIO */
