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

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

[Gabriel L. Somlo]

Generate an ACPI DSDT node for fw_cfg on pc and arm guests.

New since v8:

	- patch 3/5: on pc/x86, place FWCF node in scope \_SB.PCI0
	  (instead of directly under \_SB), to prevent any possible
	  resource conflict as might be observed by Windows
	  (thanks again to Igor Mammedov for the suggestion!)

Thanks,
  --Gabriel

>New since v7:
>
>	- edited commit blurb on 3/5 to match updated content, i.e. that
>	  the ACPI node is now inserted into the DSDT (no longer the SSDT).
>	  (Thanks to Igor Mammedov for catching that!)
>
>>New since v6:
>>	- rebased to fit on top of fb306ff and f264d36, which moved things
>>	  around in pc's acpi-build.c (only patch 3/5 affected);
>>	- kernel-side fw_cfg sysfs driver accepted into upstream linux
>>
>>>New since v5:
>>>
>>>	- rebased on top of latest QEMU git master
>>>
>>>>New since v4:
>>>>
>>>>	- rebased on top of Marc's DMA series
>>>>	- drop machine compat dependency for insertion into x86/ssdt
>>>>	  (patch 3/5), following agreement between Igor and Eduardo
>>>>	- [mm]io register range now covers DMA register as well, if
>>>>	  available.
>>>>	- s/bios/firmware in doc file updates
>>>>
>>>>>New since v3:
>>>>>
>>>>>	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
>>>>>	  inserting fw_cfg acpi node only for machines >= 2.5.
>>>>>
>>>>>	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
>>>>>	  off to avoid Windows complaining -- thanks Igor for catching that!)
>>>>>
>>>>>If there's any other feedback besides questions regarding the
>>>>>appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
>>>>>
>>>>>>New since v2:
>>>>>>
>>>>>>	- pc/i386 node in ssdt only on machine types *newer* than 2.4
>>>>>>	  (as suggested by Eduardo)
>>>>>>
>>>>>>I appreciate any further comments and reviews. Hopefully we can make
>>>>>>this palatable for upstream, modulo the lingering concerns about whether
>>>>>>"QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
>>>>>>sorted out with the kernel crew...
>>>>>>
>>>>>>>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).
>>>>>>>
>>>>>>>> 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 dsdt
  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  | 15 +++++++++++++++
 hw/i386/acpi-build.c      | 29 +++++++++++++++++++++++++++++
 hw/i386/pc.c              |  5 ++---
 hw/nvram/fw_cfg.c         |  4 +++-
 include/hw/i386/pc.h      |  2 ++
 include/hw/nvram/fw_cfg.h |  3 +++
 7 files changed, 63 insertions(+), 4 deletions(-)

-- 
2.4.3

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

[Gabriel L. Somlo]

Expose the size of the control register (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 (classic, non-DMA) port I/O region reserved
for the device.

Cc: Marc Marí <markmb@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marc Marí <markmb@redhat.com>
---
 hw/nvram/fw_cfg.c         | 4 +++-
 include/hw/nvram/fw_cfg.h | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 79c5742..ef2a219 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -32,7 +32,6 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_CTL_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
@@ -882,6 +881,9 @@ 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_CTL_SIZE);
     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 664eaf6..2667ca9 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 register */
+#define FW_CFG_CTL_SIZE         0x02
+
 #define FW_CFG_MAX_FILE_PATH    56
 
 #ifndef NO_QEMU_PROTOS
-- 
2.4.3

[Qemu-devel] [PATCH v9 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.

Cc: Marc Marí <markmb@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marc Marí <markmb@redhat.com>
---
 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 0aeefd2..56ec6cd 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -78,7 +78,6 @@
 #define DPRINTF(fmt, ...)
 #endif
 
-#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)
@@ -756,7 +755,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
+    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as);
 
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
@@ -1258,7 +1257,7 @@ void 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 8b3546e..79ffe5b 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -266,6 +266,8 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
 ISADevice *pc_find_fdc0(void);
 
+#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 v9 3/5] acpi: pc: add fw_cfg device node to dsdt

[Gabriel L. Somlo]

