[Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[Michael S. Tsirkin]

VarPackage can accept an expression evaluating to int, not just an int.
Change the API to make it more generic.
Further, rather than have users call the correct API depending on
value passed, use either PackageOp or VarPackageOp automatically.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  4 ++--
 hw/acpi/aml-build.c         | 18 ++++++++++++++----
 hw/acpi/cpu_hotplug.c       | 11 ++---------
 hw/arm/virt-acpi-build.c    |  2 +-
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 10c7946028..7cf2cf64bf 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -360,7 +360,7 @@ Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag);
 Aml *aml_if(Aml *predicate);
 Aml *aml_else(void);
 Aml *aml_while(Aml *predicate);
-Aml *aml_package(uint8_t num_elements);
+Aml *aml_package(uint64_t num_elements);
 Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
 Aml *aml_resource_template(void);
 Aml *aml_field(const char *name, AmlAccessType type, AmlLockRule lock,
@@ -373,7 +373,7 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
                       const char *name);
 Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name);
 Aml *aml_create_qword_field(Aml *srcbuf, Aml *index, const char *name);
-Aml *aml_varpackage(uint32_t num_elements);
+Aml *aml_varpackage(Aml *num_elements);
 Aml *aml_touuid(const char *uuid);
 Aml *aml_unicode(const char *str);
 Aml *aml_refof(Aml *arg);
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index def62b3112..1996768b40 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1016,9 +1016,19 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
 }
 
 /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefPackage */
-Aml *aml_package(uint8_t num_elements)
+/* Note: The ability to create variable-sized packages was first
+ * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
+ * with up to 255 elements. Windows guests up to win2k8 fail when
+ * VarPackageOp is used.
+ */
+Aml *aml_package(uint64_t num_elements)
 {
-    Aml *var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
+    Aml *var;
+
+    if (num_elements > 0xFF)
+        return aml_varpackage(aml_int(num_elements));
+
+    var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
     build_append_byte(var->buf, num_elements);
     return var;
 }
@@ -1136,10 +1146,10 @@ Aml *aml_local(int num)
 }
 
 /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
-Aml *aml_varpackage(uint32_t num_elements)
+Aml *aml_varpackage(Aml *num_elements)
 {
     Aml *var = aml_bundle(0x13 /* VarPackageOp */, AML_PACKAGE);
-    build_append_int(var->buf, num_elements);
+    aml_append(var, num_elements);
     return var;
 }
 
diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 5243918125..f8246fca6f 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -309,15 +309,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
     }
     aml_append(sb_scope, method);
 
-    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
-     *
-     * Note: The ability to create variable-sized packages was first
-     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
-     * ith up to 255 elements. Windows guests up to win2k8 fail when
-     * VarPackageOp is used.
-     */
-    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
-                                       aml_varpackage(pcms->apic_id_limit);
+    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
+    pkg = aml_package(pcms->apic_id_limit);
 
     for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
         int apic_id = apic_ids->cpus[i].arch_id;
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6ea47e2588..2adb1de378 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -174,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
 
     /* Declare the PCI Routing Table. */
-    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
+    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
     for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
         for (i = 0; i < PCI_NUM_PINS; i++) {
             int gsi = (i + bus_no) % PCI_NUM_PINS;
-- 
MST

Re: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180705235305.124423-1-mst@redhat.com
Subject: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20180705235305.124423-1-mst@redhat.com -> patchew/20180705235305.124423-1-mst@redhat.com
Switched to a new branch 'test'
cbb72d5a3a acpi: generalize aml_package / aml_varpackage

=== OUTPUT BEGIN ===
Checking PATCH 1/1: acpi: generalize aml_package / aml_varpackage...
ERROR: braces {} are necessary for all arms of this statement
#33: FILE: hw/acpi/aml-build.c:1007:
+    if (num_elements > 0xFF)
[...]

total: 1 errors, 0 warnings, 74 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[Igor Mammedov]

On Fri, 6 Jul 2018 02:53:09 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> VarPackage can accept an expression evaluating to int, not just an int.
> Change the API to make it more generic.
> Further, rather than have users call the correct API depending on
> value passed, use either PackageOp or VarPackageOp automatically.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/acpi/aml-build.h |  4 ++--
>  hw/acpi/aml-build.c         | 18 ++++++++++++++----
>  hw/acpi/cpu_hotplug.c       | 11 ++---------
>  hw/arm/virt-acpi-build.c    |  2 +-
>  4 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 10c7946028..7cf2cf64bf 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -360,7 +360,7 @@ Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag);
>  Aml *aml_if(Aml *predicate);
>  Aml *aml_else(void);
>  Aml *aml_while(Aml *predicate);
> -Aml *aml_package(uint8_t num_elements);
> +Aml *aml_package(uint64_t num_elements);
>  Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
>  Aml *aml_resource_template(void);
>  Aml *aml_field(const char *name, AmlAccessType type, AmlLockRule lock,
> @@ -373,7 +373,7 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
>                        const char *name);
>  Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name);
>  Aml *aml_create_qword_field(Aml *srcbuf, Aml *index, const char *name);
> -Aml *aml_varpackage(uint32_t num_elements);
> +Aml *aml_varpackage(Aml *num_elements);
maybe drop it from header and make static so only aml_package() would be
left as public API like in spec?


