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 <address@hidden> wrote:

> On 03/31/2016 10:07 AM, Igor Mammedov wrote:
> > 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
> 
> 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.
> >
> 
>