RE: [EXTERNAL] Re: [PATCH] x86/ioapic: Don't return 0 as valid virq

[Saurabh Singh Sengar]

Cc: rahul.tanwar@xxxxxxxxxxxxxxx, andriy.shevchenko@xxxxxxxxx

Thanks for you comments, please see my responses below.

> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Monday, March 13, 2023 2:10 AM
> To: Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>
> Cc: tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx;
> x86@xxxxxxxxxx; hpa@xxxxxxxxx; johan+linaro@xxxxxxxxxx;
> isaku.yamahata@xxxxxxxxx; Michael Kelley (LINUX)
> <mikelley@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [EXTERNAL] Re: [PATCH] x86/ioapic: Don't return 0 as valid virq
> 
> On Thu, Mar 02, 2023 at 11:34:46AM -0800, Saurabh Sengar wrote:
> > Zero is invalid virq and should't be returned as a valid value for
> > lower irq bound. If IO-APIC and gsi_top are not initialized return
> 
> Why isn't gsi_top initialized?
> 
> What is this fixing?

In the absence of a device tree node for IO-APIC,  IO-APIC is not registered,
resulting in uninitialized gsi_top. And in such cases arch_dynirq_lower_bound
will return 0. Returning 0 from this function will allow interrupts to have 0
assigned as valid irq, which is wrong. In case gsi_top is 0, lower bound of irq
should be derived from 'hint' value passed to function as 'from'.

I can add above info in commit message,  please let me know if anything
more to be added.

To be specific in our system which is a guest VM we don't need IO-APIC and hence
there is no device tree node for it. It is observed that we get irq 0 assigned to PCI-MSI.

> 
> Don't be afraid to do
> 
> git annotate arch/x86/kernel/apic/io_apic.c
> 
> and see which commit added this. This one:
> 
> 3e5bedc2c258 ("x86/apic: Fix arch_dynirq_lower_bound() bug for DT enabled
> machines")
> 
> Now add the folks from this commit to Cc and tell them why in your case
> gsi_top is not initialized and what they're breaking by doing that.

Thanks. I will add "Fixes:" and "Cc:" tag in next version.

> 
> The more your commit message explains *why* you're fixing something, the
> better it is for the maintainers/reviewers to actually know what to do.
> 
> Right now I'm reading this and I'm thinking, random, unjustified change.
> Ignore.
> 
> Ok?
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeopl
> e.kernel.org%2Ftglx%2Fnotes-about-
> netiquette&data=05%7C01%7Cssengar%40microsoft.com%7C6e1e0e21051c4
> 9c1cfe008db233a0376%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%
> 7C638142504360574969%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=OLdgb1AuLbLvlzucgNFBQEEK6G%2FsFV%2BO2TqT%2FNCujJU%3
> D&reserved=0