[PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable

[PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable

[Vincent Minet]

External ACPI tables are counted twice for the RSDT size and the load
address for the first external table is in the MADT (interrupt override
entries are overwritten).

Signed-off-by: Vincent Minet <vincent@vincent-minet.net>
---
 kvm/bios/rombios32.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..289361b 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
     addr = base_addr = ram_size - ACPI_DATA_SIZE;
     rsdt_addr = addr;
     rsdt = (void *)(addr);
-    rsdt_size = sizeof(*rsdt) + external_tables * 4;
+    rsdt_size = sizeof(*rsdt);
     addr += rsdt_size;
 
     fadt_addr = addr;
@@ -1787,6 +1787,7 @@ void acpi_bios_init(void)
             }
             int_override++;
             madt_size += sizeof(struct madt_int_override);
+            addr += sizeof(struct madt_int_override);
         }
         acpi_build_table_header((struct acpi_table_header *)madt,
                                 "APIC", madt_size, 1);
-- 
1.6.3

Re: [PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable

[Anthony Liguori]

Vincent Minet wrote:
> External ACPI tables are counted twice for the RSDT size and the load
> address for the first external table is in the MADT (interrupt override
> entries are overwritten).
>
> Signed-off-by: Vincent Minet <vincent@vincent-minet.net>
>   

Beth,

I think you had a patch attempting to address the same issue.  It was a 
bit more involved though.

Which is the proper fix and are they both to the same problem?

Regards,

Anthony Liguori

> ---
>  kvm/bios/rombios32.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
> index cbd5f15..289361b 100755
> --- a/kvm/bios/rombios32.c
> +++ b/kvm/bios/rombios32.c
> @@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
>      addr = base_addr = ram_size - ACPI_DATA_SIZE;
>      rsdt_addr = addr;
>      rsdt = (void *)(addr);
> -    rsdt_size = sizeof(*rsdt) + external_tables * 4;
> +    rsdt_size = sizeof(*rsdt);
>      addr += rsdt_size;
>  
>      fadt_addr = addr;
> @@ -1787,6 +1787,7 @@ void acpi_bios_init(void)
>              }
>              int_override++;
>              madt_size += sizeof(struct madt_int_override);
> +            addr += sizeof(struct madt_int_override);
>          }
>          acpi_build_table_header((struct acpi_table_header *)madt,
>                                  "APIC", madt_size, 1);
>   

Re: [PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable

[Beth Kon]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

Anthony Liguori wrote:
> Vincent Minet wrote:
>> External ACPI tables are counted twice for the RSDT size and the load
>> address for the first external table is in the MADT (interrupt override
>> entries are overwritten).
>>
>> Signed-off-by: Vincent Minet <vincent@vincent-minet.net>
>>   
>
> Beth,
>
> I think you had a patch attempting to address the same issue.  It was 
> a bit more involved though.
>
> Which is the proper fix and are they both to the same problem?
They are for 2 different bases. My patch was for qemu's bochs bios and 
this is for qemu-kvm/kvm/bios/rombios32.c. They are pretty divergent in 
this area of setting up the ACPI tables. My patch is still needed for 
the qemu base. I hope we'll be getting to one base soon :-)

Assuming the intent of the code was for MAX_RSDT_ENTRIES to include 
external_tables, this patch looks correct. I think one additional check 
would be needed (in my patch) to make sure that the code doesn't exceed 
MAX_RSDT_ENTRIES when the external tables are being loaded.

My patch also puts all the code that calculates madt_size in the same 
place, at the beginning of the table layout. I believe this is neater 
and will avoid problems like this one in the future. As much as 
possible, I think it best to get all the tables layed out, then fill 
them in. If for some reason this is not acceptable, we need to add a big 
note that no tables should be layed out after the madt because the madt 
may grow further down in the code and overwrite the other table.
>


>
> Regards,
>
> Anthony Liguori
>
>> ---
>>  kvm/bios/rombios32.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>> index cbd5f15..289361b 100755
>> --- a/kvm/bios/rombios32.c
>> +++ b/kvm/bios/rombios32.c
>> @@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
>>      addr = base_addr = ram_size - ACPI_DATA_SIZE;
>>      rsdt_addr = addr;
>>      rsdt = (void *)(addr);
>> -    rsdt_size = sizeof(*rsdt) + external_tables * 4;
>> +    rsdt_size = sizeof(*rsdt);
>>      addr += rsdt_size;
>>  
>>      fadt_addr = addr;
>> @@ -1787,6 +1787,7 @@ void acpi_bios_init(void)
>>              }
>>              int_override++;
>>              madt_size += sizeof(struct madt_int_override);
>> +            addr += sizeof(struct madt_int_override);
>>          }
>>          acpi_build_table_header((struct acpi_table_header *)madt,
>>                                  "APIC", madt_size, 1);
>>   
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: acpi_tables_fix_2.patch --]
[-- Type: text/x-patch, Size: 2729 bytes --]

diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..23835b6 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
     addr = base_addr = ram_size - ACPI_DATA_SIZE;
     rsdt_addr = addr;
     rsdt = (void *)(addr);
-    rsdt_size = sizeof(*rsdt) + external_tables * 4;
+    rsdt_size = sizeof(*rsdt);
     addr += rsdt_size;
 
     fadt_addr = addr;
@@ -1665,6 +1665,7 @@ void acpi_bios_init(void)
 
     addr = (addr + 7) & ~7;
     madt_addr = addr;
+    madt = (void *)(addr);
     madt_size = sizeof(*madt) +
         sizeof(struct madt_processor_apic) * MAX_CPUS +
 #ifdef BX_QEMU
@@ -1672,7 +1673,11 @@ void acpi_bios_init(void)
 #else
         sizeof(struct madt_io_apic);
 #endif
-    madt = (void *)(addr);
+    for ( i = 0; i < 16; i++ ) {
+        if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
+            madt_size += sizeof(struct madt_int_override);
+        }
+    }
     addr += madt_size;
 
 #ifdef BX_QEMU
@@ -1786,7 +1791,6 @@ void acpi_bios_init(void)
                 continue;
             }
             int_override++;
-            madt_size += sizeof(struct madt_int_override);
         }
         acpi_build_table_header((struct acpi_table_header *)madt,
                                 "APIC", madt_size, 1);
@@ -1868,17 +1872,6 @@ void acpi_bios_init(void)
     acpi_build_table_header((struct  acpi_table_header *)hpet,
                              "HPET", sizeof(*hpet), 1);
 #endif
-
-    acpi_additional_tables(); /* resets cfg to required entry */
-    for(i = 0; i < external_tables; i++) {
-        uint16_t len;
-        if(acpi_load_table(i, addr, &len) < 0)
-            BX_PANIC("Failed to load ACPI table from QEMU\n");
-        rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
-        addr += len;
-        if(addr >= ram_size)
-            BX_PANIC("ACPI table overflow\n");
-    }
 #endif
 
     /* RSDT */
@@ -1891,6 +1884,16 @@ void acpi_bios_init(void)
 //  rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(hpet_addr);
     if (nb_numa_nodes > 0)
         rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(srat_addr);
+    acpi_additional_tables(); /* resets cfg to required entry */
+    for(i = 0; i < external_tables; i++) {
+        uint16_t len;
+        if(acpi_load_table(i, addr, &len) < 0)
+            BX_PANIC("Failed to load ACPI table from QEMU\n");
+        rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
+        addr += len;
+        if((addr >= ram_size) || (nb_rsdt_entries + 1 > MAX_RSDT_ENTRIES)) 
+            BX_PANIC("ACPI table overflow\n");
+    }
 #endif
     rsdt_size -= MAX_RSDT_ENTRIES * 4;
     rsdt_size += nb_rsdt_entries * 4;

