Re: [PATCH v4 08/21] microvm/acpi: add minimal acpi support

From: Gerd Hoffmann <kraxel@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	Thomas Huth <thuth@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Sergio Lopez <slp@redhat.com>,
	qemu-devel@nongnu.org, Shannon Zhao <shannon.zhaosl@gmail.com>,
	qemu-arm@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v4 08/21] microvm/acpi: add minimal acpi support
Date: Mon, 6 Jul 2020 16:11:06 +0200	[thread overview]
Message-ID: <20200706141106.pxvzb6mnemwj43kl@sirius.home.kraxel.org> (raw)
In-Reply-To: <20200704053018-mutt-send-email-mst@kernel.org>

On Sun, Jul 05, 2020 at 07:48:14AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jul 03, 2020 at 09:25:10PM +0200, Gerd Hoffmann wrote:
> > On Fri, Jul 03, 2020 at 09:09:43AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jul 02, 2020 at 10:48:46PM +0200, Gerd Hoffmann wrote:
> > > > +    /* copy AML table into ACPI tables blob and patch header there */
> > > > +    g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
> > > > +    build_header(linker, table_data,
> > > > +        (void *)(table_data->data + table_data->len - dsdt->buf->len),
> > > > +        "DSDT", dsdt->buf->len, 5, NULL, NULL);
> > > 
> > > Why 5? Just curious ...
> > 
> > IIRC because the hw reduced hardware profile needs acpi 5+ ...
> > 
> > take care,
> >   Gerd
> 
> Well ACPI spec 5 says revision value is 2.

Yep.  Confused that with FACS.rev

> Let's use standard practice in ACPI code, and add comments near each
> value documenting earliest spec revision where this appeared, chapter
> where they came from and some verbatim text that both explains and can
> be searched for in later spec revisions.

Incremental patch with the changes I have so far.
Anything important missing?

Is there a formal spec (other than the code for arm virt)
for the virtio-mmio acpi entries btw?

take care,
  Gerd

From bc82d52a013169b3738a06f33ac7d2289922a683 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Mon, 6 Jul 2020 16:04:36 +0200
Subject: [PATCH] [fixup] microvm acpi spec refs

---
 hw/i386/acpi-microvm.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 2f7e50718f70..51916cab42f7 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -131,7 +131,7 @@ build_dsdt_microvm(GArray *table_data, BIOSLinker *linker,
     g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
     build_header(linker, table_data,
         (void *)(table_data->data + table_data->len - dsdt->buf->len),
-        "DSDT", dsdt->buf->len, 5, NULL, NULL);
+        "DSDT", dsdt->buf->len, 2, NULL, NULL);
     free_aml_allocator();
 }
 
@@ -144,25 +144,38 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
     GArray *tables_blob = tables->table_data;
     unsigned dsdt, xsdt;
     AcpiFadtData pmfadt = {
+        /*
+         * minimum version for ACPI_FADT_F_HW_REDUCED_ACPI,
+         * see acpi spec "4.1 Hardware-Reduced ACPI"
+         */
         .rev = 5,
         .minor_ver = 1,
+
         .flags = ((1 << ACPI_FADT_F_HW_REDUCED_ACPI) |
                   (1 << ACPI_FADT_F_RESET_REG_SUP)),
+
+        /* Table 5-33 FADT Format -- SLEEP_CONTROL_REG */
         .sleep_ctl = {
             .space_id = AML_AS_SYSTEM_MEMORY,
             .bit_width = 8,
             .address = GED_MMIO_BASE_REGS + ACPI_GED_REG_SLEEP_CTL,
         },
+
+        /* Table 5-33 FADT Format -- SLEEP_STATUS_REG */
         .sleep_sts = {
             .space_id = AML_AS_SYSTEM_MEMORY,
             .bit_width = 8,
             .address = GED_MMIO_BASE_REGS + ACPI_GED_REG_SLEEP_STS,
         },
+
+        /* Table 5-33 FADT Format -- RESET_REG */
         .reset_reg = {
             .space_id = AML_AS_SYSTEM_MEMORY,
             .bit_width = 8,
             .address = GED_MMIO_BASE_REGS + ACPI_GED_REG_RESET,
         },
+
+        /* Table 5-33 FADT Format -- RESET_VALUE */
         .reset_val = ACPI_GED_RESET_VALUE,
     };
 
@@ -191,7 +204,8 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
     /* RSDP is in FSEG memory, so allocate it separately */
     {
         AcpiRsdpData rsdp_data = {
-            .revision = 2,
+            /* Table 5-27 RSDP Structure */
+            .revision = 2, /* rev2 needed for xsdt support */
             .oem_id = ACPI_BUILD_APPNAME6,
             .xsdt_tbl_offset = &xsdt,
             .rsdt_tbl_offset = NULL,
-- 
2.18.4


