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

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

[Gabriel L. Somlo]

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...

Thanks much,
  --Gabriel

>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  | 13 +++++++++++++
 hw/i386/acpi-build.c      | 21 +++++++++++++++++++++
 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, 58 insertions(+), 6 deletions(-)

-- 
2.4.3

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

[Qemu-devel] [PATCH v3 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 | 21 +++++++++++++++++++++
 hw/i386/pc_piix.c    |  1 +
 hw/i386/pc_q35.c     |  1 +
 include/hw/i386/pc.h |  1 +
 4 files changed, 24 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..e3d097b 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,26 @@ 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")));
+
+        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 3f925b2..003c28a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -467,6 +467,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_machine_options(m);
     pcmc->broken_reserved_end = true;
+    pcmc->acpi_no_fw_cfg_node = true;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->alias = "pc";
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 11601ab..c4656f1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -371,6 +371,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_machine_options(m);
     pcmc->broken_reserved_end = true;
+    pcmc->acpi_no_fw_cfg_node = true;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
     m->no_floppy = 1;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index fadcaa4..2955bd9 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 v3 4/5] acpi: arm: add fw_cfg device node to dsdt

[Gabriel L. Somlo]

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

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

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

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

[Gabriel L. Somlo]

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

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

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

[Michael S. Tsirkin]

On Thu, Sep 17, 2015 at 10:56:29AM -0400, Gabriel L. Somlo wrote:
> 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...
> 
> Thanks much,
>   --Gabriel


What I'm still missing is the motivation for the change.

Went through old threads, still can't find it.

> >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  | 13 +++++++++++++
>  hw/i386/acpi-build.c      | 21 +++++++++++++++++++++
>  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, 58 insertions(+), 6 deletions(-)
> 
> -- 
> 2.4.3

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

[Gabriel L. Somlo]

On Thu, Sep 17, 2015 at 11:30:59PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 17, 2015 at 10:56:29AM -0400, Gabriel L. Somlo wrote:
> > 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...
> 
> What I'm still missing is the motivation for the change.
> 
> Went through old threads, still can't find it.

I've been working on a sysfs driver to allow viewing fw_cfg file
metadata (size, name, etc) and content (e.g. 'raw') in something
like /sys/firmware/qemu-fw-cfg/by_key/... (similar to how e.g.
smbios tables can be accessed via /sys/firmware/dmi/entries/...).

Motivation is that since we can insert user-defined blobs via the qemu
command line, wouldn't it be nice to be able to easily retrieve them
from the guest, and what's easier than "cp /sys/firmware/.../raw ..." ?

One of the main requirements I got there was to avoid probing fw_cfg
registers during initialization of the sysfs driver, and instead use
device tree on arm (and acpi on i386) to first figure out whether
fw_cfg is even expected to be there.

There's a DT entry for fw_cfg on arm, but no acpi entry for fw_cfg on
x86, so I decided why not add one ? It's a device, it occupies a mmio
or port-io region, why not tell the guest about it via ACPI ?

That's about it, in a nutshell. Thanks for any further thoughts!

--Gabriel



> 
> > >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  | 13 +++++++++++++
> >  hw/i386/acpi-build.c      | 21 +++++++++++++++++++++
> >  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, 58 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.4.3

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

[Gerd Hoffmann]

On Do, 2015-09-17 at 17:01 -0400, Gabriel L. Somlo wrote:
> > What I'm still missing is the motivation for the change.
> > 
> > Went through old threads, still can't find it.
> 
> I've been working on a sysfs driver to allow viewing fw_cfg file
> metadata (size, name, etc) and content (e.g. 'raw') in something
> like /sys/firmware/qemu-fw-cfg/by_key/... (similar to how e.g.
> smbios tables can be accessed via /sys/firmware/dmi/entries/...).
> 
> Motivation is that since we can insert user-defined blobs via the qemu
> command line, wouldn't it be nice to be able to easily retrieve them
> from the guest, and what's easier than "cp /sys/firmware/.../raw ..." ?
> 
> One of the main requirements I got there was to avoid probing fw_cfg
> registers during initialization of the sysfs driver, and instead use
> device tree on arm (and acpi on i386) to first figure out whether
> fw_cfg is even expected to be there.
> 
> There's a DT entry for fw_cfg on arm, but no acpi entry for fw_cfg on
> x86, so I decided why not add one ? It's a device, it occupies a mmio
> or port-io region, why not tell the guest about it via ACPI ?
> 
> That's about it, in a nutshell. Thanks for any further thoughts!

