Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
Signed-off-by: Hu Tao hutao@cn.fujitsu.com ---
When testing memory hotplug patchset, I found the PkgLength calculation will cause windows 2012, Windows 2008 BSOD (but not windows XP). After fixing it, the problem is resolved.
src/acpi.c | 48 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index ce988e0..ae28427 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -216,10 +216,34 @@ static inline char getHex(u32 val) { return (val <= 9) ? ('0' + val) : ('A' + val - 10); }
+int getBytes(int length, int included) +{ + int nbyte; + + if (length < (included ? 64 : 64 - 1)) { + nbyte = 1; + } else if (length < (included ? 4096 : 4096 - 2)) { + nbyte = 2; + } else if (length < (included ? 1048576 : 1048576 - 3)) { + nbyte = 3; + } else { + nbyte = 4; + } + + return nbyte; +} + // Encode a length in an SSDT. +// PkgLength includes itself, but the bytes(PkgLeadByte + ByteData) +// depends on length of following data. This function calculates the +// bytes, given the @length of following data. static u8 * -encodeLen(u8 *ssdt_ptr, int length, int bytes) +encodeLen(u8 *ssdt_ptr, int length, int included) { + int bytes = getBytes(length, included); + if (!included) + length += bytes; + switch (bytes) { default: case 4: ssdt_ptr[3] = ((length >> 20) & 0xff); @@ -265,15 +289,15 @@ build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, count -= skip;
*(ssdt_ptr++) = 0x14; // MethodOp - ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*count), 2); + ssdt_ptr = encodeLen(ssdt_ptr, 4+1+(12*count), 0); memcpy(ssdt_ptr, name, 4); ssdt_ptr += 4; - *(ssdt_ptr++) = 0x02; // MethodOp + *(ssdt_ptr++) = 0x02; // MethodFlags
int i; for (i = skip; count-- > 0; i++) { *(ssdt_ptr++) = 0xA0; // IfOp - ssdt_ptr = encodeLen(ssdt_ptr, 11, 1); + ssdt_ptr = encodeLen(ssdt_ptr, 10, 0); *(ssdt_ptr++) = 0x93; // LEqualOp *(ssdt_ptr++) = 0x68; // Arg0Op *(ssdt_ptr++) = 0x0A; // BytePrefix @@ -311,13 +335,13 @@ build_ssdt(void) { int acpi_cpus = MaxCountCPUs > 0xff ? 0xff : MaxCountCPUs; int length = (sizeof(ssdp_misc_aml) // _S3_ / _S4_ / _S5_ - + (1+3+4) // Scope(_SB_) + + (1+2+4) // Scope(_SB_) + (acpi_cpus * PROC_SIZEOF) // procs - + (1+2+5+(12*acpi_cpus)) // NTFY - + (6+2+1+(1*acpi_cpus)) // CPON - + (1+3+4) // Scope(PCI0) + + (1+1+5+(12*acpi_cpus)) // NTFY + + (6+1+1+(1*acpi_cpus)) // CPON + + (1+2+4) // Scope(PCI0) + ((PCI_SLOTS - 1) * PCIHP_SIZEOF) // slots - + (1+2+5+(12*(PCI_SLOTS - 1)))); // PCNT + + (1+1+5+(12*(PCI_SLOTS - 1))+1)); // PCNT u8 *ssdt = malloc_high(length); if (! ssdt) { warn_noalloc(); @@ -359,7 +383,7 @@ build_ssdt(void)
// build Scope(_SB_) header *(ssdt_ptr++) = 0x10; // ScopeOp - ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3); + ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 1); *(ssdt_ptr++) = '_'; *(ssdt_ptr++) = 'S'; *(ssdt_ptr++) = 'B'; @@ -387,14 +411,14 @@ build_ssdt(void) *(ssdt_ptr++) = 'O'; *(ssdt_ptr++) = 'N'; *(ssdt_ptr++) = 0x12; // PackageOp - ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2); + ssdt_ptr = encodeLen(ssdt_ptr, 1+(1*acpi_cpus), 0); *(ssdt_ptr++) = acpi_cpus; for (i=0; i<acpi_cpus; i++) *(ssdt_ptr++) = (apic_id_is_present(i)) ? 0x01 : 0x00;
// build Scope(PCI0) opcode *(ssdt_ptr++) = 0x10; // ScopeOp - ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3); + ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 1); *(ssdt_ptr++) = 'P'; *(ssdt_ptr++) = 'C'; *(ssdt_ptr++) = 'I';
On Wed, Jul 03, 2013 at 05:59:07PM +0800, Hu Tao wrote:
Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
Signed-off-by: Hu Tao hutao@cn.fujitsu.com
When testing memory hotplug patchset, I found the PkgLength calculation will cause windows 2012, Windows 2008 BSOD (but not windows XP). After fixing it, the problem is resolved.
This issue was only seen with the memory hotplug patches correct?
src/acpi.c | 48 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index ce988e0..ae28427 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -216,10 +216,34 @@ static inline char getHex(u32 val) { return (val <= 9) ? ('0' + val) : ('A' + val - 10); }
+int getBytes(int length, int included) +{
- int nbyte;
- if (length < (included ? 64 : 64 - 1)) {
nbyte = 1;- } else if (length < (included ? 4096 : 4096 - 2)) {
nbyte = 2;- } else if (length < (included ? 1048576 : 1048576 - 3)) {
nbyte = 3;- } else {
nbyte = 4;- }
- return nbyte;
+}
// Encode a length in an SSDT. +// PkgLength includes itself, but the bytes(PkgLeadByte + ByteData) +// depends on length of following data. This function calculates the +// bytes, given the @length of following data. static u8 * -encodeLen(u8 *ssdt_ptr, int length, int bytes) +encodeLen(u8 *ssdt_ptr, int length, int included) {
- int bytes = getBytes(length, included);
- if (!included)
length += bytes;- switch (bytes) { default: case 4: ssdt_ptr[3] = ((length >> 20) & 0xff);
@@ -265,15 +289,15 @@ build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, count -= skip;
*(ssdt_ptr++) = 0x14; // MethodOp
- ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*count), 2);
- ssdt_ptr = encodeLen(ssdt_ptr, 4+1+(12*count), 0); memcpy(ssdt_ptr, name, 4); ssdt_ptr += 4;
- *(ssdt_ptr++) = 0x02; // MethodOp
*(ssdt_ptr++) = 0x02; // MethodFlags
int i; for (i = skip; count-- > 0; i++) { *(ssdt_ptr++) = 0xA0; // IfOp
ssdt_ptr = encodeLen(ssdt_ptr, 11, 1);
ssdt_ptr = encodeLen(ssdt_ptr, 10, 0); *(ssdt_ptr++) = 0x93; // LEqualOp *(ssdt_ptr++) = 0x68; // Arg0Op *(ssdt_ptr++) = 0x0A; // BytePrefix@@ -311,13 +335,13 @@ build_ssdt(void) { int acpi_cpus = MaxCountCPUs > 0xff ? 0xff : MaxCountCPUs; int length = (sizeof(ssdp_misc_aml) // _S3_ / _S4_ / _S5_
+ (1+3+4) // Scope(_SB_)
+ (1+2+4) // Scope(_SB_) + (acpi_cpus * PROC_SIZEOF) // procs
+ (1+2+5+(12*acpi_cpus)) // NTFY+ (6+2+1+(1*acpi_cpus)) // CPON+ (1+3+4) // Scope(PCI0)
+ (1+1+5+(12*acpi_cpus)) // NTFY+ (6+1+1+(1*acpi_cpus)) // CPON+ (1+2+4) // Scope(PCI0) + ((PCI_SLOTS - 1) * PCIHP_SIZEOF) // slots
+ (1+2+5+(12*(PCI_SLOTS - 1)))); // PCNT
+ (1+1+5+(12*(PCI_SLOTS - 1))+1)); // PCNT@@ -359,7 +383,7 @@ build_ssdt(void)
// build Scope(_SB_) header *(ssdt_ptr++) = 0x10; // ScopeOp
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3);
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 1); *(ssdt_ptr++) = '_'; *(ssdt_ptr++) = 'S'; *(ssdt_ptr++) = 'B';
@@ -387,14 +411,14 @@ build_ssdt(void) *(ssdt_ptr++) = 'O'; *(ssdt_ptr++) = 'N'; *(ssdt_ptr++) = 0x12; // PackageOp
- ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2);
ssdt_ptr = encodeLen(ssdt_ptr, 1+(1*acpi_cpus), 0); *(ssdt_ptr++) = acpi_cpus; for (i=0; i<acpi_cpus; i++) *(ssdt_ptr++) = (apic_id_is_present(i)) ? 0x01 : 0x00;
// build Scope(PCI0) opcode *(ssdt_ptr++) = 0x10; // ScopeOp
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3);
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 1); *(ssdt_ptr++) = 'P'; *(ssdt_ptr++) = 'C'; *(ssdt_ptr++) = 'I';
-- 1.8.3.1
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
On Sun, Jul 07, 2013 at 05:49:30PM +0300, Michael S. Tsirkin wrote:
On Wed, Jul 03, 2013 at 05:59:07PM +0800, Hu Tao wrote:
Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
Signed-off-by: Hu Tao hutao@cn.fujitsu.com
When testing memory hotplug patchset, I found the PkgLength calculation will cause windows 2012, Windows 2008 BSOD (but not windows XP). After fixing it, the problem is resolved.
This issue was only seen with the memory hotplug patches correct?
Not exactly. A slight change in the code, for example, changing the 3rd parameter of encodeLen from 1 to 2 that seems harmless can also cause the problem. In general, if (ssdt_ptr - ssdt) isn't equal to length (all three variables seen in build_ssdt()), the problem occurs.
src/acpi.c | 48 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index ce988e0..ae28427 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -216,10 +216,34 @@ static inline char getHex(u32 val) { return (val <= 9) ? ('0' + val) : ('A' + val - 10); }
+int getBytes(int length, int included) +{
- int nbyte;
- if (length < (included ? 64 : 64 - 1)) {
nbyte = 1;- } else if (length < (included ? 4096 : 4096 - 2)) {
nbyte = 2;- } else if (length < (included ? 1048576 : 1048576 - 3)) {
nbyte = 3;- } else {
nbyte = 4;- }
- return nbyte;
+}
// Encode a length in an SSDT. +// PkgLength includes itself, but the bytes(PkgLeadByte + ByteData) +// depends on length of following data. This function calculates the +// bytes, given the @length of following data. static u8 * -encodeLen(u8 *ssdt_ptr, int length, int bytes) +encodeLen(u8 *ssdt_ptr, int length, int included) {
- int bytes = getBytes(length, included);
- if (!included)
length += bytes;- switch (bytes) { default: case 4: ssdt_ptr[3] = ((length >> 20) & 0xff);
@@ -265,15 +289,15 @@ build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, count -= skip;
*(ssdt_ptr++) = 0x14; // MethodOp
- ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*count), 2);
- ssdt_ptr = encodeLen(ssdt_ptr, 4+1+(12*count), 0); memcpy(ssdt_ptr, name, 4); ssdt_ptr += 4;
- *(ssdt_ptr++) = 0x02; // MethodOp
*(ssdt_ptr++) = 0x02; // MethodFlags
int i; for (i = skip; count-- > 0; i++) { *(ssdt_ptr++) = 0xA0; // IfOp
ssdt_ptr = encodeLen(ssdt_ptr, 11, 1);
ssdt_ptr = encodeLen(ssdt_ptr, 10, 0); *(ssdt_ptr++) = 0x93; // LEqualOp *(ssdt_ptr++) = 0x68; // Arg0Op *(ssdt_ptr++) = 0x0A; // BytePrefix@@ -311,13 +335,13 @@ build_ssdt(void) { int acpi_cpus = MaxCountCPUs > 0xff ? 0xff : MaxCountCPUs; int length = (sizeof(ssdp_misc_aml) // _S3_ / _S4_ / _S5_
+ (1+3+4) // Scope(_SB_)
+ (1+2+4) // Scope(_SB_) + (acpi_cpus * PROC_SIZEOF) // procs
+ (1+2+5+(12*acpi_cpus)) // NTFY+ (6+2+1+(1*acpi_cpus)) // CPON+ (1+3+4) // Scope(PCI0)
+ (1+1+5+(12*acpi_cpus)) // NTFY+ (6+1+1+(1*acpi_cpus)) // CPON+ (1+2+4) // Scope(PCI0) + ((PCI_SLOTS - 1) * PCIHP_SIZEOF) // slots
+ (1+2+5+(12*(PCI_SLOTS - 1)))); // PCNT
+ (1+1+5+(12*(PCI_SLOTS - 1))+1)); // PCNT@@ -359,7 +383,7 @@ build_ssdt(void)
// build Scope(_SB_) header *(ssdt_ptr++) = 0x10; // ScopeOp
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3);
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 1); *(ssdt_ptr++) = '_'; *(ssdt_ptr++) = 'S'; *(ssdt_ptr++) = 'B';
@@ -387,14 +411,14 @@ build_ssdt(void) *(ssdt_ptr++) = 'O'; *(ssdt_ptr++) = 'N'; *(ssdt_ptr++) = 0x12; // PackageOp
- ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2);
ssdt_ptr = encodeLen(ssdt_ptr, 1+(1*acpi_cpus), 0); *(ssdt_ptr++) = acpi_cpus; for (i=0; i<acpi_cpus; i++) *(ssdt_ptr++) = (apic_id_is_present(i)) ? 0x01 : 0x00;
// build Scope(PCI0) opcode *(ssdt_ptr++) = 0x10; // ScopeOp
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3);
- ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 1); *(ssdt_ptr++) = 'P'; *(ssdt_ptr++) = 'C'; *(ssdt_ptr++) = 'I';
-- 1.8.3.1
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
On Wed, Jul 03, 2013 at 05:59:07PM +0800, Hu Tao wrote:
Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
What isn't acpi compliant with the current calculation?
-Kevin
On Sun, Jul 07, 2013 at 11:42:40PM -0400, Kevin O'Connor wrote:
On Wed, Jul 03, 2013 at 05:59:07PM +0800, Hu Tao wrote:
Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
What isn't acpi compliant with the current calculation?
Currently, the length of PkgLength itself is hard-coded(third parameter of encodeLen). If the length of PkgLength must have strictly the relationship with package length as follow as in ACPI specification:
package length PkgLength 0-63 1 byte 64-4096(2^12) 2 bytes 4097-1048576(2^20) 3 bytes
1048576 4 bytes
then current calculation is not acpi-compliant. But if not, say, a 4-byte PkgLength can legally encode a 10-byte package, then current calculation is acpi-compliant.
On Fri, Jul 12, 2013 at 10:08:01AM +0800, Hu Tao wrote:
On Sun, Jul 07, 2013 at 11:42:40PM -0400, Kevin O'Connor wrote:
On Wed, Jul 03, 2013 at 05:59:07PM +0800, Hu Tao wrote:
Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
What isn't acpi compliant with the current calculation?
Currently, the length of PkgLength itself is hard-coded(third parameter of encodeLen). If the length of PkgLength must have strictly the relationship with package length as follow as in ACPI specification:
package length PkgLength 0-63 1 byte 64-4096(2^12) 2 bytes 4097-1048576(2^20) 3 bytes
1048576 4 bytes
then current calculation is not acpi-compliant. But if not, say, a 4-byte PkgLength can legally encode a 10-byte package, then current calculation is acpi-compliant.
My read of the spec was that one can always use a larger length encoding than needed. I haven't seen any issues with it in my tests. Can you confirm that the length encoding is causing problems for you?
-Kevin
On Sun, Jul 14, 2013 at 09:12:42PM -0400, Kevin O'Connor wrote:
On Fri, Jul 12, 2013 at 10:08:01AM +0800, Hu Tao wrote:
On Sun, Jul 07, 2013 at 11:42:40PM -0400, Kevin O'Connor wrote:
On Wed, Jul 03, 2013 at 05:59:07PM +0800, Hu Tao wrote:
Currently the calculation of PkgLength isn't acpi-compliant. This patch fixes it to be acpi-compliant.
What isn't acpi compliant with the current calculation?
Currently, the length of PkgLength itself is hard-coded(third parameter of encodeLen). If the length of PkgLength must have strictly the relationship with package length as follow as in ACPI specification:
package length PkgLength 0-63 1 byte 64-4096(2^12) 2 bytes 4097-1048576(2^20) 3 bytes
1048576 4 bytes
then current calculation is not acpi-compliant. But if not, say, a 4-byte PkgLength can legally encode a 10-byte package, then current calculation is acpi-compliant.
My read of the spec was that one can always use a larger length encoding than needed. I haven't seen any issues with it in my tests. Can you confirm that the length encoding is causing problems for you?
You're right. I confirmed that the cause of Windows BSOD is wrong hard-coded package length, not bytes of PkgLength.
-
Hu Tao -
Kevin O'Connor -
Michael S. Tsirkin