[SLOF] [PATCH v4 00/33] Add vTPM support to SLOF

[Alexey Kardashevskiy]

On 19/12/2019 11:27, Stefan Berger wrote:
> On 12/18/19 6:05 PM, Alexey Kardashevskiy wrote:
>> Hey,
>>
>> It's been a while since the last attempt :) 33 is a lot! Comments below...
>>
>> On 12/12/2019 07:26, Stefan Berger wrote:
>>> I am reposting this series of patches for adding vTPM support
>>> to SLOF. The series has grown over time due to addition of
>>> vTPM 2.0 support. The vTPM (1.2 & 2) SLOF code leans on the code I
>> Where these versions are from? PAPR (I did not look deep though) or something else? Do we have to/want to implement
> 
> There's a firmware document for Power vTPM support.

What is the name? Is it publicly available? All these questions need to be answered in commit logs or some documentation.


> That's where some of the Power relevant parts are from, such as API 
> support. Otherwise the code, as said, is very much leaning on the contributions I made to SeaBIOS and where Kevin also 
> made changes to.
> 
> 
>> anything but v2.0? What other pieces need SLOF to support v1.2 - grub, linux, qemu, aix, freebsd?
> 
> 
> The issue, at least for me, is the stack of patches...

Ditch the ones starting with "tpm:" and see what breaks? :)

I really do not understand the problem - why do we need both versions if only v2 is ever going to be used. Or not.

> 
> 
>>
>>
>>> upstreamed to SeaBIOS and where Kevin O'Connor (cc'ed) has also made
>>> changes to and given me permission to contribute the combined code to
>>> SLOF under the BSD license. One goal is to keep the two code bases in
>>> sync as much as possible.
>>>
>>> I expect that PAPR vTPM support will become available in QEMU 5.0.
>> 01/33 refers to tpm-next+spapr.v3 and there is a newer v6 and now you say it is qemu 5.0, which statement is correct?
> 
> Over the last few days I worked on subsequent submission to qemu-devel. v3 works fine with this series. The intention is 
> that vTPM support for pSeries will go into QEMU 5.0.

Then remove that part in the doc about building qemu and other lib, it only confuses.

> 
>>
>> 01/33 refers to libtpms/swtpm - are these standard libraries/tools available from distros (my ubuntu 18.04 does not have
>> libtpms). Does qemu v5.0 depend on these? Or these can be added as submodules? Do we need both libraries? It must be
>> documented then somewhere so you do not have to document it in 01/33. A few command line examples (one for qemu and one
>> for swtpm) in the cover letter should be enough.
> 
> It's swtpm that needs libtpms. Swtpm is either started by libvirt or has to be started manually as described here (file 
> will be converted to .rst):
> 
> https://github.com/qemu/qemu/blob/master/docs/specs/tpm.txt
> 
> It's been packaged for Fedora and will appear in RHAV. Not all distros may have it, yet.

Ah ok.


> 
> 
>> In general, it is too many small patches and later patches change what earlier patch in the series did, such as 26/33,
>> 31/33, please avoid that - it might represent how you developed those but it is useless when bisecting.
>> Either add commit logs into 29/33, 30/33, 31/33, 33/33, or (better) merge them in one patch.
> 
> 
> Sure, I can do that.
> 
> 
>>
>> Also please organize the series as:
>> 1. prerequisites (03/33, 28/33,...)
>> 2. vtpm v1.2 driver (if we still need it)
>> 3. vtpm v2.0 (avoid reorganising code from the previous step)
>> 4. implement menu item
> 
> Ok, I will move the prerequisites to the top but would then like to build TPM 2 on top of TPM 1.2 support, meaning all 
> TPM 1.2 patches come first, then we add TPM 2.
> 
> 
>>
>> btw do we really want the menu? Can this (whatever the menu does) be done by the QEMU command line, some properties of
>> the new tpm-spapr device? Thanks,
> 
> 
> There are aspects of TPM state that can only be controlled via a firmware menu after initialization of the TPM and 
> before the firmware passes control to the OS loader. That's why physical machines have TPM menus in the firmware.

QEMU can put a property in the device tree, SLOF can read it and switch banks. Why does this have to be a SLOF menu 
rather than an xml option in libvirt? Physical machines do not have hypervisors but our VMs do.


btw you posted patches from @linux.vnet.ibm.com domain but replying from @linux.ibm.com, this confuses the maillist's 
mailman. Just sayin' :)  Thanks,


-- 
Alexey