Received-SPF: pass (google.com: domain of linux-kernel+bounces-87317-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 29 Feb 2024 10:54:18 -0800
In-Reply-To: <87h6hrmmox.fsf@redhat.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240228101837.93642-1-vkuznets@redhat.com> <20240228101837.93642-3-vkuznets@redhat.com>
 <Zd_BY8Us6TYNBueI@google.com> <87h6hrmmox.fsf@redhat.com>
Message-ID: <ZeDS2nhkK_QDBJS0@google.com>
Subject: Re: [PATCH 2/3] KVM: x86: Use actual kvm_cpuid.base for clearing KVM_FEATURE_PV_UNHALT
From: Sean Christopherson <seanjc@google.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, 
	Li RongQing <lirongqing@baidu.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

On Thu, Feb 29, 2024, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > Am I missing something, or can we just swap() the new and old, update the new
> > in the context of the vCPU, and then undo the swap() if there's an issue?
> > vcpu->mutex is held, and accessing this state from a different task is wildly
> > unsafe, so I don't see any problem with temporarily having an in-flux state.
> >
> 
> I don't see why this approach shouldn't work and I agree it looks like
> it would make things better but I can't say that I'm in love with
> it.

Agreed, but the lack of atomicity is a pre-existing problem, though as proposed,
my idea would make it worse.  More below.

> Ideally, I would want to see the following "atomic" workflow for all
> updates:
> 
> - Check that the supplied data is correct, return an error if not. No
> changes to the state on this step.
> - Tweak the data if needed.
> - Update the state and apply the side-effects of the update. Ideally,
> there should be no errors on this step as rollback can be
> problemmatic. In the real world we will have to handle e.g. failed
> memory allocations here but in most cases the best course of action is
> to kill the VM.
> 
> Well, kvm_set_cpuid() is not like that. At least:
> - kvm_hv_vcpu_init() is a side-effect but we apply it before all checks
> are complete. There's no way back.
> - kvm_check_cpuid() sounds like a pure checker but in reallity we end up
> mangling guest FPU state in fpstate_realloc()

Yeah, I really, really don't like the call to fpu_enable_guest_xfd_features().
But to not make it worse, that call could be hoisted out of kvm_check_cpuid()
so that it can be performed after kvm_cpuid_check_equal(), i.e. be kept dead last
(and with a comment saying it needs to be dead last due to side effects that are
visible to serspace).

> Both are probably "no big deal" but certainly break the atomicity.
>
> > If we want to be paranoid, we can probably get away with killing the VM if the
> > vCPU has run and the incoming CPUID is "bad", e.g. to guard against something
> > in kvm_set_cpuid() consuming soon-to-be-stale state.  And that's actually a
> > feature of sorts, because _if_ something in kvm_set_cpuid() consumes the vCPU's
> > CPUID, then we have a bug _now_ that affects the happy path.
> >
> > Completely untested (I haven't updated the myriad helpers), but this would allow
> > us to revert/remove all of the changes that allow peeking at a CPUID array that
> > lives outside of the vCPU.
> 
> Thanks, assuming there's no urgency

Definitely no urgency.

> let me take a look at this in the course of the next week or so.

No need, it was more of an "FYI, this is what I may go futz with".  Specifically,
it will impact what I want to do with guest cpu_caps[*], hopefully in a good way.
My plan is to play around with it when I get back to that series.

[*] https://lore.kernel.org/all/20231110235528.1561679-1-seanjc@google.com