From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39112) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eZGiV-0001c0-NF for qemu-devel@nongnu.org; Wed, 10 Jan 2018 08:46:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eZGiR-0003WM-Et for qemu-devel@nongnu.org; Wed, 10 Jan 2018 08:46:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46696) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eZGiO-0003Rf-Gr for qemu-devel@nongnu.org; Wed, 10 Jan 2018 08:46:51 -0500 Date: Wed, 10 Jan 2018 15:46:45 +0200 From: "Michael S. Tsirkin" Message-ID: <20180110154507-mutt-send-email-mst@kernel.org> References: <1515506288-32291-1-git-send-email-mst@redhat.com> <20180110122535.3cc0d223@igors-macbook-pro.local> <20180110132751-mutt-send-email-mst@kernel.org> <20180110124152.10896011@igors-macbook-pro.local> <20180110135246-mutt-send-email-mst@kernel.org> <20180110133109.3f8d9965@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180110133109.3f8d9965@redhat.com> Subject: Re: [Qemu-devel] [PATCH] acpi: switch to a dummy SSDT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, Paolo Bonzini , Richard Henderson , Eduardo Habkost , stefanb@linux.vnet.ibm.com On Wed, Jan 10, 2018 at 01:31:09PM +0100, Igor Mammedov wrote: > On Wed, 10 Jan 2018 13:55:37 +0200 > "Michael S. Tsirkin" wrote: > > > On Wed, Jan 10, 2018 at 12:41:52PM +0100, Igor Mammedov wrote: > > > On Wed, 10 Jan 2018 13:29:09 +0200 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Wed, Jan 10, 2018 at 12:25:34PM +0100, Igor Mammedov wrote: > > > > > On Tue, 9 Jan 2018 15:58:11 +0200 > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > We prefer not changing table sizes depending on parameters, > > > > > > that's why we create a dummy table rather than just drop > > > > > > the MCFG table. > > > > > > > > > > > > However, a table named "QEMU" could be put to a better > > > > > > use than just a stub, e.g. we could use it to pass > > > > > > some QEMU specific info to guests. > > > > > > > > > > > > Replace with an SSDT and pad with the Nop opcode > > > > > > to preserve the length. > > > > > Not sure it's useful, do you have patches on top of that, which will use this? > > > > > > > > Yes but not me - Stefan (Cc'd) wants to add a custom table. > > > Re-purposing dummy table that might become not dummy doesn't look like good approach. > > > What if later the guest enables MCFG and reboots, DUMMY SSDT would be replaced by MCFG? > > > > So what's the issue? Why is this worse than a dummy QEMU table? > > There's actually no re-purposing here at all. Either we create > > an MCFG or an SSDT. We never re-purpose. > I think, I've misread commit message, so it's table name 'QEMU' > that's going to be reused. > > Having several tables with name QEMU shouldn't be an issue, > adding oem/oem_id should be sufficient to distinguish them > for parsing side (if there is any). It seems more work for bios to scan all such tables and not just the first one. I might be wrong here. > As for change from ignored table to always parsed SSDT table, > having ignored one seems better. > So I'd drop SSDT+memset and keep oem[id] parts in this patch. I'll wait until Stefan posts his patches using this, then we can see what does look better. > > > It'd be cleaner if Stefan would just use his own dedicated SSDT. > > > > SSDT doesn't always work. A data table is needed so e.g. seabios > > can process it easily. > > > > > > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > > > --- > > > > > > hw/i386/acpi-build.c | 17 ++++++++++++----- > > > > > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > > > index 73519ab..d0afb31e 100644 > > > > > > --- a/hw/i386/acpi-build.c > > > > > > +++ b/hw/i386/acpi-build.c > > > > > > @@ -2419,6 +2419,8 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > > > > > { > > > > > > AcpiTableMcfg *mcfg; > > > > > > const char *sig; > > > > > > + const char *oem; > > > > > > + const char *oem_id; > > > > > > int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]); > > > > > > > > > > > > mcfg = acpi_data_push(table_data, len); > > > > > > @@ -2431,16 +2433,21 @@ build_mcfg_q35(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info) > > > > > > /* MCFG is used for ECAM which can be enabled or disabled by guest. > > > > > > * To avoid table size changes (which create migration issues), > > > > > > * always create the table even if there are no allocations, > > > > > > - * but set the signature to a reserved value in this case. > > > > > > - * ACPI spec requires OSPMs to ignore such tables. > > > > > > + * but fill it with Noop values. > > > > > > + * OSPMs ignore such tables. > > > > > > */ > > > > > > if (info->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) { > > > > > > - /* Reserved signature: ignored by OSPM */ > > > > > > - sig = "QEMU"; > > > > > > + sig = "SSDT"; > > > > > > + oem = "QEMU "; > > > > > > + oem_id = "NOOP"; > > > > > > + /* 0xa3 - NoopOp */ > > > > > > + memset(&mcfg->reserved, 0xa3, len - offsetof(AcpiTableMcfg, reserved)); > > > > > > } else { > > > > > > sig = "MCFG"; > > > > > > + oem = NULL; > > > > > > + oem_id = NULL; > > > > > > } > > > > > > - build_header(linker, table_data, (void *)mcfg, sig, len, 1, NULL, NULL); > > > > > > + build_header(linker, table_data, (void *)mcfg, sig, len, 1, oem, oem_id); > > > > > > } > > > > > > > > > > > > /*