Re: [PATCH v2 4/4] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[Stefan Berger <stefanb@linux.ibm.com>]

On 7/30/21 8:44 AM, Daniel Kiper wrote:
> On Thu, Jul 29, 2021 at 09:30:49AM -0400, Stefan Berger wrote:
>> On 7/28/21 9:25 AM, Daniel Kiper wrote:
>>> On Tue, Jul 20, 2021 at 05:14:49PM -0400, Stefan Berger wrote:
>>>
>>>> +#define IEEE1275_CELL_TRUE     ((grub_ieee1275_cell_t) -1)
>>> This smells like global constant. Does not it? If yes could you define it
>>> in a global header and use it? Maybe even replace existing comparisons
>>> in the IEEE1275 code with IEEE1275_CELL_TRUE. But probably then
>>> s/IEEE1275_CELL_TRUE/GRUB_IEEE1275_CELL_TRUE/...
>> I wasn't sure and also had found local #defines in another .c file.
>>
>> #define IEEE1275_PHANDLE_INVALID  ((grub_ieee1275_cell_t) -1)
>> #define IEEE1275_IHANDLE_INVALID  ((grub_ieee1275_cell_t) 0)
>> #define IEEE1275_CELL_INVALID     ((grub_ieee1275_cell_t) -1)
>>
>> https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/kern/ieee1275/ieee1275.c#n24
>>
>> I haven't seen the usage of -1 as TRUE in other grub files. So I could move
>> it to a global header file assuming this is commonly used on this platform.
>> I have only seen usage of -1 as TRUE in the SLOF firmware in this file here:
>>
>> https://github.com/aik/SLOF/blob/master/lib/libnvram/libnvram.code#L66
>>
>> but then of course also here:
>> https://github.com/aik/SLOF/blob/master/lib/libtpm/tcgbios.c#L965
> I think we should use IEEE1275_CELL_INVALID in your code. IMO the TRUE
> sounds a bit confusing in this context. So, I would move all these

TRUE doesn't sound like CELL_INVALID and vice versa. FALSE is defined as 
0, so I think that's a better comparison (and as a value more 'common') 
after introducing GRUB_IEEE1275_CELL_FALSE.

> three constants to a global IEEE1275 header and add "GRUB_" prefix.
I'll move those three in a separate patch.
> Daniel


    stefan