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

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

[Jeffrey Hugo]

From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5
Signed-off-by: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 921dfa0..def5c9c 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -16,6 +16,34 @@
 
 #include "efistub.h"
 
+#define EFI_DT_ADDR_CELLS_DEFAULT 2
+#define EFI_DT_SIZE_CELLS_DEFAULT 2
+
+static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt)
+{
+	int offset;
+	int status;
+
+	offset = fdt_path_offset(fdt, "/");
+	/* Set the #address-cells and #size-cells values for an empty tree */
+
+	status = fdt_setprop_u32(fdt, offset, "#address-cells",
+				 EFI_DT_ADDR_CELLS_DEFAULT);
+	if (status) {
+		pr_efi(sys_table,
+		       "Failed to set #address-cells for empty dtb\n");
+		return;
+	}
+
+	status = fdt_setprop_u32(fdt, offset, "#size-cells",
+				 EFI_DT_SIZE_CELLS_DEFAULT);
+	if (status) {
+		pr_efi(sys_table,
+		       "Failed to set #size-cells for empty dtb\n");
+		return;
+	}
+}
+
 static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 			       unsigned long orig_fdt_size,
 			       void *fdt, int new_fdt_size, char *cmdline_ptr,
@@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 
 	if (orig_fdt)
 		status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
-	else
+	else {
 		status = fdt_create_empty_tree(fdt, new_fdt_size);
+		if (status == 0) {
+			/*
+			 * Any failure from the following function is non
+			 * critical
+			 */
+			fdt_update_cell_size(sys_table, fdt);
+		}
+	}
 
 	if (status != 0)
 		goto fdt_set_fail;
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Ard Biesheuvel]

Hi Jeffrey,

(adding Mark)

On 7 February 2017 at 17:59, Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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?

> Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5

Please drop these IDs when you post

> Signed-off-by: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa0..def5c9c 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -16,6 +16,34 @@
>
>  #include "efistub.h"
>
> +#define EFI_DT_ADDR_CELLS_DEFAULT 2
> +#define EFI_DT_SIZE_CELLS_DEFAULT 2
> +
> +static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt)
> +{
> +       int offset;
> +       int status;
> +
> +       offset = fdt_path_offset(fdt, "/");
> +       /* Set the #address-cells and #size-cells values for an empty tree */
> +
> +       status = fdt_setprop_u32(fdt, offset, "#address-cells",
> +                                EFI_DT_ADDR_CELLS_DEFAULT);
> +       if (status) {
> +               pr_efi(sys_table,
> +                      "Failed to set #address-cells for empty dtb\n");
> +               return;
> +       }
> +
> +       status = fdt_setprop_u32(fdt, offset, "#size-cells",
> +                                EFI_DT_SIZE_CELLS_DEFAULT);
> +       if (status) {
> +               pr_efi(sys_table,
> +                      "Failed to set #size-cells for empty dtb\n");
> +               return;
> +       }
> +}
> +
>  static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>                                unsigned long orig_fdt_size,
>                                void *fdt, int new_fdt_size, char *cmdline_ptr,
> @@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>
>         if (orig_fdt)
>                 status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
> -       else
> +       else {
>                 status = fdt_create_empty_tree(fdt, new_fdt_size);
> +               if (status == 0) {
> +                       /*
> +                        * Any failure from the following function is non
> +                        * critical
> +                        */
> +                       fdt_update_cell_size(sys_table, fdt);
> +               }
> +       }
>
>         if (status != 0)
>                 goto fdt_set_fail;
> --
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeffrey Hugo]

On 2/7/2017 11:12 AM, Ard Biesheuvel wrote:
> Hi Jeffrey,
>
> (adding Mark)
>
> On 7 February 2017 at 17:59, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>> From: Sameer Goel <sgoel@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.

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.

