Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 11:12:04AM +0200, Igor Mammedov wrote:
> On Mon, 17 Oct 2016 19:44:52 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
> > [...]
> > > @@ -236,7 +237,11 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, 
> > > MachineState *machine,
> > >      /* 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(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > > +    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);
> > > +    }  
> > 
> > Moving the check here seems to make sense, but:
> > 
> > >  
> > >      /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO 
> > > */
> > >      dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 93ff49c..f1c1013 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, 
> > > PCMachineState *pcms)  
> > 
> > [Added more context below to show the code around the change]
> > 
> > >      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 < pcms->apic_id_limit);  
> > 
> > If you really needed to remove this assert, that means you can
> > write beyond the end of numa_fw_fg[] below. Are you sure you need
> > to remove it?
> > 
> > >          j = numa_get_node_for_cpu(i);
> > >          if (j < nb_numa_nodes) {
> > >              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);  
> > 
> >                            ^^^^^^^^^^^ here
> > 
> > >          }
> Another more radical way to deal with legacy FW_CFG_NUMA
> could be to remove it altogether if machine has x2APIC cpus.
> It'd be possible to do due to BIOS using QEMU provided
> BIOS tables and not using/calling code for built in/legacy
> ACPI tables.
> 
> Could do it as patch on top if that's sounds ok.

I don't mind either way. Initializing the fields even if they are
not going to be used seems harmless, but if you believe skipping
it would make things simpler, go ahead.

In this case, what would happen if x2apic CPUs are available but
the BIOS is too old?

-- 
Eduardo