Re: [Qemu-devel] [PATCH] acpi: Fix TPM ACPI description to make TPM usable on Windows

[Igor Mammedov]

On Thu, 31 Mar 2016 00:03:57 -0400
Stefan Berger <address@hidden> wrote:

> On 03/30/2016 09:33 AM, Igor Mammedov wrote:
> > On Mon, 21 Mar 2016 10:21:11 -0400
> > Stefan Berger <address@hidden> wrote:
> >
> >> This patch addresses BZ 1281413.
> >>
> >> Fix the APCI description to make it work on Windows again. The ACPI
> >> description was broken in commit 9e47226.
> > above commit just added missing ASL description for TMP device
> > and you also posted exactly similar patch back then.
> 
> Sorry, I referenced the wrong commit. Here's the bad one:
> 
> commit 72d97b3a543a9c2c820bd463ba24751ae4247ac3
> 
> >
> > Current commit message is pretty useless, Pls describe in commit
> > message what/how is broken and how/why patch fixes it.
> 
> I am not sure what exactly broke it. All I know is that the scope was 
> changed and the device name were change (ISA.TPM versus TPM). This
> was the working ACPI description from QEMU v2.3.1:
> 
>      Scope(\_SB) {
>          /* TPM with emulated TPM TIS interface */
>          Device (TPM) {
>              Name (_HID, EisaID ("PNP0C31"))
>              Name (_CRS, ResourceTemplate ()
>              {
>                  Memory32Fixed (ReadWrite, TPM_TIS_ADDR_BASE, 
> TPM_TIS_ADDR_SIZE)
>                  // older Linux tpm_tis drivers do not work with IRQ
>                  //IRQNoFlags () {TPM_TIS_IRQ}
>              })
> 
> 
> >
> >> Signed-off-by: Stefan Berger <address@hidden>
> >> ---
> >>   hw/i386/acpi-build.c | 26 ++++++++++++--------------
> >>   1 file changed, 12 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 325d8ce..c6e90b6 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2334,22 +2334,20 @@ build_dsdt(GArray *table_data, GArray
> >> *linker, Aml *scope = aml_scope("PCI0");
> >>                   /* Scan all PCI buses. Generate tables to support
> >> hotplug. */ build_append_pci_bus_devices(scope, bus,
> >> pm->pcihp_bridge_en); -
> >> -                if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> >> -                    dev = aml_device("ISA.TPM");
> >> -                    aml_append(dev, aml_name_decl("_HID",
> >> aml_eisaid("PNP0C31")));
> >> -                    aml_append(dev, aml_name_decl("_STA",
> >> aml_int(0xF)));
> >> -                    crs = aml_resource_template();
> >> -                    aml_append(crs,
> >> aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> >> -                               TPM_TIS_ADDR_SIZE,
> >> AML_READ_WRITE));
> >> -                    aml_append(crs,
> >> aml_irq_no_flags(TPM_TIS_IRQ));
> >> -                    aml_append(dev, aml_name_decl("_CRS", crs));
> >> -                    aml_append(scope, dev);
> >> -                }
> >> -
> >> -                aml_append(sb_scope, scope);
> > it's ISA device so it should stay in ISA scope,
> > I'll post and alternative fix for TPM_TIS_ADDR_BASE as follow up
> > to this series.
> 
> Alternative means it doesn't need this patch? Or it needs it and is a 
> follow up on top of it?
Alternative needs an additional patch on top that would fix
IRQ issue one way or another.

> 
> >>               }
> >>           }
> >> +
> >> +        if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> >> +            dev = aml_device("TPM");
> >> +            aml_append(dev, aml_name_decl("_HID",
> >> aml_eisaid("PNP0C31")));
> >> +            aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> >> +            crs = aml_resource_template();
> >> +            aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> >> +                       TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> >> +            //aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ));
> > silent change,
> > why IRQ descriptor is commented out, it seems the device
> > uses/initializes it?
> > I'd split IRQ change in a separate patch that explains why it
> > shouldn't be in TPM._CRS.
> 
> We can leave it there if it works. The bug report is related to
> Windows, which I don't have running in a VM. So the safest was to
> roll back to 2.3.1 equivalent, which had the IRQ disabled.
Windows still complains if IRQ is present in CRS, so we need an
additional patch, on top of the one I've posted, that removes IRQ
preferably explaining why it's not needed there. 

I'm not sure about reason, maybe there is something wrong with TPM
device model or IRQ number (other ISA devices specify IRQs in their
CRS and there aren't any problems with them). 

> 
>      Stefan
> 
> >
> >
> >> +            aml_append(dev, aml_name_decl("_CRS", crs));
> >> +            aml_append(sb_scope, dev);
> >> +        }
> >>           aml_append(dsdt, sb_scope);
> >>       }
> >>   
> >
>