| Message ID | 20231112041643.2868316-2-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coalesced Interrupt Delivery with posted MSI | expand |
On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote: > +/* Posted-Interrupt Descriptor */ > +struct pi_desc { > + u32 pir[8]; /* Posted interrupt requested */ > + union { > + struct { > + /* bit 256 - Outstanding Notification */ > + u16 on : 1, > + /* bit 257 - Suppress Notification */ > + sn : 1, > + /* bit 271:258 - Reserved */ > + rsvd_1 : 14; > + /* bit 279:272 - Notification Vector */ > + u8 nv; > + /* bit 287:280 - Reserved */ > + u8 rsvd_2; > + /* bit 319:288 - Notification Destination */ > + u32 ndst; This mixture of bitfields and types is weird and really not intuitive: /* Posted-Interrupt Descriptor */ struct pi_desc { /* Posted interrupt requested */ u32 pir[8]; union { struct { /* bit 256 - Outstanding Notification */ u64 on : 1, /* bit 257 - Suppress Notification */ sn : 1, /* bit 271:258 - Reserved */ : 14, /* bit 279:272 - Notification Vector */ nv : 8, /* bit 287:280 - Reserved */ : 8, /* bit 319:288 - Notification Destination */ ndst : 32; }; u64 control; }; u32 rsvd[6]; } __aligned(64); Hmm? > +static inline bool pi_test_and_set_on(struct pi_desc *pi_desc) > +{ > + return test_and_set_bit(POSTED_INTR_ON, > + (unsigned long *)&pi_desc->control); Please get rid of those line breaks. Thanks, tglx
Hi Thomas, On Wed, 06 Dec 2023 17:33:28 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote: > > +/* Posted-Interrupt Descriptor */ > > +struct pi_desc { > > + u32 pir[8]; /* Posted interrupt requested */ > > + union { > > + struct { > > + /* bit 256 - Outstanding Notification > > */ > > + u16 on : 1, > > + /* bit 257 - Suppress Notification */ > > + sn : 1, > > + /* bit 271:258 - Reserved */ > > + rsvd_1 : 14; > > + /* bit 279:272 - Notification Vector */ > > + u8 nv; > > + /* bit 287:280 - Reserved */ > > + u8 rsvd_2; > > + /* bit 319:288 - Notification > > Destination */ > > + u32 ndst; > > This mixture of bitfields and types is weird and really not intuitive: > > /* Posted-Interrupt Descriptor */ > struct pi_desc { > /* Posted interrupt requested */ > u32 pir[8]; > > union { > struct { > /* bit 256 - Outstanding Notification */ > u64 on : 1, > /* bit 257 - Suppress Notification */ > sn : 1, > /* bit 271:258 - Reserved */ > : 14, > /* bit 279:272 - Notification Vector */ > nv : 8, > /* bit 287:280 - Reserved */ > : 8, > /* bit 319:288 - Notification Destination > */ ndst : 32; > }; > u64 control; > }; > u32 rsvd[6]; > } __aligned(64); > It seems bit-fields cannot pass type check. I got these compile error. arch/x86/kernel/irq.c: In function ‘intel_posted_msi_init’: ./include/linux/percpu-defs.h:363:20: error: cannot take address of bit-field ‘nv’ 363 | __verify_pcpu_ptr(&(variable)); \ | ^ ./include/linux/percpu-defs.h:219:47: note: in definition of macro ‘__verify_pcpu_ptr’ 219 | const void __percpu *__vpp_verify = (typeof((ptr) + 0))NULL; \ | ^~~ ./include/linux/percpu-defs.h:490:34: note: in expansion of macro ‘__pcpu_size_call’ 490 | #define this_cpu_write(pcp, val) __pcpu_size_call(this_cpu_write_, pcp, val) | ^~~~~~~~~~~~~~~~ arch/x86/kernel/irq.c:358:2: note: in expansion of macro ‘this_cpu_write’ 358 | this_cpu_write(posted_interrupt_desc.nv, POSTED_MSI_NOTIFICATION_VECTOR); | ^~~~~~~~~~~~~~ ./include/linux/percpu-defs.h:364:15: error: ‘sizeof’ applied to a bit-field 364 | switch(sizeof(variable)) { \ > > > +static inline bool pi_test_and_set_on(struct pi_desc *pi_desc) > > +{ > > + return test_and_set_bit(POSTED_INTR_ON, > > + (unsigned long *)&pi_desc->control); > > Please get rid of those line breaks. will do. Thanks, Jacob
On Thu, Dec 07 2023 at 20:54, Jacob Pan wrote: > On Wed, 06 Dec 2023 17:33:28 +0100, Thomas Gleixner <tglx@linutronix.de> > wrote: >> On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote: >> u32 rsvd[6]; >> } __aligned(64); >> > It seems bit-fields cannot pass type check. I got these compile error. And why are you telling me that instead if simply fixing it?
Hi Thomas, On Fri, 08 Dec 2023 10:31:20 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, Dec 07 2023 at 20:54, Jacob Pan wrote: > > On Wed, 06 Dec 2023 17:33:28 +0100, Thomas Gleixner <tglx@linutronix.de> > > wrote: > >> On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote: > >> u32 rsvd[6]; > >> } __aligned(64); > >> > > It seems bit-fields cannot pass type check. I got these compile error. > > And why are you telling me that instead if simply fixing it? My point is that I am not sure this change is worthwhile unless I don't do the per CPU pointer check. gcc cannot take bit-field address afaik. So the problem is that with this bit-field change we don't have individual members anymore to check pointers. e.g. ./include/linux/percpu-defs.h:363:20: error: cannot take address of bit-field ‘nv’ 363 | __verify_pcpu_ptr(&(variable)); Thanks, Jacob
Hi Thomas, On Fri, 08 Dec 2023 10:31:20 +0100, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, Dec 07 2023 at 20:54, Jacob Pan wrote: > > On Wed, 06 Dec 2023 17:33:28 +0100, Thomas Gleixner <tglx@linutronix.de> > > wrote: > >> On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote: > >> u32 rsvd[6]; > >> } __aligned(64); > >> > > It seems bit-fields cannot pass type check. I got these compile error. > > And why are you telling me that instead if simply fixing it? I guess we can fix it like this to use the new bitfields: void intel_posted_msi_init(void) { - this_cpu_write(posted_interrupt_desc.nv, POSTED_MSI_NOTIFICATION_VECTOR); - this_cpu_write(posted_interrupt_desc.ndst, this_cpu_read(x86_cpu_to_apicid)); + struct pi_desc *pid = this_cpu_ptr(&posted_interrupt_desc); + + pid->nv = POSTED_MSI_NOTIFICATION_VECTOR; + pid->ndst = this_cpu_read(x86_cpu_to_apicid); It is init time, no IOMMU posting yet. So no need for atomics. Thanks, Jacob
diff --git a/arch/x86/include/asm/posted_intr.h b/arch/x86/include/asm/posted_intr.h new file mode 100644 index 000000000000..9f2fa38fa57b --- /dev/null +++ b/arch/x86/include/asm/posted_intr.h @@ -0,0 +1,97 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _X86_POSTED_INTR_H +#define _X86_POSTED_INTR_H + +#define POSTED_INTR_ON 0 +#define POSTED_INTR_SN 1 + +#define PID_TABLE_ENTRY_VALID 1 + +/* Posted-Interrupt Descriptor */ +struct pi_desc { + u32 pir[8]; /* Posted interrupt requested */ + union { + struct { + /* bit 256 - Outstanding Notification */ + u16 on : 1, + /* bit 257 - Suppress Notification */ + sn : 1, + /* bit 271:258 - Reserved */ + rsvd_1 : 14; + /* bit 279:272 - Notification Vector */ + u8 nv; + /* bit 287:280 - Reserved */ + u8 rsvd_2; + /* bit 319:288 - Notification Destination */ + u32 ndst; + }; + u64 control; + }; + u32 rsvd[6]; +} __aligned(64); + +static inline bool pi_test_and_set_on(struct pi_desc *pi_desc) +{ + return test_and_set_bit(POSTED_INTR_ON, + (unsigned long *)&pi_desc->control); +} + +static inline bool pi_test_and_clear_on(struct pi_desc *pi_desc) +{ + return test_and_clear_bit(POSTED_INTR_ON, + (unsigned long *)&pi_desc->control); +} + +static inline bool pi_test_and_clear_sn(struct pi_desc *pi_desc) +{ + return test_and_clear_bit(POSTED_INTR_SN, + (unsigned long *)&pi_desc->control); +} + +static inline bool pi_test_and_set_pir(int vector, struct pi_desc *pi_desc) +{ + return test_and_set_bit(vector, (unsigned long *)pi_desc->pir); +} + +static inline bool pi_is_pir_empty(struct pi_desc *pi_desc) +{ + return bitmap_empty((unsigned long *)pi_desc->pir, NR_VECTORS); +} + +static inline void pi_set_sn(struct pi_desc *pi_desc) +{ + set_bit(POSTED_INTR_SN, + (unsigned long *)&pi_desc->control); +} + +static inline void pi_set_on(struct pi_desc *pi_desc) +{ + set_bit(POSTED_INTR_ON, + (unsigned long *)&pi_desc->control); +} + +static inline void pi_clear_on(struct pi_desc *pi_desc) +{ + clear_bit(POSTED_INTR_ON, + (unsigned long *)&pi_desc->control); +} + +static inline void pi_clear_sn(struct pi_desc *pi_desc) +{ + clear_bit(POSTED_INTR_SN, + (unsigned long *)&pi_desc->control); +} + +static inline bool pi_test_on(struct pi_desc *pi_desc) +{ + return test_bit(POSTED_INTR_ON, + (unsigned long *)&pi_desc->control); +} + +static inline bool pi_test_sn(struct pi_desc *pi_desc) +{ + return test_bit(POSTED_INTR_SN, + (unsigned long *)&pi_desc->control); +} + +#endif /* _X86_POSTED_INTR_H */ diff --git a/arch/x86/kvm/vmx/posted_intr.h b/arch/x86/kvm/vmx/posted_intr.h index 26992076552e..6b2a0226257e 100644 --- a/arch/x86/kvm/vmx/posted_intr.h +++ b/arch/x86/kvm/vmx/posted_intr.h @@ -1,98 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0 */ #ifndef __KVM_X86_VMX_POSTED_INTR_H #define __KVM_X86_VMX_POSTED_INTR_H - -#define POSTED_INTR_ON 0 -#define POSTED_INTR_SN 1 - -#define PID_TABLE_ENTRY_VALID 1 - -/* Posted-Interrupt Descriptor */ -struct pi_desc { - u32 pir[8]; /* Posted interrupt requested */ - union { - struct { - /* bit 256 - Outstanding Notification */ - u16 on : 1, - /* bit 257 - Suppress Notification */ - sn : 1, - /* bit 271:258 - Reserved */ - rsvd_1 : 14; - /* bit 279:272 - Notification Vector */ - u8 nv; - /* bit 287:280 - Reserved */ - u8 rsvd_2; - /* bit 319:288 - Notification Destination */ - u32 ndst; - }; - u64 control; - }; - u32 rsvd[6]; -} __aligned(64); - -static inline bool pi_test_and_set_on(struct pi_desc *pi_desc) -{ - return test_and_set_bit(POSTED_INTR_ON, - (unsigned long *)&pi_desc->control); -} - -static inline bool pi_test_and_clear_on(struct pi_desc *pi_desc) -{ - return test_and_clear_bit(POSTED_INTR_ON, - (unsigned long *)&pi_desc->control); -} - -static inline bool pi_test_and_clear_sn(struct pi_desc *pi_desc) -{ - return test_and_clear_bit(POSTED_INTR_SN, - (unsigned long *)&pi_desc->control); -} - -static inline bool pi_test_and_set_pir(int vector, struct pi_desc *pi_desc) -{ - return test_and_set_bit(vector, (unsigned long *)pi_desc->pir); -} - -static inline bool pi_is_pir_empty(struct pi_desc *pi_desc) -{ - return bitmap_empty((unsigned long *)pi_desc->pir, NR_VECTORS); -} - -static inline void pi_set_sn(struct pi_desc *pi_desc) -{ - set_bit(POSTED_INTR_SN, - (unsigned long *)&pi_desc->control); -} - -static inline void pi_set_on(struct pi_desc *pi_desc) -{ - set_bit(POSTED_INTR_ON, - (unsigned long *)&pi_desc->control); -} - -static inline void pi_clear_on(struct pi_desc *pi_desc) -{ - clear_bit(POSTED_INTR_ON, - (unsigned long *)&pi_desc->control); -} - -static inline void pi_clear_sn(struct pi_desc *pi_desc) -{ - clear_bit(POSTED_INTR_SN, - (unsigned long *)&pi_desc->control); -} - -static inline bool pi_test_on(struct pi_desc *pi_desc) -{ - return test_bit(POSTED_INTR_ON, - (unsigned long *)&pi_desc->control); -} - -static inline bool pi_test_sn(struct pi_desc *pi_desc) -{ - return test_bit(POSTED_INTR_SN, - (unsigned long *)&pi_desc->control); -} +#include <asm/posted_intr.h> void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu); void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 72e3943f3693..d54fa0e06c70 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -66,6 +66,7 @@ #include "vmx.h" #include "x86.h" #include "smm.h" +#include "posted_intr.h" MODULE_AUTHOR("Qumranet"); MODULE_LICENSE("GPL"); diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index c2130d2c8e24..817b76794ee1 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -7,10 +7,10 @@ #include <asm/kvm.h> #include <asm/intel_pt.h> #include <asm/perf_event.h> +#include <asm/posted_intr.h> #include "capabilities.h" #include "../kvm_cache_regs.h" -#include "posted_intr.h" #include "vmcs.h" #include "vmx_ops.h" #include "../cpuid.h"
To prepare native usage of posted interrupt, move PID declaration out of VMX code such that they can be shared. Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- arch/x86/include/asm/posted_intr.h | 97 ++++++++++++++++++++++++++++++ arch/x86/kvm/vmx/posted_intr.h | 93 +--------------------------- arch/x86/kvm/vmx/vmx.c | 1 + arch/x86/kvm/vmx/vmx.h | 2 +- 4 files changed, 100 insertions(+), 93 deletions(-) create mode 100644 arch/x86/include/asm/posted_intr.h