[Qemu-devel] [PATCH v2 0/5] add ACPI node for fw_cfg on pc and arm

[Qemu-devel] [PATCH v2 0/5] add ACPI node for fw_cfg on pc and arm

[Gabriel L. Somlo]

New since v1:

	- expose control register size (suggested by Marc Marí)

	- leaving out _UID and _STA fields (thanks Shannon & Igor)

	- using "QEMU0002" as the value of _HID (thanks Michael)

	- added documentation blurb to docs/specs/fw_cfg.txt
	  (mainly to record usage of the "QEMU0002" string with fw_cfg).

Thanks,
  --Gabriel

> This series adds a fw_cfg device node to the SSDT (on pc), or to the
> DSDT (on arm).
>
> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
> 	  define from pc.c to pc.h, so that it could be used from
> 	  acpi-build.c in patch 2/3.
> 
> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
> 
> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
>
> I made up some names - "FWCF" for the node name, and "FWCF0001"
> for _HID; no idea whether that's appropriate, or how else I should
> figure out what to use instead...
>
> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> output of "info qtree". Again, if that's wrong, please point me in
> the right direction.
>
> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
> I noticed none of the other DSDT entries contain a _STA field, wondering
> why it would (not) make sense to include that, same as on the PC.

Gabriel L. Somlo (5):
  fw_cfg: expose control register size in fw_cfg.h
  pc: fw_cfg: move ioport base constant to pc.h
  acpi: pc: add fw_cfg device node to ssdt
  acpi: arm: add fw_cfg device node to dsdt
  fw_cfg: document ACPI device node information

 docs/specs/fw_cfg.txt     |  9 +++++++++
 hw/arm/virt-acpi-build.c  | 13 +++++++++++++
 hw/i386/acpi-build.c      | 20 ++++++++++++++++++++
 hw/i386/pc.c              |  5 ++---
 hw/nvram/fw_cfg.c         |  8 +++++---
 include/hw/i386/pc.h      |  2 ++
 include/hw/nvram/fw_cfg.h |  3 +++
 7 files changed, 54 insertions(+), 6 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/5] fw_cfg: expose control register size in fw_cfg.h

[Gabriel L. Somlo]

Expose the size of the control register (FW_CFG_SIZE, renamed to
FW_CFG_CTL_SIZE) in fw_cfg.h.
Add comment to fw_cfg_io_realize() pointing out that since the
8-bit data register is always subsumed by the 16-bit control
register in the port I/O case, we use the control register width
as the *total* width of the port I/O region reserved for the device.

Suggested-by: Marc Marí <markmb@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/nvram/fw_cfg.c         | 8 +++++---
 include/hw/nvram/fw_cfg.h | 3 +++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 658f8c4..9dd95c7 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -30,7 +30,6 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
@@ -672,8 +671,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
     FWCfgIoState *s = FW_CFG_IO(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
+    /* when using port i/o, the 8-bit data register ALWAYS overlaps
+     * with half of the 16-bit control register. Hence, the total size
+     * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
-                          FW_CFG(s), "fwcfg", FW_CFG_SIZE);
+                          FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
 }
 
@@ -705,7 +707,7 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
     const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
 
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
-                          FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);
+                          FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);
 
     if (s->data_width > data_ops->valid.max_access_size) {
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index e60d3ca..5008270 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -46,6 +46,9 @@
 
 #define FW_CFG_INVALID          0xffff
 
+/* width in bytes of fw_cfg control port */
+#define FW_CFG_CTL_SIZE         0x02
+
 #define FW_CFG_MAX_FILE_PATH    56
 
 #ifndef NO_QEMU_PROTOS
-- 
2.4.3

[Qemu-devel] [PATCH v2 2/5] pc: fw_cfg: move ioport base constant to pc.h

[Gabriel L. Somlo]

Move BIOS_CFG_IOPORT define from pc.c to pc.h, and rename
it to FW_CFG_IO_BASE.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/pc.c         | 5 ++---
 include/hw/i386/pc.h | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b5107f7..1a92b4f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -86,7 +86,6 @@ void pc_set_legacy_acpi_data_size(void)
     acpi_data_size = 0x10000;
 }
 
