[PATCH] efi/libstub/arm*: Set default address and size cells values for an empty dtb

[Ard Biesheuvel]

On 7 February 2017 at 19:07, Jeffrey Hugo <jhugo at codeaurora.org> wrote:
> On 2/7/2017 12:01 PM, Mark Rutland wrote:
>>
>> On Tue, Feb 07, 2017 at 11:54:55AM -0700, Jeffrey Hugo wrote:
>>>
>>> On 2/7/2017 11:12 AM, Ard Biesheuvel wrote:
>>>>
>>>> On 7 February 2017 at 17:59, Jeffrey Hugo <jhugo at codeaurora.org> wrote:
>>>>>
>>>>> From: Sameer Goel <sgoel at codeaurora.org>
>>>>>
>>>>> In cases where a device tree is not provided (ie ACPI based system), an
>>>>> empty fdt is generated by efistub.  Sets the address and size cell
>>>>> values
>>>>> in a generated fdt to support 64 bit addressing.
>>>>>
>>>>> This enables kexec/kdump on Qualcomm Technologies QDF24XX platforms as
>>>>> those
>>>>> utilities will read the address/size values from the fdt, and such
>>>>> values
>>>>> may exceed the range provided by the 32 bit default.
>>>>>
>>>>
>>>> As far as I know, those properties are explicitly associated with the
>>>> 'reg' properties of subordinate nodes. So which nodes are we talking
>>>> about here? Are we producing an incorrect DT by not setting these? Or
>>>> is this simply a convenience to work around bugs in the tooling?
>>>
>>>
>>> I think we are producing an incorrect DT, in some instances.
>>>
>>> So we are starting from the same baseline, this is specific to ACPI
>>> systems, as an ACPI system won't have a DT from the bootloader.  DT
>>> based systems will already have a DT from the bootloader which is
>>> assumed to be correct.  On ACPI systems without a DT, efistub
>>> generates a default one.
>>>
>>> That default is assumed to be for a 32-bit system.  The cell width
>>> defaults to 1, which is 4 bytes.  You cannot represent a 64-bit
>>> value in that instance.
>>>
>>> What happens is that kexec inserts properties into the fdt which
>>> contain the start address and size on the crash kernel.  On our
>>> system, the start address is a 64-bit value, and while its not the
>>> case today, I see no reason why size could not also be a 64-bit
>>> value.  However the values that are inserted into the fdt are
>>> governed by the address and size cell values already present in the
>>> fdt.
>>>
>>> Kexec attempts to insert these values in the fdt.  The fdt only
>>> accepts 32-bit values, so it truncates what is put in.  Then later
>>> kexec/kdump read the values from the fdt, and get garbage.
>>
>>
>> I take it this is specific to the kdump properties?
>>
>> I can't immediately see what would matter for the !kdump case.
>> properties inserted under /chosen are not truncated?
>
>
> The kexec/kdump properties are added under /chosen, therefore yes,
> properties added under /chosen are truncated, per our observations.
>

You still haven't told use the name of those properties :-)

If they are not 'reg' properties, why do #address-cells & #size-cells
matter at all? Is it the dtc tool that does this internally?

>>
>>> By changing the defaults to 2 (the proposed change), 64-bit values
>>> can be inserted into the fdt, so the values we put in don't get
>>> truncated, and thus kexec/kdump read the correct thing when they
>>> need the values.
>>>
>>> I don't see how the tools could be fixed - fdt is truncating the
>>> values, and the generated fdt is already "static" at the point the
>>> tools run. We haven't had luck changing the cell size at the point
>>> the tools run. Additionally, this seems to be an issue for
>>> everything using the fdt - pushing the problem to every tool instead
>>> of fixing once at the top seems like playing a game of whack-a-mole.
>>>
>>> Does that clarify that issue for you?  Obviously the commit text
>>> needs some work, but I'd like to get on the same page first.
>>
>>
>> If you could update the commit message to explicitly mention the
>> properties being inserted for which this matters, this generally sounds
>> fine to me.
>>
>> Please Cc me on v2.
>>

Likewise