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 <so...@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 <ler...@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?
>
> 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
>
