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}
>              })
The first committed variant has IRQ uncommented, so it was always
present in _CRS (commit 9e47226)


[...]
> >> +            //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.
I've looked through code some more and Windows seems to be right in not
starting device as IRQ 5 might be used by PNP0C0F devices, see
build_link_dev(). So potential conflict is there and moving TPM device
out of PCI.ISA scope just hides problem as resource conflict
checking doesn't work anymore.

Current TPM code have 2 issues:
 1: it uses wrong IRQ, (I've tried with unused COM2's IRQ3 and warning
    goes away)
 2: IRQ is hardcoded i.e. _CRS always has TPM_TIS_IRQ value, regardless
    of what user specified at command line, for example:
       -device tpm-tis,irq=3
    has no effect on ACPI part

So to fix it correctly on top of my patch:
 1: default IRQ shouldn't be 5 (CCing Marcel, may be he has an idea
    what it should be)
 2: ACPI should pick up IRQ from tpm-tis device property, i.e. it
    shouldn't be hardcoded in ACPI part.