From: "Michael S. Tsirkin" <mst@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: pbonzini@redhat.com, qemu-devel@nongnu.org,
Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
ehabkost@redhat.com
Subject: Re: [Qemu-devel] [PATCH 8/9] pc: move apic_id_limit to PCMachineState
Date: Thu, 4 Feb 2016 20:18:04 +0200 [thread overview]
Message-ID: <20160204201658-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <20160204180838.149b3a84@nial.brq.redhat.com>
On Thu, Feb 04, 2016 at 06:08:38PM +0100, Igor Mammedov wrote:
> On Thu, 4 Feb 2016 14:48:58 +0200
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
>
> > On 02/04/2016 01:47 PM, Igor Mammedov wrote:
> > > yet another cleanup that replaces multiple apic_id_limit
> > > variables that were initialized at differnt places with
> > > a one initialization at pc_cpus_init() time and letting
> > > other code to just read it. That also allows to reduce
> > > number of a function arguments where it takes
> > > MachineState as an argument.
> > >
> >
> > This patch can collide with Eduardo's PcGuestInfo elimination series.
> > I am not sure what is the status of those patches.
> I guess it's Eduardo can take this patch or use his.
>
> Eduardo,
> I don't care much about this patch, just let me know
> what would you prefer to do with it.
Eduardo's work is in my tree, you can rebase on top of it.
> >
> > Thanks,
> > Marcel
> >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > hw/i386/acpi-build.c | 14 +++++++-------
> > > hw/i386/pc.c | 49 +++++++++++++++++++++----------------------------
> > > include/hw/i386/pc.h | 4 +++-
> > > 3 files changed, 31 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 921830e..d6cd06a 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -937,7 +937,7 @@ static Aml *build_crs(PCIHostState *host,
> > > return crs;
> > > }
> > >
> > > -static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > +static void build_processor_devices(Aml *sb_scope,
> > > AcpiPmInfo *pm, MachineState *machine)
> > > {
> > > int i;
> > > @@ -948,12 +948,13 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > Aml *ifctx;
> > > Aml *method;
> > > MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > + PCMachineState *pcms = PC_MACHINE(machine);
> > > GArray *apic_id_list = mc->possible_cpu_arch_ids();
> > >
> > > /* The current AML generator can cover the APIC ID range [0..255],
> > > * inclusive, for VCPU hotplug. */
> > > QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> > > - g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > > + g_assert(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > >
> > > /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
> > > dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
> > > @@ -1029,10 +1030,10 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > * ith up to 255 elements. Windows guests up to win2k8 fail when
> > > * VarPackageOp is used.
> > > */
> > > - pkg = acpi_cpus <= 255 ? aml_package(acpi_cpus) :
> > > - aml_varpackage(acpi_cpus);
> > > + pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > > + aml_varpackage(pcms->apic_id_limit);
> > >
> > > - for (i = 0; i < acpi_cpus; i++) {
> > > + for (i = 0; i < pcms->apic_id_limit; i++) {
> > > uint8_t b = qemu_get_cpu_by_arch_id(i) ? 0x01 : 0x00;
> > > aml_append(pkg, aml_int(b));
> > > }
> > > @@ -2233,8 +2234,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> > >
> > > sb_scope = aml_scope("\\_SB");
> > > {
> > > - build_processor_devices(sb_scope, guest_info->apic_id_limit, pm,
> > > - machine);
> > > + build_processor_devices(sb_scope, pm, machine);
> > >
> > > build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
> > > pm->mem_hp_io_len);
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 2fd8fc8..61fbb11 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -700,18 +700,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
> > > }
> > > }
> > >
> > > -/* Calculates the limit to CPU APIC ID values
> > > - *
> > > - * This function returns the limit for the APIC ID value, so that all
> > > - * CPU APIC IDs are < pc_apic_id_limit().
> > > - *
> > > - * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > > - */
> > > -static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> > > -{
> > > - return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > > -}
> > > -
> > > static void pc_build_smbios(FWCfgState *fw_cfg)
> > > {
> > > uint8_t *smbios_tables, *smbios_anchor;
> > > @@ -749,12 +737,11 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
> > > }
> > > }
> > >
> > > -static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > +static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
> > > {
> > > FWCfgState *fw_cfg;
> > > uint64_t *numa_fw_cfg;
> > > int i, j;
> > > - unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> > >
> > > fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
> > >
> > > @@ -772,7 +759,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
> > > * the APIC ID, not the "CPU index"
> > > */
> > > - fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
> > > + fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit);
> > > 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);
> > > @@ -790,11 +777,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > * of nodes, one word for each VCPU->node and one word for each node to
> > > * hold the amount of memory.
> > > */
> > > - numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
> > > + numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> > > numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> > > for (i = 0; i < max_cpus; i++) {
> > > unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > - assert(apic_id < apic_id_limit);
> > > + assert(apic_id < pcms->apic_id_limit);
> > > for (j = 0; j < nb_numa_nodes; j++) {
> > > if (test_bit(i, numa_info[j].node_cpu)) {
> > > numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
> > > @@ -803,10 +790,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > }
> > > }
> > > for (i = 0; i < nb_numa_nodes; i++) {
> > > - numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem);
> > > + numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
> > > + cpu_to_le64(numa_info[i].node_mem);
> > > }
> > > fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
> > > - (1 + apic_id_limit + nb_numa_nodes) *
> > > + (1 + pcms->apic_id_limit + nb_numa_nodes) *
> > > sizeof(*numa_fw_cfg));
> > >
> > > return fw_cfg;
> > > @@ -1120,7 +1108,14 @@ void pc_cpus_init(PCMachineState *pcms)
> > > int i;
> > > X86CPU *cpu = NULL;
> > > MachineState *machine = MACHINE(pcms);
> > > - unsigned long apic_id_limit;
> > > +
> > > + /* Calculates the limit to CPU APIC ID values so that all
> > > + * CPU APIC IDs are < pcms->apic_id_limit.
> > > + *
> > > + * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > > + * and bitmap based ACPI CPU hotplug interface
> > > + */
> > > + pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > >
> > > /* init CPUs */
> > > if (machine->cpu_model == NULL) {
> > > @@ -1131,10 +1126,9 @@ void pc_cpus_init(PCMachineState *pcms)
> > > #endif
> > > }
> > >
> > > - apic_id_limit = pc_apic_id_limit(max_cpus);
> > > - if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > > - error_report("max_cpus is too large. APIC ID of last CPU is %lu",
> > > - apic_id_limit - 1);
> > > + if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > > + error_report("max_cpus is too large. APIC ID of last CPU is %u",
> > > + pcms->apic_id_limit - 1);
> > > exit(1);
> > > }
> > >
> > > @@ -1197,7 +1191,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > >
> > > guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> > > guest_info->ram_size = pcms->below_4g_mem_size + pcms->above_4g_mem_size;
> > > - guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
> > > guest_info->apic_xrupt_override = kvm_allows_irq0_override();
> > > guest_info->numa_nodes = nb_numa_nodes;
> > > guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
> > > @@ -1206,12 +1199,12 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > > guest_info->node_mem[i] = numa_info[i].node_mem;
> > > }
> > >
> > > - guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
> > > + guest_info->node_cpu = g_malloc0(pcms->apic_id_limit *
> > > sizeof *guest_info->node_cpu);
> > >
> > > for (i = 0; i < max_cpus; i++) {
> > > unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > - assert(apic_id < guest_info->apic_id_limit);
> > > + assert(apic_id < pcms->apic_id_limit);
> > > for (j = 0; j < nb_numa_nodes; j++) {
> > > if (test_bit(i, numa_info[j].node_cpu)) {
> > > guest_info->node_cpu[apic_id] = j;
> > > @@ -1386,7 +1379,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
> > > option_rom_mr,
> > > 1);
> > >
> > > - fw_cfg = bochs_bios_init(&address_space_memory);
> > > + fw_cfg = bochs_bios_init(&address_space_memory, pcms);
> > >
> > > rom_set_fw(fw_cfg);
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 65e8f24..315fed2 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -59,6 +59,9 @@ struct PCMachineState {
> > >
> > > /* RAM information (sizes, addresses, configuration): */
> > > ram_addr_t below_4g_mem_size, above_4g_mem_size;
> > > +
> > > + /* APIC ID limit for current max_cpus */
> > > + unsigned apic_id_limit;
> > > };
> > >
> > > #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> > > @@ -154,7 +157,6 @@ typedef struct PcPciInfo {
> > > struct PcGuestInfo {
> > > bool isapc_ram_fw;
> > > hwaddr ram_size, ram_size_below_4g;
> > > - unsigned apic_id_limit;
> > > bool apic_xrupt_override;
> > > uint64_t numa_nodes;
> > > uint64_t *node_mem;
> > >
> >
next prev parent reply other threads:[~2016-02-04 18:18 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-04 11:47 [Qemu-devel] [PATCH 0/9] pc: do not create invalid MADT.LAPIC/Processor entries Igor Mammedov
2016-02-04 11:47 ` [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook Igor Mammedov
2016-02-04 12:18 ` Marcel Apfelbaum
2016-02-04 13:36 ` Igor Mammedov
2016-02-05 14:13 ` Eduardo Habkost
2016-02-05 14:49 ` Igor Mammedov
2016-02-05 15:04 ` Eduardo Habkost
2016-02-05 15:39 ` Igor Mammedov
2016-02-05 15:50 ` Eduardo Habkost
2016-02-05 16:29 ` Igor Mammedov
2016-02-04 11:47 ` [Qemu-devel] [PATCH 3/9] pc: acpi: cleanup qdev_get_machine() calls Igor Mammedov
2016-02-04 12:21 ` Marcel Apfelbaum
2016-02-04 11:47 ` [Qemu-devel] [PATCH 4/9] pc: acpi: SRAT: create only valid processor lapic entries Igor Mammedov
2016-02-05 15:07 ` Eduardo Habkost
2016-02-04 11:47 ` [Qemu-devel] [PATCH 5/9] pc: acpi: create Processor and Notify objects only for valid lapics Igor Mammedov
2016-02-05 15:17 ` Eduardo Habkost
2016-02-05 15:43 ` Igor Mammedov
2016-02-04 11:47 ` [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries " Igor Mammedov
2016-02-05 15:28 ` Eduardo Habkost
2016-02-05 16:14 ` Igor Mammedov
2016-02-11 16:11 ` Eduardo Habkost
2016-02-12 10:04 ` Igor Mammedov
2016-02-04 11:47 ` [Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus Igor Mammedov
2016-02-05 15:39 ` Eduardo Habkost
2016-02-05 16:19 ` Igor Mammedov
2016-02-05 16:44 ` Igor Mammedov
2016-02-11 15:59 ` Eduardo Habkost
2016-02-12 10:05 ` Igor Mammedov
2016-02-04 11:47 ` [Qemu-devel] [PATCH 8/9] pc: move apic_id_limit to PCMachineState Igor Mammedov
[not found] ` <56B348BA.40502@gmail.com>
2016-02-04 17:08 ` Igor Mammedov
2016-02-04 18:18 ` Michael S. Tsirkin [this message]
2016-02-04 18:24 ` Igor Mammedov
2016-02-04 11:47 ` [Qemu-devel] [PATCH 9/9] pc: acpi: clarify why possible LAPIC entries must be present in MADT Igor Mammedov
2016-02-05 15:39 ` Eduardo Habkost
2016-02-04 11:49 ` [Qemu-devel] [PATCH 1/9] cpu: rename cpu_exists() to qemu_get_cpu_by_arch_id() Igor Mammedov
2016-02-05 14:20 ` Eduardo Habkost
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160204201658-mutt-send-email-mst@redhat.com \
--to=mst@redhat.com \
--cc=ehabkost@redhat.com \
--cc=imammedo@redhat.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).