Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Date:   Mon, 16 Oct 2023 17:04:15 +0000
Message-ID: <CWA0YYN7MFBQ.3VOJQRP2X7BY8@amazon.com>
Subject: Re: [PATCH] KVM: x86: hyper-v: Don't auto-enable stimer during
 deserialization
From:   Nicolas Saenz Julienne <nsaenz@amazon.com>
To:     Sean Christopherson <seanjc@google.com>
CC:     Vitaly Kuznetsov <vkuznets@redhat.com>, <kvm@vger.kernel.org>,
        <pbonzini@redhat.com>, <tglx@linutronix.de>, <mingo@redhat.com>,
        <bp@alien8.de>, <dave.hansen@linux.intel.com>, <x86@kernel.org>,
        <hpa@zytor.com>, <graf@amazon.de>, <rkagan@amazon.de>,
        <linux-kernel@vger.kernel.org>
References: <20231016095217.37574-1-nsaenz@amazon.com>
 <87sf6a9335.fsf@redhat.com> <CW9VEIPFLJJA.3OI6RJQVQU7ZN@amazon.com>
 <ZS1kcXuGqO3O7yAq@google.com>
In-Reply-To: <ZS1kcXuGqO3O7yAq@google.com>
Precedence: bulk

Hi Sean,
On Mon Oct 16, 2023 at 4:27 PM UTC, Sean Christopherson wrote:
> I'd prefer the shortlog be more explicit about the write coming from user=
space, e.g.
>
>   KVM: x86: hyper-v: Don't auto-enable stimer on write from userspace
>
> A non-zero number of KVM's "deserialization" ioctls are used to stuff sta=
te
> without a paired "serialization".  I doubt anyone is doing that with the =
Hyper-V
> ioctls, but keeping things consistent is helpful for readers.
>
> On Mon, Oct 16, 2023, Nicolas Saenz Julienne wrote:
> > Hi Vitaly,
> >
> > On Mon Oct 16, 2023 at 12:14 PM UTC, Vitaly Kuznetsov wrote:
> > > Nicolas Saenz Julienne <nsaenz@amazon.com> writes:
> > >
> > > > By not honoring the 'stimer->config.enable' state during stimer
> > > > deserialization we might introduce spurious timer interrupts. For
>
> Avoid pronouns please.
>
> > > > example through the following events:
> > > >  - The stimer is configured in auto-enable mode.
> > > >  - The stimer's count is set and the timer enabled.
> > > >  - The stimer expires, an interrupt is injected.
> > > >  - We live migrate the VM.
>
> Same here.  "We" is already ambiguous, because the first usage is largely=
 about
> KVM, and the second usage here is much more about userspace and/or the ac=
tual
> user.
>
> > > >  - The stimer config and count are deserialized, auto-enable is ON,=
 the
> > > >    stimer is re-enabled.
> > > >  - The stimer expires right away, and injects an unwarranted interr=
upt.
> > > >
> > > > So let's not change the stimer's enable state if the MSR write come=
s
> > > > from user-space.
>
> Don't hedge, firmly state what the patch does and why the change is neces=
sary
> and correct.  If it turns out the change is wrong, then the follow-up pat=
ch can
> explain the situation.  But in the happy case where the change is correct=
, using
> language that isn't assertive can result in
>
> > > > Fixes: 1f4b34f825e8 ("kvm/x86: Hyper-V SynIC timers")
>
> Does this need a?
>
>   Cc: stable@vger.kernel


Your reply raced with my v2. I'll rework the commit message, and send a
third revision.

Nicolas