Re: [PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable

[Marcelo Tosatti]

Beth,

On Thu, May 14, 2009 at 12:20:29PM -0400, Beth Kon wrote:
> Anthony Liguori wrote:
>> Vincent Minet wrote:
>>> External ACPI tables are counted twice for the RSDT size and the load
>>> address for the first external table is in the MADT (interrupt override
>>> entries are overwritten).
>>>
>>> Signed-off-by: Vincent Minet <vincent@vincent-minet.net>
>>>   
>>
>> Beth,
>>
>> I think you had a patch attempting to address the same issue.  It was  
>> a bit more involved though.
>>
>> Which is the proper fix and are they both to the same problem?
> They are for 2 different bases. My patch was for qemu's bochs bios and  
> this is for qemu-kvm/kvm/bios/rombios32.c. They are pretty divergent in  
> this area of setting up the ACPI tables. My patch is still needed for  
> the qemu base. I hope we'll be getting to one base soon :-)
>
> Assuming the intent of the code was for MAX_RSDT_ENTRIES to include  
> external_tables, this patch looks correct. I think one additional check  
> would be needed (in my patch) to make sure that the code doesn't exceed  
> MAX_RSDT_ENTRIES when the external tables are being loaded.
>
> My patch also puts all the code that calculates madt_size in the same  
> place, at the beginning of the table layout. I believe this is neater  
> and will avoid problems like this one in the future. As much as  
> possible, I think it best to get all the tables layed out, then fill  
> them in. If for some reason this is not acceptable, we need to add a big  
> note that no tables should be layed out after the madt because the madt  
> may grow further down in the code and overwrite the other table.

I like this better too, see questions/comments below.

>>
>> Regards,
>>
>> Anthony Liguori
>>
>>> ---
>>>  kvm/bios/rombios32.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>>> index cbd5f15..289361b 100755
>>> --- a/kvm/bios/rombios32.c
>>> +++ b/kvm/bios/rombios32.c
>>> @@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
>>>      addr = base_addr = ram_size - ACPI_DATA_SIZE;
>>>      rsdt_addr = addr;
>>>      rsdt = (void *)(addr);
>>> -    rsdt_size = sizeof(*rsdt) + external_tables * 4;
>>> +    rsdt_size = sizeof(*rsdt);
>>>      addr += rsdt_size;
>>>       fadt_addr = addr;
>>> @@ -1787,6 +1787,7 @@ void acpi_bios_init(void)
>>>              }
>>>              int_override++;
>>>              madt_size += sizeof(struct madt_int_override);
>>> +            addr += sizeof(struct madt_int_override);
>>>          }
>>>          acpi_build_table_header((struct acpi_table_header *)madt,
>>>                                  "APIC", madt_size, 1);
>>>   


> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
> index cbd5f15..23835b6 100755
> --- a/kvm/bios/rombios32.c
> +++ b/kvm/bios/rombios32.c
> @@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
>      addr = base_addr = ram_size - ACPI_DATA_SIZE;
>      rsdt_addr = addr;
>      rsdt = (void *)(addr);
> -    rsdt_size = sizeof(*rsdt) + external_tables * 4;
> +    rsdt_size = sizeof(*rsdt);
>      addr += rsdt_size;
>  
>      fadt_addr = addr;
> @@ -1665,6 +1665,7 @@ void acpi_bios_init(void)
>  
>      addr = (addr + 7) & ~7;
>      madt_addr = addr;
> +    madt = (void *)(addr);
>      madt_size = sizeof(*madt) +
>          sizeof(struct madt_processor_apic) * MAX_CPUS +
>  #ifdef BX_QEMU
> @@ -1672,7 +1673,11 @@ void acpi_bios_init(void)
>  #else
>          sizeof(struct madt_io_apic);
>  #endif
> -    madt = (void *)(addr);
> +    for ( i = 0; i < 16; i++ ) {
> +        if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
> +            madt_size += sizeof(struct madt_int_override);
> +        }
> +    }
>      addr += madt_size;

This bug could only affect the HPET descriptor right? 

>  #ifdef BX_QEMU
> @@ -1786,7 +1791,6 @@ void acpi_bios_init(void)
>                  continue;
>              }
>              int_override++;
> -            madt_size += sizeof(struct madt_int_override);
>          }
>          acpi_build_table_header((struct acpi_table_header *)madt,
>                                  "APIC", madt_size, 1);
> @@ -1868,17 +1872,6 @@ void acpi_bios_init(void)
>      acpi_build_table_header((struct  acpi_table_header *)hpet,
>                               "HPET", sizeof(*hpet), 1);
>  #endif
> -
> -    acpi_additional_tables(); /* resets cfg to required entry */
> -    for(i = 0; i < external_tables; i++) {
> -        uint16_t len;
> -        if(acpi_load_table(i, addr, &len) < 0)
> -            BX_PANIC("Failed to load ACPI table from QEMU\n");
> -        rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
> -        addr += len;
> -        if(addr >= ram_size)
> -            BX_PANIC("ACPI table overflow\n");
> -    }

