[PATCH] arm/efi: Handle Xen bootargs from both xen.cfg and DT

[PATCH] arm/efi: Handle Xen bootargs from both xen.cfg and DT

[Luca Fancellu]

Currently the Xen UEFI stub can accept Xen boot arguments from
the Xen configuration file using the "options=" keyword, but also
directly from the device tree specifying xen,xen-bootargs
property.

When the configuration file is used, device tree boot arguments
are ignored and overwritten even if the keyword "options=" is
not used.

This patch handle this case, if xen,xen-bootargs is found in the
device tree, it is used for xen boot arguments regardless they
are specified in the Xen configuration file or not.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 docs/misc/efi.pandoc        | 4 ++++
 xen/arch/arm/efi/efi-boot.h | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index abafb3452758..b7d99de87f15 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -249,6 +249,10 @@ UEFI stub for module loading.
 When adding DomU modules to device tree, also add the property
 xen,uefi-cfg-load under chosen for Xen to load the Xen config file.
 Otherwise, Xen will skip the config file and rely on device tree alone.
+When using the Xen configuration file in conjunction with the device tree, you
+can specify the Xen boot arguments in the configuration file with the "options="
+keyword or in the device tree with the "xen,xen-bootargs" property, but be
+aware that a device tree value has a precedence over the configuration file.
 
 Example 1 of how to boot a true dom0less configuration:
 
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index c4ed41284597..fc1f2b9ad60e 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -497,6 +497,13 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name,
     if ( chosen < 0 )
         blexit(L"Unable to find chosen node");
 
+    /* If xen,bootargs is found in /chosen, use it for Xen */
+    if ( fdt_get_property(fdt, chosen, "xen,xen-bootargs", NULL) )
+    {
+        PrintStr(L"Using Xen boot arguments from device tree.\r\n");
+        return;
+    }
+
     status = efi_bs->AllocatePool(EfiBootServicesData, EFI_PAGE_SIZE, (void **)&buf);
     if ( EFI_ERROR(status) )
         PrintErrMesg(L"Unable to allocate string buffer", status);
-- 
2.17.1

Re: [PATCH] arm/efi: Handle Xen bootargs from both xen.cfg and DT

[Julien Grall]

Hi Luca,

On 06/12/2021 15:36, Luca Fancellu wrote:
> Currently the Xen UEFI stub can accept Xen boot arguments from
> the Xen configuration file using the "options=" keyword, but also
> directly from the device tree specifying xen,xen-bootargs
> property.
> 
> When the configuration file is used, device tree boot arguments
> are ignored and overwritten even if the keyword "options=" is
> not used.
> 
> This patch handle this case, if xen,xen-bootargs is found in the
> device tree, it is used for xen boot arguments regardless they
> are specified in the Xen configuration file or not.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
>   docs/misc/efi.pandoc        | 4 ++++
>   xen/arch/arm/efi/efi-boot.h | 7 +++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
> index abafb3452758..b7d99de87f15 100644
> --- a/docs/misc/efi.pandoc
> +++ b/docs/misc/efi.pandoc
> @@ -249,6 +249,10 @@ UEFI stub for module loading.
>   When adding DomU modules to device tree, also add the property
>   xen,uefi-cfg-load under chosen for Xen to load the Xen config file.
>   Otherwise, Xen will skip the config file and rely on device tree alone.
> +When using the Xen configuration file in conjunction with the device tree, you
> +can specify the Xen boot arguments in the configuration file with the "options="
> +keyword or in the device tree with the "xen,xen-bootargs" property, but be
> +aware that a device tree value has a precedence over the configuration file.

I am not sure I agree with the precedence chosen here. With UEFI 
environment it is a lot easier to update the configuration file over the 
Device-Tree. So this could be used to quickly test a command line before 
updating the Device-Tree.

Also, somewhat unrelated to this patch. Looking at the code, the command 
line is a concatenation of "options=" from the configuration file and 
the extra arguments provided on the command line used to execute the 
UEFI binary.

When using the command line from the Device-Tree, I think it would still 
make sense to append the later because it could allow a user to provide 
a single Device-Tree with extra options on the UEFI command line.

But I think this is a separate subject. So if we are going to go with 
the precedence you suggested, then we should at least clarify in the 
documentation that it will replace both.

Cheers,

-- 
Julien Grall

Re: [PATCH] arm/efi: Handle Xen bootargs from both xen.cfg and DT

[Luca Fancellu]

