[PATCH v5 0/2] x86/devicetree: Enable multiprocessing

[PATCH v5 0/2] x86/devicetree: Enable multiprocessing

[Ivan Gorinov]

Current x86 implementation of Device Tree does not support multiprocessing,
and the bindings documentation describes the "reg" property as CPU number
instead of hardware-assigned local APIC ID.

Ivan Gorinov (2):
  of: Documentation: Specify local APIC ID in "reg"
  x86/devicetree: Use CPU description from Device Tree

 Documentation/devicetree/bindings/x86/ce4100.txt | 38 ++++++++++++++++-----
 arch/x86/kernel/devicetree.c                     | 42 +++++++++++++++++-------
 2 files changed, 60 insertions(+), 20 deletions(-)

-- 
2.7.4

[PATCH v5 1/2] of: Documentation: Specify local APIC ID in "reg"

[Ivan Gorinov]

Set the "reg" property to the processor's local APIC ID.
Local APIC ID is assigned by hardware and may differ from CPU number.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
---
 Documentation/devicetree/bindings/x86/ce4100.txt | 38 ++++++++++++++++++------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
index b49ae59..5a4bd83 100644
--- a/Documentation/devicetree/bindings/x86/ce4100.txt
+++ b/Documentation/devicetree/bindings/x86/ce4100.txt
@@ -7,17 +7,37 @@ Many of the "generic" devices like HPET or IO APIC have the ce4100
 name in their compatible property because they first appeared in this
 SoC.
 
-The CPU node
-------------
-	cpu@0 {
-		device_type = "cpu";
-		compatible = "intel,ce4100";
-		reg = <0>;
-		lapic = <&lapic0>;
+The CPU nodes
+-------------
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			device_type = "cpu";
+			compatible = "intel,ce4100";
+			reg = <0x00>;
+		};
+
+		cpu@1 {
+			device_type = "cpu";
+			compatible = "intel,ce4100";
+			reg = <0x02>;
+		};
 	};
 
-The reg property describes the CPU number. The lapic property points to
-the local APIC timer.
+A "cpu" node describes one logical processor (hardware thread).
+
+Required properties:
+
+- device_type
+	Device type, must be "cpu".
+
+- reg
+	Local APIC ID, a unique number assigned to each processor by
+	hardware. This ID is used to specify the destination of interrupt
+	messages with "physical" destination mode, including startup IPI.
 
 The SoC node
 ------------
-- 
2.7.4

[PATCH v5 2/2] x86/devicetree: Use CPU description from Device Tree

[Ivan Gorinov]

Current x86 Device Tree implementation does not support multiprocessing.
Use new DT bindings to describe the processors.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
---
 arch/x86/kernel/devicetree.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
index 5cd387f..64671db 100644
--- a/arch/x86/kernel/devicetree.c
+++ b/arch/x86/kernel/devicetree.c
@@ -131,31 +131,49 @@ static void __init dtb_setup_hpet(void)
 #endif
 }
 
+static void __init dtb_cpu_setup(void)
+{
+	struct device_node *dn;
+	const void *prop;
+	int prop_bytes;
+	int apic_id, version;
+	int ret;
+
+	version = GET_APIC_VERSION(apic_read(APIC_LVR));
+	for_each_node_by_type(dn, "cpu") {
+		prop = of_get_property(dn, "reg", &prop_bytes);
+		if (!prop || prop_bytes < sizeof(u32)) {
+			pr_warn("%pOF: missing local APIC ID\n", dn);
+			continue;
+		}
+		apic_id = be32_to_cpup(prop);
+		generic_processor_info(apic_id, version);
+	}
+}
+
 static void __init dtb_lapic_setup(void)
 {
 #ifdef CONFIG_X86_LOCAL_APIC
 	struct device_node *dn;
 	struct resource r;
+	unsigned long lapic_addr = APIC_DEFAULT_PHYS_BASE;
 	int ret;
 
 	dn = of_find_compatible_node(NULL, NULL, "intel,ce4100-lapic");
-	if (!dn)
-		return;
-
-	ret = of_address_to_resource(dn, 0, &r);
-	if (WARN_ON(ret))
-		return;
+	if (dn) {
+		ret = of_address_to_resource(dn, 0, &r);
+		if (WARN_ON(ret))
+			return;
+		lapic_addr = r.start;
+	}
 
 	/* Did the boot loader setup the local APIC ? */
 	if (!boot_cpu_has(X86_FEATURE_APIC)) {
-		if (apic_force_enable(r.start))
+		if (apic_force_enable(lapic_addr))
 			return;
 	}
-	smp_found_config = 1;
 	pic_mode = 1;
-	register_lapic_address(r.start);
-	generic_processor_info(boot_cpu_physical_apicid,
-			       GET_APIC_VERSION(apic_read(APIC_LVR)));
+	register_lapic_address(lapic_addr);
 #endif
 }
 