The external ACPI tables fix(es) are logically separate from the MADT
intoverride size calculation, and so they could be separate patches?

>  #endif
>  
>      /* RSDT */
> @@ -1891,6 +1884,16 @@ void acpi_bios_init(void)
>  //  rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(hpet_addr);
>      if (nb_numa_nodes > 0)
>          rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(srat_addr);
> +    acpi_additional_tables(); /* resets cfg to required entry */
> +    for(i = 0; i < external_tables; i++) {
> +        uint16_t len;
> +        if(acpi_load_table(i, addr, &len) < 0)
> +            BX_PANIC("Failed to load ACPI table from QEMU\n");
> +        rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
> +        addr += len;
> +        if((addr >= ram_size) || (nb_rsdt_entries + 1 > MAX_RSDT_ENTRIES)) 
> +            BX_PANIC("ACPI table overflow\n");
> +    }
>  #endif
>      rsdt_size -= MAX_RSDT_ENTRIES * 4;
>      rsdt_size += nb_rsdt_entries * 4;

Re: [PATCH] bios: Fix MADT corruption and RSDT size when using -acpitable

[Beth Kon]

Marcelo Tosatti wrote:
> Beth,
>
> On Thu, May 14, 2009 at 12:20:29PM -0400, Beth Kon wrote:
>   
>> Anthony Liguori wrote:
>>     
>>> Vincent Minet wrote:
>>>       
>>>> External ACPI tables are counted twice for the RSDT size and the load
>>>> address for the first external table is in the MADT (interrupt override
>>>> entries are overwritten).
>>>>
>>>> Signed-off-by: Vincent Minet <vincent@vincent-minet.net>
>>>>   
>>>>         
>>> Beth,
>>>
>>> I think you had a patch attempting to address the same issue.  It was  
>>> a bit more involved though.
>>>
>>> Which is the proper fix and are they both to the same problem?
>>>       
>> They are for 2 different bases. My patch was for qemu's bochs bios and  
>> this is for qemu-kvm/kvm/bios/rombios32.c. They are pretty divergent in  
>> this area of setting up the ACPI tables. My patch is still needed for  
>> the qemu base. I hope we'll be getting to one base soon :-)
>>
>> Assuming the intent of the code was for MAX_RSDT_ENTRIES to include  
>> external_tables, this patch looks correct. I think one additional check  
>> would be needed (in my patch) to make sure that the code doesn't exceed  
>> MAX_RSDT_ENTRIES when the external tables are being loaded.
>>
>> My patch also puts all the code that calculates madt_size in the same  
>> place, at the beginning of the table layout. I believe this is neater  
>> and will avoid problems like this one in the future. As much as  
>> possible, I think it best to get all the tables layed out, then fill  
>> them in. If for some reason this is not acceptable, we need to add a big  
>> note that no tables should be layed out after the madt because the madt  
>> may grow further down in the code and overwrite the other table.
>>     
>
> I like this better too, see questions/comments below.
>
>   
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>       
>>>> ---
>>>>  kvm/bios/rombios32.c |    3 ++-
>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>>>> index cbd5f15..289361b 100755
>>>> --- a/kvm/bios/rombios32.c
>>>> +++ b/kvm/bios/rombios32.c
>>>> @@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
>>>>      addr = base_addr = ram_size - ACPI_DATA_SIZE;
>>>>      rsdt_addr = addr;
>>>>      rsdt = (void *)(addr);
>>>> -    rsdt_size = sizeof(*rsdt) + external_tables * 4;
>>>> +    rsdt_size = sizeof(*rsdt);
>>>>      addr += rsdt_size;
>>>>       fadt_addr = addr;
>>>> @@ -1787,6 +1787,7 @@ void acpi_bios_init(void)
>>>>              }
>>>>              int_override++;
>>>>              madt_size += sizeof(struct madt_int_override);
>>>> +            addr += sizeof(struct madt_int_override);
>>>>          }
>>>>          acpi_build_table_header((struct acpi_table_header *)madt,
>>>>                                  "APIC", madt_size, 1);
>>>>   
>>>>         
>
>
>   
>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>> index cbd5f15..23835b6 100755
>> --- a/kvm/bios/rombios32.c
>> +++ b/kvm/bios/rombios32.c
>> @@ -1626,7 +1626,7 @@ void acpi_bios_init(void)
>>      addr = base_addr = ram_size - ACPI_DATA_SIZE;
>>      rsdt_addr = addr;
>>      rsdt = (void *)(addr);
>> -    rsdt_size = sizeof(*rsdt) + external_tables * 4;
>> +    rsdt_size = sizeof(*rsdt);
>>      addr += rsdt_size;
>>  
>>      fadt_addr = addr;
>> @@ -1665,6 +1665,7 @@ void acpi_bios_init(void)
>>  
>>      addr = (addr + 7) & ~7;
>>      madt_addr = addr;
>> +    madt = (void *)(addr);
>>      madt_size = sizeof(*madt) +
>>          sizeof(struct madt_processor_apic) * MAX_CPUS +
>>  #ifdef BX_QEMU
>> @@ -1672,7 +1673,11 @@ void acpi_bios_init(void)
>>  #else
>>          sizeof(struct madt_io_apic);
>>  #endif
>> -    madt = (void *)(addr);
>> +    for ( i = 0; i < 16; i++ ) {
>> +        if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
>> +            madt_size += sizeof(struct madt_int_override);
>> +        }
>> +    }
>>      addr += madt_size;
>>     
>
> This bug could only affect the HPET descriptor right? 
>   
I'm not sure what you're asking. There were 2 bugs that Vincent pointed 
out. The first caused an incorrect rsdt_size to be reported, and the 
second (missing addr += sizeof(struct madt_int_override)) caused 
corruption of whatever came after the MADT. But even if his patch were 
applied, any future code that added a table and manipulated addr between 
the following points:

