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

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

[Laszlo Ersek]

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 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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.1 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] [PATCH for-2.1 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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2bbefb5..51162fc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -999,11 +999,15 @@ 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);
     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(guest_info->apic_id_limit <= 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));
@@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         build_append_nameseg(sb_scope, "_SB_");
 
         /* build Processor object for each processor */
-        for (i = 0; i < acpi_cpus; i++) {
+        for (i = 0; i < guest_info->apic_id_limit; i++) {
             uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF);
             memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF);
             proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4);
@@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker,
          *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
          */
         /* Arg0 = Processor ID = APIC ID */
-        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
+        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X",
+                                   guest_info->apic_id_limit);
 
         /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
         build_append_byte(sb_scope, 0x08); /* NameOp */
@@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker,
             GArray *package = build_alloc_array();
             uint8_t op = 0x13; /* VarPackageOp */
 
-            build_append_int(package, acpi_cpus); /* VarNumElements */
-            for (i = 0; i < acpi_cpus; i++) {
+            build_append_int(package,
+                             guest_info->apic_id_limit); /* VarNumElements */
+            for (i = 0; i < guest_info->apic_id_limit; i++) {
                 uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
                 build_append_byte(package, b);
             }
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Fri, Mar 14, 2014 at 11:22:52PM +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>
> ---
>  hw/i386/acpi-build.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2bbefb5..51162fc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -999,11 +999,15 @@ 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);

Maybe just make this line
     int acpi_cpus = guest_info->apic_id_limit;
and then the patch will be smaller.

>      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(guest_info->apic_id_limit <= 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));
> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          build_append_nameseg(sb_scope, "_SB_");
>  
>          /* build Processor object for each processor */
> -        for (i = 0; i < acpi_cpus; i++) {
> +        for (i = 0; i < guest_info->apic_id_limit; i++) {
>              uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF);
>              memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF);
>              proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4);
> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker,
>           *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
>           */
>          /* Arg0 = Processor ID = APIC ID */
> -        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
> +        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X",
> +                                   guest_info->apic_id_limit);
>  
>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
>          build_append_byte(sb_scope, 0x08); /* NameOp */
> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker,
>              GArray *package = build_alloc_array();
>              uint8_t op = 0x13; /* VarPackageOp */
>  
> -            build_append_int(package, acpi_cpus); /* VarNumElements */
> -            for (i = 0; i < acpi_cpus; i++) {
> +            build_append_int(package,
> +                             guest_info->apic_id_limit); /* VarNumElements */
> +            for (i = 0; i < guest_info->apic_id_limit; i++) {
>                  uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
>                  build_append_byte(package, b);
>              }
> -- 
> 1.8.3.1

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

[Laszlo Ersek]

On 03/16/14 13:31, Michael S. Tsirkin wrote:
> On Fri, Mar 14, 2014 at 11:22:52PM +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>
>> ---
>>  hw/i386/acpi-build.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 2bbefb5..51162fc 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -999,11 +999,15 @@ 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);
> 
> Maybe just make this line
>      int acpi_cpus = guest_info->apic_id_limit;
> and then the patch will be smaller.

I did think of that, but I didn't like the unchecked unsigned --> int
conversion. The limit checks below cover that too.

Also, the patch renders the function similar to the other acpi builder
functions; there are a handful of loops that directly name
"guest_info->apic_id_limit" in their controlling expressions.

Anyway, would you be OK with a patch that did the assignment you suggest
(rather than modifying the loops), and kept the BUILD_BUG and the
g_assert below?

Thanks
Laszlo

