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

2009-05-15 Thread 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 vinc...@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(); /*

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

2009-05-15 Thread 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 vinc...@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)

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

2009-05-14 Thread Beth Kon 

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 vinc...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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);
+

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

2009-05-13 Thread 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 vinc...@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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2009-05-13 Thread 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 vinc...@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);
  


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html