[Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Igor Mammedov]

If QEMU is started with  -numa ... Windows only notices that
CPU has been hot-added but it will not online such CPUs.

It's caused by the fact that possible CPUs are flagged as
not enabled in SRAT and Windows honoring that information
doesn't use corresponding CPU.

ACPI 5.0 Spec regarding to flag says:
"
Table 5-47 Local APIC Flags
...
Enabled: if zero, this processor is unusable, and the operating system
support will not attempt to use it.
"

Fix QEMU to adhere to spec and mark possible CPUs as enabled
in SRAT.

With that Windows onlines hot-added CPUs as expected.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4003b6b..06499da 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1269,8 +1269,7 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 }
 
 static void
-build_srat(GArray *table_data, GArray *linker,
-           AcpiCpuInfo *cpu, PcGuestInfo *guest_info)
+build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
 {
     AcpiSystemResourceAffinityTable *srat;
     AcpiSratProcessorAffinity *core;
@@ -1300,11 +1299,7 @@ build_srat(GArray *table_data, GArray *linker,
         core->proximity_lo = curnode;
         memset(core->proximity_hi, 0, 3);
         core->local_sapic_eid = 0;
-        if (test_bit(i, cpu->found_cpus)) {
-            core->flags = cpu_to_le32(1);
-        } else {
-            core->flags = cpu_to_le32(0);
-        }
+        core->flags = cpu_to_le32(1);
     }
 
 
@@ -1622,7 +1617,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     }
     if (guest_info->numa_nodes) {
         acpi_add_table(table_offsets, tables->table_data);
-        build_srat(tables->table_data, tables->linker, &cpu, guest_info);
+        build_srat(tables->table_data, tables->linker, guest_info);
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables->table_data);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Paolo Bonzini]

On 10/11/2014 17:20, Igor Mammedov wrote:
> If QEMU is started with  -numa ... Windows only notices that
> CPU has been hot-added but it will not online such CPUs.
> 
> It's caused by the fact that possible CPUs are flagged as
> not enabled in SRAT and Windows honoring that information
> doesn't use corresponding CPU.
> 
> ACPI 5.0 Spec regarding to flag says:
> "
> Table 5-47 Local APIC Flags
> ...
> Enabled: if zero, this processor is unusable, and the operating system
> support will not attempt to use it.
> "
> 
> Fix QEMU to adhere to spec and mark possible CPUs as enabled
> in SRAT.
> 
> With that Windows onlines hot-added CPUs as expected.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Does this need to be specific to pc-2.2?

Paolo

> ---
>  hw/i386/acpi-build.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4003b6b..06499da 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1269,8 +1269,7 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>  }
>  
>  static void
> -build_srat(GArray *table_data, GArray *linker,
> -           AcpiCpuInfo *cpu, PcGuestInfo *guest_info)
> +build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
>  {
>      AcpiSystemResourceAffinityTable *srat;
>      AcpiSratProcessorAffinity *core;
> @@ -1300,11 +1299,7 @@ build_srat(GArray *table_data, GArray *linker,
>          core->proximity_lo = curnode;
>          memset(core->proximity_hi, 0, 3);
>          core->local_sapic_eid = 0;
> -        if (test_bit(i, cpu->found_cpus)) {
> -            core->flags = cpu_to_le32(1);
> -        } else {
> -            core->flags = cpu_to_le32(0);
> -        }
> +        core->flags = cpu_to_le32(1);
>      }
>  
>  
> @@ -1622,7 +1617,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>      }
>      if (guest_info->numa_nodes) {
>          acpi_add_table(table_offsets, tables->table_data);
> -        build_srat(tables->table_data, tables->linker, &cpu, guest_info);
> +        build_srat(tables->table_data, tables->linker, guest_info);
>      }
>      if (acpi_get_mcfg(&mcfg)) {
>          acpi_add_table(table_offsets, tables->table_data);
> 

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Igor Mammedov]

On Mon, 10 Nov 2014 18:40:12 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 10/11/2014 17:20, Igor Mammedov wrote:
> > If QEMU is started with  -numa ... Windows only notices that
> > CPU has been hot-added but it will not online such CPUs.
> > 
> > It's caused by the fact that possible CPUs are flagged as
> > not enabled in SRAT and Windows honoring that information
> > doesn't use corresponding CPU.
> > 
> > ACPI 5.0 Spec regarding to flag says:
> > "
> > Table 5-47 Local APIC Flags
> > ...
> > Enabled: if zero, this processor is unusable, and the operating system
> > support will not attempt to use it.
> > "
> > 
> > Fix QEMU to adhere to spec and mark possible CPUs as enabled
> > in SRAT.
> > 
> > With that Windows onlines hot-added CPUs as expected.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Does this need to be specific to pc-2.2?
Why not, It's bug fix.
But I don't care if it's merged later.