>
>> Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5
>
> Please drop these IDs when you post
>
>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>  drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index 921dfa0..def5c9c 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -16,6 +16,34 @@
>>
>>  #include "efistub.h"
>>
>> +#define EFI_DT_ADDR_CELLS_DEFAULT 2
>> +#define EFI_DT_SIZE_CELLS_DEFAULT 2
>> +
>> +static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt)
>> +{
>> +       int offset;
>> +       int status;
>> +
>> +       offset = fdt_path_offset(fdt, "/");
>> +       /* Set the #address-cells and #size-cells values for an empty tree */
>> +
>> +       status = fdt_setprop_u32(fdt, offset, "#address-cells",
>> +                                EFI_DT_ADDR_CELLS_DEFAULT);
>> +       if (status) {
>> +               pr_efi(sys_table,
>> +                      "Failed to set #address-cells for empty dtb\n");
>> +               return;
>> +       }
>> +
>> +       status = fdt_setprop_u32(fdt, offset, "#size-cells",
>> +                                EFI_DT_SIZE_CELLS_DEFAULT);
>> +       if (status) {
>> +               pr_efi(sys_table,
>> +                      "Failed to set #size-cells for empty dtb\n");
>> +               return;
>> +       }
>> +}
>> +
>>  static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>                                unsigned long orig_fdt_size,
>>                                void *fdt, int new_fdt_size, char *cmdline_ptr,
>> @@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>
>>         if (orig_fdt)
>>                 status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
>> -       else
>> +       else {
>>                 status = fdt_create_empty_tree(fdt, new_fdt_size);
>> +               if (status == 0) {
>> +                       /*
>> +                        * Any failure from the following function is non
>> +                        * critical
>> +                        */
>> +                       fdt_update_cell_size(sys_table, fdt);
>> +               }
>> +       }
>>
>>         if (status != 0)
>>                 goto fdt_set_fail;
>> --
>> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Mark Rutland]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> >>From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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?

> 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.

Thanks,
Mark.

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

[Mark Rutland]

On Tue, Feb 07, 2017 at 07:01:14PM +0000, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 11:54:55AM -0700, Jeffrey Hugo wrote:

> > 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?

Whoops; please ignore the last line. I'd meant to delete that (and it
obviously makes no sense).

Mark.

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

[Jeffrey Hugo]

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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>> From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

>
>> 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.
>
> Thanks,
> Mark.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>>>
>>>>> From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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

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

[Mark Rutland]

On Tue, Feb 07, 2017 at 12:07:56PM -0700, Jeffrey Hugo 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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> >>>>From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

Sorry for the dodgy (and confusing) reply above.

What I was trying to ask was does this *only* affect kdump properties?
I note that kdump is not yet upstream for arm64.

Or are there regular kexec properties that this affects?

Or !kdump && !kexec properties?

Can you please enumerate the set of properties for which this matters?

Thanks,
Mark.

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

[Jeffrey Hugo]

On 2/7/2017 12:13 PM, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 12:07:56PM -0700, Jeffrey Hugo 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-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>>>> From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.
>
> Sorry for the dodgy (and confusing) reply above.
>
> What I was trying to ask was does this *only* affect kdump properties?
> I note that kdump is not yet upstream for arm64.
>
> Or are there regular kexec properties that this affects?
>
> Or !kdump && !kexec properties?
>
> Can you please enumerate the set of properties for which this matters?

"linux,elfcorehdr" and "linux,usable-memory-range"

We are not aware of !kdump && !kexec properties where this is an issue.

>
> Thanks,
> Mark.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Mark Rutland]

On Tue, Feb 07, 2017 at 12:29:38PM -0700, Jeffrey Hugo wrote:
> On 2/7/2017 12:13 PM, Mark Rutland wrote:
> >On Tue, Feb 07, 2017 at 12:07:56PM -0700, Jeffrey Hugo wrote:
> >>On 2/7/2017 12:01 PM, Mark Rutland wrote:
> >>>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.
> >
> >Sorry for the dodgy (and confusing) reply above.
> >
> >What I was trying to ask was does this *only* affect kdump properties?
> >I note that kdump is not yet upstream for arm64.
> >
> >Or are there regular kexec properties that this affects?
> >
> >Or !kdump && !kexec properties?
> >
> >Can you please enumerate the set of properties for which this matters?
> 
> "linux,elfcorehdr" and "linux,usable-memory-range"
> 
> We are not aware of !kdump && !kexec properties where this is an issue.

