From mboxrd@z Thu Jan 1 00:00:00 1970 Received: by 2002:a5d:4308:0:0:0:0:0 with SMTP id h8-v6csp2946957wrq; Mon, 9 Jul 2018 16:28:00 -0700 (PDT) X-Google-Smtp-Source: AAOMgpeME1YrfJRCT49y/weT0kl5m9bTQquz1T4ae4Rg/CtShNqrN/labM7Jehmr80ztCNFITgoK X-Received: by 2002:a0c:886d:: with SMTP id 42-v6mr19788270qvm.242.1531178879999; Mon, 09 Jul 2018 16:27:59 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1531178879; cv=none; d=google.com; s=arc-20160816; b=t6s8HZIV8NFOuiG5Vq4VOs7QcLSZA06LDj3RXvaPpRndHGhUMMVFz7zlskmitYqKHF cWAwX0onQpbfKq0chNNtQ6Wr6OmuPcf7DozkO4vnBdMvvCkBt1o4e2sG/fJD/IuS8KsC hHtaXnHgGHFrmmT7gjWIHmK0QX52kkYeDWSZgHVnK/Y79OOTRvCuoReSCDnVPCcdzDV0 QXI4f56X36LOvcNYxRKDB0IG3LX+SL73qcn++HODG3mqtYrrn5kCm2oYtE+PkTn+kNxE nIkoc9IfZrl8OMaCcECy8zu47a5cleVShlNkfdKvqkC97mw86Tpfbqh2GPMdcGT2ETb1 J0tw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:subject:in-reply-to :content-disposition:mime-version:references:message-id:to:from:date :arc-authentication-results; bh=6gyiFMDA3Voeh/EOT/apgRZ228C7wC3guAMekVLHwc4=; b=hJ+8OXOZ/U4+8nWPwIj7HLiOzW/fLeAJ/WQJALRZ3jz1UuDyZanOfluBzsd02OP6/K FjjlWe7++k9aNS06OTLibL8o8PRgHpS88bfpubHdz931OD3rZodF1oNcgL5Uw4CU+50a Nz7Pi2qB0h/e1qdHB2duCiQNigL4TaEVMeLhkIL2hHpwVOpFJFa+T50ukJUG0g9697jU WJrNnNf1eWVER4mPVdIadJ5D8668ZqkkIGZ/I3jN796Bls97k41EN62CcK3DTAcPSc10 XmTrxGR9U8CBrSrSNADUCqBCY5Thuu6NhZqyICGpXI8/V4oGi0Jx1JIm26uARiJZ5MVY FIFg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of qemu-arm-bounces+alex.bennee=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom="qemu-arm-bounces+alex.bennee=linaro.org@nongnu.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id v5-v6si2335476qkg.8.2018.07.09.16.27.59 for (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 09 Jul 2018 16:27:59 -0700 (PDT) Received-SPF: pass (google.com: domain of qemu-arm-bounces+alex.bennee=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; spf=pass (google.com: domain of qemu-arm-bounces+alex.bennee=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom="qemu-arm-bounces+alex.bennee=linaro.org@nongnu.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from localhost ([::1]:44865 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fcfZX-0002PJ-AB for alex.bennee@linaro.org; Mon, 09 Jul 2018 19:27:59 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57305) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fcfYG-0001du-UO for qemu-arm@nongnu.org; Mon, 09 Jul 2018 19:26:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fcfYD-0005gN-Rw for qemu-arm@nongnu.org; Mon, 09 Jul 2018 19:26:40 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:35702 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fcfYD-0005fu-L3; Mon, 09 Jul 2018 19:26:37 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3649779D36; Mon, 9 Jul 2018 23:26:37 +0000 (UTC) Received: from redhat.com (ovpn-120-239.rdu2.redhat.com [10.10.120.239]) by smtp.corp.redhat.com (Postfix) with SMTP id 1CE3F111D3CF; Mon, 9 Jul 2018 23:26:36 +0000 (UTC) Date: Tue, 10 Jul 2018 02:26:35 +0300 From: "Michael S. Tsirkin" To: Igor Mammedov Message-ID: <20180710022532-mutt-send-email-mst@kernel.org> References: <20180705235305.124423-1-mst@redhat.com> <20180709175232.5e768dfb@redhat.com> <20180709200456-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180709200456-mutt-send-email-mst@kernel.org> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 09 Jul 2018 23:26:37 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Mon, 09 Jul 2018 23:26:37 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'mst@redhat.com' RCPT:'' X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.187.233.73 Subject: Re: [Qemu-arm] [PATCH] acpi: generalize aml_package / aml_varpackage X-BeenThere: qemu-arm@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , qemu-arm@nongnu.org, qemu-devel@nongnu.org, Shannon Zhao Errors-To: qemu-arm-bounces+alex.bennee=linaro.org@nongnu.org Sender: "Qemu-arm" X-TUID: 8Kt9gPiliQdx 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" 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 > > > --- > > > 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; From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57328) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fcfYK-0001f9-Hk for qemu-devel@nongnu.org; Mon, 09 Jul 2018 19:26:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fcfYJ-0005hy-95 for qemu-devel@nongnu.org; Mon, 09 Jul 2018 19:26:44 -0400 Date: Tue, 10 Jul 2018 02:26:35 +0300 From: "Michael S. Tsirkin" Message-ID: <20180710022532-mutt-send-email-mst@kernel.org> References: <20180705235305.124423-1-mst@redhat.com> <20180709175232.5e768dfb@redhat.com> <20180709200456-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180709200456-mutt-send-email-mst@kernel.org> Subject: Re: [Qemu-devel] [PATCH] acpi: generalize aml_package / aml_varpackage List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, Shannon Zhao , Peter Maydell , qemu-arm@nongnu.org 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" 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 > > > --- > > > 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;