Add a fw_cfg device node to the ACPI DSDT. While the guest-side
firmware 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marc Marí <markmb@redhat.com>
---
 hw/i386/acpi-build.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4554eb8..915fddd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2190,6 +2190,35 @@ build_dsdt(GArray *table_data, GArray *linker,
     aml_append(scope, aml_name_decl("_S5", pkg));
     aml_append(dsdt, scope);
 
+    /* create fw_cfg node, unconditionally */
+    {
+        /* 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; when using DMA, the
+         * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
+        uint8_t io_size = object_property_get_bool(OBJECT(pcms->fw_cfg),
+                                                   "dma_enabled", NULL) ?
+                          ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
+                          FW_CFG_CTL_SIZE;
+
+        scope = aml_scope("\\_SB.PCI0");
+        dev = aml_device("FWCF");
+
+        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
+
+        /* device present, functioning, decoding, not shown in UI */
+        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
+
+        crs = aml_resource_template();
+        aml_append(crs,
+            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size)
+        );
+        aml_append(dev, aml_name_decl("_CRS", crs));
+
+        aml_append(scope, dev);
+        aml_append(dsdt, scope);
+    }
+
     if (misc->applesmc_io_base) {
         scope = aml_scope("\\_SB.PCI0.ISA");
         dev = aml_device("SMC");
-- 
2.4.3

[Qemu-devel] [PATCH v9 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marc Marí <markmb@redhat.com>
---
 hw/arm/virt-acpi-build.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 8cf9a21..7d7998b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -81,6 +81,20 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_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")));
+    /* device present, functioning, decoding, not shown in UI */
+    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
+
+    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;
@@ -548,6 +562,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
+    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
     acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
-- 
2.4.3

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

[Gabriel L. Somlo]

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Marc Marí <markmb@redhat.com>
---
 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 2099ad9..5414140 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -84,6 +84,15 @@ Selector Register address: Base + 8 (2 bytes)
 Data Register address:     Base + 0 (8 bytes)
 DMA Address address:       Base + 16 (8 bytes)
 
+== 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 firmware can not use ACPI to find fw_cfg. However, once the
+firmware 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

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

[Shannon Zhao]

On 2016/2/20 2:20, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Marc Marí <markmb@redhat.com>
Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> ---
>   hw/arm/virt-acpi-build.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 8cf9a21..7d7998b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -81,6 +81,20 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_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")));
> +    /* device present, functioning, decoding, not shown in UI */
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +    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;
> @@ -548,6 +562,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>       acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                          (irqmap[VIRT_UART] + ARM_SPI_BASE));
>       acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>       acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                       (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
>       acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
>

-- 
Shannon

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

[Igor Mammedov]

On Fri, 19 Feb 2016 13:20:27 -0500
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> Add a fw_cfg device node to the ACPI DSDT. While the guest-side
> firmware 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Marc Marí <markmb@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/acpi-build.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4554eb8..915fddd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2190,6 +2190,35 @@ build_dsdt(GArray *table_data, GArray *linker,
>      aml_append(scope, aml_name_decl("_S5", pkg));
>      aml_append(dsdt, scope);
>  
> +    /* create fw_cfg node, unconditionally */
> +    {
> +        /* 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; when using DMA, the
> +         * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
> +        uint8_t io_size = object_property_get_bool(OBJECT(pcms->fw_cfg),
> +                                                   "dma_enabled", NULL) ?
> +                          ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
> +                          FW_CFG_CTL_SIZE;
> +
> +        scope = aml_scope("\\_SB.PCI0");
> +        dev = aml_device("FWCF");
> +
> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +
> +        /* device present, functioning, decoding, not shown in UI */
> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +        crs = aml_resource_template();
> +        aml_append(crs,
> +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size)
> +        );
> +        aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +        aml_append(scope, dev);
> +        aml_append(dsdt, scope);
> +    }
> +
>      if (misc->applesmc_io_base) {
>          scope = aml_scope("\\_SB.PCI0.ISA");
>          dev = aml_device("SMC");

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

[Igor Mammedov]

On Fri, 19 Feb 2016 13:20:24 -0500
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

Gerd,

if you are going to apply it, you'll need to update
expected tables for acpi test (rebuild-expected-aml.sh)
as a commit on top of this series.

> Generate an ACPI DSDT node for fw_cfg on pc and arm guests.
> 
> New since v8:
> 
> 	- patch 3/5: on pc/x86, place FWCF node in scope \_SB.PCI0
> 	  (instead of directly under \_SB), to prevent any possible
> 	  resource conflict as might be observed by Windows
> 	  (thanks again to Igor Mammedov for the suggestion!)
> 
> Thanks,
>   --Gabriel
> 
> >New since v7:
> >
> >	- edited commit blurb on 3/5 to match updated content, i.e. that
> >	  the ACPI node is now inserted into the DSDT (no longer the SSDT).
> >	  (Thanks to Igor Mammedov for catching that!)
> >  
> >>New since v6:
> >>	- rebased to fit on top of fb306ff and f264d36, which moved things
> >>	  around in pc's acpi-build.c (only patch 3/5 affected);
> >>	- kernel-side fw_cfg sysfs driver accepted into upstream linux
> >>  
> >>>New since v5:
> >>>
> >>>	- rebased on top of latest QEMU git master
> >>>  
> >>>>New since v4:
> >>>>
> >>>>	- rebased on top of Marc's DMA series
> >>>>	- drop machine compat dependency for insertion into x86/ssdt
> >>>>	  (patch 3/5), following agreement between Igor and Eduardo
> >>>>	- [mm]io register range now covers DMA register as well, if
> >>>>	  available.
> >>>>	- s/bios/firmware in doc file updates
> >>>>  
> >>>>>New since v3:
> >>>>>
> >>>>>	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
> >>>>>	  inserting fw_cfg acpi node only for machines >= 2.5.
> >>>>>
> >>>>>	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
> >>>>>	  off to avoid Windows complaining -- thanks Igor for catching that!)
> >>>>>
> >>>>>If there's any other feedback besides questions regarding the
> >>>>>appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
> >>>>>  
> >>>>>>New since v2:
> >>>>>>
> >>>>>>	- pc/i386 node in ssdt only on machine types *newer* than 2.4
> >>>>>>	  (as suggested by Eduardo)
> >>>>>>
> >>>>>>I appreciate any further comments and reviews. Hopefully we can make
> >>>>>>this palatable for upstream, modulo the lingering concerns about whether
> >>>>>>"QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
> >>>>>>sorted out with the kernel crew...
> >>>>>>  
> >>>>>>>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).
> >>>>>>>  
> >>>>>>>> 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 dsdt
>   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  | 15 +++++++++++++++
>  hw/i386/acpi-build.c      | 29 +++++++++++++++++++++++++++++
>  hw/i386/pc.c              |  5 ++---
>  hw/nvram/fw_cfg.c         |  4 +++-
>  include/hw/i386/pc.h      |  2 ++
>  include/hw/nvram/fw_cfg.h |  3 +++
>  7 files changed, 63 insertions(+), 4 deletions(-)
> 

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

[Gerd Hoffmann]

On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:
> On Fri, 19 Feb 2016 13:20:24 -0500
> "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> 
> Gerd,
> 
> if you are going to apply it, you'll need to update
> expected tables for acpi test (rebuild-expected-aml.sh)
> as a commit on top of this series.

Sure?

Applied the series, ran "make check", no errors.

cheers,
  Gerd

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

[Igor Mammedov]

On Tue, 23 Feb 2016 09:51:52 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:
> > On Fri, 19 Feb 2016 13:20:24 -0500
> > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > 
> > Gerd,
> > 
> > if you are going to apply it, you'll need to update
> > expected tables for acpi test (rebuild-expected-aml.sh)
> > as a commit on top of this series.  
> 
> Sure?
> 
> Applied the series, ran "make check", no errors.
it's a warning so far not a hard error but it's still there.

> 
> cheers,
>   Gerd
> 

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

[Michael S. Tsirkin]

On Fri, Feb 19, 2016 at 01:20:27PM -0500, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI DSDT. While the guest-side
> firmware 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Marc Marí <markmb@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/i386/acpi-build.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4554eb8..915fddd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2190,6 +2190,35 @@ build_dsdt(GArray *table_data, GArray *linker,
>      aml_append(scope, aml_name_decl("_S5", pkg));
>      aml_append(dsdt, scope);
>  
> +    /* create fw_cfg node, unconditionally */
> +    {
> +        /* 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; when using DMA, the
> +         * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
> +        uint8_t io_size = object_property_get_bool(OBJECT(pcms->fw_cfg),
> +                                                   "dma_enabled", NULL) ?
> +                          ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
> +                          FW_CFG_CTL_SIZE;
> +
> +        scope = aml_scope("\\_SB.PCI0");
> +        dev = aml_device("FWCF");
> +
> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +
> +        /* device present, functioning, decoding, not shown in UI */
> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +        crs = aml_resource_template();
> +        aml_append(crs,
> +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size)
> +        );
> +        aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +        aml_append(scope, dev);
> +        aml_append(dsdt, scope);
> +    }
> +
>      if (misc->applesmc_io_base) {
>          scope = aml_scope("\\_SB.PCI0.ISA");
>          dev = aml_device("SMC");
> -- 
> 2.4.3

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

[Michael S. Tsirkin]

On Fri, Feb 19, 2016 at 01:20:28PM -0500, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Marc Marí <markmb@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 8cf9a21..7d7998b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -81,6 +81,20 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_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")));
> +    /* device present, functioning, decoding, not shown in UI */
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +    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;
> @@ -548,6 +562,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> +    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
>                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
>      acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
> -- 
> 2.4.3

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

[Michael S. Tsirkin]

On Fri, Feb 19, 2016 at 01:20:29PM -0500, Gabriel L. Somlo wrote:
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Marc Marí <markmb@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  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 2099ad9..5414140 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -84,6 +84,15 @@ Selector Register address: Base + 8 (2 bytes)
>  Data Register address:     Base + 0 (8 bytes)
>  DMA Address address:       Base + 16 (8 bytes)
>  
> +== 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 firmware can not use ACPI to find fw_cfg. However, once the
> +firmware 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

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

[Michael S. Tsirkin]

On Tue, Feb 23, 2016 at 09:51:52AM +0100, Gerd Hoffmann wrote:
> On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:
> > On Fri, 19 Feb 2016 13:20:24 -0500
> > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > 
> > Gerd,
> > 
> > if you are going to apply it, you'll need to update
> > expected tables for acpi test (rebuild-expected-aml.sh)
> > as a commit on top of this series.
> 
> Sure?
> 
> Applied the series, ran "make check", no errors.
> 
> cheers,
>   Gerd

It's a warning not an error because of all the false positives it causes.

This is mostly an ACPI patchset, so my tree seems more
appropriate, but in case you prefer your tree,
I sent a Reviewed-by: Michael S. Tsirkin <mst@redhat.com>.

In that case please do update the expected tables.

-- 
MST

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

[Gerd Hoffmann]

On Di, 2016-02-23 at 10:48 +0100, Igor Mammedov wrote:
> On Tue, 23 Feb 2016 09:51:52 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:
> > > On Fri, 19 Feb 2016 13:20:24 -0500
> > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > 
> > > Gerd,
> > > 
> > > if you are going to apply it, you'll need to update
> > > expected tables for acpi test (rebuild-expected-aml.sh)
> > > as a commit on top of this series.  
> > 
> > Sure?
> > 
> > Applied the series, ran "make check", no errors.
> it's a warning so far not a hard error but it's still there.

Checked again, no warning printed here ...

Also "git diff" doesn't list any changes after running the rebuild
script.

But when booting a live iso I can see the new QEMU0002 device listed
in /proc/ioports.

Any clues what is going on here?

cheers,
  Gerd

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

[Michael S. Tsirkin]

On Tue, Feb 23, 2016 at 11:39:05AM +0100, Gerd Hoffmann wrote:
> On Di, 2016-02-23 at 10:48 +0100, Igor Mammedov wrote:
> > On Tue, 23 Feb 2016 09:51:52 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> > > On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:
> > > > On Fri, 19 Feb 2016 13:20:24 -0500
> > > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > > 
> > > > Gerd,
> > > > 
> > > > if you are going to apply it, you'll need to update
> > > > expected tables for acpi test (rebuild-expected-aml.sh)
> > > > as a commit on top of this series.  
> > > 
> > > Sure?
> > > 
> > > Applied the series, ran "make check", no errors.
> > it's a warning so far not a hard error but it's still there.
> 
> Checked again, no warning printed here ...
> 
> Also "git diff" doesn't list any changes after running the rebuild
> script.
> 
> But when booting a live iso I can see the new QEMU0002 device listed
> in /proc/ioports.
> 
> Any clues what is going on here?
> 
> cheers,
>   Gerd

I'll look into this Thursday when I get to this.

-- 
MST

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

[Igor Mammedov]

On Tue, 23 Feb 2016 11:39:05 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Di, 2016-02-23 at 10:48 +0100, Igor Mammedov wrote:
> > On Tue, 23 Feb 2016 09:51:52 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:  
> > > > On Fri, 19 Feb 2016 13:20:24 -0500
> > > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > > 
> > > > Gerd,
> > > > 
> > > > if you are going to apply it, you'll need to update
> > > > expected tables for acpi test (rebuild-expected-aml.sh)
> > > > as a commit on top of this series.    
> > > 
> > > Sure?
> > > 
> > > Applied the series, ran "make check", no errors.  
> > it's a warning so far not a hard error but it's still there.  
> 
> Checked again, no warning printed here ...
it works for me, I get something like:
GTESTER check-qtest-x86_64
acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-66EZCY.dsl, aml:/tmp/aml-MBFZCY], Expected [asl:/tmp/asl-N1N0CY.dsl, aml:tests/acpi-test-data/q35/DSDT.bridge].


> 
> Also "git diff" doesn't list any changes after running the rebuild
> script.
it work for as well,
just a wild guess, may be test for some reason doesn't use qemu with patches applied
that would result in test not seeing any changes.

> But when booting a live iso I can see the new QEMU0002 device listed
> in /proc/ioports.
> 
> Any clues what is going on here?
> 
> cheers,
>   Gerd
> 
> 

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

[Michael S. Tsirkin]

On Tue, Feb 23, 2016 at 04:52:16PM +0100, Igor Mammedov wrote:
> On Tue, 23 Feb 2016 11:39:05 +0100
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > On Di, 2016-02-23 at 10:48 +0100, Igor Mammedov wrote:
> > > On Tue, 23 Feb 2016 09:51:52 +0100
> > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >   
> > > > On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:  
> > > > > On Fri, 19 Feb 2016 13:20:24 -0500
> > > > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > > > 
> > > > > Gerd,
> > > > > 
> > > > > if you are going to apply it, you'll need to update
> > > > > expected tables for acpi test (rebuild-expected-aml.sh)
> > > > > as a commit on top of this series.    
> > > > 
> > > > Sure?
> > > > 
> > > > Applied the series, ran "make check", no errors.  
> > > it's a warning so far not a hard error but it's still there.  
> > 
> > Checked again, no warning printed here ...
> it works for me, I get something like:
> GTESTER check-qtest-x86_64
> acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-66EZCY.dsl, aml:/tmp/aml-MBFZCY], Expected [asl:/tmp/asl-N1N0CY.dsl, aml:tests/acpi-test-data/q35/DSDT.bridge].

