linux-next: manual merge of the kvm tree with the tip tree

linux-next: manual merge of the kvm tree with the tip tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

Hi all,

Today's linux-next merge of the kvm tree got a conflict in:

  arch/x86/kvm/x86.c

between commit:

  eb012ef3b4e3 ("x86: Remove Intel MPX")

from the tip tree and commit:

  b666a4b69739 ("kvm: x86: Dynamically allocate guest_fpu")

from the kvm tree.

I fixed it up (the former removed some code updated by the latter, so I
did that) and can carry the fix as necessary. This is now fixed as far as
linux-next is concerned, but any non trivial conflicts should be mentioned
to your upstream maintainer when your tree is submitted for merging.
You may also want to consider cooperating with the maintainer of the
conflicting tree to minimise any particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

"x86: Remove Intel MPX" is wrong (Re: linux-next: manual merge of the kvm tree with the tip tree)

[Paolo Bonzini]

On 19/12/18 05:12, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the kvm tree got a conflict in:
> 
>   arch/x86/kvm/x86.c
> 
> between commit:
> 
>   eb012ef3b4e3 ("x86: Remove Intel MPX")
> 
> from the tip tree and commit:
> 
>   b666a4b69739 ("kvm: x86: Dynamically allocate guest_fpu")
> 
> from the kvm tree.
> 
> I fixed it up (the former removed some code updated by the latter, so I
> did that) and can carry the fix as necessary. This is now fixed as far as
> linux-next is concerned, but any non trivial conflicts should be mentioned
> to your upstream maintainer when your tree is submitted for merging.
> You may also want to consider cooperating with the maintainer of the
> conflicting tree to minimise any particularly complex conflicts.

Ouch, this resolution is wrong.  KVM will lack support for MPX in guests
now, and that is bad because it breaks live migration.  A simple fix
would be to leave the XSAVE state enabled in the kernel unconditionally
even if all the other gunk is removed; alternatively I can also try to
save/restore it only for the guest FPU.

If this patch can be bumped to 4.22, I would prefer that because it
would save me and Linus some merge window headaches.  Considering that
the patch lacked my Cc or Ack, perhaps it's the right thing to do.

Paolo

Re: "x86: Remove Intel MPX" is wrong (Re: linux-next: manual merge of the kvm tree with the tip tree)

[Dave Hansen]

On 12/19/18 12:32 PM, Paolo Bonzini wrote:
> On 19/12/18 05:12, Stephen Rothwell wrote:
>> I fixed it up (the former removed some code updated by the latter, so I
>> did that) and can carry the fix as necessary. This is now fixed as far as
>> linux-next is concerned, but any non trivial conflicts should be mentioned
>> to your upstream maintainer when your tree is submitted for merging.
>> You may also want to consider cooperating with the maintainer of the
>> conflicting tree to minimise any particularly complex conflicts.
> 
> Ouch, this resolution is wrong.  KVM will lack support for MPX in guests
> now, and that is bad because it breaks live migration.

I should have called this out in the changelog, but I removed *all* the
support because I assumed that guests don't need MPX because no other OS
supported it that I know of.

> A simple fix would be to leave the XSAVE state enabled in the kernel
> unconditionally even if all the other gunk is removed; alternatively
> I can also try to save/restore it only for the guest FPU.

We could do this in two phases: remove the APIs now, and then remove the
XSAVE enabling later (4.22 or whenever).

But, on the other hand, if we want to remove XSAVE support for MPX,
we'll have to break live migration at _some_ point, so why not just do
it now?

> If this patch can be bumped to 4.22, I would prefer that because it
> would save me and Linus some merge window headaches.  Considering that
> the patch lacked my Cc or Ack, perhaps it's the right thing to do.

That's fine with me as well.

Re: "x86: Remove Intel MPX" is wrong (Re: linux-next: manual merge of the kvm tree with the tip tree)

[Paolo Bonzini]

On 19/12/18 21:54, Dave Hansen wrote:
> I should have called this out in the changelog, but I removed *all* the
> support because I assumed that guests don't need MPX because no other OS
> supported it that I know of.

Well, as long as you could have code that sets the MPX bits in XCR0, KVM
will have to support it.  My employer happens to sell one such kernel
and will probably do so a little less than ten years from now. :)

>> A simple fix would be to leave the XSAVE state enabled in the kernel
>> unconditionally even if all the other gunk is removed; alternatively
>> I can also try to save/restore it only for the guest FPU.
> 
> We could do this in two phases: remove the APIs now, and then remove the
> XSAVE enabling later (4.22 or whenever).
> 
> But, on the other hand, if we want to remove XSAVE support for MPX,
> we'll have to break live migration at _some_ point, so why not just do
> it now?

In fact I'm not sure we want to ever remove XSAVE support for MPX in KVM
as long as the processor supports it.  That is, when KVM does
xsave/xrstor of the guest_fpu, we probably want to include MPX in there.
 That can be contained within KVM, Linux need not enable it in XCR0, but
