Re: [PATCH 2/4] KVM: x86: separate pending and injected exception
On Fri, Apr 02, 2021, Paolo Bonzini wrote:
> On 02/04/21 01:05, Sean Christopherson wrote:
> > >
> > > +struct kvm_queued_exception {
> > > + bool valid;
> > > + u8 nr;
> >
> > If we're refactoring all this code anyways, maybe change "nr" to something a
> > bit more descriptive? E.g. vector.
>
> "nr" is part of the userspace structure, so consistency is an advantage too.
Foiled at every turn. Keeping "nr" probably does make sense.
Re: [PATCH 2/4] KVM: x86: separate pending and injected exception
On 02/04/21 01:05, Sean Christopherson wrote:
+struct kvm_queued_exception {
+ bool valid;
+ u8 nr;
If we're refactoring all this code anyways, maybe change "nr" to something a
bit more descriptive? E.g. vector.
"nr" is part of the userspace structure, so consistency is an advantage too.
+ struct kvm_exception_payload {
+ bool valid;
+ unsigned long value;
u8 nested_apf;
- } exception;
+ } exception_payload;
Hmm, even if it's dead code at this time, I think the exception payload should
be part of 'struct kvm_queued_exception'. The payload is very much tied to a
single exception.
Agreed, when handling injected exceptions you can WARN that there is no
payload.
Paolo
Re: [PATCH 2/4] KVM: x86: separate pending and injected exception
On Thu, Apr 01, 2021, Maxim Levitsky wrote:
> Use 'pending_exception' and 'injected_exception' fields
> to store the pending and the injected exceptions.
>
> After this patch still only one is active, but
> in the next patch both could co-exist in some cases.
Please explain _why_.
> Signed-off-by: Maxim Levitsky
> ---
> arch/x86/include/asm/kvm_host.h | 25 --
> arch/x86/kvm/svm/nested.c | 26 +++---
> arch/x86/kvm/svm/svm.c | 6 +-
> arch/x86/kvm/vmx/nested.c | 36
> arch/x86/kvm/vmx/vmx.c | 12 +--
> arch/x86/kvm/x86.c | 145 ++--
> arch/x86/kvm/x86.h | 6 +-
> 7 files changed, 143 insertions(+), 113 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index a52f973bdff6..3b2fd276e8d5 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -547,6 +547,14 @@ struct kvm_vcpu_xen {
> u64 runstate_times[4];
> };
>
> +struct kvm_queued_exception {
> + bool valid;
> + u8 nr;
If we're refactoring all this code anyways, maybe change "nr" to something a
bit more descriptive? E.g. vector.
> + bool has_error_code;
> + u32 error_code;
> +};
> +
> +
> struct kvm_vcpu_arch {
> /*
>* rip and regs accesses must go through
> @@ -645,16 +653,15 @@ struct kvm_vcpu_arch {
>
> u8 event_exit_inst_len;
>
> - struct kvm_queued_exception {
> - bool pending;
> - bool injected;
> - bool has_error_code;
> - u8 nr;
> - u32 error_code;
> - unsigned long payload;
> - bool has_payload;
> + struct kvm_queued_exception pending_exception;
> +
> + struct kvm_exception_payload {
> + bool valid;
> + unsigned long value;
> u8 nested_apf;
> - } exception;
> + } exception_payload;
Hmm, even if it's dead code at this time, I think the exception payload should
be part of 'struct kvm_queued_exception'. The payload is very much tied to a
single exception.
> +
> + struct kvm_queued_exception injected_exception;
Any objection to keeping the current syntax, arch.exception.{pending,injected}?
Maybe it's fear of change, but I like the current style, I think because the
relevant info is condensed at the end, e.g. I can ignore "vcpu->arch.exception"
and look at "pending.vector" or whatever. E.g.
struct {
struct kvm_queued_exception pending;
struct kvm_queued_exception injected;
} exception;
>
> struct kvm_queued_interrupt {
> bool injected;
[PATCH 2/4] KVM: x86: separate pending and injected exception
Use 'pending_exception' and 'injected_exception' fields
to store the pending and the injected exceptions.
After this patch still only one is active, but
in the next patch both could co-exist in some cases.
Signed-off-by: Maxim Levitsky
---
arch/x86/include/asm/kvm_host.h | 25 --
arch/x86/kvm/svm/nested.c | 26 +++---
arch/x86/kvm/svm/svm.c | 6 +-
arch/x86/kvm/vmx/nested.c | 36
arch/x86/kvm/vmx/vmx.c | 12 +--
arch/x86/kvm/x86.c | 145 ++--
arch/x86/kvm/x86.h | 6 +-
7 files changed, 143 insertions(+), 113 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a52f973bdff6..3b2fd276e8d5 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -547,6 +547,14 @@ struct kvm_vcpu_xen {
u64 runstate_times[4];
};
+struct kvm_queued_exception {
+ bool valid;
+ u8 nr;
+ bool has_error_code;
+ u32 error_code;
+};
+
+
struct kvm_vcpu_arch {
/*
* rip and regs accesses must go through
@@ -645,16 +653,15 @@ struct kvm_vcpu_arch {
u8 event_exit_inst_len;
- struct kvm_queued_exception {
- bool pending;
- bool injected;
- bool has_error_code;
- u8 nr;
- u32 error_code;
- unsigned long payload;
- bool has_payload;
+ struct kvm_queued_exception pending_exception;
+
+ struct kvm_exception_payload {
+ bool valid;
+ unsigned long value;
u8 nested_apf;
- } exception;
+ } exception_payload;
+
+ struct kvm_queued_exception injected_exception;
struct kvm_queued_interrupt {
bool injected;
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 34a37b2bd486..7adad9b6dcad 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -349,14 +349,14 @@ static void nested_save_pending_event_to_vmcb12(struct
vcpu_svm *svm,
u32 exit_int_info = 0;
unsigned int nr;
- if (vcpu->arch.exception.injected) {
- nr = vcpu->arch.exception.nr;
+ if (vcpu->arch.injected_exception.valid) {
+ nr = vcpu->arch.injected_exception.nr;
exit_int_info = nr | SVM_EVTINJ_VALID | SVM_EVTINJ_TYPE_EXEPT;
- if (vcpu->arch.exception.has_error_code) {
+ if (vcpu->arch.injected_exception.has_error_code) {
exit_int_info |= SVM_EVTINJ_VALID_ERR;
vmcb12->control.exit_int_info_err =
- vcpu->arch.exception.error_code;
+ vcpu->arch.injected_exception.error_code;
}
} else if (vcpu->arch.nmi_injected) {
@@ -1000,30 +1000,30 @@ int nested_svm_check_permissions(struct kvm_vcpu *vcpu)
static bool nested_exit_on_exception(struct vcpu_svm *svm)
{
- unsigned int nr = svm->vcpu.arch.exception.nr;
+ unsigned int nr = svm->vcpu.arch.pending_exception.nr;
return (svm->nested.ctl.intercepts[INTERCEPT_EXCEPTION] & BIT(nr));
}
static void nested_svm_inject_exception_vmexit(struct vcpu_svm *svm)
{
- unsigned int nr = svm->vcpu.arch.exception.nr;
+ unsigned int nr = svm->vcpu.arch.pending_exception.nr;
svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + nr;
svm->vmcb->control.exit_code_hi = 0;
- if (svm->vcpu.arch.exception.has_error_code)
- svm->vmcb->control.exit_info_1 =
svm->vcpu.arch.exception.error_code;
+ if (svm->vcpu.arch.pending_exception.has_error_code)
+ svm->vmcb->control.exit_info_1 =
svm->vcpu.arch.pending_exception.error_code;
/*
* EXITINFO2 is undefined for all exception intercepts other
* than #PF.
*/
if (nr == PF_VECTOR) {
- if (svm->vcpu.arch.exception.nested_apf)
+ if (svm->vcpu.arch.exception_payload.nested_apf)
svm->vmcb->control.exit_info_2 =
svm->vcpu.arch.apf.nested_apf_token;
- else if (svm->vcpu.arch.exception.has_payload)
- svm->vmcb->control.exit_info_2 =
svm->vcpu.arch.exception.payload;
+ else if (svm->vcpu.arch.exception_payload.valid)
+ svm->vmcb->control.exit_info_2 =
svm->vcpu.arch.exception_payload.value;
else
svm->vmcb->control.exit_info_2 = svm->vcpu.arch.cr2;
} else if (nr == DB_VECTOR) {
@@ -1034,7 +1034,7 @@ static void nested_svm_inject_exception_vmexit(struct
vcpu_svm *svm)
kvm_update_dr7(>vcpu);
}
} else
- WARN_ON(svm->vcpu.arch.exception.has_payload);
+ WARN_ON(svm->vcpu.arch.exception_payload.valid);
nested_svm_vmexit(svm);
}
@@
