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