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
