diff mbox

fwts/madt: Add processor UID checking to madt tests

Message ID 1469198040-30887-1-git-send-email-prarit@redhat.com
State Rejected
Headers show

Commit Message

Prarit Bhargava July 22, 2016, 2:34 p.m. UTC 
The current madt test does not do any processor UID comparisons.  They are
marked as TODOs in the file.

This patchset implements two ACPI searches (one for old-style Processor
Declaration operator, and another for the new Processor Device operator)
that populate a list of UID entries, which can be queried for each MADT
struct's UID.

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 src/acpi/madt/madt.c |  132 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 107 insertions(+), 25 deletions(-)

Comments

Prarit Bhargava July 23, 2016, 1:30 p.m. UTC | #1 
On 07/22/2016 10:34 AM, Prarit Bhargava wrote:
> The current madt test does not do any processor UID comparisons.  They are
> marked as TODOs in the file.
> 
> This patchset implements two ACPI searches (one for old-style Processor
> Declaration operator, and another for the new Processor Device operator)
> that populate a list of UID entries, which can be queried for each MADT
> struct's UID.

Self-NAK.  While the UID code is correct the way I've handled the LSAPIC is
not.  There are three fields to examine in the LSAPIC table, the
acpi_processor_id, the uid_value, and its string representation uid_string.

acpi_processor_id, uid_value, and atoi(uid_string) should all evaluate to
the same value for a given Processor Device operator.  However, that
is NOT true for older tables with Processor Declaration operators.

I'll have to rethink this patch to get the LSAPIC evaluation correct.  It might
be as simple as doing something like:

	madt_find_processor_uid(acpi_processor_id);
	if ((acpi_processor_id == uid_value)) {
		if (uid_value == atoi(uid_string))
			madt_find_processor_uid(uid_value);
		else
			fwts_failed();
	} else
		fwts_failed();

Also ... I'm not sure I agree with this TODO comment in the LSAPIC code:

        /*
         * TODO: should the processor ID field be zero?  The use of
         * the Processor object has been deprecated in 6.0, and this
         * field is used to match this local SAPIC to the processor
         * via the ID.  This has been replaced, I believe, with the
         * processor device object _UID result.  On the other hand,
         * perhaps older tables are still using Processor objects....
         */

I think the above comment may come from the uid_string length being
greater or equal to one (this allows for the string to be empty and null
terminated), not uid_value being greater than or equal to one (see ACPI 6.0
Specification Table 5-56 Processor Local SAPIC Structure).

P.
diff mbox

Patch

diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c
index e260222a8b75..0d21fb2a2180 100644
--- a/src/acpi/madt/madt.c
+++ b/src/acpi/madt/madt.c
@@ -17,6 +17,8 @@ 
 
 #if defined(FWTS_HAS_ACPI)
 
+#include "fwts_acpi_object_eval.h"
+
 #include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -203,6 +205,95 @@  static fwts_acpi_table_info *ftable;
 
 static fwts_list msi_frame_ids;
 static fwts_list its_ids;
+static fwts_list processor_uids;
+
+struct acpi_processor {
+	ACPI_OBJECT_TYPE type;
+	uint32_t proc_id;
+	ACPI_IO_ADDRESS pblk_address;
+	uint32_t pblk_length;
+};
+
+struct acpi_integer {
+	ACPI_OBJECT_TYPE type;
+	uint64_t value;
+};
+
+struct list_integer {
+	uint64_t value;
+};
+
+static ACPI_STATUS madt_processor_handler(ACPI_HANDLE ObjHandle, uint32_t level,
+					  void *context, void **returnvalue)
+{
+	ACPI_OBJECT_TYPE acpi_type;
+	ACPI_STATUS status;
+	struct acpi_processor processor;
+	struct acpi_integer integer;
+	struct acpi_buffer pbuf = {sizeof(struct acpi_processor), &processor};
+	struct acpi_buffer ibuf = {sizeof(struct acpi_integer), &integer};
+	struct list_integer *listint;
+
+	/* Prevent -Werror=unused-parameter from complaining */
+	FWTS_UNUSED(level);
+	FWTS_UNUSED(context);
+	FWTS_UNUSED(returnvalue);
+
+	listint = malloc(sizeof(struct list_integer));
+	if (!listint)
+		return (!AE_OK);
+
+	status = AcpiGetType(ObjHandle, &acpi_type);
+	if (ACPI_FAILURE(status))
+		return (!AE_OK);
+
+	switch(acpi_type) {
+		case ACPI_TYPE_PROCESSOR:
+			status = AcpiEvaluateObject(ObjHandle, NULL, NULL,
+						    &pbuf);
+			if (ACPI_FAILURE(status))
+				return status;
+			listint->value = processor.proc_id;
+			break;
+		case ACPI_TYPE_DEVICE:
+			status = AcpiEvaluateObject(ObjHandle, "_UID", NULL,
+						     &ibuf);
+			if (ACPI_FAILURE(status))
+				return status;
+			listint->value = integer.value;
+			break;
+		default:
+			return (!AE_OK);
+	}
+	fwts_list_append(&processor_uids, listint);
+
+	return (AE_OK);
+}
+
+static void madt_find_processor_uid(fwts_framework *fw, uint64_t uid,
+				    char *table_name)
+{
+	char table_label[64];
+	fwts_list_link *item;
+	struct list_integer *listint;
+
+	/* TODO: is UID = 0 an error? */
+
+	fwts_list_foreach(item, &processor_uids) {
+		listint = fwts_list_data(struct list_integer *, item);
+		if (uid == listint->value) {
+			fwts_passed(fw, "MADT %s has matching processor "
+				    "UID %lu.", table_name, uid);
+			return;
+		}
+	}
+
+	sprintf(table_label, "MADT%sUidMismatch", table_name);
+	fwts_failed(fw, LOG_LEVEL_MEDIUM,
+		    table_label, "%s has no matching processor UID %lu",
+		    table_name, uid);
+	return;
+}
 
 static int madt_init(fwts_framework *fw)
 {
@@ -270,6 +361,14 @@  static int madt_init(fwts_framework *fw)
 	 */
 	fwts_list_init(&msi_frame_ids);
 	fwts_list_init(&its_ids);
+	fwts_list_init(&processor_uids);
+
+	if (fwts_acpica_init(fw) != FWTS_OK)
+		return FWTS_ERROR;
+
+	AcpiWalkNamespace(0x0c, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
+			  madt_processor_handler, NULL, NULL, NULL);
+	AcpiGetDevices("ACPI0007", madt_processor_handler, NULL, NULL);
 
 	return (spec_data) ? FWTS_OK : FWTS_ERROR;
 }
