From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44813) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCWSO-0007vU-H4 for qemu-devel@nongnu.org; Wed, 30 Jul 2014 12:10:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XCWSJ-00042B-NZ for qemu-devel@nongnu.org; Wed, 30 Jul 2014 12:10:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7291) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XCWSJ-000423-DB for qemu-devel@nongnu.org; Wed, 30 Jul 2014 12:10:19 -0400 Date: Wed, 30 Jul 2014 18:10:33 +0200 From: "Michael S. Tsirkin" Message-ID: <20140730161033.GF27451@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> <53D91216.1030800@redhat.com> <53D9163C.6000304@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 12:03:46PM -0400, Stefan Berger wrote: > Laszlo Ersek wrote on 07/30/2014 11:58:52 AM: >=20 > > From: Laszlo Ersek > > To: Stefan Berger/Watson/IBM@IBMUS > > Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, > > Stefan Berger > > Date: 07/30/2014 11:59 AM > > Subject: Re: [PATCH v2] Add ACPI tables for TPM > > > > On 07/30/14 17:44, Stefan Berger wrote: > > > Laszlo Ersek wrote on 07/30/2014 11:41:10 AM: > > > > > >> From: Laszlo Ersek > > >> To: Stefan Berger/Watson/IBM@IBMUS, "Michael S. Tsirkin" > > >> Cc: qemu-devel@nongnu.org, Stefan Berger > > >> Date: 07/30/2014 11:41 AM > > >> Subject: Re: [PATCH v2] Add ACPI tables for TPM > > >> > > >> On 07/30/14 17:29, 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, St= efan > > >> >> 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= AM: > > >> >> > > > >> >> > > 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 w= rote: > > >> >> > > >> From: Stefan Berger > > >> >> > > >> > > >> >> > > >> Add an SSDT ACPI table for the TPM device. > > >> >> > > >> Add a TCPA table for BIOS logging area when a TPM is bei= ng used. > > >> >> > > >> > > >> >> > > >> The latter follows this spec here: > > >> >> > > >> > > >> >> > > >> http://www.trustedcomputinggroup.org/files/static_page_f= iles/ > > >> >> > > 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_MINIMUM_SIZE; > > >> >> > > >> + uint64_t log_area_start_address; > > >> >> > > >> + size_t len =3D log_area_minimum_length + sizeof(*tc= pa); > > >> >> > > >> + > > >> >> > > >> + 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 AC= PI > > >> > table. It > > >> >> > > > works because bios happens to allocate tables from high m= emory. > > >> >> > > > 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,= ever .. > > >> >> > > > > > >> >> > > > > > >> >> > > > > > >> >> > > >> + bios_linker_loader_add_pointer(linker, > > > ACPI_BUILD_TABLE_FILE, > > >> >> > > >> + ACPI_BUILD_TABLE_FIL= E, > > >> >> > > >> + 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 descr= ibes > > > three > > >> >> > > ACPI tables: two (client vs. server) for TPM 1.2, and a thi= rd 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, = might > > >> >> > > allocate the TCPA table in an area that is unsuitable as LA= SA > > > target). > > >> >> > > > > >> >> > > Rather, in OVMF this won't work because OVMF doesn't implem= ent the > > >> >> > > linking part of the linker. The *generic* edk2 protocol > > >> >> > > (EFI_ACPI_TABLE_PROTOCOL, which is coded outside of OVMF) t= hat > > >> > OVMF uses > > >> >> > > (as a client) to install ACPI tables in guest-phys memory r= equires > > >> >> > > tables to be passed in one-by-one. > > >> >> > > > > >> >> > > The EFI_ACPI_TABLE_PROTOCOL implementation in edk2 handles = *some* > > >> >> > > well-known tables specially. It has knowledge of their inte= rnal > > >> >> > > pointers, and when you install an ACPI table, > > > EFI_ACPI_TABLE_PROTOCOL > > >> >> > > updates pointers automatically. (For example when you insta= ll 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 in= stall it > > >> >> > > right now, but TCPA.LASA will be bogus. > > >> >> > > > > >> >> > > If I wanted to implement the complete linker as Michael > > > envisioned it, > > >> >> > > then I'd have to avoid edk2's EFI_ACPI_TABLE_PROTOCOL, and > > > implement > > >> >> > > ACPI table installation from zero, trying to mimic the SeaB= IOS > > > client > > >> >> > > code, but in a way that matches the UEFI environment. I'm n= ot > > > ready to > > >> >> > > do that. Definitely not without an "official" human-languag= e > > >> >> > > specification of the linker-loader interface. > > >> >> > > > > >> >> > > I skimmed the patch but I'm not sure what exactly the TPM > > > emulation in > > >> >> > > qemu depends on. Is it a command line option? Is it default= for > > > 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'. = The > > >> >> 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_F= ILE, =20 > > >> >> > ACPI_BUILD_TABLE_FILE, > > >> >> > table_data, &fadt->firmwar= e_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 wou= ld need > > >> > a similar tag to keep the S3 resume vector around? > > >> > > >> Not in OVMF, because edk2's EFI_ACPI_TABLE_PROTOCOL special cases = FACS > > >> (containing the S3 resume vector), allocating it in EfiACPIMemoryN= VS > > > memory. > > >> > > >> Table 26. Memory Type Usage after ExitBootServices() > > >> EfiACPIMemoryNVS: This memory is to be preserved by the loader and= OS > > >> in the working and ACPI S1=E2=80=93S3 states. > > >> > > > > > > So what is a solution then for OVMF? Add another special case for T= CPA? > > > Is this counter to the specs ? Skip TCPA? > > > > In the short term, probably skip TCPA, or advise users in documentati= on > > not to enable the TPM device when running OVMF. Hmm but doesn't OVMF rely on all tables being packed without holes in memory? I remember it did, and if it still does, this breaks unless TCPA is the last ... >=20 > I guess we can do that. You can skip the TCPA for now; once the UEFI ha= s TPM > support, this table would then be needed. >=20 > Stefan If it doesn't work anyway, we can just tell people not to enable TPM with OVMF. No need for hack in OVMF to skip it. --=20 MST