> 
> Paolo
> 
> > ---
> >  hw/i386/acpi-build.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 4003b6b..06499da 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1269,8 +1269,7 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> >  }
> >  
> >  static void
> > -build_srat(GArray *table_data, GArray *linker,
> > -           AcpiCpuInfo *cpu, PcGuestInfo *guest_info)
> > +build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
> >  {
> >      AcpiSystemResourceAffinityTable *srat;
> >      AcpiSratProcessorAffinity *core;
> > @@ -1300,11 +1299,7 @@ build_srat(GArray *table_data, GArray *linker,
> >          core->proximity_lo = curnode;
> >          memset(core->proximity_hi, 0, 3);
> >          core->local_sapic_eid = 0;
> > -        if (test_bit(i, cpu->found_cpus)) {
> > -            core->flags = cpu_to_le32(1);
> > -        } else {
> > -            core->flags = cpu_to_le32(0);
> > -        }
> > +        core->flags = cpu_to_le32(1);
> >      }
> >  
> >  
> > @@ -1622,7 +1617,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
> >      }
> >      if (guest_info->numa_nodes) {
> >          acpi_add_table(table_offsets, tables->table_data);
> > -        build_srat(tables->table_data, tables->linker, &cpu, guest_info);
> > +        build_srat(tables->table_data, tables->linker, guest_info);
> >      }
> >      if (acpi_get_mcfg(&mcfg)) {
> >          acpi_add_table(table_offsets, tables->table_data);
> > 

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Paolo Bonzini]

On 10/11/2014 18:48, Igor Mammedov wrote:
>> > Does this need to be specific to pc-2.2?
> Why not, It's bug fix.
> But I don't care if it's merged later.

No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
only for the last machine type?

Paolo

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Michael S. Tsirkin]

On Mon, Nov 10, 2014 at 06:56:30PM +0100, Paolo Bonzini wrote:
> 
> 
> On 10/11/2014 18:48, Igor Mammedov wrote:
> >> > Does this need to be specific to pc-2.2?
> > Why not, It's bug fix.
> > But I don't care if it's merged later.
> 
> No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
> only for the last machine type?
> 
> Paolo

I don't see why it should, it can't break migration.

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Paolo Bonzini]

On 10/11/2014 19:17, Michael S. Tsirkin wrote:
> > No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
> > only for the last machine type?
> 
> I don't see why it should, it can't break migration.
> 

It changes the contents of the SRAT.  There is a possibility that it
triggers bugs in previously working guests.

Paolo

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Michael S. Tsirkin]

On Mon, Nov 10, 2014 at 07:26:41PM +0100, Paolo Bonzini wrote:
> 
> 
> On 10/11/2014 19:17, Michael S. Tsirkin wrote:
> > > No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
> > > only for the last machine type?
> > 
> > I don't see why it should, it can't break migration.
> > 
> 
> It changes the contents of the SRAT.  There is a possibility that it
> triggers bugs in previously working guests.
> 
> Paolo

Absolutely but that's true for any bugfix.
We don't do compatiblity without a good reason.

Or do you mean that you consider this too high a risk for 2.2?

-- 
MST

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Paolo Bonzini]

On 10/11/2014 19:34, Michael S. Tsirkin wrote:
> > It changes the contents of the SRAT.  There is a possibility that it
> > triggers bugs in previously working guests.
> 
> Absolutely but that's true for any bugfix.
> We don't do compatiblity without a good reason.
> 
> Or do you mean that you consider this too high a risk for 2.2?

No, if you think it's okay in general, it's okay now as well.

Paolo

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Igor Mammedov]

On Mon, 10 Nov 2014 18:56:30 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 10/11/2014 18:48, Igor Mammedov wrote:
> >> > Does this need to be specific to pc-2.2?
> > Why not, It's bug fix.
> > But I don't care if it's merged later.
> 
> No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
> only for the last machine type?
> 
> Paolo
As Michael sad it's for all machine types or have you meant 2.1 stable
branch? My thought was that it's closed now.

