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

[Gabriel L. Somlo]

On Thu, Feb 11, 2016 at 04:19:59PM +0100, Igor Mammedov wrote:
> On Wed, 10 Feb 2016 15:41:38 -0500
> "Gabriel L. Somlo" <address@hidden> wrote:
> 
> > Add a fw_cfg device node to the ACPI SSDT. While the guest-side
> > firmware 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.
> subj and commit msg:
> s/SSDT/DSDT/

Thanks for catching that, got it lined up for v8 :)

> > 
> > Signed-off-by: Gabriel Somlo <address@hidden>
> > Reviewed-by: Laszlo Ersek <address@hidden>
> > Reviewed-by: Marc Marí <address@hidden>
> > ---
> >  hw/i386/acpi-build.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 4554eb8..4762fd2 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2190,6 +2190,35 @@ build_dsdt(GArray *table_data, GArray *linker,
> >      aml_append(scope, aml_name_decl("_S5", pkg));
> >      aml_append(dsdt, scope);
> >  
> > +    /* create fw_cfg node, unconditionally */
> Will that unconditionally make all Windows guests ask for driver for unknown 
> device?

That didn't happen in my tests. With _STA set to 0xB, we have the
bit representing "device shown in the UI" set to "off", and so Windows
(XP and Win7 in my tests) completely ignore it.

Originally I was asked (by Eduardo, IIRC) to make insertion of the
fw_cfg acpi node conditional on the machine version, but then later we
collectively agreed to no longer require that, so there wasn't an if (...)
condition to put in front of the { ... } block anymore. I replaced the
condition with the comment that says "add unconditionally"; I can change
the wording on that if it's confusing, but I'd like to keep the extra
curly braces to match the way all other surrounding ACPI nodes are added.

Thanks,
--Gabriel

> > +    {
> > +        /* 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; when using DMA, the
> > +         * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
> > +        uint8_t io_size = object_property_get_bool(OBJECT(pcms->fw_cfg),
> > +                                                   "dma_enabled", NULL) ?
> > +                          ROUND_UP(FW_CFG_CTL_SIZE, 4) + 
> > sizeof(dma_addr_t) :
> > +                          FW_CFG_CTL_SIZE;
> > +
> > +        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();
> > +        aml_append(crs,
> > +            aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, 
> > io_size)
> > +        );
> > +        aml_append(dev, aml_name_decl("_CRS", crs));
> > +
> > +        aml_append(scope, dev);
> > +        aml_append(dsdt, scope);
> > +    }
> > +
> >      if (misc->applesmc_io_base) {
> >          scope = aml_scope("\\_SB.PCI0.ISA");
> >          dev = aml_device("SMC");
>