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

[Laszlo Ersek <lersek@redhat.com>]

On 09/29/15 18:46, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
>> On 09/27/15 23:29, Gabriel L. Somlo wrote:
>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>>> pc-*-2.5 and up. While the guest-side BIOS can't utilize
>>> this information (since it has to access the hard-coded
>>> fw_cfg device to extract ACPI tables to begin with), having
>>> fw_cfg listed in ACPI will help the guest kernel keep a more
>>> accurate inventory of in-use IO port regions.
>>>
>>> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
>>> ---
>>>  hw/i386/acpi-build.c | 23 +++++++++++++++++++++++
>>>  hw/i386/pc_piix.c    |  1 +
>>>  hw/i386/pc_q35.c     |  1 +
>>>  include/hw/i386/pc.h |  1 +
>>>  4 files changed, 26 insertions(+)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 95e0c65..ece2710 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>             PcPciInfo *pci, PcGuestInfo *guest_info)
>>>  {
>>>      MachineState *machine = MACHINE(qdev_get_machine());
>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>>      uint32_t nr_mem = machine->ram_slots;
>>>      unsigned acpi_cpus = guest_info->apic_id_limit;
>>>      Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>>>      aml_append(scope, aml_name_decl("_S5", pkg));
>>>      aml_append(ssdt, scope);
>>>  
>>> +    if (!pcmc->acpi_no_fw_cfg_node) {
>>> +        scope = aml_scope("\\_SB");
>>> +        dev = aml_device("FWCF");
>>> +
>>> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
>>> +        /* device present, functioning, decoding, not shown in UI */
>>> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>>> +
>>> +        crs = aml_resource_template();
>>> +        /* when using port i/o, the 8-bit data register *always* overlaps
>>> +         * with half of the 16-bit control register. Hence, the total size
>>> +         * of the i/o region used is FW_CFG_CTL_SIZE */
>>> +        aml_append(crs,
>>> +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
>>> +                   0x01, FW_CFG_CTL_SIZE)
>>> +        );
>>
>> I think "aml_io" should be indented so that it lines up with "crs" above it.
> 
> There are a few other nodes being added in if() {...} bloks
> immediately following the fw_cfg one. They *all* indent it this way, I
> just made mine look similar. That said, I have no problem indenting
> mine differently, if you still want me to... :)

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

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

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

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

Thanks!
Laszlo

> 
> Thanks,
> --Gabriel
> 
>>
>>> +        aml_append(dev, aml_name_decl("_CRS", crs));
>>> +
>>> +        aml_append(scope, dev);
>>> +        aml_append(ssdt, scope);
>>> +    }
>>> +
>>>      if (misc->applesmc_io_base) {
>>>          scope = aml_scope("\\_SB.PCI0.ISA");
>>>          dev = aml_device("SMC");
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 3ffb05f..7f5e5d9 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
>>>      m->alias = NULL;
>>>      m->is_default = 0;
>>>      pcmc->broken_reserved_end = true;
>>> +    pcmc->acpi_no_fw_cfg_node = true;
>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>>  }
>>>  
>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>> index 1b7d3b6..7180ca3 100644
>>> --- a/hw/i386/pc_q35.c
>>> +++ b/hw/i386/pc_q35.c
>>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>>>      pc_q35_2_5_machine_options(m);
>>>      m->alias = NULL;
>>>      pcmc->broken_reserved_end = true;
>>> +    pcmc->acpi_no_fw_cfg_node = true;
>>>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>>  }
>>>  
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index 86007e3..6d0f1bd 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@ -62,6 +62,7 @@ struct PCMachineClass {
>>>      bool broken_reserved_end;
>>>      HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>>>                                             DeviceState *dev);
>>> +    bool acpi_no_fw_cfg_node;
>>>  };
>>>  
>>>  #define TYPE_PC_MACHINE "generic-pc-machine"
>>>
>>