Re: [PATCH] KVM: x86: hyper-v: Don't auto-enable stimer during deserialization

[Nicolas Saenz Julienne <nsaenz@amazon.com>]

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 userspace, 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 state
> 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 actual
> 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 interrupt.
> > > >
> > > > So let's not change the stimer's enable state if the MSR write comes
> > > > from user-space.
>
> Don't hedge, firmly state what the patch does and why the change is necessary
> and correct.  If it turns out the change is wrong, then the follow-up patch 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