-#define BIOS_CFG_IOPORT 0x510
 #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
 #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
 #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
@@ -760,7 +759,7 @@ static FWCfgState *bochs_bios_init(void)
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
      * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
@@ -1292,7 +1291,7 @@ FWCfgState *xen_load_linux(PCMachineState *pcms,
 
     assert(MACHINE(pcms)->kernel_filename != NULL);
 
-    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
+    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
     rom_set_fw(fw_cfg);
 
     load_linux(pcms, fw_cfg);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3e002c9..fadcaa4 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -206,6 +206,8 @@ typedef void (*cpu_set_smm_t)(int smm, void *arg);
 
 void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
+#define FW_CFG_IO_BASE 0x510
+
 /* acpi_piix.c */
 
 I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-- 
2.4.3

[Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Gabriel L. Somlo]

Add a fw_cfg device node to the ACPI SSDT. While the guest-side
BIOS can't utilize this information (since it has to access the
hard-coded fw_cfg device to extract ACPI tables to begin with),
having fw_cfg listed in ACPI will help the guest kernel keep a
more accurate inventory of in-use IO port regions.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/acpi-build.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..ecdb3a5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1071,6 +1071,26 @@ build_ssdt(GArray *table_data, GArray *linker,
     aml_append(scope, aml_name_decl("_S5", pkg));
     aml_append(ssdt, scope);
 
+    if (guest_info->fw_cfg) {
+        scope = aml_scope("\\_SB");
+        dev = aml_device("FWCF");
+
+        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
+
+        crs = aml_resource_template();
+        /* when using port i/o, the 8-bit data register *always* overlaps
+         * with half of the 16-bit control register. Hence, the total size
+         * of the i/o region used is FW_CFG_CTL_SIZE */
+        aml_append(crs,
+            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
+                   0x01, FW_CFG_CTL_SIZE)
+        );
+        aml_append(dev, aml_name_decl("_CRS", crs));
+
+        aml_append(scope, dev);
+        aml_append(ssdt, scope);
+    }
+
     if (misc->applesmc_io_base) {
         scope = aml_scope("\\_SB.PCI0.ISA");
         dev = aml_device("SMC");
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Eduardo Habkost]

On Mon, Sep 14, 2015 at 10:57:31AM -0400, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> BIOS can't utilize this information (since it has to access the
> hard-coded fw_cfg device to extract ACPI tables to begin with),
> having fw_cfg listed in ACPI will help the guest kernel keep a
> more accurate inventory of in-use IO port regions.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/i386/acpi-build.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ecdb3a5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1071,6 +1071,26 @@ build_ssdt(GArray *table_data, GArray *linker,
>      aml_append(scope, aml_name_decl("_S5", pkg));
>      aml_append(ssdt, scope);
>  
> +    if (guest_info->fw_cfg) {

I sent a reply to v1 a few minutes before seeing v2. Copying it here for
reference:

Is this function ever going to be called without fw_cfg set?
acpi_setup() returns immediately if fw_cfg isn't available.

Also, this changes guest ABI, so you will need to add some compat flag
to PCMachineClass indicating if the device node should be added.


> +        scope = aml_scope("\\_SB");
> +        dev = aml_device("FWCF");
> +
> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +
> +        crs = aml_resource_template();
> +        /* when using port i/o, the 8-bit data register *always* overlaps
> +         * with half of the 16-bit control register. Hence, the total size
> +         * of the i/o region used is FW_CFG_CTL_SIZE */
> +        aml_append(crs,
> +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> +                   0x01, FW_CFG_CTL_SIZE)
> +        );
> +        aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +        aml_append(scope, dev);
> +        aml_append(ssdt, scope);
> +    }
> +
>      if (misc->applesmc_io_base) {
>          scope = aml_scope("\\_SB.PCI0.ISA");
>          dev = aml_device("SMC");
> -- 
> 2.4.3
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Gabriel L. Somlo]

