From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39746) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ciSMC-00077X-DK for qemu-devel@nongnu.org; Mon, 27 Feb 2017 15:57:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ciSMB-0008BY-8t for qemu-devel@nongnu.org; Mon, 27 Feb 2017 15:57:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46704) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ciSMB-0008BL-0Y for qemu-devel@nongnu.org; Mon, 27 Feb 2017 15:57:19 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED384C0096D9 for ; Mon, 27 Feb 2017 20:57:18 +0000 (UTC) Date: Mon, 27 Feb 2017 22:57:17 +0200 From: "Michael S. Tsirkin" Message-ID: <20170227223512-mutt-send-email-mst@kernel.org> References: <1488201146-19580-1-git-send-email-marcel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1488201146-19580-1-git-send-email-marcel@redhat.com> Subject: Re: [Qemu-devel] [PATCH V3] hw/pxb-pcie: fix PCI Express hotplug support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: qemu-devel@nongnu.org, imammedo@redhat.com On Mon, Feb 27, 2017 at 03:12:26PM +0200, Marcel Apfelbaum wrote: > Add the missing osc method for pxb-pcie devices as APCI spec recommends, > see 6.2.10.3 OSC Implementation Example for PCI Host Bridge Devices, ACPI 5.0: > > It is recommended that a machine with multiple host bridge devices > should report the same capabilities for all host bridges, and also > negotiate control of the features described in the Control Field in > the same way for all host bridges. > > Reviewed-by: Igor Mammedov > Signed-off-by: Marcel Apfelbaum > --- > > Note to maintainer: > Please update ACPI test files. > > V2 -> V3: > - Modified comment (Igor) > > V1 -> V2: > Addressed Michael S. Tsirkin's comments: > - Added documentation to q35 osc function > - Made _osc serialized. I did not add compat property > since it seems guest OSs do not care anyway about the OSC > being serialized. > - Kept the SUPP field even if is not used and also > left both SUPP and CTRL out of the _osc because all > systems I checked and the ACPI spec keep them that way, > maybe is some kind of documentation contract. > > Thanks, > Marcel > > > hw/i386/acpi-build.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 1c928ab..ad3f233 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -1796,7 +1796,7 @@ static void build_piix4_pci_hotplug(Aml *table) > aml_append(table, scope); > } > > -static Aml *build_q35_osc_method(void) > +static void build_q35_osc_method(Aml *dev) This does more than just build _OSC so please give it a better name. > { > Aml *if_ctx; > Aml *if_ctx2; > @@ -1805,7 +1805,35 @@ static Aml *build_q35_osc_method(void) > Aml *a_cwd1 = aml_name("CDW1"); > Aml *a_ctrl = aml_name("CTRL"); > > - method = aml_method("_OSC", 4, AML_NOTSERIALIZED); > + /* > + * Bits defined in the Support Field provide information > + * regarding OS supported features. > + * > + * This field is not actually used and can be removed, > + * however it appears even if unused on most DSDTs. What does this mean? Our _OSC method uses these, that's why we have them. How can you just remove them? I think you mean we could remove the code writing to it. I don't think keeping it around is justified. Same applies to CTRL btw. We could use a local variable instead. > + * > + * See: > + * Table 6-148 Interpretation of _OSC Support Field, > + * Passed in via the 2nd dword in Arg3, APCI 5.0 Why 5.0? This is not how we do it. Pls find the earliest spec version that has this. 3.0? > + */ > + aml_append(dev, aml_name_decl("SUPP", aml_int(0))); > + > + /* > + * Bits defined in the Control Field are used to submit > + * request by the OS for control/handling of the associated feature > + * > + * See: Table 6-149 Interpretation of _OSC Control Field, > + * Passed in via Arg3, ACPI 5.0 > + * Table 6-150 Interpretation of _OSC Control Field, > + * Returned Value, APCI 5.0 > + */ > + aml_append(dev, aml_name_decl("CTRL", aml_int(0))); > + > + > + /* > + * 6.2.10 _OSC (Operating System Capabilities), APCI 5.0 > + */ Same here. > + method = aml_method("_OSC", 4, AML_SERIALIZED); > aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1")); > > if_ctx = aml_if(aml_equal( > @@ -1842,7 +1870,7 @@ static Aml *build_q35_osc_method(void) > aml_append(method, else_ctx); > > aml_append(method, aml_return(aml_arg(3))); > - return method; > + aml_append(dev, method); If you drop SUPP and CTRL then you can keep it simple, right? > } > > static void > @@ -1898,9 +1926,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); > aml_append(dev, aml_name_decl("_ADR", aml_int(0))); > aml_append(dev, aml_name_decl("_UID", aml_int(1))); > - aml_append(dev, aml_name_decl("SUPP", aml_int(0))); > - aml_append(dev, aml_name_decl("CTRL", aml_int(0))); > - aml_append(dev, build_q35_osc_method()); > + build_q35_osc_method(dev); > aml_append(sb_scope, dev); > aml_append(dsdt, sb_scope); > > @@ -1964,6 +1990,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > aml_append(dev, aml_name_decl("_UID", aml_int(bus_num))); > aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03"))); > aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num))); > + if (pci_bus_is_express(bus)) { > + build_q35_osc_method(dev); > + } > > if (numa_node != NUMA_NODE_UNASSIGNED) { > aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node))); > -- > 2.5.5 so how about ---> acpi: simplify _OSC Our _OSC method has a bunch of unused code loading data into external CTRL and SUPP fields which are then never used. Drop this in favor of a single local variable. Signed-off-by: Michael S. Tsirkin --- diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index db04cf5..efbbfcb 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -1804,7 +1804,7 @@ static Aml *build_q35_osc_method(void) Aml *else_ctx; Aml *method; Aml *a_cwd1 = aml_name("CDW1"); - Aml *a_ctrl = aml_name("CTRL"); + Aml *a_ctrl = aml_local(0); method = aml_method("_OSC", 4, AML_NOTSERIALIZED); aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1")); @@ -1814,7 +1814,6 @@ static Aml *build_q35_osc_method(void) aml_append(if_ctx, aml_create_dword_field(aml_arg(3), aml_int(4), "CDW2")); aml_append(if_ctx, aml_create_dword_field(aml_arg(3), aml_int(8), "CDW3")); - aml_append(if_ctx, aml_store(aml_name("CDW2"), aml_name("SUPP"))); aml_append(if_ctx, aml_store(aml_name("CDW3"), a_ctrl)); /* @@ -1899,8 +1898,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03"))); aml_append(dev, aml_name_decl("_ADR", aml_int(0))); aml_append(dev, aml_name_decl("_UID", aml_int(1))); - aml_append(dev, aml_name_decl("SUPP", aml_int(0))); - aml_append(dev, aml_name_decl("CTRL", aml_int(0))); aml_append(dev, build_q35_osc_method()); aml_append(sb_scope, dev); aml_append(dsdt, sb_scope);