ACK: [PATCH] acpi: madt: Fix processor UID check

[ivanhu]

On 2016年10月15日 00:35, Jeffrey Hugo wrote:
> Commit 763737164714 ("fwts/madt: Add processor UID checking to madt tests")
> is broken on the Qualcomm Technologies QDF2432 and likely very fragile on
> other platforms as well.
>
> The root cause is that the added redefinition of the acpi_integer structure
> is not compatible with AcpiEvaluateObject() and can result in
> AE_BUFFER_OVERFLOW errors.
>
> AcpiEvaluateObject() ensures that the provided return buffer is large enough
> to hold the resulting object.  The size of the resulting object is
> determined via AcpiUtGetSimpleObjectSize() which uses ACPI_OBJECT as a
> baseline for the size of a resulting object (variable length objects like
> strings may be larger).  Per src/acpica/source/include/actypes.h ACPI_OBJECT
> is a union, and therefore the size of ACPI_OBJECT is atleast the size of its
> largest member.  The Integer member is not the largest member, thus an
> ACPI_OBJECT cannot fit into acpi_integer struct as defined in the madt test.
> On QDF2432, sizeof(acpi_integer) is 16, where as sizeof(ACPI_OBJECT) is 24.
> Since 24 > 16, AcpiEvaluateObject returns AE_BUFFER_OVERFLOW.
>
> This results in false test failures because the test has not properly parsed
> the processor device UIDs, and cannot later match the GICC UIDs to any
> processor UIDs.
>
> We fix this by not reinventing the wheel, and using ACPI_OBJECT as our
> output buffer for AcpiEvaluateObject (both Processor and Integer objects
> that we care about are fixed size), and parsing the union nativly for the
> values we care about.  Since the source object is also ACPI_OBJECT, the
> output buffer will always be of the same type, and thus the same size,
> preventing AE_BUFFER_OVERFLOW errors, particularly if ACPI_OBJECT grows in
> size in the future (ie an added member, or some stange compiler packing).
>
> Signed-off-by: Jeffrey Hugo <jhugo at codeaurora.org>
> ---
>
> I'm not entirely sure how this worked in the first place, but since Prarit
> origionally wrote this check, I'd appreciate if Prarit would test this fix
> to verify it doesn't cause a regression in their usecase.
>
>  src/acpi/madt/madt.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/src/acpi/madt/madt.c b/src/acpi/madt/madt.c
> index b087d27..6c7dee2 100644
> --- a/src/acpi/madt/madt.c
> +++ b/src/acpi/madt/madt.c
> @@ -232,13 +232,6 @@ 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;
> @@ -249,10 +242,8 @@ static ACPI_STATUS madt_processor_handler(ACPI_HANDLE ObjHandle, uint32_t level,
>  {
>  	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};
> +	ACPI_OBJECT obj;
> +	struct acpi_buffer buf = {sizeof(ACPI_OBJECT), &obj};
>  	struct acpi_integer *listint;
>
>  	/* Prevent -Werror=unused-parameter from complaining */
> @@ -272,20 +263,20 @@ static ACPI_STATUS madt_processor_handler(ACPI_HANDLE ObjHandle, uint32_t level,
>
>  	switch(acpi_type) {
>  	case ACPI_TYPE_PROCESSOR:
> -		status = AcpiEvaluateObject(ObjHandle, NULL, NULL, &pbuf);
> +		status = AcpiEvaluateObject(ObjHandle, NULL, NULL, &buf);
>  		if (ACPI_FAILURE(status)) {
>  			free(listint);
>  			return status;
>  		}
> -		listint->value = processor.proc_id;
> +		listint->value = obj.Processor.ProcId;
>  		break;
>  	case ACPI_TYPE_DEVICE:
> -		status = AcpiEvaluateObject(ObjHandle, "_UID", NULL, &ibuf);
> +		status = AcpiEvaluateObject(ObjHandle, "_UID", NULL, &buf);
>  		if (ACPI_FAILURE(status)) {
>  			free(listint);
>  			return status;
>  		}
> -		listint->value = integer.value;
> +		listint->value = obj.Integer.Value;
>  		break;
>  	default:
>  		free(listint);
>

Acked-by: Ivan Hu <ivan.hu at canonical.com>