From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
with TPM2 (I haven't tested TPM1, for lack of interest).

PPI test runs successfully with Windows 10 WHLK, despite the limited
number of supported funcions (tpm2_ppi_funcs table, in particular, no
function allows to manipulate Tcg2PhysicalPresenceFlags)

The way it works is relatively simple: a memory region is allocated by
QEMU to save PPI related variables. An ACPI interface is exposed by
QEMU to let the guest manipulate those. At boot, ovmf processes and
updates the PPI qemu region and request variables.

I build edk2 with:

$ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE

I test with qemu & swtpm/libtpms (tpm2 branches, swtpm_setup.sh --tpm2 --tpm-state tpmstatedir)

$ swtpm socket --tpmstate tpmstatedir --ctrl type=unixio,path=tpmsock  --tpm2 &
$ qemu .. -chardev socket,id=chrtpm,path=tpmsock -tpmdev emulator,id=tpm0,chardev=chrtpm -device tpm-crb,tpmdev=tpm0

Github trees:
https://github.com/elmarco/edk2/tree/tpm-ppi
https://github.com/elmarco/qemu/tree/tpm-ppi

Thanks

Marc-André Lureau (4):
  ovmf: add and link with Tcg2PhysicalPresenceLibNull when !TPM2_ENABLE
  ovmf: add QemuTpm.h header
  ovmf: replace SecurityPkg with OvfmPkg Tcg2PhysicalPresenceLibQemu
  ovmf: process TPM PPI request in AfterConsole()

 OvmfPkg/Include/IndustryStandard/QemuTpm.h    |  67 ++
 .../PlatformBootManagerLib/BdsPlatform.c      |   8 +
 .../PlatformBootManagerLib.inf                |   2 +
 .../DxeTcg2PhysicalPresenceLib.c              |  26 +
 .../DxeTcg2PhysicalPresenceLib.inf            |  34 +
 .../DxeTcg2PhysicalPresenceLib.c              | 881 ++++++++++++++++++
 .../DxeTcg2PhysicalPresenceLib.inf            |  67 ++
 .../DxeTcg2PhysicalPresenceLib.uni            |  26 +
 .../PhysicalPresenceStrings.uni               |  49 +
 OvmfPkg/OvmfPkgIa32.dsc                       |   4 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   4 +-
 OvmfPkg/OvmfPkgX64.dsc                        |   4 +-
 12 files changed, 1169 insertions(+), 3 deletions(-)
 create mode 100644 OvmfPkg/Include/IndustryStandard/QemuTpm.h
 create mode 100644 OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.c
 create mode 100644 OvmfPkg/Library/Tcg2PhysicalPresenceLibNull/DxeTcg2PhysicalPresenceLib.inf
 create mode 100644 OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.c
 create mode 100644 OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.inf
 create mode 100644 OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/DxeTcg2PhysicalPresenceLib.uni
 create mode 100644 OvmfPkg/Library/Tcg2PhysicalPresenceLibQemu/PhysicalPresenceStrings.uni

-- 
2.17.0.253.g3dd125b46d

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 05/15/18 14:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Hi,
> 
> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
> with TPM2 (I haven't tested TPM1, for lack of interest).
> 
> PPI test runs successfully with Windows 10 WHLK, despite the limited
> number of supported funcions (tpm2_ppi_funcs table, in particular, no
> function allows to manipulate Tcg2PhysicalPresenceFlags)
> 
> The way it works is relatively simple: a memory region is allocated by
> QEMU to save PPI related variables. An ACPI interface is exposed by
> QEMU to let the guest manipulate those. At boot, ovmf processes and
> updates the PPI qemu region and request variables.
> 
> I build edk2 with:
> 
> $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE

Is -DSECURE_BOOT_ENABLE necessary for *building* with -DTPM2_ENABLE? If
that's the case, we should update the DSC files; users building OVMF
from source shouldn't have to care about "-D" inter-dependencies, if we
can manage that somehow.

If -DSECURE_BOOT_ENABLE is only there because otherwise a guest OS
doesn't really make use of -DTPM2_ENABLE either, that's different. In
that case, it's fine to allow building OVMF with TPM2 support but
without SB support.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi

On Thu, May 17, 2018 at 9:54 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/15/18 14:30, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Hi,
>>
>> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
>> with TPM2 (I haven't tested TPM1, for lack of interest).
>>
>> PPI test runs successfully with Windows 10 WHLK, despite the limited
>> number of supported funcions (tpm2_ppi_funcs table, in particular, no
>> function allows to manipulate Tcg2PhysicalPresenceFlags)
>>
>> The way it works is relatively simple: a memory region is allocated by
>> QEMU to save PPI related variables. An ACPI interface is exposed by
>> QEMU to let the guest manipulate those. At boot, ovmf processes and
>> updates the PPI qemu region and request variables.
>>
>> I build edk2 with:
>>
>> $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE
>
> Is -DSECURE_BOOT_ENABLE necessary for *building* with -DTPM2_ENABLE? If
> that's the case, we should update the DSC files; users building OVMF
> from source shouldn't have to care about "-D" inter-dependencies, if we
> can manage that somehow.

No, that's only my build setup, because it is likely both will be used
together. TPM usage/tests seem to be fine without it.

>
> If -DSECURE_BOOT_ENABLE is only there because otherwise a guest OS
> doesn't really make use of -DTPM2_ENABLE either, that's different. In
> that case, it's fine to allow building OVMF with TPM2 support but
> without SB support.
>
> Thanks!
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 05/17/18 09:54, Laszlo Ersek wrote:
> On 05/15/18 14:30, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Hi,
>>
>> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
>> with TPM2 (I haven't tested TPM1, for lack of interest).
>>
>> PPI test runs successfully with Windows 10 WHLK, despite the limited
>> number of supported funcions (tpm2_ppi_funcs table, in particular, no
>> function allows to manipulate Tcg2PhysicalPresenceFlags)
>>
>> The way it works is relatively simple: a memory region is allocated by
>> QEMU to save PPI related variables. An ACPI interface is exposed by
>> QEMU to let the guest manipulate those. At boot, ovmf processes and
>> updates the PPI qemu region and request variables.
>>
>> I build edk2 with:
>>
>> $ build -DTPM2_ENABLE -DSECURE_BOOT_ENABLE
> 
> Is -DSECURE_BOOT_ENABLE necessary for *building* with -DTPM2_ENABLE? If
> that's the case, we should update the DSC files; users building OVMF
> from source shouldn't have to care about "-D" inter-dependencies, if we
> can manage that somehow.
> 
> If -DSECURE_BOOT_ENABLE is only there because otherwise a guest OS
> doesn't really make use of -DTPM2_ENABLE either, that's different. In
> that case, it's fine to allow building OVMF with TPM2 support but
> without SB support.

Oops, almost missed another important omission: in every commit message,
please insert the following line just above your S-o-b:

Contributed-under: TianoCore Contribution Agreement 1.1

We cannot take patches without that line. You can read about it in the
"Contributions.txt" file, in the project root directory.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi Marc-André,

On 05/15/18 14:30, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
> with TPM2 (I haven't tested TPM1, for lack of interest).

I got the review of this patch series added to my TODO list, but I'll
have to ask for your patience. :(

From an extremely superficial skim:

* please use the

    TopDirPkg/ModuleName: blah blah blah

  subject format, or more generally, if a module cannot be identified,

    TopDirPkg: blah blah blah

* the subject line and the commit message shouldn't be wider than 74
  chars;

* edk2 uses two spaces for general indentation, and I'm noticing some
  inconsistency there (4 spaces like in QEMU).

* Please consider formatting the patches with "--find-copies-harder"
  (although I can look at them with the same option after fetching the
  series from your repo). This option is usually helpful for reviewers
  when cloning and modifying modules cross-package.

* Please consider adopting the git settings at
  <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>,
  in particular:

  - "--stat=1000 --stat-graph-width=20", so that pathnames are not
    truncated in the diffstats,

  - the "xfuncname"-related settings, so that git diff hunk headers @@
    are useful for DSC and INF files too,

  - the diff order file, so that files are listed in patches in logical
    order, going from abstract / descriptive (.inf, .h) to concrete /
    imperative (.c).

Not much of a review, I know; this is all I can offer right now. If you
have the time to respin just with these superficial changes, that might
make my life easier. If you prefer to delay them, that's 100% fine too.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 05/16/18 11:29, Laszlo Ersek wrote:
> Hi Marc-André,
> 
> On 05/15/18 14:30, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Hi,
>>
>> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
>> with TPM2 (I haven't tested TPM1, for lack of interest).

Here's another general request: please make sure that all code you add
(new lines in existent files, and especially brand new files) use CRLF
line terminators.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Hi

On Wed, May 16, 2018 at 11:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi Marc-André,
>
> On 05/15/18 14:30, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Hi,
>>
>> The following series adds basic TPM PPI 1.3 support for OVMF-on-QEMU
>> with TPM2 (I haven't tested TPM1, for lack of interest).
>
> I got the review of this patch series added to my TODO list, but I'll
> have to ask for your patience. :(
>
> From an extremely superficial skim:
>
> * please use the
>
>     TopDirPkg/ModuleName: blah blah blah
>
>   subject format, or more generally, if a module cannot be identified,
>
>     TopDirPkg: blah blah blah
>

done

> * the subject line and the commit message shouldn't be wider than 74
>   chars;
>

that should be ok

> * edk2 uses two spaces for general indentation, and I'm noticing some
>   inconsistency there (4 spaces like in QEMU).

yes, I tried to respect that, but sometime fail (emacs c-basic-offset
2 isn't great with comments)

>
> * Please consider formatting the patches with "--find-copies-harder"
>   (although I can look at them with the same option after fetching the
>   series from your repo). This option is usually helpful for reviewers
>   when cloning and modifying modules cross-package.

Hmm, I didn't know that option, ok

>
> * Please consider adopting the git settings at
>   <https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers>,
>   in particular:
>
>   - "--stat=1000 --stat-graph-width=20", so that pathnames are not
>     truncated in the diffstats,
>

I use git-publish very often. I had to modify it to pass those options
(https://github.com/stefanha/git-publish/pull/48)

>   - the "xfuncname"-related settings, so that git diff hunk headers @@
>     are useful for DSC and INF files too,
>

This is already in my .git/config, I hope it takes it by default in
format-patch?

>   - the diff order file, so that files are listed in patches in logical
>     order, going from abstract / descriptive (.inf, .h) to concrete /
>     imperative (.c).
>

ok

> Not much of a review, I know; this is all I can offer right now. If you
> have the time to respin just with these superficial changes, that might
> make my life easier. If you prefer to delay them, that's 100% fine too.
>

I am going to resend with the style fixes.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 05/17/18 16:43, Marc-André Lureau wrote:
> On Wed, May 16, 2018 at 11:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:

>>   - the "xfuncname"-related settings, so that git diff hunk headers @@
>>     are useful for DSC and INF files too,
>>
> 
> This is already in my .git/config, I hope it takes it by default in
> format-patch?

You also need to classify files appropriately so that the xfuncname
setting apply to them:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09

>> Not much of a review, I know; this is all I can offer right now. If you
>> have the time to respin just with these superficial changes, that might
>> make my life easier. If you prefer to delay them, that's 100% fine too.
>>
> 
> I am going to resend with the style fixes.

I managed to review your v1 in full earlier today, so I'd prefer to
review your v3, with my more in-depth comments addressed as well.

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel