| Message ID | 20250414081449.1763962-1-grygorii_strashko@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ImageBuilder] uboot-script-gen: add xen xsm policy loading support | expand |
As a general question, why using "test" so much, rather than if [ -n "$FOO" -a "FOO" == ABC ] Using test seems far harder to read than it needs to be, and single brackets are perfectly valid POSIX shell. On Mon Apr 14, 2025 at 9:14 AM BST, Grygorii Strashko wrote: > From: Grygorii Strashko <grygorii_strashko@epam.com> > > This patch adds Xen XSM policy loading support. > > The configuration file XEN_POLICY specifies Xen hypervisor > XSM policy binary to load. > > Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> > --- > README.md | 2 ++ > scripts/uboot-script-gen | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/README.md b/README.md > index 137abef153ce..9106d2a07302 100644 > --- a/README.md > +++ b/README.md > @@ -91,6 +91,8 @@ Where: > - XEN specifies the Xen hypervisor binary to load. Note that it has to > be a regular Xen binary, not a u-boot binary. > > +- XEN_POLICY specifies the Xen hypervisor XSM policy binary to load. > + > - XEN_COLORS specifies the colors (cache coloring) to be used for Xen > and is in the format startcolor-endcolor > > diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen > index c4d26caf5e0e..343eba20e4d9 100755 > --- a/scripts/uboot-script-gen > +++ b/scripts/uboot-script-gen > @@ -315,6 +315,15 @@ function xen_device_tree_editing() > dt_set "/chosen" "#size-cells" "hex" "0x2" > dt_set "/chosen" "xen,xen-bootargs" "str" "$XEN_CMD" > > + if test "$XEN_POLICY" && test $xen_policy_addr != "-" If XEN_POLICY is a binary, shouldn't it be "test -f" ? Same later on. Also, missing quotes around $xen_policy_addr > + then > + local node_name="xen-policy@${xen_policy_addr#0x}" > + > + dt_mknode "/chosen" "$node_name" > + dt_set "/chosen/$node_name" "compatible" "str_a" "xen,xsm-policy xen,multiboot-module multiboot,module" > + dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $xen_policy_addr $xen_policy_size)" > + fi > + > if test "$DOM0_KERNEL" test -f? g > then > local node_name="dom0@${dom0_kernel_addr#0x}" > @@ -900,6 +909,14 @@ xen_file_loading() > kernel_addr=$memaddr > kernel_path=$XEN > load_file "$XEN" "host_kernel" > + > + xen_policy_addr=="-" Do you mean = ? > + if test "$XEN_POLICY" > + then > + xen_policy_addr=$memaddr > + load_file "$XEN_POLICY" "xen_policy" > + xen_policy_size=$filesize > + fi > } > > linux_file_loading() > @@ -939,6 +956,22 @@ bitstream_load_and_config() > > create_its_file_xen() > { > + if test "$XEN_POLICY" && test $xen_policy_addr != "-" > + then > + cat >> "$its_file" <<- EOF > + xen_policy { > + description = "Xen XSM policy binary"; > + data = /incbin/("$XEN_POLICY"); > + type = "kernel"; > + arch = "arm64"; > + os = "linux"; > + compression = "none"; > + load = <$xen_policy_addr>; > + $fit_algo > + }; > + EOF > + fi > + > if test "$DOM0_KERNEL" > then > if test "$ramdisk_addr" != "-" Cheers, Alejandro
On Mon Apr 14, 2025 at 12:57 PM BST, Alejandro Vallejo wrote: > As a general question, why using "test" so much, rather than > > if [ -n "$FOO" -a "FOO" == ABC ] > > Using test seems far harder to read than it needs to be, and single > brackets are perfectly valid POSIX shell. > > On Mon Apr 14, 2025 at 9:14 AM BST, Grygorii Strashko wrote: >> From: Grygorii Strashko <grygorii_strashko@epam.com> >> >> This patch adds Xen XSM policy loading support. >> >> The configuration file XEN_POLICY specifies Xen hypervisor >> XSM policy binary to load. >> >> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> >> --- >> README.md | 2 ++ >> scripts/uboot-script-gen | 33 +++++++++++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/README.md b/README.md >> index 137abef153ce..9106d2a07302 100644 >> --- a/README.md >> +++ b/README.md >> @@ -91,6 +91,8 @@ Where: >> - XEN specifies the Xen hypervisor binary to load. Note that it has to >> be a regular Xen binary, not a u-boot binary. >> >> +- XEN_POLICY specifies the Xen hypervisor XSM policy binary to load. >> + >> - XEN_COLORS specifies the colors (cache coloring) to be used for Xen >> and is in the format startcolor-endcolor >> >> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen >> index c4d26caf5e0e..343eba20e4d9 100755 >> --- a/scripts/uboot-script-gen >> +++ b/scripts/uboot-script-gen >> @@ -315,6 +315,15 @@ function xen_device_tree_editing() >> dt_set "/chosen" "#size-cells" "hex" "0x2" >> dt_set "/chosen" "xen,xen-bootargs" "str" "$XEN_CMD" >> >> + if test "$XEN_POLICY" && test $xen_policy_addr != "-" > > If XEN_POLICY is a binary, shouldn't it be "test -f" ? Same later on. On the absent flag: Nevermind, I see you're using "-n" implicitly instead. I'd rather it be less opaque, but it does function. I do think most uses of test "$FOO" would be better off with an explicit -f or -n. But that's a separate matter. Cheers, Alejandro
Hi Alejandro, On 14.04.25 14:57, Alejandro Vallejo wrote: > As a general question, why using "test" so much, rather than > > if [ -n "$FOO" -a "FOO" == ABC ] > > Using test seems far harder to read than it needs to be, and single > brackets are perfectly valid POSIX shell. > > On Mon Apr 14, 2025 at 9:14 AM BST, Grygorii Strashko wrote: >> From: Grygorii Strashko <grygorii_strashko@epam.com> >> >> This patch adds Xen XSM policy loading support. >> >> The configuration file XEN_POLICY specifies Xen hypervisor >> XSM policy binary to load. >> >> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> >> --- >> README.md | 2 ++ >> scripts/uboot-script-gen | 33 +++++++++++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/README.md b/README.md >> index 137abef153ce..9106d2a07302 100644 >> --- a/README.md >> +++ b/README.md >> @@ -91,6 +91,8 @@ Where: >> - XEN specifies the Xen hypervisor binary to load. Note that it has to >> be a regular Xen binary, not a u-boot binary. >> >> +- XEN_POLICY specifies the Xen hypervisor XSM policy binary to load. >> + >> - XEN_COLORS specifies the colors (cache coloring) to be used for Xen >> and is in the format startcolor-endcolor >> >> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen >> index c4d26caf5e0e..343eba20e4d9 100755 >> --- a/scripts/uboot-script-gen >> +++ b/scripts/uboot-script-gen >> @@ -315,6 +315,15 @@ function xen_device_tree_editing() >> dt_set "/chosen" "#size-cells" "hex" "0x2" >> dt_set "/chosen" "xen,xen-bootargs" "str" "$XEN_CMD" >> >> + if test "$XEN_POLICY" && test $xen_policy_addr != "-" > > If XEN_POLICY is a binary, shouldn't it be "test -f" ? Same later on. You're right. I'll update it as below... > > Also, missing quotes around $xen_policy_addr > >> + then >> + local node_name="xen-policy@${xen_policy_addr#0x}" >> + >> + dt_mknode "/chosen" "$node_name" >> + dt_set "/chosen/$node_name" "compatible" "str_a" "xen,xsm-policy xen,multiboot-module multiboot,module" >> + dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $xen_policy_addr $xen_policy_size)" >> + fi >> + >> if test "$DOM0_KERNEL" > > test -f? > g >> then >> local node_name="dom0@${dom0_kernel_addr#0x}" >> @@ -900,6 +909,14 @@ xen_file_loading() >> kernel_addr=$memaddr >> kernel_path=$XEN >> load_file "$XEN" "host_kernel" >> + >> + xen_policy_addr=="-" > > Do you mean = ? > >> + if test "$XEN_POLICY" >> + then I'll add here: check_file_type "${XEN_POLICY}" "SE Linux policy" and fix other comments. >> + xen_policy_addr=$memaddr >> + load_file "$XEN_POLICY" "xen_policy" >> + xen_policy_size=$filesize >> + fi >> } >> >> linux_file_loading() >> @@ -939,6 +956,22 @@ bitstream_load_and_config() >> >> create_its_file_xen() >> { >> + if test "$XEN_POLICY" && test $xen_policy_addr != "-" >> + then >> + cat >> "$its_file" <<- EOF >> + xen_policy { >> + description = "Xen XSM policy binary"; >> + data = /incbin/("$XEN_POLICY"); >> + type = "kernel"; >> + arch = "arm64"; >> + os = "linux"; >> + compression = "none"; >> + load = <$xen_policy_addr>; >> + $fit_algo >> + }; >> + EOF >> + fi >> + >> if test "$DOM0_KERNEL" >> then >> if test "$ramdisk_addr" != "-" Thanks for your review.
diff --git a/README.md b/README.md index 137abef153ce..9106d2a07302 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,8 @@ Where: - XEN specifies the Xen hypervisor binary to load. Note that it has to be a regular Xen binary, not a u-boot binary. +- XEN_POLICY specifies the Xen hypervisor XSM policy binary to load. + - XEN_COLORS specifies the colors (cache coloring) to be used for Xen and is in the format startcolor-endcolor diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen index c4d26caf5e0e..343eba20e4d9 100755 --- a/scripts/uboot-script-gen +++ b/scripts/uboot-script-gen @@ -315,6 +315,15 @@ function xen_device_tree_editing() dt_set "/chosen" "#size-cells" "hex" "0x2" dt_set "/chosen" "xen,xen-bootargs" "str" "$XEN_CMD" + if test "$XEN_POLICY" && test $xen_policy_addr != "-" + then + local node_name="xen-policy@${xen_policy_addr#0x}" + + dt_mknode "/chosen" "$node_name" + dt_set "/chosen/$node_name" "compatible" "str_a" "xen,xsm-policy xen,multiboot-module multiboot,module" + dt_set "/chosen/$node_name" "reg" "hex" "$(split_addr_size $xen_policy_addr $xen_policy_size)" + fi + if test "$DOM0_KERNEL" then local node_name="dom0@${dom0_kernel_addr#0x}" @@ -900,6 +909,14 @@ xen_file_loading() kernel_addr=$memaddr kernel_path=$XEN load_file "$XEN" "host_kernel" + + xen_policy_addr=="-" + if test "$XEN_POLICY" + then + xen_policy_addr=$memaddr + load_file "$XEN_POLICY" "xen_policy" + xen_policy_size=$filesize + fi } linux_file_loading() @@ -939,6 +956,22 @@ bitstream_load_and_config() create_its_file_xen() { + if test "$XEN_POLICY" && test $xen_policy_addr != "-" + then + cat >> "$its_file" <<- EOF + xen_policy { + description = "Xen XSM policy binary"; + data = /incbin/("$XEN_POLICY"); + type = "kernel"; + arch = "arm64"; + os = "linux"; + compression = "none"; + load = <$xen_policy_addr>; + $fit_algo + }; + EOF + fi + if test "$DOM0_KERNEL" then if test "$ramdisk_addr" != "-"