> On 8 Dec 2021, at 18:10, Julien Grall <julien@xen.org> wrote:
> 
> Hi Luca,
> 
> On 06/12/2021 15:36, Luca Fancellu wrote:
>> Currently the Xen UEFI stub can accept Xen boot arguments from
>> the Xen configuration file using the "options=" keyword, but also
>> directly from the device tree specifying xen,xen-bootargs
>> property.
>> When the configuration file is used, device tree boot arguments
>> are ignored and overwritten even if the keyword "options=" is
>> not used.
>> This patch handle this case, if xen,xen-bootargs is found in the
>> device tree, it is used for xen boot arguments regardless they
>> are specified in the Xen configuration file or not.
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>>  docs/misc/efi.pandoc        | 4 ++++
>>  xen/arch/arm/efi/efi-boot.h | 7 +++++++
>>  2 files changed, 11 insertions(+)
>> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
>> index abafb3452758..b7d99de87f15 100644
>> --- a/docs/misc/efi.pandoc
>> +++ b/docs/misc/efi.pandoc
>> @@ -249,6 +249,10 @@ UEFI stub for module loading.
>>  When adding DomU modules to device tree, also add the property
>>  xen,uefi-cfg-load under chosen for Xen to load the Xen config file.
>>  Otherwise, Xen will skip the config file and rely on device tree alone.
>> +When using the Xen configuration file in conjunction with the device tree, you
>> +can specify the Xen boot arguments in the configuration file with the "options="
>> +keyword or in the device tree with the "xen,xen-bootargs" property, but be
>> +aware that a device tree value has a precedence over the configuration file.
> 
> I am not sure I agree with the precedence chosen here. With UEFI environment it is a lot easier to update the configuration file over the Device-Tree. So this could be used to quickly test a command line before updating the Device-Tree.
> 
> Also, somewhat unrelated to this patch. Looking at the code, the command line is a concatenation of "options=" from the configuration file and the extra arguments provided on the command line used to execute the UEFI binary.
> 
> When using the command line from the Device-Tree, I think it would still make sense to append the later because it could allow a user to provide a single Device-Tree with extra options on the UEFI command line.
> 
> But I think this is a separate subject. So if we are going to go with the precedence you suggested, then we should at least clarify in the documentation that it will replace both.

Hi Julien,

Yes I see your point, currently the boot arguments are done in this way <image name> <CFG bootargs> <CMD line bootargs> as you pointed out,

would it be ok in your opinion to check if <CFG bootargs> is specified and if it’s not, use the <DT bootargs> instead (if any)?

Cheers,
Luca


> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] arm/efi: Handle Xen bootargs from both xen.cfg and DT

[Julien Grall]

Hi Luca,

On 10/12/2021 10:26, Luca Fancellu wrote:
> 
> 
>> On 8 Dec 2021, at 18:10, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Luca,
>>
>> On 06/12/2021 15:36, Luca Fancellu wrote:
>>> Currently the Xen UEFI stub can accept Xen boot arguments from
>>> the Xen configuration file using the "options=" keyword, but also
>>> directly from the device tree specifying xen,xen-bootargs
>>> property.
>>> When the configuration file is used, device tree boot arguments
>>> are ignored and overwritten even if the keyword "options=" is
>>> not used.
>>> This patch handle this case, if xen,xen-bootargs is found in the
>>> device tree, it is used for xen boot arguments regardless they
>>> are specified in the Xen configuration file or not.
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> ---
>>>   docs/misc/efi.pandoc        | 4 ++++
>>>   xen/arch/arm/efi/efi-boot.h | 7 +++++++
>>>   2 files changed, 11 insertions(+)
>>> diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
>>> index abafb3452758..b7d99de87f15 100644
>>> --- a/docs/misc/efi.pandoc
>>> +++ b/docs/misc/efi.pandoc
>>> @@ -249,6 +249,10 @@ UEFI stub for module loading.
>>>   When adding DomU modules to device tree, also add the property
>>>   xen,uefi-cfg-load under chosen for Xen to load the Xen config file.
>>>   Otherwise, Xen will skip the config file and rely on device tree alone.
>>> +When using the Xen configuration file in conjunction with the device tree, you
>>> +can specify the Xen boot arguments in the configuration file with the "options="
>>> +keyword or in the device tree with the "xen,xen-bootargs" property, but be
>>> +aware that a device tree value has a precedence over the configuration file.
>>
>> I am not sure I agree with the precedence chosen here. With UEFI environment it is a lot easier to update the configuration file over the Device-Tree. So this could be used to quickly test a command line before updating the Device-Tree.
>>
>> Also, somewhat unrelated to this patch. Looking at the code, the command line is a concatenation of "options=" from the configuration file and the extra arguments provided on the command line used to execute the UEFI binary.
>>
>> When using the command line from the Device-Tree, I think it would still make sense to append the later because it could allow a user to provide a single Device-Tree with extra options on the UEFI command line.
>>
>> But I think this is a separate subject. So if we are going to go with the precedence you suggested, then we should at least clarify in the documentation that it will replace both.
>
> Yes I see your point, currently the boot arguments are done in this way <image name> <CFG bootargs> <CMD line bootargs> as you pointed out,
> 
> would it be ok in your opinion to check if <CFG bootargs> is specified and if it’s not, use the <DT bootargs> instead (if any)?

I am happy with this approach.

Cheers,

-- 
Julien Grall