is iasl installed on the build system?
This test is skipped if there's no iasl.

> 
> > 
> > Also "git diff" doesn't list any changes after running the rebuild
> > script.
> it work for as well,
> just a wild guess, may be test for some reason doesn't use qemu with patches applied
> that would result in test not seeing any changes.
> 
> > But when booting a live iso I can see the new QEMU0002 device listed
> > in /proc/ioports.
> > 
> > Any clues what is going on here?
> > 
> > cheers,
> >   Gerd
> > 
> > 

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

[Igor Mammedov]

On Tue, 23 Feb 2016 17:54:33 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Feb 23, 2016 at 04:52:16PM +0100, Igor Mammedov wrote:
> > On Tue, 23 Feb 2016 11:39:05 +0100
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   
> > > On Di, 2016-02-23 at 10:48 +0100, Igor Mammedov wrote:  
> > > > On Tue, 23 Feb 2016 09:51:52 +0100
> > > > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > >     
> > > > > On Mo, 2016-02-22 at 13:30 +0100, Igor Mammedov wrote:    
> > > > > > On Fri, 19 Feb 2016 13:20:24 -0500
> > > > > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > > > > 
> > > > > > Gerd,
> > > > > > 
> > > > > > if you are going to apply it, you'll need to update
> > > > > > expected tables for acpi test (rebuild-expected-aml.sh)
> > > > > > as a commit on top of this series.      
> > > > > 
> > > > > Sure?
> > > > > 
> > > > > Applied the series, ran "make check", no errors.    
> > > > it's a warning so far not a hard error but it's still there.    
> > > 
> > > Checked again, no warning printed here ...  
> > it works for me, I get something like:
> > GTESTER check-qtest-x86_64
> > acpi-test: Warning! DSDT mismatch. Actual [asl:/tmp/asl-66EZCY.dsl, aml:/tmp/aml-MBFZCY], Expected [asl:/tmp/asl-N1N0CY.dsl, aml:tests/acpi-test-data/q35/DSDT.bridge].  
> 
> is iasl installed on the build system?
> This test is skipped if there's no iasl.
I've tried without iasl as well and it makes test fail
with error, so it's not Gerd's case.

> 
> >   
> > > 
> > > Also "git diff" doesn't list any changes after running the rebuild
> > > script.  
> > it work for as well,
> > just a wild guess, may be test for some reason doesn't use qemu with patches applied
> > that would result in test not seeing any changes.
> >   
> > > But when booting a live iso I can see the new QEMU0002 device listed
> > > in /proc/ioports.
> > > 
> > > Any clues what is going on here?
> > > 
> > > cheers,
> > >   Gerd
> > > 
> > >