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

[Gabriel L. Somlo]

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.

> (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 :)

Thanks much,
--Gabriel