| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zwmyzg5WiKKvySS1@google.com>
Date: Fri, 11 Oct 2024 16:20:46 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Ivan Orlov <iorlov@...zon.com>
Cc: bp@...en8.de, dave.hansen@...ux.intel.com, mingo@...hat.com,
pbonzini@...hat.com, shuah@...nel.org, tglx@...utronix.de, hpa@...or.com,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, x86@...nel.org, jalliste@...zon.com,
nh-open-source@...zon.com, pdurrant@...zon.co.uk
Subject: Re: [PATCH 1/3] KVM: x86, vmx: Add function for event delivery error generation
"KVM: VMX:" for the scope. See "Shortlog" in Documentation/process/maintainer-kvm-x86.rst
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:
--
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.
--
> when handling the MMIO during event delivery error on SVM.
>
> No functional change intended.
>
> Signed-off-by: Ivan Orlov <iorlov@...zon.com>
> ---
> arch/x86/include/asm/kvm_host.h | 2 ++
> arch/x86/kvm/vmx/vmx.c | 15 +++------------
> arch/x86/kvm/x86.c | 22 ++++++++++++++++++++++
> 3 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6d9f763a7bb9..348daba424dd 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2060,6 +2060,8 @@ void __kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu,
> u64 *data, u8 ndata);
> void kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu);
>
> +void kvm_prepare_ev_delivery_failure_exit(struct kvm_vcpu *vcpu, gpa_t gpa, bool is_mmio);
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).
> void kvm_enable_efer_bits(u64);
> bool kvm_valid_efer(struct kvm_vcpu *vcpu, u64 efer);
> int kvm_get_msr_with_filter(struct kvm_vcpu *vcpu, u32 index, u64 *data);
> 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.
> - vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> - vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
> - vcpu->run->internal.data[0] = vectoring_info;
> - vcpu->run->internal.data[1] = exit_reason.full;
> - vcpu->run->internal.data[2] = vmx_get_exit_qual(vcpu);
> - if (exit_reason.basic == EXIT_REASON_EPT_MISCONFIG) {
> - vcpu->run->internal.data[ndata++] =
> - vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> - }
> - vcpu->run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
> - vcpu->run->internal.ndata = ndata;
> + kvm_prepare_ev_delivery_failure_exit(vcpu, gpa, is_mmio);
> return 0;
> }
>
> 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.
> +{
> + 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").
> +
> + 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.
> +
> + 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.
> + run->internal.data[ndata++] = (u64)gpa;
> + run->internal.data[ndata++] = vcpu->arch.last_vmentry_cpu;
> +
> + run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> + run->internal.suberror = KVM_INTERNAL_ERROR_DELIVERY_EV;
> + run->internal.ndata = ndata;
> +}
> +EXPORT_SYMBOL_GPL(kvm_prepare_ev_delivery_failure_exit);
> +
> static int handle_emulation_failure(struct kvm_vcpu *vcpu, int emulation_type)
> {
> struct kvm *kvm = vcpu->kvm;
> --
> 2.43.0
>
Powered by blists - more mailing lists