On Mon, Sep 14, 2015 at 01:09:41PM -0300, Eduardo Habkost wrote:
> On Mon, Sep 14, 2015 at 10:57:31AM -0400, Gabriel L. Somlo wrote:
> > Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> > BIOS can't utilize this information (since it has to access the
> > hard-coded fw_cfg device to extract ACPI tables to begin with),
> > having fw_cfg listed in ACPI will help the guest kernel keep a
> > more accurate inventory of in-use IO port regions.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  hw/i386/acpi-build.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 95e0c65..ecdb3a5 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1071,6 +1071,26 @@ build_ssdt(GArray *table_data, GArray *linker,
> >      aml_append(scope, aml_name_decl("_S5", pkg));
> >      aml_append(ssdt, scope);
> >  
> > +    if (guest_info->fw_cfg) {
> 
> I sent a reply to v1 a few minutes before seeing v2. Copying it here for
> reference:
> 
> Is this function ever going to be called without fw_cfg set?
> acpi_setup() returns immediately if fw_cfg isn't available.

You're right, build_ssdt() is called from acpi_build(), which is
called directly from acpi_setup(), and also indirectly via
acpi_build_update() via acpi_add_rom_blob(), but all that eventually
ties back to acpi_setup(), *after* it already established that
guest_info->fw_cfg is non-NULL.

> Also, this changes guest ABI, so you will need to add some compat flag
> to PCMachineClass indicating if the device node should be added.

So I'll replace the "if (guest_info->fw_cfg)" check with
"if machine-type >= (pc-q35-2.5 or pc-i440fx-2.5))", in v3,
as soon as the patches for the 2.5 machine type make it into
git master (I remember seeing a reviewed-by fly by for that
earlier today :)

Thanks much,
--Gabriel

> 
> 
> > +        scope = aml_scope("\\_SB");
> > +        dev = aml_device("FWCF");
> > +
> > +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> > +
> > +        crs = aml_resource_template();
> > +        /* when using port i/o, the 8-bit data register *always* overlaps
> > +         * with half of the 16-bit control register. Hence, the total size
> > +         * of the i/o region used is FW_CFG_CTL_SIZE */
> > +        aml_append(crs,
> > +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> > +                   0x01, FW_CFG_CTL_SIZE)
> > +        );
> > +        aml_append(dev, aml_name_decl("_CRS", crs));
> > +
> > +        aml_append(scope, dev);
> > +        aml_append(ssdt, scope);
> > +    }
> > +
> >      if (misc->applesmc_io_base) {
> >          scope = aml_scope("\\_SB.PCI0.ISA");
> >          dev = aml_device("SMC");
> > -- 
> > 2.4.3
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Eduardo Habkost]

