Date:   Fri, 10 Mar 2023 14:23:55 -0800
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Steven Rostedt <rostedt@goodmis.org>, x86@kernel.org,
        linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Jason Baron <jbaron@akamai.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [RFC][PATCH 5/5] x86/kvm: Simplify static call handling
Message-ID: <20230310222355.digoem4orcpft2t6@treble>
References: <cover.1678474914.git.jpoimboe@kernel.org>
 <432e4844ba65840af4a24f5e3f561aead867f6e7.1678474914.git.jpoimboe@kernel.org>
 <ZAucD8gHx8Xp8Dlb@google.com>
 <20230310161354.1889b539@gandalf.local.home>
 <ZAuhQNS8mjRt1bOG@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZAuhQNS8mjRt1bOG@google.com>
Precedence: bulk

On Fri, Mar 10, 2023 at 09:29:36PM +0000, Sean Christopherson wrote:
> > > I would much prefer to keep KVM mostly as-is, specifically so that we don't lose
> > > this WARN_ON() that guards against a vendor module neglecting to implement a
> > > mandatory callback.  This effectively gives KVM "full" protection against consuming
> > > an unexpectedly-NULL function pointer.

Ok, sure.

> > As in my reply to patch 0/5, I suggested that static_call_update(NULL)
> > would trigger a WARN_ON() always. Then this could be cleaned up and still
> > get that warning.
> 
> I don't think that provides the functionality KVM wants/needs.  KVM only disallows
> NULL updates for select mandatory hooks.  For optional hooks, KVM needs to support
> NULL updates in some capacity to handle the scenario where a vendor module is
> reloaded with different settings, e.g. loading kvm_intel with enable_apicv=0 after
> running with enable_apicv=1.
> 
> WARN_ON() a static_call_update(..., NULL) should be ok, but I believe KVM would
> still need/want macro shenanigans, e.g.
> 
> #define __KVM_X86_OP(func) \
> 	static_call_update(kvm_x86_##func,
> 			   kvm_x86_ops.func ? kvm_x86_ops.func : STATIC_CALL_NOP);
> #define KVM_X86_OP(func) \
> 	WARN_ON(!kvm_x86_ops.func); __KVM_X86_OP(func)
> #define KVM_X86_OP_OPTIONAL __KVM_X86_OP
> #define KVM_X86_OP_OPTIONAL_RET0(func) __KVM_X86_OP

Yeah, something like that might be ok, if we just refuse NULL as an
option.

If only Peter hadn't ruined my Friday with the CFI talk.

-- 
Josh