diff mbox

[2/5] pc: Postpone SMBIOS table installation to post machine init

Message ID 1456340356-17147-3-git-send-email-minyard@acm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Corey Minyard Feb. 24, 2016, 6:59 p.m. UTC
From: Corey Minyard <cminyard@mvista.com>

This is the same place that the ACPI SSDT table gets added, so that
devices can add themselves to the SMBIOS table.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i386/pc.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Michael S. Tsirkin March 13, 2016, 2:03 p.m. UTC | #1
On Wed, Feb 24, 2016 at 12:59:13PM -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is the same place that the ACPI SSDT table gets added, so that
> devices can add themselves to the SMBIOS table.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

This changes the order of fw cfg files, which
is guest visible.
Need to make it depend on either machine type
or the presence of ipmi somehow.

> ---
>  hw/i386/pc.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0aeefd2..da8fc76 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -778,8 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
>                       acpi_tables, acpi_tables_len);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
>  
> -    pc_build_smbios(fw_cfg);
> -
>      fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
>                       &e820_reserve, sizeof(e820_reserve));
>      fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
> @@ -1161,6 +1159,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>  {
>      PCMachineState *pcms = container_of(notifier,
>                                          PCMachineState, machine_done);
> +    FWCfgState *fw_cfg = pcms->fw_cfg;
>      PCIBus *bus = pcms->bus;
>  
>      if (bus) {
> @@ -1172,15 +1171,17 @@ void pc_machine_done(Notifier *notifier, void *data)
>                  extra_hosts++;
>              }
>          }
> -        if (extra_hosts && pcms->fw_cfg) {
> +        if (extra_hosts && fw_cfg) {
>              uint64_t *val = g_malloc(sizeof(*val));
>              *val = cpu_to_le64(extra_hosts);
> -            fw_cfg_add_file(pcms->fw_cfg,
> -                    "etc/extra-pci-roots", val, sizeof(*val));
> +            fw_cfg_add_file(fw_cfg, "etc/extra-pci-roots", val, sizeof(*val));
>          }
>      }
>  
>      acpi_setup();
> +    if (fw_cfg) {
> +        pc_build_smbios(fw_cfg);
> +    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> -- 
> 2.5.0
Corey Minyard March 14, 2016, 1:30 p.m. UTC | #2
On 03/13/2016 09:03 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 24, 2016 at 12:59:13PM -0600, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This is the same place that the ACPI SSDT table gets added, so that
>> devices can add themselves to the SMBIOS table.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> This changes the order of fw cfg files, which
> is guest visible.
> Need to make it depend on either machine type

Do you mean add a new machine type, like adding a
2.7 version of pc-i440fx and pc-q35?

> or the presence of ipmi somehow.

You could pre-scan for ipmi options from early in the
pc code.  Kind of hackish.

One other option would be to just have ACPI support for
IPMI.  Older OSes might not work; I'm not sure how
important that is.

Does anyone have a preference here?  To me, adding a
new machine type looks easiest, but I'm not sure of the
effects on users.

-corey

>> ---
>>   hw/i386/pc.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 0aeefd2..da8fc76 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -778,8 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
>>                        acpi_tables, acpi_tables_len);
>>       fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
>>   
>> -    pc_build_smbios(fw_cfg);
>> -
>>       fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
>>                        &e820_reserve, sizeof(e820_reserve));
>>       fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
>> @@ -1161,6 +1159,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>>   {
>>       PCMachineState *pcms = container_of(notifier,
>>                                           PCMachineState, machine_done);
>> +    FWCfgState *fw_cfg = pcms->fw_cfg;
>>       PCIBus *bus = pcms->bus;
>>   
>>       if (bus) {
>> @@ -1172,15 +1171,17 @@ void pc_machine_done(Notifier *notifier, void *data)
>>                   extra_hosts++;
>>               }
>>           }
>> -        if (extra_hosts && pcms->fw_cfg) {
>> +        if (extra_hosts && fw_cfg) {
>>               uint64_t *val = g_malloc(sizeof(*val));
>>               *val = cpu_to_le64(extra_hosts);
>> -            fw_cfg_add_file(pcms->fw_cfg,
>> -                    "etc/extra-pci-roots", val, sizeof(*val));
>> +            fw_cfg_add_file(fw_cfg, "etc/extra-pci-roots", val, sizeof(*val));
>>           }
>>       }
>>   
>>       acpi_setup();
>> +    if (fw_cfg) {
>> +        pc_build_smbios(fw_cfg);
>> +    }
>>   }
>>   
>>   void pc_guest_info_init(PCMachineState *pcms)
>> -- 
>> 2.5.0
Paolo Bonzini March 14, 2016, 2:16 p.m. UTC | #3
On 14/03/2016 14:30, Corey Minyard wrote:
> On 03/13/2016 09:03 PM, Michael S. Tsirkin wrote:
>> On Wed, Feb 24, 2016 at 12:59:13PM -0600, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> This is the same place that the ACPI SSDT table gets added, so that
>>> devices can add themselves to the SMBIOS table.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> This changes the order of fw cfg files, which
>> is guest visible.
>> Need to make it depend on either machine type 

