[U-Boot] [RESEND PATCH v9 01/18] Revert "efi_loader: Rename sections to allow for implicit data"

[Simon Glass]

Hi Alex,

On 23 August 2018 at 06:21, Alexander Graf <agraf at suse.de> wrote:
>
> On 08/23/2018 12:45 PM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 21 August 2018 at 13:24, Alexander Graf <agraf at suse.de> wrote:
>>>
>>>
>>> On 21.08.18 19:31, Simon Glass wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> On 20 August 2018 at 16:29, Alexander Graf <agraf at suse.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 20.08.18 20:54, Simon Glass wrote:
>>>>>>
>>>>>> This partially reverts commit 7e21fbca26d18327cf7cabaad08df276a06a07d8.
>>>>>>
>>>>>> That change broke sandbox EFI support for unknown reasons. It also changes
>>>>>
>>>>> Wouldn't it be better to just figure out the reasons? So far all bugs
>>>>> I've found were linker script related and quite obvious once you start
>>>>> to dig into them.
>>>>>
>>>>>> sandbox to use--gc-sections which we don't want.
>>>>>
>>>>> Why don't we want gc-sections with sandbox?
>>>>
>>>> It is a space optimisation which we don't need for sandbox. It also
>>>> complicates the object files unnecessarily.
>>>>
>>>> Put another way, why is it desirable?
>>>
>>> The reason we need it for efi_loader is that runtime section code may
>>> generate implicit dependencies on "other data" such as strings, jump
>>> tables, etc that are generated on the fly by the compiler.
>>>
>>> The only way to ensure that all these implicit pieces of data are
>>> actually in the runtime section is by forcing function and data sections
>>> onto gcc, because then the implicit pieces of data will inherit the name
>>> tags of the function they're in.
>>
>> Do they actually not end up in the runtime section without
>> -fdata-sections, etc.? I would expect that this would just increase
>
>
> Yes, that's exactly what happens ;). Without data and function sections, implicit pieces of information go into the big bucket sections (.data/.rodata/.bss) even when the function they're used in is in an explicit section.
>
>> the size of the runtime section (potentially quite a lot). It seems
>> wrong for them to be omitted by the toolchain.
>>
>> Or are you saying that they get picked up by an earlier .lds line, and
>> so are not available for the (later) runtime section line?
>
>
> Exactly. Because they're in the generic sections, there is no way to determine that these variables really are runtime relevant at link time.
>
>
>>
>>>>>> For now I am just reverting the sandbox portion as presumably this change
>>>>>> is safe on other architectures.
>>>>>
>>>>> Sandbox is your target, so you're free to do whatever you like :). But
>>>>> I'm not sure this is the right path forward. I'd rather like to keep
>>>>> things consistent.
>>>>
>>>> In what sense?
>>>
>>> For efi_loader Runtime Services to actually work we *have* to have
>>> -fdata-sections and -ffunction-sections enabled, otherwise they just
>>> break. So by not enabling them with sandbox we don't get runtime service
>>> support after ExitBootServices (which is useless for sandbox anyway
>>> really), but most importantly we're different from all other targets.
>>
>> I'm happy to be different from other targets for now. As you say, we
>> cannot support run-time services for sandbox, at least without some
>> cunning plan I am not aware of.
>>
>>>>> So what do you expect happens with this patch? A resend of a patch 1/18
>>>>> by itself doesn't really tell me what you're trying to say.
>>>>
>>>> The resend was due to me noticing that people did not get the patch on
>>>> cc. I only sent this one patch, but I can resend send the whole series
>>>> if you like.
>>>
>>> I think that it makes sense to differentiate between "this should be
>>> applied for *this* release" and "this is a patch set that as a whole
>>> should be applied".
>>>
>>> Or in other words: If the sections really do break sandbox (and again,
>>> for me they don't - I am unable to reproduce still), I would expect that
>>> you send that patch individually as a resend :).
>>>
>>> As it stood right now, I simply didn't grasp what your intention was so
>>> I didn't include it in my pull request.
>>
>> Well I think the revert is needed for this release. For the rest of
>> the patches, perhaps they should be reviewed and applied when
>> convenient?
>>
>> You should be able to repeat the breakage by using my series without
>> the review (u-boot-dm/efi-working).
>
>
> Do you see any breakage with current master?

No the tests all pass. I still want a revert though.

Regards,
Simon