>  Aml *aml_touuid(const char *uuid);
>  Aml *aml_unicode(const char *str);
>  Aml *aml_refof(Aml *arg);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index def62b3112..1996768b40 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1016,9 +1016,19 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
>  }
>  
>  /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefPackage */
> -Aml *aml_package(uint8_t num_elements)
> +/* Note: The ability to create variable-sized packages was first
> + * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> + * with up to 255 elements. Windows guests up to win2k8 fail when
> + * VarPackageOp is used.
> + */
> +Aml *aml_package(uint64_t num_elements)
>  {
> -    Aml *var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> +    Aml *var;
> +
> +    if (num_elements > 0xFF)
> +        return aml_varpackage(aml_int(num_elements));
> +
> +    var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
>      build_append_byte(var->buf, num_elements);
>      return var;
>  }
> @@ -1136,10 +1146,10 @@ Aml *aml_local(int num)
>  }
>  
>  /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
> -Aml *aml_varpackage(uint32_t num_elements)
> +Aml *aml_varpackage(Aml *num_elements)
>  {
>      Aml *var = aml_bundle(0x13 /* VarPackageOp */, AML_PACKAGE);
> -    build_append_int(var->buf, num_elements);
> +    aml_append(var, num_elements);
>      return var;
>  }
>  
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 5243918125..f8246fca6f 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -309,15 +309,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>      }
>      aml_append(sb_scope, method);
>  
> -    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
> -     *
> -     * Note: The ability to create variable-sized packages was first
> -     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> -     * ith up to 255 elements. Windows guests up to win2k8 fail when
> -     * VarPackageOp is used.
> -     */
> -    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> -                                       aml_varpackage(pcms->apic_id_limit);
> +    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> +    pkg = aml_package(pcms->apic_id_limit);
>  
>      for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
>          int apic_id = apic_ids->cpus[i].arch_id;
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6ea47e2588..2adb1de378 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -174,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>  
>      /* Declare the PCI Routing Table. */
> -    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
> +    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
>      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
>          for (i = 0; i < PCI_NUM_PINS; i++) {
>              int gsi = (i + bus_no) % PCI_NUM_PINS;

Re: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[Michael S. Tsirkin]

On Mon, Jul 09, 2018 at 05:52:32PM +0200, Igor Mammedov wrote:
> On Fri, 6 Jul 2018 02:53:09 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > VarPackage can accept an expression evaluating to int, not just an int.
> > Change the API to make it more generic.
> > Further, rather than have users call the correct API depending on
> > value passed, use either PackageOp or VarPackageOp automatically.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/acpi/aml-build.h |  4 ++--
> >  hw/acpi/aml-build.c         | 18 ++++++++++++++----
> >  hw/acpi/cpu_hotplug.c       | 11 ++---------
> >  hw/arm/virt-acpi-build.c    |  2 +-
> >  4 files changed, 19 insertions(+), 16 deletions(-)
> > 
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index 10c7946028..7cf2cf64bf 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -360,7 +360,7 @@ Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag);
> >  Aml *aml_if(Aml *predicate);
> >  Aml *aml_else(void);
> >  Aml *aml_while(Aml *predicate);
> > -Aml *aml_package(uint8_t num_elements);
> > +Aml *aml_package(uint64_t num_elements);
> >  Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
> >  Aml *aml_resource_template(void);
> >  Aml *aml_field(const char *name, AmlAccessType type, AmlLockRule lock,
> > @@ -373,7 +373,7 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
> >                        const char *name);
> >  Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name);
> >  Aml *aml_create_qword_field(Aml *srcbuf, Aml *index, const char *name);
> > -Aml *aml_varpackage(uint32_t num_elements);
> > +Aml *aml_varpackage(Aml *num_elements);
> maybe drop it from header and make static so only aml_package() would be
> left as public API like in spec?

