From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 627BEC19F2C for ; Wed, 3 Aug 2022 19:21:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238059AbiHCTVx (ORCPT ); Wed, 3 Aug 2022 15:21:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57508 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236278AbiHCTVu (ORCPT ); Wed, 3 Aug 2022 15:21:50 -0400 Received: from mail-pg1-x535.google.com (mail-pg1-x535.google.com [IPv6:2607:f8b0:4864:20::535]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AF2FD3E750 for ; Wed, 3 Aug 2022 12:21:48 -0700 (PDT) Received: by mail-pg1-x535.google.com with SMTP id s206so15984696pgs.3 for ; Wed, 03 Aug 2022 12:21:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc; bh=L3BXoDAnjXKl7DF5MAL+W0kL9WnJVL0b1/HkhEfMB6o=; b=TjQRomc0BncZKstd0VtuEoVthcM/oSbSEL77eNSNYn0WzPnp2Edcq9KcBb1Vq9iXMx 2FmV8jclNZmRNGHba1FWsdthmkJ3V4XXTaPSkSqHuDG+9xKpKQMzBZUI3PtLAXEjMSIz Gsakob4IjBskgerDp8blcso3BMVVtiZZkx/Wh29jgydRdN9W2q6mqYzP6nrkyvIwEGKp aA43mf6vjD++eHQcpNNLrIcszEocGgEAwKLJL4QV4ONcaqkiXH7d2l0r8KzwbTGlDJhS vSQNrYdhUKAT6TmlwJoMLYxkENEXLhmJxpPs3aqiGDEtrp8sozaZxufTWJ3DaD6N/QRB gDdQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc; bh=L3BXoDAnjXKl7DF5MAL+W0kL9WnJVL0b1/HkhEfMB6o=; b=z2nd/yI8iqc5kuTzYFfVEQHIOJ80v70Tcli4X1uDIbQqHq1iFgVr1Algqa0LJ2O3Zm Qvs0P0Nc9tY6NcTtPacs5l8U3+lFoOSLozn2R/GC6RFvddFrYUeVskMvvxuKYqht2+TZ ahRAn4C1Ul9oHuU5FwswDZUr1Onu8u/SC2ARjL99u8CMpsokYhMOVB79frbqYM+kKHhx cxZ3gzV6OMnd0pejOyS13wfiU0zm9QS96K70E8jJI3W6VmhOCTKiZd3vOSx/TJA5tgYS ptXEGeP1WhGIGulwQZrHNYVBZBWH9KDB/0O01LWKV52kjhLxUsHkTVJx9H+pp978DZxF s4qQ== X-Gm-Message-State: AJIora+ZTNi7NoddM4sy3blKJVzjAprM88y6cMv6MJTvzN2n82TnZ09m bLSG712Nuj5y4YROmAzD1w/iYQ== X-Google-Smtp-Source: AGRyM1sYEfRp+NhnfWgOD9pnywTRoeKibbqNbFltK9LYASso7bT1OFsTtRUBKZ37SNcDWD/PUDLZ6A== X-Received: by 2002:a05:6a00:234f:b0:525:1f7c:f2bf with SMTP id j15-20020a056a00234f00b005251f7cf2bfmr27173548pfj.14.1659554508054; Wed, 03 Aug 2022 12:21:48 -0700 (PDT) Received: from google.com (7.104.168.34.bc.googleusercontent.com. [34.168.104.7]) by smtp.gmail.com with ESMTPSA id t124-20020a635f82000000b0040d75537824sm8339323pgb.86.2022.08.03.12.21.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 03 Aug 2022 12:21:47 -0700 (PDT) Date: Wed, 3 Aug 2022 19:21:43 +0000 From: Sean Christopherson To: Oliver Upton Cc: Mingwei Zhang , Paolo Bonzini , Vitaly Kuznetsov , Wanpeng Li , Jim Mattson , Joerg Roedel , kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/5] selftests: KVM/x86: Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86_state Message-ID: References: <20220802230718.1891356-1-mizhang@google.com> <20220802230718.1891356-3-mizhang@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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".