[Qemu-devel] [RFC PATCH 0/3] adding acpi node for fw_cfg on pc and arm

[Qemu-devel] [RFC PATCH 0/3] adding acpi node for fw_cfg on pc and arm

[Gabriel L. Somlo]

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.

TIA for any feedback, comments, reviews, etc.
 --Gabriel

Gabriel L. Somlo (3):
  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

 hw/arm/virt-acpi-build.c | 18 ++++++++++++++++++
 hw/i386/acpi-build.c     | 19 +++++++++++++++++++
 hw/i386/pc.c             |  5 ++---
 include/hw/i386/pc.h     |  3 +++
 4 files changed, 42 insertions(+), 3 deletions(-)

-- 
2.4.3

[Qemu-devel] [RFC PATCH 1/3] 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. Also, add FW_CFG_IO_SIZE define (set
to 0x02, to cover the overlapping 16-bit control and 8-bit
data ports).

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

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

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

[Gabriel L. Somlo]

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

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..9d0ec22 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1071,6 +1071,25 @@ build_ssdt(GArray *table_data, GArray *linker,
     aml_append(scope, aml_name_decl("_S5", pkg));
     aml_append(ssdt, scope);
 
+    if (guest_info->fw_cfg) {
+        scope = aml_scope("\\_SB");
+        dev = aml_device("FWCF");
+
+        aml_append(dev, aml_name_decl("_HID", aml_string("FWCF0001")));
+        /* 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, FW_CFG_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

[Qemu-devel] [RFC PATCH 3/3] 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>
---

I used surrounding examples to create acpi_dsdt_add_fw_cfg(), and
noticed that none add a _STA method, and many include a 0 _UID even
for nodes with a single instance. I wonder whether 1. I really need
the _UID, and 2. why would we be OK not including a _STA method ?

Is the #2 answer "because no exisging arm OSPM does in fact check,
and/or care about the absence of _STA" ? :)

Thanks,
 --Gabriel

 hw/arm/virt-acpi-build.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9088248..150c9f9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -110,6 +110,23 @@ 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("FWCF0001")));
+
+    /* FIXME: is this necessary ? */
+    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+    /* FIXME: why doesn't a _STA get added to any other node ? */
+    aml_append(dev, aml_name_decl("_STA", aml_int(0x0B)));
+
+    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 +536,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     acpi_dsdt_add_rtc(scope, &memmap[VIRT_RTC],
                       (irqmap[VIRT_RTC] + ARM_SPI_BASE));
+    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH 1/3] pc: fw_cfg: move ioport base constant to pc.h

[Marc Marí]

On Sat, 12 Sep 2015 19:30:40 -0400
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> Move BIOS_CFG_IOPORT define from pc.c to pc.h, and rename
> it to FW_CFG_IO_BASE. Also, add FW_CFG_IO_SIZE define (set
> to 0x02, to cover the overlapping 16-bit control and 8-bit
> data ports).
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/i386/pc.c         | 5 ++---
>  include/hw/i386/pc.h | 3 +++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b5107f7..1a92b4f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -86,7 +86,6 @@ void pc_set_legacy_acpi_data_size(void)
>      acpi_data_size = 0x10000;
>  }
>  
> -#define BIOS_CFG_IOPORT 0x510
>  #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
>  #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
>  #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> @@ -760,7 +759,7 @@ static FWCfgState *bochs_bios_init(void)
>      int i, j;
>      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
>  
> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
>      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
>       *
>       * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU
> hotplug @@ -1292,7 +1291,7 @@ FWCfgState
> *xen_load_linux(PCMachineState *pcms, 
>      assert(MACHINE(pcms)->kernel_filename != NULL);
>  
> -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
>      rom_set_fw(fw_cfg);
>  
>      load_linux(pcms, fw_cfg);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3e002c9..0cab3c5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -206,6 +206,9 @@ 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
> +#define FW_CFG_IO_SIZE  0x02
> +
>  /* acpi_piix.c */
>  
>  I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,

There is already a size defined in hw/nvram/fw_cfg.c (FW_CFG_SIZE). You
could move this definition to the .h and reuse it for ACPI. This way,
it is easier to modify.

Note that this value is used both for the size of the IO port and the
size of the CTL field when using memory regions. You can split it now in
your patches, or it will be split in my patches.

I'm not going to comment on the other patches, because I don't know
ACPI.

Thanks
Marc

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

[Michael S. Tsirkin]

On Sat, Sep 12, 2015 at 07:30:41PM -0400, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> BIOS can't utilize this information (since it has to access the
> hard-coded fw_cfg device to extract ACPI tables to begin with),
> having fw_cfg listed in ACPI will help the guest kernel keep a
> more accurate inventory of in-use IO port regions.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/i386/acpi-build.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..9d0ec22 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1071,6 +1071,25 @@ build_ssdt(GArray *table_data, GArray *linker,
>      aml_append(scope, aml_name_decl("_S5", pkg));
>      aml_append(ssdt, scope);
>  
> +    if (guest_info->fw_cfg) {
> +        scope = aml_scope("\\_SB");
> +        dev = aml_device("FWCF");
> +
> +        aml_append(dev, aml_name_decl("_HID", aml_string("FWCF0001")));

Generally that's an illegal HID. If this device has a driver,
use QEMU as a prefix. Otherwise, use one of the pre-defined ones
with a PNP ISA ID.

> +        /* 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, FW_CFG_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

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

[Gabriel L. Somlo]

On Sun, Sep 13, 2015 at 02:45:23PM +0300, Michael S. Tsirkin wrote:
> On Sat, Sep 12, 2015 at 07:30:41PM -0400, Gabriel L. Somlo wrote:
> > Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> > BIOS can't utilize this information (since it has to access the
> > hard-coded fw_cfg device to extract ACPI tables to begin with),
> > having fw_cfg listed in ACPI will help the guest kernel keep a
> > more accurate inventory of in-use IO port regions.
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  hw/i386/acpi-build.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 95e0c65..9d0ec22 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1071,6 +1071,25 @@ build_ssdt(GArray *table_data, GArray *linker,
> >      aml_append(scope, aml_name_decl("_S5", pkg));
> >      aml_append(ssdt, scope);
> >  
> > +    if (guest_info->fw_cfg) {
> > +        scope = aml_scope("\\_SB");
> > +        dev = aml_device("FWCF");
> > +
> > +        aml_append(dev, aml_name_decl("_HID", aml_string("FWCF0001")));
> 
> Generally that's an illegal HID. If this device has a driver,
> use QEMU as a prefix. Otherwise, use one of the pre-defined ones
> with a PNP ISA ID.

I'm working on a sysfs driver to allow access to fw_cfg files via the
guest kernel (similar to e.g. /sys/firmware/dmi/entries/...). That
probably means I should go with QEMU0002 (0001 is already assigned to
the pvpanic device).

I'll use that in v2, which I'll send out once I get some feedback from
the arm side as well.

Thanks much,
--Gabriel

> > +        /* 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, FW_CFG_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

Re: [Qemu-devel] [RFC PATCH 1/3] pc: fw_cfg: move ioport base constant to pc.h

[Gabriel L. Somlo]

On Sun, Sep 13, 2015 at 12:51:53PM +0200, Marc Marí wrote:
> On Sat, 12 Sep 2015 19:30:40 -0400
> "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> 
> > Move BIOS_CFG_IOPORT define from pc.c to pc.h, and rename
> > it to FW_CFG_IO_BASE. Also, add FW_CFG_IO_SIZE define (set
> > to 0x02, to cover the overlapping 16-bit control and 8-bit
> > data ports).
> > 
> > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > ---
> >  hw/i386/pc.c         | 5 ++---
> >  include/hw/i386/pc.h | 3 +++
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b5107f7..1a92b4f 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -86,7 +86,6 @@ void pc_set_legacy_acpi_data_size(void)
> >      acpi_data_size = 0x10000;
> >  }
> >  
> > -#define BIOS_CFG_IOPORT 0x510
> >  #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
> >  #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> >  #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> > @@ -760,7 +759,7 @@ static FWCfgState *bochs_bios_init(void)
> >      int i, j;
> >      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> >  
> > -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> > +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> >      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
> >       *
> >       * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU
> > hotplug @@ -1292,7 +1291,7 @@ FWCfgState
> > *xen_load_linux(PCMachineState *pcms, 
> >      assert(MACHINE(pcms)->kernel_filename != NULL);
> >  
> > -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> > +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> >      rom_set_fw(fw_cfg);
> >  
> >      load_linux(pcms, fw_cfg);
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 3e002c9..0cab3c5 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -206,6 +206,9 @@ 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
> > +#define FW_CFG_IO_SIZE  0x02
> > +
> >  /* acpi_piix.c */
> >  
> >  I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
> 
> There is already a size defined in hw/nvram/fw_cfg.c (FW_CFG_SIZE). You
> could move this definition to the .h and reuse it for ACPI. This way,
> it is easier to modify.
> 
> Note that this value is used both for the size of the IO port and the
> size of the CTL field when using memory regions. You can split it now in
> your patches, or it will be split in my patches.

Thanks for the feedback! It does look like FW_CFG_SIZE in fw_cfg.c
appears to be mainly concerned with the width of the control register,
which is a "private" property of fw_cfg.c, rather than the total size
of the fw_cfg ioport region, which is a property of hw/i386/pc.c
(same as a15memmap[VIRT_FW_CFG] contains the same (base,size)
properties for the equivalent mmio region on arm).

We could rename FW_CFG_SIZE in fw_cfg.c to FW_CFG_CTL_SIZE for
increased clarity, but the fact that it's equal to FW_CFG_IO_SIZE
on hw/i386/... seems to me like more of a coincidence...

OTOH, i386/acpi_build.c includes both pc.h and fw_cfg.h, so if I have
to, I could use FW_CFG_IO_BASE from the former and FW_CFG_SIZE from
the latter.

It's more of a question of aesthetics at this point, so I'm happy
to do it whichever way I'm told :)

Thanks,
--Gabriel

> 
> I'm not going to comment on the other patches, because I don't know
> ACPI.
> 
> Thanks
> Marc

Re: [Qemu-devel] [RFC PATCH 1/3] pc: fw_cfg: move ioport base constant to pc.h

[Marc Marí]

On Sun, 13 Sep 2015 13:28:24 -0400
"Gabriel L. Somlo" <somlo@cmu.edu> wrote:

> On Sun, Sep 13, 2015 at 12:51:53PM +0200, Marc Marí wrote:
> > On Sat, 12 Sep 2015 19:30:40 -0400
> > "Gabriel L. Somlo" <somlo@cmu.edu> wrote:
> > 
> > > Move BIOS_CFG_IOPORT define from pc.c to pc.h, and rename
> > > it to FW_CFG_IO_BASE. Also, add FW_CFG_IO_SIZE define (set
> > > to 0x02, to cover the overlapping 16-bit control and 8-bit
> > > data ports).
> > > 
> > > Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> > > ---
> > >  hw/i386/pc.c         | 5 ++---
> > >  include/hw/i386/pc.h | 3 +++
> > >  2 files changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index b5107f7..1a92b4f 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -86,7 +86,6 @@ void pc_set_legacy_acpi_data_size(void)
> > >      acpi_data_size = 0x10000;
> > >  }
> > >  
> > > -#define BIOS_CFG_IOPORT 0x510
> > >  #define FW_CFG_ACPI_TABLES (FW_CFG_ARCH_LOCAL + 0)
> > >  #define FW_CFG_SMBIOS_ENTRIES (FW_CFG_ARCH_LOCAL + 1)
> > >  #define FW_CFG_IRQ0_OVERRIDE (FW_CFG_ARCH_LOCAL + 2)
> > > @@ -760,7 +759,7 @@ static FWCfgState *bochs_bios_init(void)
> > >      int i, j;
> > >      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> > >  
> > > -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> > > +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> > >      /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
> > >       *
> > >       * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU
> > > hotplug @@ -1292,7 +1291,7 @@ FWCfgState
> > > *xen_load_linux(PCMachineState *pcms, 
> > >      assert(MACHINE(pcms)->kernel_filename != NULL);
> > >  
> > > -    fw_cfg = fw_cfg_init_io(BIOS_CFG_IOPORT);
> > > +    fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> > >      rom_set_fw(fw_cfg);
> > >  
> > >      load_linux(pcms, fw_cfg);
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 3e002c9..0cab3c5 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -206,6 +206,9 @@ 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
> > > +#define FW_CFG_IO_SIZE  0x02
> > > +
> > >  /* acpi_piix.c */
> > >  
> > >  I2CBus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t
> > > smb_io_base,
> > 
> > There is already a size defined in hw/nvram/fw_cfg.c (FW_CFG_SIZE).
> > You could move this definition to the .h and reuse it for ACPI.
> > This way, it is easier to modify.
> > 
> > Note that this value is used both for the size of the IO port and
> > the size of the CTL field when using memory regions. You can split
> > it now in your patches, or it will be split in my patches.
> 
> Thanks for the feedback! It does look like FW_CFG_SIZE in fw_cfg.c
> appears to be mainly concerned with the width of the control register,
> which is a "private" property of fw_cfg.c, rather than the total size
> of the fw_cfg ioport region, which is a property of hw/i386/pc.c
> (same as a15memmap[VIRT_FW_CFG] contains the same (base,size)
> properties for the equivalent mmio region on arm).
> 
> We could rename FW_CFG_SIZE in fw_cfg.c to FW_CFG_CTL_SIZE for
> increased clarity, but the fact that it's equal to FW_CFG_IO_SIZE
> on hw/i386/... seems to me like more of a coincidence...

In hw/nvram/fw_cfg.c

L. 675, in fw_cfg_io_realize():
memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
                      FW_CFG(s), "fwcfg", FW_CFG_SIZE); 

L. 707, in fw_cfg_mem_realize():
memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
                      FW_CFG(s), "fwcfg.ctl", FW_CFG_SIZE);