There are places in the spec that say VarPackage.
E.g. _CST: Return Value: A variable-length Package.

We probably want to keep aml_varpackage around for these
cases.

> 
> >  Aml *aml_touuid(const char *uuid);
> >  Aml *aml_unicode(const char *str);
> >  Aml *aml_refof(Aml *arg);
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index def62b3112..1996768b40 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1016,9 +1016,19 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
> >  }
> >  
> >  /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefPackage */
> > -Aml *aml_package(uint8_t num_elements)
> > +/* Note: The ability to create variable-sized packages was first
> > + * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> > + * with up to 255 elements. Windows guests up to win2k8 fail when
> > + * VarPackageOp is used.
> > + */
> > +Aml *aml_package(uint64_t num_elements)
> >  {
> > -    Aml *var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> > +    Aml *var;
> > +
> > +    if (num_elements > 0xFF)
> > +        return aml_varpackage(aml_int(num_elements));
> > +
> > +    var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> >      build_append_byte(var->buf, num_elements);
> >      return var;
> >  }
> > @@ -1136,10 +1146,10 @@ Aml *aml_local(int num)
> >  }
> >  
> >  /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
> > -Aml *aml_varpackage(uint32_t num_elements)
> > +Aml *aml_varpackage(Aml *num_elements)
> >  {
> >      Aml *var = aml_bundle(0x13 /* VarPackageOp */, AML_PACKAGE);
> > -    build_append_int(var->buf, num_elements);
> > +    aml_append(var, num_elements);
> >      return var;
> >  }
> >  
> > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > index 5243918125..f8246fca6f 100644
> > --- a/hw/acpi/cpu_hotplug.c
> > +++ b/hw/acpi/cpu_hotplug.c
> > @@ -309,15 +309,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> >      }
> >      aml_append(sb_scope, method);
> >  
> > -    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
> > -     *
> > -     * Note: The ability to create variable-sized packages was first
> > -     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> > -     * ith up to 255 elements. Windows guests up to win2k8 fail when
> > -     * VarPackageOp is used.
> > -     */
> > -    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > -                                       aml_varpackage(pcms->apic_id_limit);
> > +    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> > +    pkg = aml_package(pcms->apic_id_limit);
> >  
> >      for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> >          int apic_id = apic_ids->cpus[i].arch_id;
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 6ea47e2588..2adb1de378 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -174,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> >      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> >  
> >      /* Declare the PCI Routing Table. */
> > -    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
> > +    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
> >      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
> >          for (i = 0; i < PCI_NUM_PINS; i++) {
> >              int gsi = (i + bus_no) % PCI_NUM_PINS;

Re: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[Michael S. Tsirkin]

On Mon, Jul 09, 2018 at 08:19:17PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 09, 2018 at 05:52:32PM +0200, Igor Mammedov wrote:
> > On Fri, 6 Jul 2018 02:53:09 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > VarPackage can accept an expression evaluating to int, not just an int.
> > > Change the API to make it more generic.
> > > Further, rather than have users call the correct API depending on
> > > value passed, use either PackageOp or VarPackageOp automatically.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/hw/acpi/aml-build.h |  4 ++--
> > >  hw/acpi/aml-build.c         | 18 ++++++++++++++----
> > >  hw/acpi/cpu_hotplug.c       | 11 ++---------
> > >  hw/arm/virt-acpi-build.c    |  2 +-
> > >  4 files changed, 19 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index 10c7946028..7cf2cf64bf 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -360,7 +360,7 @@ Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag);
> > >  Aml *aml_if(Aml *predicate);
> > >  Aml *aml_else(void);
> > >  Aml *aml_while(Aml *predicate);
> > > -Aml *aml_package(uint8_t num_elements);
> > > +Aml *aml_package(uint64_t num_elements);
> > >  Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
> > >  Aml *aml_resource_template(void);
> > >  Aml *aml_field(const char *name, AmlAccessType type, AmlLockRule lock,
> > > @@ -373,7 +373,7 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
> > >                        const char *name);
> > >  Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name);
> > >  Aml *aml_create_qword_field(Aml *srcbuf, Aml *index, const char *name);
> > > -Aml *aml_varpackage(uint32_t num_elements);
> > > +Aml *aml_varpackage(Aml *num_elements);
> > maybe drop it from header and make static so only aml_package() would be
> > left as public API like in spec?
> 
> There are places in the spec that say VarPackage.
> E.g. _CST: Return Value: A variable-length Package.
> 
> We probably want to keep aml_varpackage around for these
> cases.