@@ -260,6 +278,7 @@ static void __init dtb_ioapic_setup(void) {}
 static void __init dtb_apic_setup(void)
 {
 	dtb_lapic_setup();
+	dtb_cpu_setup();
 	dtb_ioapic_setup();
 }
 
@@ -297,6 +316,7 @@ void __init x86_dtb_init(void)
 	if (!of_have_populated_dt())
 		return;
 
+	smp_found_config = 1;
 	dtb_setup_hpet();
 	dtb_apic_setup();
 }
-- 
2.7.4

Re: [PATCH v5 1/2] of: Documentation: Specify local APIC ID in "reg"

[Mark Rutland]

On Mon, Mar 12, 2018 at 03:21:48PM -0700, Ivan Gorinov wrote:
> Set the "reg" property to the processor's local APIC ID.
> Local APIC ID is assigned by hardware and may differ from CPU number.
> 
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---
>  Documentation/devicetree/bindings/x86/ce4100.txt | 38 ++++++++++++++++++------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/x86/ce4100.txt b/Documentation/devicetree/bindings/x86/ce4100.txt
> index b49ae59..5a4bd83 100644
> --- a/Documentation/devicetree/bindings/x86/ce4100.txt
> +++ b/Documentation/devicetree/bindings/x86/ce4100.txt
> @@ -7,17 +7,37 @@ Many of the "generic" devices like HPET or IO APIC have the ce4100
>  name in their compatible property because they first appeared in this
>  SoC.
>  
> -The CPU node
> -------------
> -	cpu@0 {
> -		device_type = "cpu";
> -		compatible = "intel,ce4100";
> -		reg = <0>;
> -		lapic = <&lapic0>;
> +The CPU nodes
> +-------------
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "intel,ce4100";
> +			reg = <0x00>;
> +		};
> +
> +		cpu@1 {
> +			device_type = "cpu";
> +			compatible = "intel,ce4100";
> +			reg = <0x02>;
> +		};

The unit-address (the bit after the '@' in the node name) should match
the reg, so this node should be named cpu@2.

If there's another ID associated with each CPU, then this should be
described in another property.

>  	};
>  
> -The reg property describes the CPU number. The lapic property points to
> -the local APIC timer.

Why was the lapic phandle removed?

Thanks,
Mark.

> +A "cpu" node describes one logical processor (hardware thread).
> +
> +Required properties:
> +
> +- device_type
> +	Device type, must be "cpu".
> +
> +- reg
> +	Local APIC ID, a unique number assigned to each processor by
> +	hardware. This ID is used to specify the destination of interrupt
> +	messages with "physical" destination mode, including startup IPI.
>  
>  The SoC node
>  ------------
> -- 
> 2.7.4
> 

Re: [PATCH v5 2/2] x86/devicetree: Use CPU description from Device Tree

[Mark Rutland]

