Re: [PATCH 1/3] KVM: x86, vmx: Add function for event delivery error generation

[Ivan Orlov]

Hi Sean,

On Fri, Oct 11, 2024 at 04:20:46PM -0700, Sean Christopherson wrote:
> "KVM: VMX:" for the scope.  See "Shortlog" in Documentation/process/maintainer-kvm-x86.rst
>

Ah, will update in the next version, thanks!

> On Fri, Sep 27, 2024, Ivan Orlov wrote:
> > Extract KVM_INTERNAL_ERROR_DELIVERY_EV internal error generation into
> > the SVM/VMX-agnostic 'kvm_prepare_ev_delivery_failure_exit' function, as
> > it is done for KVM_INTERNAL_ERROR_EMULATION.
> 
> Use the changelog to provide a human readable summary of the change.  There are
> definitely situations where calling out functions, variables, defines, etc. by
> name is necessary, but this isn't one such situation.
>
> > The order of internal.data array entries is preserved as is, so it is going
> > to be the same on VMX platforms (vectoring info, full exit reason, exit
> > qualification, GPA if error happened due to MMIO and last_vmentry_cpu of the
> > vcpu).
> 
> Similar to the above, let the code speak.  The "No functional change intended"
> makes it clear that the intent is to preserve the order and behavior.
> 
> > Having it as a separate function will help us to avoid code duplication
> 
> Avoid pronouns as much as possible, and no "we" or "us" as a hard rule.  E.g. this
> can all be distilled down to:
>

Yeah, makes sense. Will reformulate the message in the next version to consider
all of the changes you suggested.

> --
> Extract VMX's code for reporting an unhandleable VM-Exit during event
> delivery to userspace, so that the boilerplate code can be shared by SVM.
> 
> No functional change intended.
> --
> 

Awesome, thanks for the example!

> 
> Please wrap at 80 columns.  While checkpatch doesn't complaing until 100, my
> preference is to default to wrapping at 80, and poking past 80 only when it yields
> more readable code (which is obviously subjective, but it shouldn't be too hard
> to figure out KVM x86's preferred style).
>

Alright, will do, thanks! These rules vary from one subsystem to another, and
I'll try to keep the style consistent in the future.

> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index c67e448c6ebd..afd785e7f3a3 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -6550,19 +6550,10 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> >  	     exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
> >  	     exit_reason.basic != EXIT_REASON_TASK_SWITCH &&
> >  	     exit_reason.basic != EXIT_REASON_NOTIFY)) {
> > -		int ndata = 3;
> > +		gpa_t gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> > +		bool is_mmio = exit_reason.basic == EXIT_REASON_EPT_MISCONFIG;
> 
> There's no need for is_mmio, just pass INVALID_GPA when the GPA isn't known.
> 

Ah alright, then we definitely don't need an is_mmio field. I assume we
can't do MMIO at GPA=0, right?

> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 83fe0a78146f..8ee67fc23e5d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -8828,6 +8828,28 @@ void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu)
> >  }
> >  EXPORT_SYMBOL_GPL(kvm_prepare_emulation_failure_exit);
> >  
> > +void kvm_prepare_ev_delivery_failure_exit(struct kvm_vcpu *vcpu, gpa_t gpa, bool is_mmio)
> 
> Hmm, I don't love the name.  I really don't like that event is abbreviated, and
> I suspect many readers will be misinterpret "event delivery failure" to mean that
> _KVM_ failed to deliver an event.  Which is kinda sorta true, but it's more
> accurate to say that the CPU triggered a VM-Exit when vectoring/delivery an event,
> and KVM doesn't have code to robustly handle the situation.
> 
> Maybe kvm_prepare_event_vectoring_exit()?  Vectoring is quite specific in Intel
> terminology.
> 

Yep, sounds good, I like that the name you suggested doesn't contain 'failure'
part as essentially it is not a failure but an MMIO exit. Will update in V2.

> > +{
> > +	struct kvm_run *run = vcpu->run;
> > +	int ndata = 0;
> > +	u32 reason, intr_info, error_code;
> > +	u64 info1, info2;
> 
> Reverse fir/x-mas tree for variables.  See "Coding Style" in
> Documentation/process/maintainer-kvm-x86.rst (which will redirect you to
> Documentation/process/maintainer-tip.rst, specifically "Variable declarations").
> 

Great, didn't know about that, thanks!

> > +
> > +	kvm_x86_call(get_exit_info)(vcpu, &reason, &info1, &info2, &intr_info, &error_code);
> 
> Wrap.  Though calling back into vendor code is silly.  Pass the necessary info
> as parameters.  E.g. error_code and intr_info are unused, so the above is wasteful
> and weird.
> 

I use it here as this function gets called from the common for svm/vmx
code in the next patch, but as I can see from the next email you've already
noticed that :)

> > +
> > +	run->internal.data[ndata++] = info2;
> > +	run->internal.data[ndata++] = reason;
> > +	run->internal.data[ndata++] = info1;
> > +	if (is_mmio)
> 
> And this is where keying off MMIO gets weird.
> 

We still need to exclude one of the data elements when GPA is not known to be
backwards compatible, so we can get rid of the `is_mmio` argument, but
not from this `if` (unfortunately).

Thank you so much for the review!

Kind regards,
Ivan Orlov