Ok, I understand the problem now. Thanks for clarifying the !kdump
situation.

As per my reply to Timur, given this only affects kdump, this is all
happening in code that is not upstream, and therefore this is not
*currently* an issue.

In future, please report this kind of issue in reply to relevant
postings (e.g. [1]). At the very least, refer to these, with relevant
people Cc'd (e.g. Takahiro-san in this case).

I think there are three things which should happen:

(a) The userspace kexec-tools kdump code should take /#address-cells and
    /#size-cells into account when inserting the linux,elfcorehdr and
    linux,usable-memory-range properties. There can be DTs for 64 bit
    platforms where these are not 2.
    
    Takahiro-san, from looking at your kexec-tools repo, this is not
    currently the case. Could you address that?

(b) The kdump documentation should updated to explicitly state that
    these properties are #address-cells + #size-cells tuples, since this
    is clearly going to be confusing either way. I'll try to come up
    with wording for that, and I'll reply on the current [1] kdump
    thread.

(c) Regardless, when creating the empty DT the EFI stub should insert
    /#address-cells = <2> and /#size-cells = <2>. That better aligns
    with the usual requirements for an otherwise empty DTB, and avoids a
    tonne of fragility when the DTB is passed on or altered by other
    agents.

So, please respin this patch, stating explicitly what this matters for.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/486234.html 

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

[AKASHI, Takahiro]

Hi Mark,

Thank you for this heads-up.

On Tue, Feb 07, 2017 at 07:55:59PM +0000, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 12:29:38PM -0700, Jeffrey Hugo wrote:
> > On 2/7/2017 12:13 PM, Mark Rutland wrote:
> > >On Tue, Feb 07, 2017 at 12:07:56PM -0700, Jeffrey Hugo wrote:
> > >>On 2/7/2017 12:01 PM, Mark Rutland wrote:
> > >>>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.
> > >
> > >Sorry for the dodgy (and confusing) reply above.
> > >
> > >What I was trying to ask was does this *only* affect kdump properties?
> > >I note that kdump is not yet upstream for arm64.
> > >
> > >Or are there regular kexec properties that this affects?
> > >
> > >Or !kdump && !kexec properties?
> > >
> > >Can you please enumerate the set of properties for which this matters?
> > 
> > "linux,elfcorehdr" and "linux,usable-memory-range"
> > 
> > We are not aware of !kdump && !kexec properties where this is an issue.
> 
> Ok, I understand the problem now. Thanks for clarifying the !kdump
> situation.
> 
> As per my reply to Timur, given this only affects kdump, this is all
> happening in code that is not upstream, and therefore this is not
> *currently* an issue.
> 
> In future, please report this kind of issue in reply to relevant
> postings (e.g. [1]). At the very least, refer to these, with relevant
> people Cc'd (e.g. Takahiro-san in this case).
> 
> I think there are three things which should happen:
> 
> (a) The userspace kexec-tools kdump code should take /#address-cells and
>     /#size-cells into account when inserting the linux,elfcorehdr and
>     linux,usable-memory-range properties. There can be DTs for 64 bit
>     platforms where these are not 2.
>     
>     Takahiro-san, from looking at your kexec-tools repo, this is not
>     currently the case. Could you address that?

Yup, I will, but if this is the case,
we might need to think about another case where /#address-cells and /#size-cells
are <1> but the range of crash dump kernel can be still 64-bit wide since
the system memory information comes from ACPI table (not DT).