On Mon, Mar 12, 2018 at 03:22:33PM -0700, Ivan Gorinov wrote:
> Current x86 Device Tree implementation does not support multiprocessing.
> Use new DT bindings to describe the processors.
> 
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---
>  arch/x86/kernel/devicetree.c | 42 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> index 5cd387f..64671db 100644
> --- a/arch/x86/kernel/devicetree.c
> +++ b/arch/x86/kernel/devicetree.c
> @@ -131,31 +131,49 @@ static void __init dtb_setup_hpet(void)
>  #endif
>  }
>  
> +static void __init dtb_cpu_setup(void)
> +{
> +	struct device_node *dn;
> +	const void *prop;
> +	int prop_bytes;
> +	int apic_id, version;
> +	int ret;
> +
> +	version = GET_APIC_VERSION(apic_read(APIC_LVR));
> +	for_each_node_by_type(dn, "cpu") {
> +		prop = of_get_property(dn, "reg", &prop_bytes);
> +		if (!prop || prop_bytes < sizeof(u32)) {
> +			pr_warn("%pOF: missing local APIC ID\n", dn);
> +			continue;
> +		}
> +		apic_id = be32_to_cpup(prop);

Please use of_property_read_u32().

Thanks,
Mark.

Re: [PATCH v5 1/2] of: Documentation: Specify local APIC ID in "reg"

[Ivan Gorinov]

On Tue, 2018-03-13 at 11:01 +0000, Mark Rutland wrote:

> > +		cpu@1 {
> > +			device_type = "cpu";
> > +			compatible = "intel,ce4100";
> > +			reg = <0x02>;
> > +		};
> The unit-address (the bit after the '@' in the node name) should match
> the reg, so this node should be named cpu@2.

OK

> > -The reg property describes the CPU number. The lapic property points to
> > -the local APIC timer.
> Why was the lapic phandle removed?

The "lapic" node may not be required.

Local APIC is an essential part of every logical CPU described by a "cpu"
node, with registers accessed as memory-mapped I/O (except for x2APIC mode).
Current implementation of local APIC kernel driver requires base address to
be the same on all CPUs, default 0xfee00000. If the base address is changed
by firmware, one optional node can describe new address for all CPUs.

Re: [PATCH v5 2/2] x86/devicetree: Use CPU description from Device Tree

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6380 bytes --]

Hi Ivan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180314]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ivan-Gorinov/x86-devicetree-Enable-multiprocessing/20180314-192547
config: i386-randconfig-x012-201810 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kernel/devicetree.c: In function 'dtb_cpu_setup':
   arch/x86/kernel/devicetree.c:139:6: warning: unused variable 'ret' [-Wunused-variable]
     int ret;
         ^~~
   arch/x86/kernel/devicetree.c: In function 'x86_dtb_init':
>> arch/x86/kernel/devicetree.c:314:19: error: lvalue required as left operand of assignment
     smp_found_config = 1;
                      ^

