[PATCH 0/4] x86/ACPI: less verbose logging by default & more

Jan Beulich posted 4 patches 3 years, 2 months ago
Only 0 patches received!
[PATCH 0/4] x86/ACPI: less verbose logging by default & more
Posted by Jan Beulich 3 years, 2 months ago
1: ACPI: reduce verbosity by default
2: x86/ACPI: don't overwrite FADT
3: ACPI: replace casts by container_of()
4: x86: drop fake CONFIG_{HPET,X86_PM}_TIMER

Jan

[PATCH 1/4] ACPI: reduce verbosity by default
Posted by Jan Beulich 3 years, 2 months ago
While they're KERN_INFO messages and hence not visible by default, we
still have had reports that the amount of output is too large, not the
least because
- the command line controlled resizing of the console ring buffer
  happens only after SRAT parsing (which may alone produce more than 16k
  of output),
- the default resizing of the console ring buffer happens only after
  ACPI table parsing, since the default size gets calculated depending
  on the number or processors found.

Gate all per-processor logging behind a new "acpi=verbose", making sure
we wouldn't unintentionally pass this on to Dom0.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
An alternative might be to make the SRAT logging more compact.

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -75,13 +75,10 @@ of Boolean and String.  These are noted
 ## Parameter details
 
 ### acpi
-> `= force | ht | noirq | <boolean>`
+> `= force | ht | noirq | <boolean> | verbose`
 
 **String**, or **Boolean** to disable.
 
-The **acpi** option is used to control a set of four related boolean
-flags; `acpi_force`, `acpi_ht`, `acpi_noirq` and `acpi_disabled`.
-
 By default, Xen will scan the DMI data and blacklist certain systems
 which are known to have broken ACPI setups.  Providing `acpi=force`
 will cause Xen to ignore the blacklist and attempt to use all ACPI
@@ -97,12 +94,15 @@ which requires this option to function s
 Additionally, this will not prevent Xen from finding IO-APIC entries
 from the MP tables.
 
-Finally, any of the boolean false options can be used to disable ACPI
+Further, any of the boolean false options can be used to disable ACPI
 usage entirely.
 
 Because responsibility for ACPI processing is shared between Xen and
 the domain 0 kernel this option is automatically propagated to the
-domain 0 command line
+domain 0 command line.
+
+Finally, `acpi=verbose` will enable per-processor information logging
+which may otherwise be too noisy in particular on large systems.
 
 ### acpi_apic_instance
 > `= <integer>`
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -78,6 +78,7 @@ unsigned long __read_mostly cr4_pv32_mas
 /* "acpi=force":  Override the disable blacklist.                   */
 /* "acpi=ht":     Limit ACPI just to boot-time to enable HT.        */
 /* "acpi=noirq":  Disables ACPI interrupt routing.                  */
+/* "acpi=verbose": Enables more verbose ACPI boot time logging.     */
 static int parse_acpi_param(const char *s);
 custom_param("acpi", parse_acpi_param);
 
