Hi Jason,
Thanks for your inputs.. Further responses inline.
On 08/13/2016 08:29 AM, Jason Gunthorpe wrote:
> On Thu, Aug 11, 2016 at 04:18:34PM +0530, Nayna wrote:
>>> #1. And the next somewhat pre-existing issue is that we call
>>> tpm_bios_log_setup even if we don't have access to a bios log.
>>>
>>> Does the bios log ever change or is it static at boot? Can we move
>>> read_log into the chip and do it once so we can tell if it worked
>>> before creating security fs stuff??
>>
>> <nayna>
>>
>> So, by statement "if we don't have access to a bios log?".. Do you mean bios
>> log is empty ? or has no properties to access the log.
>
> More along the lines we call down to tpm_of and it just exits if the
> properties do not exist. For instance my DT using TPM systems do not
> have a bios log.
Ok.
>
>> Regarding bios log, so it is supposed to be written by each component in
>> boot stack during boot.
>
> Only if the boot firmware supports this functionality and indicates it
> via DT attributes or ACPI. That is what we need to check earlier.
>
>> As per moving read_log into the chip, do you mean something like as below ?
>>
>> struct tpm_chip {
>> ....
>> struct tpm_bios_log *bios_log;
>> ....
>> }
>>
>> read_log(struct tpm_chip *chip)
>> {
>>
>> allocates to chip->bios_log->bios_event_log
>> allocates to chip->bios_log->bios_event_log_end;
>>
>> return 0; // for success;
>>
>> }
>>
>> tpm_chip_register()
>> {
>> ...
>> if (read_log(chip))
>> tpm_bios_log_setup(chip);
>> ...
>> }
>
> Yes, exactly. The challenge will be to get the chip into the security
> file context, give that a try and we can probably help you. The
> locking and lifetime will probably be very subtle.
Sure, I have done this change now in my next patch, should be able to
post soon.
>
>> And as per readlog is concerned, it just reads the start and max size of the
>> log for both ACPI/OF. So, it is possible that readlog returns successfully
>> giving start and max allocated size. but actual log size is zero. So, when
>> we cat bios_measurements_file, it doesn't show anything. And I think that
>> is fine.
>
> That would be a firmware that says it support a log but generated an
> empty one, so that seems reasonble to report to user space.
>
>>
>> #2.
>>> #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) ||
>> \
>>> - defined(CONFIG_ACPI)
>>> -extern struct dentry **tpm_bios_log_setup(const char *);
>>> -extern void tpm_bios_log_teardown(struct dentry **);
>>> + defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
>>> +extern struct dentry **tpm_bios_log_setup(struct tpm_chip *chip);
>>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>>
>> I'm really deeply unhappy about these ifdefs and the general scheme
>> that was used to force this special difference into the tpm core.
>>
>> Please find another way to do it.
>>
>> IMHO, 'read_log' should simply try ACPI and OF in sequence to find the
>> log.
>>
>> <nayna> Can you help me to understand the reason for which it was done this
>> way ?
>
> When this was written the TPM subsystem was very old and lacked
> infrastructure to do this better, we have largely solved those
> problems, event log is the last large area that needs some attention..
>
Ok.
>> Secondly, did you mean something like (just a sample psudocode)
>> read_log()
>> {
>> read_of_node;
>> if (read_of_node_fails)
>> read_acpi_way
>> if (read_acpi_succeeds)
>> do acpi_way of reading for start of log and max log size.
>> else
>> return err;
>> do of_node way of reading for start of log and max log size
>> }
>
> I would do more like:
>
> read_log_of(
> {
> if (! chip->pdev->of_node .. has attribute)
> return 0
> .. return 1;
> }
>
> read_log()
> {
> if ((res = read_log_of(chip)) != 0)
> return res;
> if ((res = read_log_acpi(chip)) != 0)
> return res;
> return 0;
> }
I tried the suggested approach and since ACPI specific functions won't
be available for arch using CONFIG_OF, so the compilation fails and vice
versa for CONFIG_ACPI..
Jason, for understanding.. can you explain the issue with existing
design for read_log, where arch specific read_log is compiled based on
CONFIG option.. And then, we can just change in Makefile.. where
currently it is like
ifdef CONFIG_ACPI
tpm-y += tpm_acpi.o
else
ifdef CONFIG_OF
tpm-y += tpm_of.o
endif
endif
this can be changed to
tpm-$(CONFIG_ACPI) += tpm_acpi.o
tpm-$(CONFIG_OF) += tpm_of.o
Please let me know if I missed something.
Thanks & Regards,
- Nayna
>
>> So, is it safe to assume that if there is no of_node property , that means
>> it is ACPI ?
>
> No, ACPI also reads a property from the ACPI table, the existance of
> that should be tested directly.
>
>> #3. Where you able to test this on IBM's 'vtpm' stuff as well?,
>>
>> <nayna>This was for patch where I am directly using dev.of_node property.
>> Sorry I missed to reply in previous response. And answer is "yes", I did
>> test it. </nayna>
>
> That is excellent
>
> Jason
>
------------------------------------------------------------------------------
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