I think in general it would be good to notify the guest OS that there is
something at port 0x510 (i.e. it is not free), even if the guest has no
fw_cfg driver.

cheers,
  Gerd

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

[Igor Mammedov]

On Thu, 17 Sep 2015 10:56:29 -0400
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> 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...
> 
> Thanks much,
>   --Gabriel
> 
> >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.
ACPI patches 2-3 look fine to me
although there is one but, it will make Windows complain about
unknown device and prompt user to install driver.

> >>
> >> I made up some names - "FWCF" for the node name, and "FWCF0001"
> >> for _HID; no idea whether that's appropriate, or how else I should
> >> figure out what to use instead...
> >>
> >> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> >> output of "info qtree". Again, if that's wrong, please point me in
> >> the right direction.
> >>
> >> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
> >> I noticed none of the other DSDT entries contain a _STA field, wondering
> >> why it would (not) make sense to include that, same as on the PC.
> 
> Gabriel L. Somlo (5):
>   fw_cfg: expose control register size in fw_cfg.h
>   pc: fw_cfg: move ioport base constant to pc.h
>   acpi: pc: add fw_cfg device node to ssdt
>   acpi: arm: add fw_cfg device node to dsdt
>   fw_cfg: document ACPI device node information
> 
>  docs/specs/fw_cfg.txt     |  9 +++++++++
>  hw/arm/virt-acpi-build.c  | 13 +++++++++++++
>  hw/i386/acpi-build.c      | 21 +++++++++++++++++++++
>  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, 58 insertions(+), 6 deletions(-)
> 

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

[Gabriel L. Somlo]

On Wed, Sep 23, 2015 at 11:50:05AM +0200, Igor Mammedov wrote:
> On Thu, 17 Sep 2015 10:56:29 -0400
> "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> 
> > 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...
> > 
> > Thanks much,
> >   --Gabriel
> > 
> > >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.
> ACPI patches 2-3 look fine to me
> although there is one but, it will make Windows complain about
> unknown device and prompt user to install driver.

I wonder if the implicit _STA has bit 2 (visible to u/i) implicitly
enabled (0x0F vs. 0x0B)... I'll try to reproduce this myself, and see
if adding (back) the explicit _STA value helps.

Thanks!
--Gabriel

> 
> > >>
> > >> I made up some names - "FWCF" for the node name, and "FWCF0001"
> > >> for _HID; no idea whether that's appropriate, or how else I should
> > >> figure out what to use instead...
> > >>
> > >> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> > >> output of "info qtree". Again, if that's wrong, please point me in
> > >> the right direction.
> > >>
> > >> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
> > >> I noticed none of the other DSDT entries contain a _STA field, wondering
> > >> why it would (not) make sense to include that, same as on the PC.
> > 
> > Gabriel L. Somlo (5):
> >   fw_cfg: expose control register size in fw_cfg.h
> >   pc: fw_cfg: move ioport base constant to pc.h
> >   acpi: pc: add fw_cfg device node to ssdt
> >   acpi: arm: add fw_cfg device node to dsdt
> >   fw_cfg: document ACPI device node information
> > 
> >  docs/specs/fw_cfg.txt     |  9 +++++++++
> >  hw/arm/virt-acpi-build.c  | 13 +++++++++++++
> >  hw/i386/acpi-build.c      | 21 +++++++++++++++++++++
> >  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, 58 insertions(+), 6 deletions(-)
> > 
>