On Mon, Sep 14, 2015 at 02:16:49PM -0400, Gabriel L. Somlo wrote:
> On Mon, Sep 14, 2015 at 01:09:41PM -0300, Eduardo Habkost wrote:
> > On Mon, Sep 14, 2015 at 10:57:31AM -0400, Gabriel L. Somlo wrote:
> > > Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> > > BIOS can't utilize this information (since it has to access the
> > > hard-coded fw_cfg device to extract ACPI tables to begin with),
> > > having fw_cfg listed in ACPI will help the guest kernel keep a
> > > more accurate inventory of in-use IO port regions.
> > > 
> > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > >  hw/i386/acpi-build.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 95e0c65..ecdb3a5 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1071,6 +1071,26 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >      aml_append(scope, aml_name_decl("_S5", pkg));
> > >      aml_append(ssdt, scope);
> > >  
> > > +    if (guest_info->fw_cfg) {
> > 
> > I sent a reply to v1 a few minutes before seeing v2. Copying it here for
> > reference:
> > 
> > Is this function ever going to be called without fw_cfg set?
> > acpi_setup() returns immediately if fw_cfg isn't available.
> 
> You're right, build_ssdt() is called from acpi_build(), which is
> called directly from acpi_setup(), and also indirectly via
> acpi_build_update() via acpi_add_rom_blob(), but all that eventually
> ties back to acpi_setup(), *after* it already established that
> guest_info->fw_cfg is non-NULL.
> 
> > Also, this changes guest ABI, so you will need to add some compat flag
> > to PCMachineClass indicating if the device node should be added.
> 
> So I'll replace the "if (guest_info->fw_cfg)" check with
> "if machine-type >= (pc-q35-2.5 or pc-i440fx-2.5))", in v3,
> as soon as the patches for the 2.5 machine type make it into
> git master (I remember seeing a reviewed-by fly by for that
> earlier today :)

Sounds good, assuming you are going to implement the "machine-type >= pc-2.5"
check with something like: PC_MACHINE_GET_CLASS(machine)->acpi_no_fw_cfg_node.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Gabriel L. Somlo]

On Mon, Sep 14, 2015 at 05:16:44PM -0300, Eduardo Habkost wrote:
> On Mon, Sep 14, 2015 at 02:16:49PM -0400, Gabriel L. Somlo wrote:
> > On Mon, Sep 14, 2015 at 01:09:41PM -0300, Eduardo Habkost wrote:
> > > On Mon, Sep 14, 2015 at 10:57:31AM -0400, Gabriel L. Somlo wrote:
> > > > Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> > > > BIOS can't utilize this information (since it has to access the
> > > > hard-coded fw_cfg device to extract ACPI tables to begin with),
> > > > having fw_cfg listed in ACPI will help the guest kernel keep a
> > > > more accurate inventory of in-use IO port regions.
> > > > 
> > > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > > > ---
> > > >  hw/i386/acpi-build.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index 95e0c65..ecdb3a5 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -1071,6 +1071,26 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >      aml_append(scope, aml_name_decl("_S5", pkg));
> > > >      aml_append(ssdt, scope);
> > > >  
> > > > +    if (guest_info->fw_cfg) {
> > > 
> > > I sent a reply to v1 a few minutes before seeing v2. Copying it here for
> > > reference:
> > > 
> > > Is this function ever going to be called without fw_cfg set?
> > > acpi_setup() returns immediately if fw_cfg isn't available.
> > 
> > You're right, build_ssdt() is called from acpi_build(), which is
> > called directly from acpi_setup(), and also indirectly via
> > acpi_build_update() via acpi_add_rom_blob(), but all that eventually
> > ties back to acpi_setup(), *after* it already established that
> > guest_info->fw_cfg is non-NULL.
> > 
> > > Also, this changes guest ABI, so you will need to add some compat flag
> > > to PCMachineClass indicating if the device node should be added.
> > 
> > So I'll replace the "if (guest_info->fw_cfg)" check with
> > "if machine-type >= (pc-q35-2.5 or pc-i440fx-2.5))", in v3,
> > as soon as the patches for the 2.5 machine type make it into
> > git master (I remember seeing a reviewed-by fly by for that
> > earlier today :)
> 
> Sounds good, assuming you are going to implement the "machine-type >= pc-2.5"
> check with something like: PC_MACHINE_GET_CLASS(machine)->acpi_no_fw_cfg_node.

Thanks, that gives me something to grep for ;)

I was going to mimic how other acpi related decisions are made on pc
(piix or q35), something like below. Might even be the same thing,
once I learn about PC_MACHINE_GET_CLASS :)

Thanks,
--Gabriel


diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3f925b2..8219ed9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -64,6 +64,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 static bool pci_enabled = true;
 static bool has_acpi_build = true;
 static bool rsdp_in_ram = true;
