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

[Laszlo Ersek]

On 09/29/15 19:19, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
>> 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 <address@hidden>
>>>>> ---
>>>>>  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 <address@hidden>
>>>>
>>>> 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?
> 
> no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
> Leaving out _STA entirely would have had it default to 0x0F, and the
> "show in u/i" bit caused Windows to show it in the device manager with
> a yellow excalmation sign... So I had to go back and add an explicit
> _STA with the u/i bit turned off.

Ah okay. So when you wrote "re-adding _STA set to 0x08", you actually
meant *this* patch. Right? (I don't really understand the reference to
0x08.)

So I take you tested *this* patch with Windows 7, and it was satisfied.
Good.

> 
>> (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?)
> 
> First, to answer mst's question elswhere in this thread, I'm
> working on a kernel sysfs driver that would list fw_cfg blobs in
> sysfs (just like /sys/firmware/dmi/entries/...) so userspace could
> simply "cat" or "cp" the raw blobs.
> 
> GregKH told me to try udev for the friendly path blobname expansion
> (your "I like using find on /sys/firmware..." recommendation). He never
> said anything about ACPI (not sure he would have eventually or not).
> 
> It all started with ardb saying "NAK on arm if you touch the mmio
> registers before checking with DT that you even have a fw_cfg device".
> 
> I sort-of figured I'd better not touch IOport registers either before
> I know I have a fw_cfg device, hence this whole exercise of adding it
> to ACPI. Although I still have to figure out how one would do
> something like
> 
>       if (search_acpi_for_hid("QEMU0002") == NULL)
> 
>               return -ENODEV;
> 
> from a module_init method... Couldn't find any examples (yet) in the
> kernel source, and starting to wonder if maybe this is not how ACPI is
> supposed to work, and somehow ACPI initialization itself is somehow
> expected to trigger loading modules for devices it encounters...
> 
> Obviously, since sun4* and ppc/mac have neither DT nor ACPI, this will
> have to be limited to x86 and arm, but OK...
> 
> Dividing the overall problem into smaller, independently
> comprehensible bits doesn't seem to be working out all that
> great for me, so far... In Soviet Russia, problem divide-and-conquer YOU!
> :)
> 
> At least we're getting a documented reservation of whatever mmio or
> ioport region is occupied by fw_cfg in ACPI, so either way I think
> it's worth it (whether it ends up helping me with my long term goals
> or not :)

Unfortunately I can't help you at all with kernel driver development,
but I do sense a kind of dependency hell here (maybe even with a cycle
in it) where whatever you try to implement, someone says "please do X
first". :/

Thanks
Laszlo

> Thanks much,
> --Gabriel
>