On 04.11.20 16:56, Ilias Apalodimas wrote:
> Hi Heinrich,
>
> [...]
>>> +++ b/lib/tpm-v2.c
>>> @@ -161,7 +161,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx,
>>> unsigned int idx_min_sz,
>>> }
>>>
>>> u32 tpm2_get_capability(struct udevice *dev, u32 capability, u32 property,
>>> - void *buf, size_t prop_count)
>>> + void *buf, size_t prop_count, bool get_count)
>>
>> The implementation would be more stable if we would derive the offset
>> from field property instead of adding get_count.
>>
>
> We are not defining the tpmv2 internal structures anywhere in U-Boot.
> That's why the code is doing static sizeof(uX) instead of using offsetof.
> In the EFI part of the patchset, I've done exaclty that.
> Working with offsets on well defined struct is better, but out of scope for
> this
> patchset imho.
> We can look into refactoring the generic tpmv2 code once I add the rest of
> the EFI
> protocol parts?
Can't we add the structures that we need to tpm-v2.h and use their size
here?
Best regards
Heinrich
>
>>> {
>>> u8 command_v2[COMMAND_BUFFER_SIZE] = {
>>
>> Shouldn't COMMAND_BUFFER_SIZE be changed to something with TPM in the
>> name, e.g TPM_COMMAND_BUFFER_SIZE?
>>
>>> tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */
>>> @@ -181,13 +181,17 @@ u32 tpm2_get_capability(struct udevice *dev, u32
>>> capability, u32 property,
>>> if (ret)
>>> return ret;
>>>
>>> + /* When reading PCR properties we need the count */
>>> + properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) +
>>> + sizeof(u8) + sizeof(u32);
>>> /*
>>> * In the response buffer, the properties are located after the:
>>> * tag (u16), response size (u32), response code (u32),
>>> * YES/NO flag (u8), TPM_CAP (u32) and TPMU_CAPABILITIES (u32).
>>> */
>>
>> This comment should be above 'properties_off ='. 'get_count' related
>> field should be mentioned.
>
> Sure, I'll fix this
>
> Regards
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>> - properties_off = sizeof(u16) + sizeof(u32) + sizeof(u32) +
>>> - sizeof(u8) + sizeof(u32) + sizeof(u32);
>>> + if (!get_count)
>>> + properties_off += sizeof(u32);
>>> +
>>> memcpy(buf, &response[properties_off], response_len - properties_off);
>>>
>>> return 0;
>>>
>>