Therefore, the properties in this case should look like:
/ {
    #address-cells = <1>;
    #size-cells = <1>;
    chosen {
         ...
         linux,usable-memory-range {
             #address-cells = <2>;
             #size-cells = <2>; may be omitted if possible
             reg = < ... >;
         }
         linux,elfcorehdr {
             #address-cells = <2>;
             #size-cells = <2>; may be omitted if possible
             reg = < ... >;
         }
         ...
    }
}

Is this what you meant?
(Obviously, I will have to modify the kernel patches as well.)

Thanks,
-Takahiro AKASHI


> (b) The kdump documentation should updated to explicitly state that
>     these properties are #address-cells + #size-cells tuples, since this
>     is clearly going to be confusing either way. I'll try to come up
>     with wording for that, and I'll reply on the current [1] kdump
>     thread.
> 
> (c) Regardless, when creating the empty DT the EFI stub should insert
>     /#address-cells = <2> and /#size-cells = <2>. That better aligns
>     with the usual requirements for an otherwise empty DTB, and avoids a
>     tonne of fragility when the DTB is passed on or altered by other
>     agents.
> 
> So, please respin this patch, stating explicitly what this matters for.
> 
> Thanks,
> Mark.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/486234.html 

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

[Ard Biesheuvel]

On 8 February 2017 at 07:43, AKASHI, Takahiro
<takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> Hi Mark,
>
> Thank you for this heads-up.
>
> On Tue, Feb 07, 2017 at 07:55:59PM +0000, Mark Rutland wrote:
>> On Tue, Feb 07, 2017 at 12:29:38PM -0700, Jeffrey Hugo wrote:
>> > On 2/7/2017 12:13 PM, Mark Rutland wrote:
>> > >On Tue, Feb 07, 2017 at 12:07:56PM -0700, Jeffrey Hugo wrote:
>> > >>On 2/7/2017 12:01 PM, Mark Rutland wrote:
>> > >>>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.
>> > >
>> > >Sorry for the dodgy (and confusing) reply above.
>> > >
>> > >What I was trying to ask was does this *only* affect kdump properties?
>> > >I note that kdump is not yet upstream for arm64.
>> > >
>> > >Or are there regular kexec properties that this affects?
>> > >
>> > >Or !kdump && !kexec properties?
>> > >
>> > >Can you please enumerate the set of properties for which this matters?
>> >
>> > "linux,elfcorehdr" and "linux,usable-memory-range"
>> >
>> > We are not aware of !kdump && !kexec properties where this is an issue.
>>
>> Ok, I understand the problem now. Thanks for clarifying the !kdump
>> situation.
>>
>> As per my reply to Timur, given this only affects kdump, this is all
>> happening in code that is not upstream, and therefore this is not
>> *currently* an issue.
>>
>> In future, please report this kind of issue in reply to relevant
>> postings (e.g. [1]). At the very least, refer to these, with relevant
>> people Cc'd (e.g. Takahiro-san in this case).
>>
>> I think there are three things which should happen:
>>
>> (a) The userspace kexec-tools kdump code should take /#address-cells and
>>     /#size-cells into account when inserting the linux,elfcorehdr and
>>     linux,usable-memory-range properties. There can be DTs for 64 bit
>>     platforms where these are not 2.
>>
>>     Takahiro-san, from looking at your kexec-tools repo, this is not
>>     currently the case. Could you address that?
>
> Yup, I will, but if this is the case,
> we might need to think about another case where /#address-cells and /#size-cells
> are <1> but the range of crash dump kernel can be still 64-bit wide since
> the system memory information comes from ACPI table (not DT).
>
> Therefore, the properties in this case should look like:
> / {
>     #address-cells = <1>;
>     #size-cells = <1>;
>     chosen {
>          ...
>          linux,usable-memory-range {
>              #address-cells = <2>;
>              #size-cells = <2>; may be omitted if possible
>              reg = < ... >;
>          }
>          linux,elfcorehdr {
>              #address-cells = <2>;
>              #size-cells = <2>; may be omitted if possible
>              reg = < ... >;
>          }
>          ...
>     }
> }
>
> Is this what you meant?
> (Obviously, I will have to modify the kernel patches as well.)
>

