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

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

[Stefan Berger]

This patch addresses BZ 1281413.

Fix the APCI description to make it work on Windows again. The ACPI
description was broken in commit 9e47226.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 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);
             }
         }
+
+        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));
+            aml_append(dev, aml_name_decl("_CRS", crs));
+            aml_append(sb_scope, dev);
+        }
         aml_append(dsdt, sb_scope);
     }
 
-- 
2.4.3

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

[Cole Robinson]

On 03/21/2016 10:21 AM, Stefan Berger wrote:
> This patch addresses BZ 1281413.
> 

That is, https://bugzilla.redhat.com/show_bug.cgi?id=1281413 . Fedora 23 bug
report from a TPM user

> Fix the APCI description to make it work on Windows again. The ACPI
> description was broken in commit 9e47226.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  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);
>              }
>          }
> +
> +        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));
> +            aml_append(dev, aml_name_decl("_CRS", crs));
> +            aml_append(sb_scope, dev);
> +        }
>          aml_append(dsdt, sb_scope);
>      }
>  
> 

After this, TPM shows up as functional in windows 10 device manager. Prior to
this patch it had a warning icon.

Tested-by: Cole Robinson <crobinso@redhat.com>

- Cole

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

[Igor Mammedov]

On Mon, 21 Mar 2016 10:21:11 -0400
Stefan Berger <stefanb@us.ibm.com> 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.

Current commit message is pretty useless, Pls describe in commit
message what/how is broken and how/why patch fixes it.

> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  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.

>              }
>          }
> +
> +        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.


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

[Qemu-devel] [PATCH] pc: acpi: tpm: add missing MMIO resource to PCI0._CRS

[Igor Mammedov]

Windows will fail it initialize TMP driver with the reason:
  'device cannot find enough free resources'
That happens because parent BUS doesn't describe
MMIO resources used by TPM child device.
Fix it by describing it in top-most parent bus scope PCI0.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 35180ef..784870e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2167,6 +2167,11 @@ build_dsdt(GArray *table_data, GArray *linker,
                              0, pci->w64.begin, pci->w64.end - 1, 0,
                              pci->w64.end - pci->w64.begin));
     }
+
+    if (misc->tpm_version != TPM_VERSION_UNSPEC) {
+        aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
+                   TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
+    }
     aml_append(scope, aml_name_decl("_CRS", crs));
 
     /* reserve GPE0 block resources */
-- 
1.8.3.1

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

[Stefan Berger]

On 03/30/2016 09:33 AM, Igor Mammedov wrote:
> On Mon, 21 Mar 2016 10:21:11 -0400
> Stefan Berger <stefanb@us.ibm.com> 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 <stefanb@linux.vnet.ibm.com>
>> ---
>>   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?

>>               }
>>           }
>> +
>> +        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.

     Stefan

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

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 <stefanb@linux.vnet.ibm.com> wrote:

> On 03/30/2016 09:33 AM, Igor Mammedov wrote:
> > On Mon, 21 Mar 2016 10:21:11 -0400
> > Stefan Berger <stefanb@us.ibm.com> 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 <stefanb@linux.vnet.ibm.com>
> >> ---
> >>   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);
> >>       }
> >>   
> >
> 

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 <stefanb@linux.vnet.ibm.com> wrote:

> On 03/30/2016 09:33 AM, Igor Mammedov wrote:
> > On Mon, 21 Mar 2016 10:21:11 -0400
> > Stefan Berger <stefanb@us.ibm.com> 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.

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

[Stefan Berger]

On 03/31/2016 10:07 AM, Igor Mammedov wrote:
> On Thu, 31 Mar 2016 00:03:57 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> On 03/30/2016 09:33 AM, Igor Mammedov wrote:
>>> On Mon, 21 Mar 2016 10:21:11 -0400
>>> Stefan Berger <stefanb@us.ibm.com> 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

On Linux I can use it with IRQ 5. I have tried with 10, it also works. 
But Linux != Windows.


> 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 if we disable interrupt, as done with this patch here, it works. This 
was the configuration we had in QEMU 2.3.1 and was reported as working here:

https://bugzilla.redhat.com/show_bug.cgi?id=1281413

So I would repost the patch with a better description of what we are 
rolling back. Let me know whether you agree. We can then enable the 
interrupt mode in separate patches.



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

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

[Igor Mammedov]

On Fri, 1 Apr 2016 14:50:43 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 03/31/2016 10:07 AM, Igor Mammedov wrote:
> > On Thu, 31 Mar 2016 00:03:57 -0400
> > Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >
> >> On 03/30/2016 09:33 AM, Igor Mammedov wrote:
> >>> On Mon, 21 Mar 2016 10:21:11 -0400
> >>> Stefan Berger <stefanb@us.ibm.com> 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
> 
> On Linux I can use it with IRQ 5. I have tried with 10, it also
> works. But Linux != Windows.
Linux has less strict checks on conflicts (but it slowly improves in
that direction).

> 
> 
> > 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 if we disable interrupt, as done with this patch here, it works.
> This was the configuration we had in QEMU 2.3.1 and was reported as
> working here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1281413
> 
> So I would repost the patch with a better description of what we are 
> rolling back. Let me know whether you agree. We can then enable the 
> interrupt mode in separate patches.
I don't think that it's correct to remove the interrupt from _CRS,
that just hides the issue.
BTW: irqs 5,10,11 might be used by PCI links so they probably shouldn't
be used by TPM (I'm not sure if they could be shared).


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

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

[Michael S. Tsirkin]

On Fri, Apr 01, 2016 at 02:50:43PM -0400, Stefan Berger wrote:
> On 03/31/2016 10:07 AM, Igor Mammedov wrote:
> >On Thu, 31 Mar 2016 00:03:57 -0400
> >Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
> >
> >>On 03/30/2016 09:33 AM, Igor Mammedov wrote:
> >>>On Mon, 21 Mar 2016 10:21:11 -0400
> >>>Stefan Berger <stefanb@us.ibm.com> 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
> 
> On Linux I can use it with IRQ 5. I have tried with 10, it also works. But
> Linux != Windows.
> 
> 
> >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 if we disable interrupt, as done with this patch here, it works. This was
> the configuration we had in QEMU 2.3.1 and was reported as working here:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1281413
> 
> So I would repost the patch with a better description of what we are rolling
> back. Let me know whether you agree. We can then enable the interrupt mode
> in separate patches.

I'm fine with a revert as a first step. But really we should avoid
asserting the interrupt for no-interrupt mode and obey what user
specified.


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

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

[Marcel Apfelbaum]

On 03/31/2016 05:07 PM, Igor Mammedov wrote:
> On Thu, 31 Mar 2016 00:03:57 -0400
> Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:
>
>> On 03/30/2016 09:33 AM, Igor Mammedov wrote:
>>> On Mon, 21 Mar 2016 10:21:11 -0400
>>> Stefan Berger <stefanb@us.ibm.com> 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)

Sorry, I have no idea.
Thanks,
Marcel

>   2: ACPI should pick up IRQ from tpm-tis device property, i.e. it
>      shouldn't be hardcoded in ACPI part.
>