[Qemu-devel] [for-2.1 PATCH v2 0/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Qemu-devel] [for-2.1 PATCH v2 0/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Laszlo Ersek]

New in v2:
- simplify patch 2/2 by keeping the "acpi_cpus" local variable, only
  fixing up its type, and changing its initialization.

The current SSDT generator doesn't support hotplug of the VCPU with APIC
ID 0xFF; supply that functionality.

The series depends on Eduardo's

  [Qemu-devel] [PATCH v4 0/7] pc: Ensure APIC ID limits before aborting
                              or corrupting memory

Regression tested v1 with 4 VCPUs. Iasl disassembly of the SSDT remains
identical (modulo length / checksum).

Laszlo Ersek (2):
  i386/acpi-build: allow more than 255 elements in CPON
  i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

 hw/i386/acpi-build.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [for-2.1 PATCH v2 1/2] i386/acpi-build: allow more than 255 elements in CPON

[Laszlo Ersek]

The build_ssdt() function builds a number of AML objects that are related
to CPU hotplug, and whose IDs form a contiguous sequence of APIC IDs.
(APIC IDs are in fact discontiguous, but this is the traditional
interface: build a contiguous sequence from zero up that covers all
possible APIC IDs.) These objects are:

- a Processor() object for each VCPU,
- a NTFY method, with one branch for each VCPU,
- a CPON package with one element (hotplug status byte) for each VCPU.

The build_ssdt() function currently limits the *count* of processor
objects, and NTFY branches, and CPON elements, in 0xFF (see the assignment
to "acpi_cpus"). This allows for an inclusive APIC ID range of [0..254].
This is incorrect, because the highest APIC ID that we otherwise allow a
VCPU to take is 255.

In order to extend the maximum count to 256, and the traversed APIC ID
range correspondingly to [0..255]:
- the Processor() objects need no change,
- the NTFY method also needs no change,
- the CPON package must be updated, because it is defined with a
  DefPackage, and the number of elements in such a package can be at most
  255. We pick a DefVarPackage instead.

We replace the Op byte, and the encoding of the number of elements.
Compare:

DefPackage     := PackageOp    PkgLength NumElements    PackageElementList
DefVarPackage  := VarPackageOp PkgLength VarNumElements PackageElementList

PackageOp      := 0x12
VarPackageOp   := 0x13

NumElements    := ByteData
VarNumElements := TermArg => Integer

