| Message ID | 1476462922-9436-1-git-send-email-jhugo@codeaurora.org |
|---|---|
| State | Accepted |
| Headers | show |
On 14/10/16 17: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@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); > Good catch. Thanks Jeffrey Acked-by: Colin Ian King <colin.king@canonical.com>
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@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@canonical.com>
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);
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@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(-)