On 03/18/09 16:36, Dana H. Myers wrote:
> Li, Aubrey wrote:
>> Dana.Myers wrote:
>>
>>
>>> Li, Aubrey wrote:
>>>
>>>> Liu, Jiang wrote:
>>>>
>>>>
>>>>
>>>>> Hi all.
>>>>> I have analyzed relative code with Aubrey, and could
>>>>> only give one
>>>>> possible explanation for the phenomenon. It may be caused by buggy
>>>>> ACPI BIOS implementation which returns non Package object on _CST
>>>>> evaluation, and current cpupm driver dosen't check whether the
>>>>> returned object is a
>>>>> package object before access it. So if a Integer object is returned,
>>>>> obj->Package.Elements will be NULL, then #GP happens.
>>>>> Otherwise I couldn't
>>>>> explain why "cnt = obj->Package.Elements[0].Integer.Value;" cause
>>>>> #GP. Thanks!
>>>>>
>>>> Right, we need to use AcpiEvaluateObjectTyped to call _CST with
>>>> ACPI_TYPE_PACKAGE if we run into this case.
>>>>
>>>>
>>> Agreed; that occurred to me this morning, that we're not
>>> checking the type of the returned object before accessing it,
>>> or using AcpiEvaluateObjectTyped(). So, yes, this is a necessary
>>> change, after we understand what it actually broken here.
>>>
>>>> But who knows how BIOS implements _CST, I'm waiting for the DSL file.
>>>>
>>>>
>>> So, now that I've had a chance to look at the ACPI BIOS from an M10,
>>> the issue is more interesting.
>>>
>>> There's no _CST object in the disassembly; the Toshiba BIOS uses the
>>> ASL 'Load' operator to dynamically load objects as part of the per-CPU
>>> _OSC object evaluation, which is where, I'm certain, the _CST objects
>>> are coming from.
>>> This makes inspecting the relevant BIOS code a little more difficult.
>>> I suggest that we first insert a kernel printf to tell us what type
>>> of object is returned from AcpiEvaluateObject(), to research this.
>>>
>>
>> Did you receive the tar.gz file I sent?
>>
>> _CST is already there in cs0/cs1.dsl. the dynamical table can be dump
>> out by acpidump. anyway, _CST call YCST eventually, which will return
>> the package we need.
>>
>>
> Actually, will YCST return the value we need? YCST is a series of:
>
> if (....) {
> return (Package())
> }
>
> blocks. It appears to me that there might be nothing returned by YCST,
> which is what we're apparently seeing based on investigating the return
> value from AcpiEvaluateObject(). buffer Length == 0.
>
> Dana
>
>>
>> This is weird because as I pointed out, the element ptr is just pointing
>> an offset of the buffer we passed. If the kernel really panic on this
>> line,
>> It's centernly not a ACPI CA bug. Whatever ACPI CA does can't change
>> the address. Does this make sense?
>>
> The case in which ACPI CA *could* be at fault (and I don't think it is
> now that
> we've had a chance to look into it) is that a smaller object is returned.
> We don't pass a buffer - it's allocated. It *could* be allocated
> smaller than
> expected, or not allocated at all - which is what is happening here.
>
> #1. AcpiEvaluateObjectTyped() will catch this error case and return
> an ACPI error.
> #2. Why is YCST apparently not returning a valid Package object?
>> But I doubt the panic place is really here, can the bug reporter send
>> the
>> panic stack here?
I am going to see what this change does on M9 & M10:
-bash-3.2$ diff cpu_acpi.c
/ws/onnv-gate/usr/src/uts/i86pc/os/cpupm/cpu_acpi.c
698,699c698,699
< if (ACPI_FAILURE(AcpiEvaluateObjectTyped(handle->cs_handle, "_CST",
< NULL, &abuf, ACPI_TYPE_PACKAGE))) {
---
> if (ACPI_FAILURE(AcpiEvaluateObject(handle->cs_handle, "_CST",
> NULL, &abuf))) {
703,706d702
< if (abuf.Length < sizeof (ACPI_OBJECT)) {
< cmn_err(CE_NOTE, "!cpu_acpi: _CST evaluate length failure");
< return (-1);
< }
Regards,
Bill