The first one is the size of all the IO region, and the second one is
the size of the memory mapped CTL field.

The value for the ACPI size could be the same value used in
memory_region_init_io. But it needs to be deatached from the CTL size.
That's what I meant before. Of course, it can be the same, but it's
better to split it.

Thanks
Marc

> OTOH, i386/acpi_build.c includes both pc.h and fw_cfg.h, so if I have
> to, I could use FW_CFG_IO_BASE from the former and FW_CFG_SIZE from
> the latter.
> 
> It's more of a question of aesthetics at this point, so I'm happy
> to do it whichever way I'm told :)
> 
> > 
> > I'm not going to comment on the other patches, because I don't know
> > ACPI.
> > 
> > Thanks
> > Marc

Re: [Qemu-devel] [RFC PATCH 3/3] acpi: arm: add fw_cfg device node to dsdt

[Shannon Zhao]

On 2015/9/13 7:30, 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>
> ---
> 
> I used surrounding examples to create acpi_dsdt_add_fw_cfg(), and
> noticed that none add a _STA method, and many include a 0 _UID even
> for nodes with a single instance. I wonder whether 1. I really need
> the _UID, 
The _UID is optional if you has other way to identify it. So for the
nodes with one instance, it's not necessary. But having this is harmless.

and 2. why would we be OK not including a _STA method ?