Didn't Gerd have a patch to sort fw_cfg files?

... yes, here it is:
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05238.html

Paolo
Corey Minyard March 14, 2016, 3:01 p.m. UTC | #4
On 03/14/2016 09:16 PM, Paolo Bonzini wrote:
>
> On 14/03/2016 14:30, Corey Minyard wrote:
>> On 03/13/2016 09:03 PM, Michael S. Tsirkin wrote:
>>> On Wed, Feb 24, 2016 at 12:59:13PM -0600, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> This is the same place that the ACPI SSDT table gets added, so that
>>>> devices can add themselves to the SMBIOS table.
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> This changes the order of fw cfg files, which
>>> is guest visible.
>>> Need to make it depend on either machine type
> Didn't Gerd have a patch to sort fw_cfg files?
>
> ... yes, here it is:
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05238.html

I could take that patch and modify it to add a new machine type...

-corey

> Paolo
Gerd Hoffmann March 14, 2016, 3:25 p.m. UTC | #5
Hi,

> > Didn't Gerd have a patch to sort fw_cfg files?
> >
> > ... yes, here it is:
> > https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05238.html
> 
> I could take that patch and modify it to add a new machine type...

Sure, go ahead.  Didn't submit the patch because I wasn't sure it is
worth it, especially the compat stuff, do it on new machine types only
etc.

But if we now have to change the order _anyway_ for new machine types it
is a good opportunity to move to a sorted order at the same time, so we
will not have that problem (init order changes reorders fw_cfg file
directory) again.

cheers,
  Gerd
Michael S. Tsirkin March 15, 2016, 5:05 a.m. UTC | #6
On Mon, Mar 14, 2016 at 03:16:46PM +0100, Paolo Bonzini wrote:
> 
> 
> On 14/03/2016 14:30, Corey Minyard wrote:
> > On 03/13/2016 09:03 PM, Michael S. Tsirkin wrote:
> >> On Wed, Feb 24, 2016 at 12:59:13PM -0600, minyard@acm.org wrote:
> >>> From: Corey Minyard <cminyard@mvista.com>
> >>>
> >>> This is the same place that the ACPI SSDT table gets added, so that
> >>> devices can add themselves to the SMBIOS table.
> >>>
> >>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >> This changes the order of fw cfg files, which
> >> is guest visible.
> >> Need to make it depend on either machine type 
> 
> Didn't Gerd have a patch to sort fw_cfg files?
> 
> ... yes, here it is:
> https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg05238.html
> 
> Paolo

Yes but the messy part is the compatibility.
Specifically we need to find out the current list
of fw cfg files, and their order, and use that
existing order with old machine types.
diff mbox

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0aeefd2..da8fc76 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -778,8 +778,6 @@  static FWCfgState *bochs_bios_init(AddressSpace *as)
                      acpi_tables, acpi_tables_len);
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
 
-    pc_build_smbios(fw_cfg);
-
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
                      &e820_reserve, sizeof(e820_reserve));
     fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
@@ -1161,6 +1159,7 @@  void pc_machine_done(Notifier *notifier, void *data)
 {
     PCMachineState *pcms = container_of(notifier,
                                         PCMachineState, machine_done);
+    FWCfgState *fw_cfg = pcms->fw_cfg;
     PCIBus *bus = pcms->bus;
 
     if (bus) {
@@ -1172,15 +1171,17 @@  void pc_machine_done(Notifier *notifier, void *data)
                 extra_hosts++;
             }
         }
-        if (extra_hosts && pcms->fw_cfg) {
+        if (extra_hosts && fw_cfg) {
             uint64_t *val = g_malloc(sizeof(*val));
             *val = cpu_to_le64(extra_hosts);
-            fw_cfg_add_file(pcms->fw_cfg,
-                    "etc/extra-pci-roots", val, sizeof(*val));
+            fw_cfg_add_file(fw_cfg, "etc/extra-pci-roots", val, sizeof(*val));
         }
     }
 
     acpi_setup();
+    if (fw_cfg) {
+        pc_build_smbios(fw_cfg);
+    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)