Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-17 Thread Laszlo Ersek
On 05/17/18 16:43, Marc-André Lureau wrote:
> On Wed, May 16, 2018 at 11:29 AM, Laszlo Ersek  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



Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-17 Thread Marc-André Lureau
Hi

On Thu, May 17, 2018 at 9:54 AM, Laszlo Ersek  wrote:
> On 05/15/18 14:30, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> 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



Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-17 Thread Marc-André Lureau
Hi

On Wed, May 16, 2018 at 11:29 AM, Laszlo Ersek  wrote:
> Hi Marc-André,
>
> On 05/15/18 14:30, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> 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
>   
> ,
>   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.



Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-17 Thread Laszlo Ersek
On 05/17/18 09:54, Laszlo Ersek wrote:
> On 05/15/18 14:30, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> 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



Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-17 Thread Laszlo Ersek
On 05/15/18 14:30, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> 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



Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-17 Thread Laszlo Ersek
On 05/16/18 11:29, Laszlo Ersek wrote:
> Hi Marc-André,
> 
> On 05/15/18 14:30, marcandre.lur...@redhat.com wrote:
>> From: Marc-André Lureau 
>>
>> 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



Re: [Qemu-devel] [edk2] [PATCH 0/4] RFC: ovmf: Add support for TPM Physical Presence interface

2018-05-16 Thread Laszlo Ersek
Hi Marc-André,

On 05/15/18 14:30, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
>
> 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
  
,
  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