x86, ACPI, numa: Parse numa info early

drivers/gpu/drm/i915/i915_gem_stolen.c static unsigned long i915_stolen_to_physical(struct drm_device *dev) · line 84

Quoted 10 lines · Yinghai Lu

drivers/gpu/drm/i915/i915_gem_stolen.c +1 -1

index 69d97cb..7f9380b 100644
@@ -81,7 +81,7 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
              base -= dev_priv->mm.gtt->stolen_size;
      } else {
              /* Stolen is immediately above Top of Memory */
-             base = max_low_pfn_mapped << PAGE_SHIFT;
+             base = __REMOVED_CRAZY__ << PAGE_SHIFT;

Huh?

Quoted 13 lines · Tejun Heo

drivers/gpu/drm/i915/i915_gem_stolen.c static unsigned long i915_stolen_to_physical(struct drm_device *dev) · line 84

Quoted 10 lines · Yinghai Lu

drivers/gpu/drm/i915/i915_gem_stolen.c +1 -1

index 69d97cb..7f9380b 100644
@@ -81,7 +81,7 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
              base -= dev_priv->mm.gtt->stolen_size;
      } else {
              /* Stolen is immediately above Top of Memory */
-             base = max_low_pfn_mapped << PAGE_SHIFT;
+             base = __REMOVED_CRAZY__ << PAGE_SHIFT;

Huh?

Whole function:

Quoted 15 lines · Yinghai Lu

Quoted 13 lines · Tejun Heo

drivers/gpu/drm/i915/i915_gem_stolen.c static unsigned long i915_stolen_to_physical(struct drm_device *dev) · line 84

Quoted 10 lines · Yinghai Lu

drivers/gpu/drm/i915/i915_gem_stolen.c +1 -1

index 69d97cb..7f9380b 100644
@@ -81,7 +81,7 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
              base -= dev_priv->mm.gtt->stolen_size;
      } else {
              /* Stolen is immediately above Top of Memory */
-             base = max_low_pfn_mapped << PAGE_SHIFT;
+             base = __REMOVED_CRAZY__ << PAGE_SHIFT;

Huh?

Quoted 1 line

Whole function:

Yeah, but can't we still just do 1LLU << 32 like other places? Or at least explain what was there before? It's gonna confuse the hell out of future readers of the code.

They are not using memblock_find_in_range(), so 1ULL<< will not help.

Really hope i915 drm guys could clean that hacks.

Quoted 3 lines · Yinghai Lu

They are not using memblock_find_in_range(), so 1ULL<< will not help.

Really hope i915 drm guys could clean that hacks.

The code isn't being used. Just leave it alone. Maybe add a comment.
The change is just making things more confusing.

Quoted 5 lines · Tejun Heo

Quoted 3 lines · Yinghai Lu

They are not using memblock_find_in_range(), so 1ULL<< will not help.

Really hope i915 drm guys could clean that hacks.

Quoted 3 lines

The code isn't being used. Just leave it alone. Maybe add a comment.
The change is just making things more confusing.

Indeed, but...

Daniel: can you guys clean this up or can we just remove the #if 0 clause?

Quoted 10 lines · H. Peter Anvin

Quoted 8 lines · Tejun Heo

Quoted 3 lines · Yinghai Lu

They are not using memblock_find_in_range(), so 1ULL<< will not help.

Really hope i915 drm guys could clean that hacks.

The code isn't being used. Just leave it alone. Maybe add a comment.
The change is just making things more confusing.

Quoted 3 lines

Indeed, but...

Daniel: can you guys clean this up or can we just remove the #if 0 clause?

I guess we could just put this into a comment explaining where stolen memory for the gfx devices is at on gen2. But tbh I don't mind if we just keep the #if 0 code around. For all newer platforms we can get at that offset through mch bar registers, so I don't really care.

Quoted 2 lines · H. Peter Anvin

Quoted 1 line · Tejun Heo

On Thu, Mar 7, 2013 at 9:27 PM, Yinghai Lu <[email protected]>

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

They are not using memblock_find_in_range(), so 1ULL<< will not

help.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

Really hope i915 drm guys could clean that hacks.

The code isn't being used. Just leave it alone. Maybe add a

comment.

Quoted 6 lines

Quoted 2 lines

The change is just making things more confusing.

Indeed, but...

Daniel: can you guys clean this up or can we just remove the #if 0

clause?

I guess we could just put this into a comment explaining where stolen memory for the gfx devices is at on gen2. But tbh I don't mind if we just
keep the #if 0 code around. For all newer platforms we can get at that offset through mch bar registers, so I don't really care. -Daniel

To parse srat early, we will need to move acpi table probing early. and to keep acpi_initrd_table_override working, we need to move it ahead.

But current that is called after init_mem_mapping and relocate_initrd().

Copying need to be after memblock is ready, because it need to allocate some buffer for acpi tables.

Finding will be moved into head_32.S and head64.c, just like microcode early scanning.

So split them at first.

Also move down functions declaration to avoid #ifdef in setup.c

Signed-off-by Yinghai <[email protected]>

Cc Thomas Renninger <[email protected]>

Cc Pekka Enberg <[email protected]>

Cc Jacob Shin <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc [email protected]

drivers/acpi/osl.c +0 -3

@@ -647,14 +653,14 @@ void __init acpi_initrd_override(void *data, size_t size)
 	memblock_reserve(acpi_tables_addr, acpi_tables_addr + all_tables_size);
 	arch_reserve_mem_area(acpi_tables_addr, all_tables_size);
 
-	p = early_ioremap(acpi_tables_addr, all_tables_size);
-
 	for (no = 0; no < table_nr; no++) {
-		memcpy(p + total_offset, early_initrd_files[no].data,

include/linux/acpi.h +0 -8

@@ -79,14 +79,6 @@ typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
 typedef int (*acpi_tbl_entry_handler)(struct acpi_subtable_header *header,
 				      const unsigned long end);
 
-#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
-void acpi_initrd_override(void *data, size_t size);
-#else
-static inline void acpi_initrd_override(void *data, size_t size)
-{
-}
-#endif
-
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);

Quoted 6 lines · Yinghai Lu

drivers/acpi/osl.c +0 -0

index c9e36d7..b9d2ff0 100644
@@ -539,6 +539,7 @@ acpi_os_predefined_override(const struct acpi_predefined_names *init_val,
 

drivers/acpi/osl.c acpi_os_predefined_override(const struct acpi_predefined_names *init_val, · line 542

Quoted 3 lines

drivers/acpi/osl.c +1 -0

 static u64 acpi_tables_addr;
 static int all_tables_size;
+static int table_nr;

Not particularly good choice of name for static variable visible to multiple functions. all_tables_size isn't a stellar choice either but no need to continue the tradition. Maybe acpi_nr_initrd_files? Also, why is this one defined here away from the actual table?

drivers/acpi/osl.c static const char * const table_sigs[] = { · line 573

Quoted 3 lines

drivers/acpi/osl.c +1 -2

-/* Must not increase 10 or needs code modification below */
-#define ACPI_OVERRIDE_TABLES 10
+#define ACPI_OVERRIDE_TABLES 64

What's up with the silent bumping of table size?

drivers/acpi/osl.c static const char * const table_sigs[] = { · line 574

Quoted 2 lines

drivers/acpi/osl.c +1 -0

+static struct cpio_data __initdata early_initrd_files[ACPI_OVERRIDE_TABLES];

acpi_initrd_files[]? Do we really need the "early" designation together with initrd?

Quoted 5 lines

drivers/acpi/osl.c +0 -0

@@ -647,14 +653,14 @@ void __init acpi_initrd_override(void *data, size_t size)
      memblock_reserve(acpi_tables_addr, acpi_tables_addr + all_tables_size);
      arch_reserve_mem_area(acpi_tables_addr, all_tables_size);
 

Quoted 4 lines

drivers/acpi/osl.c +0 -3

-     p = early_ioremap(acpi_tables_addr, all_tables_size);
-
      for (no = 0; no < table_nr; no++) {
-             memcpy(p + total_offset, early_initrd_files[no].data,

drivers/acpi/osl.c void __init acpi_initrd_override(void *data, size_t size) · line 657

Quoted 10 lines

drivers/acpi/osl.c +6 -3

-                    early_initrd_files[no].size);
-             total_offset += early_initrd_files[no].size;
+             size_t size = early_initrd_files[no].size;
+
+             p = early_ioremap(acpi_tables_addr + total_offset, size);
+             memcpy(p, early_initrd_files[no].data, size);
+             early_iounmap(p, size);
+             total_offset += size;
      }
-     early_iounmap(p, all_tables_size);

Why is this necessary? Why no explanation in the description?

Quoted 22 lines

include/linux/acpi.h +0 -8

@@ -79,14 +79,6 @@ typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
 typedef int (*acpi_tbl_entry_handler)(struct acpi_subtable_header *header,
                                    const unsigned long end);
 
-#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
-void acpi_initrd_override(void *data, size_t size);
-#else
-static inline void acpi_initrd_override(void *data, size_t size)
-{
-}
-#endif
-
 char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);

@@ -485,6 +477,14 @@ static inline bool acpi_driver_match_device(struct device *dev,
 
 #endif       /* !CONFIG_ACPI */
 

include/linux/acpi.h static inline bool acpi_driver_match_device(struct device *dev, · line 486

Quoted 7 lines

include/linux/acpi.h +7 -0

+#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
+void acpi_initrd_override_find(void *data, size_t size);
+void acpi_initrd_override_copy(void);
+#else
+static inline void acpi_initrd_override_find(void *data, size_t size) { }
+static inline void acpi_initrd_override_copy(void) { }
+#endif

I don't get this part either. Why is it necessary to move the prototypes to avoid #ifdefs in setup.c? Ah, okay, you're brining it outside CONFIG_ACPI so that they're defined regardless of that config option. Can you please add why you're moving the prototype in the descriptoin? Having "what" is nice but "why" is much nicer. :)

As later 32bit only find table with phys address during 32bit flat mode in head_32.S.

To keep 32bit and 64 bit consistent, use phys_addr for all.

Use early_ioremap to access during copying.

Signed-off-by Yinghai Lu <[email protected]>

Cc Thomas Renninger <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc [email protected]

---

drivers/acpi/osl.c +0 -0

@@ -654,10 +654,13 @@ void __init acpi_initrd_override_copy(void)
 	arch_reserve_mem_area(acpi_tables_addr, all_tables_size);
 

Quoted 19 lines · Yinghai Lu

As later 32bit only find table with phys address during 32bit flat mode in head_32.S.

To keep 32bit and 64 bit consistent, use phys_addr for all.

Use early_ioremap to access during copying.

Signed-off-by Yinghai Lu <[email protected]>

Cc Thomas Renninger <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc [email protected]

---

drivers/acpi/osl.c +1 -1

@@ -654,10 +654,13 @@ void __init acpi_initrd_override_copy(void)
      arch_reserve_mem_area(acpi_tables_addr, all_tables_size);
 
      for (no = 0; no < table_nr; no++) {
-             size_t size = early_initrd_files[no].size;
+             unsigned long size = early_initrd_files[no].size;
 

drivers/acpi/osl.c void __init acpi_initrd_override_copy(void) · line 663

Quoted 6 lines

drivers/acpi/osl.c +4 -1

              p = early_ioremap(acpi_tables_addr + total_offset, size);
-             memcpy(p, early_initrd_files[no].data, size);
+             q = early_ioremap((unsigned long)early_initrd_files[no].data,
+                                      size);
+             memcpy(p, q, size);
+             early_iounmap(q, size);

Ah, okay, so the loop change in the previous patch was for this, I suppose? That chunk probably should either be a separate patch or rolled into this one.

Quoted 7 lines

Quoted 7 lines

drivers/acpi/osl.c +1 -1

@@ -654,10 +654,13 @@ void __init acpi_initrd_override_copy(void)
      arch_reserve_mem_area(acpi_tables_addr, all_tables_size);
 
      for (no = 0; no < table_nr; no++) {
-             size_t size = early_initrd_files[no].size;
+             unsigned long size = early_initrd_files[no].size;
 

Quoted 10 lines

drivers/acpi/osl.c void __init acpi_initrd_override_copy(void) · line 663

Quoted 6 lines

drivers/acpi/osl.c +4 -1

              p = early_ioremap(acpi_tables_addr + total_offset, size);
-             memcpy(p, early_initrd_files[no].data, size);
+             q = early_ioremap((unsigned long)early_initrd_files[no].data,
+                                      size);
+             memcpy(p, q, size);
+             early_iounmap(q, size);

Ah, okay, so the loop change in the previous patch was for this, I suppose? That chunk probably should either be a separate patch or rolled into this one.

merge two patches?

We will find acpi tables in initrd during head_32.S in 32bit flat mode.

So need acpi_initrd_override_find could take phys directly.

drivers/acpi/osl.c +0 -1

@@ -552,38 +552,47 @@ u8 __init acpi_table_checksum(u8 *buffer, u32 length)
 	return sum;
 }
 
-/* All but ACPI_SIG_RSDP and ACPI_SIG_FACS: */

Quoted 3 lines · Yinghai Lu

We will find acpi tables in initrd during head_32.S in 32bit flat mode.

So need acpi_initrd_override_find could take phys directly.

The patch description doesn't explain even half of what's going on.

Quoted 5 lines

drivers/acpi/osl.c +0 -0

@@ -552,38 +552,47 @@ u8 __init acpi_table_checksum(u8 *buffer, u32 length)
      return sum;
 }
 

drivers/acpi/osl.c u8 __init acpi_table_checksum(u8 *buffer, u32 length) · line 565

Quoted 11 lines

drivers/acpi/osl.c +0 -11

-/* All but ACPI_SIG_RSDP and ACPI_SIG_FACS: */
-static const char * const table_sigs[] = {
-     ACPI_SIG_BERT, ACPI_SIG_CPEP, ACPI_SIG_ECDT, ACPI_SIG_EINJ,
-     ACPI_SIG_ERST, ACPI_SIG_HEST, ACPI_SIG_MADT, ACPI_SIG_MSCT,
-     ACPI_SIG_SBST, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_ASF,
-     ACPI_SIG_BOOT, ACPI_SIG_DBGP, ACPI_SIG_DMAR, ACPI_SIG_HPET,
-     ACPI_SIG_IBFT, ACPI_SIG_IVRS, ACPI_SIG_MCFG, ACPI_SIG_MCHI,
-     ACPI_SIG_SLIC, ACPI_SIG_SPCR, ACPI_SIG_SPMI, ACPI_SIG_TCPA,
-     ACPI_SIG_UEFI, ACPI_SIG_WAET, ACPI_SIG_WDAT, ACPI_SIG_WDDT,
-     ACPI_SIG_WDRT, ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_PSDT,
-     ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };

Why is this table made a stack variable? What's the benefit of doing that?

drivers/acpi/osl.c u8 __init acpi_table_checksum(u8 *buffer, u32 length) · line 557

Quoted 5 lines

drivers/acpi/osl.c +1 -1

 /* Non-fatal errors: Affected tables/files are ignored */
 #define INVALID_TABLE(x, path, name)                                 \
-     { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); continue; }
+     do { pr_err("ACPI OVERRIDE: " x " [%s%s]\n", path, name); } while (0)

Might as well rename the macro to something which indicates it's just printing error message. Urgh... who thought embedding control flow directive like continue inside a macro was a good idea? :(

drivers/acpi/osl.c u8 __init acpi_table_checksum(u8 *buffer, u32 length) · line 564

Quoted 3 lines

drivers/acpi/osl.c +1 -1

-void __init acpi_initrd_override_find(void *data, size_t size)
+void __init acpi_initrd_override_find(void *data, size_t size, bool is_phys)

Is it really necessary to make the function take both virtual and physical addresses? Can't we just make the function take phys_addr_t and update everyone to call with physaddr? Also @is_phys isn't simple address switch. It also changes error reporting. If you're gonna keep @is_phys, let's at least write up a function comment explaining what's going on and why we need it. But, really, if at all possible, let's change the function to take single type of argument and predicate error message printing on something else (e.g. early printk initialized or whatever).

drivers/acpi/osl.c void __init acpi_initrd_override_copy(void) · line 680

Quoted 6 lines

drivers/acpi/osl.c +1 -0

@@ -654,11 +677,14 @@ void __init acpi_initrd_override_copy(void)
      arch_reserve_mem_area(acpi_tables_addr, all_tables_size);
 
      for (no = 0; no < table_nr; no++) {
+             unsigned long phys_addr = (unsigned long)early_initrd_files[no].data;

Can we please use phys_addr_t for physical addresses?

drivers/acpi/osl.c void __init acpi_initrd_override_copy(void) · line 686

Quoted 7 lines

drivers/acpi/osl.c +4 -0

              unsigned long size = early_initrd_files[no].size;
 
+             q = early_ioremap(phys_addr, size);
+             pr_info("%4.4s ACPI table found in initrd [%#010lx-%#010lx]\n",
+                             ((struct acpi_table_header *)q)->signature,
+                             phys_addr, phys_addr + size - 1);

Maybe putting pr_info after ioremapping both p and q would be easier on the eyes?

drivers/acpi/osl.c void __init acpi_initrd_override_copy(void) · line 690

Quoted 7 lines

drivers/acpi/osl.c +0 -2

              p = early_ioremap(acpi_tables_addr + total_offset, size);
-             q = early_ioremap((unsigned long)early_initrd_files[no].data,
-                                      size);
              memcpy(p, q, size);
              early_iounmap(q, size);
              early_iounmap(p, size);

Quoted 6 lines · Tejun Heo

Quoted 3 lines · Yinghai Lu

We will find acpi tables in initrd during head_32.S in 32bit flat mode.

So need acpi_initrd_override_find could take phys directly.

The patch description doesn't explain even half of what's going on.

hope HPA could understand.

Access initrd before relocate_initrd and init_memory mapping.

Quoted 6 lines

Quoted 6 lines

drivers/acpi/osl.c +0 -6

-/* All but ACPI_SIG_RSDP and ACPI_SIG_FACS: */
-static const char * const table_sigs[] = {
-     ACPI_SIG_BERT, ACPI_SIG_CPEP, ACPI_SIG_ECDT, ACPI_SIG_EINJ,
-     ACPI_SIG_ERST, ACPI_SIG_HEST, ACPI_SIG_MADT, ACPI_SIG_MSCT,
-     ACPI_SIG_SBST, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_ASF,
-     ACPI_SIG_BOOT, ACPI_SIG_DBGP, ACPI_SIG_DMAR, ACPI_SIG_HPET,

Quoted 8 lines

drivers/acpi/osl.c u8 __init acpi_table_checksum(u8 *buffer, u32 length) · line 565

Quoted 5 lines

drivers/acpi/osl.c +0 -5

-     ACPI_SIG_IBFT, ACPI_SIG_IVRS, ACPI_SIG_MCFG, ACPI_SIG_MCHI,
-     ACPI_SIG_SLIC, ACPI_SIG_SPCR, ACPI_SIG_SPMI, ACPI_SIG_TCPA,
-     ACPI_SIG_UEFI, ACPI_SIG_WAET, ACPI_SIG_WDAT, ACPI_SIG_WDDT,
-     ACPI_SIG_WDRT, ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_PSDT,
-     ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };

Why is this table made a stack variable? What's the benefit of doing that?

so I do need to switch global variables to phys and access it.

Quoted 9 lines

Is it really necessary to make the function take both virtual and physical addresses? Can't we just make the function take phys_addr_t and update everyone to call with physaddr? Also @is_phys isn't simple address switch. It also changes error reporting. If you're gonna keep @is_phys, let's at least write up a function comment explaining what's going on and why we need it. But, really, if at all possible, let's change the function to take single type of argument and predicate error message printing on something else (e.g. early printk initialized or whatever).

yes, one for 32bit from head_32.S, phys.
one for 64bit from head64.c. with _va.

Not sure if I could use early_printk from head_32.S, as Fenghua does not print out
from microcode updating early in the same parts.

Hello, Yinghai.

Quoted 11 lines · Yinghai Lu

Quoted 6 lines · Tejun Heo

Quoted 3 lines · Yinghai Lu

We will find acpi tables in initrd during head_32.S in 32bit flat mode.

So need acpi_initrd_override_find could take phys directly.

The patch description doesn't explain even half of what's going on.

hope HPA could understand.

Access initrd before relocate_initrd and init_memory mapping.

I really hope the changelogs were better. Eh well...

Quoted 15 lines

Quoted 14 lines

drivers/acpi/osl.c u8 __init acpi_table_checksum(u8 *buffer, u32 length) · line 565

Quoted 11 lines

drivers/acpi/osl.c +0 -11

-/* All but ACPI_SIG_RSDP and ACPI_SIG_FACS: */
-static const char * const table_sigs[] = {
-     ACPI_SIG_BERT, ACPI_SIG_CPEP, ACPI_SIG_ECDT, ACPI_SIG_EINJ,
-     ACPI_SIG_ERST, ACPI_SIG_HEST, ACPI_SIG_MADT, ACPI_SIG_MSCT,
-     ACPI_SIG_SBST, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_ASF,
-     ACPI_SIG_BOOT, ACPI_SIG_DBGP, ACPI_SIG_DMAR, ACPI_SIG_HPET,
-     ACPI_SIG_IBFT, ACPI_SIG_IVRS, ACPI_SIG_MCFG, ACPI_SIG_MCHI,
-     ACPI_SIG_SLIC, ACPI_SIG_SPCR, ACPI_SIG_SPMI, ACPI_SIG_TCPA,
-     ACPI_SIG_UEFI, ACPI_SIG_WAET, ACPI_SIG_WDAT, ACPI_SIG_WDDT,
-     ACPI_SIG_WDRT, ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_PSDT,
-     ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };

Why is this table made a stack variable? What's the benefit of doing that?

Quoted 1 line

so I do need to switch global variables to phys and access it.

I can't really understand what your response means. Can you please elaborate?

Quoted 11 lines

Quoted 10 lines

Is it really necessary to make the function take both virtual and physical addresses? Can't we just make the function take phys_addr_t and update everyone to call with physaddr? Also @is_phys isn't simple address switch. It also changes error reporting. If you're gonna keep @is_phys, let's at least write up a function comment explaining what's going on and why we need it. But, really, if at all possible, let's change the function to take single type of argument and predicate error message printing on something else (e.g. early printk initialized or whatever).

Quoted 2 lines

yes, one for 32bit from head_32.S, phys.
one for 64bit from head64.c. with _va.

head64.c can't call with phys? Why not?

Quoted 4 lines

Not sure if I could use early_printk from head_32.S, as Fenghua does not print out
from microcode updating early in the same parts.

ISTR it works but it doens't have to (although it would be much nicer if it did). You can test whether printk is online and skip if it isn't online yet.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

Why is this table made a stack variable? What's the benefit of doing that?

so I do need to switch global variables to phys and access it.

Quoted 2 lines

I can't really understand what your response means. Can you please elaborate?

sorry, I missed NOT.

so I do NOT need to switch global variables from kernel virtual addr to phys address and access it
in 32bit flat mode.

Quoted 4 lines

Quoted 2 lines

yes, one for 32bit from head_32.S, phys.
one for 64bit from head64.c. with _va.

head64.c can't call with phys? Why not?

HPA's #PF set up page table only handle kernel low mapping address.

and after reset_early_page_tables, only kernel high mapping address is there. and other low mapping will be supported via #PF handler.

Quoted 5 lines

Quoted 5 lines

drivers/acpi/osl.c +0 -0

@@ -552,38 +552,47 @@ u8 __init acpi_table_checksum(u8 *buffer, u32 length)
      return sum;
 }
 

Quoted 14 lines

drivers/acpi/osl.c u8 __init acpi_table_checksum(u8 *buffer, u32 length) · line 565

Quoted 11 lines

drivers/acpi/osl.c +0 -11

-/* All but ACPI_SIG_RSDP and ACPI_SIG_FACS: */
-static const char * const table_sigs[] = {
-     ACPI_SIG_BERT, ACPI_SIG_CPEP, ACPI_SIG_ECDT, ACPI_SIG_EINJ,
-     ACPI_SIG_ERST, ACPI_SIG_HEST, ACPI_SIG_MADT, ACPI_SIG_MSCT,
-     ACPI_SIG_SBST, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_ASF,
-     ACPI_SIG_BOOT, ACPI_SIG_DBGP, ACPI_SIG_DMAR, ACPI_SIG_HPET,
-     ACPI_SIG_IBFT, ACPI_SIG_IVRS, ACPI_SIG_MCFG, ACPI_SIG_MCHI,
-     ACPI_SIG_SLIC, ACPI_SIG_SPCR, ACPI_SIG_SPMI, ACPI_SIG_TCPA,
-     ACPI_SIG_UEFI, ACPI_SIG_WAET, ACPI_SIG_WDAT, ACPI_SIG_WDDT,
-     ACPI_SIG_WDRT, ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_PSDT,
-     ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };

Why is this table made a stack variable? What's the benefit of doing that?

so I do need to switch global variables to phys and access it.

Quoted 6 lines · Tejun Heo

Quoted 3 lines · Yinghai Lu

We will find acpi tables in initrd during head_32.S in 32bit flat mode.

So need acpi_initrd_override_find could take phys directly.

The patch description doesn't explain even half of what's going on.

hope HPA could understand.

arch/x86/kernel/setup.c +14 -0

index 668e658..d43545a 100644
@@ -424,6 +424,32 @@ static void __init reserve_initrd(void)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
+void __init x86_acpi_override_find(void)
+{
+	unsigned long ramdisk_image, ramdisk_size;
+	unsigned char *p = NULL;
+
+#ifdef CONFIG_X86_32
+	struct boot_params *boot_params_p;
+
+	boot_params_p = (struct boot_params *)__pa_symbol(&boot_params);
+	ramdisk_image = boot_params_p->hdr.ramdisk_image;
+	ramdisk_size  = boot_params_p->hdr.ramdisk_size;
+	p = (unsigned char *)ramdisk_image;
+	acpi_initrd_override_find(p, ramdisk_size, true);

Quoted 8 lines · Yinghai Lu

arch/x86/kernel/head_32.S +0 -0

index 73afd11..ca08f0e 100644
@@ -149,6 +149,10 @@ ENTRY(startup_32)
      call load_ucode_bsp
 #endif
 

arch/x86/kernel/head_32.S ENTRY(startup_32) · line 154

Quoted 3 lines

arch/x86/kernel/head_32.S +3 -0

+#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
+     call x86_acpi_override_find
+#endif

The function is always defined. We can probalby lose ifdef here?

Also, does it really have to be called from head_32.S? No way this can be done after entering C code? It would be great if you can explain overall design choices in the head message (and important patches).

Quoted 23 lines

arch/x86/kernel/setup.c +14 -0

index 668e658..d43545a 100644
@@ -424,6 +424,32 @@ static void __init reserve_initrd(void)
 }
 #endif /* CONFIG_BLK_DEV_INITRD */
 
+#ifdef CONFIG_ACPI_INITRD_TABLE_OVERRIDE
+void __init x86_acpi_override_find(void)
+{
+     unsigned long ramdisk_image, ramdisk_size;
+     unsigned char *p = NULL;
+
+#ifdef CONFIG_X86_32
+     struct boot_params *boot_params_p;
+
+     boot_params_p = (struct boot_params *)__pa_symbol(&boot_params);
+     ramdisk_image = boot_params_p->hdr.ramdisk_image;
+     ramdisk_size  = boot_params_p->hdr.ramdisk_size;
+     p = (unsigned char *)ramdisk_image;
+     acpi_initrd_override_find(p, ramdisk_size, true);

arch/x86/kernel/setup.c static void __init reserve_initrd(void) · line 450

Quoted 10 lines

arch/x86/kernel/setup.c +10 -0

+#else
+     ramdisk_image = get_ramdisk_image();
+     ramdisk_size  = get_ramdisk_size();
+     if (ramdisk_image)
+             p = __va(ramdisk_image);
+     acpi_initrd_override_find(p, ramdisk_size, false);
+#endif
+}
+#else
+void __init x86_acpi_override_find(void) { }

And add a comment here why we're not doing static inline for the dummy function?

Quoted 4 lines

Also, does it really have to be called from head_32.S? No way this can be done after entering C code? It would be great if you can explain overall design choices in the head message (and important patches).

have to be with head_32.S and it is with 32bit flat mode, so could access 4G blow without setting page table.

Will try add to more in the change log.

static int __init numa_register_memblks(struct numa_meminfo *mi)

+out:

Quoted 1 line

static int __init numa_register_memblks(struct numa_meminfo *mi)

After this patch, the above name is a bit misleading, I think.

We could use numa_meminfo directly instead of memblock nid.

So we could move down set memblock nid down and only do it one time for successful path

Move node_map_pfn_alignment() to arch/x86/mm as no other user for it.

Quoted 6 lines · Yinghai Lu

We could use numa_meminfo directly instead of memblock nid.

So we could move down set memblock nid down and only do it one time for successful path

Move node_map_pfn_alignment() to arch/x86/mm as no other user for it.

Please don't move and update in the same patch. It makes it difficult to review what's really changing.

Only set memblock nid one time.

Also rename numa_register_memblks to numa_check_memblks() after move set memblock nid out.

Quoted 1 line · Yinghai Lu

Only set memblock nid one time.

Would be awesome if the description explains why we're doing this and why we're allowed to do this now.

arch/x86/mm/numa.c +6 -4

-static int __init numa_check_memblks(struct numa_meminfo *mi)
+
+int __init numa_check_memblks(struct numa_meminfo *mi)
 {
+	nodemask_t tmp_node_map;
 	unsigned long pfn_align;
 
 	/* Account for nodes with cpus and no memory */
-	node_possible_map = numa_nodes_parsed;
-	numa_nodemask_from_meminfo(&node_possible_map, mi);
-	if (WARN_ON(nodes_empty(node_possible_map)))
+	tmp_node_map = numa_nodes_parsed;
+	numa_nodemask_from_meminfo(&tmp_node_map, mi);
+	if (WARN_ON(nodes_empty(tmp_node_map)))
 		return -EINVAL;
 
 	if (!numa_meminfo_cover_memory(mi))

arch/x86/mm/numa.c +2 -2

@@ -608,8 +611,6 @@ static int __init numa_init(int (*init_func)(void))
 	if (ret < 0)
 		return ret;
 
-	numa_emulation(&numa_meminfo, numa_distance_cnt);
-
 	ret = numa_check_memblks(&numa_meminfo);
 	if (ret < 0)
 		return ret;

@@ -669,6 +670,8 @@ void __init x86_numa_init(void)
 	numa_init(dummy_numa_init);
 
 out:
+	numa_emulation(&numa_meminfo, numa_distance_cnt);
+
 	for (i = 0; i < mi->nr_blks; i++) {
 		struct numa_memblk *mb = &mi->blk[i];
 		memblock_set_node(mb->start, mb->end - mb->start, mb->nid);

arch/x86/mm/numa_emulation.c +0 -0

index dbbbb47..5a0433d 100644

Quoted 21 lines · Yinghai Lu

arch/x86/mm/numa.c +6 -4

-static int __init numa_check_memblks(struct numa_meminfo *mi)
+
+int __init numa_check_memblks(struct numa_meminfo *mi)
 {
+     nodemask_t tmp_node_map;
      unsigned long pfn_align;
 
      /* Account for nodes with cpus and no memory */
-     node_possible_map = numa_nodes_parsed;
-     numa_nodemask_from_meminfo(&node_possible_map, mi);
-     if (WARN_ON(nodes_empty(node_possible_map)))
+     tmp_node_map = numa_nodes_parsed;
+     numa_nodemask_from_meminfo(&tmp_node_map, mi);
+     if (WARN_ON(nodes_empty(tmp_node_map)))
              return -EINVAL;
 
      if (!numa_meminfo_cover_memory(mi))

@@ -562,6 +564,7 @@ static int __init numa_check_memblks(struct numa_meminfo *mi)
              return -EINVAL;
      }
 

arch/x86/mm/numa.c static int __init numa_check_memblks(struct numa_meminfo *mi) · line 567

Quoted 1 line

arch/x86/mm/numa.c +1 -0

+     node_possible_map = tmp_node_map;

Hmmm.... it's kinda nasty to have a side effect like the above for a function named numa_check_memblks(). Maybe we can move this to the caller or name the function to make it clear that some global state is being updated?

Quoted 27 lines

arch/x86/mm/numa.c +2 -2

@@ -608,8 +611,6 @@ static int __init numa_init(int (*init_func)(void))
      if (ret < 0)
              return ret;
 
-     numa_emulation(&numa_meminfo, numa_distance_cnt);
-
      ret = numa_check_memblks(&numa_meminfo);
      if (ret < 0)
              return ret;

@@ -669,6 +670,8 @@ void __init x86_numa_init(void)
      numa_init(dummy_numa_init);
 
 out:
+     numa_emulation(&numa_meminfo, numa_distance_cnt);
+
      for (i = 0; i < mi->nr_blks; i++) {
              struct numa_memblk *mb = &mi->blk[i];
              memblock_set_node(mb->start, mb->end - mb->start, mb->nid);

arch/x86/mm/numa_emulation.c +0 -0

index dbbbb47..5a0433d 100644
@@ -348,7 +348,7 @@ void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt)
      if (ret < 0)
              goto no_emu;
 

arch/x86/mm/numa_emulation.c void __init numa_emulation(struct numa_meminfo *numa_meminfo, int numa_dist_cnt) · line 354

Quoted 5 lines

arch/x86/mm/numa_emulation.c +1 -1

-     if (numa_cleanup_meminfo(&ei) < 0) {
+     if (numa_cleanup_meminfo(&ei) < 0 || numa_check_memblks(&ei) < 0) {
              pr_warning("NUMA: Warning: constructed meminfo invalid, disabling emulation\n");
              goto no_emu;
      }

Given that acpi is the only mechanism which matters in any modern NUMA machines, I think the re-ordering should be fine.

drivers/acpi/numa.c acpi_table_parse_srat(enum acpi_srat_type id, · line 293

Quoted 9 lines · Yinghai Lu

drivers/acpi/numa.c +7 -0

+void __init acpi_numa_init_only_slit(void)
+{
+     /* SLIT: System Locality Information Table */
+     acpi_table_parse(ACPI_SIG_SLIT, acpi_parse_slit);
+}
+
+static int __init __acpi_numa_init(bool with_slit)
 {
      int cnt = 0;

Hmmm.... how about just having the following two functions

        acpi_numa_init_srat();
        acpi_numa_init_slit();

and update both x86 and ia64 to use the two functions?

ia64 like to call in this seqence
acpi_numa_init()
parse srat
parse slit
then
acpi_numa_arch_fixup()

in this arch_fixup, it will try to fill dummy distance_matrix.

so would to keep acpi_numa_init ...

Quoted 7 lines · Yinghai Lu

ia64 like to call in this seqence
acpi_numa_init()
parse srat
parse slit
then
acpi_numa_arch_fixup()

Quoted 3 lines

in this arch_fixup, it will try to fill dummy distance_matrix.

so would to keep acpi_numa_init ...

Can't it just call acpi_numa_init_srat() and then init_slit()? What am I missing?

If node with ram is hotplugable, local node mem for page table and vmemmap should be on that node ram.

This patch is some kind of refreshment of

| commit 1411e0ec3123ae4c4ead6bfc9fe3ee5a3ae5c327
| Date:   Mon Dec 27 16:48:17 2010 -0800
|
|    x86-64, numa: Put pgtable to local node memory

That was reverted before.

We have reason to reintroduce it to make memory hotplug work.

Split calling of init_mem_mapping into early_initmem_info for nodes after we get numa info there.

First node will be low range.
Need to rework alloc_low_pages to alloc page table in following order:
BRK, local node, low range

Still only load_cr3 one time, otherwise we would break xen 64bit again.

Quoted 15 lines · Yinghai Lu

If node with ram is hotplugable, local node mem for page table and vmemmap should be on that node ram.

This patch is some kind of refreshment of

| commit 1411e0ec3123ae4c4ead6bfc9fe3ee5a3ae5c327
| Date:   Mon Dec 27 16:48:17 2010 -0800
|
|    x86-64, numa: Put pgtable to local node memory

That was reverted before.

We have reason to reintroduce it to make memory hotplug work.

Split calling of init_mem_mapping into early_initmem_info for nodes after we get numa info there.

Quoted 5 lines

First node will be low range.
Need to rework alloc_low_pages to alloc page table in following order:
BRK, local node, low range

Still only load_cr3 one time, otherwise we would break xen 64bit again.

Hmmm... can you please split this patch further? init_mem_mapping() change can be separated, no?

Also, comments are disturbingly missing.
How are other people reading the code supposed to know what it's trying to achieve why and how? Hmmm... we're also likely to end up with smaller mapping for misaligned NUMA configurations (I think my test machine is like that). Is it guaranteed that the top level ends up in the first node? It really needs documentation.

Hi Yinghai,

On 03/08/2013 12:58 PM, Yinghai Lu wrote:
......

Quoted 5 lines

        /* xen has big range in reserved near end of ram, skip it at

first.*/
- addr = memblock_find_in_range(ISA_END_ADDRESS, end, PMD_SIZE,
PMD_SIZE);
+ addr = memblock_find_in_range(begin, end, PMD_SIZE, PMD_SIZE);

Found that the latest code here is:

414 addr = memblock_find_in_range(ISA_END_ADDRESS, end, PMD_SIZE,

 415                          PAGE_SIZE);
                             ^^^^^^^^^^^^

The "align" is PAGE_SIZE, not PMD_SIZE. Not sure if it is a problem. :)