Re: [PATCH v2 4/5] arm/efi: Simplify efi_arch_handle_cmdline()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi,

> On Nov 23, 2023, at 12:20, Henry Wang <Henry.Wang@xxxxxxx> wrote:
> 
> Hi,
> 
>> On Nov 23, 2023, at 02:03, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> 
>> On 22/11/2023 3:49 pm, Luca Fancellu wrote:
>>> 
>>>> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> 
>>>> On 21/11/2023 8:33 pm, Luca Fancellu wrote:
>>>>> + CC henry
>>>>> 
>>>>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
>>>>>> wrote:
>>>>>> 
>>>>>> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, 
>>>>>> but this
>>>>>> logic looks incorrect.  It was inherited from the x86 side, where the 
>>>>>> logic
>>>>>> was redundant and has now been removed.
>>>>>> 
>>>>>> In the ARM case it inserts the image name into "xen,xen-bootargs" and 
>>>>>> there is
>>>>>> no logic at all to strip this before parsing it as the command line.
>>>>>> 
>>>>>> The absence of any logic to strip an image name suggests that it 
>>>>>> shouldn't
>>>>>> exist there, or having a Xen image named e.g. "hmp-unsafe" in the 
>>>>>> filesystem
>>>>>> is going to lead to some unexpected behaviour on boot.
>>>>>> 
>>>>>> No functional change.
>>>>>> 
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>> ---
>>>>>> v2:
>>>>>> * New.
>>>>>> 
>>>>>> I'm afraid that all of this reasoning is based on reading the source 
>>>>>> code.  I
>>>>>> don't have any way to try this out in a real ARM UEFI environment.
>>>>> I will test this one tomorrow on an arm board
>>> I confirm that booting though UEFI on an arm board works
>>> 
>>> Reviewed-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>>> Tested-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> 
>> Thanks, and you confirmed that the first cmdline parameter is still usable?
> 
> Today I tried this series on an N1SDP board using UEFI boot. I had a device 
> tree with
> xen,xen-bootargs = "console=dtuart dtuart=serial0:115200n8 noreboot 
> dom0_mem=1024M   bootscrub=0 iommu=no";
> 
> Xen can be successfully boot on the board with the series applied, and I got
> ```
> (XEN) Command line: console=dtuart dtuart=serial0:115200n8 noreboot 
> dom0_mem=1024M bootscrub=0 iommu=no
> […]
> ```
> 
> Also I can interact with the board:
> ```
> n1sdp login: root
> root@n1sdp:~# ^C
> root@n1sdp:~# ^C
> root@n1sdp:~# ^C
> ```
> 
> So I think the first cmdline parameter is still usable. I will wait for Luca 
> to confirm on his
> side as I believe he used a different board in his test.

Also I tried to switch the order of the cmdline parameter in the device tree, 
for example use:
xen,xen-bootargs = “dom0_mem=512M console=dtuart dtuart=serial0:115200n8 
noreboot  bootscrub=0 iommu=no”;

This time I had:
```
[...]
(XEN) Command line: dom0_mem=512M console=dtuart dtuart=serial0:115200n8 
noreboot  bootscrub=0 iommu=no
[…]
(XEN) Allocating 1:1 mappings totalling 512MB for dom0:
[…]
```

So I think we are fine.

Kind regards,
Henry

> 
> Tested-by: Henry Wang <Henry.Wang@xxxxxxx>
> 
> Kind regards,
> Henry
> 
>> 
>> ~Andrew
>