On 11/4/20 6:26 PM, Ilias Apalodimas wrote:
Hi Heinrich,
On Wed, Nov 04, 2020 at 05:50:16PM +0100, Heinrich Schuchardt wrote:
On 04.11.20 16:56, Ilias Apalodimas wrote:
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?
Yes we can, but the scope of the patch is adding EFI_TCG2_PROTOCOL, not re-factor the tpmv2 code along the way. Since the only change on the existing code to support the functionality needed for the EFI_TCG2_PROTOCOL protocol is just 4 bytes on an existing buffer, can we stick to that and think of refactoring the tpm stuff afterwards?
You expect a TPML_PCR_SELECTION when capabilities is TPM_CAP_PCRS.
typedef struct { UINT32 count; TPMS_PCR_SELECTION pcrSelections[TPM2_NUM_PCR_BANKS]; } TPML_PCR_SELECTION;
Why do you have to skip over the UINT32 here?
You just have to define this one structure in preparation of patch 2 and then you can correctly parse the response in your implementation of the EFI protocol.
I suggest not to change tpm2_get_capability() at all.
Best regards
Heinrich
Regards /Ilias
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;