From mboxrd@z Thu Jan 1 00:00:00 1970 Received: by 2002:a5d:6844:0:0:0:0:0 with SMTP id o4-v6csp745387wrw; Fri, 9 Nov 2018 06:24:30 -0800 (PST) X-Google-Smtp-Source: AJdET5fhx6QSrZrqeDoOzzGLu80Y71X1u9qALEbQvR6m2WETD1e4I4CCwHM0DdSF++gMv2kmv5/l X-Received: by 2002:ac8:2ccc:: with SMTP id 12mr1312793qtx.277.1541773470309; Fri, 09 Nov 2018 06:24:30 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1541773470; cv=none; d=google.com; s=arc-20160816; b=L3zw9cb2BVmQH67Ps2nEbM09pIP+exllTuA/Efz5RSTh92YpaDS+m0wk3c/uOU3NrA 2ANikibZwKtOK3aRJ1HAMiO/ZSJ4IEdX4uZ3GFPPLUOnvMdCJ+3/9W9NuBSId25arJ2f we3bid0GcbN9/cFC8uRDQ231xUFwsEsrBcNMIrh6A0tJ/eHD0tAQW2YOaE//b3Xt3U4B SjED0tbE7vXI4zMTqxZUkwujBpvlGlNgCGOs+rl6crTqCTwnp5/8oZn59R2pmqR07sQv S0O7+OtT50xvdBvQzlGQc48/vNTi/yLM1saRAbLW6GHtdE52NDMn/m+ihodwKfGLZv1l P/ew== 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 :content-transfer-encoding:mime-version:references:in-reply-to :message-id:to:from:date; bh=wNLmwPht/dpa5TOMIb/3Qzw3mW6TcjzpwHWgDwbwioY=; b=uoe4JSBKoUgEvcOM7DC6kNPtyNoFjnMkBmQIg/0OWuFkJihTevl3kknxdw8p+KrZZf htg4DJmWUK+FsuKoSdOJZtJxrD7XdMavZz5RqBVFJ0RF2V9LzzfZ8jxGWTx3uEsZf6+X XEi40hHDU0kR/w7uvsttmSAwMUTyBJ82Y/OhQx39iEUXMRksKpBX4VcWOmrWFdJeVs14 C+DAA/5t06E5FtsBO/t9CLacR+PirWDyxlHV0C/wKMXzMidpHQipmsso5vnLTZccQaxB LRrvUwx4Y8E8zz6nP6+bALblpE1vyOMLVzdco7VTY3z2G1jNeAF9nuHUEy7Q3ONCznTr j+bg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of qemu-devel-bounces+alex.bennee=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom="qemu-devel-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 o187si5911859qkb.117.2018.11.09.06.24.30 for (version=TLS1 cipher=AES128-SHA bits=128/128); Fri, 09 Nov 2018 06:24:30 -0800 (PST) Received-SPF: pass (google.com: domain of qemu-devel-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-devel-bounces+alex.bennee=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom="qemu-devel-bounces+alex.bennee=linaro.org@nongnu.org"; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from localhost ([::1]:34412 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gL7i1-0007o4-OH for alex.bennee@linaro.org; Fri, 09 Nov 2018 09:24:29 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49968) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gL7hN-0007mo-Op for qemu-devel@nongnu.org; Fri, 09 Nov 2018 09:23:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gL7hM-00081n-0E for qemu-devel@nongnu.org; Fri, 09 Nov 2018 09:23:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53198) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gL7h7-0007Z8-8q; Fri, 09 Nov 2018 09:23:33 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2211B3DE04; Fri, 9 Nov 2018 14:23:24 +0000 (UTC) Received: from localhost (unknown [10.43.2.182]) by smtp.corp.redhat.com (Postfix) with ESMTP id C50C95C220; Fri, 9 Nov 2018 14:23:17 +0000 (UTC) Date: Fri, 9 Nov 2018 15:23:16 +0100 From: Igor Mammedov To: Samuel Ortiz Message-ID: <20181109152316.43b79217@redhat.com> In-Reply-To: <20181105014047.26447-2-sameo@linux.intel.com> References: <20181105014047.26447-1-sameo@linux.intel.com> <20181105014047.26447-2-sameo@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 09 Nov 2018 14:23:24 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [PATCH v5 01/24] hw: i386: Decouple the ACPI build from the PC machine type X-BeenThere: qemu-devel@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 , Stefano Stabellini , Eduardo Habkost , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Shannon Zhao , qemu-arm@nongnu.org, Paolo Bonzini , Anthony Perard , xen-devel@lists.xenproject.org, Richard Henderson Errors-To: qemu-devel-bounces+alex.bennee=linaro.org@nongnu.org Sender: "Qemu-devel" X-TUID: tWx9jpQZsyV4 On Mon, 5 Nov 2018 02:40:24 +0100 Samuel Ortiz wrote: > ACPI tables are platform and machine type and even architecture > agnostic, and as such we want to provide an internal ACPI API that > only depends on platform agnostic information. > > For the x86 architecture, in order to build ACPI tables independently > from the PC or Q35 machine types, we are moving a few MachineState > structure fields into a machine type agnostic structure called > AcpiConfiguration. The structure fields we move are: It's not obvious why new structure is needed, especially at the beginning of series. We probably should place this patch much later in the series (if we need it at all) and try generalize a much as possible without using it. And try to come up with an API that doesn't need centralized collection of data somehow related to ACPI (most of the fields here are not generic and applicable to a specific board/target). For generic API, I'd prefer a separate building blocks like build_fadt()/... that take as an input only parameters necessary to compose a table/aml part with occasional board interface hooks instead of all encompassing AcpiConfiguration and board specific 'acpi_build' that would use them when/if needed. We probably should split series into several smaller (if possible independent) ones, so people won't be scared of its sheer size and run away from reviewing it. That way it would be easier to review, amend certain parts and merge. acpi_setup() & co probably should be the last things that's are generalized as they are called by concrete boards and might collect board specific data and apply compat workarounds for building ACPI tables (assuming that we won't push non generic data into generic API). See more comments below > HotplugHandler *acpi_dev > AcpiNVDIMMState acpi_nvdimm_state; > FWCfgState *fw_cfg > ram_addr_t below_4g_mem_size, above_4g_mem_size > bool apic_xrupt_override > unsigned apic_id_limit > uint64_t numa_nodes > uint64_t numa_mem > > Signed-off-by: Samuel Ortiz > --- > hw/i386/acpi-build.h | 4 +- > include/hw/acpi/acpi.h | 44 ++++++++++ > include/hw/i386/pc.h | 19 ++--- > hw/acpi/cpu_hotplug.c | 9 +- > hw/arm/virt-acpi-build.c | 10 --- > hw/i386/acpi-build.c | 136 ++++++++++++++---------------- > hw/i386/pc.c | 176 ++++++++++++++++++++++++--------------- > hw/i386/pc_piix.c | 21 ++--- > hw/i386/pc_q35.c | 21 ++--- > hw/i386/xen/xen-hvm.c | 19 +++-- > 10 files changed, 257 insertions(+), 202 deletions(-) > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h > index 007332e51c..065a1d8250 100644 > --- a/hw/i386/acpi-build.h > +++ b/hw/i386/acpi-build.h > @@ -2,6 +2,8 @@ > #ifndef HW_I386_ACPI_BUILD_H > #define HW_I386_ACPI_BUILD_H > > -void acpi_setup(void); > +#include "hw/acpi/acpi.h" > + > +void acpi_setup(MachineState *machine, AcpiConfiguration *acpi_conf); > > #endif > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h > index c20ace0d0b..254c8d0cfc 100644 > --- a/include/hw/acpi/acpi.h > +++ b/include/hw/acpi/acpi.h > @@ -24,6 +24,8 @@ > #include "exec/memory.h" > #include "hw/irq.h" > #include "hw/acpi/acpi_dev_interface.h" > +#include "hw/hotplug.h" > +#include "hw/mem/nvdimm.h" > > /* > * current device naming scheme supports up to 256 memory devices > @@ -186,6 +188,48 @@ extern int acpi_enabled; > extern char unsigned *acpi_tables; > extern size_t acpi_tables_len; > > +typedef > +struct AcpiBuildState { > + /* Copy of table in RAM (for patching). */ > + MemoryRegion *table_mr; > + /* Is table patched? */ > + bool patched; > + void *rsdp; > + MemoryRegion *rsdp_mr; > + MemoryRegion *linker_mr; > +} AcpiBuildState; > + > +typedef > +struct AcpiConfiguration { We used to have a similar intermediate structure PcGuestInfo, but got rid of it in the end. Even with other questions aside I'm not quite convinced that it's good idea to reintroduce similar one again. > + /* Machine class ACPI settings */ > + int legacy_acpi_table_size; > + bool rsdp_in_ram; > + unsigned acpi_data_size; (*) well, all 2 are the legacy stuff, I'd rather not to push it into generic API and keep it in the caller as board specific/machine version code. > + > + /* Machine state ACPI settings */ > + HotplugHandler *acpi_dev; > + AcpiNVDIMMState acpi_nvdimm_state; > + > + /* > + * The fields below are machine settings that > + * are not ACPI specific. However they are needed > + * for building ACPI tables and as such should be > + * carried through the ACPI configuration structure. > + */ if they are not ACPI specific, then it shouldn't be in acpi configuration. Some of the fields are compat hacks, which doesn't belong to generic API so I'd leave them in board specific code and some are target specific which also doesn't belong in generic place. > + bool legacy_cpu_hotplug; > + bool linuxboot_dma_enabled; > + FWCfgState *fw_cfg; > + ram_addr_t below_4g_mem_size, above_4g_mem_size;; Just curious, how is this applicable to i386/virt machine? Does it also have memory split in 2 regions? Is it possible to have only one region? > + uint64_t numa_nodes; > + uint64_t *node_mem; that's kept in PCMachine for the sake of legacy SeaBIOS which builds ACPI tables on its own. I'd suggest to use existing globals instead (like ARM does) so that we wouldn't have to hunt down extra copies later when those globals are re-factored to properties. > + bool apic_xrupt_override; > + unsigned apic_id_limit; > + PCIHostState *pci_host; > + > + /* Build state */ > + AcpiBuildState *build_state; > +} AcpiConfiguration; > + [...] From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Mammedov Subject: Re: [PATCH v5 01/24] hw: i386: Decouple the ACPI build from the PC machine type Date: Fri, 9 Nov 2018 15:23:16 +0100 Message-ID: <20181109152316.43b79217@redhat.com> References: <20181105014047.26447-1-sameo@linux.intel.com> <20181105014047.26447-2-sameo@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1gL7gz-0003BN-W7 for xen-devel@lists.xenproject.org; Fri, 09 Nov 2018 14:23:26 +0000 In-Reply-To: <20181105014047.26447-2-sameo@linux.intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" To: Samuel Ortiz Cc: Peter Maydell , Stefano Stabellini , Eduardo Habkost , "Michael S. Tsirkin" , qemu-devel@nongnu.org, Shannon Zhao , qemu-arm@nongnu.org, Marcel Apfelbaum , Paolo Bonzini , Anthony Perard , xen-devel@lists.xenproject.org, Richard Henderson List-Id: xen-devel@lists.xenproject.org T24gTW9uLCAgNSBOb3YgMjAxOCAwMjo0MDoyNCArMDEwMApTYW11ZWwgT3J0aXogPHNhbWVvQGxp bnV4LmludGVsLmNvbT4gd3JvdGU6Cgo+IEFDUEkgdGFibGVzIGFyZSBwbGF0Zm9ybSBhbmQgbWFj aGluZSB0eXBlIGFuZCBldmVuIGFyY2hpdGVjdHVyZQo+IGFnbm9zdGljLCBhbmQgYXMgc3VjaCB3 ZSB3YW50IHRvIHByb3ZpZGUgYW4gaW50ZXJuYWwgQUNQSSBBUEkgdGhhdAo+IG9ubHkgZGVwZW5k cyBvbiBwbGF0Zm9ybSBhZ25vc3RpYyBpbmZvcm1hdGlvbi4KPiAKPiBGb3IgdGhlIHg4NiBhcmNo aXRlY3R1cmUsIGluIG9yZGVyIHRvIGJ1aWxkIEFDUEkgdGFibGVzIGluZGVwZW5kZW50bHkKPiBm cm9tIHRoZSBQQyBvciBRMzUgbWFjaGluZSB0eXBlcywgd2UgYXJlIG1vdmluZyBhIGZldyBNYWNo aW5lU3RhdGUKPiBzdHJ1Y3R1cmUgZmllbGRzIGludG8gYSBtYWNoaW5lIHR5cGUgYWdub3N0aWMg c3RydWN0dXJlIGNhbGxlZAo+IEFjcGlDb25maWd1cmF0aW9uLiBUaGUgc3RydWN0dXJlIGZpZWxk cyB3ZSBtb3ZlIGFyZToKCkl0J3Mgbm90IG9idmlvdXMgd2h5IG5ldyBzdHJ1Y3R1cmUgaXMgbmVl ZGVkLCBlc3BlY2lhbGx5IGF0CnRoZSBiZWdpbm5pbmcgb2Ygc2VyaWVzLiBXZSBwcm9iYWJseSBz aG91bGQgcGxhY2UgdGhpcyBwYXRjaAptdWNoIGxhdGVyIGluIHRoZSBzZXJpZXMgKGlmIHdlIG5l ZWQgaXQgYXQgYWxsKSBhbmQgdHJ5CmdlbmVyYWxpemUgYSBtdWNoIGFzIHBvc3NpYmxlIHdpdGhv dXQgdXNpbmcgaXQuCgpBbmQgdHJ5IHRvIGNvbWUgdXAgd2l0aCBhbiBBUEkgdGhhdCBkb2Vzbid0 IG5lZWQgY2VudHJhbGl6ZWQgY29sbGVjdGlvbgpvZiBkYXRhIHNvbWVob3cgcmVsYXRlZCB0byBB Q1BJIChtb3N0IG9mIHRoZSBmaWVsZHMgaGVyZSBhcmUgbm90IGdlbmVyaWMKYW5kIGFwcGxpY2Fi bGUgdG8gYSBzcGVjaWZpYyBib2FyZC90YXJnZXQpLgoKRm9yIGdlbmVyaWMgQVBJLCBJJ2QgcHJl ZmVyIGEgc2VwYXJhdGUgYnVpbGRpbmcgYmxvY2tzCmxpa2UgYnVpbGRfZmFkdCgpLy4uLiB0aGF0 IHRha2UgYXMgYW4gaW5wdXQgb25seSBwYXJhbWV0ZXJzCm5lY2Vzc2FyeSB0byBjb21wb3NlIGEg dGFibGUvYW1sIHBhcnQgd2l0aCBvY2Nhc2lvbmFsIGJvYXJkCmludGVyZmFjZSBob29rcyBpbnN0 ZWFkIG9mIGFsbCBlbmNvbXBhc3NpbmcgQWNwaUNvbmZpZ3VyYXRpb24KYW5kIGJvYXJkIHNwZWNp ZmljICdhY3BpX2J1aWxkJyB0aGF0IHdvdWxkIHVzZSB0aGVtIHdoZW4vaWYgbmVlZGVkLgoKV2Ug cHJvYmFibHkgc2hvdWxkIHNwbGl0IHNlcmllcyBpbnRvIHNldmVyYWwgc21hbGxlcgooaWYgcG9z c2libGUgaW5kZXBlbmRlbnQpIG9uZXMsIHNvIHBlb3BsZSB3b24ndCBiZSBzY2FyZWQgb2YKaXRz IHNoZWVyIHNpemUgYW5kIHJ1biBhd2F5IGZyb20gcmV2aWV3aW5nIGl0LgpUaGF0IHdheSBpdCB3 b3VsZCBiZSBlYXNpZXIgdG8gcmV2aWV3LCBhbWVuZCBjZXJ0YWluIHBhcnRzIGFuZCBtZXJnZS4K CmFjcGlfc2V0dXAoKSAmIGNvIHByb2JhYmx5IHNob3VsZCBiZSB0aGUgbGFzdCB0aGluZ3MgdGhh dCdzIGFyZQpnZW5lcmFsaXplZCBhcyB0aGV5IGFyZSBjYWxsZWQgYnkgY29uY3JldGUgYm9hcmRz IGFuZCBtaWdodCBjb2xsZWN0CmJvYXJkIHNwZWNpZmljIGRhdGEgYW5kIGFwcGx5IGNvbXBhdCB3 b3JrYXJvdW5kcyBmb3IgYnVpbGRpbmcgQUNQSSB0YWJsZXMKKGFzc3VtaW5nIHRoYXQgd2Ugd29u J3QgcHVzaCBub24gZ2VuZXJpYyBkYXRhIGludG8gZ2VuZXJpYyBBUEkpLgoKU2VlIG1vcmUgY29t bWVudHMgYmVsb3cKCj4gICAgSG90cGx1Z0hhbmRsZXIgKmFjcGlfZGV2Cj4gICAgQWNwaU5WRElN TVN0YXRlIGFjcGlfbnZkaW1tX3N0YXRlOwo+ICAgIEZXQ2ZnU3RhdGUgKmZ3X2NmZwo+ICAgIHJh bV9hZGRyX3QgYmVsb3dfNGdfbWVtX3NpemUsIGFib3ZlXzRnX21lbV9zaXplCj4gICAgYm9vbCBh cGljX3hydXB0X292ZXJyaWRlCj4gICAgdW5zaWduZWQgYXBpY19pZF9saW1pdAo+ICAgIHVpbnQ2 NF90IG51bWFfbm9kZXMKPiAgICB1aW50NjRfdCBudW1hX21lbQo+IAo+IFNpZ25lZC1vZmYtYnk6 IFNhbXVlbCBPcnRpeiA8c2FtZW9AbGludXguaW50ZWwuY29tPgo+IC0tLQo+ICBody9pMzg2L2Fj cGktYnVpbGQuaCAgICAgfCAgIDQgKy0KPiAgaW5jbHVkZS9ody9hY3BpL2FjcGkuaCAgIHwgIDQ0 ICsrKysrKysrKysKPiAgaW5jbHVkZS9ody9pMzg2L3BjLmggICAgIHwgIDE5ICsrLS0tCj4gIGh3 L2FjcGkvY3B1X2hvdHBsdWcuYyAgICB8ICAgOSArLQo+ICBody9hcm0vdmlydC1hY3BpLWJ1aWxk LmMgfCAgMTAgLS0tCj4gIGh3L2kzODYvYWNwaS1idWlsZC5jICAgICB8IDEzNiArKysrKysrKysr KysrKy0tLS0tLS0tLS0tLS0tLS0KPiAgaHcvaTM4Ni9wYy5jICAgICAgICAgICAgIHwgMTc2ICsr KysrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLQo+ICBody9pMzg2L3BjX3BpaXgu YyAgICAgICAgfCAgMjEgKystLS0KPiAgaHcvaTM4Ni9wY19xMzUuYyAgICAgICAgIHwgIDIxICsr LS0tCj4gIGh3L2kzODYveGVuL3hlbi1odm0uYyAgICB8ICAxOSArKystLQo+ICAxMCBmaWxlcyBj aGFuZ2VkLCAyNTcgaW5zZXJ0aW9ucygrKSwgMjAyIGRlbGV0aW9ucygtKQo+IAo+IGRpZmYgLS1n aXQgYS9ody9pMzg2L2FjcGktYnVpbGQuaCBiL2h3L2kzODYvYWNwaS1idWlsZC5oCj4gaW5kZXgg MDA3MzMyZTUxYy4uMDY1YTFkODI1MCAxMDA2NDQKPiAtLS0gYS9ody9pMzg2L2FjcGktYnVpbGQu aAo+ICsrKyBiL2h3L2kzODYvYWNwaS1idWlsZC5oCj4gQEAgLTIsNiArMiw4IEBACj4gICNpZm5k ZWYgSFdfSTM4Nl9BQ1BJX0JVSUxEX0gKPiAgI2RlZmluZSBIV19JMzg2X0FDUElfQlVJTERfSAo+ ICAKPiAtdm9pZCBhY3BpX3NldHVwKHZvaWQpOwo+ICsjaW5jbHVkZSAiaHcvYWNwaS9hY3BpLmgi Cj4gKwo+ICt2b2lkIGFjcGlfc2V0dXAoTWFjaGluZVN0YXRlICptYWNoaW5lLCBBY3BpQ29uZmln dXJhdGlvbiAqYWNwaV9jb25mKTsKPiAgCj4gICNlbmRpZgo+IGRpZmYgLS1naXQgYS9pbmNsdWRl L2h3L2FjcGkvYWNwaS5oIGIvaW5jbHVkZS9ody9hY3BpL2FjcGkuaAo+IGluZGV4IGMyMGFjZTBk MGIuLjI1NGM4ZDBjZmMgMTAwNjQ0Cj4gLS0tIGEvaW5jbHVkZS9ody9hY3BpL2FjcGkuaAo+ICsr KyBiL2luY2x1ZGUvaHcvYWNwaS9hY3BpLmgKPiBAQCAtMjQsNiArMjQsOCBAQAo+ICAjaW5jbHVk ZSAiZXhlYy9tZW1vcnkuaCIKPiAgI2luY2x1ZGUgImh3L2lycS5oIgo+ICAjaW5jbHVkZSAiaHcv YWNwaS9hY3BpX2Rldl9pbnRlcmZhY2UuaCIKPiArI2luY2x1ZGUgImh3L2hvdHBsdWcuaCIKPiAr I2luY2x1ZGUgImh3L21lbS9udmRpbW0uaCIKPiAgCj4gIC8qCj4gICAqIGN1cnJlbnQgZGV2aWNl IG5hbWluZyBzY2hlbWUgc3VwcG9ydHMgdXAgdG8gMjU2IG1lbW9yeSBkZXZpY2VzCj4gQEAgLTE4 Niw2ICsxODgsNDggQEAgZXh0ZXJuIGludCBhY3BpX2VuYWJsZWQ7Cj4gIGV4dGVybiBjaGFyIHVu c2lnbmVkICphY3BpX3RhYmxlczsKPiAgZXh0ZXJuIHNpemVfdCBhY3BpX3RhYmxlc19sZW47Cj4g IAo+ICt0eXBlZGVmCj4gK3N0cnVjdCBBY3BpQnVpbGRTdGF0ZSB7Cj4gKyAgICAvKiBDb3B5IG9m IHRhYmxlIGluIFJBTSAoZm9yIHBhdGNoaW5nKS4gKi8KPiArICAgIE1lbW9yeVJlZ2lvbiAqdGFi bGVfbXI7Cj4gKyAgICAvKiBJcyB0YWJsZSBwYXRjaGVkPyAqLwo+ICsgICAgYm9vbCBwYXRjaGVk Owo+ICsgICAgdm9pZCAqcnNkcDsKPiArICAgIE1lbW9yeVJlZ2lvbiAqcnNkcF9tcjsKPiArICAg IE1lbW9yeVJlZ2lvbiAqbGlua2VyX21yOwo+ICt9IEFjcGlCdWlsZFN0YXRlOwo+ICsKPiArdHlw ZWRlZgo+ICtzdHJ1Y3QgQWNwaUNvbmZpZ3VyYXRpb24gewpXZSB1c2VkIHRvIGhhdmUgYSBzaW1p bGFyIGludGVybWVkaWF0ZSBzdHJ1Y3R1cmUgUGNHdWVzdEluZm8sCmJ1dCBnb3QgcmlkIG9mIGl0 IGluIHRoZSBlbmQuIEV2ZW4gd2l0aCBvdGhlciBxdWVzdGlvbnMgYXNpZGUKSSdtIG5vdCBxdWl0 ZSBjb252aW5jZWQgdGhhdCBpdCdzIGdvb2QgaWRlYSB0byByZWludHJvZHVjZSBzaW1pbGFyCm9u ZSBhZ2Fpbi4KCgo+ICsgICAgLyogTWFjaGluZSBjbGFzcyBBQ1BJIHNldHRpbmdzICovCj4gKyAg ICBpbnQgbGVnYWN5X2FjcGlfdGFibGVfc2l6ZTsKPiArICAgIGJvb2wgcnNkcF9pbl9yYW07Cj4g KyAgICB1bnNpZ25lZCBhY3BpX2RhdGFfc2l6ZTsKICgqKSB3ZWxsLCBhbGwgMiBhcmUgdGhlIGxl Z2FjeSBzdHVmZiwgSSdkIHJhdGhlciBub3QgdG8gcHVzaCBpdCBpbnRvCmdlbmVyaWMgQVBJIGFu ZCBrZWVwIGl0IGluIHRoZSBjYWxsZXIgYXMgYm9hcmQgc3BlY2lmaWMvbWFjaGluZQp2ZXJzaW9u IGNvZGUuCgo+ICsKPiArICAgIC8qIE1hY2hpbmUgc3RhdGUgQUNQSSBzZXR0aW5ncyAqLwo+ICsg ICAgSG90cGx1Z0hhbmRsZXIgKmFjcGlfZGV2Owo+ICsgICAgQWNwaU5WRElNTVN0YXRlIGFjcGlf bnZkaW1tX3N0YXRlOwo+ICsKPiArICAgIC8qCj4gKyAgICAgKiBUaGUgZmllbGRzIGJlbG93IGFy ZSBtYWNoaW5lIHNldHRpbmdzIHRoYXQKPiArICAgICAqIGFyZSBub3QgQUNQSSBzcGVjaWZpYy4g SG93ZXZlciB0aGV5IGFyZSBuZWVkZWQKPiArICAgICAqIGZvciBidWlsZGluZyBBQ1BJIHRhYmxl cyBhbmQgYXMgc3VjaCBzaG91bGQgYmUKPiArICAgICAqIGNhcnJpZWQgdGhyb3VnaCB0aGUgQUNQ SSBjb25maWd1cmF0aW9uIHN0cnVjdHVyZS4KPiArICAgICAqLwppZiB0aGV5IGFyZSBub3QgQUNQ SSBzcGVjaWZpYywgdGhlbiBpdCBzaG91bGRuJ3QgYmUgaW4gYWNwaQpjb25maWd1cmF0aW9uLiBT b21lIG9mIHRoZSBmaWVsZHMgYXJlIGNvbXBhdCBoYWNrcywgd2hpY2ggZG9lc24ndApiZWxvbmcg dG8gZ2VuZXJpYyBBUEkgc28gSSdkIGxlYXZlIHRoZW0gaW4gYm9hcmQgc3BlY2lmaWMgY29kZQph bmQgc29tZSBhcmUgdGFyZ2V0IHNwZWNpZmljIHdoaWNoIGFsc28gZG9lc24ndCBiZWxvbmcgaW4g Z2VuZXJpYwpwbGFjZS4KCj4gKyAgICBib29sIGxlZ2FjeV9jcHVfaG90cGx1ZzsKPiArICAgIGJv b2wgbGludXhib290X2RtYV9lbmFibGVkOwo+ICsgICAgRldDZmdTdGF0ZSAqZndfY2ZnOwoKPiAr ICAgIHJhbV9hZGRyX3QgYmVsb3dfNGdfbWVtX3NpemUsIGFib3ZlXzRnX21lbV9zaXplOzsKSnVz dCBjdXJpb3VzLCBob3cgaXMgdGhpcyBhcHBsaWNhYmxlIHRvIGkzODYvdmlydCBtYWNoaW5lPwpE b2VzIGl0IGFsc28gaGF2ZSBtZW1vcnkgc3BsaXQgaW4gMiByZWdpb25zPwpJcyBpdCBwb3NzaWJs ZSB0byBoYXZlIG9ubHkgb25lIHJlZ2lvbj8KCj4gKyAgICB1aW50NjRfdCBudW1hX25vZGVzOwo+ ICsgICAgdWludDY0X3QgKm5vZGVfbWVtOwp0aGF0J3Mga2VwdCBpbiBQQ01hY2hpbmUgZm9yIHRo ZSBzYWtlIG9mIGxlZ2FjeSBTZWFCSU9TCndoaWNoIGJ1aWxkcyBBQ1BJIHRhYmxlcyBvbiBpdHMg b3duLgpJJ2Qgc3VnZ2VzdCB0byB1c2UgZXhpc3RpbmcgZ2xvYmFscyBpbnN0ZWFkIChsaWtlIEFS TSBkb2VzKQpzbyB0aGF0IHdlIHdvdWxkbid0IGhhdmUgdG8gaHVudCBkb3duIGV4dHJhIGNvcGll cyBsYXRlcgp3aGVuIHRob3NlIGdsb2JhbHMgYXJlIHJlLWZhY3RvcmVkIHRvIHByb3BlcnRpZXMu Cgo+ICsgICAgYm9vbCBhcGljX3hydXB0X292ZXJyaWRlOwo+ICsgICAgdW5zaWduZWQgYXBpY19p ZF9saW1pdDsKPiArICAgIFBDSUhvc3RTdGF0ZSAqcGNpX2hvc3Q7Cj4gKwo+ICsgICAgLyogQnVp bGQgc3RhdGUgKi8KPiArICAgIEFjcGlCdWlsZFN0YXRlICpidWlsZF9zdGF0ZTsKPiArfSBBY3Bp Q29uZmlndXJhdGlvbjsKPiArClsuLi5dCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpYZW4tZGV2ZWwgbWFpbGluZyBsaXN0Clhlbi1kZXZlbEBsaXN0cy54 ZW5wcm9qZWN0Lm9yZwpodHRwczovL2xpc3RzLnhlbnByb2plY3Qub3JnL21haWxtYW4vbGlzdGlu Zm8veGVuLWRldmVs From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49968) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gL7hN-0007mo-Op for qemu-devel@nongnu.org; Fri, 09 Nov 2018 09:23:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gL7hM-00081n-0E for qemu-devel@nongnu.org; Fri, 09 Nov 2018 09:23:49 -0500 Date: Fri, 9 Nov 2018 15:23:16 +0100 From: Igor Mammedov Message-ID: <20181109152316.43b79217@redhat.com> In-Reply-To: <20181105014047.26447-2-sameo@linux.intel.com> References: <20181105014047.26447-1-sameo@linux.intel.com> <20181105014047.26447-2-sameo@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 01/24] hw: i386: Decouple the ACPI build from the PC machine type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Ortiz Cc: qemu-devel@nongnu.org, Shannon Zhao , Stefano Stabellini , Anthony Perard , Richard Henderson , Marcel Apfelbaum , xen-devel@lists.xenproject.org, Paolo Bonzini , "Michael S. Tsirkin" , qemu-arm@nongnu.org, Peter Maydell , Eduardo Habkost On Mon, 5 Nov 2018 02:40:24 +0100 Samuel Ortiz wrote: > ACPI tables are platform and machine type and even architecture > agnostic, and as such we want to provide an internal ACPI API that > only depends on platform agnostic information. > > For the x86 architecture, in order to build ACPI tables independently > from the PC or Q35 machine types, we are moving a few MachineState > structure fields into a machine type agnostic structure called > AcpiConfiguration. The structure fields we move are: It's not obvious why new structure is needed, especially at the beginning of series. We probably should place this patch much later in the series (if we need it at all) and try generalize a much as possible without using it. And try to come up with an API that doesn't need centralized collection of data somehow related to ACPI (most of the fields here are not generic and applicable to a specific board/target). For generic API, I'd prefer a separate building blocks like build_fadt()/... that take as an input only parameters necessary to compose a table/aml part with occasional board interface hooks instead of all encompassing AcpiConfiguration and board specific 'acpi_build' that would use them when/if needed. We probably should split series into several smaller (if possible independent) ones, so people won't be scared of its sheer size and run away from reviewing it. That way it would be easier to review, amend certain parts and merge. acpi_setup() & co probably should be the last things that's are generalized as they are called by concrete boards and might collect board specific data and apply compat workarounds for building ACPI tables (assuming that we won't push non generic data into generic API). See more comments below > HotplugHandler *acpi_dev > AcpiNVDIMMState acpi_nvdimm_state; > FWCfgState *fw_cfg > ram_addr_t below_4g_mem_size, above_4g_mem_size > bool apic_xrupt_override > unsigned apic_id_limit > uint64_t numa_nodes > uint64_t numa_mem > > Signed-off-by: Samuel Ortiz > --- > hw/i386/acpi-build.h | 4 +- > include/hw/acpi/acpi.h | 44 ++++++++++ > include/hw/i386/pc.h | 19 ++--- > hw/acpi/cpu_hotplug.c | 9 +- > hw/arm/virt-acpi-build.c | 10 --- > hw/i386/acpi-build.c | 136 ++++++++++++++---------------- > hw/i386/pc.c | 176 ++++++++++++++++++++++++--------------- > hw/i386/pc_piix.c | 21 ++--- > hw/i386/pc_q35.c | 21 ++--- > hw/i386/xen/xen-hvm.c | 19 +++-- > 10 files changed, 257 insertions(+), 202 deletions(-) > > diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h > index 007332e51c..065a1d8250 100644 > --- a/hw/i386/acpi-build.h > +++ b/hw/i386/acpi-build.h > @@ -2,6 +2,8 @@ > #ifndef HW_I386_ACPI_BUILD_H > #define HW_I386_ACPI_BUILD_H > > -void acpi_setup(void); > +#include "hw/acpi/acpi.h" > + > +void acpi_setup(MachineState *machine, AcpiConfiguration *acpi_conf); > > #endif > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h > index c20ace0d0b..254c8d0cfc 100644 > --- a/include/hw/acpi/acpi.h > +++ b/include/hw/acpi/acpi.h > @@ -24,6 +24,8 @@ > #include "exec/memory.h" > #include "hw/irq.h" > #include "hw/acpi/acpi_dev_interface.h" > +#include "hw/hotplug.h" > +#include "hw/mem/nvdimm.h" > > /* > * current device naming scheme supports up to 256 memory devices > @@ -186,6 +188,48 @@ extern int acpi_enabled; > extern char unsigned *acpi_tables; > extern size_t acpi_tables_len; > > +typedef > +struct AcpiBuildState { > + /* Copy of table in RAM (for patching). */ > + MemoryRegion *table_mr; > + /* Is table patched? */ > + bool patched; > + void *rsdp; > + MemoryRegion *rsdp_mr; > + MemoryRegion *linker_mr; > +} AcpiBuildState; > + > +typedef > +struct AcpiConfiguration { We used to have a similar intermediate structure PcGuestInfo, but got rid of it in the end. Even with other questions aside I'm not quite convinced that it's good idea to reintroduce similar one again. > + /* Machine class ACPI settings */ > + int legacy_acpi_table_size; > + bool rsdp_in_ram; > + unsigned acpi_data_size; (*) well, all 2 are the legacy stuff, I'd rather not to push it into generic API and keep it in the caller as board specific/machine version code. > + > + /* Machine state ACPI settings */ > + HotplugHandler *acpi_dev; > + AcpiNVDIMMState acpi_nvdimm_state; > + > + /* > + * The fields below are machine settings that > + * are not ACPI specific. However they are needed > + * for building ACPI tables and as such should be > + * carried through the ACPI configuration structure. > + */ if they are not ACPI specific, then it shouldn't be in acpi configuration. Some of the fields are compat hacks, which doesn't belong to generic API so I'd leave them in board specific code and some are target specific which also doesn't belong in generic place. > + bool legacy_cpu_hotplug; > + bool linuxboot_dma_enabled; > + FWCfgState *fw_cfg; > + ram_addr_t below_4g_mem_size, above_4g_mem_size;; Just curious, how is this applicable to i386/virt machine? Does it also have memory split in 2 regions? Is it possible to have only one region? > + uint64_t numa_nodes; > + uint64_t *node_mem; that's kept in PCMachine for the sake of legacy SeaBIOS which builds ACPI tables on its own. I'd suggest to use existing globals instead (like ARM does) so that we wouldn't have to hunt down extra copies later when those globals are re-factored to properties. > + bool apic_xrupt_override; > + unsigned apic_id_limit; > + PCIHostState *pci_host; > + > + /* Build state */ > + AcpiBuildState *build_state; > +} AcpiConfiguration; > + [...]