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 07:28:39PM +0200, Laszlo Ersek wrote:
> 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.

Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my
brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for
the confusion... :)

Thanks,
--Gabriel

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