All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm
@ 2015-11-14  2:57 Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 1/5] fw_cfg: expose control register size in fw_cfg.h Gabriel L. Somlo
                   ` (6 more replies)
  0 siblings, 7 replies; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-14  2:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

New since v4:

	- rebased on top of Marc's DMA series
	- drop machine compat dependency for insertion into x86/ssdt
	  (patch 3/5), following agreement between Igor and Eduardo
	- [mm]io register range now covers DMA register as well, if
	  available.
	- s/bios/firmware in doc file updates

Thanks,
  --Gabriel

>New since v3:
>
>	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
>	  inserting fw_cfg acpi node only for machines >= 2.5.
>
>	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
>	  off to avoid Windows complaining -- thanks Igor for catching that!)
>
>If there's any other feedback besides questions regarding the
>appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
>
>>New since v2:
>>
>>	- pc/i386 node in ssdt only on machine types *newer* than 2.4
>>	  (as suggested by Eduardo)
>>
>>I appreciate any further comments and reviews. Hopefully we can make
>>this palatable for upstream, modulo the lingering concerns about whether
>>"QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
>>sorted out with the kernel crew...
>>
>>>New since v1:
>>>
>>>	- expose control register size (suggested by Marc Marí)
>>>
>>>	- leaving out _UID and _STA fields (thanks Shannon & Igor)
>>>
>>>	- using "QEMU0002" as the value of _HID (thanks Michael)
>>>
>>>	- added documentation blurb to docs/specs/fw_cfg.txt
>>>	  (mainly to record usage of the "QEMU0002" string with fw_cfg).
>>>
>>>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
>>>> DSDT (on arm).
>>>>
>>>> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
>>>> 	  define from pc.c to pc.h, so that it could be used from
>>>> 	  acpi-build.c in patch 2/3.
>>>> 
>>>> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
>>>> 
>>>> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
>>>>
>>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
>>>> for _HID; no idea whether that's appropriate, or how else I should
>>>> figure out what to use instead...
>>>>
>>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
>>>> output of "info qtree". Again, if that's wrong, please point me in
>>>> the right direction.
>>>>
>>>> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
>>>> I noticed none of the other DSDT entries contain a _STA field, wondering
>>>> why it would (not) make sense to include that, same as on the PC.
Gabriel L. Somlo (5):
  fw_cfg: expose control register size in fw_cfg.h
  pc: fw_cfg: move ioport base constant to pc.h
  acpi: pc: add fw_cfg device node to 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      | 29 +++++++++++++++++++++++++++++
 hw/i386/pc.c              |  5 ++---
 hw/nvram/fw_cfg.c         |  4 +++-
 include/hw/i386/pc.h      |  2 ++
 include/hw/nvram/fw_cfg.h |  3 +++
 7 files changed, 63 insertions(+), 4 deletions(-)

-- 
2.4.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v5 1/5] fw_cfg: expose control register size in fw_cfg.h
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
@ 2015-11-14  2:57 ` Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 2/5] pc: fw_cfg: move ioport base constant to pc.h Gabriel L. Somlo
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-14  2:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

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

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index a1d650d..06a4ff0 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -31,7 +31,6 @@
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 
-#define FW_CFG_CTL_SIZE 2
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
@@ -881,6 +880,9 @@ static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
     FWCfgIoState *s = FW_CFG_IO(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
+    /* when using port i/o, the 8-bit data register ALWAYS overlaps
+     * with half of the 16-bit control register. Hence, the total size
+     * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                           FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
     sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index 664eaf6..2667ca9 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -46,6 +46,9 @@
 
 #define FW_CFG_INVALID          0xffff
 
+/* width in bytes of fw_cfg control register */
+#define FW_CFG_CTL_SIZE         0x02
+
 #define FW_CFG_MAX_FILE_PATH    56
 
 #ifndef NO_QEMU_PROTOS
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v5 2/5] pc: fw_cfg: move ioport base constant to pc.h
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 1/5] fw_cfg: expose control register size in fw_cfg.h Gabriel L. Somlo
@ 2015-11-14  2:57 ` Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 3/5] acpi: pc: add fw_cfg device node to ssdt Gabriel L. Somlo
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-14  2:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

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

Cc: Marc Marí <markmb@redhat.com>
Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 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 5e20e07..5763d20 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -85,7 +85,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)
@@ -759,7 +758,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
-    fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
+    fw_cfg = fw_cfg_init_io_dma(FW_CFG_IO_BASE, FW_CFG_IO_BASE + 4, as);
 
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
@@ -1278,7 +1277,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 4bbc0ff..8f89f07 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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v5 3/5] acpi: pc: add fw_cfg device node to ssdt
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 1/5] fw_cfg: expose control register size in fw_cfg.h Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 2/5] pc: fw_cfg: move ioport base constant to pc.h Gabriel L. Somlo
@ 2015-11-14  2:57 ` Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 4/5] acpi: arm: add fw_cfg device node to dsdt Gabriel L. Somlo
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-14  2:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

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

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/acpi-build.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 29e30ce..17eb99e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1071,6 +1071,35 @@ build_ssdt(GArray *table_data, GArray *linker,
     aml_append(scope, aml_name_decl("_S5", pkg));
     aml_append(ssdt, scope);
 
+    /* create fw_cfg node, unconditionally */
+    {
+        /* when using port i/o, the 8-bit data register *always* overlaps
+         * with half of the 16-bit control register. Hence, the total size
+         * of the i/o region used is FW_CFG_CTL_SIZE; when using DMA, the
+         * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
+        uint8_t io_size = object_property_get_bool(OBJECT(guest_info->fw_cfg),
+                                                   "dma_enabled", NULL) ?
+                          ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
+                          FW_CFG_CTL_SIZE;
+
+        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();
+        aml_append(crs,
+            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size)
+        );
+        aml_append(dev, aml_name_decl("_CRS", crs));
+
+        aml_append(scope, dev);
+        aml_append(ssdt, scope);
+    }
+
     if (misc->applesmc_io_base) {
         scope = aml_scope("\\_SB.PCI0.ISA");
         dev = aml_device("SMC");
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v5 4/5] acpi: arm: add fw_cfg device node to dsdt
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
                   ` (2 preceding siblings ...)
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 3/5] acpi: pc: add fw_cfg device node to ssdt Gabriel L. Somlo
@ 2015-11-14  2:57 ` Gabriel L. Somlo
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 5/5] fw_cfg: document ACPI device node information Gabriel L. Somlo
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-14  2:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

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

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>
---
 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 8fd0b1c..e070fe8 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;
@@ -534,6 +548,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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v5 5/5] fw_cfg: document ACPI device node information
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
                   ` (3 preceding siblings ...)
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 4/5] acpi: arm: add fw_cfg device node to dsdt Gabriel L. Somlo
@ 2015-11-14  2:57 ` Gabriel L. Somlo
  2015-11-14 12:49 ` [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Marc Marí
  2015-11-23 19:31 ` Gabriel L. Somlo
  6 siblings, 0 replies; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-14  2:57 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

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

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index 2099ad9..5414140 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -84,6 +84,15 @@ Selector Register address: Base + 8 (2 bytes)
 Data Register address:     Base + 0 (8 bytes)
 DMA Address address:       Base + 16 (8 bytes)
 
+== ACPI Interface ==
+
+The fw_cfg device is defined with ACPI ID "QEMU0002". Since we expect
+ACPI tables to be passed into the guest through the fw_cfg device itself,
+the guest-side firmware can not use ACPI to find fw_cfg. However, once the
+firmware is finished setting up ACPI tables and hands control over to the
+guest kernel, the latter can use the fw_cfg ACPI node for a more accurate
+inventory of in-use IOport or MMIO regions.
+
 == Firmware Configuration Items ==
 
 === Signature (Key 0x0000, FW_CFG_SIGNATURE) ===
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
                   ` (4 preceding siblings ...)
  2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 5/5] fw_cfg: document ACPI device node information Gabriel L. Somlo
@ 2015-11-14 12:49 ` Marc Marí
  2015-11-23 19:31 ` Gabriel L. Somlo
  6 siblings, 0 replies; 11+ messages in thread
From: Marc Marí @ 2015-11-14 12:49 UTC (permalink / raw)
  To: Gabriel L. Somlo
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	qemu-devel, leif.lindholm, kraxel, pbonzini, imammedo, lersek,
	rth

On Fri, 13 Nov 2015 21:57:13 -0500
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> New since v4:
> 
> 	- rebased on top of Marc's DMA series
> 	- drop machine compat dependency for insertion into x86/ssdt
> 	  (patch 3/5), following agreement between Igor and Eduardo
> 	- [mm]io register range now covers DMA register as well, if
> 	  available.
> 	- s/bios/firmware in doc file updates
> 
> Thanks,
>   --Gabriel
> 
> >New since v3:
> >
> >	- rebased to work on top of 87e896ab (introducing pc-*-25
> >	classes), inserting fw_cfg acpi node only for machines >=
> >	2.5.
> >
> >	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility
> >	turned off to avoid Windows complaining -- thanks Igor for
> >	catching that!)
> >
> >If there's any other feedback besides questions regarding the
> >appropriateness of "QEMU0002" as the value of _HID, please don't
> >hesitate!
> >
> >>New since v2:
> >>
> >>	- pc/i386 node in ssdt only on machine types *newer* than
> >>	2.4 (as suggested by Eduardo)
> >>
> >>I appreciate any further comments and reviews. Hopefully we can make
> >>this palatable for upstream, modulo the lingering concerns about
> >>whether "QEMU0002" is ok to use as the value of _HID, which I'll
> >>hopefully get sorted out with the kernel crew...
> >>
> >>>New since v1:
> >>>
> >>>	- expose control register size (suggested by Marc Marí)
> >>>
> >>>	- leaving out _UID and _STA fields (thanks Shannon & Igor)
> >>>
> >>>	- using "QEMU0002" as the value of _HID (thanks Michael)
> >>>
> >>>	- added documentation blurb to docs/specs/fw_cfg.txt
> >>>	  (mainly to record usage of the "QEMU0002" string with
> >>>	fw_cfg).
> >>>
> >>>> This series adds a fw_cfg device node to the SSDT (on pc), or to
> >>>> the DSDT (on arm).
> >>>>
> >>>> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT
> >>>> (0x510) define from pc.c to pc.h, so that it could be used from
> >>>> 	  acpi-build.c in patch 2/3.
> >>>> 
> >>>> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
> >>>> 
> >>>> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
> >>>>
> >>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
> >>>> for _HID; no idea whether that's appropriate, or how else I
> >>>> should figure out what to use instead...
> >>>>
> >>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> >>>> output of "info qtree". Again, if that's wrong, please point me
> >>>> in the right direction.
> >>>>
> >>>> Re. 3/3 (also mentioned after the commit blurb in the patch
> >>>> itself), I noticed none of the other DSDT entries contain a _STA
> >>>> field, wondering why it would (not) make sense to include that,
> >>>> same as on the PC.
> Gabriel L. Somlo (5):
>   fw_cfg: expose control register size in fw_cfg.h
>   pc: fw_cfg: move ioport base constant to pc.h
>   acpi: pc: add fw_cfg device node to 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      | 29 +++++++++++++++++++++++++++++
>  hw/i386/pc.c              |  5 ++---
>  hw/nvram/fw_cfg.c         |  4 +++-
>  include/hw/i386/pc.h      |  2 ++
>  include/hw/nvram/fw_cfg.h |  3 +++
>  7 files changed, 63 insertions(+), 4 deletions(-)
> 

Thanks for your work on this series and the other series and sorry I
couldn't be more active in the last few weeks.

Reviewed-by: Marc Marí <markmb@redhat.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm
  2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
                   ` (5 preceding siblings ...)
  2015-11-14 12:49 ` [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Marc Marí
@ 2015-11-23 19:31 ` Gabriel L. Somlo
  2015-11-23 19:46   ` Laszlo Ersek
  6 siblings, 1 reply; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-23 19:31 UTC (permalink / raw)
  To: qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, lersek, rth

Couple of items:

1. Ping ? :)

2. Thank you markmb for your R-b !

3. If anyone's had a chance to look over the corresponding guest-side
   kernel sysfs driver which utilizes this, you will have noticed I'm
   automatically initializing the driver based on DeviceTree or ACPI
   on ARM and x86, and that there's also the option of passing in
   command line parameters for the other architectures on which fw_cfg
   is available (sun4* and ppc/mac).

   The issue I'm wondering about is that, while architectures where
   fw_cfg registers show up as IO ports (x86 and sun4u) have the same
   register_offset:size values (0:2 for control, 1:1 for data), MMIO
   on everything *other* than ARM is different:

	- ARM has 8:2 for control, and 0:8 for data

	- sun4m and ppc/mac both have 0:2 for control, and 2:1 for data

   Right now, neither DeviceTree nor ACPI specify register offsets
   within the MMIO or PortIO area they associate with fw_cfg:

  	- ACPI has (in _CRS) either:

            IO (Decode16,
                0x0510,             // Range Minimum
                0x0510,             // Range Maximum
                0x01,               // Alignment
                0x02,               // Length
                )

	  or:

            Memory32Fixed (ReadWrite,
                           0x09020000,  // Address Base
                           0x0000000a,  // Address Length
                          )

	- DT does something along this lines:

	    {
	            #size-cells = <0x2>;
	            #address-cells = <0x2>;

	            fw-cfg@9020000 {
	                    compatible = "qemu,fw-cfg-mmio";
	                    reg = <0x0 0x9020000 0x0 0xa>;
	            };
	    };

  So in the guest-side kernel sysfs driver initialization routine,
  I need to assume x86 (and, respectively, ARM) register offset:size
  values unless explicitly provided on the command line.

  Are we likely to EVER try to supply defaults via DT (or ACPI) for
  any other architectures besides ARM and x86 ? If so, is there a way
  to additionally provide offsets (and sizes), besides just the
  overall range ?

  If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
  then nevermind I said anything :)

Thanks much,
--Gabriel 

On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
> New since v4:
> 
> 	- rebased on top of Marc's DMA series
> 	- drop machine compat dependency for insertion into x86/ssdt
> 	  (patch 3/5), following agreement between Igor and Eduardo
> 	- [mm]io register range now covers DMA register as well, if
> 	  available.
> 	- s/bios/firmware in doc file updates
> 
> Thanks,
>   --Gabriel
> 
> >New since v3:
> >
> >	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
> >	  inserting fw_cfg acpi node only for machines >= 2.5.
> >
> >	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
> >	  off to avoid Windows complaining -- thanks Igor for catching that!)
> >
> >If there's any other feedback besides questions regarding the
> >appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
> >
> >>New since v2:
> >>
> >>	- pc/i386 node in ssdt only on machine types *newer* than 2.4
> >>	  (as suggested by Eduardo)
> >>
> >>I appreciate any further comments and reviews. Hopefully we can make
> >>this palatable for upstream, modulo the lingering concerns about whether
> >>"QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
> >>sorted out with the kernel crew...
> >>
> >>>New since v1:
> >>>
> >>>	- expose control register size (suggested by Marc Marí)
> >>>
> >>>	- leaving out _UID and _STA fields (thanks Shannon & Igor)
> >>>
> >>>	- using "QEMU0002" as the value of _HID (thanks Michael)
> >>>
> >>>	- added documentation blurb to docs/specs/fw_cfg.txt
> >>>	  (mainly to record usage of the "QEMU0002" string with fw_cfg).
> >>>
> >>>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
> >>>> DSDT (on arm).
> >>>>
> >>>> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
> >>>> 	  define from pc.c to pc.h, so that it could be used from
> >>>> 	  acpi-build.c in patch 2/3.
> >>>> 
> >>>> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
> >>>> 
> >>>> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
> >>>>
> >>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
> >>>> for _HID; no idea whether that's appropriate, or how else I should
> >>>> figure out what to use instead...
> >>>>
> >>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> >>>> output of "info qtree". Again, if that's wrong, please point me in
> >>>> the right direction.
> >>>>
> >>>> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
> >>>> I noticed none of the other DSDT entries contain a _STA field, wondering
> >>>> why it would (not) make sense to include that, same as on the PC.
> Gabriel L. Somlo (5):
>   fw_cfg: expose control register size in fw_cfg.h
>   pc: fw_cfg: move ioport base constant to pc.h
>   acpi: pc: add fw_cfg device node to 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      | 29 +++++++++++++++++++++++++++++
>  hw/i386/pc.c              |  5 ++---
>  hw/nvram/fw_cfg.c         |  4 +++-
>  include/hw/i386/pc.h      |  2 ++
>  include/hw/nvram/fw_cfg.h |  3 +++
>  7 files changed, 63 insertions(+), 4 deletions(-)
> 
> -- 
> 2.4.3
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm
  2015-11-23 19:31 ` Gabriel L. Somlo
@ 2015-11-23 19:46   ` Laszlo Ersek
  2015-11-23 20:28     ` Gabriel L. Somlo
  0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2015-11-23 19:46 UTC (permalink / raw)
  To: Gabriel L. Somlo, qemu-devel
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	leif.lindholm, kraxel, pbonzini, imammedo, markmb, rth

On 11/23/15 20:31, Gabriel L. Somlo wrote:
> Couple of items:
> 
> 1. Ping ? :)
> 
> 2. Thank you markmb for your R-b !
> 
> 3. If anyone's had a chance to look over the corresponding guest-side
>    kernel sysfs driver which utilizes this, you will have noticed I'm
>    automatically initializing the driver based on DeviceTree or ACPI
>    on ARM and x86, and that there's also the option of passing in
>    command line parameters for the other architectures on which fw_cfg
>    is available (sun4* and ppc/mac).
> 
>    The issue I'm wondering about is that, while architectures where
>    fw_cfg registers show up as IO ports (x86 and sun4u) have the same
>    register_offset:size values (0:2 for control, 1:1 for data), MMIO
>    on everything *other* than ARM is different:
> 
> 	- ARM has 8:2 for control, and 0:8 for data
> 
> 	- sun4m and ppc/mac both have 0:2 for control, and 2:1 for data
> 
>    Right now, neither DeviceTree nor ACPI specify register offsets
>    within the MMIO or PortIO area they associate with fw_cfg:
> 
>   	- ACPI has (in _CRS) either:
> 
>             IO (Decode16,
>                 0x0510,             // Range Minimum
>                 0x0510,             // Range Maximum
>                 0x01,               // Alignment
>                 0x02,               // Length
>                 )
> 
> 	  or:
> 
>             Memory32Fixed (ReadWrite,
>                            0x09020000,  // Address Base
>                            0x0000000a,  // Address Length
>                           )
> 
> 	- DT does something along this lines:
> 
> 	    {
> 	            #size-cells = <0x2>;
> 	            #address-cells = <0x2>;
> 
> 	            fw-cfg@9020000 {
> 	                    compatible = "qemu,fw-cfg-mmio";
> 	                    reg = <0x0 0x9020000 0x0 0xa>;
> 	            };
> 	    };
> 
>   So in the guest-side kernel sysfs driver initialization routine,
>   I need to assume x86 (and, respectively, ARM) register offset:size
>   values unless explicitly provided on the command line.
> 
>   Are we likely to EVER try to supply defaults via DT (or ACPI) for
>   any other architectures besides ARM and x86 ? If so, is there a way
>   to additionally provide offsets (and sizes), besides just the
>   overall range ?

I guess this is where the bindings docs would provide the specification...

Laszlo

> 
>   If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
>   then nevermind I said anything :)
> 
> Thanks much,
> --Gabriel 
> 
> On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
>> New since v4:
>>
>> 	- rebased on top of Marc's DMA series
>> 	- drop machine compat dependency for insertion into x86/ssdt
>> 	  (patch 3/5), following agreement between Igor and Eduardo
>> 	- [mm]io register range now covers DMA register as well, if
>> 	  available.
>> 	- s/bios/firmware in doc file updates
>>
>> Thanks,
>>   --Gabriel
>>
>>> New since v3:
>>>
>>> 	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
>>> 	  inserting fw_cfg acpi node only for machines >= 2.5.
>>>
>>> 	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
>>> 	  off to avoid Windows complaining -- thanks Igor for catching that!)
>>>
>>> If there's any other feedback besides questions regarding the
>>> appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
>>>
>>>> New since v2:
>>>>
>>>> 	- pc/i386 node in ssdt only on machine types *newer* than 2.4
>>>> 	  (as suggested by Eduardo)
>>>>
>>>> I appreciate any further comments and reviews. Hopefully we can make
>>>> this palatable for upstream, modulo the lingering concerns about whether
>>>> "QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
>>>> sorted out with the kernel crew...
>>>>
>>>>> New since v1:
>>>>>
>>>>> 	- expose control register size (suggested by Marc Marí)
>>>>>
>>>>> 	- leaving out _UID and _STA fields (thanks Shannon & Igor)
>>>>>
>>>>> 	- using "QEMU0002" as the value of _HID (thanks Michael)
>>>>>
>>>>> 	- added documentation blurb to docs/specs/fw_cfg.txt
>>>>> 	  (mainly to record usage of the "QEMU0002" string with fw_cfg).
>>>>>
>>>>>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
>>>>>> DSDT (on arm).
>>>>>>
>>>>>> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
>>>>>> 	  define from pc.c to pc.h, so that it could be used from
>>>>>> 	  acpi-build.c in patch 2/3.
>>>>>>
>>>>>> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
>>>>>>
>>>>>> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
>>>>>>
>>>>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
>>>>>> for _HID; no idea whether that's appropriate, or how else I should
>>>>>> figure out what to use instead...
>>>>>>
>>>>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
>>>>>> output of "info qtree". Again, if that's wrong, please point me in
>>>>>> the right direction.
>>>>>>
>>>>>> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
>>>>>> I noticed none of the other DSDT entries contain a _STA field, wondering
>>>>>> why it would (not) make sense to include that, same as on the PC.
>> Gabriel L. Somlo (5):
>>   fw_cfg: expose control register size in fw_cfg.h
>>   pc: fw_cfg: move ioport base constant to pc.h
>>   acpi: pc: add fw_cfg device node to 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      | 29 +++++++++++++++++++++++++++++
>>  hw/i386/pc.c              |  5 ++---
>>  hw/nvram/fw_cfg.c         |  4 +++-
>>  include/hw/i386/pc.h      |  2 ++
>>  include/hw/nvram/fw_cfg.h |  3 +++
>>  7 files changed, 63 insertions(+), 4 deletions(-)
>>
>> -- 
>> 2.4.3
>>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm
  2015-11-23 19:46   ` Laszlo Ersek
@ 2015-11-23 20:28     ` Gabriel L. Somlo
  2015-11-23 20:45       ` Laszlo Ersek
  0 siblings, 1 reply; 11+ messages in thread
From: Gabriel L. Somlo @ 2015-11-23 20:28 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	qemu-devel, leif.lindholm, kraxel, pbonzini, imammedo, markmb,
	rth

On Mon, Nov 23, 2015 at 08:46:33PM +0100, Laszlo Ersek wrote:
> On 11/23/15 20:31, Gabriel L. Somlo wrote:
> > Couple of items:
> > 
> > 1. Ping ? :)
> > 
> > 2. Thank you markmb for your R-b !
> > 
> > 3. If anyone's had a chance to look over the corresponding guest-side
> >    kernel sysfs driver which utilizes this, you will have noticed I'm
> >    automatically initializing the driver based on DeviceTree or ACPI
> >    on ARM and x86, and that there's also the option of passing in
> >    command line parameters for the other architectures on which fw_cfg
> >    is available (sun4* and ppc/mac).
> > 
> >    The issue I'm wondering about is that, while architectures where
> >    fw_cfg registers show up as IO ports (x86 and sun4u) have the same
> >    register_offset:size values (0:2 for control, 1:1 for data), MMIO
> >    on everything *other* than ARM is different:
> > 
> > 	- ARM has 8:2 for control, and 0:8 for data
> > 
> > 	- sun4m and ppc/mac both have 0:2 for control, and 2:1 for data
> > 
> >    Right now, neither DeviceTree nor ACPI specify register offsets
> >    within the MMIO or PortIO area they associate with fw_cfg:
> > 
> >   	- ACPI has (in _CRS) either:
> > 
> >             IO (Decode16,
> >                 0x0510,             // Range Minimum
> >                 0x0510,             // Range Maximum
> >                 0x01,               // Alignment
> >                 0x02,               // Length
> >                 )
> > 
> > 	  or:
> > 
> >             Memory32Fixed (ReadWrite,
> >                            0x09020000,  // Address Base
> >                            0x0000000a,  // Address Length
> >                           )
> > 
> > 	- DT does something along this lines:
> > 
> > 	    {
> > 	            #size-cells = <0x2>;
> > 	            #address-cells = <0x2>;
> > 
> > 	            fw-cfg@9020000 {
> > 	                    compatible = "qemu,fw-cfg-mmio";
> > 	                    reg = <0x0 0x9020000 0x0 0xa>;
> > 	            };
> > 	    };
> > 
> >   So in the guest-side kernel sysfs driver initialization routine,
> >   I need to assume x86 (and, respectively, ARM) register offset:size
> >   values unless explicitly provided on the command line.
> > 
> >   Are we likely to EVER try to supply defaults via DT (or ACPI) for
> >   any other architectures besides ARM and x86 ? If so, is there a way
> >   to additionally provide offsets (and sizes), besides just the
> >   overall range ?
> 
> I guess this is where the bindings docs would provide the specification...

OK, but is there a way to *functionally* specify register offsets, in
a way that can automagically get translated into a set of

	( IORESOURCE_IO | IORESOURCE_MEM ) + IORESOURCE_REG + ...

? Right now we get _IO or _MEM only, and have to assume the register
offsets based on the architecture. I tried to figure this out on my
own, but nothing obvious stands out either in the way DT nodes are
documented, nor in the way ACPI nodes are written in several of the
machines I acpidump-ed to look for clues [*]. Maybe not too many devices
have this much variability across the platforms they're deployed on :)

[*] In ACPI there's "Offset", but that's for accessing fields within an
    operation range, and there's "Register" which seems promising:

    replace IO(...) or Memory32Fixed(...) with a set of Register(...)
    statements in _CRS, but not immediately clear how one would
    specify which one is Data vs Ctrl (order ?), and whether there's
    an equivalent way to specify this via DT...


I guess there's always something like

#if defined ARCH_X86

#define CTRL_OFF 0x00
#define DATA_OFF 0x01

#elif defined ARCH_ARM

#define CTRL_OFF 0x08
#define DATA_OFF 0x00

#else ...

Although I haven't seen a lot of that done anywhere in the kernel, so
it might be frowned upon... :)


Thanks,
--Gabriel


> > 
> >   If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
> >   then nevermind I said anything :)
> > 
> > Thanks much,
> > --Gabriel 
> > 
> > On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
> >> New since v4:
> >>
> >> 	- rebased on top of Marc's DMA series
> >> 	- drop machine compat dependency for insertion into x86/ssdt
> >> 	  (patch 3/5), following agreement between Igor and Eduardo
> >> 	- [mm]io register range now covers DMA register as well, if
> >> 	  available.
> >> 	- s/bios/firmware in doc file updates
> >>
> >> Thanks,
> >>   --Gabriel
> >>
> >>> New since v3:
> >>>
> >>> 	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
> >>> 	  inserting fw_cfg acpi node only for machines >= 2.5.
> >>>
> >>> 	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
> >>> 	  off to avoid Windows complaining -- thanks Igor for catching that!)
> >>>
> >>> If there's any other feedback besides questions regarding the
> >>> appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
> >>>
> >>>> New since v2:
> >>>>
> >>>> 	- pc/i386 node in ssdt only on machine types *newer* than 2.4
> >>>> 	  (as suggested by Eduardo)
> >>>>
> >>>> I appreciate any further comments and reviews. Hopefully we can make
> >>>> this palatable for upstream, modulo the lingering concerns about whether
> >>>> "QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
> >>>> sorted out with the kernel crew...
> >>>>
> >>>>> New since v1:
> >>>>>
> >>>>> 	- expose control register size (suggested by Marc Marí)
> >>>>>
> >>>>> 	- leaving out _UID and _STA fields (thanks Shannon & Igor)
> >>>>>
> >>>>> 	- using "QEMU0002" as the value of _HID (thanks Michael)
> >>>>>
> >>>>> 	- added documentation blurb to docs/specs/fw_cfg.txt
> >>>>> 	  (mainly to record usage of the "QEMU0002" string with fw_cfg).
> >>>>>
> >>>>>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
> >>>>>> DSDT (on arm).
> >>>>>>
> >>>>>> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
> >>>>>> 	  define from pc.c to pc.h, so that it could be used from
> >>>>>> 	  acpi-build.c in patch 2/3.
> >>>>>>
> >>>>>> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
> >>>>>>
> >>>>>> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
> >>>>>>
> >>>>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
> >>>>>> for _HID; no idea whether that's appropriate, or how else I should
> >>>>>> figure out what to use instead...
> >>>>>>
> >>>>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> >>>>>> output of "info qtree". Again, if that's wrong, please point me in
> >>>>>> the right direction.
> >>>>>>
> >>>>>> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
> >>>>>> I noticed none of the other DSDT entries contain a _STA field, wondering
> >>>>>> why it would (not) make sense to include that, same as on the PC.
> >> Gabriel L. Somlo (5):
> >>   fw_cfg: expose control register size in fw_cfg.h
> >>   pc: fw_cfg: move ioport base constant to pc.h
> >>   acpi: pc: add fw_cfg device node to 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      | 29 +++++++++++++++++++++++++++++
> >>  hw/i386/pc.c              |  5 ++---
> >>  hw/nvram/fw_cfg.c         |  4 +++-
> >>  include/hw/i386/pc.h      |  2 ++
> >>  include/hw/nvram/fw_cfg.h |  3 +++
> >>  7 files changed, 63 insertions(+), 4 deletions(-)
> >>
> >> -- 
> >> 2.4.3
> >>
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm
  2015-11-23 20:28     ` Gabriel L. Somlo
@ 2015-11-23 20:45       ` Laszlo Ersek
  0 siblings, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2015-11-23 20:45 UTC (permalink / raw)
  To: Gabriel L. Somlo
  Cc: peter.maydell, matt.fleming, ehabkost, mst, ard.biesheuvel,
	qemu-devel, leif.lindholm, kraxel, pbonzini, imammedo, markmb,
	rth

On 11/23/15 21:28, Gabriel L. Somlo wrote:
> On Mon, Nov 23, 2015 at 08:46:33PM +0100, Laszlo Ersek wrote:
>> On 11/23/15 20:31, Gabriel L. Somlo wrote:
>>> Couple of items:
>>>
>>> 1. Ping ? :)
>>>
>>> 2. Thank you markmb for your R-b !
>>>
>>> 3. If anyone's had a chance to look over the corresponding guest-side
>>>    kernel sysfs driver which utilizes this, you will have noticed I'm
>>>    automatically initializing the driver based on DeviceTree or ACPI
>>>    on ARM and x86, and that there's also the option of passing in
>>>    command line parameters for the other architectures on which fw_cfg
>>>    is available (sun4* and ppc/mac).
>>>
>>>    The issue I'm wondering about is that, while architectures where
>>>    fw_cfg registers show up as IO ports (x86 and sun4u) have the same
>>>    register_offset:size values (0:2 for control, 1:1 for data), MMIO
>>>    on everything *other* than ARM is different:
>>>
>>> 	- ARM has 8:2 for control, and 0:8 for data
>>>
>>> 	- sun4m and ppc/mac both have 0:2 for control, and 2:1 for data
>>>
>>>    Right now, neither DeviceTree nor ACPI specify register offsets
>>>    within the MMIO or PortIO area they associate with fw_cfg:
>>>
>>>   	- ACPI has (in _CRS) either:
>>>
>>>             IO (Decode16,
>>>                 0x0510,             // Range Minimum
>>>                 0x0510,             // Range Maximum
>>>                 0x01,               // Alignment
>>>                 0x02,               // Length
>>>                 )
>>>
>>> 	  or:
>>>
>>>             Memory32Fixed (ReadWrite,
>>>                            0x09020000,  // Address Base
>>>                            0x0000000a,  // Address Length
>>>                           )
>>>
>>> 	- DT does something along this lines:
>>>
>>> 	    {
>>> 	            #size-cells = <0x2>;
>>> 	            #address-cells = <0x2>;
>>>
>>> 	            fw-cfg@9020000 {
>>> 	                    compatible = "qemu,fw-cfg-mmio";
>>> 	                    reg = <0x0 0x9020000 0x0 0xa>;
>>> 	            };
>>> 	    };
>>>
>>>   So in the guest-side kernel sysfs driver initialization routine,
>>>   I need to assume x86 (and, respectively, ARM) register offset:size
>>>   values unless explicitly provided on the command line.
>>>
>>>   Are we likely to EVER try to supply defaults via DT (or ACPI) for
>>>   any other architectures besides ARM and x86 ? If so, is there a way
>>>   to additionally provide offsets (and sizes), besides just the
>>>   overall range ?
>>
>> I guess this is where the bindings docs would provide the specification...
> 
> OK, but is there a way to *functionally* specify register offsets, in
> a way that can automagically get translated into a set of
> 
> 	( IORESOURCE_IO | IORESOURCE_MEM ) + IORESOURCE_REG + ...
> 
> ? Right now we get _IO or _MEM only, and have to assume the register
> offsets based on the architecture. I tried to figure this out on my
> own, but nothing obvious stands out either in the way DT nodes are
> documented, nor in the way ACPI nodes are written in several of the
> machines I acpidump-ed to look for clues [*]. Maybe not too many devices
> have this much variability across the platforms they're deployed on :)
> 
> [*] In ACPI there's "Offset", but that's for accessing fields within an
>     operation range, and there's "Register" which seems promising:
> 
>     replace IO(...) or Memory32Fixed(...) with a set of Register(...)
>     statements in _CRS, but not immediately clear how one would
>     specify which one is Data vs Ctrl (order ?), and whether there's
>     an equivalent way to specify this via DT...
> 
> 
> I guess there's always something like
> 
> #if defined ARCH_X86
> 
> #define CTRL_OFF 0x00
> #define DATA_OFF 0x01
> 
> #elif defined ARCH_ARM
> 
> #define CTRL_OFF 0x08
> #define DATA_OFF 0x00
> 
> #else ...
> 
> Although I haven't seen a lot of that done anywhere in the kernel, so
> it might be frowned upon... :)

That's actually what I meant (although I'm not sure how it is expected
to be laid out in the kernel, arch-dependently). In general, "bindings"
are just a bunch of arbitrary details that cannot be deduced otherwise,
so they must be listed in some dry and boring written document.

Thanks
Laszlo

> 
> 
> Thanks,
> --Gabriel
> 
> 
>>>
>>>   If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
>>>   then nevermind I said anything :)
>>>
>>> Thanks much,
>>> --Gabriel 
>>>
>>> On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
>>>> New since v4:
>>>>
>>>> 	- rebased on top of Marc's DMA series
>>>> 	- drop machine compat dependency for insertion into x86/ssdt
>>>> 	  (patch 3/5), following agreement between Igor and Eduardo
>>>> 	- [mm]io register range now covers DMA register as well, if
>>>> 	  available.
>>>> 	- s/bios/firmware in doc file updates
>>>>
>>>> Thanks,
>>>>   --Gabriel
>>>>
>>>>> New since v3:
>>>>>
>>>>> 	- rebased to work on top of 87e896ab (introducing pc-*-25 classes),
>>>>> 	  inserting fw_cfg acpi node only for machines >= 2.5.
>>>>>
>>>>> 	- reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
>>>>> 	  off to avoid Windows complaining -- thanks Igor for catching that!)
>>>>>
>>>>> If there's any other feedback besides questions regarding the
>>>>> appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
>>>>>
>>>>>> New since v2:
>>>>>>
>>>>>> 	- pc/i386 node in ssdt only on machine types *newer* than 2.4
>>>>>> 	  (as suggested by Eduardo)
>>>>>>
>>>>>> I appreciate any further comments and reviews. Hopefully we can make
>>>>>> this palatable for upstream, modulo the lingering concerns about whether
>>>>>> "QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
>>>>>> sorted out with the kernel crew...
>>>>>>
>>>>>>> New since v1:
>>>>>>>
>>>>>>> 	- expose control register size (suggested by Marc Marí)
>>>>>>>
>>>>>>> 	- leaving out _UID and _STA fields (thanks Shannon & Igor)
>>>>>>>
>>>>>>> 	- using "QEMU0002" as the value of _HID (thanks Michael)
>>>>>>>
>>>>>>> 	- added documentation blurb to docs/specs/fw_cfg.txt
>>>>>>> 	  (mainly to record usage of the "QEMU0002" string with fw_cfg).
>>>>>>>
>>>>>>>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
>>>>>>>> DSDT (on arm).
>>>>>>>>
>>>>>>>> 	- Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
>>>>>>>> 	  define from pc.c to pc.h, so that it could be used from
>>>>>>>> 	  acpi-build.c in patch 2/3.
>>>>>>>>
>>>>>>>> 	- Patch 2/3 adds a fw_cfg node to the pc SSDT.
>>>>>>>>
>>>>>>>> 	- Patch 3/3 adds a fw_cfg node to the arm DSDT.
>>>>>>>>
>>>>>>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
>>>>>>>> for _HID; no idea whether that's appropriate, or how else I should
>>>>>>>> figure out what to use instead...
>>>>>>>>
>>>>>>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
>>>>>>>> output of "info qtree". Again, if that's wrong, please point me in
>>>>>>>> the right direction.
>>>>>>>>
>>>>>>>> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
>>>>>>>> I noticed none of the other DSDT entries contain a _STA field, wondering
>>>>>>>> why it would (not) make sense to include that, same as on the PC.
>>>> Gabriel L. Somlo (5):
>>>>   fw_cfg: expose control register size in fw_cfg.h
>>>>   pc: fw_cfg: move ioport base constant to pc.h
>>>>   acpi: pc: add fw_cfg device node to 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      | 29 +++++++++++++++++++++++++++++
>>>>  hw/i386/pc.c              |  5 ++---
>>>>  hw/nvram/fw_cfg.c         |  4 +++-
>>>>  include/hw/i386/pc.h      |  2 ++
>>>>  include/hw/nvram/fw_cfg.h |  3 +++
>>>>  7 files changed, 63 insertions(+), 4 deletions(-)
>>>>
>>>> -- 
>>>> 2.4.3
>>>>
>>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-11-23 20:45 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-14  2:57 [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Gabriel L. Somlo
2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 1/5] fw_cfg: expose control register size in fw_cfg.h Gabriel L. Somlo
2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 2/5] pc: fw_cfg: move ioport base constant to pc.h Gabriel L. Somlo
2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 3/5] acpi: pc: add fw_cfg device node to ssdt Gabriel L. Somlo
2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 4/5] acpi: arm: add fw_cfg device node to dsdt Gabriel L. Somlo
2015-11-14  2:57 ` [Qemu-devel] [PATCH v5 5/5] fw_cfg: document ACPI device node information Gabriel L. Somlo
2015-11-14 12:49 ` [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm Marc Marí
2015-11-23 19:31 ` Gabriel L. Somlo
2015-11-23 19:46   ` Laszlo Ersek
2015-11-23 20:28     ` Gabriel L. Somlo
2015-11-23 20:45       ` Laszlo Ersek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.