The build_append_int() function implements precisely the following TermArg
encodings (a subset of what the ACPI spec describes):

  TermArg             := DataObject
  DataObject          := ComputationalData
  ComputationalData   := ConstObj | ByteConst | WordConst | DWordConst

  directly encoded in the function, with build_append_byte():
    ConstObj          := ZeroOp | OneOp
      ZeroOp          := 0x00
      OneOp           := 0x01

  call to build_append_value(..., 1):
    ByteConst         := BytePrefix ByteData
      BytePrefix      := 0x0A
      ByteData        := 0x00 - 0xFF

  call to build_append_value(..., 2):
    WordConst         := WordPrefix WordData
      WordPrefix      := 0x0B
      WordData        := ByteData[0:7] ByteData[8:15]

  call to build_append_value(..., 4):
    DWordConst        := DWordPrefix DWordData
      DWordPrefix     := 0x0C
      DWordData       := WordData[0:15] WordData[16:31]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index da2741c..2bbefb5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1050,9 +1050,9 @@ build_ssdt(GArray *table_data, GArray *linker,
 
         {
             GArray *package = build_alloc_array();
-            uint8_t op = 0x12; /* PackageOp */
+            uint8_t op = 0x13; /* VarPackageOp */
 
-            build_append_byte(package, acpi_cpus); /* NumElements */
+            build_append_int(package, acpi_cpus); /* VarNumElements */
             for (i = 0; i < acpi_cpus; i++) {
                 uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
                 build_append_byte(package, b);
-- 
1.8.3.1

[Qemu-devel] [for-2.1 PATCH v2 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Laszlo Ersek]

Building on the previous patch, raise the maximal count of processor
objects / NTFY branches / CPON elements from 255 to 256. This allows the
VCPU with APIC ID 0xFF to be hotplugged.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2bbefb5..c9fe07f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -999,11 +999,16 @@ build_ssdt(GArray *table_data, GArray *linker,
            AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
            PcPciInfo *pci, PcGuestInfo *guest_info)
 {
-    int acpi_cpus = MIN(0xff, guest_info->apic_id_limit);
+    unsigned acpi_cpus = guest_info->apic_id_limit;
     int ssdt_start = table_data->len;
     uint8_t *ssdt_ptr;
     int i;
 
+    /* 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);
+
     /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
     ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));
     memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
-- 
1.8.3.1

Re: [Qemu-devel] [for-2.1 PATCH v2 0/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Michael S. Tsirkin]

On Mon, Mar 17, 2014 at 05:05:15PM +0100, Laszlo Ersek wrote:
> New in v2:
> - simplify patch 2/2 by keeping the "acpi_cpus" local variable, only
>   fixing up its type, and changing its initialization.

Applied, thanks!

> The current SSDT generator doesn't support hotplug of the VCPU with APIC
> ID 0xFF; supply that functionality.
> 
> The series depends on Eduardo's
> 
>   [Qemu-devel] [PATCH v4 0/7] pc: Ensure APIC ID limits before aborting
>                               or corrupting memory
> 
> Regression tested v1 with 4 VCPUs. Iasl disassembly of the SSDT remains
> identical (modulo length / checksum).
> 
> Laszlo Ersek (2):
>   i386/acpi-build: allow more than 255 elements in CPON
>   i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF
> 
>  hw/i386/acpi-build.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [for-2.1 PATCH v2 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Michael S. Tsirkin]

On Mon, Mar 17, 2014 at 05:05:17PM +0100, Laszlo Ersek wrote:
> Building on the previous patch, raise the maximal count of processor
> objects / NTFY branches / CPON elements from 255 to 256. This allows the
> VCPU with APIC ID 0xFF to be hotplugged.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>


I note that we still have:
    if (endvalue >= MAX_CPUMASK_BITS) {
        endvalue = MAX_CPUMASK_BITS - 1;
        fprintf(stderr,
            "qemu: NUMA: A max of %d VCPUs are supported\n",
             MAX_CPUMASK_BITS);
    }
and MAX_CPUMASK_BITS is 255.

Seems inconsistent?

> ---
>  hw/i386/acpi-build.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2bbefb5..c9fe07f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -999,11 +999,16 @@ build_ssdt(GArray *table_data, GArray *linker,
>             AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
>             PcPciInfo *pci, PcGuestInfo *guest_info)
>  {
> -    int acpi_cpus = MIN(0xff, guest_info->apic_id_limit);
> +    unsigned acpi_cpus = guest_info->apic_id_limit;
>      int ssdt_start = table_data->len;
>      uint8_t *ssdt_ptr;
>      int i;
>  
> +    /* 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);
> +
>      /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
>      ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));
>      memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
> -- 
> 1.8.3.1

Re: [Qemu-devel] [for-2.1 PATCH v2 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Eduardo Habkost]

On Tue, Mar 18, 2014 at 04:03:25PM +0200, Michael S. Tsirkin wrote:
> On Mon, Mar 17, 2014 at 05:05:17PM +0100, Laszlo Ersek wrote:
> > Building on the previous patch, raise the maximal count of processor
> > objects / NTFY branches / CPON elements from 255 to 256. This allows the
> > VCPU with APIC ID 0xFF to be hotplugged.
> > 
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> 
> I note that we still have:
>     if (endvalue >= MAX_CPUMASK_BITS) {
>         endvalue = MAX_CPUMASK_BITS - 1;
>         fprintf(stderr,
>             "qemu: NUMA: A max of %d VCPUs are supported\n",
>              MAX_CPUMASK_BITS);
>     }
> and MAX_CPUMASK_BITS is 255.
> 
> Seems inconsistent?
> 

MAX_CPUMASK_BITS (now renamed to MAX_CPUS) limits CPU indexes and total
CPU count. This patch is about APIC IDs (which may be larger than
max_cpus if threads-per-core or cores-per-socket is not a power of 2).

(That doesn't mean we can't decide to increase MAX_CPUS later, too.)


> > ---
> >  hw/i386/acpi-build.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 2bbefb5..c9fe07f 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -999,11 +999,16 @@ build_ssdt(GArray *table_data, GArray *linker,
> >             AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> >             PcPciInfo *pci, PcGuestInfo *guest_info)
> >  {
> > -    int acpi_cpus = MIN(0xff, guest_info->apic_id_limit);
> > +    unsigned acpi_cpus = guest_info->apic_id_limit;
> >      int ssdt_start = table_data->len;
> >      uint8_t *ssdt_ptr;
> >      int i;
> >  
> > +    /* 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);
> > +
> >      /* Copy header and patch values in the S3_ / S4_ / S5_ packages */
> >      ssdt_ptr = acpi_data_push(table_data, sizeof(ssdp_misc_aml));
> >      memcpy(ssdt_ptr, ssdp_misc_aml, sizeof(ssdp_misc_aml));
> > -- 
> > 1.8.3.1

-- 
Eduardo

Re: [Qemu-devel] [for-2.1 PATCH v2 2/2] i386/acpi-build: support hotplug of VCPU with APIC ID 0xFF

[Laszlo Ersek]

On 03/18/14 15:54, Eduardo Habkost wrote:
> On Tue, Mar 18, 2014 at 04:03:25PM +0200, Michael S. Tsirkin wrote:
>> On Mon, Mar 17, 2014 at 05:05:17PM +0100, Laszlo Ersek wrote:
>>> Building on the previous patch, raise the maximal count of processor
>>> objects / NTFY branches / CPON elements from 255 to 256. This allows the
>>> VCPU with APIC ID 0xFF to be hotplugged.
>>>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>>
>> I note that we still have:
>>     if (endvalue >= MAX_CPUMASK_BITS) {
>>         endvalue = MAX_CPUMASK_BITS - 1;
>>         fprintf(stderr,
>>             "qemu: NUMA: A max of %d VCPUs are supported\n",
>>              MAX_CPUMASK_BITS);
>>     }
>> and MAX_CPUMASK_BITS is 255.
>>
>> Seems inconsistent?
>>
> 
> MAX_CPUMASK_BITS (now renamed to MAX_CPUS) limits CPU indexes and total
> CPU count. This patch is about APIC IDs (which may be larger than
> max_cpus if threads-per-core or cores-per-socket is not a power of 2).

Yea I welcome Eduardo's patchset not only because it fixes the
out-of-range accesses caused by "uncontrolled" APIC IDs, but also
because it disentangles these limits from one another.

Thanks
Laszlo