| Message ID | 20231121201540.1528161-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Enable -Wwrite-strings | expand |
+ CC henry > On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Roberto Bagnara <roberto.bagnara@bugseng.com> > CC: Nicola Vetrini <nicola.vetrini@bugseng.com> > > 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
On 21/11/2023 8:33 pm, Luca Fancellu wrote: > + CC henry > >> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Wei Liu <wl@xen.org> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien@xen.org> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> CC: Bertrand Marquis <bertrand.marquis@arm.com> >> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >> >> 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 Thanks. I have a sneaking suspicion that: diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 9b9018574919..8bca5b9a1523 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -46,6 +46,12 @@ #include <xsm/xsm.h> #include <asm/acpi.h> +static int __init parse_ucode(const char *s) +{ + panic("Xen image name interpreted as a cmdline parameter\n"); +} +custom_param("xen.efi", parse_xen); + struct bootinfo __initdata bootinfo; /* will trigger. ~Andrew
Hi Both, > On Nov 22, 2023, at 04:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 21/11/2023 8:33 pm, Luca Fancellu wrote: >> + CC henry >> >>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Wei Liu <wl@xen.org> >>> CC: Stefano Stabellini <sstabellini@kernel.org> >>> CC: Julien Grall <julien@xen.org> >>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> >>> 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 > > Thanks. I have a sneaking suspicion that: > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 9b9018574919..8bca5b9a1523 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -46,6 +46,12 @@ > #include <xsm/xsm.h> > #include <asm/acpi.h> > > +static int __init parse_ucode(const char *s) > +{ > + panic("Xen image name interpreted as a cmdline parameter\n"); > +} > +custom_param("xen.efi", parse_xen); > + > struct bootinfo __initdata bootinfo; > > /* > > will trigger. I saw I am CCed for this patch, so I think I am now going to throw this series to our CI and see if it explodes. Do you want me to also include above hunk? Kind regards, Henry > > ~Andrew
On 22/11/2023 1:31 am, Henry Wang wrote: > Hi Both, > >> On Nov 22, 2023, at 04:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>> + CC henry >>> >>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>> CC: Wei Liu <wl@xen.org> >>>> CC: Stefano Stabellini <sstabellini@kernel.org> >>>> CC: Julien Grall <julien@xen.org> >>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> >>>> 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 >> Thanks. I have a sneaking suspicion that: >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 9b9018574919..8bca5b9a1523 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -46,6 +46,12 @@ >> #include <xsm/xsm.h> >> #include <asm/acpi.h> >> >> +static int __init parse_ucode(const char *s) >> +{ >> + panic("Xen image name interpreted as a cmdline parameter\n"); >> +} >> +custom_param("xen.efi", parse_xen); >> + >> struct bootinfo __initdata bootinfo; >> >> /* >> >> will trigger. > I saw I am CCed for this patch, so I think I am now going to throw this series > to our CI and see if it explodes. Do you want me to also include above hunk? Depends - up to you. It needs tailoring for the name of the Xen binary used, if it doesn't end up as a plain "xen.efi". And to be more specific, it ought to trigger before this patch but cease triggering after it. It's probably better suited to a manual dev test in a real ARM UEFI environment. ~Andrew
> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 21/11/2023 8:33 pm, Luca Fancellu wrote: >> + CC henry >> >>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Wei Liu <wl@xen.org> >>> CC: Stefano Stabellini <sstabellini@kernel.org> >>> CC: Julien Grall <julien@xen.org> >>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> >>> 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@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
On 22/11/2023 3:49 pm, Luca Fancellu wrote: > >> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>> + CC henry >>> >>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>> CC: Wei Liu <wl@xen.org> >>>> CC: Stefano Stabellini <sstabellini@kernel.org> >>>> CC: Julien Grall <julien@xen.org> >>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> >>>> 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@arm.com> > Tested-by: Luca Fancellu <luca.fancellu@arm.com> Thanks, and you confirmed that the first cmdline parameter is still usable? ~Andrew
On Wed, 22 Nov 2023, Andrew Cooper wrote: > On 22/11/2023 3:49 pm, Luca Fancellu wrote: > > > >> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> > >> On 21/11/2023 8:33 pm, Luca Fancellu wrote: > >>> + CC henry > >>> > >>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> > >>>> --- > >>>> CC: Jan Beulich <JBeulich@suse.com> > >>>> CC: Roger Pau Monné <roger.pau@citrix.com> > >>>> CC: Wei Liu <wl@xen.org> > >>>> CC: Stefano Stabellini <sstabellini@kernel.org> > >>>> CC: Julien Grall <julien@xen.org> > >>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > >>>> CC: Bertrand Marquis <bertrand.marquis@arm.com> > >>>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> > >>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> > >>>> > >>>> 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@arm.com> > > Tested-by: Luca Fancellu <luca.fancellu@arm.com> > > Thanks, and you confirmed that the first cmdline parameter is still usable? Assuming Luca or Henry come back with a confirmation that the first command line parameter is still passed through correctly: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi, > On Nov 23, 2023, at 02:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 22/11/2023 3:49 pm, Luca Fancellu wrote: >> >>> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> >>> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>>> + CC henry >>>> >>>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>>>> --- >>>>> 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@arm.com> >> Tested-by: Luca Fancellu <luca.fancellu@arm.com> > > 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. Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > > ~Andrew
Hi, > On Nov 23, 2023, at 12:20, Henry Wang <Henry.Wang@arm.com> wrote: > > Hi, > >> On Nov 23, 2023, at 02:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 22/11/2023 3:49 pm, Luca Fancellu wrote: >>> >>>> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> >>>> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>>>> + CC henry >>>>> >>>>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>>>>> --- >>>>>> 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@arm.com> >>> Tested-by: Luca Fancellu <luca.fancellu@arm.com> >> >> 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@arm.com> > > Kind regards, > Henry > >> >> ~Andrew >
> On 22 Nov 2023, at 18:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 22/11/2023 3:49 pm, Luca Fancellu wrote: >> >>> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> >>> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>>> + CC henry >>>> >>>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>>>> --- >>>>> CC: Jan Beulich <JBeulich@suse.com> >>>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>>> CC: Wei Liu <wl@xen.org> >>>>> CC: Stefano Stabellini <sstabellini@kernel.org> >>>>> CC: Julien Grall <julien@xen.org> >>>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>>>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >>>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> >>>>> 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@arm.com> >> Tested-by: Luca Fancellu <luca.fancellu@arm.com> > > Thanks, and you confirmed that the first cmdline parameter is still usable? Yes, I’m using a xen.cfg like this: [global] default=xen [xen] options=noreboot dom0_mem=4096M bootscrub=0 iommu=on loglvl=error guest_loglvl=error kernel=Image console=hvc0 earlycon=xenboot rootwait root=PARTUUID=6a60524d-061d-454a-bfd1-38989910eccd And in Xen boot I can see this: [...] (XEN) Command line: noreboot dom0_mem=4096M bootscrub=0 iommu=on loglvl=error guest_loglvl=error [...] So I think it’s passing every parameter > > ~Andrew
On 23/11/2023 9:46 am, Luca Fancellu wrote: > >> On 22 Nov 2023, at 18:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 22/11/2023 3:49 pm, Luca Fancellu wrote: >>>> On 21 Nov 2023, at 20:41, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> >>>> On 21/11/2023 8:33 pm, Luca Fancellu wrote: >>>>> + CC henry >>>>> >>>>>> On 21 Nov 2023, at 20:15, Andrew Cooper <andrew.cooper3@citrix.com> 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@citrix.com> >>>>>> --- >>>>>> CC: Jan Beulich <JBeulich@suse.com> >>>>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>>>> CC: Wei Liu <wl@xen.org> >>>>>> CC: Stefano Stabellini <sstabellini@kernel.org> >>>>>> CC: Julien Grall <julien@xen.org> >>>>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>>>> CC: Bertrand Marquis <bertrand.marquis@arm.com> >>>>>> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> >>>>>> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>>> >>>>>> 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@arm.com> >>> Tested-by: Luca Fancellu <luca.fancellu@arm.com> >> Thanks, and you confirmed that the first cmdline parameter is still usable? > Yes, I’m using a xen.cfg like this: > > [global] > default=xen > > [xen] > options=noreboot dom0_mem=4096M bootscrub=0 iommu=on loglvl=error guest_loglvl=error > kernel=Image console=hvc0 earlycon=xenboot rootwait root=PARTUUID=6a60524d-061d-454a-bfd1-38989910eccd > > And in Xen boot I can see this: > > [...] > (XEN) Command line: noreboot dom0_mem=4096M bootscrub=0 iommu=on loglvl=error guest_loglvl=error > [...] > > So I think it’s passing every parameter Thankyou all for the testing. ~Andrew
Hi Andrew, On 21/11/2023 20:15, Andrew Cooper 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. I have noticed this behavior a few years ago but never really looked why we added the name. So thanks for looking at it. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 1c3640bb65fd..59d217667ff3 100644 --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -461,7 +461,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, union string name; char *buf; EFI_STATUS status; - int prop_len; + int prop_len = 0; int chosen; /* locate chosen node, which is where we add Xen module info. */ @@ -473,20 +473,6 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, if ( EFI_ERROR(status) ) PrintErrMesg(L"Unable to allocate string buffer", status); - if ( image_name ) - { - name.w = image_name; - w2s(&name); - } - else - name.s = "xen"; - - prop_len = 0; - prop_len += snprintf(buf + prop_len, - EFI_PAGE_SIZE - prop_len, "%s", name.s); - if ( prop_len >= EFI_PAGE_SIZE ) - blexit(L"FDT string overflow"); - if ( cfgfile_options ) { PrintMessage(L"Using bootargs from Xen configuration file.");
-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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> 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. --- xen/arch/arm/efi/efi-boot.h | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-)