[PATCH 1/2] Clean up MADT Table Creation

[PATCH 1/2] Clean up MADT Table Creation

[Beth Kon]

This patch is based on the recent patch from Vincent Minet. I split Vincent's
changes into 2 patches (to separate MADT and RSDT table cleanup, as suggested by
Marcelo) and added a bit to them. And to give credit where it is due, this
cleanup is also related to the patch Marcelo provided when the HPET addition 
tripped over the same problem. (Thanks again Marcelo :-) 

This patch moves all the table layout calculations to the same area of
acpi_bios_init. This prevents corruption problems when, in the middle of
filling in the tables, the MADT table size grows. The idea is to do all the 
layout in one section, then fill things in afterwards. It also corrects a 
problem where the madt table was memset to 0 before the final size of the 
table had been determined.

Signed-off-by: Beth Kon <eak@us.ibm.com>

diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..7f62e4f 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -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);

[PATCH 2/2] Clean up RSDT Table Creation

[Beth Kon]

This patch is also based on the patch by Vincent Minet. It corrects the size
calculation of the RSDT, and checks for overflow of MAX_RSDT_ENTRIES, 
assuming that the external table entry count is contained within
MAX_RSDT_ENTRIES.

Signed-off-by: Beth Kon <eak@us.ibm.com>

diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index 7f62e4f..ac8f9c5 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;
@@ -1873,16 +1873,6 @@ void acpi_bios_init(void)
                              "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 */
@@ -1895,6 +1885,19 @@ 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 */
+    /* external_tables load must occur last to 
+     * properly check for MAX_RSDT_ENTRIES overflow.
+     */
+    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 > MAX_RSDT_ENTRIES))     
+            BX_PANIC("ACPI table overflow\n");
+    }
 #endif
     rsdt_size -= MAX_RSDT_ENTRIES * 4;
     rsdt_size += nb_rsdt_entries * 4;

Re: [PATCH 2/2] Clean up RSDT Table Creation

[Avi Kivity]

Beth Kon wrote:
> This patch is also based on the patch by Vincent Minet. It corrects the size
> calculation of the RSDT, and checks for overflow of MAX_RSDT_ENTRIES, 
> assuming that the external table entry count is contained within
> MAX_RSDT_ENTRIES.
>
> Signed-off-by: Beth Kon <eak@us.ibm.com>
>
> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
> index 7f62e4f..ac8f9c5 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;
> @@ -1873,16 +1873,6 @@ void acpi_bios_init(void)
>                               "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 */
> @@ -1895,6 +1885,19 @@ 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 */
> +    /* external_tables load must occur last to 
> +     * properly check for MAX_RSDT_ENTRIES overflow.
> +     */
> +    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 > MAX_RSDT_ENTRIES))     
> +            BX_PANIC("ACPI table overflow\n");
> +    }
>  #endif
>      rsdt_size -= MAX_RSDT_ENTRIES * 4;
>      rsdt_size += nb_rsdt_entries * 4;
>   

Same comment - instead of calculating the size incrementally, set 
rsdt_end = &rsdt->table_offset_entry[nb_rsdt_entries] and calculate the 
size from that.

btw, why did you move the code?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/2] Clean up MADT Table Creation

[Avi Kivity]

Beth Kon wrote:
> This patch is based on the recent patch from Vincent Minet. I split Vincent's
> changes into 2 patches (to separate MADT and RSDT table cleanup, as suggested by
> Marcelo) and added a bit to them. And to give credit where it is due, this
> cleanup is also related to the patch Marcelo provided when the HPET addition 
> tripped over the same problem. (Thanks again Marcelo :-) 
>
> This patch moves all the table layout calculations to the same area of
> acpi_bios_init. This prevents corruption problems when, in the middle of
> filling in the tables, the MADT table size grows. The idea is to do all the 
> layout in one section, then fill things in afterwards. It also corrects a 
> problem where the madt table was memset to 0 before the final size of the 
> table had been determined.
>
> Signed-off-by: Beth Kon <eak@us.ibm.com>
>
> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
> index cbd5f15..7f62e4f 100755
> --- a/kvm/bios/rombios32.c
> +++ b/kvm/bios/rombios32.c
> @@ -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;
>  
>   

You're just duplicating the override creation loop (with its internal 
if); if we update it, we'll have to update this too.

Why not set madt_end = int_override and calculate madt_size = madt_end - 
madt?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/2] Clean up MADT Table Creation

[Beth Kon]

Avi Kivity wrote:
> Beth Kon wrote:
>> This patch is based on the recent patch from Vincent Minet. I split 
>> Vincent's
>> changes into 2 patches (to separate MADT and RSDT table cleanup, as 
>> suggested by
>> Marcelo) and added a bit to them. And to give credit where it is due, 
>> this
>> cleanup is also related to the patch Marcelo provided when the HPET 
>> addition tripped over the same problem. (Thanks again Marcelo :-)
>> This patch moves all the table layout calculations to the same area of
>> acpi_bios_init. This prevents corruption problems when, in the middle of
>> filling in the tables, the MADT table size grows. The idea is to do 
>> all the layout in one section, then fill things in afterwards. It 
>> also corrects a problem where the madt table was memset to 0 before 
>> the final size of the table had been determined.
>>
>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>
>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>> index cbd5f15..7f62e4f 100755
>> --- a/kvm/bios/rombios32.c
>> +++ b/kvm/bios/rombios32.c
>> @@ -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;
>>  
>>   
>
> You're just duplicating the override creation loop (with its internal 
> if); if we update it, we'll have to update this too.
Yep, that's a valid complaint. I'll resubmit shortly.
>
> Why not set madt_end = int_override and calculate madt_size = madt_end 
> - madt?
>