+static bool has_acpi_fw_cfg = true;
 static int legacy_acpi_table_size;
 static bool smbios_defaults = true;
 static bool smbios_legacy_mode;
@@ -168,6 +169,7 @@ static void pc_init1(MachineState *machine,
     guest_info->isapc_ram_fw = !pci_enabled;
     guest_info->has_reserved_memory = has_reserved_memory;
     guest_info->rsdp_in_ram = rsdp_in_ram;
+    guest_info->has_acpi_fw_cfg = has_acpi_fw_cfg;
 
     if (smbios_defaults) {
         MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -316,6 +318,7 @@ static void pc_compat_2_2(MachineState *machine)
 {
     pc_compat_2_3(machine);
     rsdp_in_ram = false;
+    has_acpi_fw_cfg = false;
     machine->suppress_vmdesc = true;
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 11601ab..29daf08 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -52,6 +52,7 @@
 
 static bool has_acpi_build = true;
 static bool rsdp_in_ram = true;
+static bool has_acpi_fw_cfg = true;
 static bool smbios_defaults = true;
 static bool smbios_legacy_mode;
 static bool smbios_uuid_encoded = true;
@@ -154,6 +155,7 @@ static void pc_q35_init(MachineState *machine)
     guest_info->has_acpi_build = has_acpi_build;
     guest_info->has_reserved_memory = has_reserved_memory;
     guest_info->rsdp_in_ram = rsdp_in_ram;
+    guest_info->has_acpi_fw_cfg = has_acpi_fw_cfg;
 
     /* Migration was not supported in 2.0 for Q35, so do not bother
      * with this hack (see hw/i386/acpi-build.c).
@@ -299,6 +301,7 @@ static void pc_compat_2_2(MachineState *machine)
 {
     pc_compat_2_3(machine);
     rsdp_in_ram = false;
+    has_acpi_fw_cfg = false;
     machine->suppress_vmdesc = true;
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fadcaa4..4896475 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -103,6 +103,7 @@ struct PcGuestInfo {
     bool has_acpi_build;
     bool has_reserved_memory;
     bool rsdp_in_ram;
+    bool has_acpi_fw_cfg;
 };
 
 /* parallel.c */

Re: [Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Gabriel L. Somlo]

On Mon, Sep 14, 2015 at 04:34:02PM -0400, Gabriel L. Somlo wrote:
> > > So I'll replace the "if (guest_info->fw_cfg)" check with
> > > "if machine-type >= (pc-q35-2.5 or pc-i440fx-2.5))", in v3,
> > > as soon as the patches for the 2.5 machine type make it into
> > > git master (I remember seeing a reviewed-by fly by for that
> > > earlier today :)
> > 
> > Sounds good, assuming you are going to implement the "machine-type >= pc-2.5"
> > check with something like: PC_MACHINE_GET_CLASS(machine)->acpi_no_fw_cfg_node.
> 
> Thanks, that gives me something to grep for ;)
> 
> I was going to mimic how other acpi related decisions are made on pc
> (piix or q35), something like below. Might even be the same thing,
> once I learn about PC_MACHINE_GET_CLASS :)

OK, so not exactly the same thing. What's the trade-off between
adding a boolean field to PCMachineClass vs. PcGuestInfo? Either
would work technically, and PcGuestInfo already has a bunch of
acpi related booleans. But if the new canonical place for this
kind of thing is PCMachineClass rather than PcGuestInfo, it's OK
with me...

Please advise.

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 3/5] acpi: pc: add fw_cfg device node to ssdt

[Eduardo Habkost]

