From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43881) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCWNu-0002qo-Si for qemu-devel@nongnu.org; Wed, 30 Jul 2014 12:05:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCWNp-0002Zr-QQ for qemu-devel@nongnu.org; Wed, 30 Jul 2014 12:05:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37650) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCWNp-0002Zh-Hz for qemu-devel@nongnu.org; Wed, 30 Jul 2014 12:05:41 -0400 Date: Wed, 30 Jul 2014 18:05:56 +0200 From: "Michael S. Tsirkin" Message-ID: <20140730160556.GD27451@redhat.com> References: <1406631139-6754-1-git-send-email-stefanb@us.ibm.com> <20140730132027.GA26025@redhat.com> <53D902F6.1050106@redhat.com> <20140730152040.GE26313@redhat.com> <20140730155036.GA27451@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] Add ACPI tables for TPM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: Laszlo Ersek , qemu-devel@nongnu.org, Stefan Berger On Wed, Jul 30, 2014 at 11:59:43AM -0400, Stefan Berger wrote: > "Michael S. Tsirkin" wrote on 07/30/2014 11:50:36 AM: >=20 > > From: "Michael S. Tsirkin" > > To: Stefan Berger/Watson/IBM@IBMUS > > Cc: Laszlo Ersek , qemu-devel@nongnu.org, Stefan > > Berger > > Date: 07/30/2014 11:50 AM > > Subject: Re: [PATCH v2] Add ACPI tables for TPM > > > > On Wed, Jul 30, 2014 at 11:29:36AM -0400, Stefan Berger wrote: > > > "Michael S. Tsirkin" wrote on 07/30/2014 11:20:41 = AM: > > > > > > > From: "Michael S. Tsirkin" > > > > To: Stefan Berger/Watson/IBM@IBMUS > > > > Cc: Laszlo Ersek , qemu-devel@nongnu.org, Stef= an > > > > Berger > > > > Date: 07/30/2014 11:20 AM > > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM > > > > > > > > On Wed, Jul 30, 2014 at 11:10:27AM -0400, Stefan Berger wrote: > > > > > Laszlo Ersek wrote on 07/30/2014 10:36:38 A= M: > > > > > > > > > > > From: Laszlo Ersek > > > > > > To: "Michael S. Tsirkin" , Stefan Berger/ > > Watson/IBM@IBMUS > > > > > > Cc: qemu-devel@nongnu.org, Stefan Berger > > > > > > Date: 07/30/2014 10:36 AM > > > > > > Subject: Re: [PATCH v2] Add ACPI tables for TPM > > > > > > > > > > > > On 07/30/14 15:20, Michael S. Tsirkin wrote: > > > > > > > On Tue, Jul 29, 2014 at 06:52:19AM -0400, Stefan Berger wro= te: > > > > > > >> From: Stefan Berger > > > > > > >> > > > > > > >> Add an SSDT ACPI table for the TPM device. > > > > > > >> Add a TCPA table for BIOS logging area when a TPM is being= used. > > > > > > >> > > > > > > >> The latter follows this spec here: > > > > > > >> > > > > > > >> http://www.trustedcomputinggroup.org/files/static_page_fil= es/ > > > > > > DCD4188E-1A4B-B294-D050A155FB6F7385/ > > > > > > TCG_ACPIGeneralSpecification_PublicReview.pdf > > > > > > > > > > > > (Thanks for CC'ing me, Michael.) > > > > > > > > > > > > I skimmed this spec. > > > > > > > > > > > > >> +static void > > > > > > >> +build_tpm_tcpa(GArray *table_data, GArray *linker) > > > > > > >> +{ > > > > > > >> + Acpi20Tcpa *tcpa; > > > > > > >> + uint32_t log_area_minimum_length =3D TPM_LOG_AREA_MIN= IMUM_SIZE; > > > > > > >> + uint64_t log_area_start_address; > > > > > > >> + size_t len =3D log_area_minimum_length + sizeof(*tcpa= ); > > > > > > >> + > > > > > > >> + log_area_start_address =3D table_data->len + sizeof(*= tcpa); > > > > > > >> + > > > > > > >> + tcpa =3D acpi_data_push(table_data, len); > > > > > > >> + > > > > > > >> + tcpa->platform_class =3D cpu_to_le16 > > (TPM_TCPA_ACPI_CLASS_CLIENT); > > > > > > >> + tcpa->log_area_minimum_length =3D cpu_to_le32 > > > > (log_area_minimum_length); > > > > > > >> + tcpa->log_area_start_address =3D cpu_to_le64 > > > > (log_area_start_address); > > > > > > >> + > > > > > > >> + /* LASA address to be filled by Guest linker */ > > > > > > > > > > > > > > Hmm, you are simply allocating log area as part of the > > ACPI table. It > > > > > > > works because bios happens to allocate tables from high mem= ory. > > > > > > > But I think this is a problem in practice because > > > > > > > bios is allowed to allocate acpi memory differently. > > > > > > > On the other hand log presumably needs to reside in > > > > > > > physical memory somewhere. > > > > > > > > > > > > > > If you need bios to allocate this memory, then we will > > > > > > > need a new allocation type for this, add it to linker > > > > > > > in bios and qemu. > > > > > > > > > > > > > > Alternatively, find some other way to get hold of > > > > > > > physical memory. > > > > > > > Is there a way to disable the log completely? > > > > > > > As defined in your patch, I doubt there's anything there, e= ver .. > > > > > > > > > > > > > > > > > > > > > > > > > > > >> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TAB= LE_FILE, > > > > > > >> + ACPI_BUILD_TABLE_FILE, > > > > > > >> + table_data, > > > > > > &tcpa->log_area_start_address, > > > > > > >> + sizeof > > > > (tcpa->log_area_start_address)); > > > > > > >> + build_header(linker, table_data, > > > > > > >> + (void *)tcpa, "TCPA", sizeof(*tcpa), 2); > > > > > > >> +} > > > > > > > > > > > > So here's my understanding. The spec referenced above describ= es three > > > > > > ACPI tables: two (client vs. server) for TPM 1.2, and a third= one > > > > > > (usable by both client & server platforms) for TPM 2.0. > > > > > > > > > > > > The code above prepares a TPM 1.2 table. (Signature: "TCPA".) > > > > > > > > > > > > This table has a field called LASA (Log Area Start Address) > > which points > > > > > > to somewhere in (guest-)physical memory. The patch adds a > > "dummy range" > > > > > > to the end of the TCPA table itself, and asks the linker to > > set LASA to > > > > > > the beginning of that range. > > > > > > > > > > > > This won't work in OVMF, and not just because of the reason > > that Michael > > > > > > mentions (ie. because the firmware, in particular SeaBIOS, mi= ght > > > > > > allocate the TCPA table in an area that is unsuitable as LASA > target). > > > > > > > > > > > > Rather, in OVMF this won't work because OVMF doesn't implemen= t the > > > > > > linking part of the linker. The *generic* edk2 protocol > > > > > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) > > that OVMF uses > > > > > > (as a client) to install ACPI tables in guest-phys memory req= uires > > > > > > tables to be passed in one-by-one. > > > > > > > > > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles *s= ome* > > > > > > well-known tables specially. It has knowledge of their intern= al > > > > > > pointers, and when you install an ACPI table, EFI_ACPI_TABLE_= PROTOCOL > > > > > > updates pointers automatically. (For example when you > > install the FACS, > > > > > > the protocol links it automatically into FACP.) > > > > > > > > > > > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 doesn't > > seem to know > > > > > > anything about the TCPA table, let alone the unstructured > > (?) TCG event > > > > > > log that is pointed-to by TCPA.LASA. > > > > > > > > > > > > (I grepped for the TCPA signature, > > > > > > > > > > > > > > > > > > > > > EFI_ACPI_5_0_TRUSTED_COMPUTING_PLATFORM_ALLIANCE_CAPABILITIES_TABLE_SIG= NATURE.) > > > > > > > > > > > > This means that if you pass down a TCPA table, OVMF will inst= all it > > > > > > right now, but TCPA.LASA will be bogus. > > > > > > > > > > > > If I wanted to implement the complete linker as Michael envis= ioned > it, > > > > > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and im= plement > > > > > > ACPI table installation from zero, trying to mimic the SeaBIO= S client > > > > > > code, but in a way that matches the UEFI environment. I'm not= ready > to > > > > > > do that. Definitely not without an "official" human-language > > > > > > specification of the linker-loader interface. > > > > > > > > > > > > I skimmed the patch but I'm not sure what exactly the TPM emu= lation > in > > > > > > qemu depends on. Is it a command line option? Is it default f= or some > > > > > > machine types? > > > > > > > > > > > > Alternatively, I could recognize the TCPA signature in OVMF > > when parsing > > > > > > the ACPI blobs for table headers, and filter it out. > > > > > > > > > > This is the code for what I would call 'pointer relocation'. Th= e > > > > TCPA table is > > > > > not the only place where this is used, but why is it an issue > > > > there while not > > > > > with the following? > > > > > > > > > > fadt->firmware_ctrl =3D cpu_to_le32(facs); > > > > > /* FACS address to be filled by Guest linker */ > > > > > bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FIL= E, =20 > > > > > ACPI_BUILD_TABLE_FILE, > > > > > table_data, &fadt->firmware_= ctrl, > > > > > sizeof fadt->firmware_ctrl); > > > > > > > > > > Regards, > > > > > Stefan > > > > > > > > > > > > Becase FACS is an ACPI table. So BIOS allocates it > > > > from E820_RESERVED at the moment but it does not have to, > > > > it could mark it with E820_ACPI. > > > > Guest can then interpret the tables and then release the > > > > memory if it wishes. > > > > > > > > If you want to do it for TCPA you must tell bios that > > > > this is not ACPI memory. > > > > > > I see. Presumably the whole slew of FADT, FACS, RSDP, & RSDT would = need a > > > similar tag to keep the S3 resume vector around? > > > > > > Stefan > > > > Interesting, good point. > > Yes ACPI spec says > > The BIOS aligns the FACS on a 64-byte boundary anywhere within the > > system=E2=80=99s memory address space. The memory where the FACS s= tructure > > resides must not be reported as system AddressRangeMemory in the s= ystem > > address map. For example, the E820 address map reporting interface= would > > report the region as AddressRangeReserved. For more information ab= out > > system address map reporting interfaces, see Section 15, =E2=80=9C= System Address > > Map Interfaces.=E2=80=9D > > > > I don't see where would the requirement for other tables come from. > > Can you clarify please? > > >=20 > Looking at SeaBIOS: >=20 > resume.c::s3_resume() calls find_resume_vector() >=20 > fw/biostables::find_resume_vector() calls find_fadt() >=20 > fw/biostables::find_fadt() accesses the RSDP then the RSDT then travers= es its > table of pointers to other ACPI tables and checks all their signatures = until > the FACP_SIGNATURE is found. >=20 > fw/biostables::find_resume_vector() then accesses the FACS and takes th= e > firmware_waking_vector from it. >=20 > Had any of the tables been deallocated, S3 resume wouldn't work anymore. > Besides that the signature checking wouldn't be all that great if the m= emory > was now used for something else. > Stefan Well this works because it reserves all tables. If seabios wanted to stop doing this, I guess it would have to stop looking for these things, instead store FACS address somewhere else in reserved memory. But nothing says it can't right? --=20 MST