From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1amRKi-0002TD-BA for qemu-devel@nongnu.org; Sat, 02 Apr 2016 15:35:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1amRKY-00009a-Ni for qemu-devel@nongnu.org; Sat, 02 Apr 2016 15:35:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1amRKY-00008z-7K for qemu-devel@nongnu.org; Sat, 02 Apr 2016 15:35:34 -0400 Date: Sat, 2 Apr 2016 21:35:24 +0200 From: Igor Mammedov Message-ID: <20160402213524.4cf76b82@igors-macbook-pro.local> In-Reply-To: <56FEC303.4090504@linux.vnet.ibm.com> References: <1458570071-1935-1-git-send-email-stefanb@us.ibm.com> <20160330153347.302b7d0b@igors-macbook-pro.local> <56FCA1AD.9000507@linux.vnet.ibm.com> <20160331160736.2576b590@igors-macbook-pro.local> <56FEC303.4090504@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] acpi: Fix TPM ACPI description to make TPM usable on Windows List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: marcel@redhat.com, crobinso@redhat.com, Stefan Berger , qemu-devel@nongnu.org, mst@redhat.com On Fri, 1 Apr 2016 14:50:43 -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 wrote: > > > >> On 03/30/2016 09:33 AM, Igor Mammedov wrote: > >>> On Mon, 21 Mar 2016 10:21:11 -0400 > >>> Stefan Berger 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. > > > >