From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlFRq-0003eB-Mi for qemu-devel@nongnu.org; Tue, 26 Nov 2013 05:00:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VlFRi-0003sy-HH for qemu-devel@nongnu.org; Tue, 26 Nov 2013 05:00:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16957) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VlFRi-0003sk-AA for qemu-devel@nongnu.org; Tue, 26 Nov 2013 05:00:42 -0500 Date: Tue, 26 Nov 2013 12:03:51 +0200 From: "Michael S. Tsirkin" Message-ID: <20131126100351.GC18777@redhat.com> References: <1384273995-16486-1-git-send-email-cminyard@mvista.com> <1384273995-16486-15-git-send-email-cminyard@mvista.com> <20131114073036.GB12673@redhat.com> <5284CFE0.4010304@acm.org> <20131114133853.GA5658@redhat.com> <5284D2CF.8040409@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5284D2CF.8040409@acm.org> Subject: Re: [Qemu-devel] [PATCH 14/16] pc: Postpone adding ACPI and SMBIOS to fw_cfg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Minyard Cc: Bret Ketchum , Corey Minyard , Corey Minyard , qemu-devel@nongnu.org, Andreas =?iso-8859-1?Q?F=E4rber?= 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 > >>> 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. > > 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 OK need some help with that? Main options are - write code in ASL, supply entry in ACPI always, patch some fields to enable/disable it dynamically see how pvpanic entry is disabled for an example - write code in ASL append it to ACPI if necessary see how CPU entries are added depending on number of CPUs for an example - generate code in AML directly see acpi based pci hotplug on pci branch in my tree for an example > > > >> 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