vim +314 arch/x86/kernel/devicetree.c

   132	
   133	static void __init dtb_cpu_setup(void)
   134	{
   135		struct device_node *dn;
   136		const void *prop;
   137		int prop_bytes;
   138		int apic_id, version;
 > 139		int ret;
   140	
   141		version = GET_APIC_VERSION(apic_read(APIC_LVR));
   142		for_each_node_by_type(dn, "cpu") {
   143			prop = of_get_property(dn, "reg", &prop_bytes);
   144			if (!prop || prop_bytes < sizeof(u32)) {
   145				pr_warn("%pOF: missing local APIC ID\n", dn);
   146				continue;
   147			}
   148			apic_id = be32_to_cpup(prop);
   149			generic_processor_info(apic_id, version);
   150		}
   151	}
   152	
   153	static void __init dtb_lapic_setup(void)
   154	{
   155	#ifdef CONFIG_X86_LOCAL_APIC
   156		struct device_node *dn;
   157		struct resource r;
   158		unsigned long lapic_addr = APIC_DEFAULT_PHYS_BASE;
   159		int ret;
   160	
   161		dn = of_find_compatible_node(NULL, NULL, "intel,ce4100-lapic");
   162		if (dn) {
   163			ret = of_address_to_resource(dn, 0, &r);
   164			if (WARN_ON(ret))
   165				return;
   166			lapic_addr = r.start;
   167		}
   168	
   169		/* Did the boot loader setup the local APIC ? */
   170		if (!boot_cpu_has(X86_FEATURE_APIC)) {
   171			if (apic_force_enable(lapic_addr))
   172				return;
   173		}
   174		pic_mode = 1;
   175		register_lapic_address(lapic_addr);
   176	#endif
   177	}
   178	
   179	#ifdef CONFIG_X86_IO_APIC
   180	static unsigned int ioapic_id;
   181	
   182	struct of_ioapic_type {
   183		u32 out_type;
   184		u32 trigger;
   185		u32 polarity;
   186	};
   187	
   188	static struct of_ioapic_type of_ioapic_type[] =
   189	{
   190		{
   191			.out_type	= IRQ_TYPE_EDGE_RISING,
   192			.trigger	= IOAPIC_EDGE,
   193			.polarity	= 1,
   194		},
   195		{
   196			.out_type	= IRQ_TYPE_LEVEL_LOW,
   197			.trigger	= IOAPIC_LEVEL,
   198			.polarity	= 0,
   199		},
   200		{
   201			.out_type	= IRQ_TYPE_LEVEL_HIGH,
   202			.trigger	= IOAPIC_LEVEL,
   203			.polarity	= 1,
   204		},
   205		{
   206			.out_type	= IRQ_TYPE_EDGE_FALLING,
   207			.trigger	= IOAPIC_EDGE,
   208			.polarity	= 0,
   209		},
   210	};
   211	
   212	static int dt_irqdomain_alloc(struct irq_domain *domain, unsigned int virq,
   213				      unsigned int nr_irqs, void *arg)
   214	{
   215		struct of_phandle_args *irq_data = (void *)arg;
   216		struct of_ioapic_type *it;
   217		struct irq_alloc_info tmp;
   218	
   219		if (WARN_ON(irq_data->args_count < 2))
   220			return -EINVAL;
   221		if (irq_data->args[1] >= ARRAY_SIZE(of_ioapic_type))
   222			return -EINVAL;
   223	
   224		it = &of_ioapic_type[irq_data->args[1]];
   225		ioapic_set_alloc_attr(&tmp, NUMA_NO_NODE, it->trigger, it->polarity);
   226		tmp.ioapic_id = mpc_ioapic_id(mp_irqdomain_ioapic_idx(domain));
   227		tmp.ioapic_pin = irq_data->args[0];
   228	
   229		return mp_irqdomain_alloc(domain, virq, nr_irqs, &tmp);
   230	}
   231	
   232	static const struct irq_domain_ops ioapic_irq_domain_ops = {
   233		.alloc		= dt_irqdomain_alloc,
   234		.free		= mp_irqdomain_free,
   235		.activate	= mp_irqdomain_activate,
   236		.deactivate	= mp_irqdomain_deactivate,
   237	};
   238	
   239	static void __init dtb_add_ioapic(struct device_node *dn)
   240	{
   241		struct resource r;
   242		int ret;
   243		struct ioapic_domain_cfg cfg = {
   244			.type = IOAPIC_DOMAIN_DYNAMIC,
   245			.ops = &ioapic_irq_domain_ops,
   246			.dev = dn,
   247		};
   248	
   249		ret = of_address_to_resource(dn, 0, &r);
   250		if (ret) {
   251			printk(KERN_ERR "Can't obtain address from device node %pOF.\n", dn);
   252			return;
   253		}
   254		mp_register_ioapic(++ioapic_id, r.start, gsi_top, &cfg);
   255	}
   256	
   257	static void __init dtb_ioapic_setup(void)
   258	{
   259		struct device_node *dn;
   260	
   261		for_each_compatible_node(dn, NULL, "intel,ce4100-ioapic")
   262			dtb_add_ioapic(dn);
   263	
   264		if (nr_ioapics) {
   265			of_ioapic = 1;
   266			return;
   267		}
   268		printk(KERN_ERR "Error: No information about IO-APIC in OF.\n");
   269	}
   270	#else
   271	static void __init dtb_ioapic_setup(void) {}
   272	#endif
   273	
   274	static void __init dtb_apic_setup(void)
   275	{
   276		dtb_lapic_setup();
   277		dtb_cpu_setup();
   278		dtb_ioapic_setup();
   279	}
   280	
   281	#ifdef CONFIG_OF_FLATTREE
   282	static void __init x86_flattree_get_config(void)
   283	{
   284		u32 size, map_len;
   285		void *dt;
   286	
   287		if (!initial_dtb)
   288			return;
   289	
   290		map_len = max(PAGE_SIZE - (initial_dtb & ~PAGE_MASK), (u64)128);
   291	
   292		initial_boot_params = dt = early_memremap(initial_dtb, map_len);
   293		size = of_get_flat_dt_size();
   294		if (map_len < size) {
   295			early_memunmap(dt, map_len);
   296			initial_boot_params = dt = early_memremap(initial_dtb, size);
   297			map_len = size;
   298		}
   299	
   300		unflatten_and_copy_device_tree();
   301		early_memunmap(dt, map_len);
   302	}
   303	#else
   304	static inline void x86_flattree_get_config(void) { }
   305	#endif
   306	
   307	void __init x86_dtb_init(void)
   308	{
   309		x86_flattree_get_config();
   310	
   311		if (!of_have_populated_dt())
   312			return;
   313	
 > 314		smp_found_config = 1;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29380 bytes --]