[SLOF] [PATCH v2 00/20] Add vTPM support to SLOF

[Thomas Huth]

On 17/11/15 18:02, Stefan Berger wrote:
> The following series of patches adds TPM support to SLOF.
> In particular it adds the following:
> 
> - TPM drivers for hardware interface and CRQ interface
> - TPM initialization
> - TPM logging area and firmware API to transfer it to the OS
>   (measurements are visible in sysfs)
> - Some measurement code (Static Core Root Of Trust)
> - TPM menu (accessible via 't' key during boot if TPM is available)
> - Firmware API extensions following Power Firmware Doc
>   (to make trusted grub work)
> 
> 
> Having a vTPM attached to a VM provides the following benefits:
> 
> - enablement of trusted boot; this allow us to eventually extend the chain 
>   of trust from the hypervisor to the guests
> - enablement of attestation so that one can verify what software is 
>   running on a machine
> - provides TPM functionality to VMs, which includes a standardized 
>   mechanism to store keys and other blobs
>   (Linux trusted keys, GNU TLS's TPM extensions)
> 
> Necessarily, some of its parts are written in Forth, many are written
> in 'C'. The extensions are known to work with QEMU for ppc64 running Linux.

Ok, summary time: After scanning through the patch series, most of it
sounds basically sane to me, but there are a couple of things I really
don't like and where I'd like to ask you to rework it:

- The "struct tpm_driver" function pointers seem over-engineered, I
  think that could be made much easier by direct calls (unless you
  really plan multiple drivers in the near future)

- In the second half of the patch series, you add a lot of functions
  (or rather one function and two wrappers) three times: One time in
  vio-vtpm-cdriver.fs, one time in tpm-static.fs and one time in
  vtpm-sml.fs. Is it really necessary to have those functions in both
  device tree nodes, /vdevice/vtpm and /ibm,vtpm ? What does the spec
  say?
  Anyway, I think you should at least get rid of the functions
  in tpm-static.fs and implement the stuff only in /ibm,vtpm instead.

- Also for the other functions in tpm-static.fs, I think it would be
  nicer if you would move them to /ibm,vtpm instead to get rid of
  tpm-static.fs completely.

- Since your patch series is already really huge, please try to focus
  on the basics first and omit things like the menu in this initial
  series. Send these things later once the basics have been included.

 Thomas