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

[Igor Mammedov <imammedo@redhat.com>]

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

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

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


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

> 
> I'll let Gabriel answer your other question below. :)
> 
> Thanks
> Laszlo
> 
> 
> > 
> > 
> >> +        scope = aml_scope("\\_SB");
> >> +        dev = aml_device("FWCF");
> >> +
> >> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> >> +        /* device present, functioning, decoding, not shown in UI */
> >> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> >> +
> >> +        crs = aml_resource_template();
> >> +        /* when using port i/o, the 8-bit data register *always* overlaps
> >> +         * with half of the 16-bit control register. Hence, the total size
> >> +         * of the i/o region used is FW_CFG_CTL_SIZE */
> >> +        aml_append(crs,
> >> +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> >> +                   0x01, FW_CFG_CTL_SIZE)
> >> +        );
> > could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
> > in any way?
> > 
> > 
> >> +        aml_append(dev, aml_name_decl("_CRS", crs));
> >> +
> >> +        aml_append(scope, dev);
> >> +        aml_append(ssdt, scope);
> >> +    }
> >> +
> >>      if (misc->applesmc_io_base) {
> >>          scope = aml_scope("\\_SB.PCI0.ISA");
> >>          dev = aml_device("SMC");
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 3ffb05f..7f5e5d9 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> >>      m->alias = NULL;
> >>      m->is_default = 0;
> >>      pcmc->broken_reserved_end = true;
> >> +    pcmc->acpi_no_fw_cfg_node = true;
> >>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >>  }
> >>  
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index 1b7d3b6..7180ca3 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >>      pc_q35_2_5_machine_options(m);
> >>      m->alias = NULL;
> >>      pcmc->broken_reserved_end = true;
> >> +    pcmc->acpi_no_fw_cfg_node = true;
> >>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >>  }
> >>  
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 86007e3..6d0f1bd 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -62,6 +62,7 @@ struct PCMachineClass {
> >>      bool broken_reserved_end;
> >>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> >>                                             DeviceState *dev);
> >> +    bool acpi_no_fw_cfg_node;
> >>  };
> >>  
> >>  #define TYPE_PC_MACHINE "generic-pc-machine"
> > 
>