| Message ID | 20240405223110.1609888-12-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Coalesced Interrupt Delivery with posted MSI | expand |
On 4/6/2024 6:31 AM, Jacob Pan wrote: > Add a command line opt-in option for posted MSI if CONFIG_X86_POSTED_MSI=y. > > Also introduce a helper function for testing if posted MSI is supported on > the platform. > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > --- > Documentation/admin-guide/kernel-parameters.txt | 1 + > arch/x86/include/asm/irq_remapping.h | 11 +++++++++++ > drivers/iommu/irq_remapping.c | 13 ++++++++++++- > 3 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index bb884c14b2f6..e5fd02423c4c 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2251,6 +2251,7 @@ > no_x2apic_optout > BIOS x2APIC opt-out request will be ignored > nopost disable Interrupt Posting > + posted_msi enable MSIs delivered as posted interrupts > > iomem= Disable strict checking of access to MMIO memory > strict regions from userspace. > diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h > index 7a2ed154a5e1..e46bde61029b 100644 > --- a/arch/x86/include/asm/irq_remapping.h > +++ b/arch/x86/include/asm/irq_remapping.h > @@ -50,6 +50,17 @@ static inline struct irq_domain *arch_get_ir_parent_domain(void) > return x86_vector_domain; > } > > +#ifdef CONFIG_X86_POSTED_MSI > +extern int enable_posted_msi; > + > +static inline bool posted_msi_supported(void) > +{ > + return enable_posted_msi && irq_remapping_cap(IRQ_POSTING_CAP); > +} Out of this patch set's scope, but, dropping into irq_remappping_cap(), I'd like to bring this change for discussion: diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c index 4047ac396728..ef2de9034897 100644 --- a/drivers/iommu/irq_remapping.c +++ b/drivers/iommu/irq_remapping.c @@ -98,7 +98,7 @@ void set_irq_remapping_broken(void) bool irq_remapping_cap(enum irq_remap_cap cap) { - if (!remap_ops || disable_irq_post) + if (!remap_ops || disable_irq_remap) return false; return (remap_ops->capability & (1 << cap)); 1. irq_remapping_cap() is to exam some cap, though at present it has only 1 cap, i.e. IRQ_POSTING_CAP, simply return false just because of disable_irq_post isn't good. Instead, IRQ_REMAP is the foundation of all remapping caps. 2. disable_irq_post is used by Intel iommu code only, here irq_remapping_cap() is common code. e.g. AMD iommu code doesn't use it to judge set cap of irq_post or not. > +#else > +static inline bool posted_msi_supported(void) { return false; }; > +#endif
Hi Robert, On Sat, 6 Apr 2024 12:31:14 +0800, Robert Hoo <robert.hoo.linux@gmail.com> wrote: > On 4/6/2024 6:31 AM, Jacob Pan wrote: > > Add a command line opt-in option for posted MSI if > > CONFIG_X86_POSTED_MSI=y. > > > > Also introduce a helper function for testing if posted MSI is supported > > on the platform. > > > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> > > --- > > Documentation/admin-guide/kernel-parameters.txt | 1 + > > arch/x86/include/asm/irq_remapping.h | 11 +++++++++++ > > drivers/iommu/irq_remapping.c | 13 ++++++++++++- > > 3 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > > b/Documentation/admin-guide/kernel-parameters.txt index > > bb884c14b2f6..e5fd02423c4c 100644 --- > > a/Documentation/admin-guide/kernel-parameters.txt +++ > > b/Documentation/admin-guide/kernel-parameters.txt @@ -2251,6 +2251,7 @@ > > no_x2apic_optout > > BIOS x2APIC opt-out request will be > > ignored nopost disable Interrupt Posting > > + posted_msi enable MSIs delivered as posted > > interrupts > > iomem= Disable strict checking of access to > > MMIO memory strict regions from userspace. > > diff --git a/arch/x86/include/asm/irq_remapping.h > > b/arch/x86/include/asm/irq_remapping.h index 7a2ed154a5e1..e46bde61029b > > 100644 --- a/arch/x86/include/asm/irq_remapping.h > > +++ b/arch/x86/include/asm/irq_remapping.h > > @@ -50,6 +50,17 @@ static inline struct irq_domain > > *arch_get_ir_parent_domain(void) return x86_vector_domain; > > } > > > > +#ifdef CONFIG_X86_POSTED_MSI > > +extern int enable_posted_msi; > > + > > +static inline bool posted_msi_supported(void) > > +{ > > + return enable_posted_msi && irq_remapping_cap(IRQ_POSTING_CAP); > > +} > > Out of this patch set's scope, but, dropping into irq_remappping_cap(), > I'd like to bring this change for discussion: > > diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c > index 4047ac396728..ef2de9034897 100644 > --- a/drivers/iommu/irq_remapping.c > +++ b/drivers/iommu/irq_remapping.c > @@ -98,7 +98,7 @@ void set_irq_remapping_broken(void) > > bool irq_remapping_cap(enum irq_remap_cap cap) > { > - if (!remap_ops || disable_irq_post) > + if (!remap_ops || disable_irq_remap) > return false; > > return (remap_ops->capability & (1 << cap)); > > > 1. irq_remapping_cap() is to exam some cap, though at present it has only > 1 cap, i.e. IRQ_POSTING_CAP, simply return false just because of > disable_irq_post isn't good. Instead, IRQ_REMAP is the foundation of all > remapping caps. 2. disable_irq_post is used by Intel iommu code only, > here irq_remapping_cap() is common code. e.g. AMD iommu code doesn't use > it to judge set cap of irq_post or not. I agree, posting should be treated as a sub-capability of remapping. IRQ_POSTING_CAP is only set when remapping is on. We need to delete this such that posting is always off when remapping is off. --- a/drivers/iommu/intel/irq_remapping.c +++ b/drivers/iommu/intel/irq_remapping.c @@ -1038,11 +1038,7 @@ static void disable_irq_remapping(void) iommu_disable_irq_remapping(iommu); } - /* - * Clear Posted-Interrupts capability. - */ - if (!disable_irq_post) - intel_irq_remap_ops.capability &= ~(1 << IRQ_POSTING_CAP); + intel_irq_remap_ops.capability &= ~(1 << IRQ_POSTING_CAP); } > > +#else > > +static inline bool posted_msi_supported(void) { return false; }; > > +#endif > Thanks, Jacob
> From: Jacob Pan <jacob.jun.pan@linux.intel.com> > Sent: Saturday, April 6, 2024 6:31 AM > > +#ifdef CONFIG_X86_POSTED_MSI > + else if (!strncmp(str, "posted_msi", 10)) { > + if (disable_irq_post || disable_irq_remap) > + pr_warn("Posted MSI not enabled due to > conflicting options!"); > + else > + enable_posted_msi = 1; > + } > +#endif the check of disable_irq_remap is unnecessary. It's unlikely to have a configuration with disable_irq_post=0 while disable_irq_remap=1 given the latter has bigger scope. but thinking more do we really need a check here? there is no order guarantee that "posted_msi" is parsed after the parameters deciding the value of two disable variables. it probably makes more sense to just set enable_posted_msi here and then do all required checks when picking up the irqchip in intel_irq_remapping_alloc().
On 4/9/2024 7:33 AM, Jacob Pan wrote: > Hi Robert, > > On Sat, 6 Apr 2024 12:31:14 +0800, Robert Hoo <robert.hoo.linux@gmail.com> > wrote: > >> On 4/6/2024 6:31 AM, Jacob Pan wrote: >>> Add a command line opt-in option for posted MSI if >>> CONFIG_X86_POSTED_MSI=y. >>> >>> Also introduce a helper function for testing if posted MSI is supported >>> on the platform. >>> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> >>> --- >>> Documentation/admin-guide/kernel-parameters.txt | 1 + >>> arch/x86/include/asm/irq_remapping.h | 11 +++++++++++ >>> drivers/iommu/irq_remapping.c | 13 ++++++++++++- >>> 3 files changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/admin-guide/kernel-parameters.txt >>> b/Documentation/admin-guide/kernel-parameters.txt index >>> bb884c14b2f6..e5fd02423c4c 100644 --- >>> a/Documentation/admin-guide/kernel-parameters.txt +++ >>> b/Documentation/admin-guide/kernel-parameters.txt @@ -2251,6 +2251,7 @@ >>> no_x2apic_optout >>> BIOS x2APIC opt-out request will be >>> ignored nopost disable Interrupt Posting >>> + posted_msi enable MSIs delivered as posted >>> interrupts >>> iomem= Disable strict checking of access to >>> MMIO memory strict regions from userspace. >>> diff --git a/arch/x86/include/asm/irq_remapping.h >>> b/arch/x86/include/asm/irq_remapping.h index 7a2ed154a5e1..e46bde61029b >>> 100644 --- a/arch/x86/include/asm/irq_remapping.h >>> +++ b/arch/x86/include/asm/irq_remapping.h >>> @@ -50,6 +50,17 @@ static inline struct irq_domain >>> *arch_get_ir_parent_domain(void) return x86_vector_domain; >>> } >>> >>> +#ifdef CONFIG_X86_POSTED_MSI >>> +extern int enable_posted_msi; >>> + >>> +static inline bool posted_msi_supported(void) >>> +{ >>> + return enable_posted_msi && irq_remapping_cap(IRQ_POSTING_CAP); >>> +} >> >> Out of this patch set's scope, but, dropping into irq_remappping_cap(), >> I'd like to bring this change for discussion: >> >> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c >> index 4047ac396728..ef2de9034897 100644 >> --- a/drivers/iommu/irq_remapping.c >> +++ b/drivers/iommu/irq_remapping.c >> @@ -98,7 +98,7 @@ void set_irq_remapping_broken(void) >> >> bool irq_remapping_cap(enum irq_remap_cap cap) >> { >> - if (!remap_ops || disable_irq_post) >> + if (!remap_ops || disable_irq_remap) >> return false; >> >> return (remap_ops->capability & (1 << cap)); >> >> >> 1. irq_remapping_cap() is to exam some cap, though at present it has only >> 1 cap, i.e. IRQ_POSTING_CAP, simply return false just because of >> disable_irq_post isn't good. Instead, IRQ_REMAP is the foundation of all >> remapping caps. 2. disable_irq_post is used by Intel iommu code only, >> here irq_remapping_cap() is common code. e.g. AMD iommu code doesn't use >> it to judge set cap of irq_post or not. > I agree, posting should be treated as a sub-capability of remapping. > IRQ_POSTING_CAP is only set when remapping is on. > > We need to delete this such that posting is always off when remapping is > off. > > --- a/drivers/iommu/intel/irq_remapping.c > +++ b/drivers/iommu/intel/irq_remapping.c > @@ -1038,11 +1038,7 @@ static void disable_irq_remapping(void) > iommu_disable_irq_remapping(iommu); > } > > - /* > - * Clear Posted-Interrupts capability. > - */ > - if (!disable_irq_post) > - intel_irq_remap_ops.capability &= ~(1 << IRQ_POSTING_CAP); > + intel_irq_remap_ops.capability &= ~(1 << IRQ_POSTING_CAP); > } > Right.
Hi Kevin, On Fri, 12 Apr 2024 09:31:32 +0000, "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Jacob Pan <jacob.jun.pan@linux.intel.com> > > Sent: Saturday, April 6, 2024 6:31 AM > > > > +#ifdef CONFIG_X86_POSTED_MSI > > + else if (!strncmp(str, "posted_msi", 10)) { > > + if (disable_irq_post || disable_irq_remap) > > + pr_warn("Posted MSI not enabled due to > > conflicting options!"); > > + else > > + enable_posted_msi = 1; > > + } > > +#endif > > the check of disable_irq_remap is unnecessary. It's unlikely to have > a configuration with disable_irq_post=0 while disable_irq_remap=1 > given the latter has bigger scope. > > but thinking more do we really need a check here? there is no order > guarantee that "posted_msi" is parsed after the parameters deciding > the value of two disable variables. > > it probably makes more sense to just set enable_posted_msi here > and then do all required checks when picking up the irqchip in > intel_irq_remapping_alloc(). Makes sense, I have a helper function posted_msi_supported() called in intel_irq_remapping_alloc() already. My intention was to alert negligent users, but is is not really necessary as you said. Thanks, Jacob
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index bb884c14b2f6..e5fd02423c4c 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -2251,6 +2251,7 @@ no_x2apic_optout BIOS x2APIC opt-out request will be ignored nopost disable Interrupt Posting + posted_msi enable MSIs delivered as posted interrupts iomem= Disable strict checking of access to MMIO memory strict regions from userspace. diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h index 7a2ed154a5e1..e46bde61029b 100644 --- a/arch/x86/include/asm/irq_remapping.h +++ b/arch/x86/include/asm/irq_remapping.h @@ -50,6 +50,17 @@ static inline struct irq_domain *arch_get_ir_parent_domain(void) return x86_vector_domain; } +#ifdef CONFIG_X86_POSTED_MSI +extern int enable_posted_msi; + +static inline bool posted_msi_supported(void) +{ + return enable_posted_msi && irq_remapping_cap(IRQ_POSTING_CAP); +} +#else +static inline bool posted_msi_supported(void) { return false; }; +#endif + #else /* CONFIG_IRQ_REMAP */ static inline bool irq_remapping_cap(enum irq_remap_cap cap) { return 0; } diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c index ee59647c2050..5672783c9f1f 100644 --- a/drivers/iommu/irq_remapping.c +++ b/drivers/iommu/irq_remapping.c @@ -24,6 +24,10 @@ int no_x2apic_optout; int disable_irq_post = 0; +#ifdef CONFIG_X86_POSTED_MSI +int enable_posted_msi; +#endif + static int disable_irq_remap; static struct irq_remap_ops *remap_ops; @@ -70,7 +74,14 @@ static __init int setup_irqremap(char *str) no_x2apic_optout = 1; else if (!strncmp(str, "nopost", 6)) disable_irq_post = 1; - +#ifdef CONFIG_X86_POSTED_MSI + else if (!strncmp(str, "posted_msi", 10)) { + if (disable_irq_post || disable_irq_remap) + pr_warn("Posted MSI not enabled due to conflicting options!"); + else + enable_posted_msi = 1; + } +#endif str += strcspn(str, ","); while (*str == ',') str++;
Add a command line opt-in option for posted MSI if CONFIG_X86_POSTED_MSI=y. Also introduce a helper function for testing if posted MSI is supported on the platform. Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- Documentation/admin-guide/kernel-parameters.txt | 1 + arch/x86/include/asm/irq_remapping.h | 11 +++++++++++ drivers/iommu/irq_remapping.c | 13 ++++++++++++- 3 files changed, 24 insertions(+), 1 deletion(-)