@@ -412,11 +511,7 @@  static int madt_local_apic(fwts_framework *fw,
 	fwts_acpi_madt_processor_local_apic *lapic =
 		(fwts_acpi_madt_processor_local_apic *)data;
 
-	/*
-	 *  TODO: verify UID field has a corresponding
-	 *  Processor device object with a matching
-	 *  _UID result
-	 */
+	madt_find_processor_uid(fw, lapic->acpi_processor_id, "LAPIC");
 
 	if (lapic->flags & 0xfffffffe)
 		fwts_failed(fw, LOG_LEVEL_MEDIUM,
@@ -527,11 +622,7 @@  static int madt_local_apic_nmi(fwts_framework *fw,
 	fwts_acpi_madt_local_apic_nmi *nmi =
 		(fwts_acpi_madt_local_apic_nmi *)data;
 
-	/*
-	 *  TODO: verify UID field has a corresponding
-	 *  Processor device object with a matching
-	 *  _UID result
-	 */
+	madt_find_processor_uid(fw, nmi->acpi_processor_id, "LAPICNMI");
 
 	if (nmi->flags & 0xfffffff0)
 		fwts_failed(fw, LOG_LEVEL_MEDIUM,
@@ -693,11 +784,7 @@  static int madt_local_sapic(fwts_framework *fw,
 			    "reserved and properly set to zero.",
 			    madt_sub_names[hdr->type]);
 
-	/*
-	 *  TODO: verify UID field has a corresponding
-	 *  Processor device object with a matching
-	 *  _UID result (or Processor object ID?)
-	 */
+	madt_find_processor_uid(fw, lsapic->acpi_processor_id, "LSAPIC");
 
 	for (tmp = 0, ii = 0; ii < (int)strlen(lsapic->uid_string); ii++)
 		if (isascii(lsapic->uid_string[ii]))
@@ -817,11 +904,7 @@  static int madt_local_x2apic(fwts_framework *fw,
 			    "reserved and properly set to zero.",
 			    madt_sub_names[hdr->type]);
 
-	/*
-	 *  TODO: verify UID field has a corresponding
-	 *  Processor device object with a matching
-	 *  _UID result
-	 */
+	madt_find_processor_uid(fw, lx2apic->processor_uid, "X2APIC");
 
 	return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header));
 }
@@ -869,11 +952,7 @@  static int madt_gicc(fwts_framework *fw,
 			    "MADT %s reserved field properly set to zero.",
 			    madt_sub_names[hdr->type]);
 
-	/*
-	 *  TODO: verify UID field has a corresponding
-	 *  Processor device object with a matching
-	 *  _UID result
-	 */
+	madt_find_processor_uid(fw, gic->processor_uid, "GICC");
 
 	mask = 0xfffffffc;
 	start = 2;
@@ -1395,9 +1474,12 @@  static int madt_subtables(fwts_framework *fw)
 
 static int madt_deinit(fwts_framework *fw)
 {
+	fwts_acpica_deinit();
+
 	/* only minor clean up needed */
 	fwts_list_free_items(&msi_frame_ids, NULL);
 	fwts_list_free_items(&its_ids, NULL);
+	fwts_list_free_items(&processor_uids, NULL);
 
 	return (fw) ? FWTS_ERROR : FWTS_OK;
 }