there is one ugly thing: the xsavec offsets would be different between
guest_fpu and other FPUs.  Since KVM is not using supervisor states,
perhaps it's better if that part of the KVM code is completely detached
from the kernel FPU code and uses xsaveopt/xrstoropt instead.

So perhaps the plan should be:

1) remove MPX APIs now

2) then stop using kernel xsave code for KVM's guest_fpu

3) then stop enabling MPX XSAVE in core kernel.

Paolo

>> If this patch can be bumped to 4.22, I would prefer that because it
>> would save me and Linus some merge window headaches.  Considering that
>> the patch lacked my Cc or Ack, perhaps it's the right thing to do.
> 
> That's fine with me as well.

Re: "x86: Remove Intel MPX" is wrong (Re: linux-next: manual merge of the kvm tree with the tip tree)

[Dave Hansen]

On 12/19/18 1:00 PM, Paolo Bonzini wrote:
> On 19/12/18 21:54, Dave Hansen wrote:
>> I should have called this out in the changelog, but I removed *all* the
>> support because I assumed that guests don't need MPX because no other OS
>> supported it that I know of.
> 
> Well, as long as you could have code that sets the MPX bits in XCR0, KVM
> will have to support it.  My employer happens to sell one such kernel
> and will probably do so a little less than ten years from now. :)

Does your employer sell a system that supports live migration across
major releases?  Or, is it always that you support migration to _newer_
releases but not older?

>>> A simple fix would be to leave the XSAVE state enabled in the kernel
>>> unconditionally even if all the other gunk is removed; alternatively
>>> I can also try to save/restore it only for the guest FPU.
>>
>> We could do this in two phases: remove the APIs now, and then remove the
>> XSAVE enabling later (4.22 or whenever).
>>
>> But, on the other hand, if we want to remove XSAVE support for MPX,
>> we'll have to break live migration at _some_ point, so why not just do
>> it now?
> 
> In fact I'm not sure we want to ever remove XSAVE support for MPX in KVM
> as long as the processor supports it.  That is, when KVM does
> xsave/xrstor of the guest_fpu, we probably want to include MPX in there.
> That can be contained within KVM, Linux need not enable it in XCR0, 

I believe you need the feature bit set in XCR0 for XSAVE* to be able to
operate on it.  So, you could do this, but you would need to save XCR0,
set the XCR0 MPX bits, do XSAVE or XRSTOR, and restore XCR0 all with
preemption (and interrupts?) off.

You could just open-code the MPX save/restore, though.  MPX is
XSAVE-managed, but not XSAVE-enabled like some other features, IIRC.

Re: "x86: Remove Intel MPX" is wrong (Re: linux-next: manual merge of the kvm tree with the tip tree)

[Paolo Bonzini]

On 19/12/18 22:28, Dave Hansen wrote:
> 
> On 12/19/18 1:00 PM, Paolo Bonzini wrote:
>> On 19/12/18 21:54, Dave Hansen wrote:
>>> I should have called this out in the changelog, but I removed *all* the
>>> support because I assumed that guests don't need MPX because no other OS
>>> supported it that I know of.
>>
>> Well, as long as you could have code that sets the MPX bits in XCR0, KVM
>> will have to support it.  My employer happens to sell one such kernel
>> and will probably do so a little less than ten years from now. :)
> 
> Does your employer sell a system that supports live migration across
> major releases?  Or, is it always that you support migration to _newer_
> releases but not older?

Only to the immediately following major release, but a major release has
a looooong lifetime.  So guests running on RHEL6 will have to reboot
when moving to RHEL8, and will drop MPX support.  But RHEL8 is stuck
supporting MPX even if it's off by default because guests can be
migrated from RHEL7 hosts to RHEL8.

>> In fact I'm not sure we want to ever remove XSAVE support for MPX in KVM
>> as long as the processor supports it.  That is, when KVM does
>> xsave/xrstor of the guest_fpu, we probably want to include MPX in there.
>> That can be contained within KVM, Linux need not enable it in XCR0, 
> 
> I believe you need the feature bit set in XCR0 for XSAVE* to be able to
> operate on it.  So, you could do this, but you would need to save XCR0,
> set the XCR0 MPX bits, do XSAVE or XRSTOR, and restore XCR0 all with
> preemption (and interrupts?) off.

Yes, and on context switch KVM does rely on the kernel saving/restoring
MPX state to userspace (QEMU)'s FPU struct though.  However, I can move
that to the preempt notifier, either open coded or wrapped with XSETBVs.
 One more reason to do our own xsave/xrstor in non-compacted format when
loading/storing guest_fpu.

Paolo

> You could just open-code the MPX save/restore, though.  MPX is
> XSAVE-managed, but not XSAVE-enabled like some other features, IIRC.