Re: [RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp_setup()

[Dou Liyang]

Hi Thomas,

At 04/05/2017 07:56 PM, Thomas Gleixner wrote:

On Wed, 29 Mar 2017, Dou Liyang wrote:

+/* Setup local APIC timer and get the Id*/
+static int __init apic_bsp_timer_setup(void)


This does not make sense. The id and the timers have nothing to do with
each other.


Yes, Indeed. Here is more like the rest work for APIC setup, which
can't be executed earlier, The name is not suitable. I will refactor
it.


+{
+   int id;
+
+   if (x2apic_mode)
+   id = apic_read(APIC_LDR);
+   else
+   id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
+
+   if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
+   check_timer();


Why are you moving this to the APIC? check_timer() has absolutely nothing


Because, the jiffies is used in check_timer() for checking timer irq 
works(timer_irq_works()) and can't work at the beginning.


So, keep check_timer() in IOAPIC setup function (setup_IO_APIC()) which
has been moved to init_IRQ() will make the kernel not boot up.


to do with the apic timer. It's a IOAPIC only issue and required to test
that the IRQ0 interrupt delivery works through the IOAPIC.



Yes, I see, split check_timer() is not a good way, and move the APIC
initialization to the end of init_IRQ() is also not well.

Thanks,
Liyang


Thanks,

tglx

Re: [RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp_setup()

[Dou Liyang]

Hi Thomas,

At 04/05/2017 07:56 PM, Thomas Gleixner wrote:

On Wed, 29 Mar 2017, Dou Liyang wrote:

+/* Setup local APIC timer and get the Id*/
+static int __init apic_bsp_timer_setup(void)


This does not make sense. The id and the timers have nothing to do with
each other.


Yes, Indeed. Here is more like the rest work for APIC setup, which
can't be executed earlier, The name is not suitable. I will refactor
it.


+{
+   int id;
+
+   if (x2apic_mode)
+   id = apic_read(APIC_LDR);
+   else
+   id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
+
+   if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
+   check_timer();


Why are you moving this to the APIC? check_timer() has absolutely nothing


Because, the jiffies is used in check_timer() for checking timer irq 
works(timer_irq_works()) and can't work at the beginning.


So, keep check_timer() in IOAPIC setup function (setup_IO_APIC()) which
has been moved to init_IRQ() will make the kernel not boot up.


to do with the apic timer. It's a IOAPIC only issue and required to test
that the IRQ0 interrupt delivery works through the IOAPIC.



Yes, I see, split check_timer() is not a good way, and move the APIC
initialization to the end of init_IRQ() is also not well.

Thanks,
Liyang


Thanks,

tglx

Re: [RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp_setup()

[Thomas Gleixner]

On Wed, 29 Mar 2017, Dou Liyang wrote:
> +/* Setup local APIC timer and get the Id*/
> +static int __init apic_bsp_timer_setup(void)

This does not make sense. The id and the timers have nothing to do with
each other. 

> +{
> + int id;
> +
> + if (x2apic_mode)
> + id = apic_read(APIC_LDR);
> + else
> + id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
> +
> + if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
> + check_timer();

Why are you moving this to the APIC? check_timer() has absolutely nothing
to do with the apic timer. It's a IOAPIC only issue and required to test
that the IRQ0 interrupt delivery works through the IOAPIC.

Thanks,

tglx

Re: [RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp_setup()

[Thomas Gleixner]

On Wed, 29 Mar 2017, Dou Liyang wrote:
> +/* Setup local APIC timer and get the Id*/
> +static int __init apic_bsp_timer_setup(void)

This does not make sense. The id and the timers have nothing to do with
each other. 

> +{
> + int id;
> +
> + if (x2apic_mode)
> + id = apic_read(APIC_LDR);
> + else
> + id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
> +
> + if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
> + check_timer();

Why are you moving this to the APIC? check_timer() has absolutely nothing
to do with the apic timer. It's a IOAPIC only issue and required to test
that the IRQ0 interrupt delivery works through the IOAPIC.

Thanks,

tglx

[RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp_setup()

[Dou Liyang]

The apic_bsp_setup() contains the APIC timer related code, which
leads to hard reuse the local APIC and I/O APIC setup independently.

Extract the related code to a single function for setuping APIC in
advance.

Signed-off-by: Dou Liyang 
---
 arch/x86/include/asm/io_apic.h |  2 ++
 arch/x86/kernel/apic/apic.c| 28 +---
 arch/x86/kernel/apic/io_apic.c |  4 +---
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 6cbf2cf..535ca00 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -189,6 +189,7 @@ static inline unsigned int io_apic_read(unsigned int apic, 
unsigned int reg)
return x86_io_apic_ops.read(apic, reg);
 }
 
+extern void check_timer(void);
 extern void setup_IO_APIC(void);
 extern void enable_IO_APIC(void);
 extern void disable_IO_APIC(void);
@@ -230,6 +231,7 @@ static inline void io_apic_init_mappings(void) { }
 #define native_io_apic_readNULL
 #define native_disable_io_apic NULL
 
+static inline void check_timer(void) { }
 static inline void setup_IO_APIC(void) { }
 static inline void enable_IO_APIC(void) { }
 static inline void setup_ioapic_dest(void) { }
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index bf4ccd0..0ba8a85 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2324,6 +2324,25 @@ static void __init apic_bsp_up_setup(void)
physid_set_mask_of_physid(boot_cpu_physical_apicid, 
_cpu_present_map);
 }
 
+/* Setup local APIC timer and get the Id*/
+static int __init apic_bsp_timer_setup(void)
+{
+   int id;
+
+   if (x2apic_mode)
+   id = apic_read(APIC_LDR);
+   else
+   id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
+
+   if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
+   check_timer();
+
+   /* Setup local timer */
+   x86_init.timers.setup_percpu_clockev();
+
+   return id;
+}
+
 /**
  * apic_bsp_setup - Setup function for local apic and io-apic
  * @upmode:Force UP mode (for APIC_init_uniprocessor)
@@ -2340,17 +2359,12 @@ int __init apic_bsp_setup(bool upmode)
apic_bsp_up_setup();
setup_local_APIC();
 
-   if (x2apic_mode)
-   id = apic_read(APIC_LDR);
-   else
-   id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
-
enable_IO_APIC();
end_local_APIC_setup();
irq_remap_enable_fault_handling();
setup_IO_APIC();
-   /* Setup local timer */
-   x86_init.timers.setup_percpu_clockev();
+
+   id = apic_bsp_timer_setup();
return id;
 }
 
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 347bb9f..e19b88f 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2047,7 +2047,7 @@ static int mp_alloc_timer_irq(int ioapic, int pin)
  *
  * FIXME: really need to revamp this for all platforms.
  */
-static inline void __init check_timer(void)
+void __init check_timer(void)
 {
struct irq_data *irq_data = irq_get_irq_data(0);
struct mp_chip_data *data = irq_data->chip_data;
@@ -2278,8 +2278,6 @@ void __init setup_IO_APIC(void)
sync_Arb_IDs();
setup_IO_APIC_irqs();
init_IO_APIC_traps();
-   if (nr_legacy_irqs())
-   check_timer();
 
ioapic_initialized = 1;
 }
-- 
2.5.5

[RFC PATCH 3/6] x86/apic: Extract APIC timer related code from apic_bsp_setup()

[Dou Liyang]

The apic_bsp_setup() contains the APIC timer related code, which
leads to hard reuse the local APIC and I/O APIC setup independently.

Extract the related code to a single function for setuping APIC in
advance.

Signed-off-by: Dou Liyang 
---
 arch/x86/include/asm/io_apic.h |  2 ++
 arch/x86/kernel/apic/apic.c| 28 +---
 arch/x86/kernel/apic/io_apic.c |  4 +---
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 6cbf2cf..535ca00 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -189,6 +189,7 @@ static inline unsigned int io_apic_read(unsigned int apic, 
unsigned int reg)
return x86_io_apic_ops.read(apic, reg);
 }
 
+extern void check_timer(void);
 extern void setup_IO_APIC(void);
 extern void enable_IO_APIC(void);
 extern void disable_IO_APIC(void);
@@ -230,6 +231,7 @@ static inline void io_apic_init_mappings(void) { }
 #define native_io_apic_readNULL
 #define native_disable_io_apic NULL
 
+static inline void check_timer(void) { }
 static inline void setup_IO_APIC(void) { }
 static inline void enable_IO_APIC(void) { }
 static inline void setup_ioapic_dest(void) { }
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index bf4ccd0..0ba8a85 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2324,6 +2324,25 @@ static void __init apic_bsp_up_setup(void)
physid_set_mask_of_physid(boot_cpu_physical_apicid, 
_cpu_present_map);
 }
 
+/* Setup local APIC timer and get the Id*/
+static int __init apic_bsp_timer_setup(void)
+{
+   int id;
+
+   if (x2apic_mode)
+   id = apic_read(APIC_LDR);
+   else
+   id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
+
+   if (!skip_ioapic_setup && nr_ioapics && nr_legacy_irqs())
+   check_timer();
+
+   /* Setup local timer */
+   x86_init.timers.setup_percpu_clockev();
+
+   return id;
+}
+
 /**
  * apic_bsp_setup - Setup function for local apic and io-apic
  * @upmode:Force UP mode (for APIC_init_uniprocessor)
@@ -2340,17 +2359,12 @@ int __init apic_bsp_setup(bool upmode)
apic_bsp_up_setup();
setup_local_APIC();
 
-   if (x2apic_mode)
-   id = apic_read(APIC_LDR);
-   else
-   id = GET_APIC_LOGICAL_ID(apic_read(APIC_LDR));
-
enable_IO_APIC();
end_local_APIC_setup();
irq_remap_enable_fault_handling();
setup_IO_APIC();
-   /* Setup local timer */
-   x86_init.timers.setup_percpu_clockev();
+
+   id = apic_bsp_timer_setup();
return id;
 }
 
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 347bb9f..e19b88f 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2047,7 +2047,7 @@ static int mp_alloc_timer_irq(int ioapic, int pin)
  *
  * FIXME: really need to revamp this for all platforms.
  */
-static inline void __init check_timer(void)
+void __init check_timer(void)
 {
struct irq_data *irq_data = irq_get_irq_data(0);
struct mp_chip_data *data = irq_data->chip_data;
@@ -2278,8 +2278,6 @@ void __init setup_IO_APIC(void)
sync_Arb_IDs();
setup_IO_APIC_irqs();
init_IO_APIC_traps();
-   if (nr_legacy_irqs())
-   check_timer();
 
ioapic_initialized = 1;
 }
-- 
2.5.5