And thinking more about this, if you want a package with
# of elements that isn't a constant, you want aml_varpackage.
If it's a constant you have the handy aml_package shortcut.

> > 
> > >  Aml *aml_touuid(const char *uuid);
> > >  Aml *aml_unicode(const char *str);
> > >  Aml *aml_refof(Aml *arg);
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index def62b3112..1996768b40 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1016,9 +1016,19 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
> > >  }
> > >  
> > >  /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefPackage */
> > > -Aml *aml_package(uint8_t num_elements)
> > > +/* Note: The ability to create variable-sized packages was first
> > > + * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> > > + * with up to 255 elements. Windows guests up to win2k8 fail when
> > > + * VarPackageOp is used.
> > > + */
> > > +Aml *aml_package(uint64_t num_elements)
> > >  {
> > > -    Aml *var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> > > +    Aml *var;
> > > +
> > > +    if (num_elements > 0xFF)
> > > +        return aml_varpackage(aml_int(num_elements));
> > > +
> > > +    var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> > >      build_append_byte(var->buf, num_elements);
> > >      return var;
> > >  }
> > > @@ -1136,10 +1146,10 @@ Aml *aml_local(int num)
> > >  }
> > >  
> > >  /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
> > > -Aml *aml_varpackage(uint32_t num_elements)
> > > +Aml *aml_varpackage(Aml *num_elements)
> > >  {
> > >      Aml *var = aml_bundle(0x13 /* VarPackageOp */, AML_PACKAGE);
> > > -    build_append_int(var->buf, num_elements);
> > > +    aml_append(var, num_elements);
> > >      return var;
> > >  }
> > >  
> > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > > index 5243918125..f8246fca6f 100644
> > > --- a/hw/acpi/cpu_hotplug.c
> > > +++ b/hw/acpi/cpu_hotplug.c
> > > @@ -309,15 +309,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> > >      }
> > >      aml_append(sb_scope, method);
> > >  
> > > -    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
> > > -     *
> > > -     * Note: The ability to create variable-sized packages was first
> > > -     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> > > -     * ith up to 255 elements. Windows guests up to win2k8 fail when
> > > -     * VarPackageOp is used.
> > > -     */
> > > -    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > > -                                       aml_varpackage(pcms->apic_id_limit);
> > > +    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> > > +    pkg = aml_package(pcms->apic_id_limit);
> > >  
> > >      for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> > >          int apic_id = apic_ids->cpus[i].arch_id;
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 6ea47e2588..2adb1de378 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -174,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> > >      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > >  
> > >      /* Declare the PCI Routing Table. */
> > > -    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
> > > +    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
> > >      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
> > >          for (i = 0; i < PCI_NUM_PINS; i++) {
> > >              int gsi = (i + bus_no) % PCI_NUM_PINS;

Re: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage

[Igor Mammedov]

On Tue, 10 Jul 2018 02:26:35 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 09, 2018 at 08:19:17PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 09, 2018 at 05:52:32PM +0200, Igor Mammedov wrote:  
> > > On Fri, 6 Jul 2018 02:53:09 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > VarPackage can accept an expression evaluating to int, not just an int.
> > > > Change the API to make it more generic.
> > > > Further, rather than have users call the correct API depending on
> > > > value passed, use either PackageOp or VarPackageOp automatically.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  include/hw/acpi/aml-build.h |  4 ++--
> > > >  hw/acpi/aml-build.c         | 18 ++++++++++++++----
> > > >  hw/acpi/cpu_hotplug.c       | 11 ++---------
> > > >  hw/arm/virt-acpi-build.c    |  2 +-
> > > >  4 files changed, 19 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > > index 10c7946028..7cf2cf64bf 100644
> > > > --- a/include/hw/acpi/aml-build.h
> > > > +++ b/include/hw/acpi/aml-build.h
> > > > @@ -360,7 +360,7 @@ Aml *aml_method(const char *name, int arg_count, AmlSerializeFlag sflag);
> > > >  Aml *aml_if(Aml *predicate);
> > > >  Aml *aml_else(void);
> > > >  Aml *aml_while(Aml *predicate);
> > > > -Aml *aml_package(uint8_t num_elements);
> > > > +Aml *aml_package(uint64_t num_elements);
> > > >  Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
> > > >  Aml *aml_resource_template(void);
> > > >  Aml *aml_field(const char *name, AmlAccessType type, AmlLockRule lock,
> > > > @@ -373,7 +373,7 @@ Aml *aml_create_field(Aml *srcbuf, Aml *bit_index, Aml *num_bits,
> > > >                        const char *name);
> > > >  Aml *aml_create_dword_field(Aml *srcbuf, Aml *index, const char *name);
> > > >  Aml *aml_create_qword_field(Aml *srcbuf, Aml *index, const char *name);
> > > > -Aml *aml_varpackage(uint32_t num_elements);
> > > > +Aml *aml_varpackage(Aml *num_elements);  
> > > maybe drop it from header and make static so only aml_package() would be
> > > left as public API like in spec?  
> > 
> > There are places in the spec that say VarPackage.
> > E.g. _CST: Return Value: A variable-length Package.
the only thing is that one has only "Package()" ASL to declare it,
compiler probably uses argument type to deduce which one to use.

> > 
> > We probably want to keep aml_varpackage around for these
> > cases.  
> 
> And thinking more about this, if you want a package with
> # of elements that isn't a constant, you want aml_varpackage.
> If it's a constant you have the handy aml_package shortcut.
not sure windows would like it (it seems to have issues with
dynamic packages (according to Marc-Andre tests))
but lets keep it for now we can always remove it later.

with style issue fixed:

Reviewed-by: Igor Mammedov <imammedo@redhat.com>



> 
> > >   
> > > >  Aml *aml_touuid(const char *uuid);
> > > >  Aml *aml_unicode(const char *str);
> > > >  Aml *aml_refof(Aml *arg);
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index def62b3112..1996768b40 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.c
> > > > @@ -1016,9 +1016,19 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
> > > >  }
> > > >  
> > > >  /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefPackage */
> > > > -Aml *aml_package(uint8_t num_elements)
> > > > +/* Note: The ability to create variable-sized packages was first
> > > > + * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> > > > + * with up to 255 elements. Windows guests up to win2k8 fail when
> > > > + * VarPackageOp is used.
> > > > + */
> > > > +Aml *aml_package(uint64_t num_elements)
> > > >  {
> > > > -    Aml *var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> > > > +    Aml *var;
> > > > +
> > > > +    if (num_elements > 0xFF)
> > > > +        return aml_varpackage(aml_int(num_elements));
> > > > +
> > > > +    var = aml_bundle(0x12 /* PackageOp */, AML_PACKAGE);
> > > >      build_append_byte(var->buf, num_elements);
> > > >      return var;
> > > >  }
> > > > @@ -1136,10 +1146,10 @@ Aml *aml_local(int num)
> > > >  }
> > > >  
> > > >  /* ACPI 2.0a: 17.2.2 Data Objects Encoding: DefVarPackage */
> > > > -Aml *aml_varpackage(uint32_t num_elements)
> > > > +Aml *aml_varpackage(Aml *num_elements)
> > > >  {
> > > >      Aml *var = aml_bundle(0x13 /* VarPackageOp */, AML_PACKAGE);
> > > > -    build_append_int(var->buf, num_elements);
> > > > +    aml_append(var, num_elements);
> > > >      return var;
> > > >  }
> > > >  
> > > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > > > index 5243918125..f8246fca6f 100644
> > > > --- a/hw/acpi/cpu_hotplug.c
> > > > +++ b/hw/acpi/cpu_hotplug.c
> > > > @@ -309,15 +309,8 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> > > >      }
> > > >      aml_append(sb_scope, method);
> > > >  
> > > > -    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })"
> > > > -     *
> > > > -     * Note: The ability to create variable-sized packages was first
> > > > -     * introduced in ACPI 2.0. ACPI 1.0 only allowed fixed-size packages
> > > > -     * ith up to 255 elements. Windows guests up to win2k8 fail when
> > > > -     * VarPackageOp is used.
> > > > -     */
> > > > -    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > > > -                                       aml_varpackage(pcms->apic_id_limit);
> > > > +    /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> > > > +    pkg = aml_package(pcms->apic_id_limit);
> > > >  
> > > >      for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> > > >          int apic_id = apic_ids->cpus[i].arch_id;
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 6ea47e2588..2adb1de378 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -174,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> > > >      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > > >  
> > > >      /* Declare the PCI Routing Table. */
> > > > -    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
> > > > +    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
> > > >      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
> > > >          for (i = 0; i < PCI_NUM_PINS; i++) {
> > > >              int gsi = (i + bus_no) % PCI_NUM_PINS;