Re: [PATCH v6 6/9] KVM: x86: lapic: don't allow to change APIC ID unconditionally

[Sean Christopherson <seanjc@google.com>]

On Wed, Mar 09, 2022, Maxim Levitsky wrote:
> On Wed, 2022-03-09 at 06:01 +0000, Sean Christopherson wrote:
> > > Could you share the links?
> > 
> > Doh, sorry (they're both in this one).
> > 
> > https://lore.kernel.org/all/20220301135526.136554-5-mlevitsk@redhat.com
> > 
> 
> My opinion on this subject is very simple: we need to draw the line somewhere.

...

> I also understand your concerns - and I am not going to fight over this, a module
> param for read only apic id, will work for me.

Sadly, I don't think a module param would actually help.  I was thinking it would
avoid breakage by allowing for graceful fallback on migration failure, but that
was wishful thinking.  An inhibit seems like the least awful idea if we don't end
up making it unconditionally readonly.

> All I wanted to do is to make KVM better by simplifying it - KVM is already
> as complex as it can get, anything to make it simpler is welcome IMHO.

I agree that simplifying KVM is a goal, and that we need to decide when enough is
enough.  But we also can't break userspace or existing deployments, that's a very
clearly drawn line in Linux.

My biggest worry is that, unlike the KVM_SET_CPUID2 breakage, which was obvious
and came relatively quick, this could cause breakage at the worst possible time
(migration) months or years down the road.

Since the goal is to simplify KVM, can we try the inhibit route and see what the
code looks like before making a decision?  I think it might actually yield a less
awful KVM than the readonly approach, especially if the inhibit is "sticky", i.e.
we don't try to remove the inhibit on subsequent changes.

Killing the VM, as proposed, is very user unfriendly as the user will have no idea
why the VM was killed.  WARN is out of the question because this is user triggerable.
Returning an emulation error would be ideal, but getting that result up through
apic_mmio_write() could be annoying and end up being more complex.

The touchpoints will all be the same, unless I'm missing something the difference
should only be a call to set an inhibit instead killing the VM.