Re: Patch "KVM: VMX: Convert vcpu_vmx.exit_reason to a union" has been added to the 5.10-stable tree

Re: Patch "KVM: VMX: Convert vcpu_vmx.exit_reason to a union" has been added to the 5.10-stable tree

[Sean Christopherson]

+Paolo

On Sun, Apr 18, 2021, Sasha Levin wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     KVM: VMX: Convert vcpu_vmx.exit_reason to a union
> 
> to the 5.10-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      kvm-vmx-convert-vcpu_vmx.exit_reason-to-a-union.patch
> and it can be found in the queue-5.10 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.

I'm not sure we want this going into stable kernels, even for 5.10 and 5.11.
I assume it got pulled in to resolve a conflict with commit 04c4f2ee3f68 ("KVM:
VMX: Don't use vcpu->run->internal.ndata as an array index"), but that's should
be trivial to resolve since it's just a collision with surrounding code.

Maybe we'll end up with a more painful conflict in the future that would be best
solved by grabbing this refactoring, but I don't think we're there yet.


> commit 1499b54db7d9e7e5f5014307e8391e3ad7986f1f
> Author: Sean Christopherson <seanjc@google.com>
> Date:   Fri Nov 6 17:03:12 2020 +0800
> 
>     KVM: VMX: Convert vcpu_vmx.exit_reason to a union
>     
>     [ Upstream commit 8e53324021645f820a01bf8aa745711c802c8542 ]
>     
>     Convert vcpu_vmx.exit_reason from a u32 to a union (of size u32).  The
>     full VM_EXIT_REASON field is comprised of a 16-bit basic exit reason in
>     bits 15:0, and single-bit modifiers in bits 31:16.
>     
>     Historically, KVM has only had to worry about handling the "failed
>     VM-Entry" modifier, which could only be set in very specific flows and
>     required dedicated handling.  I.e. manually stripping the FAILED_VMENTRY
>     bit was a somewhat viable approach.  But even with only a single bit to
>     worry about, KVM has had several bugs related to comparing a basic exit
>     reason against the full exit reason store in vcpu_vmx.
>     
>     Upcoming Intel features, e.g. SGX, will add new modifier bits that can
>     be set on more or less any VM-Exit, as opposed to the significantly more
>     restricted FAILED_VMENTRY, i.e. correctly handling everything in one-off
>     flows isn't scalable.  Tracking exit reason in a union forces code to
>     explicitly choose between consuming the full exit reason and the basic
>     exit, and is a convenient way to document and access the modifiers.
>     
>     No functional change intended.
>     
>     Cc: Xiaoyao Li <xiaoyao.li@intel.com>
>     Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>     Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>     Message-Id: <20201106090315.18606-2-chenyi.qiang@intel.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>

Re: Patch "KVM: VMX: Convert vcpu_vmx.exit_reason to a union" has been added to the 5.10-stable tree

[Sasha Levin]

On Mon, Apr 19, 2021 at 09:54:18PM +0000, Sean Christopherson wrote:
>+Paolo
>
>On Sun, Apr 18, 2021, Sasha Levin wrote:
>> This is a note to let you know that I've just added the patch titled
>>
>>     KVM: VMX: Convert vcpu_vmx.exit_reason to a union
>>
>> to the 5.10-stable tree which can be found at:
>>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>>      kvm-vmx-convert-vcpu_vmx.exit_reason-to-a-union.patch
>> and it can be found in the queue-5.10 subdirectory.
>>
>> If you, or anyone else, feels it should not be added to the stable tree,
>> please let <stable@vger.kernel.org> know about it.
>
>I'm not sure we want this going into stable kernels, even for 5.10 and 5.11.
>I assume it got pulled in to resolve a conflict with commit 04c4f2ee3f68 ("KVM:
>VMX: Don't use vcpu->run->internal.ndata as an array index"), but that's should

Right.

>be trivial to resolve since it's just a collision with surrounding code.

That's probably right too.

>Maybe we'll end up with a more painful conflict in the future that would be best
>solved by grabbing this refactoring, but I don't think we're there yet.

This is the tricky part: when we start having these conflicts it's
usually too late to refactor, no one cares, and backports just don't
happen.

I'd actually point to the file shuffling (commits like a821bab2d1ee
("KVM: VMX: Move VMX specific files to a "vmx" subdirectory")) you did a
few years ago in arch/x86/kvm/ as an example to why we can't wait: those
changes made a lot of sense upstream, but for stable kernels it meant
that patches were now trying to touch the wrong files and would often
fail or do the wrong thing.

On hindsight, we probably should have moved files around in stable trees
as well to match what upstream had, but at this point it's too late to
go back and fix that, and we're stuck manually editing paths for the
lifetime of most of the LTS trees.

-- 
Thanks,
Sasha

Re: Patch "KVM: VMX: Convert vcpu_vmx.exit_reason to a union" has been added to the 5.10-stable tree

[Sean Christopherson]

On Mon, Apr 19, 2021, Sasha Levin wrote:
> On Mon, Apr 19, 2021 at 09:54:18PM +0000, Sean Christopherson wrote:
> > Maybe we'll end up with a more painful conflict in the future that would be best
> > solved by grabbing this refactoring, but I don't think we're there yet.
> 
> This is the tricky part: when we start having these conflicts it's
> usually too late to refactor, no one cares, and backports just don't
> happen.
> 
> I'd actually point to the file shuffling (commits like a821bab2d1ee
> ("KVM: VMX: Move VMX specific files to a "vmx" subdirectory")) you did a
> few years ago in arch/x86/kvm/ as an example to why we can't wait: those
> changes made a lot of sense upstream, but for stable kernels it meant
> that patches were now trying to touch the wrong files and would often
> fail or do the wrong thing.
> 
> On hindsight, we probably should have moved files around in stable trees
> as well to match what upstream had, but at this point it's too late to
> go back and fix that, and we're stuck manually editing paths for the
> lifetime of most of the LTS trees.

And I guess there's also the argument that inducing even a handful of manual
backports is more risky overall than taking this one "unnecessary" patch.

Objection withdrawn, I don't have a strong opinion either way :-)