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

[Marcel Apfelbaum <marcel@redhat.com>]

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