Re: [PATCH v4 03/16] KVM: Add KVM_CAP_MEMORY_FAULT_INFO

[Sean Christopherson <seanjc@google.com>]

On Tue, Aug 15, 2023, Anish Moorthy wrote:
> On Fri, Aug 11, 2023 at 3:12 PM Anish Moorthy <amoorthy@google.com> wrote:
> >
> > On Wed, Jun 14, 2023 at 10:35 AM Sean Christopherson <seanjc@google.com> 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@google.com> 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 */