I think the cell sizes of reg properties are defined in terms of the
#address/size-cells attributes of the parent node. This makes sense
when you think of it, because the regs of all nodes under the same
parent should have the same size, given that they all live in their
parent's address space. Of course, /chosen is special since it does
not describe a bus.

So that means the tooling /could/ inject #address-cells/size-cells
properties either at the root, or in /chosen, but not in the kdump
properties themselves. However, that is not the point.

The point is that the tooling should take *existing* cell counts into
account: if there are no address-cells/size-cells properties under
/chosen or under the root node, the tooling should use 32-bit
quantities for address and size. If there are such properties, it
should emit reg tuples of the same size as the cells properties
describe.

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

[AKASHI, Takahiro]

On Wed, Feb 08, 2017 at 10:40:53AM +0000, Ard Biesheuvel wrote:
> On 8 February 2017 at 07:43, AKASHI, Takahiro
> <takahiro.akashi@linaro.org> wrote:
> > Hi Mark,
> >
> > Thank you for this heads-up.
> >
> > On Tue, Feb 07, 2017 at 07:55:59PM +0000, Mark Rutland wrote:
> >> On Tue, Feb 07, 2017 at 12:29:38PM -0700, Jeffrey Hugo wrote:
> >> > On 2/7/2017 12:13 PM, Mark Rutland wrote:
> >> > >On Tue, Feb 07, 2017 at 12:07:56PM -0700, Jeffrey Hugo wrote:
> >> > >>On 2/7/2017 12:01 PM, Mark Rutland wrote:
> >> > >>>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.
> >> > >
> >> > >Sorry for the dodgy (and confusing) reply above.
> >> > >
> >> > >What I was trying to ask was does this *only* affect kdump properties?
> >> > >I note that kdump is not yet upstream for arm64.
> >> > >
> >> > >Or are there regular kexec properties that this affects?
> >> > >
> >> > >Or !kdump && !kexec properties?
> >> > >
> >> > >Can you please enumerate the set of properties for which this matters?
> >> >
> >> > "linux,elfcorehdr" and "linux,usable-memory-range"
> >> >
> >> > We are not aware of !kdump && !kexec properties where this is an issue.
> >>
> >> Ok, I understand the problem now. Thanks for clarifying the !kdump
> >> situation.
> >>
> >> As per my reply to Timur, given this only affects kdump, this is all
> >> happening in code that is not upstream, and therefore this is not
> >> *currently* an issue.
> >>
> >> In future, please report this kind of issue in reply to relevant
> >> postings (e.g. [1]). At the very least, refer to these, with relevant
> >> people Cc'd (e.g. Takahiro-san in this case).
> >>
> >> I think there are three things which should happen:
> >>
> >> (a) The userspace kexec-tools kdump code should take /#address-cells and
> >>     /#size-cells into account when inserting the linux,elfcorehdr and
> >>     linux,usable-memory-range properties. There can be DTs for 64 bit
> >>     platforms where these are not 2.
> >>
> >>     Takahiro-san, from looking at your kexec-tools repo, this is not
> >>     currently the case. Could you address that?
> >
> > Yup, I will, but if this is the case,
> > we might need to think about another case where /#address-cells and /#size-cells
> > are <1> but the range of crash dump kernel can be still 64-bit wide since
> > the system memory information comes from ACPI table (not DT).
> >
> > Therefore, the properties in this case should look like:
> > / {
> >     #address-cells = <1>;
> >     #size-cells = <1>;
> >     chosen {
> >          ...
> >          linux,usable-memory-range {
> >              #address-cells = <2>;
> >              #size-cells = <2>; may be omitted if possible
> >              reg = < ... >;
> >          }
> >          linux,elfcorehdr {
> >              #address-cells = <2>;
> >              #size-cells = <2>; may be omitted if possible
> >              reg = < ... >;
> >          }
> >          ...
> >     }
> > }
> >
> > Is this what you meant?
> > (Obviously, I will have to modify the kernel patches as well.)
> >
> 
> I think the cell sizes of reg properties are defined in terms of the
> #address/size-cells attributes of the parent node. This makes sense
> when you think of it, because the regs of all nodes under the same
> parent should have the same size, given that they all live in their
> parent's address space. Of course, /chosen is special since it does
> not describe a bus.

