Re: [Qemu-devel] [PATCH 14/16] pc: Postpone adding ACPI and SMBIOS to fw_cfg

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 14, 2013 at 07:40:31AM -0600, Corey Minyard wrote:
> On 11/14/2013 07:38 AM, Michael S. Tsirkin wrote:
> > On Thu, Nov 14, 2013 at 07:28:00AM -0600, Corey Minyard wrote:
> >> On 11/14/2013 01:30 AM, Michael S. Tsirkin wrote:
> >>> On Tue, Nov 12, 2013 at 10:33:13AM -0600, Corey Minyard wrote:
> >>>> Postpone the addition of the ACPI and SMBIOS tables until after
> >>>> device initialization.  This allows devices to add entries to these
> >>>> tables.
> >>>>
> >>>> Signed-off-by: Corey Minyard <cminyard@mvsita.com>
> >>> Why delay adding FW_CFG_ACPI_TABLES?
> >>> These are normally specified by user so they are constant.
> >> Because the IPMI device has to dynamically add an entry based on its
> >> configuration.
> > Where? Which patch does this? It would be a strange thing to do
> > because FW_CFG_ACPI_TABLES is normally intended for user-supplied tables
> > (though q35 misused it for other purposes).
> 
> I don't have that patch out yet.  I had written some code to dynamically
> generate ACPI tables, but that was rejected and from what I understand
> something was done recently so these tables can be dynamically generated
> some other way.  I haven't had time to look into this, but I wanted to
> get the main patch set out first.

OK so for review, it might be a good idea to split out the changes
required for necessary functionality as opposed to groundwork
that will be used down the line.

> 
> I know that these tables are generally fixed, but the entry for IPMI
> should only be present if the device is specified, and its contents
> depend on the configuration supplied,
> 
> -corey

I see. These tables are generated in hw/i386/acpi-build.c
This is invoked in machine done callback so there shouldn't
in an issue to probe for IPMI and it's configuration using QOM.

> >
> >> As it is the tables get added to the BIOS access before
> >> devices initialize.
> >>
> >> -corey
> >>
> >>>> ---
> >>>>  hw/i386/pc.c | 38 ++++++++++++++++++++++++++++++--------
> >>>>  1 file changed, 30 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >>>> index dee409d..765c95e 100644
> >>>> --- a/hw/i386/pc.c
> >>>> +++ b/hw/i386/pc.c
> >>>> @@ -607,8 +607,6 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> >>>>  static FWCfgState *bochs_bios_init(void)
> >>>>  {
> >>>>      FWCfgState *fw_cfg;
> >>>> -    uint8_t *smbios_table;
> >>>> -    size_t smbios_len;
> >>>>      uint64_t *numa_fw_cfg;
> >>>>      int i, j;
> >>>>      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> >>>> @@ -631,14 +629,8 @@ static FWCfgState *bochs_bios_init(void)
> >>>>      fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
> >>>>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
> >>>>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> >>>> -    fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
> >>>> -                     acpi_tables, acpi_tables_len);
> >>>>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
> >>>>  
> >>>> -    smbios_table = smbios_get_table(&smbios_len);
> >>>> -    if (smbios_table)
> >>>> -        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
> >>>> -                         smbios_table, smbios_len);
> >>>>      fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
> >>>>                       &e820_table, sizeof(e820_table));
> >>>>  
> >>>> @@ -1127,6 +1119,31 @@ void pc_acpi_init(const char *default_dsdt)
> >>>>      }
> >>>>  }
> >>>>  
> >>>> +struct pc_bios_post_init {
> >>>> +    Notifier post_init;
> >>>> +    void *fw_cfg;
> >>>> +};
> >>>> +
> >>>> +/* Add the ACPI and SMBIOS tables after all the hardware has been initialized.
> >>>> + * This gives devices a chance to add to those tables.
> >>>> + */
> >>>> +static void pc_bios_post_initfn(Notifier *n, void *opaque)
> >>>> +{
> >>>> +    struct pc_bios_post_init *p = container_of(n, struct pc_bios_post_init,
> >>>> +                                               post_init);
> >>>> +    uint8_t *smbios_table;
> >>>> +    size_t smbios_len;
> >>>> +
> >>>> +    fw_cfg_add_bytes(p->fw_cfg, FW_CFG_ACPI_TABLES,
> >>>> +                     acpi_tables, acpi_tables_len);
> >>>> +    smbios_table = smbios_get_table(&smbios_len);
> >>>> +    if (smbios_table)
> >>>> +        fw_cfg_add_bytes(p->fw_cfg, FW_CFG_SMBIOS_ENTRIES,
> >>>> +                         smbios_table, smbios_len);
> >>>> +}
> >>>> +
> >>>> +static struct pc_bios_post_init post_init;
> >>>> +
> >>>>  FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> >>>>                             const char *kernel_filename,
> >>>>                             const char *kernel_cmdline,
> >>>> @@ -1196,6 +1213,11 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
> >>>>          rom_add_option(option_rom[i].name, option_rom[i].bootindex);
> >>>>      }
> >>>>      guest_info->fw_cfg = fw_cfg;
> >>>> +
> >>>> +    post_init.fw_cfg = fw_cfg;
> >>>> +    post_init.post_init.notify = pc_bios_post_initfn;
> >>>> +    qemu_add_machine_init_done_notifier(&post_init.post_init);
> >>>> +
> >>>>      return fw_cfg;
> >>>>  }
> >>>>  
> >>>> -- 
> >>>> 1.8.3.1