linux-kernel - Re: [PATCH 2/5] selftests: KVM/x86: Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YusK4OxpNQ6cQavN@google.com>
Date:   Wed, 3 Aug 2022 23:55:12 +0000
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Mingwei Zhang <mizhang@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] selftests: KVM/x86: Fix vcpu_{save,load}_state() by
 adding APIC state into kvm_x86_state

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