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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z+ElLSmJHkBqDPIT@intel.com>
Date: Mon, 24 Mar 2025 17:26:05 +0800
From: Chao Gao <chao.gao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, Yosry Ahmed <yosry.ahmed@...ux.dev>
Subject: Re: [PATCH v2 3/3] KVM: x86: Add a module param to control and
 enumerate device posted IRQs

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.

>address that by having the variable incorporate irq_remapping_cap(IRQ_POSTING_CAP)
>but not enable_apicv, but that's still a bit funky.
>
>Given that enable_apicv already has the "variable in kvm.ko, module param in
>kvm-{amd,intel}.ko" behavior, and that I am planning on giving enable_ipiv the
>same treatment (long story), my strong vote is to go with option #2 as it's the
>most flexibile, most accurate, and consistent with existing knobs.