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