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

[Sean Christopherson <seanjc@google.com>]

On Mon, Aug 14, 2023, Anish Moorthy wrote:
> On Mon, Aug 14, 2023 at 11:01 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > What is/was the error?  It's probably worth digging into; "static inline" should
> > work just fine, so there might be something funky elsewhere that you're papering
> > over.
> 
> What I get is
> 
> > ./include/linux/kvm_host.h:2298:20: error: function 'kvm_handle_guest_uaccess_fault' has internal linkage but is not defined [-Werror,-Wundefined-internal]
> > static inline void kvm_handle_guest_uaccess_fault(struct kvm_vcpu *vcpu,
> >                    ^
> > arch/x86/kvm/mmu/mmu.c:3323:2: note: used here
> >         kvm_handle_guest_uaccess_fault(vcpu, gfn_to_gpa(fault->gfn), PAGE_SIZE,
> >         ^
> > 1 error generated.
> 
> (mmu.c:3323 is in kvm_handle_error_pfn()). I tried shoving the
> definition of the function from kvm_main.c to kvm_host.h so that I
> could make it "static inline": but then the same "internal linkage"
> error pops up in the kvm_vcpu_read/write_guest_page() functions.

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.

> 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.

Hrm.  The best option would probably be to have a "nested" or "previous" exit union,
and copy the existing exit information to that field prior to filling a new exit
reason.  But that would require an absolute insane amount of refactoring because
everything just writes the fields directly. *sigh*

I suppose we could copy the information into two places for "speculative" exits,
the actual exit union and a separate "speculative" field.  I might be grasping at
straws though, not sure that ugliness would be worth making it slightly easier to
deal with the (A) scenario from earlier.

FWIW, my trick for quick finding the real size is to feed the size+1 into an alignment.
Unless you get really unlucky, that alignment will be illegal and the compiler
will tell you the size, e.g. 

arch/x86/kvm/x86.c:13405:9: error: requested alignment ‘2353’ is not a positive power of 2
13405 |         unsigned int len __aligned(sizeof(*run) + 1);
      |         ^~~~~~~~