...
(about line 1676)
madt = (void *)(addr);
addr += madt_size;
...
(about line 1789)
madt_size += sizeof(struct madt_int_override);
addr += sizeof(struct madt_int_override);

would have wound up causing some kind of corruption, as happened with 
the HPET. Also the "memset(madt, 0, madt_size)" around line 1740 was not 
using the complete madt_size.

So this seems undesirable, and that's why I suggested moving all addr 
manipulation (with the exception of additional tables at the very end) 
to the same section of the table layout code. Seems best to manage 
madt_size all in one place.

>   
>>  #ifdef BX_QEMU
>> @@ -1786,7 +1791,6 @@ void acpi_bios_init(void)
>>                  continue;
>>              }
>>              int_override++;
>> -            madt_size += sizeof(struct madt_int_override);
>>          }
>>          acpi_build_table_header((struct acpi_table_header *)madt,
>>                                  "APIC", madt_size, 1);
>> @@ -1868,17 +1872,6 @@ void acpi_bios_init(void)
>>      acpi_build_table_header((struct  acpi_table_header *)hpet,
>>                               "HPET", sizeof(*hpet), 1);
>>  #endif
>> -
>> -    acpi_additional_tables(); /* resets cfg to required entry */
>> -    for(i = 0; i < external_tables; i++) {
>> -        uint16_t len;
>> -        if(acpi_load_table(i, addr, &len) < 0)
>> -            BX_PANIC("Failed to load ACPI table from QEMU\n");
>> -        rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
>> -        addr += len;
>> -        if(addr >= ram_size)
>> -            BX_PANIC("ACPI table overflow\n");
>> -    }
>>     
>
> The external ACPI tables fix(es) are logically separate from the MADT
> intoverride size calculation, and so they could be separate patches?
>   
Yes that's true.
>>  #endif
>>  
>>      /* RSDT */
>> @@ -1891,6 +1884,16 @@ void acpi_bios_init(void)
>>  //  rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(hpet_addr);
>>      if (nb_numa_nodes > 0)
>>          rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(srat_addr);
>> +    acpi_additional_tables(); /* resets cfg to required entry */
>> +    for(i = 0; i < external_tables; i++) {
>> +        uint16_t len;
>> +        if(acpi_load_table(i, addr, &len) < 0)
>> +            BX_PANIC("Failed to load ACPI table from QEMU\n");
>> +        rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(addr);
>> +        addr += len;
>> +        if((addr >= ram_size) || (nb_rsdt_entries + 1 > MAX_RSDT_ENTRIES)) 
>> +            BX_PANIC("ACPI table overflow\n");
>> +    }
>>  #endif
>>      rsdt_size -= MAX_RSDT_ENTRIES * 4;
>>      rsdt_size += nb_rsdt_entries * 4;
>>     
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>