Re: [PATCH v10 20/20] x86/efi: EFI stub DRTM launch support for Secure Launch

[ross.philipson@oracle.com]

On 8/29/24 6:28 AM, Ard Biesheuvel wrote:
> On Thu, 29 Aug 2024 at 15:24, Jonathan McDowell <noodles@earth.li> wrote:
>>
>> On Wed, Aug 28, 2024 at 01:19:16PM -0700, ross.philipson@oracle.com wrote:
>>> On 8/28/24 10:14 AM, Ard Biesheuvel wrote:
>>>> On Wed, 28 Aug 2024 at 19:09, kernel test robot <lkp@intel.com> wrote:
>>>>>
>>>>> Hi Ross,
>>>>>
>>>>> kernel test robot noticed the following build warnings:
>>>>>
>>>>> [auto build test WARNING on tip/x86/core]
>>>>> [also build test WARNING on char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus herbert-cryptodev-2.6/master efi/next linus/master v6.11-rc5]
>>>>> [cannot apply to herbert-crypto-2.6/master next-20240828]
>>>>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>>>>> And when submitting patch, we suggest to use '--base' as documented in
>>>>> https://urldefense.com/v3/__https://git-scm.com/docs/git-format-patch*_base_tree_information__;Iw!!ACWV5N9M2RV99hQ!KhkZK77BXRIR4F24tKkUeIlIrdqXtUW2vcnDV74c_5BmrQBQaQ4FqcDKKv9LB3HQUocTGkrmIxuz-LAC$ ]
>>>>>
>>>>> url:    https://urldefense.com/v3/__https://github.com/intel-lab-lkp/linux/commits/Ross-Philipson/Documentation-x86-Secure-Launch-kernel-documentation/20240827-065225__;!!ACWV5N9M2RV99hQ!KhkZK77BXRIR4F24tKkUeIlIrdqXtUW2vcnDV74c_5BmrQBQaQ4FqcDKKv9LB3HQUocTGkrmI7Z6SQKy$
>>>>> base:   tip/x86/core
>>>>> patch link:    https://urldefense.com/v3/__https://lore.kernel.org/r/20240826223835.3928819-21-ross.philipson*40oracle.com__;JQ!!ACWV5N9M2RV99hQ!KhkZK77BXRIR4F24tKkUeIlIrdqXtUW2vcnDV74c_5BmrQBQaQ4FqcDKKv9LB3HQUocTGkrmIzWfs1XZ$
>>>>> patch subject: [PATCH v10 20/20] x86/efi: EFI stub DRTM launch support for Secure Launch
>>>>> config: i386-randconfig-062-20240828 (https://urldefense.com/v3/__https://download.01.org/0day-ci/archive/20240829/202408290030.FEbUhHbr-lkp@intel.com/config__;!!ACWV5N9M2RV99hQ!KhkZK77BXRIR4F24tKkUeIlIrdqXtUW2vcnDV74c_5BmrQBQaQ4FqcDKKv9LB3HQUocTGkrmIwkYG0TY$ )
>>>>
>>>>
>>>> This is a i386 32-bit build, which makes no sense: this stuff should
>>>> just declare 'depends on 64BIT'
>>>
>>> Our config entry already has 'depends on X86_64' which in turn depends on
>>> 64BIT. I would think that would be enough. Do you think it needs an explicit
>>> 'depends on 64BIT' in our entry as well?
>>
>> The error is in x86-stub.c, which is pre-existing and compiled for 32
>> bit as well, so you need more than a "depends" here.
>>
> 
> Ugh, that means this is my fault - apologies. Replacing the #ifdef
> with IS_ENABLED() makes the code visible to the 32-bit compiler, even
> though the code is disregarded.
> 
> I'd still prefer IS_ENABLED(), but this would require the code in
> question to live in a separate compilation unit (which depends on
> CONFIG_SECURE_LAUNCH). If that is too fiddly, feel free to bring back
> the #ifdef CONFIG_SECURE_LAUNCH here (and retain my R-b)

Thanks to both of you for the followup.

If there was a very large amount of new code here, I would consider 
making a separate compilation unit. I can't really say if this is a 
wider issue with the the EFI libstub code but if it ware broken up 
later, it would make sense to move our bits to where 64bit only code lives.

Given that it is a small block of code though, I am inclined to just go 
back to #ifdef's for now.

Thanks
Ross