[U-Boot] [PATCH 4/8] cmd: avb2.0: avb command for performing verification

[Igor Opaniuk]

Hi Simon,

> If there is no need for operations and indirection through function
> pointers, why not just call the functions directly?

This is the way how libavb is integrated into bootloader. libavb interfaces with
the bootloader through the supplied AvbOps struct. This includes operations to
read and write data from partitions, read and write rollback indexes, check
if the public key used to make a signature should be accepted, and so on.
This is what Google AVB 2.0 specification says.
For additional details please check [1].

In this particular patch, all avb commands, except the main "avb
verify", are used
to debug these particular callback implementations
(can be used when enabling this feature on new platform etc.).
The reason I use indirection through function pointers is that they
are not supposed
to be public, and are not intended to be used somehow directly, only by libavb
(check [2] how libavb is using it)

>> (I've left a notice in the cover letter that checkpatch
>> will fail on the commit, which introduces libavb; also, there is the ongoing
>> discussion there regarding why libavb/libavb_ab should be
>> kept as it's. Please join us, if you don't mind).
>
> I do see the cover letter thread but could not find the camel case
> discussion. Can you please point me to it?

There is no any particular discussion about CamelCase, but a few general
statements why I should avoid introducing any changes in
libavb/libavb_ab libraries.
Regarding our particular case, AvbSlotVerifyResult and AvbSlotVerifyData types
(I guess you're talking about these CamelCase instances) are defined inside
libavb library, so obviously I can't simply replace it with something, that
conforms to the Linux kernel coding style

Thanks

[1] https://android.googlesource.com/platform/external/avb/+/master/README.md#system-dependencies
[2] https://android.googlesource.com/platform/external/avb/+/master/libavb/avb_cmdline.c#71

On 15 May 2018 at 21:28, Simon Glass <sjg at chromium.org> wrote:
> (Tom can you please comment on the CamelCase question?)
>
> Hi Igor,
>
> On 15 May 2018 at 11:31, Igor Opaniuk <igor.opaniuk at linaro.org> wrote:
>> On 15 May 2018 at 19:26, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Igor,
>>>
>>> On 16 May 2018 at 01:44, Igor Opaniuk <igor.opaniuk at linaro.org> wrote:
>>>> Hi Simon,
>>>>
>>>> I've dug into DriverModel documentation and even created a PoC for
>>>> existing avb commands. The problem is that (maybe I missed out some
>>>> key concepts) I'm still
>>>> not sure if it makes sense to follow it driver mode in the context of
>>>> AVB 2.0 feature and
>>>> what kind of extra devices can be used within the same uclass in this case?
>>>> Could you please explain in detail.
>>>> Thanks
>>>
>>> avb_ops_alloc() is allocating a struct and then assigning operations
>>> to its members.
>>>
>>> This is what driver model is designed to do. It handles the
>>> allocations and the operations become part of the uclass interface.
>>>
>>> At present it looks like you only have one driver. I'm not sure if it
>>> would make sense to have a second one.
>>>
>>> As a counter example see cros_ec uclass, which does things
>>> differently. It models the EC interface (I2C, SPI, LPC) using DM, with
>>> just a single impl on top.
>>
>> Right, I do understand what DriverModel is and why it should
>> be used in the case of real-device drivers. But regarding Android Verified
>> Boot 2.0 feature, which introduces verification capabilities
>> and leverages only device-independent APIs,
>> I see no reason why it should be used here.
>> Could you please check [1] and confirm that this set of commands should
>> really follow this model.
>
> If there is no need for operations and indirection through function
> pointers, why not just call the functions directly?
>
> I do think (as the code is currently structured) that DM makes sense,
> even though I understand that you are not creating a device. Perhaps
> Tom might have thoughts on this though?
>
>>
>>> Also can you please drop the CamelCase in the patches? We don't use
>>> that in U-Boot.
>>
>> Frankly, I don't like it also, bit all CamelCase instances in
>> the code relate to libavb/libavb_ab library ([2]), which is planned to be
>> deviated from AOSP upstream in the future.
>> That's why a decision was made to not introduce any changes to simplify this
>> process, as Google intensively introduces new changes to it.
>
> Tom, what do you think? It's not how things usually work, but perhaps
> there is precedent for this?
>
>> (I've left a notice in the cover letter that checkpatch
>> will fail on the commit, which introduces libavb; also, there is the ongoing
>> discussion there regarding why libavb/libavb_ab should be
>> kept as it's. Please join us, if you don't mind).
>
> I do see the cover letter thread but could not find the camel case
> discussion. Can you please point me to it?
>
>>
>> Thanks!
>>
>> Best regards,
>> Igor
>>
>> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
>> [2] https://android.googlesource.com/platform/external/avb/
>>>>
>
> [..]
>
> Regards,
> Simon



-- 
Regards,
Igor Opaniuk