> 
>>      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(guest_info->apic_id_limit <= 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));
>> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>          build_append_nameseg(sb_scope, "_SB_");
>>  
>>          /* build Processor object for each processor */
>> -        for (i = 0; i < acpi_cpus; i++) {
>> +        for (i = 0; i < guest_info->apic_id_limit; i++) {
>>              uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF);
>>              memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF);
>>              proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4);
>> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker,
>>           *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
>>           */
>>          /* Arg0 = Processor ID = APIC ID */
>> -        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
>> +        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X",
>> +                                   guest_info->apic_id_limit);
>>  
>>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
>>          build_append_byte(sb_scope, 0x08); /* NameOp */
>> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker,
>>              GArray *package = build_alloc_array();
>>              uint8_t op = 0x13; /* VarPackageOp */
>>  
>> -            build_append_int(package, acpi_cpus); /* VarNumElements */
>> -            for (i = 0; i < acpi_cpus; i++) {
>> +            build_append_int(package,
>> +                             guest_info->apic_id_limit); /* VarNumElements */
>> +            for (i = 0; i < guest_info->apic_id_limit; i++) {
>>                  uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
>>                  build_append_byte(package, b);
>>              }
>> -- 
>> 1.8.3.1

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

[Michael S. Tsirkin]

On Mon, Mar 17, 2014 at 11:08:46AM +0100, Laszlo Ersek wrote:
> On 03/16/14 13:31, Michael S. Tsirkin wrote:
> > On Fri, Mar 14, 2014 at 11:22:52PM +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>
> >> ---
> >>  hw/i386/acpi-build.c | 16 +++++++++++-----
> >>  1 file changed, 11 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 2bbefb5..51162fc 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -999,11 +999,15 @@ 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);
> > 
> > Maybe just make this line
> >      int acpi_cpus = guest_info->apic_id_limit;
> > and then the patch will be smaller.
> 
> I did think of that, but I didn't like the unchecked unsigned --> int
> conversion. The limit checks below cover that too.

Make it
	unsigned acpi_cpus
then?

> Also, the patch renders the function similar to the other acpi builder
> functions; there are a handful of loops that directly name
> "guest_info->apic_id_limit" in their controlling expressions.

I'm fine with refactoring this if you feel it's
more readable (I don't bit I wrote this so I know I'm biased),
but let's make it a separate patch then.
 
> Anyway, would you be OK with a patch that did the assignment you suggest
> (rather than modifying the loops), and kept the BUILD_BUG and the
> g_assert below?
> 
> Thanks
> Laszlo

Yes, fine.


> > 
> >>      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(guest_info->apic_id_limit <= 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));
> >> @@ -1029,7 +1033,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>          build_append_nameseg(sb_scope, "_SB_");
> >>  
> >>          /* build Processor object for each processor */
> >> -        for (i = 0; i < acpi_cpus; i++) {
> >> +        for (i = 0; i < guest_info->apic_id_limit; i++) {
> >>              uint8_t *proc = acpi_data_push(sb_scope, ACPI_PROC_SIZEOF);
> >>              memcpy(proc, ACPI_PROC_AML, ACPI_PROC_SIZEOF);
> >>              proc[ACPI_PROC_OFFSET_CPUHEX] = acpi_get_hex(i >> 4);
> >> @@ -1042,7 +1046,8 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>           *   Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}
> >>           */
> >>          /* Arg0 = Processor ID = APIC ID */
> >> -        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X", acpi_cpus);
> >> +        build_append_notify_method(sb_scope, "NTFY", "CP%0.02X",
> >> +                                   guest_info->apic_id_limit);
> >>  
> >>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> >>          build_append_byte(sb_scope, 0x08); /* NameOp */
> >> @@ -1052,8 +1057,9 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>              GArray *package = build_alloc_array();
> >>              uint8_t op = 0x13; /* VarPackageOp */
> >>  
> >> -            build_append_int(package, acpi_cpus); /* VarNumElements */
> >> -            for (i = 0; i < acpi_cpus; i++) {
> >> +            build_append_int(package,
> >> +                             guest_info->apic_id_limit); /* VarNumElements */
> >> +            for (i = 0; i < guest_info->apic_id_limit; i++) {
> >>                  uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
> >>                  build_append_byte(package, b);
> >>              }
> >> -- 
> >> 1.8.3.1