@@ -216,9 +217,6 @@ static char __initdata acpi_param[10] =
 
 static int __init parse_acpi_param(const char *s)
 {
-    /* Save the parameter so it can be propagated to domain0. */
-    safe_strcpy(acpi_param, s);
-
     /* Interpret the parameter for use within Xen. */
     if ( !parse_bool(s, NULL) )
     {
@@ -240,9 +238,17 @@ static int __init parse_acpi_param(const
     {
         acpi_noirq_set();
     }
+    else if ( !strcmp(s, "verbose") )
+    {
+        acpi_verbose = true;
+        return 0;
+    }
     else
         return -EINVAL;
 
+    /* Save the parameter so it can be propagated to domain0. */
+    safe_strcpy(acpi_param, s);
+
     return 0;
 }
 
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -230,8 +230,10 @@ acpi_numa_x2apic_affinity_init(const str
 	apicid_to_node[pa->apic_id] = node;
 	node_set(node, processor_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC %08x -> Node %u\n",
-	       pxm, pa->apic_id, node);
+
+	if (acpi_verbose)
+		printk(KERN_INFO "SRAT: PXM %u -> APIC %08x -> Node %u\n",
+		       pxm, pa->apic_id, node);
 }
 
 /* Callback for Proximity Domain -> LAPIC mapping */
@@ -263,8 +265,10 @@ acpi_numa_processor_affinity_init(const
 	apicid_to_node[pa->apic_id] = node;
 	node_set(node, processor_nodes_parsed);
 	acpi_numa = 1;
-	printk(KERN_INFO "SRAT: PXM %u -> APIC %02x -> Node %u\n",
-	       pxm, pa->apic_id, node);
+
+	if (acpi_verbose)
+		printk(KERN_INFO "SRAT: PXM %u -> APIC %02x -> Node %u\n",
+		       pxm, pa->apic_id, node);
 }
 
 /* Callback for parsing of the Proximity Domain <-> Memory Area mappings */
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -36,6 +36,8 @@
 
 #define ACPI_MAX_TABLES		128
 
+bool __initdata acpi_verbose;
+
 static const char *__initdata
 mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" };
 static const char *__initdata
@@ -51,6 +53,7 @@ void __init acpi_table_print_madt_entry(
 	switch (header->type) {
 
 	case ACPI_MADT_TYPE_LOCAL_APIC:
+		if (acpi_verbose)
 		{
 			struct acpi_madt_local_apic *p =
 			    (struct acpi_madt_local_apic *)header;
@@ -62,6 +65,7 @@ void __init acpi_table_print_madt_entry(
 		break;
 
 	case ACPI_MADT_TYPE_LOCAL_X2APIC:
+		if (acpi_verbose)
 		{
 			struct acpi_madt_local_x2apic *p =
 			    (struct acpi_madt_local_x2apic *)header;
@@ -115,6 +119,7 @@ void __init acpi_table_print_madt_entry(
 		break;
 
 	case ACPI_MADT_TYPE_LOCAL_APIC_NMI:
+		if (acpi_verbose)
 		{
 			struct acpi_madt_local_apic_nmi *p =
 			    (struct acpi_madt_local_apic_nmi *)header;
@@ -128,6 +133,7 @@ void __init acpi_table_print_madt_entry(
 		break;
 
 	case ACPI_MADT_TYPE_LOCAL_X2APIC_NMI:
+		if (acpi_verbose)
 		{
 			u16 polarity, trigger;
 			struct acpi_madt_local_x2apic_nmi *p =
@@ -167,6 +173,7 @@ void __init acpi_table_print_madt_entry(
 		break;
 
 	case ACPI_MADT_TYPE_LOCAL_SAPIC:
+		if (acpi_verbose)
 		{
 			struct acpi_madt_local_sapic *p =
 			    (struct acpi_madt_local_sapic *)header;
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -53,6 +53,8 @@
 
 extern acpi_physical_address rsdp_hint;
 
+extern bool acpi_verbose;
+
 enum acpi_interrupt_id {
 	ACPI_INTERRUPT_PMI	= 1,
 	ACPI_INTERRUPT_INIT,


[PATCH 2/4] x86/ACPI: don't overwrite FADT
Posted by Jan Beulich 3 years, 2 months ago
When marking fields invalid for our own purposes, we should do so in our
local copy (so we will notice later on), not in the firmware provided
one (which another entity may want to look at again, e.g. after kexec).
Also mark the function parameter const to notice such issues right away.

Instead use the pointer at the firmware copy for specifying an adjacent
printk()'s arguments. If nothing else this at least reduces the number
of relocations the assembler hasto emit and the linker has to process.

Fixes: 62d1a69a4e9f ("ACPI: support v5 (reduced HW) sleep interface")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -349,7 +349,7 @@ static int __init acpi_invalidate_bgrt(s
 
 /* Get pm1x_cnt and pm1x_evt information for ACPI sleep */
 static void __init
-acpi_fadt_parse_sleep_info(struct acpi_table_fadt *fadt)
+acpi_fadt_parse_sleep_info(const struct acpi_table_fadt *fadt)
 {
 	struct acpi_table_facs *facs = NULL;
 	uint64_t facs_pa;
@@ -362,10 +362,10 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 		printk(KERN_INFO PREFIX
 		       "v5 SLEEP INFO: control[%d:%"PRIx64"],"
 		       " status[%d:%"PRIx64"]\n",
-		       acpi_sinfo.sleep_control.space_id,
-		       acpi_sinfo.sleep_control.address,
-		       acpi_sinfo.sleep_status.space_id,
-		       acpi_sinfo.sleep_status.address);
+		       fadt->sleep_control.space_id,
+		       fadt->sleep_control.address,
+		       fadt->sleep_status.space_id,
+		       fadt->sleep_status.address);
 
 		if ((fadt->sleep_control.address &&
 		     (fadt->sleep_control.bit_offset ||
@@ -384,8 +384,8 @@ acpi_fadt_parse_sleep_info(struct acpi_t
 			       fadt->sleep_status.bit_offset,
 			       fadt->sleep_status.bit_width,
 			       fadt->sleep_status.access_width);
-			fadt->sleep_control.address = 0;
-			fadt->sleep_status.address = 0;
+			acpi_sinfo.sleep_control.address = 0;
+			acpi_sinfo.sleep_status.address = 0;
 		}
 	}
 


[PATCH 3/4] ACPI: replace casts by container_of()
Posted by Jan Beulich 3 years, 2 months ago
The latter is slightly more type-safe. Also add const where possible,
including without need to touch further code. Additionally replace an
adjacent unnecessary use of u16.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Intentionally not paying attention to line length in acpi/tables.c, as
adjacent code violates the limit already quite heavily.

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -63,9 +63,8 @@ static u64 acpi_lapic_addr __initdata =
 
 static int __init acpi_parse_madt(struct acpi_table_header *table)
 {
-	struct acpi_table_madt *madt;
-
-	madt = (struct acpi_table_madt *)table;
+	struct acpi_table_madt *madt =
+		container_of(table, struct acpi_table_madt, header);
 
 	if (madt->address) {
 		acpi_lapic_addr = (u64) madt->address;
@@ -277,7 +276,8 @@ acpi_parse_nmi_src(struct acpi_subtable_
 
 static int __init acpi_parse_hpet(struct acpi_table_header *table)
 {
-	struct acpi_table_hpet *hpet_tbl = (struct acpi_table_hpet *)table;
+	const struct acpi_table_hpet *hpet_tbl =
+		container_of(table, const struct acpi_table_hpet, header);
 
 	if (hpet_tbl->address.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) {
 		printk(KERN_WARNING PREFIX "HPET timers must be located in "
@@ -471,7 +471,8 @@ acpi_fadt_parse_sleep_info(const struct
 
 static int __init acpi_parse_fadt(struct acpi_table_header *table)
 {
-	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
+	const struct acpi_table_fadt *fadt =
+		container_of(table, const struct acpi_table_fadt, header);
 
 #ifdef	CONFIG_ACPI_INTERPRETER
 	/* initialize sci_int early for INT_SRC_OVR MADT parsing */
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -55,8 +55,9 @@ void __init acpi_table_print_madt_entry(
 	case ACPI_MADT_TYPE_LOCAL_APIC:
 		if (acpi_verbose)
 		{
-			struct acpi_madt_local_apic *p =
-			    (struct acpi_madt_local_apic *)header;
+			const struct acpi_madt_local_apic *p =
+			    container_of(header, const struct acpi_madt_local_apic, header);
+
 			printk(KERN_INFO PREFIX
 			       "LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
 			       p->processor_id, p->id,
@@ -67,8 +68,9 @@ void __init acpi_table_print_madt_entry(
 	case ACPI_MADT_TYPE_LOCAL_X2APIC:
 		if (acpi_verbose)
 		{
-			struct acpi_madt_local_x2apic *p =
-			    (struct acpi_madt_local_x2apic *)header;
+			const struct acpi_madt_local_x2apic *p =
+			    container_of(header, const struct acpi_madt_local_x2apic, header);
+
 			printk(KERN_INFO PREFIX
 			       "X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
 			       p->local_apic_id, p->uid,
@@ -79,8 +81,9 @@ void __init acpi_table_print_madt_entry(
 
 	case ACPI_MADT_TYPE_IO_APIC:
 		{
-			struct acpi_madt_io_apic *p =
-			    (struct acpi_madt_io_apic *)header;
+			const struct acpi_madt_io_apic *p =
+			    container_of(header, const struct acpi_madt_io_apic, header);
+
 			printk(KERN_INFO PREFIX
 			       "IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
 			       p->id, p->address, p->global_irq_base);
@@ -89,8 +92,9 @@ void __init acpi_table_print_madt_entry(
 
 	case ACPI_MADT_TYPE_INTERRUPT_OVERRIDE:
 		{
-			struct acpi_madt_interrupt_override *p =
-			    (struct acpi_madt_interrupt_override *)header;
+			const struct acpi_madt_interrupt_override *p =
+			    container_of(header, const struct acpi_madt_interrupt_override, header);
+
 			printk(KERN_INFO PREFIX
 			       "INT_SRC_OVR (bus %d bus_irq %d global_irq %d %s %s)\n",
 			       p->bus, p->source_irq, p->global_irq,
@@ -108,8 +112,9 @@ void __init acpi_table_print_madt_entry(
 
 	case ACPI_MADT_TYPE_NMI_SOURCE:
 		{
-			struct acpi_madt_nmi_source *p =
-			    (struct acpi_madt_nmi_source *)header;
+			const struct acpi_madt_nmi_source *p =
+			    container_of(header, const struct acpi_madt_nmi_source, header);
+
 			printk(KERN_INFO PREFIX
 			       "NMI_SRC (%s %s global_irq %d)\n",
 			       mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
@@ -121,8 +126,9 @@ void __init acpi_table_print_madt_entry(
 	case ACPI_MADT_TYPE_LOCAL_APIC_NMI:
 		if (acpi_verbose)
 		{
-			struct acpi_madt_local_apic_nmi *p =
-			    (struct acpi_madt_local_apic_nmi *)header;
+			const struct acpi_madt_local_apic_nmi *p =
+			    container_of(header, const struct acpi_madt_local_apic_nmi, header);
+
 			printk(KERN_INFO PREFIX
 			       "LAPIC_NMI (acpi_id[0x%02x] %s %s lint[%#x])\n",
 			       p->processor_id,
@@ -135,12 +141,10 @@ void __init acpi_table_print_madt_entry(
 	case ACPI_MADT_TYPE_LOCAL_X2APIC_NMI:
 		if (acpi_verbose)
 		{
-			u16 polarity, trigger;
-			struct acpi_madt_local_x2apic_nmi *p =
-			    (struct acpi_madt_local_x2apic_nmi *)header;
-
-			polarity = p->inti_flags & ACPI_MADT_POLARITY_MASK;
-			trigger = (p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2;
+			const struct acpi_madt_local_x2apic_nmi *p =
+			    container_of(header, const struct acpi_madt_local_x2apic_nmi, header);
+			unsigned int polarity = MASK_EXTR(p->inti_flags, ACPI_MADT_POLARITY_MASK);
+			unsigned int trigger = MASK_EXTR(p->inti_flags, ACPI_MADT_TRIGGER_MASK);
 
 			printk(KERN_INFO PREFIX
 			       "X2APIC_NMI (uid[0x%02x] %s %s lint[%#x])\n",
@@ -153,8 +157,9 @@ void __init acpi_table_print_madt_entry(
 
 	case ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE:
 		{
-			struct acpi_madt_local_apic_override *p =
-			    (struct acpi_madt_local_apic_override *)header;
+			const struct acpi_madt_local_apic_override *p =
+			    container_of(header, const struct acpi_madt_local_apic_override, header);
+
 			printk(KERN_INFO PREFIX
 			       "LAPIC_ADDR_OVR (address[%p])\n",
 			       (void *)(unsigned long)p->address);
@@ -163,8 +168,9 @@ void __init acpi_table_print_madt_entry(
 
 	case ACPI_MADT_TYPE_IO_SAPIC:
 		{
-			struct acpi_madt_io_sapic *p =
-			    (struct acpi_madt_io_sapic *)header;
+			const struct acpi_madt_io_sapic *p =
+			    container_of(header, const struct acpi_madt_io_sapic, header);
+
 			printk(KERN_INFO PREFIX
 			       "IOSAPIC (id[%#x] address[%p] gsi_base[%d])\n",
 			       p->id, (void *)(unsigned long)p->address,
@@ -175,8 +181,9 @@ void __init acpi_table_print_madt_entry(
 	case ACPI_MADT_TYPE_LOCAL_SAPIC:
 		if (acpi_verbose)
 		{
-			struct acpi_madt_local_sapic *p =
-			    (struct acpi_madt_local_sapic *)header;
+			const struct acpi_madt_local_sapic *p =
+			    container_of(header, const struct acpi_madt_local_sapic, header);
+
 			printk(KERN_INFO PREFIX
 			       "LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
 			       p->processor_id, p->id, p->eid,
@@ -186,8 +193,9 @@ void __init acpi_table_print_madt_entry(
 
 	case ACPI_MADT_TYPE_INTERRUPT_SOURCE:
 		{
-			struct acpi_madt_interrupt_source *p =
-			    (struct acpi_madt_interrupt_source *)header;
+			const struct acpi_madt_interrupt_source *p =
+			    container_of(header, const struct acpi_madt_interrupt_source, header);
+
 			printk(KERN_INFO PREFIX
 			       "PLAT_INT_SRC (%s %s type[%#x] id[0x%04x] eid[%#x] iosapic_vector[%#x] global_irq[%#x]\n",
 			       mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],


[PATCH 4/4] x86: drop fake CONFIG_{HPET,X86_PM}_TIMER
Posted by Jan Beulich 3 years, 2 months ago
I don't think we mean to ever make them real Kconfig options, so let's
just do away with them.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -37,9 +37,7 @@
 #include <asm/io.h>
 #include <asm/mpspec.h>
 #include <asm/processor.h>
-#ifdef CONFIG_HPET_TIMER
 #include <asm/hpet.h> /* for hpet_address */
-#endif
 #include <mach_apic.h>
 #include <mach_mpparse.h>
 
@@ -272,8 +270,6 @@ acpi_parse_nmi_src(struct acpi_subtable_
 	return 0;
 }
 
-#ifdef CONFIG_HPET_TIMER
-
 static int __init acpi_parse_hpet(struct acpi_table_header *table)
 {
 	const struct acpi_table_hpet *hpet_tbl =
@@ -309,9 +305,6 @@ static int __init acpi_parse_hpet(struct
 
 	return 0;
 }
-#else
-#define	acpi_parse_hpet	NULL
-#endif
 
 static int __init acpi_invalidate_bgrt(struct acpi_table_header *table)
 {
@@ -484,7 +477,6 @@ static int __init acpi_parse_fadt(struct
 	    fadt->force_apic_physical_destination_mode;
 #endif
 
-#ifdef CONFIG_X86_PM_TIMER
 	/* detect the location of the ACPI PM Timer */
 	if (fadt->header.revision >= FADT2_REVISION_ID &&
 	    fadt->xpm_timer_block.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
@@ -517,7 +509,6 @@ static int __init acpi_parse_fadt(struct
 	if (pmtmr_ioport)
 		printk(KERN_INFO PREFIX "PM-Timer IO Port: %#x (%u bits)\n",
 		       pmtmr_ioport, pmtmr_width);
-#endif
 
 	acpi_smi_cmd       = fadt->smi_command;
 	acpi_enable_value  = fadt->acpi_enable;
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -19,8 +19,6 @@
 
 #define BITS_PER_XEN_ULONG BITS_PER_LONG
 
-#define CONFIG_X86_PM_TIMER 1
-#define CONFIG_HPET_TIMER 1
 #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
 #define CONFIG_DISCONTIGMEM 1
 #define CONFIG_NUMA_EMU 1