| Message ID | 20220802230718.1891356-3-mizhang@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a race between posted interrupt delivery and migration in a nested VM | expand |
Hi Mingwei, On Tue, Aug 02, 2022 at 11:07:15PM +0000, Mingwei Zhang wrote: > Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86_state and > properly save/restore it in vcpu_{save,load}_state(). When vcpu resets, > APIC state become software disabled in kernel and thus the corresponding > vCPU is not able to receive posted interrupts [1]. So, add APIC > save/restore in userspace in selftest library code. Of course, there are no hard rules around it but IMO a changelog is easier to grok if it first describes the what/why of the problem, then afterwards how it is fixed by the commit. > [1] commit 97222cc83163 ("KVM: Emulate local APIC in kernel"). What is the reason for the citation here? > Cc: Jim Mattson <jmattson@google.com> > nit: no newline between footers. > Signed-off-by: Mingwei Zhang <mizhang@google.com> -- Thanks, Oliver
KVM: selftests: for the shortlog. On Wed, Aug 03, 2022, Oliver Upton wrote: > Hi Mingwei, > > On Tue, Aug 02, 2022 at 11:07:15PM +0000, Mingwei Zhang wrote: > > Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86_state and > > properly save/restore it in vcpu_{save,load}_state(). When vcpu resets, > > APIC state become software disabled in kernel and thus the corresponding > > vCPU is not able to receive posted interrupts [1]. So, add APIC > > save/restore in userspace in selftest library code. > > Of course, there are no hard rules around it but IMO a changelog is > easier to grok if it first describes the what/why of the problem, then > afterwards how it is fixed by the commit. I strongly disagree. :-) To some extent, it's a personal preference, e.g. I find it easier to understand the details (why something is a problem) if I have the extra context of how a problem is fixed (or: what code was broken). But beyond personal preference, there are less subjective reasons for stating what a patch does before diving into details. First and foremost, what code is actually being changed is the most important information, and so that information should be easy to find. Changelogs that bury the "what's actually changing" in a one-liner after 3+ paragraphs of background make it very hard to find that information. Maybe for initial review one could argue that "what's the bug" is more important, but for skimming logs and git archeology, the gory details matter less and less. E.g. when doing a series of "git blame", the details of each change along the way are useless, the details only matter for the culprit; I just want to quickly determine whether or not a commit might be of interest. Another argument for stating "what's changing" first is that it's almost always possible to state "what's changing" in a single sentence. Conversely, all but the most simple bugs require multiple sentences or paragraphs to fully describe the problem. If both the "what's changing" and "what's the bug" are super short then the order doesn't matter. But if one is shorter (almost always the "what's changing), then covering the shorter one first is advantageous because it's less of an inconvenience for readers/reviewers that have a strict ordering preference. E.g. having to skip one sentence to get to the stuff you care about is less painful than me having to skip three paragraphs to get to the stuff that I care about. I think the underlying problem with this changelog (and the shortlog) is that it's too literal about what is being fixed. Shortlogs and changelogs shouldn't be play-by-play descriptions of the code changes, they should be abstractions of the problem and the fix. E.g. KVM: selftests: Save/restore vAPIC state in "migration" tests Save/restore vAPIC state as part of vCPU save/load so that it's preserved across VM "migration". This will allow testing that posted interrupts are properly handled across VM migration. With that, the first sentence covers both the "what's changing" and provides a high-level description of the "bug" it's fixing. And the second sentence covers (a) "why do we want this patch", (b) "why wasn't this a problem before", and (c) "what's the urgency of this patch".
On Wed, Aug 03, 2022 at 07:21:43PM +0000, Sean Christopherson wrote: > KVM: selftests: for the shortlog. > > On Wed, Aug 03, 2022, Oliver Upton wrote: > > Hi Mingwei, > > > > On Tue, Aug 02, 2022 at 11:07:15PM +0000, Mingwei Zhang wrote: > > > Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86_state and > > > properly save/restore it in vcpu_{save,load}_state(). When vcpu resets, > > > APIC state become software disabled in kernel and thus the corresponding > > > vCPU is not able to receive posted interrupts [1]. So, add APIC > > > save/restore in userspace in selftest library code. > > > > Of course, there are no hard rules around it but IMO a changelog is > > easier to grok if it first describes the what/why of the problem, then > > afterwards how it is fixed by the commit. > > I strongly disagree. :-) To some extent, it's a personal preference, e.g. I > find it easier to understand the details (why something is a problem) if I have > the extra context of how a problem is fixed (or: what code was broken). > Sorry, what I wrote definitely was asking for strict ordering. Thank you for rightly calling that out. My actual issue if I had been bothered to articulate it well was that $WHAT was effectively restated in different terms which can be confusing. Where possible, atomically addressing what, why and how can lead to a crisper changelog. [...] > KVM: selftests: Save/restore vAPIC state in "migration" tests > > Save/restore vAPIC state as part of vCPU save/load so that it's preserved > across VM "migration". This will allow testing that posted interrupts > are properly handled across VM migration. > > With that, the first sentence covers both the "what's changing" and provides a > high-level description of the "bug" it's fixing. And the second sentence covers > (a) "why do we want this patch", (b) "why wasn't this a problem before", and (c) > "what's the urgency of this patch". LGTM. -- Thanks, Oliver
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index 24fde97f6121..ac883b8eab57 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -457,6 +457,16 @@ static inline void vcpu_fpu_set(struct kvm_vcpu *vcpu, struct kvm_fpu *fpu) vcpu_ioctl(vcpu, KVM_SET_FPU, fpu); } +static inline void vcpu_apic_get(struct kvm_vcpu *vcpu, struct kvm_lapic_state *apic) +{ + vcpu_ioctl(vcpu, KVM_GET_LAPIC, apic); +} + +static inline void vcpu_apic_set(struct kvm_vcpu *vcpu, struct kvm_lapic_state *apic) +{ + vcpu_ioctl(vcpu, KVM_SET_LAPIC, apic); +} + static inline int __vcpu_get_reg(struct kvm_vcpu *vcpu, uint64_t id, void *addr) { struct kvm_one_reg reg = { .id = id, .addr = (uint64_t)addr }; diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h index 45edf45821d0..bf5f874709a4 100644 --- a/tools/testing/selftests/kvm/include/x86_64/processor.h +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h @@ -225,6 +225,7 @@ struct kvm_x86_state { struct kvm_nested_state nested; char nested_[16384]; }; + struct kvm_lapic_state apic; struct kvm_msrs msrs; }; diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c index f35626df1dea..d18da71654b6 100644 --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c @@ -980,6 +980,7 @@ struct kvm_x86_state *vcpu_save_state(struct kvm_vcpu *vcpu) vcpu_msrs_get(vcpu, &state->msrs); vcpu_debugregs_get(vcpu, &state->debugregs); + vcpu_apic_get(vcpu, &state->apic); return state; } @@ -997,6 +998,7 @@ void vcpu_load_state(struct kvm_vcpu *vcpu, struct kvm_x86_state *state) vcpu_mp_state_set(vcpu, &state->mp_state); vcpu_debugregs_set(vcpu, &state->debugregs); vcpu_regs_set(vcpu, &state->regs); + vcpu_apic_set(vcpu, &state->apic); if (state->nested.size) vcpu_nested_state_set(vcpu, &state->nested);
Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86_state and properly save/restore it in vcpu_{save,load}_state(). When vcpu resets, APIC state become software disabled in kernel and thus the corresponding vCPU is not able to receive posted interrupts [1]. So, add APIC save/restore in userspace in selftest library code. [1] commit 97222cc83163 ("KVM: Emulate local APIC in kernel"). Cc: Jim Mattson <jmattson@google.com> Signed-off-by: Mingwei Zhang <mizhang@google.com> --- tools/testing/selftests/kvm/include/kvm_util_base.h | 10 ++++++++++ tools/testing/selftests/kvm/include/x86_64/processor.h | 1 + tools/testing/selftests/kvm/lib/x86_64/processor.c | 2 ++ 3 files changed, 13 insertions(+)