| Message ID | 20180515121433.6112-4-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for TPM Physical Presence interface | expand |
On Tue, May 15, 2018 at 02:14:32PM +0200, Marc-André Lureau wrote: > From: Stefan Berger <stefanb@linux.vnet.ibm.com> > > The TPM Physical Presence interface consists of an ACPI part, a shared > memory part, and code in the firmware. Users can send messages to the > firmware by writing a code into the shared memory through invoking the > ACPI code. When a reboot happens, the firmware looks for the code and > acts on it by sending sequences of commands to the TPM. > > This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't > assume that SMIs are necessary to use. It uses a similar datastructure for > the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use > of it. I extended the shared memory data structure with an array of 256 > bytes, one for each code that could be implemented. The array contains > flags describing the individual codes. This decouples the ACPI implementation > from the firmware implementation. > > The underlying TCG specification is accessible from the following page. > > https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ > > This patch implements version 1.30. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > --- > > v4 (Marc-André): > - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI > handling. > - replace 'return Package (..) {} ' with scoped variables, to fix > Windows ACPI handling. > > v3: > - add support for PPI to CRB > - split up OperationRegion TPPI into two parts, one containing > the registers (TPP1) and the other one the flags (TPP2); switched > the order of the flags versus registers in the code > - adapted ACPI code to small changes to the array of flags where > previous flag 0 was removed and now shifting right wasn't always > necessary anymore > > v2: > - get rid of FAIL variable; function 5 was using it and always > returns 0; the value is related to the ACPI function call not > a possible failure of the TPM function call. > - extend shared memory data structure with per-opcode entries > holding flags and use those flags to determine what to return > to caller > - implement interface version 1.3 > --- > include/hw/acpi/tpm.h | 21 +++ > hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 314 insertions(+), 1 deletion(-) > > diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > index f79d68a77a..fc53f08827 100644 > --- a/include/hw/acpi/tpm.h > +++ b/include/hw/acpi/tpm.h > @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) > #define TPM_PPI_VERSION_NONE 0 > #define TPM_PPI_VERSION_1_30 1 > > +struct tpm_ppi { The name violate the coding style. > + uint8_t func[256]; /* 0x000: per TPM function implementation flags; > + set by BIOS */ > +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ > +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) > +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) > +#define TPM_PPI_FUNC_BLOCKED (2 << 0) > +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) > +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) > +#define TPM_PPI_FUNC_MASK (7 << 0) > + uint8_t ppin; /* 0x100 : set by BIOS */ Are you sure it's right? Below ints will all end up misaligned ... > + uint32_t ppip; /* 0x101 : set by ACPI; not used */ > + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ > + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ > + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ > + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ > + uint32_t fret; /* 0x115 : set by ACPI; not used */ > + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ > + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ > +} QEMU_PACKED; > + > #endif /* HW_ACPI_TPM_H */ Igor could you pls take a quick look at the rest?
Hi On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, May 15, 2018 at 02:14:32PM +0200, Marc-André Lureau wrote: >> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >> >> The TPM Physical Presence interface consists of an ACPI part, a shared >> memory part, and code in the firmware. Users can send messages to the >> firmware by writing a code into the shared memory through invoking the >> ACPI code. When a reboot happens, the firmware looks for the code and >> acts on it by sending sequences of commands to the TPM. >> >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >> assume that SMIs are necessary to use. It uses a similar datastructure for >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >> of it. I extended the shared memory data structure with an array of 256 >> bytes, one for each code that could be implemented. The array contains >> flags describing the individual codes. This decouples the ACPI implementation >> from the firmware implementation. >> >> The underlying TCG specification is accessible from the following page. >> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >> >> This patch implements version 1.30. >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> >> --- >> >> v4 (Marc-André): >> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >> handling. >> - replace 'return Package (..) {} ' with scoped variables, to fix >> Windows ACPI handling. >> >> v3: >> - add support for PPI to CRB >> - split up OperationRegion TPPI into two parts, one containing >> the registers (TPP1) and the other one the flags (TPP2); switched >> the order of the flags versus registers in the code >> - adapted ACPI code to small changes to the array of flags where >> previous flag 0 was removed and now shifting right wasn't always >> necessary anymore >> >> v2: >> - get rid of FAIL variable; function 5 was using it and always >> returns 0; the value is related to the ACPI function call not >> a possible failure of the TPM function call. >> - extend shared memory data structure with per-opcode entries >> holding flags and use those flags to determine what to return >> to caller >> - implement interface version 1.3 >> --- >> include/hw/acpi/tpm.h | 21 +++ >> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 314 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >> index f79d68a77a..fc53f08827 100644 >> --- a/include/hw/acpi/tpm.h >> +++ b/include/hw/acpi/tpm.h >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) >> #define TPM_PPI_VERSION_NONE 0 >> #define TPM_PPI_VERSION_1_30 1 >> >> +struct tpm_ppi { > > The name violate the coding style. That's easy to change. Stefan could do it on commit if the rest of the patch is unchanged. > > >> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; >> + set by BIOS */ >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) >> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) >> +#define TPM_PPI_FUNC_MASK (7 << 0) >> + uint8_t ppin; /* 0x100 : set by BIOS */ > > Are you sure it's right? Below ints will all end up misaligned ... Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we change it in qemu, we will have to change it in edk2 as well >> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ >> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ >> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ >> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ >> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ >> + uint32_t fret; /* 0x115 : set by ACPI; not used */ >> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ >> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ >> +} QEMU_PACKED; >> + >> #endif /* HW_ACPI_TPM_H */ > > Igor could you pls take a quick look at the rest? > > -- > MST > thanks
On 06/20/18 16:35, Marc-André Lureau wrote: > Hi > > On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> On Tue, May 15, 2018 at 02:14:32PM +0200, Marc-André Lureau wrote: >>> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> >>> The TPM Physical Presence interface consists of an ACPI part, a shared >>> memory part, and code in the firmware. Users can send messages to the >>> firmware by writing a code into the shared memory through invoking the >>> ACPI code. When a reboot happens, the firmware looks for the code and >>> acts on it by sending sequences of commands to the TPM. >>> >>> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >>> assume that SMIs are necessary to use. It uses a similar datastructure for >>> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >>> of it. I extended the shared memory data structure with an array of 256 >>> bytes, one for each code that could be implemented. The array contains >>> flags describing the individual codes. This decouples the ACPI implementation >>> from the firmware implementation. >>> >>> The underlying TCG specification is accessible from the following page. >>> >>> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >>> >>> This patch implements version 1.30. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> >>> --- >>> >>> v4 (Marc-André): >>> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >>> handling. >>> - replace 'return Package (..) {} ' with scoped variables, to fix >>> Windows ACPI handling. >>> >>> v3: >>> - add support for PPI to CRB >>> - split up OperationRegion TPPI into two parts, one containing >>> the registers (TPP1) and the other one the flags (TPP2); switched >>> the order of the flags versus registers in the code >>> - adapted ACPI code to small changes to the array of flags where >>> previous flag 0 was removed and now shifting right wasn't always >>> necessary anymore >>> >>> v2: >>> - get rid of FAIL variable; function 5 was using it and always >>> returns 0; the value is related to the ACPI function call not >>> a possible failure of the TPM function call. >>> - extend shared memory data structure with per-opcode entries >>> holding flags and use those flags to determine what to return >>> to caller >>> - implement interface version 1.3 >>> --- >>> include/hw/acpi/tpm.h | 21 +++ >>> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 314 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >>> index f79d68a77a..fc53f08827 100644 >>> --- a/include/hw/acpi/tpm.h >>> +++ b/include/hw/acpi/tpm.h >>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) >>> #define TPM_PPI_VERSION_NONE 0 >>> #define TPM_PPI_VERSION_1_30 1 >>> >>> +struct tpm_ppi { >> >> The name violate the coding style. > > That's easy to change. Stefan could do it on commit if the rest of the > patch is unchanged. >> >> >>> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; >>> + set by BIOS */ >>> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ >>> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) >>> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) >>> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) >>> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) >>> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) >>> +#define TPM_PPI_FUNC_MASK (7 << 0) >>> + uint8_t ppin; /* 0x100 : set by BIOS */ >> >> Are you sure it's right? Below ints will all end up misaligned ... > > Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we > change it in qemu, we will have to change it in edk2 as well I don't see why the misalignment is a problem. AIUI functionally it shouldn't be an issue, and performance is not critical. We did make sure the struct was packed in edk2 too. Thanks, Laszlo > >>> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ >>> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ >>> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ >>> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ >>> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ >>> + uint32_t fret; /* 0x115 : set by ACPI; not used */ >>> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ >>> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ >>> +} QEMU_PACKED; >>> + >>> #endif /* HW_ACPI_TPM_H */ >> >> Igor could you pls take a quick look at the rest? >> >> -- >> MST >> > > thanks > >
On Wed, Jun 20, 2018 at 04:35:23PM +0200, Marc-André Lureau wrote: > Hi > > On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, May 15, 2018 at 02:14:32PM +0200, Marc-André Lureau wrote: > >> From: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> > >> The TPM Physical Presence interface consists of an ACPI part, a shared > >> memory part, and code in the firmware. Users can send messages to the > >> firmware by writing a code into the shared memory through invoking the > >> ACPI code. When a reboot happens, the firmware looks for the code and > >> acts on it by sending sequences of commands to the TPM. > >> > >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't > >> assume that SMIs are necessary to use. It uses a similar datastructure for > >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use > >> of it. I extended the shared memory data structure with an array of 256 > >> bytes, one for each code that could be implemented. The array contains > >> flags describing the individual codes. This decouples the ACPI implementation > >> from the firmware implementation. > >> > >> The underlying TCG specification is accessible from the following page. > >> > >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ > >> > >> This patch implements version 1.30. > >> > >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> > >> --- > >> > >> v4 (Marc-André): > >> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI > >> handling. > >> - replace 'return Package (..) {} ' with scoped variables, to fix > >> Windows ACPI handling. > >> > >> v3: > >> - add support for PPI to CRB > >> - split up OperationRegion TPPI into two parts, one containing > >> the registers (TPP1) and the other one the flags (TPP2); switched > >> the order of the flags versus registers in the code > >> - adapted ACPI code to small changes to the array of flags where > >> previous flag 0 was removed and now shifting right wasn't always > >> necessary anymore > >> > >> v2: > >> - get rid of FAIL variable; function 5 was using it and always > >> returns 0; the value is related to the ACPI function call not > >> a possible failure of the TPM function call. > >> - extend shared memory data structure with per-opcode entries > >> holding flags and use those flags to determine what to return > >> to caller > >> - implement interface version 1.3 > >> --- > >> include/hw/acpi/tpm.h | 21 +++ > >> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- > >> 2 files changed, 314 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > >> index f79d68a77a..fc53f08827 100644 > >> --- a/include/hw/acpi/tpm.h > >> +++ b/include/hw/acpi/tpm.h > >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) > >> #define TPM_PPI_VERSION_NONE 0 > >> #define TPM_PPI_VERSION_1_30 1 > >> > >> +struct tpm_ppi { > > > > The name violate the coding style. > > That's easy to change. Stefan could do it on commit if the rest of the > patch is unchanged. > > > > > >> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; > >> + set by BIOS */ > >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ > >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) > >> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) > >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) > >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) > >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) > >> +#define TPM_PPI_FUNC_MASK (7 << 0) > >> + uint8_t ppin; /* 0x100 : set by BIOS */ > > > > Are you sure it's right? Below ints will all end up misaligned ... > > Hmm. Sadly, we didn't noticed when doing the edk2 part either. If we > change it in qemu, we will have to change it in edk2 as well Might be worth it, it's often very slow to access unaligned fields. > >> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ > >> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ > >> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ > >> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ > >> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ > >> + uint32_t fret; /* 0x115 : set by ACPI; not used */ > >> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ > >> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ > >> +} QEMU_PACKED; > >> + > >> #endif /* HW_ACPI_TPM_H */ > > > > Igor could you pls take a quick look at the rest? > > > > -- > > MST > > > > thanks > > > -- > Marc-André Lureau
On 06/20/2018 10:35 AM, Marc-André Lureau wrote: > Hi > > On Wed, Jun 20, 2018 at 4:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> On Tue, May 15, 2018 at 02:14:32PM +0200, Marc-André Lureau wrote: >>> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> >>> The TPM Physical Presence interface consists of an ACPI part, a shared >>> memory part, and code in the firmware. Users can send messages to the >>> firmware by writing a code into the shared memory through invoking the >>> ACPI code. When a reboot happens, the firmware looks for the code and >>> acts on it by sending sequences of commands to the TPM. >>> >>> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >>> assume that SMIs are necessary to use. It uses a similar datastructure for >>> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >>> of it. I extended the shared memory data structure with an array of 256 >>> bytes, one for each code that could be implemented. The array contains >>> flags describing the individual codes. This decouples the ACPI implementation >>> from the firmware implementation. >>> >>> The underlying TCG specification is accessible from the following page. >>> >>> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >>> >>> This patch implements version 1.30. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> >>> --- >>> >>> v4 (Marc-André): >>> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >>> handling. >>> - replace 'return Package (..) {} ' with scoped variables, to fix >>> Windows ACPI handling. >>> >>> v3: >>> - add support for PPI to CRB >>> - split up OperationRegion TPPI into two parts, one containing >>> the registers (TPP1) and the other one the flags (TPP2); switched >>> the order of the flags versus registers in the code >>> - adapted ACPI code to small changes to the array of flags where >>> previous flag 0 was removed and now shifting right wasn't always >>> necessary anymore >>> >>> v2: >>> - get rid of FAIL variable; function 5 was using it and always >>> returns 0; the value is related to the ACPI function call not >>> a possible failure of the TPM function call. >>> - extend shared memory data structure with per-opcode entries >>> holding flags and use those flags to determine what to return >>> to caller >>> - implement interface version 1.3 >>> --- >>> include/hw/acpi/tpm.h | 21 +++ >>> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 314 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >>> index f79d68a77a..fc53f08827 100644 >>> --- a/include/hw/acpi/tpm.h >>> +++ b/include/hw/acpi/tpm.h >>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) >>> #define TPM_PPI_VERSION_NONE 0 >>> #define TPM_PPI_VERSION_1_30 1 >>> >>> +struct tpm_ppi { >> The name violate the coding style. > That's easy to change. Stefan could do it on commit if the rest of the > patch is unchanged. Call it TPMPPIData?
On Tue, 15 May 2018 14:14:32 +0200 Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > From: Stefan Berger <stefanb@linux.vnet.ibm.com> > > The TPM Physical Presence interface consists of an ACPI part, a shared > memory part, and code in the firmware. Users can send messages to the > firmware by writing a code into the shared memory through invoking the > ACPI code. When a reboot happens, the firmware looks for the code and > acts on it by sending sequences of commands to the TPM. > > This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't > assume that SMIs are necessary to use. It uses a similar datastructure for > the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use > of it. I extended the shared memory data structure with an array of 256 > bytes, one for each code that could be implemented. The array contains > flags describing the individual codes. This decouples the ACPI implementation > from the firmware implementation. > > The underlying TCG specification is accessible from the following page. > > https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ > > This patch implements version 1.30. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > > --- > > v4 (Marc-André): > - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI > handling. > - replace 'return Package (..) {} ' with scoped variables, to fix > Windows ACPI handling. > > v3: > - add support for PPI to CRB > - split up OperationRegion TPPI into two parts, one containing > the registers (TPP1) and the other one the flags (TPP2); switched > the order of the flags versus registers in the code > - adapted ACPI code to small changes to the array of flags where > previous flag 0 was removed and now shifting right wasn't always > necessary anymore > > v2: > - get rid of FAIL variable; function 5 was using it and always > returns 0; the value is related to the ACPI function call not > a possible failure of the TPM function call. > - extend shared memory data structure with per-opcode entries > holding flags and use those flags to determine what to return > to caller > - implement interface version 1.3 > --- > include/hw/acpi/tpm.h | 21 +++ > hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 314 insertions(+), 1 deletion(-) > > diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > index f79d68a77a..fc53f08827 100644 > --- a/include/hw/acpi/tpm.h > +++ b/include/hw/acpi/tpm.h > @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) > #define TPM_PPI_VERSION_NONE 0 > #define TPM_PPI_VERSION_1_30 1 > > +struct tpm_ppi { > + uint8_t func[256]; /* 0x000: per TPM function implementation flags; > + set by BIOS */ > +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ > +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) > +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) > +#define TPM_PPI_FUNC_BLOCKED (2 << 0) > +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) > +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) > +#define TPM_PPI_FUNC_MASK (7 << 0) > + uint8_t ppin; /* 0x100 : set by BIOS */ > + uint32_t ppip; /* 0x101 : set by ACPI; not used */ > + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ > + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ > + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ > + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ > + uint32_t fret; /* 0x115 : set by ACPI; not used */ > + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ > + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ > +} QEMU_PACKED; is this structure plant to be used for accessing data from within QEMU or it is just used for convenience of calculating offsets/sizes for AML? > + > #endif /* HW_ACPI_TPM_H */ > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index f6d447f03a..95be4f0710 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -43,6 +43,7 @@ > #include "hw/acpi/memory_hotplug.h" > #include "sysemu/tpm.h" > #include "hw/acpi/tpm.h" > +#include "hw/tpm/tpm_ppi.h" > #include "hw/acpi/vmgenid.h" > #include "sysemu/tpm_backend.h" > #include "hw/timer/mc146818rtc_regs.h" > @@ -1789,6 +1790,292 @@ static Aml *build_q35_osc_method(void) > return method; > } > > +static void > +build_tpm_ppi(Aml *dev) > +{ > + Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak; > + struct tpm_ppi *tpm_ppi = NULL; > + int i; plain AML variables throughout the function make code rather unreadable (well, like any other AML code would be). Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate more or less sanely named variables to make it more understandable. that also should allow to reduce LOC this function takes. > + /* > + * TPP1 is for the flags that indicate which PPI operations > + * are supported by the firmware. The firmware is expected to > + * write these flags. > + */ > + aml_append(dev, > + aml_operation_region("TPP1", AML_SYSTEM_MEMORY, > + aml_int(TPM_PPI_ADDR_BASE), > + sizeof(tpm_ppi->func))); > + field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); > + for (i = 0; i < sizeof(tpm_ppi->func); i++) { > + char *tmp = g_strdup_printf("FN%02X", i); > + aml_append(field, aml_named_field(tmp, BITS_PER_BYTE)); > + g_free(tmp); > + } > + aml_append(dev, field); > + > + /* > + * TPP2 is for the registers that ACPI code used to pass > + * the PPI code and parameter (PPRQ, PPRM) to the firmware. > + */ > + aml_append(dev, > + aml_operation_region("TPP2", AML_SYSTEM_MEMORY, > + aml_int(TPM_PPI_ADDR_BASE + > + offsetof(struct tpm_ppi, ppin)), > + sizeof(struct tpm_ppi) - > + sizeof(tpm_ppi->func))); > + field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); > + aml_append(field, aml_named_field("PPIN", > + sizeof(uint8_t) * BITS_PER_BYTE)); here you already don't use tpm_ppi for sizing, so it creates a confusing mix of both approaches. From ACPI pov I'd prefer PPI table documented somewhere in spec docs (similar docs/specs/acpi_mem_hotplug.txt) and would look like in other use-cases: aml_append(field, aml_named_field("PPIN", 8) and drop "struct tpm_ppi" altogether replacing places it was used by explicit constants. > + aml_append(field, aml_named_field("PPIP", > + sizeof(uint32_t) * BITS_PER_BYTE)); > + aml_append(field, aml_named_field("PPRP", > + sizeof(uint32_t) * BITS_PER_BYTE)); > + aml_append(field, aml_named_field("PPRQ", > + sizeof(uint32_t) * BITS_PER_BYTE)); > + aml_append(field, aml_named_field("PPRM", > + sizeof(uint32_t) * BITS_PER_BYTE)); > + aml_append(field, aml_named_field("LPPR", > + sizeof(uint32_t) * BITS_PER_BYTE)); > + aml_append(dev, field); > + > + method = aml_method("TPFN", 1, AML_SERIALIZED); not quite sure how this method (supposed to ) work(s), it could use nice comment explaining mechanics. > + { > + for (i = 0; i < sizeof(tpm_ppi->func); i++) { > + ifctx = aml_if(aml_equal(aml_int(i), aml_arg(0))); > + { > + aml_append(ifctx, aml_return(aml_name("FN%02X", i))); > + } > + aml_append(method, ifctx); > + } > + aml_append(method, aml_return(aml_int(0))); > + } > + aml_append(dev, method); > + > + pak = aml_package(2); > + aml_append(pak, aml_int(0)); > + aml_append(pak, aml_int(0)); > + name = aml_name_decl("TPM2", pak); > + aml_append(dev, name); > + > + pak = aml_package(3); > + aml_append(pak, aml_int(0)); > + aml_append(pak, aml_int(0)); > + aml_append(pak, aml_int(0)); > + name = aml_name_decl("TPM3", pak); > + aml_append(dev, name); > + > + method = aml_method("_DSM", 4, AML_SERIALIZED); > + { > + uint8_t zerobyte[1] = { 0 }; > + > + ifctx = aml_if( > + aml_equal(aml_arg(0), > + aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653"))); a comment pointing to a specific part/version of spec that documents this _DSM for every uuid/function used here would help to review it. > + { > + aml_append(ifctx, > + aml_store(aml_to_integer(aml_arg(2)), aml_local(0))); > + > + /* standard DSM query function */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0))); > + { > + uint8_t byte_list[2] = { 0xff, 0x01 }; > + aml_append(ifctx2, aml_return(aml_buffer(2, byte_list))); > + } > + aml_append(ifctx, ifctx2); > + > + /* interface version: 1.3 */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1))); > + { > + aml_append(ifctx2, aml_return(aml_string("1.3"))); > + } > + aml_append(ifctx, ifctx2); > + > + /* submit TPM operation */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2))); > + { > + /* get opcode */ > + aml_append(ifctx2, > + aml_store(aml_derefof(aml_index(aml_arg(3), > + aml_int(0))), > + aml_local(0))); > + /* get opcode flags */ > + aml_append(ifctx2, > + aml_store(aml_call1("TPFN", aml_local(0)), > + aml_local(1))); > + ifctx3 = aml_if( > + aml_equal( > + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), > + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); > + { > + /* 1: not implemented */ > + aml_append(ifctx3, aml_return(aml_int(1))); > + } > + aml_append(ifctx2, ifctx3); > + aml_append(ifctx2, aml_store(aml_local(0), aml_name("PPRQ"))); > + aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM"))); > + /* 0: success */ > + aml_append(ifctx2, aml_return(aml_int(0))); > + } > + aml_append(ifctx, ifctx2); > + > + /* get pending TPM operation */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3))); > + { > + /* revision to integer */ > + aml_append(ifctx2, > + aml_store( > + aml_to_integer(aml_arg(1)), > + aml_local(1))); > + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); > + { > + aml_append(ifctx3, > + aml_store( > + aml_name("PPRQ"), > + aml_index(aml_name("TPM2"), aml_int(1)))); > + aml_append(ifctx3, aml_return(aml_name("TPM2"))); > + } > + aml_append(ifctx2, ifctx3); > + > + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); > + { > + aml_append(ifctx3, > + aml_store( > + aml_name("PPRQ"), > + aml_index(aml_name("TPM3"), aml_int(1)))); > + aml_append(ifctx3, > + aml_store( > + aml_name("PPRM"), > + aml_index(aml_name("TPM3"), aml_int(2)))); > + aml_append(ifctx3, aml_return(aml_name("TPM3"))); > + } > + aml_append(ifctx2, ifctx3); > + } > + aml_append(ifctx, ifctx2); > + > + /* get platform-specific action to transition to pre-OS env. */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4))); > + { > + /* reboot */ > + aml_append(ifctx2, aml_return(aml_int(2))); > + } > + aml_append(ifctx, ifctx2); > + > + /* get TPM operation response */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5))); > + { > + aml_append(ifctx2, > + aml_store( > + aml_name("LPPR"), > + aml_index(aml_name("TPM3"), aml_int(1)))); > + aml_append(ifctx2, > + aml_store( > + aml_name("PPRP"), > + aml_index(aml_name("TPM3"), aml_int(2)))); > + aml_append(ifctx2, aml_return(aml_name("TPM3"))); > + } > + aml_append(ifctx, ifctx2); > + > + /* submit preferred user language */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6))); > + { > + /* 3 = not implemented */ > + aml_append(ifctx2, aml_return(aml_int(3))); > + } > + aml_append(ifctx, ifctx2); > + > + /* submit TPM operation v2 */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7))); > + { > + /* get opcode */ > + aml_append(ifctx2, > + aml_store(aml_derefof(aml_index(aml_arg(3), > + aml_int(0))), > + aml_local(0))); > + /* get opcode flags */ > + aml_append(ifctx2, > + aml_store(aml_call1("TPFN", aml_local(0)), > + aml_local(1))); > + ifctx3 = aml_if( > + aml_equal( > + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), > + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); > + { > + /* 1: not implemented */ > + aml_append(ifctx3, aml_return(aml_int(1))); > + } > + aml_append(ifctx2, ifctx3); > + > + ifctx3 = aml_if( > + aml_equal( > + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), > + aml_int(TPM_PPI_FUNC_BLOCKED))); > + { > + /* 3: blocked by firmware */ > + aml_append(ifctx3, aml_return(aml_int(3))); > + } > + aml_append(ifctx2, ifctx3); > + > + /* revision to integer */ > + aml_append(ifctx2, > + aml_store( > + aml_to_integer(aml_arg(1)), > + aml_local(1))); > + > + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); > + { > + /* revision 1 */ > + aml_append(ifctx3, aml_store(aml_local(0), > + aml_name("PPRQ"))); > + aml_append(ifctx3, aml_store(aml_int(0), > + aml_name("PPRM"))); > + } > + aml_append(ifctx2, ifctx3); > + > + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); > + { > + /* revision 2 */ > + aml_append(ifctx3, > + aml_store(aml_local(0), aml_name("PPRQ"))); > + aml_append(ifctx3, > + aml_store( > + aml_derefof(aml_index(aml_arg(3), > + aml_int(1))), > + aml_name("PPRM"))); > + } > + aml_append(ifctx2, ifctx3); > + /* 0: success */ > + aml_append(ifctx2, aml_return(aml_int(0))); > + } > + aml_append(ifctx, ifctx2); > + > + /* get user confirmation status for operation */ > + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8))); > + { > + /* get opcode */ > + aml_append(ifctx2, > + aml_store(aml_derefof(aml_index(aml_arg(3), > + aml_int(0))), > + aml_local(0))); > + /* get opcode flags */ > + aml_append(ifctx2, > + aml_store(aml_call1("TPFN", aml_local(0)), > + aml_local(1))); > + /* return confirmation status code */ > + aml_append(ifctx2, > + aml_return( > + aml_and(aml_local(1), > + aml_int(TPM_PPI_FUNC_MASK), NULL))); > + } > + aml_append(ifctx, ifctx2); > + > + aml_append(ifctx, aml_return(aml_buffer(1, zerobyte))); > + } > + aml_append(method, ifctx); > + } > + aml_append(dev, method); > +} > + > static void > build_dsdt(GArray *table_data, BIOSLinker *linker, > AcpiPmInfo *pm, AcpiMiscInfo *misc, > @@ -2153,6 +2440,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > */ > /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > aml_append(dev, aml_name_decl("_CRS", crs)); > + > + build_tpm_ppi(dev); > + > aml_append(scope, dev); > } > > @@ -2172,6 +2462,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > aml_append(method, aml_return(aml_int(0x0f))); > aml_append(dev, method); > > + build_tpm_ppi(dev); > + > aml_append(sb_scope, dev); > } > > @@ -2918,7 +3210,7 @@ void acpi_setup(void) > tpm_config = (FWCfgTPMConfig) { > .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE), > .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())), > - .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE) what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used? maybe patch ordering is problem? what if we put fwcfg patch after this one? > + .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30) > }; > fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config", > &tpm_config, sizeof tpm_config); hopefully patch would be more review-able with comments and nicely named variables, so I could actually review it functionality wise without reading whole TPM spec(s)
Hi On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 15 May 2018 14:14:32 +0200 > Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > >> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >> >> The TPM Physical Presence interface consists of an ACPI part, a shared >> memory part, and code in the firmware. Users can send messages to the >> firmware by writing a code into the shared memory through invoking the >> ACPI code. When a reboot happens, the firmware looks for the code and >> acts on it by sending sequences of commands to the TPM. >> >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >> assume that SMIs are necessary to use. It uses a similar datastructure for >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >> of it. I extended the shared memory data structure with an array of 256 >> bytes, one for each code that could be implemented. The array contains >> flags describing the individual codes. This decouples the ACPI implementation >> from the firmware implementation. >> >> The underlying TCG specification is accessible from the following page. >> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >> >> This patch implements version 1.30. >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> >> --- >> >> v4 (Marc-André): >> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >> handling. >> - replace 'return Package (..) {} ' with scoped variables, to fix >> Windows ACPI handling. >> >> v3: >> - add support for PPI to CRB >> - split up OperationRegion TPPI into two parts, one containing >> the registers (TPP1) and the other one the flags (TPP2); switched >> the order of the flags versus registers in the code >> - adapted ACPI code to small changes to the array of flags where >> previous flag 0 was removed and now shifting right wasn't always >> necessary anymore >> >> v2: >> - get rid of FAIL variable; function 5 was using it and always >> returns 0; the value is related to the ACPI function call not >> a possible failure of the TPM function call. >> - extend shared memory data structure with per-opcode entries >> holding flags and use those flags to determine what to return >> to caller >> - implement interface version 1.3 >> --- >> include/hw/acpi/tpm.h | 21 +++ >> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 314 insertions(+), 1 deletion(-) >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >> index f79d68a77a..fc53f08827 100644 >> --- a/include/hw/acpi/tpm.h >> +++ b/include/hw/acpi/tpm.h >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) >> #define TPM_PPI_VERSION_NONE 0 >> #define TPM_PPI_VERSION_1_30 1 >> >> +struct tpm_ppi { >> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; >> + set by BIOS */ >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) >> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) >> +#define TPM_PPI_FUNC_MASK (7 << 0) >> + uint8_t ppin; /* 0x100 : set by BIOS */ >> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ >> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ >> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ >> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ >> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ >> + uint32_t fret; /* 0x115 : set by ACPI; not used */ >> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ >> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ >> +} QEMU_PACKED; > is this structure plant to be used for accessing data from within QEMU > or it is just used for convenience of calculating offsets/sizes for AML? > > For convenience (and it can be useful for debugging). >> + >> #endif /* HW_ACPI_TPM_H */ >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index f6d447f03a..95be4f0710 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -43,6 +43,7 @@ >> #include "hw/acpi/memory_hotplug.h" >> #include "sysemu/tpm.h" >> #include "hw/acpi/tpm.h" >> +#include "hw/tpm/tpm_ppi.h" >> #include "hw/acpi/vmgenid.h" >> #include "sysemu/tpm_backend.h" >> #include "hw/timer/mc146818rtc_regs.h" >> @@ -1789,6 +1790,292 @@ static Aml *build_q35_osc_method(void) >> return method; >> } >> >> +static void >> +build_tpm_ppi(Aml *dev) >> +{ >> + Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak; >> + struct tpm_ppi *tpm_ppi = NULL; >> + int i; > plain AML variables throughout the function make code rather unreadable > (well, like any other AML code would be). > Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate > more or less sanely named variables to make it more understandable. > that also should allow to reduce LOC this function takes. Sorry, I don't understand what I should take from build_legacy_cpu_hotplug_aml() approach. Could you describe a little bit? > >> + /* >> + * TPP1 is for the flags that indicate which PPI operations >> + * are supported by the firmware. The firmware is expected to >> + * write these flags. >> + */ >> + aml_append(dev, >> + aml_operation_region("TPP1", AML_SYSTEM_MEMORY, >> + aml_int(TPM_PPI_ADDR_BASE), >> + sizeof(tpm_ppi->func))); >> + field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); >> + for (i = 0; i < sizeof(tpm_ppi->func); i++) { >> + char *tmp = g_strdup_printf("FN%02X", i); >> + aml_append(field, aml_named_field(tmp, BITS_PER_BYTE)); >> + g_free(tmp); >> + } >> + aml_append(dev, field); >> + >> + /* >> + * TPP2 is for the registers that ACPI code used to pass >> + * the PPI code and parameter (PPRQ, PPRM) to the firmware. >> + */ >> + aml_append(dev, >> + aml_operation_region("TPP2", AML_SYSTEM_MEMORY, >> + aml_int(TPM_PPI_ADDR_BASE + >> + offsetof(struct tpm_ppi, ppin)), >> + sizeof(struct tpm_ppi) - >> + sizeof(tpm_ppi->func))); >> + field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); >> + aml_append(field, aml_named_field("PPIN", >> + sizeof(uint8_t) * BITS_PER_BYTE)); > here you already don't use tpm_ppi for sizing, so it creates a confusing mix > of both approaches. True, sizeof_field() will become handy here. I can leave a TODO? > > From ACPI pov I'd prefer PPI table documented somewhere in spec docs > (similar docs/specs/acpi_mem_hotplug.txt) and would look like in > other use-cases: > > aml_append(field, aml_named_field("PPIN", 8) > > and drop "struct tpm_ppi" altogether replacing places it was used by > explicit constants. I have a slight preference for the tpm_ppi structure. But ok with replacing it with constant. Stefan, do you agree? >> + aml_append(field, aml_named_field("PPIP", >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> + aml_append(field, aml_named_field("PPRP", >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> + aml_append(field, aml_named_field("PPRQ", >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> + aml_append(field, aml_named_field("PPRM", >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> + aml_append(field, aml_named_field("LPPR", >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> + aml_append(dev, field); >> + >> + method = aml_method("TPFN", 1, AML_SERIALIZED); > not quite sure how this method (supposed to ) work(s), > it could use nice comment explaining mechanics. > Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It took me a while to figure this out. My laptop TPM ACPI table uses the same trick, so I assume this is a Windows acpi bug/limitation. Instead we use a function that returns the corresponding field. So we declare each field / array entry: OperationRegion (TPP1, SystemMemory, 0xFED45000, 0x0100) Field (TPP1, AnyAcc, NoLock, Preserve) { FN00, 8, FN01, 8,.... And the method to access it: Method (TPFN, 1, Serialized) { If ((Zero == Arg0)) { Return (FN00) /* \_SB_.TPM_.FN00 */ } ..... >> + { >> + for (i = 0; i < sizeof(tpm_ppi->func); i++) { >> + ifctx = aml_if(aml_equal(aml_int(i), aml_arg(0))); >> + { >> + aml_append(ifctx, aml_return(aml_name("FN%02X", i))); >> + } >> + aml_append(method, ifctx); >> + } >> + aml_append(method, aml_return(aml_int(0))); >> + } >> + aml_append(dev, method); >> + >> + pak = aml_package(2); >> + aml_append(pak, aml_int(0)); >> + aml_append(pak, aml_int(0)); >> + name = aml_name_decl("TPM2", pak); >> + aml_append(dev, name); >> + >> + pak = aml_package(3); >> + aml_append(pak, aml_int(0)); >> + aml_append(pak, aml_int(0)); >> + aml_append(pak, aml_int(0)); >> + name = aml_name_decl("TPM3", pak); >> + aml_append(dev, name); >> + >> + method = aml_method("_DSM", 4, AML_SERIALIZED); >> + { >> + uint8_t zerobyte[1] = { 0 }; >> + >> + ifctx = aml_if( >> + aml_equal(aml_arg(0), >> + aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653"))); > a comment pointing to a specific part/version of spec that documents this _DSM > for every uuid/function used here would help to review it. ok, fixing that. The spec PDF is fairly easy to read imho: https://trustedcomputinggroup.org/wp-content/uploads/Physical-Presence-Interface_1-30_0-52.pdf > >> + { >> + aml_append(ifctx, >> + aml_store(aml_to_integer(aml_arg(2)), aml_local(0))); >> + >> + /* standard DSM query function */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0))); >> + { >> + uint8_t byte_list[2] = { 0xff, 0x01 }; >> + aml_append(ifctx2, aml_return(aml_buffer(2, byte_list))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* interface version: 1.3 */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1))); >> + { >> + aml_append(ifctx2, aml_return(aml_string("1.3"))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* submit TPM operation */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2))); >> + { >> + /* get opcode */ >> + aml_append(ifctx2, >> + aml_store(aml_derefof(aml_index(aml_arg(3), >> + aml_int(0))), >> + aml_local(0))); >> + /* get opcode flags */ >> + aml_append(ifctx2, >> + aml_store(aml_call1("TPFN", aml_local(0)), >> + aml_local(1))); >> + ifctx3 = aml_if( >> + aml_equal( >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), >> + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); >> + { >> + /* 1: not implemented */ >> + aml_append(ifctx3, aml_return(aml_int(1))); >> + } >> + aml_append(ifctx2, ifctx3); >> + aml_append(ifctx2, aml_store(aml_local(0), aml_name("PPRQ"))); >> + aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM"))); >> + /* 0: success */ >> + aml_append(ifctx2, aml_return(aml_int(0))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* get pending TPM operation */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3))); >> + { >> + /* revision to integer */ >> + aml_append(ifctx2, >> + aml_store( >> + aml_to_integer(aml_arg(1)), >> + aml_local(1))); >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); >> + { >> + aml_append(ifctx3, >> + aml_store( >> + aml_name("PPRQ"), >> + aml_index(aml_name("TPM2"), aml_int(1)))); >> + aml_append(ifctx3, aml_return(aml_name("TPM2"))); >> + } >> + aml_append(ifctx2, ifctx3); >> + >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); >> + { >> + aml_append(ifctx3, >> + aml_store( >> + aml_name("PPRQ"), >> + aml_index(aml_name("TPM3"), aml_int(1)))); >> + aml_append(ifctx3, >> + aml_store( >> + aml_name("PPRM"), >> + aml_index(aml_name("TPM3"), aml_int(2)))); >> + aml_append(ifctx3, aml_return(aml_name("TPM3"))); >> + } >> + aml_append(ifctx2, ifctx3); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* get platform-specific action to transition to pre-OS env. */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4))); >> + { >> + /* reboot */ >> + aml_append(ifctx2, aml_return(aml_int(2))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* get TPM operation response */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5))); >> + { >> + aml_append(ifctx2, >> + aml_store( >> + aml_name("LPPR"), >> + aml_index(aml_name("TPM3"), aml_int(1)))); >> + aml_append(ifctx2, >> + aml_store( >> + aml_name("PPRP"), >> + aml_index(aml_name("TPM3"), aml_int(2)))); >> + aml_append(ifctx2, aml_return(aml_name("TPM3"))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* submit preferred user language */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6))); >> + { >> + /* 3 = not implemented */ >> + aml_append(ifctx2, aml_return(aml_int(3))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* submit TPM operation v2 */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7))); >> + { >> + /* get opcode */ >> + aml_append(ifctx2, >> + aml_store(aml_derefof(aml_index(aml_arg(3), >> + aml_int(0))), >> + aml_local(0))); >> + /* get opcode flags */ >> + aml_append(ifctx2, >> + aml_store(aml_call1("TPFN", aml_local(0)), >> + aml_local(1))); >> + ifctx3 = aml_if( >> + aml_equal( >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), >> + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); >> + { >> + /* 1: not implemented */ >> + aml_append(ifctx3, aml_return(aml_int(1))); >> + } >> + aml_append(ifctx2, ifctx3); >> + >> + ifctx3 = aml_if( >> + aml_equal( >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), >> + aml_int(TPM_PPI_FUNC_BLOCKED))); >> + { >> + /* 3: blocked by firmware */ >> + aml_append(ifctx3, aml_return(aml_int(3))); >> + } >> + aml_append(ifctx2, ifctx3); >> + >> + /* revision to integer */ >> + aml_append(ifctx2, >> + aml_store( >> + aml_to_integer(aml_arg(1)), >> + aml_local(1))); >> + >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); >> + { >> + /* revision 1 */ >> + aml_append(ifctx3, aml_store(aml_local(0), >> + aml_name("PPRQ"))); >> + aml_append(ifctx3, aml_store(aml_int(0), >> + aml_name("PPRM"))); >> + } >> + aml_append(ifctx2, ifctx3); >> + >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); >> + { >> + /* revision 2 */ >> + aml_append(ifctx3, >> + aml_store(aml_local(0), aml_name("PPRQ"))); >> + aml_append(ifctx3, >> + aml_store( >> + aml_derefof(aml_index(aml_arg(3), >> + aml_int(1))), >> + aml_name("PPRM"))); >> + } >> + aml_append(ifctx2, ifctx3); >> + /* 0: success */ >> + aml_append(ifctx2, aml_return(aml_int(0))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + /* get user confirmation status for operation */ >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8))); >> + { >> + /* get opcode */ >> + aml_append(ifctx2, >> + aml_store(aml_derefof(aml_index(aml_arg(3), >> + aml_int(0))), >> + aml_local(0))); >> + /* get opcode flags */ >> + aml_append(ifctx2, >> + aml_store(aml_call1("TPFN", aml_local(0)), >> + aml_local(1))); >> + /* return confirmation status code */ >> + aml_append(ifctx2, >> + aml_return( >> + aml_and(aml_local(1), >> + aml_int(TPM_PPI_FUNC_MASK), NULL))); >> + } >> + aml_append(ifctx, ifctx2); >> + >> + aml_append(ifctx, aml_return(aml_buffer(1, zerobyte))); >> + } >> + aml_append(method, ifctx); >> + } >> + aml_append(dev, method); >> +} >> + >> static void >> build_dsdt(GArray *table_data, BIOSLinker *linker, >> AcpiPmInfo *pm, AcpiMiscInfo *misc, >> @@ -2153,6 +2440,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, >> */ >> /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ >> aml_append(dev, aml_name_decl("_CRS", crs)); >> + >> + build_tpm_ppi(dev); >> + >> aml_append(scope, dev); >> } >> >> @@ -2172,6 +2462,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, >> aml_append(method, aml_return(aml_int(0x0f))); >> aml_append(dev, method); >> >> + build_tpm_ppi(dev); >> + >> aml_append(sb_scope, dev); >> } >> >> @@ -2918,7 +3210,7 @@ void acpi_setup(void) >> tpm_config = (FWCfgTPMConfig) { >> .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE), >> .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())), >> - .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE) > what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used? > maybe patch ordering is problem? what if we put fwcfg patch after this one? edk2 already knows that NONE (0) is no PPI. So we at least have to keep the same indexing. I don't see much point in reordering. If you look at v4, you may want to squash things together too, but that makes reviewing more complicated. As the long as the split is natural, and no regression are introduced, I would rather keep it the way it is. > >> + .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30) >> }; >> fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config", >> &tpm_config, sizeof tpm_config); > > hopefully patch would be more review-able with comments and nicely named > variables, so I could actually review it functionality wise without reading > whole TPM spec(s) Let see what I can do. Thanks!
Hi On Thu, Jun 21, 2018 at 3:21 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: >>> + method = aml_method("TPFN", 1, AML_SERIALIZED); >> not quite sure how this method (supposed to ) work(s), >> it could use nice comment explaining mechanics. >> > > Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It > took me a while to figure this out. My laptop TPM ACPI table uses the > same trick, so I assume this is a Windows acpi bug/limitation. Instead > we use a function that returns the corresponding field. Sorry, I am wrong, my laptop doesn't use such table. I don't know where I saw someone using the same trick though..
On 06/21/2018 09:21 AM, Marc-André Lureau wrote: > Hi > > On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote: >> On Tue, 15 May 2018 14:14:32 +0200 >> Marc-André Lureau <marcandre.lureau@redhat.com> wrote: >> >>> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> >>> The TPM Physical Presence interface consists of an ACPI part, a shared >>> memory part, and code in the firmware. Users can send messages to the >>> firmware by writing a code into the shared memory through invoking the >>> ACPI code. When a reboot happens, the firmware looks for the code and >>> acts on it by sending sequences of commands to the TPM. >>> >>> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >>> assume that SMIs are necessary to use. It uses a similar datastructure for >>> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >>> of it. I extended the shared memory data structure with an array of 256 >>> bytes, one for each code that could be implemented. The array contains >>> flags describing the individual codes. This decouples the ACPI implementation >>> from the firmware implementation. >>> >>> The underlying TCG specification is accessible from the following page. >>> >>> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >>> >>> This patch implements version 1.30. >>> >>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>> >>> --- >>> >>> v4 (Marc-André): >>> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >>> handling. >>> - replace 'return Package (..) {} ' with scoped variables, to fix >>> Windows ACPI handling. >>> >>> v3: >>> - add support for PPI to CRB >>> - split up OperationRegion TPPI into two parts, one containing >>> the registers (TPP1) and the other one the flags (TPP2); switched >>> the order of the flags versus registers in the code >>> - adapted ACPI code to small changes to the array of flags where >>> previous flag 0 was removed and now shifting right wasn't always >>> necessary anymore >>> >>> v2: >>> - get rid of FAIL variable; function 5 was using it and always >>> returns 0; the value is related to the ACPI function call not >>> a possible failure of the TPM function call. >>> - extend shared memory data structure with per-opcode entries >>> holding flags and use those flags to determine what to return >>> to caller >>> - implement interface version 1.3 >>> --- >>> >>> >> From ACPI pov I'd prefer PPI table documented somewhere in spec docs >> (similar docs/specs/acpi_mem_hotplug.txt) and would look like in >> other use-cases: >> >> aml_append(field, aml_named_field("PPIN", 8) >> >> and drop "struct tpm_ppi" altogether replacing places it was used by >> explicit constants. > I have a slight preference for the tpm_ppi structure. But ok with > replacing it with constant. Stefan, do you agree? The PPI structure will appear in the firmware code and if it is the same and is correctly used we know that both are in sync. So I wouldn't get rid of it based on just that. If we have to changed all sizeof(uint32_t/uint8_t) to correctly use that shared structure, I'd prefer that. Stefan
Hi On Thu, Jun 21, 2018 at 3:22 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Thu, Jun 21, 2018 at 3:21 PM, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > >>>> + method = aml_method("TPFN", 1, AML_SERIALIZED); >>> not quite sure how this method (supposed to ) work(s), >>> it could use nice comment explaining mechanics. >>> >> >> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It >> took me a while to figure this out. My laptop TPM ACPI table uses the >> same trick, so I assume this is a Windows acpi bug/limitation. Instead >> we use a function that returns the corresponding field. > > Sorry, I am wrong, my laptop doesn't use such table. I don't know > where I saw someone using the same trick though.. fwiw, I also asked on OSR list for help but got no answer: http://www.osronline.com/showThread.CFM?link=288617
On Thu, 21 Jun 2018 15:21:02 +0200 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > On Tue, 15 May 2018 14:14:32 +0200 > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > > >> From: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> > >> The TPM Physical Presence interface consists of an ACPI part, a shared > >> memory part, and code in the firmware. Users can send messages to the > >> firmware by writing a code into the shared memory through invoking the > >> ACPI code. When a reboot happens, the firmware looks for the code and > >> acts on it by sending sequences of commands to the TPM. > >> > >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't > >> assume that SMIs are necessary to use. It uses a similar datastructure for > >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use > >> of it. I extended the shared memory data structure with an array of 256 > >> bytes, one for each code that could be implemented. The array contains > >> flags describing the individual codes. This decouples the ACPI implementation > >> from the firmware implementation. > >> > >> The underlying TCG specification is accessible from the following page. > >> > >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ > >> > >> This patch implements version 1.30. > >> > >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> > >> --- > >> > >> v4 (Marc-André): > >> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI > >> handling. > >> - replace 'return Package (..) {} ' with scoped variables, to fix > >> Windows ACPI handling. > >> > >> v3: > >> - add support for PPI to CRB > >> - split up OperationRegion TPPI into two parts, one containing > >> the registers (TPP1) and the other one the flags (TPP2); switched > >> the order of the flags versus registers in the code > >> - adapted ACPI code to small changes to the array of flags where > >> previous flag 0 was removed and now shifting right wasn't always > >> necessary anymore > >> > >> v2: > >> - get rid of FAIL variable; function 5 was using it and always > >> returns 0; the value is related to the ACPI function call not > >> a possible failure of the TPM function call. > >> - extend shared memory data structure with per-opcode entries > >> holding flags and use those flags to determine what to return > >> to caller > >> - implement interface version 1.3 > >> --- > >> include/hw/acpi/tpm.h | 21 +++ > >> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- > >> 2 files changed, 314 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > >> index f79d68a77a..fc53f08827 100644 > >> --- a/include/hw/acpi/tpm.h > >> +++ b/include/hw/acpi/tpm.h > >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) > >> #define TPM_PPI_VERSION_NONE 0 > >> #define TPM_PPI_VERSION_1_30 1 > >> > >> +struct tpm_ppi { > >> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; > >> + set by BIOS */ > >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ > >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) > >> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) > >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) > >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) > >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) > >> +#define TPM_PPI_FUNC_MASK (7 << 0) > >> + uint8_t ppin; /* 0x100 : set by BIOS */ > >> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ > >> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ > >> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ > >> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ > >> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ > >> + uint32_t fret; /* 0x115 : set by ACPI; not used */ > >> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ > >> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ > >> +} QEMU_PACKED; > > is this structure plant to be used for accessing data from within QEMU > > or it is just used for convenience of calculating offsets/sizes for AML? > > > > > > For convenience (and it can be useful for debugging). we are gradually getting rid of usage "struct foo" in ACPI code (i.e. when I have time to convert old struct based approach to build_append_int() table based format). and for new code I usually require ACPI parts be struct less (if there is no previous struct baggage to back it up). Hence my request to drop dummy struct and document layout properly in related spec doc (that's where FW should look for documentation and not into struct definition somewhere in the header). So I'd strongly prefer it be done this way. > >> + > >> #endif /* HW_ACPI_TPM_H */ > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >> index f6d447f03a..95be4f0710 100644 > >> --- a/hw/i386/acpi-build.c > >> +++ b/hw/i386/acpi-build.c > >> @@ -43,6 +43,7 @@ > >> #include "hw/acpi/memory_hotplug.h" > >> #include "sysemu/tpm.h" > >> #include "hw/acpi/tpm.h" > >> +#include "hw/tpm/tpm_ppi.h" > >> #include "hw/acpi/vmgenid.h" > >> #include "sysemu/tpm_backend.h" > >> #include "hw/timer/mc146818rtc_regs.h" > >> @@ -1789,6 +1790,292 @@ static Aml *build_q35_osc_method(void) > >> return method; > >> } > >> > >> +static void > >> +build_tpm_ppi(Aml *dev) > >> +{ > >> + Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak; > >> + struct tpm_ppi *tpm_ppi = NULL; > >> + int i; > > plain AML variables throughout the function make code rather unreadable > > (well, like any other AML code would be). > > Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate > > more or less sanely named variables to make it more understandable. > > that also should allow to reduce LOC this function takes. > > Sorry, I don't understand what I should take from > build_legacy_cpu_hotplug_aml() approach. Could you describe a little > bit? I was talking about something like this: Aml *apic_id = aml_arg(0); Aml *cpu_on = aml_local(0); ... Aml *cpus_map = aml_name(CPU_ON_BITMAP); ... aml_append(method, aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on)) ^^^ this part becomes much more read-able for reviewer/maintainer versus pure ASL aml_store(aml_derefof(aml_index(aml_name("CPON"), aml_arg(0))), aml_local(0))) > > > >> + /* > >> + * TPP1 is for the CPONflags that indicate which PPI operations > >> + * are supported by the firmware. The firmware is expected to > >> + * write these flags. > >> + */ > >> + aml_append(dev, > >> + aml_operation_region("TPP1", AML_SYSTEM_MEMORY, > >> + aml_int(TPM_PPI_ADDR_BASE), > >> + sizeof(tpm_ppi->func))); > >> + field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); > >> + for (i = 0; i < sizeof(tpm_ppi->func); i++) { > >> + char *tmp = g_strdup_printf("FN%02X", i); > >> + aml_append(field, aml_named_field(tmp, BITS_PER_BYTE)); > >> + g_free(tmp); > >> + } > >> + aml_append(dev, field); > >> + > >> + /* > >> + * TPP2 is for the registers that ACPI code used to pass > >> + * the PPI code and parameter (PPRQ, PPRM) to the firmware. > >> + */ > >> + aml_append(dev, > >> + aml_operation_region("TPP2", AML_SYSTEM_MEMORY, > >> + aml_int(TPM_PPI_ADDR_BASE + > >> + offsetof(struct tpm_ppi, ppin)), > >> + sizeof(struct tpm_ppi) - > >> + sizeof(tpm_ppi->func))); > >> + field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); > >> + aml_append(field, aml_named_field("PPIN", > >> + sizeof(uint8_t) * BITS_PER_BYTE)); > > here you already don't use tpm_ppi for sizing, so it creates a confusing mix > > of both approaches. > > True, sizeof_field() will become handy here. I can leave a TODO? I'd just use a constant /8/ here like the rest of the code that uses aml_named_field() (no need to over-complicate or create another precedent to copy from) Why TODO, changes I suggested for this patch would change it quite heavily so why merge patch that would be changed by follow up patch almost immediately. I'd prefer cleaner code being merged unless there are very good reasons to merge something that should be rewritten afterwards. > > > > > > From ACPI pov I'd prefer PPI table documented somewhere in spec docs > > (similar docs/specs/acpi_mem_hotplug.txt) and would look like in > > other use-cases: > > > > aml_append(field, aml_named_field("PPIN", 8) > > > > and drop "struct tpm_ppi" altogether replacing places it was used by > > explicit constants. > > I have a slight preference for the tpm_ppi structure. But ok with > replacing it with constant. Stefan, do you agree? > > >> + aml_append(field, aml_named_field("PPIP", > >> + sizeof(uint32_t) * BITS_PER_BYTE)); > >> + aml_append(field, aml_named_field("PPRP", > >> + sizeof(uint32_t) * BITS_PER_BYTE)); > >> + aml_append(field, aml_named_field("PPRQ", > >> + sizeof(uint32_t) * BITS_PER_BYTE)); > >> + aml_append(field, aml_named_field("PPRM", > >> + sizeof(uint32_t) * BITS_PER_BYTE)); > >> + aml_append(field, aml_named_field("LPPR", > >> + sizeof(uint32_t) * BITS_PER_BYTE)); > >> + aml_append(dev, field); > >> + > >> + method = aml_method("TPFN", 1, AML_SERIALIZED); > > not quite sure how this method (supposed to ) work(s), > > it could use nice comment explaining mechanics. > > > > Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It > took me a while to figure this out. My laptop TPM ACPI table uses the > same trick, so I assume this is a Windows acpi bug/limitation. Instead > we use a function that returns the corresponding field. > > So we declare each field / array entry: > OperationRegion (TPP1, SystemMemory, 0xFED45000, 0x0100) > Field (TPP1, AnyAcc, NoLock, Preserve) > { > FN00, 8, > FN01, 8,.... > > And the method to access it: > > Method (TPFN, 1, Serialized) > { > If ((Zero == Arg0)) > { > Return (FN00) /* \_SB_.TPM_.FN00 */ > } > ..... > > > > >> + { > >> + for (i = 0; i < sizeof(tpm_ppi->func); i++) { > >> + ifctx = aml_if(aml_equal(aml_int(i), aml_arg(0))); > >> + { > >> + aml_append(ifctx, aml_return(aml_name("FN%02X", i))); > >> + } > >> + aml_append(method, ifctx); > >> + } > >> + aml_append(method, aml_return(aml_int(0))); > >> + } > >> + aml_append(dev, method); > >> + > >> + pak = aml_package(2); > >> + aml_append(pak, aml_int(0)); > >> + aml_append(pak, aml_int(0)); > >> + name = aml_name_decl("TPM2", pak); > >> + aml_append(dev, name); > >> + > >> + pak = aml_package(3); > >> + aml_append(pak, aml_int(0)); > >> + aml_append(pak, aml_int(0)); > >> + aml_append(pak, aml_int(0)); > >> + name = aml_name_decl("TPM3", pak); > >> + aml_append(dev, name); > >> + > >> + method = aml_method("_DSM", 4, AML_SERIALIZED); > >> + { > >> + uint8_t zerobyte[1] = { 0 }; > >> + > >> + ifctx = aml_if( > >> + aml_equal(aml_arg(0), > >> + aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653"))); > > a comment pointing to a specific part/version of spec that documents this _DSM > > for every uuid/function used here would help to review it. > > ok, fixing that. > > The spec PDF is fairly easy to read imho: > > https://trustedcomputinggroup.org/wp-content/uploads/Physical-Presence-Interface_1-30_0-52.pdf > > > >> + { > >> + aml_append(ifctx, > >> + aml_store(aml_to_integer(aml_arg(2)), aml_local(0))); > >> + > >> + /* standard DSM query function */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0))); > >> + { > >> + uint8_t byte_list[2] = { 0xff, 0x01 }; > >> + aml_append(ifctx2, aml_return(aml_buffer(2, byte_list))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* interface version: 1.3 */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1))); > >> + { > >> + aml_append(ifctx2, aml_return(aml_string("1.3"))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* submit TPM operation */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2))); > >> + { > >> + /* get opcode */ > >> + aml_append(ifctx2, > >> + aml_store(aml_derefof(aml_index(aml_arg(3), > >> + aml_int(0))), > >> + aml_local(0))); > >> + /* get opcode flags */ > >> + aml_append(ifctx2, > >> + aml_store(aml_call1("TPFN", aml_local(0)), > >> + aml_local(1))); > >> + ifctx3 = aml_if( > >> + aml_equal( > >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), > >> + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); > >> + { > >> + /* 1: not implemented */ > >> + aml_append(ifctx3, aml_return(aml_int(1))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + aml_append(ifctx2, aml_store(aml_local(0), aml_name("PPRQ"))); > >> + aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM"))); > >> + /* 0: success */ > >> + aml_append(ifctx2, aml_return(aml_int(0))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* get pending TPM operation */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3))); > >> + { > >> + /* revision to integer */ > >> + aml_append(ifctx2, > >> + aml_store( > >> + aml_to_integer(aml_arg(1)), > >> + aml_local(1))); > >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); > >> + { > >> + aml_append(ifctx3, > >> + aml_store( > >> + aml_name("PPRQ"), > >> + aml_index(aml_name("TPM2"), aml_int(1)))); > >> + aml_append(ifctx3, aml_return(aml_name("TPM2"))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + > >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); > >> + { > >> + aml_append(ifctx3, > >> + aml_store( > >> + aml_name("PPRQ"), > >> + aml_index(aml_name("TPM3"), aml_int(1)))); > >> + aml_append(ifctx3, > >> + aml_store( > >> + aml_name("PPRM"), > >> + aml_index(aml_name("TPM3"), aml_int(2)))); > >> + aml_append(ifctx3, aml_return(aml_name("TPM3"))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* get platform-specific action to transition to pre-OS env. */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4))); > >> + { > >> + /* reboot */ > >> + aml_append(ifctx2, aml_return(aml_int(2))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* get TPM operation response */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5))); > >> + { > >> + aml_append(ifctx2, > >> + aml_store( > >> + aml_name("LPPR"), > >> + aml_index(aml_name("TPM3"), aml_int(1)))); > >> + aml_append(ifctx2, > >> + aml_store( > >> + aml_name("PPRP"), > >> + aml_index(aml_name("TPM3"), aml_int(2)))); > >> + aml_append(ifctx2, aml_return(aml_name("TPM3"))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* submit preferred user language */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6))); > >> + { > >> + /* 3 = not implemented */ > >> + aml_append(ifctx2, aml_return(aml_int(3))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* submit TPM operation v2 */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7))); > >> + { > >> + /* get opcode */ > >> + aml_append(ifctx2, > >> + aml_store(aml_derefof(aml_index(aml_arg(3), > >> + aml_int(0))), > >> + aml_local(0))); > >> + /* get opcode flags */ > >> + aml_append(ifctx2, > >> + aml_store(aml_call1("TPFN", aml_local(0)), > >> + aml_local(1))); > >> + ifctx3 = aml_if( > >> + aml_equal( > >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), > >> + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); > >> + { > >> + /* 1: not implemented */ > >> + aml_append(ifctx3, aml_return(aml_int(1))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + > >> + ifctx3 = aml_if( > >> + aml_equal( > >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), > >> + aml_int(TPM_PPI_FUNC_BLOCKED))); > >> + { > >> + /* 3: blocked by firmware */ > >> + aml_append(ifctx3, aml_return(aml_int(3))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + > >> + /* revision to integer */ > >> + aml_append(ifctx2, > >> + aml_store( > >> + aml_to_integer(aml_arg(1)), > >> + aml_local(1))); > >> + > >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); > >> + { > >> + /* revision 1 */ > >> + aml_append(ifctx3, aml_store(aml_local(0), > >> + aml_name("PPRQ"))); > >> + aml_append(ifctx3, aml_store(aml_int(0), > >> + aml_name("PPRM"))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + > >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); > >> + { > >> + /* revision 2 */ > >> + aml_append(ifctx3, > >> + aml_store(aml_local(0), aml_name("PPRQ"))); > >> + aml_append(ifctx3, > >> + aml_store( > >> + aml_derefof(aml_index(aml_arg(3), > >> + aml_int(1))), > >> + aml_name("PPRM"))); > >> + } > >> + aml_append(ifctx2, ifctx3); > >> + /* 0: success */ > >> + aml_append(ifctx2, aml_return(aml_int(0))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + /* get user confirmation status for operation */ > >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8))); > >> + { > >> + /* get opcode */ > >> + aml_append(ifctx2, > >> + aml_store(aml_derefof(aml_index(aml_arg(3), > >> + aml_int(0))), > >> + aml_local(0))); > >> + /* get opcode flags */ > >> + aml_append(ifctx2, > >> + aml_store(aml_call1("TPFN", aml_local(0)), > >> + aml_local(1))); > >> + /* return confirmation status code */ > >> + aml_append(ifctx2, > >> + aml_return( > >> + aml_and(aml_local(1), > >> + aml_int(TPM_PPI_FUNC_MASK), NULL))); > >> + } > >> + aml_append(ifctx, ifctx2); > >> + > >> + aml_append(ifctx, aml_return(aml_buffer(1, zerobyte))); > >> + } > >> + aml_append(method, ifctx); > >> + } > >> + aml_append(dev, method); > >> +} > >> + > >> static void > >> build_dsdt(GArray *table_data, BIOSLinker *linker, > >> AcpiPmInfo *pm, AcpiMiscInfo *misc, > >> @@ -2153,6 +2440,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > >> */ > >> /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ > >> aml_append(dev, aml_name_decl("_CRS", crs)); > >> + > >> + build_tpm_ppi(dev); > >> + > >> aml_append(scope, dev); > >> } > >> > >> @@ -2172,6 +2462,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > >> aml_append(method, aml_return(aml_int(0x0f))); > >> aml_append(dev, method); > >> > >> + build_tpm_ppi(dev); > >> + > >> aml_append(sb_scope, dev); > >> } > >> > >> @@ -2918,7 +3210,7 @@ void acpi_setup(void) > >> tpm_config = (FWCfgTPMConfig) { > >> .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE), > >> .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())), > >> - .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE) > > what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used? > > maybe patch ordering is problem? what if we put fwcfg patch after this one? > > edk2 already knows that NONE (0) is no PPI. So we at least have to > keep the same indexing. > > I don't see much point in reordering. If you look at v4, you may want > to squash things together too, but that makes reviewing more > complicated. As the long as the split is natural, and no regression > are introduced, I would rather keep it the way it is. > > > > >> + .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30) > >> }; > >> fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config", > >> &tpm_config, sizeof tpm_config); > > > > hopefully patch would be more review-able with comments and nicely named > > variables, so I could actually review it functionality wise without reading > > whole TPM spec(s) > > Let see what I can do. > > Thanks! > >
On Thu, 21 Jun 2018 16:13:32 +0200 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Thu, Jun 21, 2018 at 3:22 PM, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > Hi > > > > On Thu, Jun 21, 2018 at 3:21 PM, Marc-André Lureau > > <marcandre.lureau@gmail.com> wrote: > > > >>>> + method = aml_method("TPFN", 1, AML_SERIALIZED); > >>> not quite sure how this method (supposed to ) work(s), > >>> it could use nice comment explaining mechanics. > >>> > >> > >> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It > >> took me a while to figure this out. My laptop TPM ACPI table uses the > >> same trick, so I assume this is a Windows acpi bug/limitation. Instead > >> we use a function that returns the corresponding field. > > > > Sorry, I am wrong, my laptop doesn't use such table. I don't know > > where I saw someone using the same trick though.. > > fwiw, I also asked on OSR list for help but got no answer: > http://www.osronline.com/showThread.CFM?link=288617 it is/might be fine if there is a workaround to make Windows work, we should mention it in comment though (including windows versions where it doesn't work) so in future someone else (including me and you) won't break it by accident. > > > > >
Hi On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 21 Jun 2018 15:21:02 +0200 > Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > >> Hi >> >> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote: >> > On Tue, 15 May 2018 14:14:32 +0200 >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote: >> > >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >> >> >> >> The TPM Physical Presence interface consists of an ACPI part, a shared >> >> memory part, and code in the firmware. Users can send messages to the >> >> firmware by writing a code into the shared memory through invoking the >> >> ACPI code. When a reboot happens, the firmware looks for the code and >> >> acts on it by sending sequences of commands to the TPM. >> >> >> >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >> >> assume that SMIs are necessary to use. It uses a similar datastructure for >> >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >> >> of it. I extended the shared memory data structure with an array of 256 >> >> bytes, one for each code that could be implemented. The array contains >> >> flags describing the individual codes. This decouples the ACPI implementation >> >> from the firmware implementation. >> >> >> >> The underlying TCG specification is accessible from the following page. >> >> >> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >> >> >> >> This patch implements version 1.30. >> >> >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >> >> >> >> --- >> >> >> >> v4 (Marc-André): >> >> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >> >> handling. >> >> - replace 'return Package (..) {} ' with scoped variables, to fix >> >> Windows ACPI handling. >> >> >> >> v3: >> >> - add support for PPI to CRB >> >> - split up OperationRegion TPPI into two parts, one containing >> >> the registers (TPP1) and the other one the flags (TPP2); switched >> >> the order of the flags versus registers in the code >> >> - adapted ACPI code to small changes to the array of flags where >> >> previous flag 0 was removed and now shifting right wasn't always >> >> necessary anymore >> >> >> >> v2: >> >> - get rid of FAIL variable; function 5 was using it and always >> >> returns 0; the value is related to the ACPI function call not >> >> a possible failure of the TPM function call. >> >> - extend shared memory data structure with per-opcode entries >> >> holding flags and use those flags to determine what to return >> >> to caller >> >> - implement interface version 1.3 >> >> --- >> >> include/hw/acpi/tpm.h | 21 +++ >> >> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- >> >> 2 files changed, 314 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >> >> index f79d68a77a..fc53f08827 100644 >> >> --- a/include/hw/acpi/tpm.h >> >> +++ b/include/hw/acpi/tpm.h >> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) >> >> #define TPM_PPI_VERSION_NONE 0 >> >> #define TPM_PPI_VERSION_1_30 1 >> >> >> >> +struct tpm_ppi { >> >> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; >> >> + set by BIOS */ >> >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ >> >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) >> >> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) >> >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) >> >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) >> >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) >> >> +#define TPM_PPI_FUNC_MASK (7 << 0) >> >> + uint8_t ppin; /* 0x100 : set by BIOS */ >> >> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ >> >> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ >> >> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ >> >> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ >> >> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ >> >> + uint32_t fret; /* 0x115 : set by ACPI; not used */ >> >> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ >> >> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ >> >> +} QEMU_PACKED; >> > is this structure plant to be used for accessing data from within QEMU >> > or it is just used for convenience of calculating offsets/sizes for AML? >> > >> > >> >> For convenience (and it can be useful for debugging). > we are gradually getting rid of usage "struct foo" in ACPI code > (i.e. when I have time to convert old struct based approach > to build_append_int() table based format). > and for new code I usually require ACPI parts be struct less > (if there is no previous struct baggage to back it up). > The tpm_ppi struct is not an ACPI table, it's a fixed memory region / layout to exchange between guest/os and firmware. > Hence my request to drop dummy struct and document layout > properly in related spec doc (that's where FW should look for > documentation and not into struct definition somewhere in > the header). So I'd strongly prefer it be done this way. There is not much more than the field description for me ;). Stefan, would you like to develop the structure usage? > >> >> + >> >> #endif /* HW_ACPI_TPM_H */ >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> >> index f6d447f03a..95be4f0710 100644 >> >> --- a/hw/i386/acpi-build.c >> >> +++ b/hw/i386/acpi-build.c >> >> @@ -43,6 +43,7 @@ >> >> #include "hw/acpi/memory_hotplug.h" >> >> #include "sysemu/tpm.h" >> >> #include "hw/acpi/tpm.h" >> >> +#include "hw/tpm/tpm_ppi.h" >> >> #include "hw/acpi/vmgenid.h" >> >> #include "sysemu/tpm_backend.h" >> >> #include "hw/timer/mc146818rtc_regs.h" >> >> @@ -1789,6 +1790,292 @@ static Aml *build_q35_osc_method(void) >> >> return method; >> >> } >> >> >> >> +static void >> >> +build_tpm_ppi(Aml *dev) >> >> +{ >> >> + Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak; >> >> + struct tpm_ppi *tpm_ppi = NULL; >> >> + int i; >> > plain AML variables throughout the function make code rather unreadable >> > (well, like any other AML code would be). >> > Pls take a look at how build_legacy_cpu_hotplug_aml() uses intermediate >> > more or less sanely named variables to make it more understandable. >> > that also should allow to reduce LOC this function takes. >> >> Sorry, I don't understand what I should take from >> build_legacy_cpu_hotplug_aml() approach. Could you describe a little >> bit? > I was talking about something like this: > > Aml *apic_id = aml_arg(0); > Aml *cpu_on = aml_local(0); > ... > Aml *cpus_map = aml_name(CPU_ON_BITMAP); > ... > aml_append(method, > aml_store(aml_derefof(aml_index(cpus_map, apic_id)), cpu_on)) > > ^^^ this part becomes much more read-able for reviewer/maintainer versus pure ASL > > aml_store(aml_derefof(aml_index(aml_name("CPON"), aml_arg(0))), aml_local(0))) Ok > >> > >> >> + /* >> >> + * TPP1 is for the CPONflags that indicate which PPI operations >> >> + * are supported by the firmware. The firmware is expected to >> >> + * write these flags. >> >> + */ >> >> + aml_append(dev, >> >> + aml_operation_region("TPP1", AML_SYSTEM_MEMORY, >> >> + aml_int(TPM_PPI_ADDR_BASE), >> >> + sizeof(tpm_ppi->func))); >> >> + field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); >> >> + for (i = 0; i < sizeof(tpm_ppi->func); i++) { >> >> + char *tmp = g_strdup_printf("FN%02X", i); >> >> + aml_append(field, aml_named_field(tmp, BITS_PER_BYTE)); >> >> + g_free(tmp); >> >> + } >> >> + aml_append(dev, field); >> >> + >> >> + /* >> >> + * TPP2 is for the registers that ACPI code used to pass >> >> + * the PPI code and parameter (PPRQ, PPRM) to the firmware. >> >> + */ >> >> + aml_append(dev, >> >> + aml_operation_region("TPP2", AML_SYSTEM_MEMORY, >> >> + aml_int(TPM_PPI_ADDR_BASE + >> >> + offsetof(struct tpm_ppi, ppin)), >> >> + sizeof(struct tpm_ppi) - >> >> + sizeof(tpm_ppi->func))); >> >> + field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); >> >> + aml_append(field, aml_named_field("PPIN", >> >> + sizeof(uint8_t) * BITS_PER_BYTE)); >> > here you already don't use tpm_ppi for sizing, so it creates a confusing mix >> > of both approaches. >> >> True, sizeof_field() will become handy here. I can leave a TODO? > > I'd just use a constant /8/ here like the rest of the code that uses aml_named_field() > (no need to over-complicate or create another precedent to copy from) ok > > Why TODO, changes I suggested for this patch would change it quite heavily > so why merge patch that would be changed by follow up patch almost immediately. > I'd prefer cleaner code being merged unless there are very good reasons > to merge something that should be rewritten afterwards. I thought we should use the upcoming sizeof_field() here. In this case, TODO would make sense to avoid waiting for the other series. > >> >> >> > >> > From ACPI pov I'd prefer PPI table documented somewhere in spec docs >> > (similar docs/specs/acpi_mem_hotplug.txt) and would look like in >> > other use-cases: >> > >> > aml_append(field, aml_named_field("PPIN", 8) >> > >> > and drop "struct tpm_ppi" altogether replacing places it was used by >> > explicit constants. >> >> I have a slight preference for the tpm_ppi structure. But ok with >> replacing it with constant. Stefan, do you agree? >> >> >> + aml_append(field, aml_named_field("PPIP", >> >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> >> + aml_append(field, aml_named_field("PPRP", >> >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> >> + aml_append(field, aml_named_field("PPRQ", >> >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> >> + aml_append(field, aml_named_field("PPRM", >> >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> >> + aml_append(field, aml_named_field("LPPR", >> >> + sizeof(uint32_t) * BITS_PER_BYTE)); >> >> + aml_append(dev, field); >> >> + >> >> + method = aml_method("TPFN", 1, AML_SERIALIZED); >> > not quite sure how this method (supposed to ) work(s), >> > it could use nice comment explaining mechanics. >> > >> >> Windows doesn't like DerefOf (FUNC [N]), it returned wrong values. It >> took me a while to figure this out. My laptop TPM ACPI table uses the >> same trick, so I assume this is a Windows acpi bug/limitation. Instead >> we use a function that returns the corresponding field. >> >> So we declare each field / array entry: >> OperationRegion (TPP1, SystemMemory, 0xFED45000, 0x0100) >> Field (TPP1, AnyAcc, NoLock, Preserve) >> { >> FN00, 8, >> FN01, 8,.... >> >> And the method to access it: >> >> Method (TPFN, 1, Serialized) >> { >> If ((Zero == Arg0)) >> { >> Return (FN00) /* \_SB_.TPM_.FN00 */ >> } >> ..... >> >> >> >> >> + { >> >> + for (i = 0; i < sizeof(tpm_ppi->func); i++) { >> >> + ifctx = aml_if(aml_equal(aml_int(i), aml_arg(0))); >> >> + { >> >> + aml_append(ifctx, aml_return(aml_name("FN%02X", i))); >> >> + } >> >> + aml_append(method, ifctx); >> >> + } >> >> + aml_append(method, aml_return(aml_int(0))); >> >> + } >> >> + aml_append(dev, method); >> >> + >> >> + pak = aml_package(2); >> >> + aml_append(pak, aml_int(0)); >> >> + aml_append(pak, aml_int(0)); >> >> + name = aml_name_decl("TPM2", pak); >> >> + aml_append(dev, name); >> >> + >> >> + pak = aml_package(3); >> >> + aml_append(pak, aml_int(0)); >> >> + aml_append(pak, aml_int(0)); >> >> + aml_append(pak, aml_int(0)); >> >> + name = aml_name_decl("TPM3", pak); >> >> + aml_append(dev, name); >> >> + >> >> + method = aml_method("_DSM", 4, AML_SERIALIZED); >> >> + { >> >> + uint8_t zerobyte[1] = { 0 }; >> >> + >> >> + ifctx = aml_if( >> >> + aml_equal(aml_arg(0), >> >> + aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653"))); >> > a comment pointing to a specific part/version of spec that documents this _DSM >> > for every uuid/function used here would help to review it. >> >> ok, fixing that. >> >> The spec PDF is fairly easy to read imho: >> >> https://trustedcomputinggroup.org/wp-content/uploads/Physical-Presence-Interface_1-30_0-52.pdf >> > >> >> + { >> >> + aml_append(ifctx, >> >> + aml_store(aml_to_integer(aml_arg(2)), aml_local(0))); >> >> + >> >> + /* standard DSM query function */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0))); >> >> + { >> >> + uint8_t byte_list[2] = { 0xff, 0x01 }; >> >> + aml_append(ifctx2, aml_return(aml_buffer(2, byte_list))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* interface version: 1.3 */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1))); >> >> + { >> >> + aml_append(ifctx2, aml_return(aml_string("1.3"))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* submit TPM operation */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2))); >> >> + { >> >> + /* get opcode */ >> >> + aml_append(ifctx2, >> >> + aml_store(aml_derefof(aml_index(aml_arg(3), >> >> + aml_int(0))), >> >> + aml_local(0))); >> >> + /* get opcode flags */ >> >> + aml_append(ifctx2, >> >> + aml_store(aml_call1("TPFN", aml_local(0)), >> >> + aml_local(1))); >> >> + ifctx3 = aml_if( >> >> + aml_equal( >> >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), >> >> + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); >> >> + { >> >> + /* 1: not implemented */ >> >> + aml_append(ifctx3, aml_return(aml_int(1))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + aml_append(ifctx2, aml_store(aml_local(0), aml_name("PPRQ"))); >> >> + aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM"))); >> >> + /* 0: success */ >> >> + aml_append(ifctx2, aml_return(aml_int(0))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* get pending TPM operation */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3))); >> >> + { >> >> + /* revision to integer */ >> >> + aml_append(ifctx2, >> >> + aml_store( >> >> + aml_to_integer(aml_arg(1)), >> >> + aml_local(1))); >> >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); >> >> + { >> >> + aml_append(ifctx3, >> >> + aml_store( >> >> + aml_name("PPRQ"), >> >> + aml_index(aml_name("TPM2"), aml_int(1)))); >> >> + aml_append(ifctx3, aml_return(aml_name("TPM2"))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + >> >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); >> >> + { >> >> + aml_append(ifctx3, >> >> + aml_store( >> >> + aml_name("PPRQ"), >> >> + aml_index(aml_name("TPM3"), aml_int(1)))); >> >> + aml_append(ifctx3, >> >> + aml_store( >> >> + aml_name("PPRM"), >> >> + aml_index(aml_name("TPM3"), aml_int(2)))); >> >> + aml_append(ifctx3, aml_return(aml_name("TPM3"))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* get platform-specific action to transition to pre-OS env. */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4))); >> >> + { >> >> + /* reboot */ >> >> + aml_append(ifctx2, aml_return(aml_int(2))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* get TPM operation response */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5))); >> >> + { >> >> + aml_append(ifctx2, >> >> + aml_store( >> >> + aml_name("LPPR"), >> >> + aml_index(aml_name("TPM3"), aml_int(1)))); >> >> + aml_append(ifctx2, >> >> + aml_store( >> >> + aml_name("PPRP"), >> >> + aml_index(aml_name("TPM3"), aml_int(2)))); >> >> + aml_append(ifctx2, aml_return(aml_name("TPM3"))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* submit preferred user language */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6))); >> >> + { >> >> + /* 3 = not implemented */ >> >> + aml_append(ifctx2, aml_return(aml_int(3))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* submit TPM operation v2 */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7))); >> >> + { >> >> + /* get opcode */ >> >> + aml_append(ifctx2, >> >> + aml_store(aml_derefof(aml_index(aml_arg(3), >> >> + aml_int(0))), >> >> + aml_local(0))); >> >> + /* get opcode flags */ >> >> + aml_append(ifctx2, >> >> + aml_store(aml_call1("TPFN", aml_local(0)), >> >> + aml_local(1))); >> >> + ifctx3 = aml_if( >> >> + aml_equal( >> >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), >> >> + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); >> >> + { >> >> + /* 1: not implemented */ >> >> + aml_append(ifctx3, aml_return(aml_int(1))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + >> >> + ifctx3 = aml_if( >> >> + aml_equal( >> >> + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), >> >> + aml_int(TPM_PPI_FUNC_BLOCKED))); >> >> + { >> >> + /* 3: blocked by firmware */ >> >> + aml_append(ifctx3, aml_return(aml_int(3))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + >> >> + /* revision to integer */ >> >> + aml_append(ifctx2, >> >> + aml_store( >> >> + aml_to_integer(aml_arg(1)), >> >> + aml_local(1))); >> >> + >> >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); >> >> + { >> >> + /* revision 1 */ >> >> + aml_append(ifctx3, aml_store(aml_local(0), >> >> + aml_name("PPRQ"))); >> >> + aml_append(ifctx3, aml_store(aml_int(0), >> >> + aml_name("PPRM"))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + >> >> + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); >> >> + { >> >> + /* revision 2 */ >> >> + aml_append(ifctx3, >> >> + aml_store(aml_local(0), aml_name("PPRQ"))); >> >> + aml_append(ifctx3, >> >> + aml_store( >> >> + aml_derefof(aml_index(aml_arg(3), >> >> + aml_int(1))), >> >> + aml_name("PPRM"))); >> >> + } >> >> + aml_append(ifctx2, ifctx3); >> >> + /* 0: success */ >> >> + aml_append(ifctx2, aml_return(aml_int(0))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + /* get user confirmation status for operation */ >> >> + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8))); >> >> + { >> >> + /* get opcode */ >> >> + aml_append(ifctx2, >> >> + aml_store(aml_derefof(aml_index(aml_arg(3), >> >> + aml_int(0))), >> >> + aml_local(0))); >> >> + /* get opcode flags */ >> >> + aml_append(ifctx2, >> >> + aml_store(aml_call1("TPFN", aml_local(0)), >> >> + aml_local(1))); >> >> + /* return confirmation status code */ >> >> + aml_append(ifctx2, >> >> + aml_return( >> >> + aml_and(aml_local(1), >> >> + aml_int(TPM_PPI_FUNC_MASK), NULL))); >> >> + } >> >> + aml_append(ifctx, ifctx2); >> >> + >> >> + aml_append(ifctx, aml_return(aml_buffer(1, zerobyte))); >> >> + } >> >> + aml_append(method, ifctx); >> >> + } >> >> + aml_append(dev, method); >> >> +} >> >> + >> >> static void >> >> build_dsdt(GArray *table_data, BIOSLinker *linker, >> >> AcpiPmInfo *pm, AcpiMiscInfo *misc, >> >> @@ -2153,6 +2440,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, >> >> */ >> >> /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ >> >> aml_append(dev, aml_name_decl("_CRS", crs)); >> >> + >> >> + build_tpm_ppi(dev); >> >> + >> >> aml_append(scope, dev); >> >> } >> >> >> >> @@ -2172,6 +2462,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, >> >> aml_append(method, aml_return(aml_int(0x0f))); >> >> aml_append(dev, method); >> >> >> >> + build_tpm_ppi(dev); >> >> + >> >> aml_append(sb_scope, dev); >> >> } >> >> >> >> @@ -2918,7 +3210,7 @@ void acpi_setup(void) >> >> tpm_config = (FWCfgTPMConfig) { >> >> .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE), >> >> .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())), >> >> - .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE) >> > what was the point of introducing TPM_PPI_VERSION_NONE if it would not be used? >> > maybe patch ordering is problem? what if we put fwcfg patch after this one? >> >> edk2 already knows that NONE (0) is no PPI. So we at least have to >> keep the same indexing. >> >> I don't see much point in reordering. If you look at v4, you may want >> to squash things together too, but that makes reviewing more >> complicated. As the long as the split is natural, and no regression >> are introduced, I would rather keep it the way it is. >> >> > >> >> + .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30) >> >> }; >> >> fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config", >> >> &tpm_config, sizeof tpm_config); >> > >> > hopefully patch would be more review-able with comments and nicely named >> > variables, so I could actually review it functionality wise without reading >> > whole TPM spec(s) >> >> Let see what I can do. >> >> Thanks! >> >> >
On 06/21/2018 01:10 PM, Marc-André Lureau wrote: > Hi > > On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <imammedo@redhat.com> wrote: >> On Thu, 21 Jun 2018 15:21:02 +0200 >> Marc-André Lureau <marcandre.lureau@gmail.com> wrote: >> >>> Hi >>> >>> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote: >>>> On Tue, 15 May 2018 14:14:32 +0200 >>>> Marc-André Lureau <marcandre.lureau@redhat.com> wrote: >>>> >>>>> From: Stefan Berger <stefanb@linux.vnet.ibm.com> >>>>> >>>>> The TPM Physical Presence interface consists of an ACPI part, a shared >>>>> memory part, and code in the firmware. Users can send messages to the >>>>> firmware by writing a code into the shared memory through invoking the >>>>> ACPI code. When a reboot happens, the firmware looks for the code and >>>>> acts on it by sending sequences of commands to the TPM. >>>>> >>>>> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't >>>>> assume that SMIs are necessary to use. It uses a similar datastructure for >>>>> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use >>>>> of it. I extended the shared memory data structure with an array of 256 >>>>> bytes, one for each code that could be implemented. The array contains >>>>> flags describing the individual codes. This decouples the ACPI implementation >>>>> from the firmware implementation. >>>>> >>>>> The underlying TCG specification is accessible from the following page. >>>>> >>>>> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ >>>>> >>>>> This patch implements version 1.30. >>>>> >>>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> >>>>> >>>>> --- >>>>> >>>>> v4 (Marc-André): >>>>> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI >>>>> handling. >>>>> - replace 'return Package (..) {} ' with scoped variables, to fix >>>>> Windows ACPI handling. >>>>> >>>>> v3: >>>>> - add support for PPI to CRB >>>>> - split up OperationRegion TPPI into two parts, one containing >>>>> the registers (TPP1) and the other one the flags (TPP2); switched >>>>> the order of the flags versus registers in the code >>>>> - adapted ACPI code to small changes to the array of flags where >>>>> previous flag 0 was removed and now shifting right wasn't always >>>>> necessary anymore >>>>> >>>>> v2: >>>>> - get rid of FAIL variable; function 5 was using it and always >>>>> returns 0; the value is related to the ACPI function call not >>>>> a possible failure of the TPM function call. >>>>> - extend shared memory data structure with per-opcode entries >>>>> holding flags and use those flags to determine what to return >>>>> to caller >>>>> - implement interface version 1.3 >>>>> --- >>>>> include/hw/acpi/tpm.h | 21 +++ >>>>> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- >>>>> 2 files changed, 314 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h >>>>> index f79d68a77a..fc53f08827 100644 >>>>> --- a/include/hw/acpi/tpm.h >>>>> +++ b/include/hw/acpi/tpm.h >>>>> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) >>>>> #define TPM_PPI_VERSION_NONE 0 >>>>> #define TPM_PPI_VERSION_1_30 1 >>>>> >>>>> +struct tpm_ppi { >>>>> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; >>>>> + set by BIOS */ >>>>> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ >>>>> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) >>>>> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) >>>>> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) >>>>> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) >>>>> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) >>>>> +#define TPM_PPI_FUNC_MASK (7 << 0) >>>>> + uint8_t ppin; /* 0x100 : set by BIOS */ >>>>> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ >>>>> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ >>>>> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ >>>>> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ >>>>> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ >>>>> + uint32_t fret; /* 0x115 : set by ACPI; not used */ >>>>> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ >>>>> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ >>>>> +} QEMU_PACKED; >>>> is this structure plant to be used for accessing data from within QEMU >>>> or it is just used for convenience of calculating offsets/sizes for AML? >>>> >>>> >>> For convenience (and it can be useful for debugging). >> we are gradually getting rid of usage "struct foo" in ACPI code >> (i.e. when I have time to convert old struct based approach >> to build_append_int() table based format). >> and for new code I usually require ACPI parts be struct less >> (if there is no previous struct baggage to back it up). >> > The tpm_ppi struct is not an ACPI table, it's a fixed memory region / > layout to exchange between guest/os and firmware. > >> Hence my request to drop dummy struct and document layout >> properly in related spec doc (that's where FW should look for >> documentation and not into struct definition somewhere in >> the header). So I'd strongly prefer it be done this way. > There is not much more than the field description for me ;). Stefan, > would you like to develop the structure usage? Will do.
On Thu, 21 Jun 2018 19:10:38 +0200 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Thu, Jun 21, 2018 at 4:23 PM, Igor Mammedov <imammedo@redhat.com> wrote: > > On Thu, 21 Jun 2018 15:21:02 +0200 > > Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > > >> Hi > >> > >> On Thu, Jun 21, 2018 at 2:54 PM, Igor Mammedov <imammedo@redhat.com> wrote: > >> > On Tue, 15 May 2018 14:14:32 +0200 > >> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > >> > > >> >> From: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> >> > >> >> The TPM Physical Presence interface consists of an ACPI part, a shared > >> >> memory part, and code in the firmware. Users can send messages to the > >> >> firmware by writing a code into the shared memory through invoking the > >> >> ACPI code. When a reboot happens, the firmware looks for the code and > >> >> acts on it by sending sequences of commands to the TPM. > >> >> > >> >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't > >> >> assume that SMIs are necessary to use. It uses a similar datastructure for > >> >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use > >> >> of it. I extended the shared memory data structure with an array of 256 > >> >> bytes, one for each code that could be implemented. The array contains > >> >> flags describing the individual codes. This decouples the ACPI implementation > >> >> from the firmware implementation. > >> >> > >> >> The underlying TCG specification is accessible from the following page. > >> >> > >> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/ > >> >> > >> >> This patch implements version 1.30. > >> >> > >> >> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> > >> >> > >> >> --- > >> >> > >> >> v4 (Marc-André): > >> >> - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI > >> >> handling. > >> >> - replace 'return Package (..) {} ' with scoped variables, to fix > >> >> Windows ACPI handling. > >> >> > >> >> v3: > >> >> - add support for PPI to CRB > >> >> - split up OperationRegion TPPI into two parts, one containing > >> >> the registers (TPP1) and the other one the flags (TPP2); switched > >> >> the order of the flags versus registers in the code > >> >> - adapted ACPI code to small changes to the array of flags where > >> >> previous flag 0 was removed and now shifting right wasn't always > >> >> necessary anymore > >> >> > >> >> v2: > >> >> - get rid of FAIL variable; function 5 was using it and always > >> >> returns 0; the value is related to the ACPI function call not > >> >> a possible failure of the TPM function call. > >> >> - extend shared memory data structure with per-opcode entries > >> >> holding flags and use those flags to determine what to return > >> >> to caller > >> >> - implement interface version 1.3 > >> >> --- > >> >> include/hw/acpi/tpm.h | 21 +++ > >> >> hw/i386/acpi-build.c | 294 +++++++++++++++++++++++++++++++++++++++++- > >> >> 2 files changed, 314 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h > >> >> index f79d68a77a..fc53f08827 100644 > >> >> --- a/include/hw/acpi/tpm.h > >> >> +++ b/include/hw/acpi/tpm.h > >> >> @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) > >> >> #define TPM_PPI_VERSION_NONE 0 > >> >> #define TPM_PPI_VERSION_1_30 1 > >> >> > >> >> +struct tpm_ppi { > >> >> + uint8_t func[256]; /* 0x000: per TPM function implementation flags; > >> >> + set by BIOS */ > >> >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ > >> >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) > >> >> +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) > >> >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0) > >> >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) > >> >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) > >> >> +#define TPM_PPI_FUNC_MASK (7 << 0) > >> >> + uint8_t ppin; /* 0x100 : set by BIOS */ > >> >> + uint32_t ppip; /* 0x101 : set by ACPI; not used */ > >> >> + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ > >> >> + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ > >> >> + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ > >> >> + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ > >> >> + uint32_t fret; /* 0x115 : set by ACPI; not used */ > >> >> + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ > >> >> + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ > >> >> +} QEMU_PACKED; > >> > is this structure plant to be used for accessing data from within QEMU > >> > or it is just used for convenience of calculating offsets/sizes for AML? > >> > > >> > > >> > >> For convenience (and it can be useful for debugging). > > we are gradually getting rid of usage "struct foo" in ACPI code > > (i.e. when I have time to convert old struct based approach > > to build_append_int() table based format). > > and for new code I usually require ACPI parts be struct less > > (if there is no previous struct baggage to back it up). > > > > The tpm_ppi struct is not an ACPI table, it's a fixed memory region / > layout to exchange between guest/os and firmware. it's interface between ACPI and firmware and it's used in ACPI parts of code (mainly as means to document layout). I wouldn't care much if it were buried inside of TPM code, but it's used mainly as dummy struct for documenting and by ACPI parts of code for an implicit sizing math, for that I'd like you to use spec/constants like any other new ACPI code does. So lets keep things consistent here at least for ACPI parts that I maintain. > > Hence my request to drop dummy struct and document layout > > properly in related spec doc (that's where FW should look for > > documentation and not into struct definition somewhere in > > the header). So I'd strongly prefer it be done this way. > > There is not much more than the field description for me ;). Stefan, > would you like to develop the structure usage? You should use table format to document ACPI interface in spec (ex: docs/specs/acpi_mem_hotplug.txt) at least for consistence sake. I don't care that firmware side uses C code as documentation as it's up whatever maintainers prefer over there. [...]
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h index f79d68a77a..fc53f08827 100644 --- a/include/hw/acpi/tpm.h +++ b/include/hw/acpi/tpm.h @@ -196,4 +196,25 @@ REG32(CRB_DATA_BUFFER, 0x80) #define TPM_PPI_VERSION_NONE 0 #define TPM_PPI_VERSION_1_30 1 +struct tpm_ppi { + uint8_t func[256]; /* 0x000: per TPM function implementation flags; + set by BIOS */ +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */ +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0) +#define TPM_PPI_FUNC_BIOS_ONLY (1 << 0) +#define TPM_PPI_FUNC_BLOCKED (2 << 0) +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0) +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0) +#define TPM_PPI_FUNC_MASK (7 << 0) + uint8_t ppin; /* 0x100 : set by BIOS */ + uint32_t ppip; /* 0x101 : set by ACPI; not used */ + uint32_t pprp; /* 0x105 : response from TPM; set by BIOS */ + uint32_t pprq; /* 0x109 : opcode; set by ACPI */ + uint32_t pprm; /* 0x10d : parameter for opcode; set by ACPI */ + uint32_t lppr; /* 0x111 : last opcode; set by BIOS */ + uint32_t fret; /* 0x115 : set by ACPI; not used */ + uint8_t res1[0x40]; /* 0x119 : reserved for future use */ + uint8_t next_step; /* 0x159 : next step after reboot; set by BIOS */ +} QEMU_PACKED; + #endif /* HW_ACPI_TPM_H */ diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index f6d447f03a..95be4f0710 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -43,6 +43,7 @@ #include "hw/acpi/memory_hotplug.h" #include "sysemu/tpm.h" #include "hw/acpi/tpm.h" +#include "hw/tpm/tpm_ppi.h" #include "hw/acpi/vmgenid.h" #include "sysemu/tpm_backend.h" #include "hw/timer/mc146818rtc_regs.h" @@ -1789,6 +1790,292 @@ static Aml *build_q35_osc_method(void) return method; } +static void +build_tpm_ppi(Aml *dev) +{ + Aml *method, *name, *field, *ifctx, *ifctx2, *ifctx3, *pak; + struct tpm_ppi *tpm_ppi = NULL; + int i; + + /* + * TPP1 is for the flags that indicate which PPI operations + * are supported by the firmware. The firmware is expected to + * write these flags. + */ + aml_append(dev, + aml_operation_region("TPP1", AML_SYSTEM_MEMORY, + aml_int(TPM_PPI_ADDR_BASE), + sizeof(tpm_ppi->func))); + field = aml_field("TPP1", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); + for (i = 0; i < sizeof(tpm_ppi->func); i++) { + char *tmp = g_strdup_printf("FN%02X", i); + aml_append(field, aml_named_field(tmp, BITS_PER_BYTE)); + g_free(tmp); + } + aml_append(dev, field); + + /* + * TPP2 is for the registers that ACPI code used to pass + * the PPI code and parameter (PPRQ, PPRM) to the firmware. + */ + aml_append(dev, + aml_operation_region("TPP2", AML_SYSTEM_MEMORY, + aml_int(TPM_PPI_ADDR_BASE + + offsetof(struct tpm_ppi, ppin)), + sizeof(struct tpm_ppi) - + sizeof(tpm_ppi->func))); + field = aml_field("TPP2", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE); + aml_append(field, aml_named_field("PPIN", + sizeof(uint8_t) * BITS_PER_BYTE)); + aml_append(field, aml_named_field("PPIP", + sizeof(uint32_t) * BITS_PER_BYTE)); + aml_append(field, aml_named_field("PPRP", + sizeof(uint32_t) * BITS_PER_BYTE)); + aml_append(field, aml_named_field("PPRQ", + sizeof(uint32_t) * BITS_PER_BYTE)); + aml_append(field, aml_named_field("PPRM", + sizeof(uint32_t) * BITS_PER_BYTE)); + aml_append(field, aml_named_field("LPPR", + sizeof(uint32_t) * BITS_PER_BYTE)); + aml_append(dev, field); + + method = aml_method("TPFN", 1, AML_SERIALIZED); + { + for (i = 0; i < sizeof(tpm_ppi->func); i++) { + ifctx = aml_if(aml_equal(aml_int(i), aml_arg(0))); + { + aml_append(ifctx, aml_return(aml_name("FN%02X", i))); + } + aml_append(method, ifctx); + } + aml_append(method, aml_return(aml_int(0))); + } + aml_append(dev, method); + + pak = aml_package(2); + aml_append(pak, aml_int(0)); + aml_append(pak, aml_int(0)); + name = aml_name_decl("TPM2", pak); + aml_append(dev, name); + + pak = aml_package(3); + aml_append(pak, aml_int(0)); + aml_append(pak, aml_int(0)); + aml_append(pak, aml_int(0)); + name = aml_name_decl("TPM3", pak); + aml_append(dev, name); + + method = aml_method("_DSM", 4, AML_SERIALIZED); + { + uint8_t zerobyte[1] = { 0 }; + + ifctx = aml_if( + aml_equal(aml_arg(0), + aml_touuid("3DDDFAA6-361B-4EB4-A424-8D10089D1653"))); + { + aml_append(ifctx, + aml_store(aml_to_integer(aml_arg(2)), aml_local(0))); + + /* standard DSM query function */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(0))); + { + uint8_t byte_list[2] = { 0xff, 0x01 }; + aml_append(ifctx2, aml_return(aml_buffer(2, byte_list))); + } + aml_append(ifctx, ifctx2); + + /* interface version: 1.3 */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(1))); + { + aml_append(ifctx2, aml_return(aml_string("1.3"))); + } + aml_append(ifctx, ifctx2); + + /* submit TPM operation */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(2))); + { + /* get opcode */ + aml_append(ifctx2, + aml_store(aml_derefof(aml_index(aml_arg(3), + aml_int(0))), + aml_local(0))); + /* get opcode flags */ + aml_append(ifctx2, + aml_store(aml_call1("TPFN", aml_local(0)), + aml_local(1))); + ifctx3 = aml_if( + aml_equal( + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); + { + /* 1: not implemented */ + aml_append(ifctx3, aml_return(aml_int(1))); + } + aml_append(ifctx2, ifctx3); + aml_append(ifctx2, aml_store(aml_local(0), aml_name("PPRQ"))); + aml_append(ifctx2, aml_store(aml_int(0), aml_name("PPRM"))); + /* 0: success */ + aml_append(ifctx2, aml_return(aml_int(0))); + } + aml_append(ifctx, ifctx2); + + /* get pending TPM operation */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(3))); + { + /* revision to integer */ + aml_append(ifctx2, + aml_store( + aml_to_integer(aml_arg(1)), + aml_local(1))); + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); + { + aml_append(ifctx3, + aml_store( + aml_name("PPRQ"), + aml_index(aml_name("TPM2"), aml_int(1)))); + aml_append(ifctx3, aml_return(aml_name("TPM2"))); + } + aml_append(ifctx2, ifctx3); + + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); + { + aml_append(ifctx3, + aml_store( + aml_name("PPRQ"), + aml_index(aml_name("TPM3"), aml_int(1)))); + aml_append(ifctx3, + aml_store( + aml_name("PPRM"), + aml_index(aml_name("TPM3"), aml_int(2)))); + aml_append(ifctx3, aml_return(aml_name("TPM3"))); + } + aml_append(ifctx2, ifctx3); + } + aml_append(ifctx, ifctx2); + + /* get platform-specific action to transition to pre-OS env. */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(4))); + { + /* reboot */ + aml_append(ifctx2, aml_return(aml_int(2))); + } + aml_append(ifctx, ifctx2); + + /* get TPM operation response */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(5))); + { + aml_append(ifctx2, + aml_store( + aml_name("LPPR"), + aml_index(aml_name("TPM3"), aml_int(1)))); + aml_append(ifctx2, + aml_store( + aml_name("PPRP"), + aml_index(aml_name("TPM3"), aml_int(2)))); + aml_append(ifctx2, aml_return(aml_name("TPM3"))); + } + aml_append(ifctx, ifctx2); + + /* submit preferred user language */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(6))); + { + /* 3 = not implemented */ + aml_append(ifctx2, aml_return(aml_int(3))); + } + aml_append(ifctx, ifctx2); + + /* submit TPM operation v2 */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(7))); + { + /* get opcode */ + aml_append(ifctx2, + aml_store(aml_derefof(aml_index(aml_arg(3), + aml_int(0))), + aml_local(0))); + /* get opcode flags */ + aml_append(ifctx2, + aml_store(aml_call1("TPFN", aml_local(0)), + aml_local(1))); + ifctx3 = aml_if( + aml_equal( + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), + aml_int(TPM_PPI_FUNC_NOT_IMPLEMENTED))); + { + /* 1: not implemented */ + aml_append(ifctx3, aml_return(aml_int(1))); + } + aml_append(ifctx2, ifctx3); + + ifctx3 = aml_if( + aml_equal( + aml_and(aml_local(1), aml_int(TPM_PPI_FUNC_MASK), NULL), + aml_int(TPM_PPI_FUNC_BLOCKED))); + { + /* 3: blocked by firmware */ + aml_append(ifctx3, aml_return(aml_int(3))); + } + aml_append(ifctx2, ifctx3); + + /* revision to integer */ + aml_append(ifctx2, + aml_store( + aml_to_integer(aml_arg(1)), + aml_local(1))); + + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(1))); + { + /* revision 1 */ + aml_append(ifctx3, aml_store(aml_local(0), + aml_name("PPRQ"))); + aml_append(ifctx3, aml_store(aml_int(0), + aml_name("PPRM"))); + } + aml_append(ifctx2, ifctx3); + + ifctx3 = aml_if(aml_equal(aml_local(1), aml_int(2))); + { + /* revision 2 */ + aml_append(ifctx3, + aml_store(aml_local(0), aml_name("PPRQ"))); + aml_append(ifctx3, + aml_store( + aml_derefof(aml_index(aml_arg(3), + aml_int(1))), + aml_name("PPRM"))); + } + aml_append(ifctx2, ifctx3); + /* 0: success */ + aml_append(ifctx2, aml_return(aml_int(0))); + } + aml_append(ifctx, ifctx2); + + /* get user confirmation status for operation */ + ifctx2 = aml_if(aml_equal(aml_local(0), aml_int(8))); + { + /* get opcode */ + aml_append(ifctx2, + aml_store(aml_derefof(aml_index(aml_arg(3), + aml_int(0))), + aml_local(0))); + /* get opcode flags */ + aml_append(ifctx2, + aml_store(aml_call1("TPFN", aml_local(0)), + aml_local(1))); + /* return confirmation status code */ + aml_append(ifctx2, + aml_return( + aml_and(aml_local(1), + aml_int(TPM_PPI_FUNC_MASK), NULL))); + } + aml_append(ifctx, ifctx2); + + aml_append(ifctx, aml_return(aml_buffer(1, zerobyte))); + } + aml_append(method, ifctx); + } + aml_append(dev, method); +} + static void build_dsdt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm, AcpiMiscInfo *misc, @@ -2153,6 +2440,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, */ /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */ aml_append(dev, aml_name_decl("_CRS", crs)); + + build_tpm_ppi(dev); + aml_append(scope, dev); } @@ -2172,6 +2462,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, aml_append(method, aml_return(aml_int(0x0f))); aml_append(dev, method); + build_tpm_ppi(dev); + aml_append(sb_scope, dev); } @@ -2918,7 +3210,7 @@ void acpi_setup(void) tpm_config = (FWCfgTPMConfig) { .tpmppi_address = cpu_to_le32(TPM_PPI_ADDR_BASE), .tpm_version = cpu_to_le32(tpm_get_version(tpm_find())), - .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_NONE) + .tpmppi_version = cpu_to_le32(TPM_PPI_VERSION_1_30) }; fw_cfg_add_file(pcms->fw_cfg, "etc/tpm/config", &tpm_config, sizeof tpm_config);