| Message ID | 1486490390-25251-1-git-send-email-jhugo@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > 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
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? > Change-Id: Ie7f3637e375bd6631c6bda1f7b3c9003765ff4a5 Please remove this kind of tags from upstream patches. It's irrelevant. > 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. Thanks, Mark.
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. >
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 >
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@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. 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.
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.
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@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. > > 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 7 February 2017 at 19:07, Jeffrey Hugo <jhugo@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@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. >> >> >> 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
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@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. > > > >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.
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.
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@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. >>> >>> 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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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.
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
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
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. 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.
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.
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
On Wed, Feb 8, 2017 at 4:40 AM, Ard Biesheuvel <ard.biesheuvel@linaro.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..
On Thu, Feb 9, 2017 at 2:27 AM, AKASHI, Takahiro <takahiro.akashi@linaro.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.
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;