[v2,1/2] x86/of: Add support for boot time interrupt delivery mode configuration

  Presently, init/boot time interrupt delivery mode is enumerated
only for ACPI enabled systems by parsing MADT table or for older
systems by parsing MP table. But for OF based x86 systems, it is
assumed & hardcoded to legacy PIC mode. This is a bug for
platforms which are OF based but do not use 8259 compliant legacy
PIC interrupt controller. Such platforms can not even boot because
of this bug/hardcoding.

Fix this bug by adding support for configuration of init time
interrupt delivery mode for x86 OF based systems by introducing a
new optional boolean property 'intel,virtual-wire-mode' for
interrupt-controller node of local APIC. This property emulates
IMCRP Bit 7 of MP feature info byte 2 of MP floating pointer
structure [1].

Defaults to legacy PIC mode if absent. Configures it to virtual
wire compatibility mode if present.

[1] https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual

Fixes: 3879a6f329483 ("x86: dtb: Add early parsing of IO_APIC")
Signed-off-by: Rahul Tanwar <rtanwar@maxlinear.com>
---
 arch/x86/kernel/devicetree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
  

On Wed, Nov 16, 2022 at 06:28:20PM +0800, Rahul Tanwar wrote:
> Presently, init/boot time interrupt delivery mode is enumerated
> only for ACPI enabled systems by parsing MADT table or for older
> systems by parsing MP table. But for OF based x86 systems, it is
> assumed & hardcoded to legacy PIC mode. This is a bug for
> platforms which are OF based but do not use 8259 compliant legacy
> PIC interrupt controller. Such platforms can not even boot because
> of this bug/hardcoding.
> 
> Fix this bug by adding support for configuration of init time
> interrupt delivery mode for x86 OF based systems by introducing a
> new optional boolean property 'intel,virtual-wire-mode' for
> interrupt-controller node of local APIC. This property emulates
> IMCRP Bit 7 of MP feature info byte 2 of MP floating pointer
> structure [1].
> 
> Defaults to legacy PIC mode if absent. Configures it to virtual
> wire compatibility mode if present.

> [1] https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual

Link: ?

...

> +	if (of_property_read_bool(dn, "intel,virtual-wire-mode")) {

You need a separate patch to show this property being added (yes,
I have just commented on your patch 2).

> +		printk(KERN_NOTICE "Virtual Wire compatibility mode.\n");
> +		pic_mode = 0;
> +	} else {
> +		printk(KERN_NOTICE "IMCR and PIC compatibility mode.\n");
> +		pic_mode = 1;

Why not pr_notice()  in both cases?

> +	}
  

On 16/11/2022 6:42 pm, Andy Shevchenko wrote:
> This email was sent from outside of MaxLinear.
> 
> On Wed, Nov 16, 2022 at 06:28:20PM +0800, Rahul Tanwar wrote:
>  > Presently, init/boot time interrupt delivery mode is enumerated
>  > only for ACPI enabled systems by parsing MADT table or for older
>  > systems by parsing MP table. But for OF based x86 systems, it is
>  > assumed & hardcoded to legacy PIC mode. This is a bug for
>  > platforms which are OF based but do not use 8259 compliant legacy
>  > PIC interrupt controller. Such platforms can not even boot because
>  > of this bug/hardcoding.
>  >
>  > Fix this bug by adding support for configuration of init time
>  > interrupt delivery mode for x86 OF based systems by introducing a
>  > new optional boolean property 'intel,virtual-wire-mode' for
>  > interrupt-controller node of local APIC. This property emulates
>  > IMCRP Bit 7 of MP feature info byte 2 of MP floating pointer
>  > structure [1].
>  >
>  > Defaults to legacy PIC mode if absent. Configures it to virtual
>  > wire compatibility mode if present.
> 
>  > [1] 
> https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual <https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual>
> 
> Link: ?


Rechecked. Link URL works for me. Am i missing any standard practice to 
add URL links in commit log ?



> 
> ...
> 
>  > + if (of_property_read_bool(dn, "intel,virtual-wire-mode")) {
> 
> You need a separate patch to show this property being added (yes,
> I have just commented on your patch 2).
> 
>  > + printk(KERN_NOTICE "Virtual Wire compatibility mode.\n");
>  > + pic_mode = 0;
>  > + } else {
>  > + printk(KERN_NOTICE "IMCR and PIC compatibility mode.\n");
>  > + pic_mode = 1;
> 
> Why not pr_notice() in both cases?

Reset of the file uses printk(KERN_xxx ""). In v1, i used pr_notice() 
but on reviewing again found it to be odd one out in the file. So 
switched to printk(KERN_xxx ""). I can revert back to using pr_notice() 
if you think that's a better fit. Thanks.

Regards,
Rahul

> 
>  > + }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>
  

On 16/11/2022 6:42 pm, Andy Shevchenko wrote:
> This email was sent from outside of MaxLinear.
> 
> On Wed, Nov 16, 2022 at 06:28:20PM +0800, Rahul Tanwar wrote:
>  > Presently, init/boot time interrupt delivery mode is enumerated
>  > only for ACPI enabled systems by parsing MADT table or for older
>  > systems by parsing MP table. But for OF based x86 systems, it is
>  > assumed & hardcoded to legacy PIC mode. This is a bug for
>  > platforms which are OF based but do not use 8259 compliant legacy
>  > PIC interrupt controller. Such platforms can not even boot because
>  > of this bug/hardcoding.
>  >
>  > Fix this bug by adding support for configuration of init time
>  > interrupt delivery mode for x86 OF based systems by introducing a
>  > new optional boolean property 'intel,virtual-wire-mode' for
>  > interrupt-controller node of local APIC. This property emulates
>  > IMCRP Bit 7 of MP feature info byte 2 of MP floating pointer
>  > structure [1].
>  >
>  > Defaults to legacy PIC mode if absent. Configures it to virtual
>  > wire compatibility mode if present.
> 
>  > [1] 
> https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual <https://www.manualslib.com/manual/77733/Intel-Multiprocessor.html?page=40#manual>
> 
> Link: ?
> 
> ...
> 
>  > + if (of_property_read_bool(dn, "intel,virtual-wire-mode")) {
> 
> You need a separate patch to show this property being added (yes,
> I have just commented on your patch 2).
>

Well noted about it. Will update. Thanks.

Regards,
Rahul


>  > + printk(KERN_NOTICE "Virtual Wire compatibility mode.\n");
>  > + pic_mode = 0;
>  > + } else {
>  > + printk(KERN_NOTICE "IMCR and PIC compatibility mode.\n");
>  > + pic_mode = 1;
> 
> Why not pr_notice() in both cases?
> 
>  > + }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>
  

On Wed, Nov 16, 2022 at 11:25:47AM +0000, Rahul Tanwar wrote:
> On 16/11/2022 6:42 pm, Andy Shevchenko wrote:
> > On Wed, Nov 16, 2022 at 06:28:20PM +0800, Rahul Tanwar wrote:

...

> > Why not pr_notice() in both cases?
> 
> Reset of the file uses printk(KERN_xxx ""). In v1, i used pr_notice() 
> but on reviewing again found it to be odd one out in the file. So 
> switched to printk(KERN_xxx ""). I can revert back to using pr_notice() 
> if you think that's a better fit. Thanks.

I don;t know why we should use antique style of printing APIs in new patches.
Even if the old code uses that, you can create a followup that can do two
things:
- uses pr_lvl() instead of printk(KERN_LVL)
- keeps string literals unbroken between the lines (if any
  of such breakage exists)