Re: [PATCH AUTOSEL 5.16 07/28] x86/kvm/fpu: Limit guest user_xfeatures to supported bits of XCR0

From: Peter Xu <peterx@redhat.com>
To: Leonardo Bras Soares Passos <leobras@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sasha Levin <sashal@kernel.org>,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "Chang S. Bae" <chang.seok.bae@intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	kvm@vger.kernel.org
Subject: Re: [PATCH AUTOSEL 5.16 07/28] x86/kvm/fpu: Limit guest user_xfeatures to supported bits of XCR0
Date: Wed, 8 Jun 2022 16:53:04 -0400	[thread overview]
Message-ID: <YqEMMOPwc/ctL26P@xz-m1.local> (raw)
In-Reply-To: <CAJ6HWG7x_VA3JAsopojCq+t2-MDZ-rn4DXZqt0SoXEDxTzrRMQ@mail.gmail.com>

On Wed, Jun 08, 2022 at 05:34:18PM -0300, Leonardo Bras Soares Passos wrote:
> Hello Peter,
> 
> On Tue, Jun 7, 2022 at 5:07 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Tue, Jun 07, 2022 at 02:17:54PM -0400, Peter Xu wrote:
> > > On Tue, Jun 07, 2022 at 03:04:27PM +0000, Sean Christopherson wrote:
> > > > On Tue, Jun 07, 2022, Paolo Bonzini wrote:
> > > > > On 6/6/22 23:27, Peter Xu wrote:
> > > > > > On Mon, Jun 06, 2022 at 06:18:12PM +0200, Paolo Bonzini wrote:
> > > > > > > > However there seems to be something missing at least to me, on why it'll
> > > > > > > > fail a migration from 5.15 (without this patch) to 5.18 (with this patch).
> > > > > > > > In my test case, user_xfeatures will be 0x7 (FP|SSE|YMM) if without this
> > > > > > > > patch, but 0x0 if with it.
> > > > > > >
> > > > > > > What CPU model are you using for the VM?
> > > > > >
> > > > > > I didn't specify it, assuming it's qemu64 with no extra parameters.
> > > > >
> > > > > Ok, so indeed it lacks AVX and this patch can have an effect.
> > > > >
> > > > > > > For example, if the source lacks this patch but the destination has it,
> > > > > > > the source will transmit YMM registers, but the destination will fail to
> > > > > > > set them if they are not available for the selected CPU model.
> > > > > > >
> > > > > > > See the commit message: "As a bonus, it will also fail if userspace tries to
> > > > > > > set fpu features (with the KVM_SET_XSAVE ioctl) that are not compatible to
> > > > > > > the guest configuration.  Such features will never be returned by
> > > > > > > KVM_GET_XSAVE or KVM_GET_XSAVE2."
> > > > > >
> > > > > > IIUC you meant we should have failed KVM_SET_XSAVE when they're not aligned
> > > > > > (probably by failing validate_user_xstate_header when checking against the
> > > > > > user_xfeatures on dest host). But that's probably not my case, because here
> > > > > > KVM_SET_XSAVE succeeded, it's just that the guest gets a double fault after
> > > > > > the precopy migration completes (or for postcopy when the switchover is
> > > > > > done).
> > > > >
> > > > > Difficult to say what's happening without seeing at least the guest code
> > > > > around the double fault (above you said "fail a migration" and I thought
> > > > > that was a different scenario than the double fault), and possibly which was
> > > > > the first exception that contributed to the double fault.
> > > >
> > > > Regardless of why the guest explodes in the way it does, is someone planning on
> > > > bisecting this (if necessary?) and sending a backport to v5.15?  There's another
> > > > bug report that is more than likely hitting the same bug.
> > >
> > > What's the bisection you mentioned?  I actually did a bisection and I also
> > > checked reverting Leo's change can also fix this issue.  Or do you mean
> > > something else?
> >
> > Ah, I forgot to mention on the "stable tree decisions": IIUC it also means
> > we should apply Leo's patch to all the stable trees if possible, then
> > migrations between them won't trigger the misterous faults anymore,
> > including when migrating to the latest Linux versions.
> >
> > However there's the delimma that other kernels (any kernel that does not
> > have Leo's patch) will start to fail migrations to the stable branches that
> > apply Leo's patch too..
> 
> IIUC, you commented before that the migration issue should be solved with a
> QEMU fix, is that correct? That would mean something like 'QEMU is relying on a
> kernel bug to work', and should be no blocker for fixing the kernel.

The QEMU fix (that I posted [1]) is not a real fix, only the kernel fix is.

The QEMU patchset only allows the migration to fail early, the kernel patch
allows the migration to go through with no problem as long as both sides
are applied with the fix (or both are not..).  So there're two issues we're
tackling with and IMHO we should fix both.

[1] https://lore.kernel.org/qemu-devel/20220607230645.53950-1-peterx@redhat.com/

> 
> If that's the case, I think we should apply the fix to every supported
> stable branch that
> have the fpku issue, and in parallel come with a qemu fix for that.
> 
> What do you think about it?

Yes I mostly agree with you. I think your patch still does the right thing
by not migrating anything the guest doesn't even support, and that seems to
be the only way to fix the pksu-like issue on migrations between hosts with
different processor configurations.  But it'll also bring other unwanted
side effects, that's why IMHO we need some careful thoughts and I hope I
didn't miss anything important.

Thanks,

-- 
Peter Xu