I've tested it with RHEL6-7x64 and xp3,Windows server 2003-2012R2x64.
So far no regression was noticed.

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Paolo Bonzini]

On 11/11/2014 09:35, Igor Mammedov wrote:
> > 
> > No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
> > only for the last machine type?
> 
> As Michael sad it's for all machine types or have you meant 2.1 stable
> branch? My thought was that it's closed now.

I meant pc-2.1.  But if you and Michael agree, it's fine to have it
there as well.

2.1 stable branch should be open until 2.1.3 is released, shortly after
2.2.0.  2.1.2 was unplanned, so there should be a .3 release like we had
2.1.3.

Hmm, http://wiki.qemu.org/index.php?title=Planning/2.1 disagrees.  No
big deal.

Paolo

> I've tested it with RHEL6-7x64 and xp3,Windows server 2003-2012R2x64.
> So far no regression was noticed.

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Igor Mammedov]

On Tue, 11 Nov 2014 12:08:06 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 11/11/2014 09:35, Igor Mammedov wrote:
> > > 
> > > No, I mean can you do it also for pc-2.1 and earlier?  Or should it be
> > > only for the last machine type?
> > 
> > As Michael sad it's for all machine types or have you meant 2.1 stable
> > branch? My thought was that it's closed now.
> 
> I meant pc-2.1.  But if you and Michael agree, it's fine to have it
> there as well.
It should be safe, any migrated old machine will use original SRAT
unit reboot, after that it will pickup fixed table. Scope is limited to
 -numa + cpu_hotplug guests, for other configurations SRAT will stay the same.

> 
> 2.1 stable branch should be open until 2.1.3 is released, shortly after
> 2.2.0.  2.1.2 was unplanned, so there should be a .3 release like we had
> 2.1.3.
> 
> Hmm, http://wiki.qemu.org/index.php?title=Planning/2.1 disagrees.  No
> big deal.
> 
> Paolo
> 
> > I've tested it with RHEL6-7x64 and xp3,Windows server 2003-2012R2x64.
> > So far no regression was noticed.
> 

Re: [Qemu-devel] [PATCH for-2.2] pc: acpi: mark all possible CPUs as enabled in SRAT

[Igor Mammedov]

On Mon, 10 Nov 2014 16:20:50 +0000
Igor Mammedov <imammedo@redhat.com> wrote:

> If QEMU is started with  -numa ... Windows only notices that
> CPU has been hot-added but it will not online such CPUs.
> 
> It's caused by the fact that possible CPUs are flagged as
> not enabled in SRAT and Windows honoring that information
> doesn't use corresponding CPU.
> 
> ACPI 5.0 Spec regarding to flag says:
> "
> Table 5-47 Local APIC Flags
> ...
> Enabled: if zero, this processor is unusable, and the operating system
> support will not attempt to use it.
> "
> 
> Fix QEMU to adhere to spec and mark possible CPUs as enabled
> in SRAT.
> 
> With that Windows onlines hot-added CPUs as expected.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
ping

> ---
>  hw/i386/acpi-build.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4003b6b..06499da 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1269,8 +1269,7 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>  }
>  
>  static void
> -build_srat(GArray *table_data, GArray *linker,
> -           AcpiCpuInfo *cpu, PcGuestInfo *guest_info)
> +build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
>  {
>      AcpiSystemResourceAffinityTable *srat;
>      AcpiSratProcessorAffinity *core;
> @@ -1300,11 +1299,7 @@ build_srat(GArray *table_data, GArray *linker,
>          core->proximity_lo = curnode;
>          memset(core->proximity_hi, 0, 3);
>          core->local_sapic_eid = 0;
> -        if (test_bit(i, cpu->found_cpus)) {
> -            core->flags = cpu_to_le32(1);
> -        } else {
> -            core->flags = cpu_to_le32(0);
> -        }
> +        core->flags = cpu_to_le32(1);
>      }
>  
>  
> @@ -1622,7 +1617,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>      }
>      if (guest_info->numa_nodes) {
>          acpi_add_table(table_offsets, tables->table_data);
> -        build_srat(tables->table_data, tables->linker, &cpu, guest_info);
> +        build_srat(tables->table_data, tables->linker, guest_info);
>      }
>      if (acpi_get_mcfg(&mcfg)) {
>          acpi_add_table(table_offsets, tables->table_data);