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

2016-10-18 Thread Igor Mammedov
On Tue, 18 Oct 2016 08:39:05 -0200
Eduardo Habkost  wrote:

> On Tue, Oct 18, 2016 at 11:12:04AM +0200, Igor Mammedov wrote:
> > On Mon, 17 Oct 2016 19:44:52 -0200
> > Eduardo Habkost  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.
Keying of >255 cpus is a chance to drop legacy FW_CFG_NUMA which isn't
used by modern QEMU/SeaBIOS.
We can't do that for < 255 as it would break migration/BIOS where
it is already present.
I'll look at it some more from SeaBIOS perspective if it's feasible/useful.

> In this case, what would happen if x2apic CPUs are available but
> the BIOS is too old?
Old BIOS is unfixable is there are more than 255,
most often old BIOS would hang in smp_setup() waiting
cmos_smp_count CPUs to wakeup or crash later if smp_scan() lucky and
wins the race.

Or BIOS could fail earlier/later during buffer overflow/allocation
when initializing legacy ACPI tables/pir/mp_table if etc/max-cpus
is big enough.
Corresponding SeaBIOS series is mentioned in cover letter.

This series or dropping FW_CFG_NUMA should not affect (existing) VMs
with less that 255 CPUs though.



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

2016-10-18 Thread 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  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



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

2016-10-18 Thread Eduardo Habkost
On Tue, Oct 18, 2016 at 11:02:54AM +0200, Igor Mammedov wrote:
> On Mon, 17 Oct 2016 19:44:52 -0200
> Eduardo Habkost  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
> Shouldn't above
>   numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> allocate sufficiently sized array?

I believe it should, but that would mean the assert() is still
valid.

> 
> That's aside, the assert could be kept as it doesn't get in a way
> if you'd prefer it that way.

Yes, please. The assert() removal seems unnecessary (and
confusing, because it made me believe that the condition was not
going to be valid anymore).

-- 
Eduardo



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

2016-10-18 Thread Igor Mammedov
On Mon, 17 Oct 2016 19:44:52 -0200
Eduardo Habkost  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.

> >  }
> > 
> > @@ -1190,12 +1189,6 @@ void pc_cpus_init(PCMachineState *pcms)
> >   * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> >   */
> >  pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 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);
> > -}
> > -
> >  pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
> >  sizeof(CPUArchId) * max_cpus);
> >  for (i = 0; i < max_cpus; i++) {
> > -- 
> > 2.7.4
> >   
> 




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

2016-10-18 Thread Igor Mammedov
On Mon, 17 Oct 2016 19:44:52 -0200
Eduardo Habkost  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
Shouldn't above
  numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
allocate sufficiently sized array?

That's aside, the assert could be kept as it doesn't get in a way
if you'd prefer it that way.

> >  }
> >  }
> > 
> > @@ -1190,12 +1189,6 @@ void pc_cpus_init(PCMachineState *pcms)
> >   * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> >   */
> >  pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 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);
> > -}
> > -
> >  pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
> >  sizeof(CPUArchId) * max_cpus);
> >  for (i = 0; i < max_cpus; i++) {
> > -- 
> > 2.7.4
> >   
> 




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

2016-10-17 Thread Eduardo Habkost
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

>  }
>  }
> 
> @@ -1190,12 +1189,6 @@ void pc_cpus_init(PCMachineState *pcms)
>   * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
>   */
>  pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 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);
> -}
> -
>  pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
>  sizeof(CPUArchId) * max_cpus);
>  for (i = 0; i < max_cpus; i++) {
> -- 
> 2.7.4
> 

-- 
Eduardo