On Mon, Sep 14, 2015 at 04:54:25PM -0400, Gabriel L. Somlo wrote:
> On Mon, Sep 14, 2015 at 04:34:02PM -0400, Gabriel L. Somlo wrote:
> > > > So I'll replace the "if (guest_info->fw_cfg)" check with
> > > > "if machine-type >= (pc-q35-2.5 or pc-i440fx-2.5))", in v3,
> > > > as soon as the patches for the 2.5 machine type make it into
> > > > git master (I remember seeing a reviewed-by fly by for that
> > > > earlier today :)
> > > 
> > > Sounds good, assuming you are going to implement the "machine-type >= pc-2.5"
> > > check with something like: PC_MACHINE_GET_CLASS(machine)->acpi_no_fw_cfg_node.
> > 
> > Thanks, that gives me something to grep for ;)
> > 
> > I was going to mimic how other acpi related decisions are made on pc
> > (piix or q35), something like below. Might even be the same thing,
> > once I learn about PC_MACHINE_GET_CLASS :)
> 
> OK, so not exactly the same thing. What's the trade-off between
> adding a boolean field to PCMachineClass vs. PcGuestInfo? Either
> would work technically, and PcGuestInfo already has a bunch of
> acpi related booleans. But if the new canonical place for this
> kind of thing is PCMachineClass rather than PcGuestInfo, it's OK
> with me...

PCGuestInfo is initialized only when pc_init1() is called and we are
already initializing a machine. PCMachineClass is initialized when the
corresponding machine_options() is called at class_init time. Keeping
machine-specific compat info at PCMachineClass will allow us to move
that information somewhere else more easily in the future (and allow us
to avoid needing new global variables and new pc_compat_*() functions).

See how PCMachineClass::broken_reserved_end gets used, for reference.

-- 
Eduardo

[Qemu-devel] [PATCH v2 4/5] acpi: arm: add fw_cfg device node to dsdt

[Gabriel L. Somlo]

Add a fw_cfg device node to the ACPI DSDT. This is mostly
informational, as the authoritative fw_cfg MMIO region(s)
are listed in the Device Tree. However, since we are building
ACPI tables, we might as well be thorough while at it...

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/arm/virt-acpi-build.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9088248..dcf9752 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -110,6 +110,18 @@ static void acpi_dsdt_add_rtc(Aml *scope, const MemMapEntry *rtc_memmap,
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
+{
+    Aml *dev = aml_device("FWCF");
+    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
+
+    Aml *crs = aml_resource_template();
+    aml_append(crs, aml_memory32_fixed(fw_cfg_memmap->base,
+                                       fw_cfg_memmap->size, AML_READ_WRITE));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+    aml_append(scope, dev);
+}
+
 static void acpi_dsdt_add_flash(Aml *scope, const MemMapEntry *flash_memmap)
 {
     Aml *dev, *crs;
@@ -519,6 +531,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
                       (irqmap[VIRT_RTC] + ARM_SPI_BASE));
+    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 4/5] acpi: arm: add fw_cfg device node to dsdt

[Shannon Zhao]

On 2015/9/14 22:57, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI DSDT. This is mostly
> informational, as the authoritative fw_cfg MMIO region(s)
> are listed in the Device Tree. However, since we are building
> ACPI tables, we might as well be thorough while at it...
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/arm/virt-acpi-build.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9088248..dcf9752 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -110,6 +110,18 @@ static void acpi_dsdt_add_rtc(Aml *scope, const MemMapEntry *rtc_memmap,
>      aml_append(scope, dev);
>  }
>  
> +static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
> +{
> +    Aml *dev = aml_device("FWCF");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +
> +    Aml *crs = aml_resource_template();
> +    aml_append(crs, aml_memory32_fixed(fw_cfg_memmap->base,
> +                                       fw_cfg_memmap->size, AML_READ_WRITE));
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +    aml_append(scope, dev);
> +}
> +
>  static void acpi_dsdt_add_flash(Aml *scope, const MemMapEntry *flash_memmap)
>  {
>      Aml *dev, *crs;
> @@ -519,6 +531,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>      acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
>                        (irqmap[VIRT_RTC] + ARM_SPI_BASE));
> +    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> 

This looks fine to me. But from what you said, the kernel driver is not
in upstream kernel yet, so this would be applied after the kernel patch
applied in case of unexpected change.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH v2 4/5] acpi: arm: add fw_cfg device node to dsdt

