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

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

[Gabriel L. Somlo]

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!

Thanks much,
  --Gabriel

>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 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  | 15 +++++++++++++++
 hw/i386/acpi-build.c      | 23 +++++++++++++++++++++++
 hw/i386/pc.c              |  5 ++---
 hw/i386/pc_piix.c         |  1 +
 hw/i386/pc_q35.c          |  1 +
 hw/nvram/fw_cfg.c         |  8 +++++---
 include/hw/i386/pc.h      |  3 +++
 include/hw/nvram/fw_cfg.h |  3 +++
 9 files changed, 62 insertions(+), 6 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v4 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 v4 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 461c128..70a5898 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 ab5413f..86007e3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -211,6 +211,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 v4 3/5] acpi: pc: add fw_cfg device node to ssdt

[Gabriel L. Somlo]

Add a fw_cfg device node to the ACPI SSDT, on machine types
pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
 hw/i386/pc_piix.c    |  1 +
 hw/i386/pc_q35.c     |  1 +
 include/hw/i386/pc.h |  1 +
 4 files changed, 26 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..ece2710 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
            PcPciInfo *pci, PcGuestInfo *guest_info)
 {
     MachineState *machine = MACHINE(qdev_get_machine());
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
     uint32_t nr_mem = machine->ram_slots;
     unsigned acpi_cpus = guest_info->apic_id_limit;
     Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
@@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
     aml_append(scope, aml_name_decl("_S5", pkg));
     aml_append(ssdt, scope);
 
+    if (!pcmc->acpi_no_fw_cfg_node) {
+        scope = aml_scope("\\_SB");
+        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();
+        /* 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");
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3ffb05f..7f5e5d9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     m->alias = NULL;
     m->is_default = 0;
     pcmc->broken_reserved_end = true;
+    pcmc->acpi_no_fw_cfg_node = true;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1b7d3b6..7180ca3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     pc_q35_2_5_machine_options(m);
     m->alias = NULL;
     pcmc->broken_reserved_end = true;
+    pcmc->acpi_no_fw_cfg_node = true;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 86007e3..6d0f1bd 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -62,6 +62,7 @@ struct PCMachineClass {
     bool broken_reserved_end;
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
+    bool acpi_no_fw_cfg_node;
 };
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
-- 
2.4.3

[Qemu-devel] [PATCH v4 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1aaff1f..f314132 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -110,6 +110,20 @@ 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")));
+    /* 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;
@@ -529,6 +543,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

[Qemu-devel] [PATCH v4 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 bc95404..4d85701 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

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

[Laszlo Ersek]

On 09/27/15 23:28, Gabriel L. Somlo wrote:
> 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 */

Suggest to say "control register" here, rather than "control port".

Other than that,

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> +#define FW_CFG_CTL_SIZE         0x02
> +
>  #define FW_CFG_MAX_FILE_PATH    56
>  
>  #ifndef NO_QEMU_PROTOS
> 

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

[Laszlo Ersek]

On 09/27/15 23:28, Gabriel L. Somlo wrote:
> 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 461c128..70a5898 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 ab5413f..86007e3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -211,6 +211,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,
> 

I'm torn whether this should be unified with the *other*
BIOS_CFG_IOPORT* macro definitions (and uses), but since those are
independent (justifiedly), it probably shouldn't.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Michael S. Tsirkin]

On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
> 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!
> 
> Thanks much,
>   --Gabriel

How does /proc/ioports look before and after this patch?


> >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 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  | 15 +++++++++++++++
>  hw/i386/acpi-build.c      | 23 +++++++++++++++++++++++
>  hw/i386/pc.c              |  5 ++---
>  hw/i386/pc_piix.c         |  1 +
>  hw/i386/pc_q35.c          |  1 +
>  hw/nvram/fw_cfg.c         |  8 +++++---
>  include/hw/i386/pc.h      |  3 +++
>  include/hw/nvram/fw_cfg.h |  3 +++
>  9 files changed, 62 insertions(+), 6 deletions(-)
> 
> -- 
> 2.4.3

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

[Laszlo Ersek]

On 09/27/15 23:29, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI SSDT, on machine types
> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
>  hw/i386/pc_piix.c    |  1 +
>  hw/i386/pc_q35.c     |  1 +
>  include/hw/i386/pc.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ece2710 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>             PcPciInfo *pci, PcGuestInfo *guest_info)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>      uint32_t nr_mem = machine->ram_slots;
>      unsigned acpi_cpus = guest_info->apic_id_limit;
>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>      aml_append(scope, aml_name_decl("_S5", pkg));
>      aml_append(ssdt, scope);
>  
> +    if (!pcmc->acpi_no_fw_cfg_node) {
> +        scope = aml_scope("\\_SB");
> +        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();
> +        /* 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)
> +        );

I think "aml_io" should be indented so that it lines up with "crs" above it.

Other than that:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

What Windows guests did you test this with? ("Testing" meant as "looked
at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
that.

Thanks
Laszlo

> +        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");
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3ffb05f..7f5e5d9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      m->alias = NULL;
>      m->is_default = 0;
>      pcmc->broken_reserved_end = true;
> +    pcmc->acpi_no_fw_cfg_node = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1b7d3b6..7180ca3 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      pc_q35_2_5_machine_options(m);
>      m->alias = NULL;
>      pcmc->broken_reserved_end = true;
> +    pcmc->acpi_no_fw_cfg_node = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 86007e3..6d0f1bd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -62,6 +62,7 @@ struct PCMachineClass {
>      bool broken_reserved_end;
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> +    bool acpi_no_fw_cfg_node;
>  };
>  
>  #define TYPE_PC_MACHINE "generic-pc-machine"
> 

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

[Laszlo Ersek]

On 09/27/15 23:29, 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 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1aaff1f..f314132 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -110,6 +110,20 @@ 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")));
> +    /* 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;
> @@ -529,6 +543,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);
> 

Looks sane to me.

Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
/proc/iomem?) I can help with that, if you'd like.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

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

[Laszlo Ersek]

On 09/27/15 23:29, Gabriel L. Somlo wrote:
> 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 bc95404..4d85701 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".

I'd have preferred spelling out "_HID" or "ACPI Hardware ID" here, but I
agree that this wording matches that of "docs/specs/pvpanic.txt".

> 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

Please replace both "BIOS" and "bios" with "firmware".

> +guest kernel, the latter can use the fw_cfg ACPI node for a more accurate
> +inventory of in-use IOport or MMIO regions.
> +

Looks good otherwise. For the next version:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

>  == Firmware Configuration Items ==
>  
>  === Signature (Key 0x0000, FW_CFG_SIGNATURE) ===
> 

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

[Laszlo Ersek]

On 09/29/15 12:27, Michael S. Tsirkin wrote:
> On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
>> 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!
>>
>> Thanks much,
>>   --Gabriel
> 
> How does /proc/ioports look before and after this patch?

+1, I asked the same (/proc/iomem) wrt. the ARM patch.

Thanks
Laszlo

> 
> 
>>> 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 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  | 15 +++++++++++++++
>>  hw/i386/acpi-build.c      | 23 +++++++++++++++++++++++
>>  hw/i386/pc.c              |  5 ++---
>>  hw/i386/pc_piix.c         |  1 +
>>  hw/i386/pc_q35.c          |  1 +
>>  hw/nvram/fw_cfg.c         |  8 +++++---
>>  include/hw/i386/pc.h      |  3 +++
>>  include/hw/nvram/fw_cfg.h |  3 +++
>>  9 files changed, 62 insertions(+), 6 deletions(-)
>>
>> -- 
>> 2.4.3

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

[Laszlo Ersek]

On 09/29/15 12:27, Michael S. Tsirkin wrote:
> On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
>> 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!
>>
>> Thanks much,
>>   --Gabriel
> 
> How does /proc/ioports look before and after this patch?

... I vaguely remember that /proc/ioports and /proc/iomem tracks only
actual allocations by drivers. So the driver is supposed to get the
resources from ACPI, but until a driver actually allocates the ports (I
fail to recall the exact Linux APIs ATM -- apologies), the registers
might not show up in these pseudo-files.

OTOH Gabriel is working on a guest kernel driver that would look at ACPI
I think...

Laszlo

> 
> 
>>> 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 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  | 15 +++++++++++++++
>>  hw/i386/acpi-build.c      | 23 +++++++++++++++++++++++
>>  hw/i386/pc.c              |  5 ++---
>>  hw/i386/pc_piix.c         |  1 +
>>  hw/i386/pc_q35.c          |  1 +
>>  hw/nvram/fw_cfg.c         |  8 +++++---
>>  include/hw/i386/pc.h      |  3 +++
>>  include/hw/nvram/fw_cfg.h |  3 +++
>>  9 files changed, 62 insertions(+), 6 deletions(-)
>>
>> -- 
>> 2.4.3

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

[Michael S. Tsirkin]

On Tue, Sep 29, 2015 at 03:59:28PM +0200, Laszlo Ersek wrote:
> On 09/29/15 12:27, Michael S. Tsirkin wrote:
> > On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
> >> 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!
> >>
> >> Thanks much,
> >>   --Gabriel
> > 
> > How does /proc/ioports look before and after this patch?
> 
> ... I vaguely remember that /proc/ioports and /proc/iomem tracks only
> actual allocations by drivers. So the driver is supposed to get the
> resources from ACPI, but until a driver actually allocates the ports (I
> fail to recall the exact Linux APIs ATM -- apologies), the registers
> might not show up in these pseudo-files.
> 
> OTOH Gabriel is working on a guest kernel driver that would look at ACPI
> I think...
> 
> Laszlo

What does the driver do? I hope it doesn't poke at _CRS ...

> > 
> > 
> >>> 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 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  | 15 +++++++++++++++
> >>  hw/i386/acpi-build.c      | 23 +++++++++++++++++++++++
> >>  hw/i386/pc.c              |  5 ++---
> >>  hw/i386/pc_piix.c         |  1 +
> >>  hw/i386/pc_q35.c          |  1 +
> >>  hw/nvram/fw_cfg.c         |  8 +++++---
> >>  include/hw/i386/pc.h      |  3 +++
> >>  include/hw/nvram/fw_cfg.h |  3 +++
> >>  9 files changed, 62 insertions(+), 6 deletions(-)
> >>
> >> -- 
> >> 2.4.3

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
> On 09/27/15 23:29, Gabriel L. Somlo wrote:
> > Add a fw_cfg device node to the ACPI SSDT, on machine types
> > pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> >  hw/i386/pc_piix.c    |  1 +
> >  hw/i386/pc_q35.c     |  1 +
> >  include/hw/i386/pc.h |  1 +
> >  4 files changed, 26 insertions(+)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 95e0c65..ece2710 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >             PcPciInfo *pci, PcGuestInfo *guest_info)
> >  {
> >      MachineState *machine = MACHINE(qdev_get_machine());
> > +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >      uint32_t nr_mem = machine->ram_slots;
> >      unsigned acpi_cpus = guest_info->apic_id_limit;
> >      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> > @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >      aml_append(scope, aml_name_decl("_S5", pkg));
> >      aml_append(ssdt, scope);
> >  
> > +    if (!pcmc->acpi_no_fw_cfg_node) {
> > +        scope = aml_scope("\\_SB");
> > +        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();
> > +        /* 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)
> > +        );
> 
> I think "aml_io" should be indented so that it lines up with "crs" above it.

There are a few other nodes being added in if() {...} bloks
immediately following the fw_cfg one. They *all* indent it this way, I
just made mine look similar. That said, I have no problem indenting
mine differently, if you still want me to... :)

> 
> Other than that:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> What Windows guests did you test this with? ("Testing" meant as "looked
> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
> that.

I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
complains about not being able to find a driver for it :)

Thanks,
--Gabriel

> 
> > +        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");
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 3ffb05f..7f5e5d9 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> >      m->alias = NULL;
> >      m->is_default = 0;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->acpi_no_fw_cfg_node = true;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 1b7d3b6..7180ca3 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >      pc_q35_2_5_machine_options(m);
> >      m->alias = NULL;
> >      pcmc->broken_reserved_end = true;
> > +    pcmc->acpi_no_fw_cfg_node = true;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >  }
> >  
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 86007e3..6d0f1bd 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -62,6 +62,7 @@ struct PCMachineClass {
> >      bool broken_reserved_end;
> >      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >                                             DeviceState *dev);
> > +    bool acpi_no_fw_cfg_node;
> >  };
> >  
> >  #define TYPE_PC_MACHINE "generic-pc-machine"
> > 
> 

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

[Laszlo Ersek]

On 09/29/15 18:46, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
>> On 09/27/15 23:29, Gabriel L. Somlo wrote:
>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
>>>  hw/i386/pc_piix.c    |  1 +
>>>  hw/i386/pc_q35.c     |  1 +
>>>  include/hw/i386/pc.h |  1 +
>>>  4 files changed, 26 insertions(+)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 95e0c65..ece2710 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>             PcPciInfo *pci, PcGuestInfo *guest_info)
>>>  {
>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>>      uint32_t nr_mem = machine->ram_slots;
>>>      unsigned acpi_cpus = guest_info->apic_id_limit;
>>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>      aml_append(scope, aml_name_decl("_S5", pkg));
>>>      aml_append(ssdt, scope);
>>>  
>>> +    if (!pcmc->acpi_no_fw_cfg_node) {
>>> +        scope = aml_scope("\\_SB");
>>> +        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();
>>> +        /* 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)
>>> +        );
>>
>> I think "aml_io" should be indented so that it lines up with "crs" above it.
> 
> There are a few other nodes being added in if() {...} bloks
> immediately following the fw_cfg one. They *all* indent it this way, I
> just made mine look similar. That said, I have no problem indenting
> mine differently, if you still want me to... :)

Nah, if you are consistent with existing code there, I'm fine.

> 
>>
>> Other than that:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> What Windows guests did you test this with? ("Testing" meant as "looked
>> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
>> that.
> 
> I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
> complains about not being able to find a driver for it :)

So you had to clear bit 0 (value 1, "device is present") and bit 1
(value 2, "device is enabled and decoding its resources") in _STA,
relative to 0xB visible above? I'm not sure if that's a good thing.
First, setting bit 3 (value 8, "device is functioning properly"0 without
the device being present is... strange. Second, won't that prevent you
from using the resources even in the Linux driver?

(My working assumption is that you're doing this for QEMU because GregKH
(IIRC?) told you that the kernel driver should be keying off of ACPI. Is
that right?)

Thanks!
Laszlo

> 
> Thanks,
> --Gabriel
> 
>>
>>> +        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");
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 3ffb05f..7f5e5d9 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>>>      m->alias = NULL;
>>>      m->is_default = 0;
>>>      pcmc->broken_reserved_end = true;
>>> +    pcmc->acpi_no_fw_cfg_node = true;
>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>>  }
>>>  
>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>> index 1b7d3b6..7180ca3 100644
>>> --- a/hw/i386/pc_q35.c
>>> +++ b/hw/i386/pc_q35.c
>>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>>>      pc_q35_2_5_machine_options(m);
>>>      m->alias = NULL;
>>>      pcmc->broken_reserved_end = true;
>>> +    pcmc->acpi_no_fw_cfg_node = true;
>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>>  }
>>>  
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index 86007e3..6d0f1bd 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -62,6 +62,7 @@ struct PCMachineClass {
>>>      bool broken_reserved_end;
>>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>>                                             DeviceState *dev);
>>> +    bool acpi_no_fw_cfg_node;
>>>  };
>>>  
>>>  #define TYPE_PC_MACHINE "generic-pc-machine"
>>>
>>

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
> On 09/29/15 18:46, Gabriel L. Somlo wrote:
> > On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
> >> On 09/27/15 23:29, Gabriel L. Somlo wrote:
> >>> Add a fw_cfg device node to the ACPI SSDT, on machine types
> >>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> >>>  hw/i386/pc_piix.c    |  1 +
> >>>  hw/i386/pc_q35.c     |  1 +
> >>>  include/hw/i386/pc.h |  1 +
> >>>  4 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>> index 95e0c65..ece2710 100644
> >>> --- a/hw/i386/acpi-build.c
> >>> +++ b/hw/i386/acpi-build.c
> >>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>             PcPciInfo *pci, PcGuestInfo *guest_info)
> >>>  {
> >>>      MachineState *machine = MACHINE(qdev_get_machine());
> >>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >>>      uint32_t nr_mem = machine->ram_slots;
> >>>      unsigned acpi_cpus = guest_info->apic_id_limit;
> >>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> >>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>      aml_append(scope, aml_name_decl("_S5", pkg));
> >>>      aml_append(ssdt, scope);
> >>>  
> >>> +    if (!pcmc->acpi_no_fw_cfg_node) {
> >>> +        scope = aml_scope("\\_SB");
> >>> +        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();
> >>> +        /* 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)
> >>> +        );
> >>
> >> I think "aml_io" should be indented so that it lines up with "crs" above it.
> > 
> > There are a few other nodes being added in if() {...} bloks
> > immediately following the fw_cfg one. They *all* indent it this way, I
> > just made mine look similar. That said, I have no problem indenting
> > mine differently, if you still want me to... :)
> 
> Nah, if you are consistent with existing code there, I'm fine.
> 
> > 
> >>
> >> Other than that:
> >>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >>
> >> What Windows guests did you test this with? ("Testing" meant as "looked
> >> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
> >> that.
> > 
> > I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
> > complains about not being able to find a driver for it :)
> 
> So you had to clear bit 0 (value 1, "device is present") and bit 1
> (value 2, "device is enabled and decoding its resources") in _STA,
> relative to 0xB visible above? I'm not sure if that's a good thing.
> First, setting bit 3 (value 8, "device is functioning properly"0 without
> the device being present is... strange. Second, won't that prevent you
> from using the resources even in the Linux driver?

no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
Leaving out _STA entirely would have had it default to 0x0F, and the
"show in u/i" bit caused Windows to show it in the device manager with
a yellow excalmation sign... So I had to go back and add an explicit
_STA with the u/i bit turned off.

> (My working assumption is that you're doing this for QEMU because GregKH
> (IIRC?) told you that the kernel driver should be keying off of ACPI. Is
> that right?)

First, to answer mst's question elswhere in this thread, I'm
working on a kernel sysfs driver that would list fw_cfg blobs in
sysfs (just like /sys/firmware/dmi/entries/...) so userspace could
simply "cat" or "cp" the raw blobs.

GregKH told me to try udev for the friendly path blobname expansion
(your "I like using find on /sys/firmware..." recommendation). He never
said anything about ACPI (not sure he would have eventually or not).

It all started with ardb saying "NAK on arm if you touch the mmio
registers before checking with DT that you even have a fw_cfg device".

I sort-of figured I'd better not touch IOport registers either before
I know I have a fw_cfg device, hence this whole exercise of adding it
to ACPI. Although I still have to figure out how one would do
something like

	if (search_acpi_for_hid("QEMU0002") == NULL)

		return -ENODEV;

from a module_init method... Couldn't find any examples (yet) in the
kernel source, and starting to wonder if maybe this is not how ACPI is
supposed to work, and somehow ACPI initialization itself is somehow
expected to trigger loading modules for devices it encounters...

Obviously, since sun4* and ppc/mac have neither DT nor ACPI, this will
have to be limited to x86 and arm, but OK...

Dividing the overall problem into smaller, independently
comprehensible bits doesn't seem to be working out all that
great for me, so far... In Soviet Russia, problem divide-and-conquer YOU!
:)

At least we're getting a documented reservation of whatever mmio or
ioport region is occupied by fw_cfg in ACPI, so either way I think
it's worth it (whether it ends up helping me with my long term goals
or not :)

Thanks much,
--Gabriel

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

[Laszlo Ersek]

On 09/29/15 19:19, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
>> On 09/29/15 18:46, Gabriel L. Somlo wrote:
>>> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
>>>> On 09/27/15 23:29, Gabriel L. Somlo wrote:
>>>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>>>>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
>>>>>  hw/i386/pc_piix.c    |  1 +
>>>>>  hw/i386/pc_q35.c     |  1 +
>>>>>  include/hw/i386/pc.h |  1 +
>>>>>  4 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>>> index 95e0c65..ece2710 100644
>>>>> --- a/hw/i386/acpi-build.c
>>>>> +++ b/hw/i386/acpi-build.c
>>>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>>>             PcPciInfo *pci, PcGuestInfo *guest_info)
>>>>>  {
>>>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>>>>      uint32_t nr_mem = machine->ram_slots;
>>>>>      unsigned acpi_cpus = guest_info->apic_id_limit;
>>>>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
>>>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>>>      aml_append(scope, aml_name_decl("_S5", pkg));
>>>>>      aml_append(ssdt, scope);
>>>>>  
>>>>> +    if (!pcmc->acpi_no_fw_cfg_node) {
>>>>> +        scope = aml_scope("\\_SB");
>>>>> +        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();
>>>>> +        /* 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)
>>>>> +        );
>>>>
>>>> I think "aml_io" should be indented so that it lines up with "crs" above it.
>>>
>>> There are a few other nodes being added in if() {...} bloks
>>> immediately following the fw_cfg one. They *all* indent it this way, I
>>> just made mine look similar. That said, I have no problem indenting
>>> mine differently, if you still want me to... :)
>>
>> Nah, if you are consistent with existing code there, I'm fine.
>>
>>>
>>>>
>>>> Other than that:
>>>>
>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>> What Windows guests did you test this with? ("Testing" meant as "looked
>>>> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
>>>> that.
>>>
>>> I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
>>> complains about not being able to find a driver for it :)
>>
>> So you had to clear bit 0 (value 1, "device is present") and bit 1
>> (value 2, "device is enabled and decoding its resources") in _STA,
>> relative to 0xB visible above? I'm not sure if that's a good thing.
>> First, setting bit 3 (value 8, "device is functioning properly"0 without
>> the device being present is... strange. Second, won't that prevent you
>> from using the resources even in the Linux driver?
> 
> no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
> Leaving out _STA entirely would have had it default to 0x0F, and the
> "show in u/i" bit caused Windows to show it in the device manager with
> a yellow excalmation sign... So I had to go back and add an explicit
> _STA with the u/i bit turned off.

Ah okay. So when you wrote "re-adding _STA set to 0x08", you actually
meant *this* patch. Right? (I don't really understand the reference to
0x08.)

So I take you tested *this* patch with Windows 7, and it was satisfied.
Good.

> 
>> (My working assumption is that you're doing this for QEMU because GregKH
>> (IIRC?) told you that the kernel driver should be keying off of ACPI. Is
>> that right?)
> 
> First, to answer mst's question elswhere in this thread, I'm
> working on a kernel sysfs driver that would list fw_cfg blobs in
> sysfs (just like /sys/firmware/dmi/entries/...) so userspace could
> simply "cat" or "cp" the raw blobs.
> 
> GregKH told me to try udev for the friendly path blobname expansion
> (your "I like using find on /sys/firmware..." recommendation). He never
> said anything about ACPI (not sure he would have eventually or not).
> 
> It all started with ardb saying "NAK on arm if you touch the mmio
> registers before checking with DT that you even have a fw_cfg device".
> 
> I sort-of figured I'd better not touch IOport registers either before
> I know I have a fw_cfg device, hence this whole exercise of adding it
> to ACPI. Although I still have to figure out how one would do
> something like
> 
> 	if (search_acpi_for_hid("QEMU0002") == NULL)
> 
> 		return -ENODEV;
> 
> from a module_init method... Couldn't find any examples (yet) in the
> kernel source, and starting to wonder if maybe this is not how ACPI is
> supposed to work, and somehow ACPI initialization itself is somehow
> expected to trigger loading modules for devices it encounters...
> 
> Obviously, since sun4* and ppc/mac have neither DT nor ACPI, this will
> have to be limited to x86 and arm, but OK...
> 
> Dividing the overall problem into smaller, independently
> comprehensible bits doesn't seem to be working out all that
> great for me, so far... In Soviet Russia, problem divide-and-conquer YOU!
> :)
> 
> At least we're getting a documented reservation of whatever mmio or
> ioport region is occupied by fw_cfg in ACPI, so either way I think
> it's worth it (whether it ends up helping me with my long term goals
> or not :)

Unfortunately I can't help you at all with kernel driver development,
but I do sense a kind of dependency hell here (maybe even with a cycle
in it) where whatever you try to implement, someone says "please do X
first". :/

Thanks
Laszlo

> Thanks much,
> --Gabriel
> 

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 01:27:38PM +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
> > 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!
> > 
> > Thanks much,
> >   --Gabriel
> 
> How does /proc/ioports look before and after this patch?

0000-0cf7 : PCI Bus 0000:00
  0000-001f : dma1
  0020-0021 : pic1
  0040-0043 : timer0
  0050-0053 : timer1
  0060-0060 : keyboard
  0064-0064 : keyboard
  0070-0071 : rtc0
  0080-008f : dma page reg
  00a0-00a1 : pic2
  00c0-00df : dma2
  00f0-00ff : fpu
  0378-037a : parport0
  03c0-03df : vga+
  03f8-03ff : serial
  0510-0511 : QEMU0002:00		<<<< this shows up "after"
  0600-067f : 0000:00:1f.0
    0600-0603 : ACPI PM1a_EVT_BLK
    0604-0605 : ACPI PM1a_CNT_BLK
    0608-060b : ACPI PM_TMR
    0620-062f : ACPI GPE0_BLK
  0700-073f : 0000:00:1f.3
    0700-073f : i801_smbus
0cf8-0cff : PCI conf1
0d00-ffff : PCI Bus 0000:00
  c000-c03f : 0000:00:02.0
    c000-c03f : e1000
  c080-c09f : 0000:00:1f.2
    c080-c09f : ahci


Thanks,
--Gabriel

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
> On 09/27/15 23:29, 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 | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 1aaff1f..f314132 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -110,6 +110,20 @@ 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")));
> > +    /* 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;
> > @@ -529,6 +543,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);
> > 
> 
> Looks sane to me.
> 
> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
> /proc/iomem?) I can help with that, if you'd like.

I have a F22 arm setup generated by virt-builder, which I start using:

bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
  -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
  -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
  -append "console=ttyAMA0 root=/dev/vda3 ro" \
  -device virtio-blk-device,drive=hd0 \
  -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
  -device virtio-net-device,netdev=usernet \
  -netdev user,id=usernet \
  -monitor stdio

I used it to successfully test my (rude, mmio-poking) kernel sysfs
driver, but somehow it doesn't seem to pay any attention to ACPI.

/proc/iomem looks the same before and after my patch, so no entry for
fw_cfg once I add it to the ssdt host-side.

'acpidump -b' says "Could not get ACPI tables, AE_NOT_FOUND". There's
no /proc/acpi or /sys/firmware/acpi...

Although ACPI tables do get placed in fw_cfg (after loading my linux
kernel driver I can see the "/etc/acpi/tables" blob). And if I 'iasl -d'
that, I can see the FWCF node in the ssdt :)

In the short run, if you can test on your end that'd be great. Also,
if you have any idea why I'm not seeing ACPI at all (missing kernel
module, or wrong firmware, or ???) please let me know...

Thanks,
--Gabriel

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 03:59:28PM +0200, Laszlo Ersek wrote:
> On 09/29/15 12:27, Michael S. Tsirkin wrote:
> > On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
> >> 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!
> >>
> >> Thanks much,
> >>   --Gabriel
> > 
> > How does /proc/ioports look before and after this patch?
> 
> ... I vaguely remember that /proc/ioports and /proc/iomem tracks only
> actual allocations by drivers. So the driver is supposed to get the
> resources from ACPI, but until a driver actually allocates the ports (I
> fail to recall the exact Linux APIs ATM -- apologies), the registers
> might not show up in these pseudo-files.

At least on the i386 side of things, /proc/ioport gets a new line
entry for

  0510-0511 : QEMU0002:00

simply due to the presence of the ACPI node, and without having loaded
any driver.

> 
> OTOH Gabriel is working on a guest kernel driver that would look at ACPI
> I think...

As I mentioned elsewhere, I'm hoping to be able to use the presence of
the fw_cfg node in ACPI (and the base + width data contained in its
_CRS) to avoid probing IO ports. I'm not really clear on how that's
done, and so far haven't found a similar example in the Linux kernel
to use for inspiration, but I'm working on that...

BTW, after I load my (rude, ioport probing, non-acpi-using) sysfs
fw_cfg driver, I get an additional entry on i386:

cat /proc/ioports
...
  0510-0511 : QEMU0002:00
    0510-0511 : fw_cfg IOport on i386, sun4u	<<< after modprobe qemu_fw_cfg
...

Thanks,
--Gabriel

> >>> 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 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  | 15 +++++++++++++++
> >>  hw/i386/acpi-build.c      | 23 +++++++++++++++++++++++
> >>  hw/i386/pc.c              |  5 ++---
> >>  hw/i386/pc_piix.c         |  1 +
> >>  hw/i386/pc_q35.c          |  1 +
> >>  hw/nvram/fw_cfg.c         |  8 +++++---
> >>  include/hw/i386/pc.h      |  3 +++
> >>  include/hw/nvram/fw_cfg.h |  3 +++
> >>  9 files changed, 62 insertions(+), 6 deletions(-)
> >>
> >> -- 
> >> 2.4.3
> 

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

[Laszlo Ersek]

On 09/29/15 20:26, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
>> On 09/27/15 23:29, 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 | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 1aaff1f..f314132 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -110,6 +110,20 @@ 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")));
>>> +    /* 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;
>>> @@ -529,6 +543,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);
>>>
>>
>> Looks sane to me.
>>
>> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
>> /proc/iomem?) I can help with that, if you'd like.
> 
> I have a F22 arm setup generated by virt-builder, which I start using:
> 
> bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
>   -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
>   -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
>   -append "console=ttyAMA0 root=/dev/vda3 ro" \
>   -device virtio-blk-device,drive=hd0 \
>   -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
>   -device virtio-net-device,netdev=usernet \
>   -netdev user,id=usernet \
>   -monitor stdio
> 
> I used it to successfully test my (rude, mmio-poking) kernel sysfs
> driver, but somehow it doesn't seem to pay any attention to ACPI.
> 
> /proc/iomem looks the same before and after my patch, so no entry for
> fw_cfg once I add it to the ssdt host-side.
> 
> 'acpidump -b' says "Could not get ACPI tables, AE_NOT_FOUND". There's
> no /proc/acpi or /sys/firmware/acpi...
> 
> Although ACPI tables do get placed in fw_cfg (after loading my linux
> kernel driver I can see the "/etc/acpi/tables" blob). And if I 'iasl -d'
> that, I can see the FWCF node in the ssdt :)
> 
> In the short run, if you can test on your end that'd be great. Also,
> if you have any idea why I'm not seeing ACPI at all (missing kernel
> module, or wrong firmware, or ???) please let me know...

You are not seeing ACPI tables in the guest because you aren't using a
guest firmware that downloads and installs them (-> ArmVirtQemu, aka.
AAVMF, built from edk2). I'll test your series for that scenario and
report back later.

Thanks
Laszlo



> Thanks,
> --Gabriel
> 
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 05:15:25PM +0300, Michael S. Tsirkin wrote:
> On Tue, Sep 29, 2015 at 03:59:28PM +0200, Laszlo Ersek wrote:
> > On 09/29/15 12:27, Michael S. Tsirkin wrote:
> > > On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
> > >> 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!
> > >>
> > >> Thanks much,
> > >>   --Gabriel
> > > 
> > > How does /proc/ioports look before and after this patch?
> > 
> > ... I vaguely remember that /proc/ioports and /proc/iomem tracks only
> > actual allocations by drivers. So the driver is supposed to get the
> > resources from ACPI, but until a driver actually allocates the ports (I
> > fail to recall the exact Linux APIs ATM -- apologies), the registers
> > might not show up in these pseudo-files.
> > 
> > OTOH Gabriel is working on a guest kernel driver that would look at ACPI
> > I think...
> > 
> > Laszlo
> 
> What does the driver do? I hope it doesn't poke at _CRS ...

I mentioned this elsewhere in the thread, but since I didn't
address your _CRS remark explicitly:

The driver I'm working on (guest-)kernel-side serves to
access fw_cfg blob metadata and raw content in sysfs (look
at /sys/firmware/dmi/entries/... for something similar).

The driver is written, tested, and works great, but right now
it has a list of port-io and mmio (base + size) pairs which
I'm probing in decreasing order of probability:

	1. io-port on i386 (and sun4u)
	2. mmio on arm
	3. mmio on ppc/mac
	4. mmio on sun4m

from its module_init function.

The arm guys basically said "No, you can't do that, use DT to first
*know* for sure you have fw_cfg before touching its mmio registers".

I've sort of assumed that's valid on i386 as well, and that I should
query ACPI for a fw_cfg node (and yes, use whatever is in _CRS to
set the value of the io-port (or mmio) base, and width).

That means dropping support for ppc/mac and sun4m since there's no DT
or ACPI there. I'm also not quite sure how I'd query ACPI during a
module_init function, so if you know of any examples I could use for
inspiratin, I'd be really thankful for a pointer :)

Anyhow, that's the story, any further comments and clues much
appreciated!

Thanks,
--Gabriel

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

[Laszlo Ersek]

On 09/29/15 21:04, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 05:15:25PM +0300, Michael S. Tsirkin wrote:
>> On Tue, Sep 29, 2015 at 03:59:28PM +0200, Laszlo Ersek wrote:
>>> On 09/29/15 12:27, Michael S. Tsirkin wrote:
>>>> On Sun, Sep 27, 2015 at 05:28:57PM -0400, Gabriel L. Somlo wrote:
>>>>> 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!
>>>>>
>>>>> Thanks much,
>>>>>   --Gabriel
>>>>
>>>> How does /proc/ioports look before and after this patch?
>>>
>>> ... I vaguely remember that /proc/ioports and /proc/iomem tracks only
>>> actual allocations by drivers. So the driver is supposed to get the
>>> resources from ACPI, but until a driver actually allocates the ports (I
>>> fail to recall the exact Linux APIs ATM -- apologies), the registers
>>> might not show up in these pseudo-files.
>>>
>>> OTOH Gabriel is working on a guest kernel driver that would look at ACPI
>>> I think...
>>>
>>> Laszlo
>>
>> What does the driver do? I hope it doesn't poke at _CRS ...
> 
> I mentioned this elsewhere in the thread, but since I didn't
> address your _CRS remark explicitly:
> 
> The driver I'm working on (guest-)kernel-side serves to
> access fw_cfg blob metadata and raw content in sysfs (look
> at /sys/firmware/dmi/entries/... for something similar).
> 
> The driver is written, tested, and works great, but right now
> it has a list of port-io and mmio (base + size) pairs which
> I'm probing in decreasing order of probability:
> 
> 	1. io-port on i386 (and sun4u)
> 	2. mmio on arm
> 	3. mmio on ppc/mac
> 	4. mmio on sun4m
> 
> from its module_init function.
> 
> The arm guys basically said "No, you can't do that, use DT to first
> *know* for sure you have fw_cfg before touching its mmio registers".
> 
> I've sort of assumed that's valid on i386 as well, and that I should
> query ACPI for a fw_cfg node (and yes, use whatever is in _CRS to
> set the value of the io-port (or mmio) base, and width).
> 
> That means dropping support for ppc/mac and sun4m since there's no DT
> or ACPI there. I'm also not quite sure how I'd query ACPI during a
> module_init function, so if you know of any examples I could use for
> inspiratin, I'd be really thankful for a pointer :)
> 
> Anyhow, that's the story, any further comments and clues much
> appreciated!

I'd recommend looking at acpi_dev_get_resources(),
acpi_dev_resource_io(), etc in "drivers/acpi/resource.c", but I think
that's exactly what Michael said he hoped your kernel code wouldn't
do... I'm a bit confused about that, admittedly.

Also, if you go the ACPI way, your kernel driver will have to probe /
bind the device based on _HID -- see eg.
"drivers/platform/x86/pvpanic.c" (which is the guest driver for the
pvpanic device, QEMU0001).

Thanks
Laszlo

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

[Gabriel L. Somlo]

On Tue, Sep 29, 2015 at 07:28:39PM +0200, Laszlo Ersek wrote:
> On 09/29/15 19:19, Gabriel L. Somlo wrote:
> > On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
> >> On 09/29/15 18:46, Gabriel L. Somlo wrote:
> >>> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
> >>>> On 09/27/15 23:29, Gabriel L. Somlo wrote:
> >>>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
> >>>>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> >>>>>  hw/i386/pc_piix.c    |  1 +
> >>>>>  hw/i386/pc_q35.c     |  1 +
> >>>>>  include/hw/i386/pc.h |  1 +
> >>>>>  4 files changed, 26 insertions(+)
> >>>>>
> >>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>>> index 95e0c65..ece2710 100644
> >>>>> --- a/hw/i386/acpi-build.c
> >>>>> +++ b/hw/i386/acpi-build.c
> >>>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>>>             PcPciInfo *pci, PcGuestInfo *guest_info)
> >>>>>  {
> >>>>>      MachineState *machine = MACHINE(qdev_get_machine());
> >>>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >>>>>      uint32_t nr_mem = machine->ram_slots;
> >>>>>      unsigned acpi_cpus = guest_info->apic_id_limit;
> >>>>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> >>>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>>>      aml_append(scope, aml_name_decl("_S5", pkg));
> >>>>>      aml_append(ssdt, scope);
> >>>>>  
> >>>>> +    if (!pcmc->acpi_no_fw_cfg_node) {
> >>>>> +        scope = aml_scope("\\_SB");
> >>>>> +        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();
> >>>>> +        /* 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)
> >>>>> +        );
> >>>>
> >>>> I think "aml_io" should be indented so that it lines up with "crs" above it.
> >>>
> >>> There are a few other nodes being added in if() {...} bloks
> >>> immediately following the fw_cfg one. They *all* indent it this way, I
> >>> just made mine look similar. That said, I have no problem indenting
> >>> mine differently, if you still want me to... :)
> >>
> >> Nah, if you are consistent with existing code there, I'm fine.
> >>
> >>>
> >>>>
> >>>> Other than that:
> >>>>
> >>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >>>>
> >>>> What Windows guests did you test this with? ("Testing" meant as "looked
> >>>> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
> >>>> that.
> >>>
> >>> I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
> >>> complains about not being able to find a driver for it :)
> >>
> >> So you had to clear bit 0 (value 1, "device is present") and bit 1
> >> (value 2, "device is enabled and decoding its resources") in _STA,
> >> relative to 0xB visible above? I'm not sure if that's a good thing.
> >> First, setting bit 3 (value 8, "device is functioning properly"0 without
> >> the device being present is... strange. Second, won't that prevent you
> >> from using the resources even in the Linux driver?
> > 
> > no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
> > Leaving out _STA entirely would have had it default to 0x0F, and the
> > "show in u/i" bit caused Windows to show it in the device manager with
> > a yellow excalmation sign... So I had to go back and add an explicit
> > _STA with the u/i bit turned off.
> 
> Ah okay. So when you wrote "re-adding _STA set to 0x08", you actually
> meant *this* patch. Right? (I don't really understand the reference to
> 0x08.)
> 
> So I take you tested *this* patch with Windows 7, and it was satisfied.
> Good.

Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my
brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for
the confusion... :)

Thanks,
--Gabriel

> 
> > 
> >> (My working assumption is that you're doing this for QEMU because GregKH
> >> (IIRC?) told you that the kernel driver should be keying off of ACPI. Is
> >> that right?)
> > 
> > First, to answer mst's question elswhere in this thread, I'm
> > working on a kernel sysfs driver that would list fw_cfg blobs in
> > sysfs (just like /sys/firmware/dmi/entries/...) so userspace could
> > simply "cat" or "cp" the raw blobs.
> > 
> > GregKH told me to try udev for the friendly path blobname expansion
> > (your "I like using find on /sys/firmware..." recommendation). He never
> > said anything about ACPI (not sure he would have eventually or not).
> > 
> > It all started with ardb saying "NAK on arm if you touch the mmio
> > registers before checking with DT that you even have a fw_cfg device".
> > 
> > I sort-of figured I'd better not touch IOport registers either before
> > I know I have a fw_cfg device, hence this whole exercise of adding it
> > to ACPI. Although I still have to figure out how one would do
> > something like
> > 
> > 	if (search_acpi_for_hid("QEMU0002") == NULL)
> > 
> > 		return -ENODEV;
> > 
> > from a module_init method... Couldn't find any examples (yet) in the
> > kernel source, and starting to wonder if maybe this is not how ACPI is
> > supposed to work, and somehow ACPI initialization itself is somehow
> > expected to trigger loading modules for devices it encounters...
> > 
> > Obviously, since sun4* and ppc/mac have neither DT nor ACPI, this will
> > have to be limited to x86 and arm, but OK...
> > 
> > Dividing the overall problem into smaller, independently
> > comprehensible bits doesn't seem to be working out all that
> > great for me, so far... In Soviet Russia, problem divide-and-conquer YOU!
> > :)
> > 
> > At least we're getting a documented reservation of whatever mmio or
> > ioport region is occupied by fw_cfg in ACPI, so either way I think
> > it's worth it (whether it ends up helping me with my long term goals
> > or not :)
> 
> Unfortunately I can't help you at all with kernel driver development,
> but I do sense a kind of dependency hell here (maybe even with a cycle
> in it) where whatever you try to implement, someone says "please do X
> first". :/
> 
> Thanks
> Laszlo
> 
> > Thanks much,
> > --Gabriel
> > 
> 

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

[Ard Biesheuvel]

On 29 September 2015 at 20:26, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
>> On 09/27/15 23:29, 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 | 15 +++++++++++++++
>> >  1 file changed, 15 insertions(+)
>> >
>> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> > index 1aaff1f..f314132 100644
>> > --- a/hw/arm/virt-acpi-build.c
>> > +++ b/hw/arm/virt-acpi-build.c
>> > @@ -110,6 +110,20 @@ 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")));
>> > +    /* 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;
>> > @@ -529,6 +543,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);
>> >
>>
>> Looks sane to me.
>>
>> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
>> /proc/iomem?) I can help with that, if you'd like.
>
> I have a F22 arm setup generated by virt-builder, which I start using:
>
> bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
>   -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
>   -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
>   -append "console=ttyAMA0 root=/dev/vda3 ro" \
>   -device virtio-blk-device,drive=hd0 \
>   -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
>   -device virtio-net-device,netdev=usernet \
>   -netdev user,id=usernet \
>   -monitor stdio
>

Note that you are booting 32-bit ARM here, which does not support ACPI nor UEFI.
(UEFI is work in progress, so you can try my ARM 32-bit UEFI tree if
you need to: https://git.linaro.org/people/ard.biesheuvel/linux-arm.git/shortlog/refs/heads/arm-efi-combined-v2)

You will need to create an arm64 / AArch64 setup and boot the virt
model using 'qemu-system-aarch64 -M virt -cpu cortex-a57 ...' instead.
In either case, as Laszlo pointed out, you need UEFI firmware in QEMU
as well.

-- 
Ard.

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

[Laszlo Ersek]

On 09/30/15 11:59, Ard Biesheuvel wrote:
> On 29 September 2015 at 20:26, Gabriel L. Somlo <somlo@cmu.edu> wrote:
>> On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
>>> On 09/27/15 23:29, 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 | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index 1aaff1f..f314132 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -110,6 +110,20 @@ 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")));
>>>> +    /* 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;
>>>> @@ -529,6 +543,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);
>>>>
>>>
>>> Looks sane to me.
>>>
>>> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
>>> /proc/iomem?) I can help with that, if you'd like.
>>
>> I have a F22 arm setup generated by virt-builder, which I start using:
>>
>> bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
>>   -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
>>   -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
>>   -append "console=ttyAMA0 root=/dev/vda3 ro" \
>>   -device virtio-blk-device,drive=hd0 \
>>   -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
>>   -device virtio-net-device,netdev=usernet \
>>   -netdev user,id=usernet \
>>   -monitor stdio
>>
> 
> Note that you are booting 32-bit ARM here, which does not support ACPI nor UEFI.
> (UEFI is work in progress, so you can try my ARM 32-bit UEFI tree if
> you need to: https://git.linaro.org/people/ard.biesheuvel/linux-arm.git/shortlog/refs/heads/arm-efi-combined-v2)
> 
> You will need to create an arm64 / AArch64 setup and boot the virt
> model using 'qemu-system-aarch64 -M virt -cpu cortex-a57 ...' instead.
> In either case, as Laszlo pointed out, you need UEFI firmware in QEMU
> as well.

I'm about to follow up with my test results, and I considered writing up
a more or less complete guide for Gabriel to test this with an aarch64
guest.

However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
run KVM guests), then I don't think he should bother. Booting just the
UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
for checking "/proc/iomem", he'd really need to boot into guest Linux,
and *that* takes absolutely forever with TCG.

(Dependent on your guest distro, of course; I have tested Fedora 21+ and
RHELSA / RHEL-7 candidates thus far. I wouldn't recommend TCG for those.)

So, I'll just leave these links here for posterity (they could be
somewhat outdated), and I offer to help with aarch64 guest testing in
the future as well, if the patch series overlaps with my interests.

https://wiki.linaro.org/LEG/UEFIforQEMU
https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU

Thanks
Laszlo

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

[Laszlo Ersek]

test results from an aarch64 Linux guest (using KVM and UEFI):

On 09/29/15 12:40, Laszlo Ersek wrote:
> On 09/27/15 23:29, 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 | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 1aaff1f..f314132 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -110,6 +110,20 @@ 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")));
>> +    /* 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;
>> @@ -529,6 +543,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);
>>
> 
> Looks sane to me.
> 
> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d;

So I dumped and decompiled the DSDT, and the relevant output is:

>         Device (FWCF)
>         {
>             Name (_HID, "QEMU0002")  // _HID: Hardware ID
>             Name (_STA, 0x0B)  // _STA: Status
>             Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>             {
>                 Memory32Fixed (ReadWrite,
>                     0x09020000,         // Address Base
>                     0x0000000A,         // Address Length
>                     )
>             })
>         }

This is correct -- the fw_cfg MMIO register block is correctly described by the above. (The actual size will change once Marc's fw_cfg-DMA series is merged, but that will be reflected by this patch automatically.)

Second,

> cat
> /proc/iomem?) I can help with that, if you'd like.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

this is the contents of /proc/iomem:

> 00000000-03ffffff : LNRO0015:00
> 04000000-07ffffff : LNRO0015:01
> 09000000-09000fff : ARMH0011:00
>   09000000-09000fff : ARMH0011:00
> 09010000-09010fff : LNRO0013:00
> 09020000-09020009 : QEMU0002:00 <-------- see it here (it's inclusive)
> 0a000000-0a0001ff : LNRO0005:00
> 0a000200-0a0003ff : LNRO0005:01
> 0a000400-0a0005ff : LNRO0005:02
> 0a000600-0a0007ff : LNRO0005:03
> 0a000800-0a0009ff : LNRO0005:04
> 0a000a00-0a000bff : LNRO0005:05
> 0a000c00-0a000dff : LNRO0005:06
> 0a000e00-0a000fff : LNRO0005:07
> 0a001000-0a0011ff : LNRO0005:08
> 0a001200-0a0013ff : LNRO0005:09
> 0a001400-0a0015ff : LNRO0005:0a
> 0a001600-0a0017ff : LNRO0005:0b
> 0a001800-0a0019ff : LNRO0005:0c
> 0a001a00-0a001bff : LNRO0005:0d
> 0a001c00-0a001dff : LNRO0005:0e
> 0a001e00-0a001fff : LNRO0005:0f
> 0a002000-0a0021ff : LNRO0005:10
> 0a002200-0a0023ff : LNRO0005:11
> 0a002400-0a0025ff : LNRO0005:12
> 0a002600-0a0027ff : LNRO0005:13
> 0a002800-0a0029ff : LNRO0005:14
> 0a002a00-0a002bff : LNRO0005:15
> 0a002c00-0a002dff : LNRO0005:16
> 0a002e00-0a002fff : LNRO0005:17
> 0a003000-0a0031ff : LNRO0005:18
> 0a003200-0a0033ff : LNRO0005:19
> 0a003400-0a0035ff : LNRO0005:1a
> 0a003600-0a0037ff : LNRO0005:1b
> 0a003800-0a0039ff : LNRO0005:1c
> 0a003a00-0a003bff : LNRO0005:1d
> 0a003c00-0a003dff : LNRO0005:1e
>   0a003c00-0a003dff : LNRO0005:1e
> 0a003e00-0a003fff : LNRO0005:1f
>   0a003e00-0a003fff : LNRO0005:1f
> 10000000-3efeffff : PCI Bus 0000:00
> 3f000000-3fffffff : PCI MMCONFIG 0000 [bus 00-0f]
> 40000000-13fffffff : System RAM
>   40080000-40c22523 : Kernel code
>   40d20000-414dffff : Kernel data
>   7fe00000-ffdfffff : Crash kernel
>   fff30000-fff8ffff : ACPI RAM
>   fffe0000-ffffffff : ACPI RAM
> 8000000000-8000000000 : PCI Bus 0000:00

Therefore

Tested-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

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

[Peter Maydell]

On 30 September 2015 at 11:21, Laszlo Ersek <lersek@redhat.com> wrote:
> However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
> run KVM guests), then I don't think he should bother. Booting just the
> UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
> for checking "/proc/iomem", he'd really need to boot into guest Linux,
> and *that* takes absolutely forever with TCG.

If it actually takes forever that's a bug of some sort I think.
TCG isn't all that snappy but it shouldn't take more than a few
minutes to boot and it should be at least usably responsive on
the command line once you get there. (Best not to try to boot
into a GUI, though.)

-- PMM

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

[Laszlo Ersek]

On 09/30/15 13:13, Peter Maydell wrote:
> On 30 September 2015 at 11:21, Laszlo Ersek <lersek@redhat.com> wrote:
>> However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
>> run KVM guests), then I don't think he should bother. Booting just the
>> UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
>> for checking "/proc/iomem", he'd really need to boot into guest Linux,
>> and *that* takes absolutely forever with TCG.
> 
> If it actually takes forever that's a bug of some sort I think.
> TCG isn't all that snappy but it shouldn't take more than a few
> minutes to boot and it should be at least usably responsive on
> the command line once you get there. (Best not to try to boot
> into a GUI, though.)

Yes, TCG is fast, relative to the feat it pulls off, but in absolute
terms, even those minutes to boot are annoying when you repeatedly test
something in the guest.

Here's a timing from my new company laptop (Thinkpad W541, i7-4810MQ CPU
@ 2.80GHz, running docked); QEMU built with --enable-debug:

(1) From starting the guest until the EFI stub of the kernel launches:
omitted (we're not timing the firmware, as it is not universally
necessary for testing)

(2) From launching the EFI stub until the login prompt appears on the
serial console: 3 minutes 46 seconds

(3) After logging in super fast, the time it takes to get a shell
prompt: 50 seconds

(4) The time it takes for background services to quiesce (= for QEMU to
stop spinning) while waiting idly at the shell prompt (because it makes
no sense to issue commands earlier): 1 minute 19 seconds

(5) Once the guest quiesces, shutting it down with "poweroff": 1 minute
36 seconds.

In total, 7 minutes 31 seconds for a test cycle (not counting the
firmware), without running any actual commands in the guest.

Again, it depends on the services that are enabled in systemd, but you
usually want to test with a guest OS that users normally run.

(I realize step (5) can be avoided if you have a qcow2 snapshot -- just
kill the guest when you're done, and revert the image to the snapshot
before next boot; hopefully new guest files are not important. I also
agree the first investment in a TCG guest should be heavily trimming its
services.)

So -- there's no bug, but TCG does not appear very suitable for testing
in guest userspace *now*.

... This is not to diminish TCG's general brilliance, and usefulness in
certain situations. I haven't forgotten that aarch64 emulation in TCG
was a long awaited godsend after the Foundation Model!

Still: Gabriel, how do you feel about buying a 96Boards EE (when it
becomes available)? :)

https://www.96boards.org/products/ee/
http://community.arm.com/people/jeffunderhill/status/9831
https://community.amd.com/community/amd-business/blog/2015/06/23/extending-arm-s-ecosystem-for-server-developers

Thanks
Laszlo

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

[Paolo Bonzini]

On 30/09/2015 02:18, Gabriel L. Somlo wrote:
> Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my
> brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for
> the confusion... :)

IIRC from the pvpanic trainwreck, Windows XP and 2003 always complain
even for 0x0B about a missing driver.

Paolo

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

[Gabriel L. Somlo]

On Wed, Sep 30, 2015 at 03:01:13PM +0200, Paolo Bonzini wrote:
> 
> 
> On 30/09/2015 02:18, Gabriel L. Somlo wrote:
> > Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my
> > brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for
> > the confusion... :)
> 
> IIRC from the pvpanic trainwreck, Windows XP and 2003 always complain
> even for 0x0B about a missing driver.

Don't have 2003, but dug up and started my old xp_sp3 image, and
Device Manager only complains when _STA defaults to 0x0f (i.e. when
I omit generating it in aml completely).

With _STA set to 0x0b (like in the latest version of the patch), the
fw_cfg device does not show up with a missing device complaint on xp.

Thanks,
--Gabriel

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

[Paolo Bonzini]

On 30/09/2015 16:16, Gabriel L. Somlo wrote:
>>> > > Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my
>>> > > brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for
>>> > > the confusion... :)
>> > 
>> > IIRC from the pvpanic trainwreck, Windows XP and 2003 always complain
>> > even for 0x0B about a missing driver.
> Don't have 2003, but dug up and started my old xp_sp3 image, and
> Device Manager only complains when _STA defaults to 0x0f (i.e. when
> I omit generating it in aml completely).
> 
> With _STA set to 0x0b (like in the latest version of the patch), the
> fw_cfg device does not show up with a missing device complaint on xp.

Great, thanks for testing.

Paolo

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

[Gerd Hoffmann]

  Hi,

> Here's a timing from my new company laptop (Thinkpad W541, i7-4810MQ CPU
> @ 2.80GHz, running docked); QEMU built with --enable-debug:

Almost the same here.

> (2) From launching the EFI stub until the login prompt appears on the
> serial console: 3 minutes 46 seconds

Less than half that time here.
Guess there is room to improve that for you ...

I'm running Fedora 22 as guest, minimal install, with stuff being added
as needed.

> Again, it depends on the services that are enabled in systemd, but you
> usually want to test with a guest OS that users normally run.

Depends.  Especially when mucking with firmware stuff this shouldn't be
that important, and gnome desktop install start alot more stuff, even
when booting into runlevel 3 only.

> So -- there's no bug, but TCG does not appear very suitable for testing
> in guest userspace *now*.

It's ok for testing now and then.  You don't want do actual development
with such a setup though.

cheers,
  Gerd

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

[Peter Maydell]

On 30 September 2015 at 13:22, Laszlo Ersek <lersek@redhat.com> wrote:
> (3) After logging in super fast, the time it takes to get a shell
> prompt: 50 seconds

That's bad. This should be more like 5 seconds at worst, it is for me.
This does depend a lot on how your guest is configured,
of course -- if you have a full fat distro with all the
desktop bells and whistles it's probably going to be slower.

> (4) The time it takes for background services to quiesce (= for QEMU to
> stop spinning) while waiting idly at the shell prompt (because it makes
> no sense to issue commands earlier): 1 minute 19 seconds

Not sure what these "background services" are. Sounds like your
guest image is a bit overconfigured...

thanks
-- PMM

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

[Gabriel L. Somlo]

On Wed, Sep 30, 2015 at 02:22:26PM +0200, Laszlo Ersek wrote:
> On 09/30/15 13:13, Peter Maydell wrote:
> > On 30 September 2015 at 11:21, Laszlo Ersek <lersek@redhat.com> wrote:
> >> However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
> >> run KVM guests), then I don't think he should bother. Booting just the
> >> UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
> >> for checking "/proc/iomem", he'd really need to boot into guest Linux,
> >> and *that* takes absolutely forever with TCG.
> > 
> > If it actually takes forever that's a bug of some sort I think.
> > TCG isn't all that snappy but it shouldn't take more than a few
> > minutes to boot and it should be at least usably responsive on
> > the command line once you get there. (Best not to try to boot
> > into a GUI, though.)
> 
> Yes, TCG is fast, relative to the feat it pulls off, but in absolute
> terms, even those minutes to boot are annoying when you repeatedly test
> something in the guest.
> 
> Here's a timing from my new company laptop (Thinkpad W541, i7-4810MQ CPU
> @ 2.80GHz, running docked); QEMU built with --enable-debug:
> 
> (1) From starting the guest until the EFI stub of the kernel launches:
> omitted (we're not timing the firmware, as it is not universally
> necessary for testing)
> 
> (2) From launching the EFI stub until the login prompt appears on the
> serial console: 3 minutes 46 seconds
> 
> (3) After logging in super fast, the time it takes to get a shell
> prompt: 50 seconds
> 
> (4) The time it takes for background services to quiesce (= for QEMU to
> stop spinning) while waiting idly at the shell prompt (because it makes
> no sense to issue commands earlier): 1 minute 19 seconds
> 
> (5) Once the guest quiesces, shutting it down with "poweroff": 1 minute
> 36 seconds.
> 
> In total, 7 minutes 31 seconds for a test cycle (not counting the
> firmware), without running any actual commands in the guest.
> 
> Again, it depends on the services that are enabled in systemd, but you
> usually want to test with a guest OS that users normally run.
> 
> (I realize step (5) can be avoided if you have a qcow2 snapshot -- just
> kill the guest when you're done, and revert the image to the snapshot
> before next boot; hopefully new guest files are not important. I also
> agree the first investment in a TCG guest should be heavily trimming its
> services.)
> 
> So -- there's no bug, but TCG does not appear very suitable for testing
> in guest userspace *now*.
> 
> ... This is not to diminish TCG's general brilliance, and usefulness in
> certain situations. I haven't forgotten that aarch64 emulation in TCG
> was a long awaited godsend after the Foundation Model!
> 
> Still: Gabriel, how do you feel about buying a 96Boards EE (when it
> becomes available)? :)
> 
> https://www.96boards.org/products/ee/
> http://community.arm.com/people/jeffunderhill/status/9831
> https://community.amd.com/community/amd-business/blog/2015/06/23/extending-arm-s-ecosystem-for-server-developers

Hmm, that looks like a sweet piece of hardware, and I can definitely
think of a few excuses I can bring up with my boss to order me one or
two :) Any idea how long until they're for sale ?

Also, Laszlo -- thanks for testing on arm! I'll fire off v5 with all
the feedback I collected soon, hopefully early tomorrow. I've been
trying to squeeze in work on the kernel driver between meetings today,
and on second look, I think I can immitate enough of pvpanic's setup
and tear-down to replace my module_init and module_exit routines, and
make this an acpi-only driver. We'll see how that works out, thanks
again for the pointers !

--Gabriel

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

[Igor Mammedov]

On Sun, 27 Sep 2015 17:29:00 -0400
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> Add a fw_cfg device node to the ACPI SSDT, on machine types
> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
>  hw/i386/pc_piix.c    |  1 +
>  hw/i386/pc_q35.c     |  1 +
>  include/hw/i386/pc.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ece2710 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>             PcPciInfo *pci, PcGuestInfo *guest_info)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>      uint32_t nr_mem = machine->ram_slots;
>      unsigned acpi_cpus = guest_info->apic_id_limit;
>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>      aml_append(scope, aml_name_decl("_S5", pkg));
>      aml_append(ssdt, scope);
>  
> +    if (!pcmc->acpi_no_fw_cfg_node) {
we don't really care about SSDT size changes since during
migration ACPI blobs will be migrated from source, so
machine compat bloat is excessive here. It would be better
to just remove it.


> +        scope = aml_scope("\\_SB");
> +        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();
> +        /* 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)
> +        );
could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
in any way?


> +        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");
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3ffb05f..7f5e5d9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>      m->alias = NULL;
>      m->is_default = 0;
>      pcmc->broken_reserved_end = true;
> +    pcmc->acpi_no_fw_cfg_node = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1b7d3b6..7180ca3 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>      pc_q35_2_5_machine_options(m);
>      m->alias = NULL;
>      pcmc->broken_reserved_end = true;
> +    pcmc->acpi_no_fw_cfg_node = true;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>  }
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 86007e3..6d0f1bd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -62,6 +62,7 @@ struct PCMachineClass {
>      bool broken_reserved_end;
>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>                                             DeviceState *dev);
> +    bool acpi_no_fw_cfg_node;
>  };
>  
>  #define TYPE_PC_MACHINE "generic-pc-machine"

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

[Laszlo Ersek]

On 10/01/15 09:02, Igor Mammedov wrote:
> On Sun, 27 Sep 2015 17:29:00 -0400
> "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> 
>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
>>  hw/i386/pc_piix.c    |  1 +
>>  hw/i386/pc_q35.c     |  1 +
>>  include/hw/i386/pc.h |  1 +
>>  4 files changed, 26 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 95e0c65..ece2710 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>             PcPciInfo *pci, PcGuestInfo *guest_info)
>>  {
>>      MachineState *machine = MACHINE(qdev_get_machine());
>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>      uint32_t nr_mem = machine->ram_slots;
>>      unsigned acpi_cpus = guest_info->apic_id_limit;
>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>>      aml_append(scope, aml_name_decl("_S5", pkg));
>>      aml_append(ssdt, scope);
>>  
>> +    if (!pcmc->acpi_no_fw_cfg_node) {
> we don't really care about SSDT size changes since during
> migration ACPI blobs will be migrated from source, so
> machine compat bloat is excessive here. It would be better
> to just remove it.

This was Eduardo's suggestion, if I recall correctly:

http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983

The idea being, if you move a guest from older QEMU to newer QEMU, but
keep the machine type (or more precisely, the full machine config, like
the domain XML) intact, the ACPI device node should not appear out of
the blue.

I'll let Gabriel answer your other question below. :)

Thanks
Laszlo


> 
> 
>> +        scope = aml_scope("\\_SB");
>> +        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();
>> +        /* 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)
>> +        );
> could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
> in any way?
> 
> 
>> +        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");
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 3ffb05f..7f5e5d9 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>>      m->alias = NULL;
>>      m->is_default = 0;
>>      pcmc->broken_reserved_end = true;
>> +    pcmc->acpi_no_fw_cfg_node = true;
>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>  }
>>  
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 1b7d3b6..7180ca3 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>>      pc_q35_2_5_machine_options(m);
>>      m->alias = NULL;
>>      pcmc->broken_reserved_end = true;
>> +    pcmc->acpi_no_fw_cfg_node = true;
>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>  }
>>  
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 86007e3..6d0f1bd 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -62,6 +62,7 @@ struct PCMachineClass {
>>      bool broken_reserved_end;
>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>                                             DeviceState *dev);
>> +    bool acpi_no_fw_cfg_node;
>>  };
>>  
>>  #define TYPE_PC_MACHINE "generic-pc-machine"
> 

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

[Igor Mammedov]

On Thu, 1 Oct 2015 10:27:15 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/01/15 09:02, Igor Mammedov wrote:
> > On Sun, 27 Sep 2015 17:29:00 -0400
> > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > 
> >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> >> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> >>  hw/i386/pc_piix.c    |  1 +
> >>  hw/i386/pc_q35.c     |  1 +
> >>  include/hw/i386/pc.h |  1 +
> >>  4 files changed, 26 insertions(+)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 95e0c65..ece2710 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>             PcPciInfo *pci, PcGuestInfo *guest_info)
> >>  {
> >>      MachineState *machine = MACHINE(qdev_get_machine());
> >> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >>      uint32_t nr_mem = machine->ram_slots;
> >>      unsigned acpi_cpus = guest_info->apic_id_limit;
> >>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>      aml_append(scope, aml_name_decl("_S5", pkg));
> >>      aml_append(ssdt, scope);
> >>  
> >> +    if (!pcmc->acpi_no_fw_cfg_node) {
> > we don't really care about SSDT size changes since during
> > migration ACPI blobs will be migrated from source, so
> > machine compat bloat is excessive here. It would be better
> > to just remove it.
> 
> This was Eduardo's suggestion, if I recall correctly:
> 
> http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> 
> The idea being, if you move a guest from older QEMU to newer QEMU, but
> keep the machine type (or more precisely, the full machine config, like
> the domain XML) intact, the ACPI device node should not appear out of
> the blue.
This ACPI device is an always used resource declaration regardless
of machine type so it makes sense to tell guest about used resource.

The only reason for machine compat code would be if guest suddenly
started to ask for a driver but as Gabriel showed with _STA(0xB)
it doesn't, so I'm trying to keep ACPI code machine compat agnostic
as much as possible.


PS:
is it me alone or email to qemu-devel arrives with huge delay (upto 2 days)?

> 
> I'll let Gabriel answer your other question below. :)
> 
> Thanks
> Laszlo
> 
> 
> > 
> > 
> >> +        scope = aml_scope("\\_SB");
> >> +        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();
> >> +        /* 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)
> >> +        );
> > could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
> > in any way?
> > 
> > 
> >> +        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");
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 3ffb05f..7f5e5d9 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> >>      m->alias = NULL;
> >>      m->is_default = 0;
> >>      pcmc->broken_reserved_end = true;
> >> +    pcmc->acpi_no_fw_cfg_node = true;
> >>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >>  }
> >>  
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index 1b7d3b6..7180ca3 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >>      pc_q35_2_5_machine_options(m);
> >>      m->alias = NULL;
> >>      pcmc->broken_reserved_end = true;
> >> +    pcmc->acpi_no_fw_cfg_node = true;
> >>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >>  }
> >>  
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 86007e3..6d0f1bd 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -62,6 +62,7 @@ struct PCMachineClass {
> >>      bool broken_reserved_end;
> >>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >>                                             DeviceState *dev);
> >> +    bool acpi_no_fw_cfg_node;
> >>  };
> >>  
> >>  #define TYPE_PC_MACHINE "generic-pc-machine"
> > 
> 

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

[Laszlo Ersek]

On 10/01/15 13:33, Igor Mammedov wrote:
> On Thu, 1 Oct 2015 10:27:15 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/01/15 09:02, Igor Mammedov wrote:
>>> On Sun, 27 Sep 2015 17:29:00 -0400
>>> "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
>>>
>>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>>>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
>>>>  hw/i386/pc_piix.c    |  1 +
>>>>  hw/i386/pc_q35.c     |  1 +
>>>>  include/hw/i386/pc.h |  1 +
>>>>  4 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>>> index 95e0c65..ece2710 100644
>>>> --- a/hw/i386/acpi-build.c
>>>> +++ b/hw/i386/acpi-build.c
>>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>>             PcPciInfo *pci, PcGuestInfo *guest_info)
>>>>  {
>>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>>>      uint32_t nr_mem = machine->ram_slots;
>>>>      unsigned acpi_cpus = guest_info->apic_id_limit;
>>>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
>>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>>      aml_append(scope, aml_name_decl("_S5", pkg));
>>>>      aml_append(ssdt, scope);
>>>>  
>>>> +    if (!pcmc->acpi_no_fw_cfg_node) {
>>> we don't really care about SSDT size changes since during
>>> migration ACPI blobs will be migrated from source, so
>>> machine compat bloat is excessive here. It would be better
>>> to just remove it.
>>
>> This was Eduardo's suggestion, if I recall correctly:
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
>>
>> The idea being, if you move a guest from older QEMU to newer QEMU, but
>> keep the machine type (or more precisely, the full machine config, like
>> the domain XML) intact, the ACPI device node should not appear out of
>> the blue.
> This ACPI device is an always used resource declaration regardless
> of machine type so it makes sense to tell guest about used resource.
> 
> The only reason for machine compat code would be if guest suddenly
> started to ask for a driver but as Gabriel showed with _STA(0xB)
> it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> as much as possible.

Fine by me, but I think Eduardo should agree (or disagree) as well,
because it was his point originally.

> 
> 
> PS:
> is it me alone or email to qemu-devel arrives with huge delay (upto 2 days)?

Several subscribers have reported the same, and I'm seeing delays myself.

Thanks
Laszlo

> 
>>
>> I'll let Gabriel answer your other question below. :)
>>
>> Thanks
>> Laszlo
>>
>>
>>>
>>>
>>>> +        scope = aml_scope("\\_SB");
>>>> +        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();
>>>> +        /* 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)
>>>> +        );
>>> could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
>>> in any way?
>>>
>>>
>>>> +        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");
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index 3ffb05f..7f5e5d9 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>>>>      m->alias = NULL;
>>>>      m->is_default = 0;
>>>>      pcmc->broken_reserved_end = true;
>>>> +    pcmc->acpi_no_fw_cfg_node = true;
>>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>>>  }
>>>>  
>>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>>> index 1b7d3b6..7180ca3 100644
>>>> --- a/hw/i386/pc_q35.c
>>>> +++ b/hw/i386/pc_q35.c
>>>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>>>>      pc_q35_2_5_machine_options(m);
>>>>      m->alias = NULL;
>>>>      pcmc->broken_reserved_end = true;
>>>> +    pcmc->acpi_no_fw_cfg_node = true;
>>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>>>  }
>>>>  
>>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>>> index 86007e3..6d0f1bd 100644
>>>> --- a/include/hw/i386/pc.h
>>>> +++ b/include/hw/i386/pc.h
>>>> @@ -62,6 +62,7 @@ struct PCMachineClass {
>>>>      bool broken_reserved_end;
>>>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>>>                                             DeviceState *dev);
>>>> +    bool acpi_no_fw_cfg_node;
>>>>  };
>>>>  
>>>>  #define TYPE_PC_MACHINE "generic-pc-machine"
>>>
>>
> 

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

[Gabriel L. Somlo]

On Wed, Sep 30, 2015 at 12:21:08PM +0200, Laszlo Ersek wrote:
> On 09/30/15 11:59, Ard Biesheuvel wrote:
> > On 29 September 2015 at 20:26, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> >> On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
> >>> On 09/27/15 23:29, 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 | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>>> index 1aaff1f..f314132 100644
> >>>> --- a/hw/arm/virt-acpi-build.c
> >>>> +++ b/hw/arm/virt-acpi-build.c
> >>>> @@ -110,6 +110,20 @@ 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")));
> >>>> +    /* 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;
> >>>> @@ -529,6 +543,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);
> >>>>
> >>>
> >>> Looks sane to me.
> >>>
> >>> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
> >>> /proc/iomem?) I can help with that, if you'd like.
> >>
> >> I have a F22 arm setup generated by virt-builder, which I start using:
> >>
> >> bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
> >>   -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
> >>   -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
> >>   -append "console=ttyAMA0 root=/dev/vda3 ro" \
> >>   -device virtio-blk-device,drive=hd0 \
> >>   -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
> >>   -device virtio-net-device,netdev=usernet \
> >>   -netdev user,id=usernet \
> >>   -monitor stdio
> >>
> > 
> > Note that you are booting 32-bit ARM here, which does not support ACPI nor UEFI.
> > (UEFI is work in progress, so you can try my ARM 32-bit UEFI tree if
> > you need to: https://git.linaro.org/people/ard.biesheuvel/linux-arm.git/shortlog/refs/heads/arm-efi-combined-v2)

I'm noticing that even on 32-bit arm there are ACPI tables generated and
inserted into fw_cfg, so is there any reason OTHER than lack of firmware
support for ACPI not being supported on 32-bit arm ?

> > You will need to create an arm64 / AArch64 setup and boot the virt
> > model using 'qemu-system-aarch64 -M virt -cpu cortex-a57 ...' instead.
> > In either case, as Laszlo pointed out, you need UEFI firmware in QEMU
> > as well.
> 
> I'm about to follow up with my test results, and I considered writing up
> a more or less complete guide for Gabriel to test this with an aarch64
> guest.
> 
> However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
> run KVM guests), then I don't think he should bother. Booting just the
> UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
> for checking "/proc/iomem", he'd really need to boot into guest Linux,
> and *that* takes absolutely forever with TCG.
> 
> (Dependent on your guest distro, of course; I have tested Fedora 21+ and
> RHELSA / RHEL-7 candidates thus far. I wouldn't recommend TCG for those.)
> 
> So, I'll just leave these links here for posterity (they could be
> somewhat outdated), and I offer to help with aarch64 guest testing in
> the future as well, if the patch series overlaps with my interests.
> 
> https://wiki.linaro.org/LEG/UEFIforQEMU
> https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU

Thanks to both of you for the pointers, I will probably invest some
time in getting set up with UEFI firmware for my arm guests at some
point, when the dust settles on all the other fun :)

Thanks,
--Gabriel

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

[Ard Biesheuvel]

On 1 October 2015 at 14:22, Gabriel L. Somlo <somlo@cmu.edu> wrote:
> On Wed, Sep 30, 2015 at 12:21:08PM +0200, Laszlo Ersek wrote:
>> On 09/30/15 11:59, Ard Biesheuvel wrote:
>> > On 29 September 2015 at 20:26, Gabriel L. Somlo <somlo@cmu.edu> wrote:
>> >> On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
>> >>> On 09/27/15 23:29, 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 | 15 +++++++++++++++
>> >>>>  1 file changed, 15 insertions(+)
>> >>>>
>> >>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> >>>> index 1aaff1f..f314132 100644
>> >>>> --- a/hw/arm/virt-acpi-build.c
>> >>>> +++ b/hw/arm/virt-acpi-build.c
>> >>>> @@ -110,6 +110,20 @@ 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")));
>> >>>> +    /* 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;
>> >>>> @@ -529,6 +543,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);
>> >>>>
>> >>>
>> >>> Looks sane to me.
>> >>>
>> >>> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
>> >>> /proc/iomem?) I can help with that, if you'd like.
>> >>
>> >> I have a F22 arm setup generated by virt-builder, which I start using:
>> >>
>> >> bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
>> >>   -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
>> >>   -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
>> >>   -append "console=ttyAMA0 root=/dev/vda3 ro" \
>> >>   -device virtio-blk-device,drive=hd0 \
>> >>   -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
>> >>   -device virtio-net-device,netdev=usernet \
>> >>   -netdev user,id=usernet \
>> >>   -monitor stdio
>> >>
>> >
>> > Note that you are booting 32-bit ARM here, which does not support ACPI nor UEFI.
>> > (UEFI is work in progress, so you can try my ARM 32-bit UEFI tree if
>> > you need to: https://git.linaro.org/people/ard.biesheuvel/linux-arm.git/shortlog/refs/heads/arm-efi-combined-v2)
>
> I'm noticing that even on 32-bit arm there are ACPI tables generated and
> inserted into fw_cfg, so is there any reason OTHER than lack of firmware
> support for ACPI not being supported on 32-bit arm ?
>

The 32-bit ARM/Linux kernel simply does not implement *any* kind of
ACPI support or awareness, so whether the firmware implements it or
not is irrelevant.

>> > You will need to create an arm64 / AArch64 setup and boot the virt
>> > model using 'qemu-system-aarch64 -M virt -cpu cortex-a57 ...' instead.
>> > In either case, as Laszlo pointed out, you need UEFI firmware in QEMU
>> > as well.
>>
>> I'm about to follow up with my test results, and I considered writing up
>> a more or less complete guide for Gabriel to test this with an aarch64
>> guest.
>>
>> However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
>> run KVM guests), then I don't think he should bother. Booting just the
>> UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
>> for checking "/proc/iomem", he'd really need to boot into guest Linux,
>> and *that* takes absolutely forever with TCG.
>>
>> (Dependent on your guest distro, of course; I have tested Fedora 21+ and
>> RHELSA / RHEL-7 candidates thus far. I wouldn't recommend TCG for those.)
>>
>> So, I'll just leave these links here for posterity (they could be
>> somewhat outdated), and I offer to help with aarch64 guest testing in
>> the future as well, if the patch series overlaps with my interests.
>>
>> https://wiki.linaro.org/LEG/UEFIforQEMU
>> https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU
>
> Thanks to both of you for the pointers, I will probably invest some
> time in getting set up with UEFI firmware for my arm guests at some
> point, when the dust settles on all the other fun :)
>
> Thanks,
> --Gabriel

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

[Laszlo Ersek]

On 10/01/15 14:22, Gabriel L. Somlo wrote:
> On Wed, Sep 30, 2015 at 12:21:08PM +0200, Laszlo Ersek wrote:
>> On 09/30/15 11:59, Ard Biesheuvel wrote:
>>> On 29 September 2015 at 20:26, Gabriel L. Somlo <somlo@cmu.edu> wrote:
>>>> On Tue, Sep 29, 2015 at 12:40:16PM +0200, Laszlo Ersek wrote:
>>>>> On 09/27/15 23:29, 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 | 15 +++++++++++++++
>>>>>>  1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>>>> index 1aaff1f..f314132 100644
>>>>>> --- a/hw/arm/virt-acpi-build.c
>>>>>> +++ b/hw/arm/virt-acpi-build.c
>>>>>> @@ -110,6 +110,20 @@ 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")));
>>>>>> +    /* 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;
>>>>>> @@ -529,6 +543,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);
>>>>>>
>>>>>
>>>>> Looks sane to me.
>>>>>
>>>>> Did you test this with an aarch64 Linux guest (acpidump -b; iasl -d; cat
>>>>> /proc/iomem?) I can help with that, if you'd like.
>>>>
>>>> I have a F22 arm setup generated by virt-builder, which I start using:
>>>>
>>>> bin/qemu-system-arm -M virt,accel=tcg -cpu cortex-a15 \
>>>>   -kernel ./ArmVirtBuilder/vmlinuz-4.0.4-301.fc22.armv7hl+lpae \
>>>>   -initrd ./ArmVirtBuilder/initramfs-4.0.4-301.fc22.armv7hl+lpae.img \
>>>>   -append "console=ttyAMA0 root=/dev/vda3 ro" \
>>>>   -device virtio-blk-device,drive=hd0 \
>>>>   -drive id=hd0,if=none,snapshot=on,file=./ArmVirtBuilder/fedora-22.img \
>>>>   -device virtio-net-device,netdev=usernet \
>>>>   -netdev user,id=usernet \
>>>>   -monitor stdio
>>>>
>>>
>>> Note that you are booting 32-bit ARM here, which does not support ACPI nor UEFI.
>>> (UEFI is work in progress, so you can try my ARM 32-bit UEFI tree if
>>> you need to: https://git.linaro.org/people/ard.biesheuvel/linux-arm.git/shortlog/refs/heads/arm-efi-combined-v2)
> 
> I'm noticing that even on 32-bit arm there are ACPI tables generated and
> inserted into fw_cfg, so is there any reason OTHER than lack of firmware
> support for ACPI not being supported on 32-bit arm ?

I think Ard was speaking about the guest kernel (his pointer certainly
looks like a kernel repo). With regard to firmware, edk2's
ArmVirtPkg/ArmVirtQemu.dsc builds just fine for 32-bit ARM guests.

So, it's not a QEMU or guest firmware problem; I think it's a guest
kernel problem.

Your question could be reformulated as "why has ACPI adoption been slow
in the ARM kernel", and my answer is "let's not go there". :)

> 
>>> You will need to create an arm64 / AArch64 setup and boot the virt
>>> model using 'qemu-system-aarch64 -M virt -cpu cortex-a57 ...' instead.
>>> In either case, as Laszlo pointed out, you need UEFI firmware in QEMU
>>> as well.
>>
>> I'm about to follow up with my test results, and I considered writing up
>> a more or less complete guide for Gabriel to test this with an aarch64
>> guest.
>>
>> However: if Gabriel has no access to actual aarch64 hardware (ie. cannot
>> run KVM guests), then I don't think he should bother. Booting just the
>> UEFI firmware on qemu-system-aarch64 with TCG acceleration is fine, but
>> for checking "/proc/iomem", he'd really need to boot into guest Linux,
>> and *that* takes absolutely forever with TCG.
>>
>> (Dependent on your guest distro, of course; I have tested Fedora 21+ and
>> RHELSA / RHEL-7 candidates thus far. I wouldn't recommend TCG for those.)
>>
>> So, I'll just leave these links here for posterity (they could be
>> somewhat outdated), and I offer to help with aarch64 guest testing in
>> the future as well, if the patch series overlaps with my interests.
>>
>> https://wiki.linaro.org/LEG/UEFIforQEMU
>> https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU
> 
> Thanks to both of you for the pointers, I will probably invest some
> time in getting set up with UEFI firmware for my arm guests at some
> point, when the dust settles on all the other fun :)

Re your other email... If you can convince people in your organization
that hold the strings of purses to buy aarch64 hardware on the org
level, you can buy it right now. It's pricier than the 96Boards EE stuff
is supposed to be, but it's available. Should you feel a burning desire
to test ACPI (or other) work in aarch64 KVM guests. :)

Thanks
Laszlo

> 
> Thanks,
> --Gabriel
> 

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

[Peter Maydell]

On 1 October 2015 at 13:35, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/01/15 14:22, Gabriel L. Somlo wrote:
>> I'm noticing that even on 32-bit arm there are ACPI tables generated and
>> inserted into fw_cfg, so is there any reason OTHER than lack of firmware
>> support for ACPI not being supported on 32-bit arm ?

This is mostly just that we didn't bother to suppress them for
32-bit boots, though they're useless there.

> I think Ard was speaking about the guest kernel (his pointer certainly
> looks like a kernel repo). With regard to firmware, edk2's
> ArmVirtPkg/ArmVirtQemu.dsc builds just fine for 32-bit ARM guests.
>
> So, it's not a QEMU or guest firmware problem; I think it's a guest
> kernel problem.

Only if you think ACPI support is a requirement. It's not for 32-bit;
there's no compelling reason to implement it there.

thanks
-- PMM

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

[Laszlo Ersek]

On 10/01/15 14:39, Peter Maydell wrote:
> On 1 October 2015 at 13:35, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/01/15 14:22, Gabriel L. Somlo wrote:
>>> I'm noticing that even on 32-bit arm there are ACPI tables generated and
>>> inserted into fw_cfg, so is there any reason OTHER than lack of firmware
>>> support for ACPI not being supported on 32-bit arm ?
> 
> This is mostly just that we didn't bother to suppress them for
> 32-bit boots, though they're useless there.
> 
>> I think Ard was speaking about the guest kernel (his pointer certainly
>> looks like a kernel repo). With regard to firmware, edk2's
>> ArmVirtPkg/ArmVirtQemu.dsc builds just fine for 32-bit ARM guests.
>>
>> So, it's not a QEMU or guest firmware problem; I think it's a guest
>> kernel problem.
> 
> Only if you think ACPI support is a requirement.

Oh, certainly. I was speaking with that working assumption (based on how
Gabriel asked his question); I wasn't trying to state that assumption as
a fact. Sorry about the misunderstanding.

Thanks
Laszlo

> It's not for 32-bit;
> there's no compelling reason to implement it there.



> 
> thanks
> -- PMM
> 

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

[Gabriel L. Somlo]

On Thu, Oct 01, 2015 at 01:52:59PM +0200, Laszlo Ersek wrote:
> On 10/01/15 13:33, Igor Mammedov wrote:
> > On Thu, 1 Oct 2015 10:27:15 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> >> On 10/01/15 09:02, Igor Mammedov wrote:
> >>> On Sun, 27 Sep 2015 17:29:00 -0400
> >>> "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> >>>
> >>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
> >>>> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> >>>>  hw/i386/pc_piix.c    |  1 +
> >>>>  hw/i386/pc_q35.c     |  1 +
> >>>>  include/hw/i386/pc.h |  1 +
> >>>>  4 files changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>>> index 95e0c65..ece2710 100644
> >>>> --- a/hw/i386/acpi-build.c
> >>>> +++ b/hw/i386/acpi-build.c
> >>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>>             PcPciInfo *pci, PcGuestInfo *guest_info)
> >>>>  {
> >>>>      MachineState *machine = MACHINE(qdev_get_machine());
> >>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >>>>      uint32_t nr_mem = machine->ram_slots;
> >>>>      unsigned acpi_cpus = guest_info->apic_id_limit;
> >>>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> >>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>>>      aml_append(scope, aml_name_decl("_S5", pkg));
> >>>>      aml_append(ssdt, scope);
> >>>>  
> >>>> +    if (!pcmc->acpi_no_fw_cfg_node) {
> >>> we don't really care about SSDT size changes since during
> >>> migration ACPI blobs will be migrated from source, so
> >>> machine compat bloat is excessive here. It would be better
> >>> to just remove it.
> >>
> >> This was Eduardo's suggestion, if I recall correctly:
> >>
> >> http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> >>
> >> The idea being, if you move a guest from older QEMU to newer QEMU, but
> >> keep the machine type (or more precisely, the full machine config, like
> >> the domain XML) intact, the ACPI device node should not appear out of
> >> the blue.
> > This ACPI device is an always used resource declaration regardless
> > of machine type so it makes sense to tell guest about used resource.
> > 
> > The only reason for machine compat code would be if guest suddenly
> > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > as much as possible.
> 
> Fine by me, but I think Eduardo should agree (or disagree) as well,
> because it was his point originally.

I have no problem taking the compat logic back out, if Eduardo agrees
that's an OK thing to do.

> >>>> +        scope = aml_scope("\\_SB");
> >>>> +        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();
> >>>> +        /* 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)
> >>>> +        );
> >>> could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
> >>> in any way?

On bot PIIX and Q35, there's no _CRS under PCI0 in the dsdt.
All I could find was

Scope (\_SB.PCI0)
{
    Name (_CRS, ...)
        IO (Decode16,
            0x0CF8,             // Range Minimum
            0x0CF8,             // Range Maximum
            0x01,               // Alignment
            0x08,               // Length
            )
    ...

So that does not overlap with fw_cfg.

In fact, I searched for all "IO (Decode..." occurrences, and nothing
in either DSDT or SSDT has any overlap with [ 0x510 .. 0x512 ].

Thanks,
 -- Gabriel


> >>>
> >>>
> >>>> +        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");
> >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>> index 3ffb05f..7f5e5d9 100644
> >>>> --- a/hw/i386/pc_piix.c
> >>>> +++ b/hw/i386/pc_piix.c
> >>>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> >>>>      m->alias = NULL;
> >>>>      m->is_default = 0;
> >>>>      pcmc->broken_reserved_end = true;
> >>>> +    pcmc->acpi_no_fw_cfg_node = true;
> >>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >>>>  }
> >>>>  
> >>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >>>> index 1b7d3b6..7180ca3 100644
> >>>> --- a/hw/i386/pc_q35.c
> >>>> +++ b/hw/i386/pc_q35.c
> >>>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >>>>      pc_q35_2_5_machine_options(m);
> >>>>      m->alias = NULL;
> >>>>      pcmc->broken_reserved_end = true;
> >>>> +    pcmc->acpi_no_fw_cfg_node = true;
> >>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >>>>  }
> >>>>  
> >>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >>>> index 86007e3..6d0f1bd 100644
> >>>> --- a/include/hw/i386/pc.h
> >>>> +++ b/include/hw/i386/pc.h
> >>>> @@ -62,6 +62,7 @@ struct PCMachineClass {
> >>>>      bool broken_reserved_end;
> >>>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >>>>                                             DeviceState *dev);
> >>>> +    bool acpi_no_fw_cfg_node;
> >>>>  };
> >>>>  
> >>>>  #define TYPE_PC_MACHINE "generic-pc-machine"
> >>>
> >>
> > 
> 

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On 10/01/2015 05:52 AM, Laszlo Ersek wrote:

>> PS:
>> is it me alone or email to qemu-devel arrives with huge delay (upto 2 days)?
> 
> Several subscribers have reported the same, and I'm seeing delays myself.

gnu.org servers (hosts of our nongnu.org list) got hit with a huge spam
bomb last weekend; in the process of dealing with the system overload
from that, LOTS of lists had traffic being held for multiple hours.  It
looks like the mail is a bit snappier today, but still plowing through
the backlog of mails that were pending retries when the systems weren't
as loaded.

I wish there were a nice URL to post to an outage report from the
fsfadmin folks, but can't find one.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Gabriel L. Somlo]

On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> On Thu, 1 Oct 2015 10:27:15 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
> > On 10/01/15 09:02, Igor Mammedov wrote:
> > > On Sun, 27 Sep 2015 17:29:00 -0400
> > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > 
> > >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> > >> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> > >>  hw/i386/pc_piix.c    |  1 +
> > >>  hw/i386/pc_q35.c     |  1 +
> > >>  include/hw/i386/pc.h |  1 +
> > >>  4 files changed, 26 insertions(+)
> > >>
> > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > >> index 95e0c65..ece2710 100644
> > >> --- a/hw/i386/acpi-build.c
> > >> +++ b/hw/i386/acpi-build.c
> > >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >>             PcPciInfo *pci, PcGuestInfo *guest_info)
> > >>  {
> > >>      MachineState *machine = MACHINE(qdev_get_machine());
> > >> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > >>      uint32_t nr_mem = machine->ram_slots;
> > >>      unsigned acpi_cpus = guest_info->apic_id_limit;
> > >>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> > >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >>      aml_append(scope, aml_name_decl("_S5", pkg));
> > >>      aml_append(ssdt, scope);
> > >>  
> > >> +    if (!pcmc->acpi_no_fw_cfg_node) {
> > > we don't really care about SSDT size changes since during
> > > migration ACPI blobs will be migrated from source, so
> > > machine compat bloat is excessive here. It would be better
> > > to just remove it.
> > 
> > This was Eduardo's suggestion, if I recall correctly:
> > 
> > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > 
> > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > keep the machine type (or more precisely, the full machine config, like
> > the domain XML) intact, the ACPI device node should not appear out of
> > the blue.
> This ACPI device is an always used resource declaration regardless
> of machine type so it makes sense to tell guest about used resource.
> 
> The only reason for machine compat code would be if guest suddenly
> started to ask for a driver but as Gabriel showed with _STA(0xB)
> it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> as much as possible.

Eduardo, what do you think about this ? I'm hoping to do a v5 over the
weekend or early next week, and which way this should go is one of the
couple of decisions that I still have open.

Thanks much,
--Gabriel

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

[Eduardo Habkost]

On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> > On Thu, 1 Oct 2015 10:27:15 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> > > On 10/01/15 09:02, Igor Mammedov wrote:
> > > > On Sun, 27 Sep 2015 17:29:00 -0400
> > > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > > 
> > > >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> > > >> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> > > >>  hw/i386/pc_piix.c    |  1 +
> > > >>  hw/i386/pc_q35.c     |  1 +
> > > >>  include/hw/i386/pc.h |  1 +
> > > >>  4 files changed, 26 insertions(+)
> > > >>
> > > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > >> index 95e0c65..ece2710 100644
> > > >> --- a/hw/i386/acpi-build.c
> > > >> +++ b/hw/i386/acpi-build.c
> > > >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >>             PcPciInfo *pci, PcGuestInfo *guest_info)
> > > >>  {
> > > >>      MachineState *machine = MACHINE(qdev_get_machine());
> > > >> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > > >>      uint32_t nr_mem = machine->ram_slots;
> > > >>      unsigned acpi_cpus = guest_info->apic_id_limit;
> > > >>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> > > >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >>      aml_append(scope, aml_name_decl("_S5", pkg));
> > > >>      aml_append(ssdt, scope);
> > > >>  
> > > >> +    if (!pcmc->acpi_no_fw_cfg_node) {
> > > > we don't really care about SSDT size changes since during
> > > > migration ACPI blobs will be migrated from source, so
> > > > machine compat bloat is excessive here. It would be better
> > > > to just remove it.

What about non-live migration?

> > > 
> > > This was Eduardo's suggestion, if I recall correctly:
> > > 
> > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > 
> > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > keep the machine type (or more precisely, the full machine config, like
> > > the domain XML) intact, the ACPI device node should not appear out of
> > > the blue.
> > This ACPI device is an always used resource declaration regardless
> > of machine type so it makes sense to tell guest about used resource.
> > 
> > The only reason for machine compat code would be if guest suddenly
> > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > as much as possible.
> 
> Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> weekend or early next week, and which way this should go is one of the
> couple of decisions that I still have open.

The general rule is that anything that's visible to the guest shouldn't
change on a QEMU upgrade if the machine-type is kept the same. If we
want to avoid the compat code, we need careful testing to ensure this
won't make any guest OS do something unexpected.

One of the things that may break if guest-visible bits of the machine
change is Windows license activation, but the rules Windows use to
trigger reactivation aren't very clear.

-- 
Eduardo

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote:
> One of the things that may break if guest-visible bits of the machine
> change is Windows license activation, but the rules Windows use to
> trigger reactivation aren't very clear.

They are easy to find on the internet.

> -- 
> Eduardo

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

[Eduardo Habkost]

On Wed, Oct 14, 2015 at 12:18:10AM +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote:
> > One of the things that may break if guest-visible bits of the machine
> > change is Windows license activation, but the rules Windows use to
> > trigger reactivation aren't very clear.
> 
> They are easy to find on the internet.

I couldn't find them[1]. If you have a pointer to a clear description of
the rules for all Windows versions, I would love to see it.

[1] All I have found were things like:

  "Do I need to activate Windows after making a hardware change?
  Maybe. [...]"
  http://windows.microsoft.com/en-us/windows/activating-windows-faq#1TC=windows-7

  "Microsoft is characteristically shy about discussing the details of
  activation. [...]"
  http://www.zdnet.com/article/microsoft-quietly-rewrites-its-activation-rules-for-windows-10/

  "What triggers the need to reactivate Windows? As intended, each
  hardware component gets a relative weight, and from that WGA determines
  whether your copy of Windows 7 needs reactivation. The weight and the
  number of changes is apparently a guarded secret. If you upgrade too
  much at once, WAT decides that your PC is new, and things can get messy.
  The actual algorithm that Microsoft uses is not disclosed, [...]"
  http://searchitchannel.techtarget.com/feature/How-Windows-7-hardware-upgrades-affect-licensing


-- 
Eduardo

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 07:43:00PM -0300, Eduardo Habkost wrote:
> On Wed, Oct 14, 2015 at 12:18:10AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote:
> > > One of the things that may break if guest-visible bits of the machine
> > > change is Windows license activation, but the rules Windows use to
> > > trigger reactivation aren't very clear.
> > 
> > They are easy to find on the internet.
> 
> I couldn't find them[1]. If you have a pointer to a clear description of
> the rules for all Windows versions, I would love to see it.


The detailed info is not hard to find:
http://en.wikipedia.org/wiki/Microsoft_Product_Activation
links to:
http://technet.microsoft.com/en-us/library/bb457054.aspx


1
Display Adapter
00010 (5)
2
SCSI Adapter
00011 (5)
3
IDE Adapter
0011 (4)
4
Network Adapter MAC Address
1001011000 (10)
5
RAM Amount Range (i.e. 0-64mb, 64-128mb, etc)
101 (3)
6
Processor Type
011 (3)
7
Processor Serial Number
000000 (6)
8
Hard Drive Device
1101100 (7)
9
Hard Drive Volume Serial Number
1001000001 (10)
10
CD—ROM / CD-RW / DVD-ROM
010111 (6)
-
"Dockable"
0 (1)
-
Hardware Hash version (version of algorithm used)
001 (3)

Also:
	Microsoft defines "substantially different" hardware differently for PCs
	that are configured to be dockable. Additionally, the network adapter is
	given a superior "weighting." If the PC is not dockable and a network
	adapter exists and is not changed, 6 or more of the other above values
	would have to change before reactivation was required. If a network
	adapter existed but is changed or never existed at all, 4 or more
	changes (including the changed network adapter if it previously existed)
	will result in a requirement to reactivate.

So it's also not hard to try it out with kvm.  Apply a patch, change
3 other things: nic mac, ram size (by a factor of >2), cpu type and boot.



> >
> >_______________________________________________
> >Xen-devel mailing list
> >Xen-devel@lists.xen.org
> >http://lists.xen.org/xen-devel
> >
> >-----
> >No virus found in this message.
> >Checked by AVG - www.avg.com
> >Version: 2014.0.4592 / Virus Database: 3986/7769 - Release Date: 06/30/14
> >
> 
> 
> -- 
> Ross Philipson

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

[Igor Mammedov]

On Tue, 13 Oct 2015 16:10:03 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> > On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> > > On Thu, 1 Oct 2015 10:27:15 +0200
> > > Laszlo Ersek <lersek@redhat.com> wrote:
> > > 
> > > > On 10/01/15 09:02, Igor Mammedov wrote:
> > > > > On Sun, 27 Sep 2015 17:29:00 -0400
> > > > > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > > > > 
> > > > >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> > > > >> pc-*-2.5 and up. 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 | 23 +++++++++++++++++++++++
> > > > >>  hw/i386/pc_piix.c    |  1 +
> > > > >>  hw/i386/pc_q35.c     |  1 +
> > > > >>  include/hw/i386/pc.h |  1 +
> > > > >>  4 files changed, 26 insertions(+)
> > > > >>
> > > > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > >> index 95e0c65..ece2710 100644
> > > > >> --- a/hw/i386/acpi-build.c
> > > > >> +++ b/hw/i386/acpi-build.c
> > > > >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > > >>             PcPciInfo *pci, PcGuestInfo *guest_info)
> > > > >>  {
> > > > >>      MachineState *machine = MACHINE(qdev_get_machine());
> > > > >> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > > > >>      uint32_t nr_mem = machine->ram_slots;
> > > > >>      unsigned acpi_cpus = guest_info->apic_id_limit;
> > > > >>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> > > > >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > > >>      aml_append(scope, aml_name_decl("_S5", pkg));
> > > > >>      aml_append(ssdt, scope);
> > > > >>  
> > > > >> +    if (!pcmc->acpi_no_fw_cfg_node) {
> > > > > we don't really care about SSDT size changes since during
> > > > > migration ACPI blobs will be migrated from source, so
> > > > > machine compat bloat is excessive here. It would be better
> > > > > to just remove it.
> 
> What about non-live migration?
I don't see any issues here, it should just work, since usually
original SSDT from source is used (copied as part of migrated ram).

> > > > 
> > > > This was Eduardo's suggestion, if I recall correctly:
> > > > 
> > > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > > 
> > > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > > keep the machine type (or more precisely, the full machine config, like
> > > > the domain XML) intact, the ACPI device node should not appear out of
> > > > the blue.
> > > This ACPI device is an always used resource declaration regardless
> > > of machine type so it makes sense to tell guest about used resource.
> > > 
> > > The only reason for machine compat code would be if guest suddenly
> > > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > > as much as possible.
> > 
> > Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> > weekend or early next week, and which way this should go is one of the
> > couple of decisions that I still have open.
> 
> The general rule is that anything that's visible to the guest shouldn't
> change on a QEMU upgrade if the machine-type is kept the same. If we
> want to avoid the compat code, we need careful testing to ensure this
> won't make any guest OS do something unexpected.
> 
> One of the things that may break if guest-visible bits of the machine
> change is Windows license activation, but the rules Windows use to
> trigger reactivation aren't very clear.
Practice shows that changing ACPI tables doesn't affect MS Activation so far.

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

[Eduardo Habkost]

On Wed, Oct 14, 2015 at 08:06:36AM +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 07:43:00PM -0300, Eduardo Habkost wrote:
> > On Wed, Oct 14, 2015 at 12:18:10AM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote:
> > > > One of the things that may break if guest-visible bits of the machine
> > > > change is Windows license activation, but the rules Windows use to
> > > > trigger reactivation aren't very clear.
> > > 
> > > They are easy to find on the internet.
> > 
> > I couldn't find them[1]. If you have a pointer to a clear description of
> > the rules for all Windows versions, I would love to see it.
> 
> 
> The detailed info is not hard to find:
> http://en.wikipedia.org/wiki/Microsoft_Product_Activation
> links to:
> http://technet.microsoft.com/en-us/library/bb457054.aspx

Thanks. Do you know if there's public information for other Windows
versions, or only Windows XP?

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Oct 14, 2015 at 10:45:00AM +0200, Igor Mammedov wrote:
> On Tue, 13 Oct 2015 16:10:03 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> > > On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
[...]
> > > > > >> +    if (!pcmc->acpi_no_fw_cfg_node) {
> > > > > > we don't really care about SSDT size changes since during
> > > > > > migration ACPI blobs will be migrated from source, so
> > > > > > machine compat bloat is excessive here. It would be better
> > > > > > to just remove it.
> > 
> > What about non-live migration?
> I don't see any issues here, it should just work, since usually
> original SSDT from source is used (copied as part of migrated ram).

I mean: shutdown, followed by QEMU upgrade, followed by reboot.

> > > > > 
> > > > > This was Eduardo's suggestion, if I recall correctly:
> > > > > 
> > > > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > > > 
> > > > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > > > keep the machine type (or more precisely, the full machine config, like
> > > > > the domain XML) intact, the ACPI device node should not appear out of
> > > > > the blue.
> > > > This ACPI device is an always used resource declaration regardless
> > > > of machine type so it makes sense to tell guest about used resource.
> > > > 
> > > > The only reason for machine compat code would be if guest suddenly
> > > > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > > > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > > > as much as possible.
> > > 
> > > Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> > > weekend or early next week, and which way this should go is one of the
> > > couple of decisions that I still have open.
> > 
> > The general rule is that anything that's visible to the guest shouldn't
> > change on a QEMU upgrade if the machine-type is kept the same. If we
> > want to avoid the compat code, we need careful testing to ensure this
> > won't make any guest OS do something unexpected.
> > 
> > One of the things that may break if guest-visible bits of the machine
> > change is Windows license activation, but the rules Windows use to
> > trigger reactivation aren't very clear.
> Practice shows that changing ACPI tables doesn't affect MS Activation so far.

I still see any guest-visible change in a machine-type as a bug. But at
least it seems to be a minor bug.

I just noticed we have made ACPI table changes in the past without any
compat code, so that's not something new. So I won't mind too much if
you really want to avoid acpi_no_fw_cfg_node.

-- 
Eduardo

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

[Igor Mammedov]

On Wed, 14 Oct 2015 13:47:32 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Oct 14, 2015 at 10:45:00AM +0200, Igor Mammedov wrote:
> > On Tue, 13 Oct 2015 16:10:03 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> > > > On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> [...]
> > > > > > >> +    if (!pcmc->acpi_no_fw_cfg_node) {
> > > > > > > we don't really care about SSDT size changes since during
> > > > > > > migration ACPI blobs will be migrated from source, so
> > > > > > > machine compat bloat is excessive here. It would be better
> > > > > > > to just remove it.
> > > 
> > > What about non-live migration?
> > I don't see any issues here, it should just work, since usually
> > original SSDT from source is used (copied as part of migrated ram).
> 
> I mean: shutdown, followed by QEMU upgrade, followed by reboot.
Then guest will get new ACPI tables as well possibly updated BIOS
regardless of machine type. (see below)

> 
> > > > > > 
> > > > > > This was Eduardo's suggestion, if I recall correctly:
> > > > > > 
> > > > > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > > > > 
> > > > > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > > > > keep the machine type (or more precisely, the full machine config, like
> > > > > > the domain XML) intact, the ACPI device node should not appear out of
> > > > > > the blue.
> > > > > This ACPI device is an always used resource declaration regardless
> > > > > of machine type so it makes sense to tell guest about used resource.
> > > > > 
> > > > > The only reason for machine compat code would be if guest suddenly
> > > > > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > > > > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > > > > as much as possible.
> > > > 
> > > > Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> > > > weekend or early next week, and which way this should go is one of the
> > > > couple of decisions that I still have open.
> > > 
> > > The general rule is that anything that's visible to the guest shouldn't
> > > change on a QEMU upgrade if the machine-type is kept the same. If we
> > > want to avoid the compat code, we need careful testing to ensure this
> > > won't make any guest OS do something unexpected.
> > > 
> > > One of the things that may break if guest-visible bits of the machine
> > > change is Windows license activation, but the rules Windows use to
> > > trigger reactivation aren't very clear.
> > Practice shows that changing ACPI tables doesn't affect MS Activation so far.
> 
> I still see any guest-visible change in a machine-type as a bug. But at
> least it seems to be a minor bug.
> 
> I just noticed we have made ACPI table changes in the past without any
> compat code, so that's not something new. So I won't mind too much if
> you really want to avoid acpi_no_fw_cfg_node.
That's because ACPI tables are considered to be part (they are in QEMU for
convenience purpose only) of firmware and as expected if one run's
the same machine type with updated BIOS, one'll get updated tables.

So I prefer to avoid adding compat cruft in ACPI code unless
we have to it, which is not case with this patch.