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? 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; > >>> > >> >