I see.

> So that means the tooling /could/ inject #address-cells/size-cells
> properties either at the root, or in /chosen, but not in the kdump

/chosen, too?
Now I believe that /chosen should not have #address-cells/size-cells
because there is no assumption that all the nodes under /chosen
have the same values for #address-cells/#size-cells.
(Anyhow, this is very exceptional given the meanings of /chosen.)

> properties themselves. However, that is not the point.
> 
> The point is that the tooling should take *existing* cell counts into
> account: if there are no address-cells/size-cells properties under
> /chosen or under the root node, the tooling should use 32-bit
> quantities for address and size. If there are such properties, it
> should emit reg tuples of the same size as the cells properties
> describe.

Let me make sure one thing:
is there a requirement that the root node must have #address-cells/
#size-cells?
I found such a statement, at least, in booting-without-of.txt
(only applied to ppc? See III-5-a).

Thanks,
-Takahiro AKASHI

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

[Timur Tabi]

On Thu, Feb 9, 2017 at 2:27 AM, AKASHI, Takahiro
<takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> is there a requirement that the root node must have #address-cells/
> #size-cells?
> I found such a statement, at least, in booting-without-of.txt
> (only applied to ppc? See III-5-a).

If you want to be able to represent 64-bit 'reg' addresses anywhere in
the device tree, then you will need #address-cells = <2> and
#size-cells = <2> at any appropriate location, which is usually the
root node for a 64-bit processor.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Timur Tabi]

On Wed, Feb 8, 2017 at 4:40 AM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> The point is that the tooling should take *existing* cell counts into
> account: if there are no address-cells/size-cells properties under
> /chosen or under the root node, the tooling should use 32-bit
> quantities for address and size. If there are such properties, it
> should emit reg tuples of the same size as the cells properties
> describe.

I agree with this 100%, but that means that we need to be able to
inject address-cells/size-cells properties at some point, otherwise
we'll never be able to store 64-bit addresses.  And that's what
Sameer's patch does.  It injects those properties at the appropriate
time.  Any code that parses 'reg' properties without taking the
address-cells/size-cells properties into account is technically
incorrect and should be fixed..

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Mark Rutland]

On Wed, Feb 08, 2017 at 04:43:04PM +0900, AKASHI, Takahiro wrote:
> On Tue, Feb 07, 2017 at 07:55:59PM +0000, Mark Rutland wrote:

> > (a) The userspace kexec-tools kdump code should take /#address-cells and
> >     /#size-cells into account when inserting the linux,elfcorehdr and
> >     linux,usable-memory-range properties. There can be DTs for 64 bit
> >     platforms where these are not 2.
> >     
> >     Takahiro-san, from looking at your kexec-tools repo, this is not
> >     currently the case. Could you address that?
> 
> Yup, I will, but if this is the case,
> we might need to think about another case where /#address-cells and /#size-cells
> are <1> but the range of crash dump kernel can be still 64-bit wide since
> the system memory information comes from ACPI table (not DT).

That should only happen on an ACPI system for which the vendor has also
provided a DT in the FW, and also chose to use /#address-cells = <1>
and/or /#size-cells = <1>. We could log a warning and give up in this
case.

In the Purely DT case it shouldn't be a problem, since that would imply
all memory is within 32 bits.

... the other option Ard suggested was that we make these properties
always take 64-bit address and size values, and don't use #address-cells
or #size-cells at all when parsing them. I wasn't keen on this since the
properties would be reg-like, but potentially different in size. Maybe
that is the better option, though. :/