[Gabriel L. Somlo]

On Tue, Sep 15, 2015 at 10:06:45AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/9/14 22:57, Gabriel L. Somlo wrote:
> > Add a fw_cfg device node to the ACPI DSDT. This is mostly
> > informational, as the authoritative fw_cfg MMIO region(s)
> > are listed in the Device Tree. However, since we are building
> > ACPI tables, we might as well be thorough while at it...
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  hw/arm/virt-acpi-build.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 9088248..dcf9752 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -110,6 +110,18 @@ static void acpi_dsdt_add_rtc(Aml *scope, const MemMapEntry *rtc_memmap,
> >      aml_append(scope, dev);
> >  }
> >  
> > +static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
> > +{
> > +    Aml *dev = aml_device("FWCF");
> > +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> > +
> > +    Aml *crs = aml_resource_template();
> > +    aml_append(crs, aml_memory32_fixed(fw_cfg_memmap->base,
> > +                                       fw_cfg_memmap->size, AML_READ_WRITE));
> > +    aml_append(dev, aml_name_decl("_CRS", crs));
> > +    aml_append(scope, dev);
> > +}
> > +
> >  static void acpi_dsdt_add_flash(Aml *scope, const MemMapEntry *flash_memmap)
> >  {
> >      Aml *dev, *crs;
> > @@ -519,6 +531,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> >      acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
> >                        (irqmap[VIRT_RTC] + ARM_SPI_BASE));
> > +    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> >      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> > 
> 
> This looks fine to me. But from what you said, the kernel driver is not
> in upstream kernel yet, so this would be applied after the kernel patch
> applied in case of unexpected change.

I guess that's ok if you're only talking about the arm side of things
-- although I can't imagine how any of the above would need to change
depending on what happens with the guest-side kernel sysfs driver...
We're talking a node name ("FWCF"), a _HID ("QEMU0002"), and a mmio
region (given to us by memmap[VIRT_FW_CFG]), and that's all there is
to it...

However, on the i386 side, this needs to go in *before* I can continue
working on the guest-side kernel sysfs drivers. One of the
requirements I got was not to probe IOports or MMIO registers blindly,
but rather to use DT on arm and ACPI on i386 to first make sure fw_cfg
is expected to be there, before tickling its registers :)

So I need (at least the i386 part of) this series in qemu before I can
move ahead with the (guest) kernel driver...

Thanks,
--Gabriel

Re: [Qemu-devel] [PATCH v2 4/5] acpi: arm: add fw_cfg device node to dsdt

[Peter Maydell]

On 15 September 2015 at 14:42, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> On Tue, Sep 15, 2015 at 10:06:45AM +0800, Shannon Zhao wrote:
>> This looks fine to me. But from what you said, the kernel driver is not
>> in upstream kernel yet, so this would be applied after the kernel patch
>> applied in case of unexpected change.
>
> I guess that's ok if you're only talking about the arm side of things
> -- although I can't imagine how any of the above would need to change
> depending on what happens with the guest-side kernel sysfs driver...
> We're talking a node name ("FWCF"), a _HID ("QEMU0002"), and a mmio
> region (given to us by memmap[VIRT_FW_CFG]), and that's all there is
> to it...

The underlying requirement here is "we don't want patches until
the ABI is fixed". For ACPI and DTB on ARM the final arbiter of
the ABI seems to be the kernel (partly because the reviewers there
are better informed with the overall ACPI/DTB requirements).
I don't know who the final arbiter of the ACPI ABI for x86 is.

We've already had a couple of issues with problems with the ACPI
ABI for what appear to be very simple "just some registers and
an interrupt" type devices, including "is this the correct HID?".

> However, on the i386 side, this needs to go in *before* I can continue
> working on the guest-side kernel sysfs drivers. One of the
> requirements I got was not to probe IOports or MMIO registers blindly,
> but rather to use DT on arm and ACPI on i386 to first make sure fw_cfg
> is expected to be there, before tickling its registers :)

