Re: [PATCH v2 3/3] KVM: x86: Add a module param to control and enumerate device posted IRQs

[Sean Christopherson <seanjc@google.com>]

On Mon, Mar 24, 2025, Chao Gao wrote:
> On Fri, Mar 21, 2025 at 01:44:47PM -0700, Sean Christopherson wrote:
> >On Fri, Mar 21, 2025, Chao Gao wrote:
> >> On Thu, Mar 20, 2025 at 10:59:19AM -0700, Sean Christopherson wrote:
> >> >@@ -9776,8 +9777,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
> >> >        if (r != 0)
> >> >                goto out_mmu_exit;
> >> > 
> >> >-       enable_device_posted_irqs &= enable_apicv &&
> >> >-                                    irq_remapping_cap(IRQ_POSTING_CAP);
> >> >+       enable_device_posted_irqs = allow_device_posted_irqs && enable_apicv &&
> >> >+                                   irq_remapping_cap(IRQ_POSTING_CAP);
> >> 
> >> Can we simply drop this ...
> >> 
> >> > 
> >> >        kvm_ops_update(ops);
> >> > 
> >> >@@ -14033,6 +14034,8 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_rmp_fault);
> >> > 
> >> > static int __init kvm_x86_init(void)
> >> > {
> >> >+       allow_device_posted_irqs = enable_device_posted_irqs;
> >> >+
> >> >        kvm_init_xstate_sizes();
> >> > 
> >> >        kvm_mmu_x86_module_init();
> >> >
> >> >
> >> >Option #2 is to shove the module param into vendor code, but leave the variable
> >> >in kvm.ko, like we do for enable_apicv.
> >> >
> >> >I'm leaning toward option #2, as it's more flexible, arguably more intuitive, and
> >> >doesn't prevent putting the logic in kvm_x86_vendor_init().
> >> >
> >> 
> >> and do
> >> 
> >> bool kvm_arch_has_irq_bypass(void)
> >> {
> >> 	return enable_device_posted_irqs && enable_apicv &&
> >> 	       irq_remapping_cap(IRQ_POSTING_CAP);
> >> }
> >
> >That would avoid the vendor module issues, but it would result in
> >allow_device_posted_irqs not reflecting the state of KVM.  We could partially
> 
> Ok. I missed that.
> 
> btw, is using module_param_cb() a bad idea? like:
> 
> module_param_cb(nx_huge_pages, &nx_huge_pages_ops, &nx_huge_pages, 0644);
> 
> with a proper .get callback, we can reflect the state of KVM to userspace
> accurately.

It's not a bad idea, but it comes with tradeoffs too.  A little bit more code,
but more importantly enable_device_posted_irqs wouldn't reflect KVM's internal
state, which could result in bugs if KVM were to check the module param directly.
I don't think that'd be likely to happen, but given that pretty much every other
"simple" param in KVM reflects KVM's state directly, it'd be an easy mistake to
make.

That, and being able to set toggle the param when reloading the vendor module is
actually valuable, as there are setups where kvm.ko is built-in, but the vendor
modules are not.