Look at 6.3.7 _STA (Status) in ACPI Spec 6.0, it says "OSPM evaluates
the _STA object before it evaluates a device _INI method." So if the
device doesn't have _INI method, _STA is not necessary. And "If the _STA
object does not exist for the device, the device is assumed to be both
present and functional."

> 
> Is the #2 answer "because no exisging arm OSPM does in fact check,
> and/or care about the absence of _STA" ? :)
> 
> Thanks,
>  --Gabriel
> 
>  hw/arm/virt-acpi-build.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9088248..150c9f9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -110,6 +110,23 @@ 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("FWCF0001")));
> +
> +    /* FIXME: is this necessary ? */
> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +    /* FIXME: why doesn't a _STA get added to any other node ? */
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0x0B)));
> +
> +    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 +536,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);
> 

-- 
Shannon

Re: [Qemu-devel] [RFC PATCH 3/3] acpi: arm: add fw_cfg device node to dsdt

[Igor Mammedov]

On Sat, 12 Sep 2015 19:30:42 -0400
"Gabriel L. Somlo" <somlo@cmu.edu> 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>
> ---
> 
> I used surrounding examples to create acpi_dsdt_add_fw_cfg(), and
> noticed that none add a _STA method, and many include a 0 _UID even
> for nodes with a single instance. I wonder whether 1. I really need
> the _UID, and 2. why would we be OK not including a _STA method ?
> 
> Is the #2 answer "because no exisging arm OSPM does in fact check,
> and/or care about the absence of _STA" ? :)
> 
> Thanks,
>  --Gabriel
> 
>  hw/arm/virt-acpi-build.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9088248..150c9f9 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -110,6 +110,23 @@ 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("FWCF0001")));
> +
> +    /* FIXME: is this necessary ? */
in this case you don't need _UID since there is only one instance of given
device.

> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +    /* FIXME: why doesn't a _STA get added to any other node ? */
> +    aml_append(dev, aml_name_decl("_STA", aml_int(0x0B)));
if _STA is not present OSPM must assume that device is present, that's
why it's not used for always present devices.
0x0B is used to hide given device (usually driver-less) in
Windows's device manager.

> +
> +    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 +536,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);

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

[Eduardo Habkost]

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

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

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

> +        scope = aml_scope("\\_SB");
> +        dev = aml_device("FWCF");
> +
> +        aml_append(dev, aml_name_decl("_HID", aml_string("FWCF0001")));
> +        /* 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, FW_CFG_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
> 

-- 
Eduardo