> So I need (at least the i386 part of) this series in qemu before I can
> move ahead with the (guest) kernel driver...

Why can't you work on your guest kernel driver using a set of
out-of-tree QEMU patches to test it with?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] acpi: arm: add fw_cfg device node to dsdt

[Gabriel L. Somlo]

On Tue, Sep 15, 2015 at 02:51:31PM +0100, Peter Maydell wrote:
> On 15 September 2015 at 14:42, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> > On Tue, Sep 15, 2015 at 10:06:45AM +0800, Shannon Zhao wrote:
> >> This looks fine to me. But from what you said, the kernel driver is not
> >> in upstream kernel yet, so this would be applied after the kernel patch
> >> applied in case of unexpected change.
> >
> > I guess that's ok if you're only talking about the arm side of things
> > -- although I can't imagine how any of the above would need to change
> > depending on what happens with the guest-side kernel sysfs driver...
> > We're talking a node name ("FWCF"), a _HID ("QEMU0002"), and a mmio
> > region (given to us by memmap[VIRT_FW_CFG]), and that's all there is
> > to it...
> 
> The underlying requirement here is "we don't want patches until
> the ABI is fixed". For ACPI and DTB on ARM the final arbiter of
> the ABI seems to be the kernel (partly because the reviewers there
> are better informed with the overall ACPI/DTB requirements).
> I don't know who the final arbiter of the ACPI ABI for x86 is.
> 
> We've already had a couple of issues with problems with the ACPI
> ABI for what appear to be very simple "just some registers and
> an interrupt" type devices, including "is this the correct HID?".

OK, assuming part of upstreaming a potential fw_cfg sysfs driver
includes bickering over whether "QEMU0002" should or shouldn't be the
correct _HID, then yeah, keeping this stuff out-of-tree until that's
settled feels like the right thing to do...
 
> > However, on the i386 side, this needs to go in *before* I can continue
> > working on the guest-side kernel sysfs drivers. One of the
> > requirements I got was not to probe IOports or MMIO registers blindly,
> > but rather to use DT on arm and ACPI on i386 to first make sure fw_cfg
> > is expected to be there, before tickling its registers :)
> 
> > So I need (at least the i386 part of) this series in qemu before I can
> > move ahead with the (guest) kernel driver...
> 
> Why can't you work on your guest kernel driver using a set of
> out-of-tree QEMU patches to test it with?

As long as I don't get "convince the qemu guys first, then come back
and talk to us" from the kernel people as well, that should be OK with
me :) :)

I'd still appreciate feedback here in the mean time (e.g. once I do v3
to conditionally include fw_cfg only on >= 2.5, per Eduardo's
suggestion), and whatever else it would take to get the series ready
for acceptance (modulo the kernel naming convention negotiations
you're anticipating).

Thanks a ton,
--Gabriel

[Qemu-devel] [PATCH v2 5/5] fw_cfg: document ACPI device node information

[Gabriel L. Somlo]

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 docs/specs/fw_cfg.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 5bc7b96..b5f4b5d 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -77,6 +77,15 @@ increasing address order, similar to memcpy().
 Selector Register IOport: 0x510
 Data Register IOport:     0x511
 
+== ACPI Interface ==
+
+The fw_cfg device is defined with ACPI ID "QEMU0002". Since we expect
+ACPI tables to be passed into the guest through the fw_cfg device itself,
+the guest-side BIOS can not use ACPI to find fw_cfg. However, once the
+bios is finished setting up ACPI tables and hands control over to the
+guest kernel, the latter can use the fw_cfg ACPI node for a more accurate
+inventory of in-use IOport or MMIO regions.
+
 == Firmware Configuration Items ==
 
 === Signature (Key 0x0000, FW_CFG_SIGNATURE) ===
-- 
2.4.3