> Therefore, the properties in this case should look like:
> / {
>     #address-cells = <1>;
>     #size-cells = <1>;
>     chosen {
>          ...
>          linux,usable-memory-range {
>              #address-cells = <2>;
>              #size-cells = <2>; may be omitted if possible
>              reg = < ... >;
>          }
>          linux,elfcorehdr {
>              #address-cells = <2>;
>              #size-cells = <2>; may be omitted if possible
>              reg = < ... >;
>          }
>          ...
>     }
> }
> 
> Is this what you meant?
> (Obviously, I will have to modify the kernel patches as well.)

No; I was assuming we'd only change the userspace code, not the binding
or the kernel parsing.

If nothing else, the root /#address-cells and /#size-cells would have to
be widened to handle the translation, so the above alone is not
sufficient.

Thanks,
Mark.

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

[Mark Rutland]

Hi,

On Tue, Feb 07, 2017 at 10:59:50AM -0700, Jeffrey Hugo wrote:
> From: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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.

The description here doesn't state why this is a problem for ACPI.

What values are being read by the tools, and for what purpose?

Are they extracting data from the DTB, or is this part of inserting a
new property?

Why does this adversely affect ACPI?

> Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5

Please remove this kind of tags from upstream patches. It's irrelevant.

> Signed-off-by: Sameer Goel <sgoel-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
>  drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 921dfa0..def5c9c 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -16,6 +16,34 @@
>  
>  #include "efistub.h"
>  
> +#define EFI_DT_ADDR_CELLS_DEFAULT 2
> +#define EFI_DT_SIZE_CELLS_DEFAULT 2
> +
> +static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt)
> +{
> +	int offset;
> +	int status;
> +
> +	offset = fdt_path_offset(fdt, "/");
> +	/* Set the #address-cells and #size-cells values for an empty tree */
> +
> +	status = fdt_setprop_u32(fdt, offset, "#address-cells",
> +				 EFI_DT_ADDR_CELLS_DEFAULT);
> +	if (status) {
> +		pr_efi(sys_table,
> +		       "Failed to set #address-cells for empty dtb\n");
> +		return;
> +	}
> +
> +	status = fdt_setprop_u32(fdt, offset, "#size-cells",
> +				 EFI_DT_SIZE_CELLS_DEFAULT);
> +	if (status) {
> +		pr_efi(sys_table,
> +		       "Failed to set #size-cells for empty dtb\n");
> +		return;
> +	}
> +}

We don't seem to log anything for most other failures within
update_fdt() where this is called.

Are these really much more special?

