diff mbox

[1/2] acpi: tpm: Fix TPM ACPI description (BZ 1281413)

Message ID 1459733876-22908-2-git-send-email-stefanb@us.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stefan Berger April 4, 2016, 1:37 a.m. UTC
This patch addresses BZ 1281413.

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

Fix the APCI description to make it work on operating systems that are
more strict about the contents of the TPM's ACPI description than Linux
is. The ACPI description was broken in commit 9e472263.

We roll back the ACPI description to where it was in QEMU 2.3.1 and
deactivate the interrupt, modify the scope to \_SB, and change the
name of the device back to 'TPM' from 'ISA.TPM'. Here's the ACPI
description from QEMU 2.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}
            })
            Method (_STA, 0, NotSerialized) {
                Return (0x0F)
            }
        }
    }

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/i386/acpi-build.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

Comments

Michael S. Tsirkin April 4, 2016, 9:04 a.m. UTC | #1
On Sun, Apr 03, 2016 at 09:37:55PM -0400, Stefan Berger wrote:
> This patch addresses BZ 1281413.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1281413
> 
> Fix the APCI description to make it work on operating systems that are
> more strict about the contents of the TPM's ACPI description than Linux
> is. The ACPI description was broken in commit 9e472263.
> 
> We roll back the ACPI description to where it was in QEMU 2.3.1 and
> deactivate the interrupt, modify the scope to \_SB, and change the
> name of the device back to 'TPM' from 'ISA.TPM'. Here's the ACPI
> description from QEMU 2.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}
>             })
>             Method (_STA, 0, NotSerialized) {
>                 Return (0x0F)
>             }
>         }
>     }
> 
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

Can we instead use Igor's patch
    pc: acpi: tpm: add missing MMIO resource to PCI0._CRS
and only drop the irq_no_flags value?


> ---
>  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 35180ef..e11c721 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2335,22 +2335,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.5.5
Stefan Berger April 4, 2016, 10:17 a.m. UTC | #2
On 04/04/2016 05:04 AM, Michael S. Tsirkin wrote:
> On Sun, Apr 03, 2016 at 09:37:55PM -0400, Stefan Berger wrote:
>> This patch addresses BZ 1281413.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1281413
>>
>> Fix the APCI description to make it work on operating systems that are
>> more strict about the contents of the TPM's ACPI description than Linux
>> is. The ACPI description was broken in commit 9e472263.
>>
>> We roll back the ACPI description to where it was in QEMU 2.3.1 and
>> deactivate the interrupt, modify the scope to \_SB, and change the
>> name of the device back to 'TPM' from 'ISA.TPM'. Here's the ACPI
>> description from QEMU 2.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}
>>              })
>>              Method (_STA, 0, NotSerialized) {
>>                  Return (0x0F)
>>              }
>>          }
>>      }
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Can we instead use Igor's patch
>      pc: acpi: tpm: add missing MMIO resource to PCI0._CRS
> and only drop the irq_no_flags value?

Igor's patch adds this here:

+
+    if (misc->tpm_version != TPM_VERSION_UNSPEC) {
+        aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
+                   TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
+    }


Now we that memory description there twice? I am not sure whether this 
is necessary, but you are the APCI experts.



>
>
>> ---
>>   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 35180ef..e11c721 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -2335,22 +2335,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.5.5
Igor Mammedov April 4, 2016, 11:49 a.m. UTC | #3
On Mon, 4 Apr 2016 06:17:38 -0400
Stefan Berger <stefanb@linux.vnet.ibm.com> wrote:

> On 04/04/2016 05:04 AM, Michael S. Tsirkin wrote:
> > On Sun, Apr 03, 2016 at 09:37:55PM -0400, Stefan Berger wrote:
> >> This patch addresses BZ 1281413.
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1281413
> >>
> >> Fix the APCI description to make it work on operating systems that
> >> are more strict about the contents of the TPM's ACPI description
> >> than Linux is. The ACPI description was broken in commit 9e472263.
> >>
> >> We roll back the ACPI description to where it was in QEMU 2.3.1 and
> >> deactivate the interrupt, modify the scope to \_SB, and change the
> >> name of the device back to 'TPM' from 'ISA.TPM'. Here's the ACPI
> >> description from QEMU 2.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}
> >>              })
> >>              Method (_STA, 0, NotSerialized) {
> >>                  Return (0x0F)
> >>              }
> >>          }
> >>      }
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > Can we instead use Igor's patch
> >      pc: acpi: tpm: add missing MMIO resource to PCI0._CRS
> > and only drop the irq_no_flags value?
> 
> Igor's patch adds this here:
> 
> +
> +    if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> +        aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> +                   TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> +    }
> 
> 
> Now we that memory description there twice? I am not sure whether
> this is necessary, but you are the APCI experts.
Since TPM_TIS_ADDR_BASE is outside of PCI window of PCI0, we need to
add it explicitly to PCI0._CRS.
Moving out of scope is plainly wrong and works just by accident.

Granted that Michael is fine with hiding IRQ and provided
that you'll fix IRQ handling later, I won't object commenting
IRQ entry for 2.6.

> 
> 
> 
> >
> >
> >> ---
> >>   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 35180ef..e11c721 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -2335,22 +2335,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.5.5
>
diff mbox

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 35180ef..e11c721 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2335,22 +2335,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);
     }