>  static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  			       unsigned long orig_fdt_size,
>  			       void *fdt, int new_fdt_size, char *cmdline_ptr,
> @@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>  
>  	if (orig_fdt)
>  		status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
> -	else
> +	else {

Nit: if one side of an if-else has braces, the other should too per the
usual kernel coding style.

Thanks,
Mark.

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

[Jeffrey Hugo]

On 2/7/2017 11:15 AM, Mark Rutland wrote:
> Hi,
>
> On Tue, Feb 07, 2017 at 10:59:50AM -0700, Jeffrey Hugo wrote:
>> From: Sameer Goel <sgoel@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.
>
> The description here doesn't state why this is a problem for ACPI.
>
> What values are being read by the tools, and for what purpose?
>
> Are they extracting data from the DTB, or is this part of inserting a
> new property?
>
> Why does this adversely affect ACPI?

I think the response I am about to send to Ard will address these questions.

>
>> Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5
>
> Please remove this kind of tags from upstream patches. It's irrelevant.

Yep, sorry about that.  I should know better.

>
>> Signed-off-by: Sameer Goel <sgoel@codeaurora.org>
>> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
>> ---
>>  drivers/firmware/efi/libstub/fdt.c | 38 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
>> index 921dfa0..def5c9c 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -16,6 +16,34 @@
>>
>>  #include "efistub.h"
>>
>> +#define EFI_DT_ADDR_CELLS_DEFAULT 2
>> +#define EFI_DT_SIZE_CELLS_DEFAULT 2
>> +
>> +static void fdt_update_cell_size(efi_system_table_t *sys_table, void *fdt)
>> +{
>> +	int offset;
>> +	int status;
>> +
>> +	offset = fdt_path_offset(fdt, "/");
>> +	/* Set the #address-cells and #size-cells values for an empty tree */
>> +
>> +	status = fdt_setprop_u32(fdt, offset, "#address-cells",
>> +				 EFI_DT_ADDR_CELLS_DEFAULT);
>> +	if (status) {
>> +		pr_efi(sys_table,
>> +		       "Failed to set #address-cells for empty dtb\n");
>> +		return;
>> +	}
>> +
>> +	status = fdt_setprop_u32(fdt, offset, "#size-cells",
>> +				 EFI_DT_SIZE_CELLS_DEFAULT);
>> +	if (status) {
>> +		pr_efi(sys_table,
>> +		       "Failed to set #size-cells for empty dtb\n");
>> +		return;
>> +	}
>> +}
>
> We don't seem to log anything for most other failures within
> update_fdt() where this is called.
>
> Are these really much more special?
>
>>  static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>  			       unsigned long orig_fdt_size,
>>  			       void *fdt, int new_fdt_size, char *cmdline_ptr,
>> @@ -44,8 +72,16 @@ static efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
>>
>>  	if (orig_fdt)
>>  		status = fdt_open_into(orig_fdt, fdt, new_fdt_size);
>> -	else
>> +	else {
>
> Nit: if one side of an if-else has braces, the other should too per the
> usual kernel coding style.

Will fix.

>
> Thanks,
> Mark.
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

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

[Timur Tabi]

On Tue, Feb 7, 2017 at 12:15 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>
>> 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.
>
> The description here doesn't state why this is a problem for ACPI.

The patch description could use some work.  It's a problem for ACPI
because EFI-based systems call typically fdt_create_empty_tree(),
which is where the problem lies.

The bug is that fdt_create_empty_tree() literally creates an empty
tree.  By default if a node is missing #address-cells and #size-cells
properties, then it's assume that both values are equal to 1, i.e.
32-bit addresses.

When update_fdt() in drivers/firmware/efi/libstub/fdt.c creates an
empty tree, it then proceeds to inject 64-bit addresses into that
tree.  When kdump tries to process the address properties, it reads
the wrong values because it thinks they are all 32-bit addresses.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Mark Rutland]

On Tue, Feb 07, 2017 at 01:24:53PM -0600, Timur Tabi wrote:
> On Tue, Feb 7, 2017 at 12:15 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> >> 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.
> >
> > The description here doesn't state why this is a problem for ACPI.
> 
> The patch description could use some work.  It's a problem for ACPI
> because EFI-based systems call typically fdt_create_empty_tree(),
> which is where the problem lies.
> 
> The bug is that fdt_create_empty_tree() literally creates an empty
> tree.  By default if a node is missing #address-cells and #size-cells
> properties, then it's assume that both values are equal to 1, i.e.
> 32-bit addresses.

Sure, I understand this.

> When update_fdt() in drivers/firmware/efi/libstub/fdt.c creates an
> empty tree, it then proceeds to inject 64-bit addresses into that
> tree.  When kdump tries to process the address properties, it reads
> the wrong values because it thinks they are all 32-bit addresses.

This is *not* true.

The EFI stub only injects values which are always defined to be 64 bits
in width.

In Takahiro-san's arm64/kdump branch, the userspace kdump code doesn't
parse properties out of the DT.

In fact, it simply assumes that the kdump-specific properties always
have 64 bits of address, and 64-bits of size, and inserts these
sized accordingly.

The kdump kernel, however, tries to use /#address-cells and
/#size-cells. That is where I assume things go wrong.

There is no upstream kdump code for arm64.

Thanks,
Mark.