[PATCH 1/2] uboot-script-gen: prevent user mistakes due to DOM0_KERNEL becoming optional

[PATCH 1/2] uboot-script-gen: prevent user mistakes due to DOM0_KERNEL becoming optional

[Xenia Ragiadakou]

Before enabling true dom0less configuration, the script was failing instantly
if DOM0_KERNEL parameter was not specified. This behaviour has changed and
this needs to be communicated to the user.

Mention in README.md that for dom0less configurations, the parameter
DOM0_KERNEL is optional.

If DOM0_KERNEL is not set, check that no other dom0 specific parameters are
specified by the user. Fail the script early with an appropriate error
message, if it was invoked with erroneous configuration settings.

Change message "Dom0 kernel is not specified, continue with dom0less setup."
to "Dom0 kernel is not specified, continue with true dom0less setup."
to refer more accurately to a dom0less setup without dom0.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 README.md                |  1 +
 scripts/uboot-script-gen | 21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/README.md b/README.md
index 17ff206..cb15ca5 100644
--- a/README.md
+++ b/README.md
@@ -100,6 +100,7 @@ Where:
   been specified in XEN_PASSTHROUGH_PATHS.
 
 - DOM0_KERNEL specifies the Dom0 kernel file to load.
+  For dom0less configurations, the parameter is optional.
 
 - DOM0_MEM specifies the amount of memory for Dom0 VM in MB. The default
   is 1024. This is only applicable when XEN_CMD is not specified.
diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index e85c6ec..085e29f 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -410,6 +410,20 @@ function find_root_dev()
 
 function xen_config()
 {
+    if test -z "$DOM0_KERNEL"
+    then
+        if test "$NUM_DOMUS" -eq "0"
+        then
+            echo "Neither dom0 or domUs are specified, exiting."
+            exit 1
+        elif test "$DOM0_MEM" || test "$DOM0_VCPUS" || test "$DOM0_COLORS" || test "$DOM0_CMD" || test "$DOM0_RAMDISK" || test "$DOM0_ROOTFS"
+        then
+            echo "For dom0less configuration without dom0, no dom0 specific parameters should be specified, exiting."
+            exit 1
+        fi
+        echo "Dom0 kernel is not specified, continue with true dom0less setup."
+    fi
+
     if [ -z "$XEN_CMD" ]
     then
         if [ -z "$DOM0_MEM" ]
@@ -457,13 +471,6 @@ function xen_config()
     fi
     if test -z "$DOM0_KERNEL"
     then
-        if test "$NUM_DOMUS" -eq "0"
-        then
-            echo "Neither dom0 or domUs are specified, exiting."
-            exit 1
-        fi
-        echo "Dom0 kernel is not specified, continue with dom0less setup."
-        unset DOM0_RAMDISK
         # Remove dom0 specific parameters from the XEN command line.
         local params=($XEN_CMD)
         XEN_CMD="${params[@]/dom0*/}"
-- 
2.34.1

[PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[Xenia Ragiadakou]

To be inline with XEN, do not enable direct mapping automatically for all
statically allocated domains.

Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
---
 README.md                | 4 ++--
 scripts/uboot-script-gen | 8 ++------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/README.md b/README.md
index cb15ca5..03e437b 100644
--- a/README.md
+++ b/README.md
@@ -169,8 +169,8 @@ Where:
   if specified, indicates the host physical address regions
   [baseaddr, baseaddr + size) to be reserved to the VM for static allocation.
 
-- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
-  If set to 1, the VM is direct mapped. The default is 1.
+- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
+  By default, direct mapping is disabled.
   This is only applicable when DOMU_STATIC_MEM is specified.
 
 - LINUX is optional but specifies the Linux kernel for when Xen is NOT
diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index 085e29f..66ce6f7 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -52,7 +52,7 @@ function dt_set()
             echo "fdt set $path $var $array" >> $UBOOT_SOURCE
         elif test $data_type = "bool"
         then
-            if test "$data" -eq 1
+            if test "$data" == "1"
             then
                 echo "fdt set $path $var" >> $UBOOT_SOURCE
             fi
@@ -74,7 +74,7 @@ function dt_set()
             fdtput $FDTEDIT -p -t s $path $var $data
         elif test $data_type = "bool"
         then
-            if test "$data" -eq 1
+            if test "$data" == "1"
             then
                 fdtput $FDTEDIT -p $path $var
             fi
@@ -491,10 +491,6 @@ function xen_config()
         then
             DOMU_CMD[$i]="console=ttyAMA0"
         fi
-        if test -z "${DOMU_DIRECT_MAP[$i]}"
-        then
-             DOMU_DIRECT_MAP[$i]=1
-        fi
         i=$(( $i + 1 ))
     done
 }
-- 
2.34.1

Re: [Viryaos-discuss] [PATCH 1/2] uboot-script-gen: prevent user mistakes due to DOM0_KERNEL becoming optional

[Ayan Kumar Halder]

On 26/06/2022 19:45, Xenia Ragiadakou wrote:
> Before enabling true dom0less configuration, the script was failing instantly
> if DOM0_KERNEL parameter was not specified. This behaviour has changed and
> this needs to be communicated to the user.
>
> Mention in README.md that for dom0less configurations, the parameter
> DOM0_KERNEL is optional.
>
> If DOM0_KERNEL is not set, check that no other dom0 specific parameters are
> specified by the user. Fail the script early with an appropriate error
> message, if it was invoked with erroneous configuration settings.
>
> Change message "Dom0 kernel is not specified, continue with dom0less setup."
> to "Dom0 kernel is not specified, continue with true dom0less setup."
> to refer more accurately to a dom0less setup without dom0.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>
> ---
>   README.md                |  1 +
>   scripts/uboot-script-gen | 21 ++++++++++++++-------
>   2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/README.md b/README.md
> index 17ff206..cb15ca5 100644
> --- a/README.md
> +++ b/README.md
> @@ -100,6 +100,7 @@ Where:
>     been specified in XEN_PASSTHROUGH_PATHS.
>   
>   - DOM0_KERNEL specifies the Dom0 kernel file to load.
> +  For dom0less configurations, the parameter is optional.
>   
>   - DOM0_MEM specifies the amount of memory for Dom0 VM in MB. The default
>     is 1024. This is only applicable when XEN_CMD is not specified.
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index e85c6ec..085e29f 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -410,6 +410,20 @@ function find_root_dev()
>   
>   function xen_config()
>   {
> +    if test -z "$DOM0_KERNEL"
> +    then
> +        if test "$NUM_DOMUS" -eq "0"
> +        then
> +            echo "Neither dom0 or domUs are specified, exiting."
> +            exit 1
> +        elif test "$DOM0_MEM" || test "$DOM0_VCPUS" || test "$DOM0_COLORS" || test "$DOM0_CMD" || test "$DOM0_RAMDISK" || test "$DOM0_ROOTFS"
> +        then
> +            echo "For dom0less configuration without dom0, no dom0 specific parameters should be specified, exiting."
> +            exit 1
> +        fi
> +        echo "Dom0 kernel is not specified, continue with true dom0less setup."
> +    fi
> +
>       if [ -z "$XEN_CMD" ]
>       then
>           if [ -z "$DOM0_MEM" ]
> @@ -457,13 +471,6 @@ function xen_config()
>       fi
>       if test -z "$DOM0_KERNEL"
>       then
> -        if test "$NUM_DOMUS" -eq "0"
> -        then
> -            echo "Neither dom0 or domUs are specified, exiting."
> -            exit 1
> -        fi
> -        echo "Dom0 kernel is not specified, continue with dom0less setup."
> -        unset DOM0_RAMDISK
>           # Remove dom0 specific parameters from the XEN command line.
>           local params=($XEN_CMD)
>           XEN_CMD="${params[@]/dom0*/}"

Re: [Viryaos-discuss] [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[Ayan Kumar Halder]

On 26/06/2022 19:45, Xenia Ragiadakou wrote:
> To be inline with XEN, do not enable direct mapping automatically for all
> statically allocated domains.
>
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>
> ---
>   README.md                | 4 ++--
>   scripts/uboot-script-gen | 8 ++------
>   2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/README.md b/README.md
> index cb15ca5..03e437b 100644
> --- a/README.md
> +++ b/README.md
> @@ -169,8 +169,8 @@ Where:
>     if specified, indicates the host physical address regions
>     [baseaddr, baseaddr + size) to be reserved to the VM for static allocation.
>   
> -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
> -  If set to 1, the VM is direct mapped. The default is 1.
> +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
> +  By default, direct mapping is disabled.
>     This is only applicable when DOMU_STATIC_MEM is specified.
>   
>   - LINUX is optional but specifies the Linux kernel for when Xen is NOT
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 085e29f..66ce6f7 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -52,7 +52,7 @@ function dt_set()
>               echo "fdt set $path $var $array" >> $UBOOT_SOURCE
>           elif test $data_type = "bool"
>           then
> -            if test "$data" -eq 1
> +            if test "$data" == "1"
>               then
>                   echo "fdt set $path $var" >> $UBOOT_SOURCE
>               fi
> @@ -74,7 +74,7 @@ function dt_set()
>               fdtput $FDTEDIT -p -t s $path $var $data
>           elif test $data_type = "bool"
>           then
> -            if test "$data" -eq 1
> +            if test "$data" == "1"
>               then
>                   fdtput $FDTEDIT -p $path $var
>               fi
> @@ -491,10 +491,6 @@ function xen_config()
>           then
>               DOMU_CMD[$i]="console=ttyAMA0"
>           fi
> -        if test -z "${DOMU_DIRECT_MAP[$i]}"
> -        then
> -             DOMU_DIRECT_MAP[$i]=1
> -        fi
>           i=$(( $i + 1 ))
>       done
>   }

Re: [PATCH 1/2] uboot-script-gen: prevent user mistakes due to DOM0_KERNEL becoming optional

[Stefano Stabellini]

On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
> Before enabling true dom0less configuration, the script was failing instantly
> if DOM0_KERNEL parameter was not specified. This behaviour has changed and
> this needs to be communicated to the user.
> 
> Mention in README.md that for dom0less configurations, the parameter
> DOM0_KERNEL is optional.
> 
> If DOM0_KERNEL is not set, check that no other dom0 specific parameters are
> specified by the user. Fail the script early with an appropriate error
> message, if it was invoked with erroneous configuration settings.
> 
> Change message "Dom0 kernel is not specified, continue with dom0less setup."
> to "Dom0 kernel is not specified, continue with true dom0less setup."
> to refer more accurately to a dom0less setup without dom0.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  README.md                |  1 +
>  scripts/uboot-script-gen | 21 ++++++++++++++-------
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/README.md b/README.md
> index 17ff206..cb15ca5 100644
> --- a/README.md
> +++ b/README.md
> @@ -100,6 +100,7 @@ Where:
>    been specified in XEN_PASSTHROUGH_PATHS.
>  
>  - DOM0_KERNEL specifies the Dom0 kernel file to load.
> +  For dom0less configurations, the parameter is optional.
>  
>  - DOM0_MEM specifies the amount of memory for Dom0 VM in MB. The default
>    is 1024. This is only applicable when XEN_CMD is not specified.
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index e85c6ec..085e29f 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -410,6 +410,20 @@ function find_root_dev()
>  
>  function xen_config()
>  {
> +    if test -z "$DOM0_KERNEL"
> +    then
> +        if test "$NUM_DOMUS" -eq "0"
> +        then
> +            echo "Neither dom0 or domUs are specified, exiting."
> +            exit 1
> +        elif test "$DOM0_MEM" || test "$DOM0_VCPUS" || test "$DOM0_COLORS" || test "$DOM0_CMD" || test "$DOM0_RAMDISK" || test "$DOM0_ROOTFS"
> +        then
> +            echo "For dom0less configuration without dom0, no dom0 specific parameters should be specified, exiting."
> +            exit 1
> +        fi
> +        echo "Dom0 kernel is not specified, continue with true dom0less setup."
> +    fi
> +
>      if [ -z "$XEN_CMD" ]
>      then
>          if [ -z "$DOM0_MEM" ]
> @@ -457,13 +471,6 @@ function xen_config()
>      fi
>      if test -z "$DOM0_KERNEL"
>      then
> -        if test "$NUM_DOMUS" -eq "0"
> -        then
> -            echo "Neither dom0 or domUs are specified, exiting."
> -            exit 1
> -        fi
> -        echo "Dom0 kernel is not specified, continue with dom0less setup."
> -        unset DOM0_RAMDISK
>          # Remove dom0 specific parameters from the XEN command line.
>          local params=($XEN_CMD)
>          XEN_CMD="${params[@]/dom0*/}"
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[Stefano Stabellini]

On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
> To be inline with XEN, do not enable direct mapping automatically for all
> statically allocated domains.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>

Actually I don't know about this one. I think it is OK that ImageBuilder
defaults are different from Xen defaults. This is a case where I think
it would be good to enable DOMU_DIRECT_MAP by default when
DOMU_STATIC_MEM is specified.


> ---
>  README.md                | 4 ++--
>  scripts/uboot-script-gen | 8 ++------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/README.md b/README.md
> index cb15ca5..03e437b 100644
> --- a/README.md
> +++ b/README.md
> @@ -169,8 +169,8 @@ Where:
>    if specified, indicates the host physical address regions
>    [baseaddr, baseaddr + size) to be reserved to the VM for static allocation.
>  
> -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
> -  If set to 1, the VM is direct mapped. The default is 1.
> +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
> +  By default, direct mapping is disabled.
>    This is only applicable when DOMU_STATIC_MEM is specified.
>  
>  - LINUX is optional but specifies the Linux kernel for when Xen is NOT
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 085e29f..66ce6f7 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -52,7 +52,7 @@ function dt_set()
>              echo "fdt set $path $var $array" >> $UBOOT_SOURCE
>          elif test $data_type = "bool"
>          then
> -            if test "$data" -eq 1
> +            if test "$data" == "1"
>              then
>                  echo "fdt set $path $var" >> $UBOOT_SOURCE
>              fi
> @@ -74,7 +74,7 @@ function dt_set()
>              fdtput $FDTEDIT -p -t s $path $var $data
>          elif test $data_type = "bool"
>          then
> -            if test "$data" -eq 1
> +            if test "$data" == "1"
>              then
>                  fdtput $FDTEDIT -p $path $var
>              fi
> @@ -491,10 +491,6 @@ function xen_config()
>          then
>              DOMU_CMD[$i]="console=ttyAMA0"
>          fi
> -        if test -z "${DOMU_DIRECT_MAP[$i]}"
> -        then
> -             DOMU_DIRECT_MAP[$i]=1
> -        fi
>          i=$(( $i + 1 ))
>      done
>  }
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[xenia]

Hi Stefano,

On 6/29/22 03:28, Stefano Stabellini wrote:
> On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
>> To be inline with XEN, do not enable direct mapping automatically for all
>> statically allocated domains.
>>
>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> Actually I don't know about this one. I think it is OK that ImageBuilder
> defaults are different from Xen defaults. This is a case where I think
> it would be good to enable DOMU_DIRECT_MAP by default when
> DOMU_STATIC_MEM is specified.
Just realized that I forgot to add [ImageBuilder] tag to the patches. 
Sorry about that.

I cc Ayan, since the change was suggested by him.
I have no strong preference on the default value.

Xenia

>> ---
>>   README.md                | 4 ++--
>>   scripts/uboot-script-gen | 8 ++------
>>   2 files changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/README.md b/README.md
>> index cb15ca5..03e437b 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -169,8 +169,8 @@ Where:
>>     if specified, indicates the host physical address regions
>>     [baseaddr, baseaddr + size) to be reserved to the VM for static allocation.
>>   
>> -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
>> -  If set to 1, the VM is direct mapped. The default is 1.
>> +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
>> +  By default, direct mapping is disabled.
>>     This is only applicable when DOMU_STATIC_MEM is specified.
>>   
>>   - LINUX is optional but specifies the Linux kernel for when Xen is NOT
>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>> index 085e29f..66ce6f7 100755
>> --- a/scripts/uboot-script-gen
>> +++ b/scripts/uboot-script-gen
>> @@ -52,7 +52,7 @@ function dt_set()
>>               echo "fdt set $path $var $array" >> $UBOOT_SOURCE
>>           elif test $data_type = "bool"
>>           then
>> -            if test "$data" -eq 1
>> +            if test "$data" == "1"
>>               then
>>                   echo "fdt set $path $var" >> $UBOOT_SOURCE
>>               fi
>> @@ -74,7 +74,7 @@ function dt_set()
>>               fdtput $FDTEDIT -p -t s $path $var $data
>>           elif test $data_type = "bool"
>>           then
>> -            if test "$data" -eq 1
>> +            if test "$data" == "1"
>>               then
>>                   fdtput $FDTEDIT -p $path $var
>>               fi
>> @@ -491,10 +491,6 @@ function xen_config()
>>           then
>>               DOMU_CMD[$i]="console=ttyAMA0"
>>           fi
>> -        if test -z "${DOMU_DIRECT_MAP[$i]}"
>> -        then
>> -             DOMU_DIRECT_MAP[$i]=1
>> -        fi
>>           i=$(( $i + 1 ))
>>       done
>>   }
>> -- 
>> 2.34.1
>>

Re: [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[Ayan Kumar Halder]

Hi Stefano/Xenia,

On 29/06/2022 18:01, xenia wrote:
> Hi Stefano,
>
> On 6/29/22 03:28, Stefano Stabellini wrote:
>> On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
>>> To be inline with XEN, do not enable direct mapping automatically 
>>> for all
>>> statically allocated domains.
>>>
>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>> Actually I don't know about this one. I think it is OK that ImageBuilder
>> defaults are different from Xen defaults. This is a case where I think
>> it would be good to enable DOMU_DIRECT_MAP by default when
>> DOMU_STATIC_MEM is specified.
> Just realized that I forgot to add [ImageBuilder] tag to the patches. 
> Sorry about that.

@Stefano, why do you wish the Imagebuilder's behaviour to differ from 
Xen ? Is there any use-case that helps.

- Ayan

>
> I cc Ayan, since the change was suggested by him.
> I have no strong preference on the default value.
>
> Xenia
>
>>> ---
>>>   README.md                | 4 ++--
>>>   scripts/uboot-script-gen | 8 ++------
>>>   2 files changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/README.md b/README.md
>>> index cb15ca5..03e437b 100644
>>> --- a/README.md
>>> +++ b/README.md
>>> @@ -169,8 +169,8 @@ Where:
>>>     if specified, indicates the host physical address regions
>>>     [baseaddr, baseaddr + size) to be reserved to the VM for static 
>>> allocation.
>>>   -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
>>> -  If set to 1, the VM is direct mapped. The default is 1.
>>> +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
>>> +  By default, direct mapping is disabled.
>>>     This is only applicable when DOMU_STATIC_MEM is specified.
>>>     - LINUX is optional but specifies the Linux kernel for when Xen 
>>> is NOT
>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>>> index 085e29f..66ce6f7 100755
>>> --- a/scripts/uboot-script-gen
>>> +++ b/scripts/uboot-script-gen
>>> @@ -52,7 +52,7 @@ function dt_set()
>>>               echo "fdt set $path $var $array" >> $UBOOT_SOURCE
>>>           elif test $data_type = "bool"
>>>           then
>>> -            if test "$data" -eq 1
>>> +            if test "$data" == "1"
>>>               then
>>>                   echo "fdt set $path $var" >> $UBOOT_SOURCE
>>>               fi
>>> @@ -74,7 +74,7 @@ function dt_set()
>>>               fdtput $FDTEDIT -p -t s $path $var $data
>>>           elif test $data_type = "bool"
>>>           then
>>> -            if test "$data" -eq 1
>>> +            if test "$data" == "1"
>>>               then
>>>                   fdtput $FDTEDIT -p $path $var
>>>               fi
>>> @@ -491,10 +491,6 @@ function xen_config()
>>>           then
>>>               DOMU_CMD[$i]="console=ttyAMA0"
>>>           fi
>>> -        if test -z "${DOMU_DIRECT_MAP[$i]}"
>>> -        then
>>> -             DOMU_DIRECT_MAP[$i]=1
>>> -        fi
>>>           i=$(( $i + 1 ))
>>>       done
>>>   }
>>> -- 
>>> 2.34.1
>>>

Re: [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]

On Wed, 29 Jun 2022, Ayan Kumar Halder wrote:
> Hi Stefano/Xenia,
> 
> On 29/06/2022 18:01, xenia wrote:
> > Hi Stefano,
> > 
> > On 6/29/22 03:28, Stefano Stabellini wrote:
> > > On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
> > > > To be inline with XEN, do not enable direct mapping automatically for
> > > > all
> > > > statically allocated domains.
> > > > 
> > > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
> > > Actually I don't know about this one. I think it is OK that ImageBuilder
> > > defaults are different from Xen defaults. This is a case where I think
> > > it would be good to enable DOMU_DIRECT_MAP by default when
> > > DOMU_STATIC_MEM is specified.
> > Just realized that I forgot to add [ImageBuilder] tag to the patches. Sorry
> > about that.
> 
> @Stefano, why do you wish the Imagebuilder's behaviour to differ from Xen ? Is
> there any use-case that helps.

As background, ImageBuilder is meant to be very simple to use especially
for the most common configurations. In fact, I think ImageBuilder
doesn't necessarely have to support all the options that Xen offers,
only the most common and important.

If someone wants an esoteric option, they can always edit the generated
boot.source and make any necessary changes. I make sure to explain that
editing boot.source is always a possibility in all the talks I gave
about ImageBuilder.

Now to answer the specific question. I am positive that the most common
configuration for people that wants static memory is to have direct_map.
That is because the two go hand-in-hand in configuration where the IOMMU
is not used. So I think that from an ImageBuilder perspective direct_map
should default to enabled when static memory is requested. It can always
be disabled, both using DOMU_DIRECT_MAP, or editing boot.source.


> > I cc Ayan, since the change was suggested by him.
> > I have no strong preference on the default value.
> > 
> > Xenia
> > 
> > > > ---
> > > >   README.md                | 4 ++--
> > > >   scripts/uboot-script-gen | 8 ++------
> > > >   2 files changed, 4 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/README.md b/README.md
> > > > index cb15ca5..03e437b 100644
> > > > --- a/README.md
> > > > +++ b/README.md
> > > > @@ -169,8 +169,8 @@ Where:
> > > >     if specified, indicates the host physical address regions
> > > >     [baseaddr, baseaddr + size) to be reserved to the VM for static
> > > > allocation.
> > > >   -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
> > > > -  If set to 1, the VM is direct mapped. The default is 1.
> > > > +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
> > > > +  By default, direct mapping is disabled.
> > > >     This is only applicable when DOMU_STATIC_MEM is specified.
> > > >     - LINUX is optional but specifies the Linux kernel for when Xen is
> > > > NOT
> > > > diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> > > > index 085e29f..66ce6f7 100755
> > > > --- a/scripts/uboot-script-gen
> > > > +++ b/scripts/uboot-script-gen
> > > > @@ -52,7 +52,7 @@ function dt_set()
> > > >               echo "fdt set $path $var $array" >> $UBOOT_SOURCE
> > > >           elif test $data_type = "bool"
> > > >           then
> > > > -            if test "$data" -eq 1
> > > > +            if test "$data" == "1"
> > > >               then
> > > >                   echo "fdt set $path $var" >> $UBOOT_SOURCE
> > > >               fi
> > > > @@ -74,7 +74,7 @@ function dt_set()
> > > >               fdtput $FDTEDIT -p -t s $path $var $data
> > > >           elif test $data_type = "bool"
> > > >           then
> > > > -            if test "$data" -eq 1
> > > > +            if test "$data" == "1"
> > > >               then
> > > >                   fdtput $FDTEDIT -p $path $var
> > > >               fi
> > > > @@ -491,10 +491,6 @@ function xen_config()
> > > >           then
> > > >               DOMU_CMD[$i]="console=ttyAMA0"
> > > >           fi
> > > > -        if test -z "${DOMU_DIRECT_MAP[$i]}"
> > > > -        then
> > > > -             DOMU_DIRECT_MAP[$i]=1
> > > > -        fi
> > > >           i=$(( $i + 1 ))
> > > >       done
> > > >   }
> > > > -- 
> > > > 2.34.1
> > > > 
> 

Re: [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[Ayan Kumar Halder]

On 29/06/2022 21:28, Stefano Stabellini wrote:
> On Wed, 29 Jun 2022, Ayan Kumar Halder wrote:
>> Hi Stefano/Xenia,
>>
>> On 29/06/2022 18:01, xenia wrote:
>>> Hi Stefano,
>>>
>>> On 6/29/22 03:28, Stefano Stabellini wrote:
>>>> On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
>>>>> To be inline with XEN, do not enable direct mapping automatically for
>>>>> all
>>>>> statically allocated domains.
>>>>>
>>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>>> Actually I don't know about this one. I think it is OK that ImageBuilder
>>>> defaults are different from Xen defaults. This is a case where I think
>>>> it would be good to enable DOMU_DIRECT_MAP by default when
>>>> DOMU_STATIC_MEM is specified.
>>> Just realized that I forgot to add [ImageBuilder] tag to the patches. Sorry
>>> about that.
>> @Stefano, why do you wish the Imagebuilder's behaviour to differ from Xen ? Is
>> there any use-case that helps.
> As background, ImageBuilder is meant to be very simple to use especially
> for the most common configurations. In fact, I think ImageBuilder
> doesn't necessarely have to support all the options that Xen offers,
> only the most common and important.
>
> If someone wants an esoteric option, they can always edit the generated
> boot.source and make any necessary changes. I make sure to explain that
> editing boot.source is always a possibility in all the talks I gave
> about ImageBuilder.
>
> Now to answer the specific question. I am positive that the most common
> configuration for people that wants static memory is to have direct_map.
> That is because the two go hand-in-hand in configuration where the IOMMU
> is not used. So I think that from an ImageBuilder perspective direct_map
> should default to enabled when static memory is requested. It can always
> be disabled, both using DOMU_DIRECT_MAP, or editing boot.source.

Many thanks for the explanation. This makes sense.

I think this patch can be dropped then.

Xenia, apologies for suggesting you to do this in the first place.

- Ayan

>
>
>>> I cc Ayan, since the change was suggested by him.
>>> I have no strong preference on the default value.
>>>
>>> Xenia
>>>
>>>>> ---
>>>>>    README.md                | 4 ++--
>>>>>    scripts/uboot-script-gen | 8 ++------
>>>>>    2 files changed, 4 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/README.md b/README.md
>>>>> index cb15ca5..03e437b 100644
>>>>> --- a/README.md
>>>>> +++ b/README.md
>>>>> @@ -169,8 +169,8 @@ Where:
>>>>>      if specified, indicates the host physical address regions
>>>>>      [baseaddr, baseaddr + size) to be reserved to the VM for static
>>>>> allocation.
>>>>>    -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
>>>>> -  If set to 1, the VM is direct mapped. The default is 1.
>>>>> +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
>>>>> +  By default, direct mapping is disabled.
>>>>>      This is only applicable when DOMU_STATIC_MEM is specified.
>>>>>      - LINUX is optional but specifies the Linux kernel for when Xen is
>>>>> NOT
>>>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>>>>> index 085e29f..66ce6f7 100755
>>>>> --- a/scripts/uboot-script-gen
>>>>> +++ b/scripts/uboot-script-gen
>>>>> @@ -52,7 +52,7 @@ function dt_set()
>>>>>                echo "fdt set $path $var $array" >> $UBOOT_SOURCE
>>>>>            elif test $data_type = "bool"
>>>>>            then
>>>>> -            if test "$data" -eq 1
>>>>> +            if test "$data" == "1"
>>>>>                then
>>>>>                    echo "fdt set $path $var" >> $UBOOT_SOURCE
>>>>>                fi
>>>>> @@ -74,7 +74,7 @@ function dt_set()
>>>>>                fdtput $FDTEDIT -p -t s $path $var $data
>>>>>            elif test $data_type = "bool"
>>>>>            then
>>>>> -            if test "$data" -eq 1
>>>>> +            if test "$data" == "1"
>>>>>                then
>>>>>                    fdtput $FDTEDIT -p $path $var
>>>>>                fi
>>>>> @@ -491,10 +491,6 @@ function xen_config()
>>>>>            then
>>>>>                DOMU_CMD[$i]="console=ttyAMA0"
>>>>>            fi
>>>>> -        if test -z "${DOMU_DIRECT_MAP[$i]}"
>>>>> -        then
>>>>> -             DOMU_DIRECT_MAP[$i]=1
>>>>> -        fi
>>>>>            i=$(( $i + 1 ))
>>>>>        done
>>>>>    }
>>>>> -- 
>>>>> 2.34.1
>>>>>

Re: [PATCH 2/2] uboot-script-gen: do not enable direct mapping by default

[xenia]

Hi Ayan,

On 6/30/22 14:28, Ayan Kumar Halder wrote:
>
> On 29/06/2022 21:28, Stefano Stabellini wrote:
>> On Wed, 29 Jun 2022, Ayan Kumar Halder wrote:
>>> Hi Stefano/Xenia,
>>>
>>> On 29/06/2022 18:01, xenia wrote:
>>>> Hi Stefano,
>>>>
>>>> On 6/29/22 03:28, Stefano Stabellini wrote:
>>>>> On Sun, 26 Jun 2022, Xenia Ragiadakou wrote:
>>>>>> To be inline with XEN, do not enable direct mapping automatically 
>>>>>> for
>>>>>> all
>>>>>> statically allocated domains.
>>>>>>
>>>>>> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com>
>>>>> Actually I don't know about this one. I think it is OK that 
>>>>> ImageBuilder
>>>>> defaults are different from Xen defaults. This is a case where I 
>>>>> think
>>>>> it would be good to enable DOMU_DIRECT_MAP by default when
>>>>> DOMU_STATIC_MEM is specified.
>>>> Just realized that I forgot to add [ImageBuilder] tag to the 
>>>> patches. Sorry
>>>> about that.
>>> @Stefano, why do you wish the Imagebuilder's behaviour to differ 
>>> from Xen ? Is
>>> there any use-case that helps.
>> As background, ImageBuilder is meant to be very simple to use especially
>> for the most common configurations. In fact, I think ImageBuilder
>> doesn't necessarely have to support all the options that Xen offers,
>> only the most common and important.
>>
>> If someone wants an esoteric option, they can always edit the generated
>> boot.source and make any necessary changes. I make sure to explain that
>> editing boot.source is always a possibility in all the talks I gave
>> about ImageBuilder.
>>
>> Now to answer the specific question. I am positive that the most common
>> configuration for people that wants static memory is to have direct_map.
>> That is because the two go hand-in-hand in configuration where the IOMMU
>> is not used. So I think that from an ImageBuilder perspective direct_map
>> should default to enabled when static memory is requested. It can always
>> be disabled, both using DOMU_DIRECT_MAP, or editing boot.source.
>
> Many thanks for the explanation. This makes sense.
>
> I think this patch can be dropped then.
>
> Xenia, apologies for suggesting you to do this in the first place.

No worries, it 's all part of the process :)

> - Ayan
>
>>
>>>> I cc Ayan, since the change was suggested by him.
>>>> I have no strong preference on the default value.
>>>>
>>>> Xenia
>>>>
>>>>>> ---
>>>>>>    README.md                | 4 ++--
>>>>>>    scripts/uboot-script-gen | 8 ++------
>>>>>>    2 files changed, 4 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/README.md b/README.md
>>>>>> index cb15ca5..03e437b 100644
>>>>>> --- a/README.md
>>>>>> +++ b/README.md
>>>>>> @@ -169,8 +169,8 @@ Where:
>>>>>>      if specified, indicates the host physical address regions
>>>>>>      [baseaddr, baseaddr + size) to be reserved to the VM for static
>>>>>> allocation.
>>>>>>    -- DOMU_DIRECT_MAP[number] can be set to 1 or 0.
>>>>>> -  If set to 1, the VM is direct mapped. The default is 1.
>>>>>> +- DOMU_DIRECT_MAP[number] if set to 1, enables direct mapping.
>>>>>> +  By default, direct mapping is disabled.
>>>>>>      This is only applicable when DOMU_STATIC_MEM is specified.
>>>>>>      - LINUX is optional but specifies the Linux kernel for when 
>>>>>> Xen is
>>>>>> NOT
>>>>>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>>>>>> index 085e29f..66ce6f7 100755
>>>>>> --- a/scripts/uboot-script-gen
>>>>>> +++ b/scripts/uboot-script-gen
>>>>>> @@ -52,7 +52,7 @@ function dt_set()
>>>>>>                echo "fdt set $path $var $array" >> $UBOOT_SOURCE
>>>>>>            elif test $data_type = "bool"
>>>>>>            then
>>>>>> -            if test "$data" -eq 1
>>>>>> +            if test "$data" == "1"
>>>>>>                then
>>>>>>                    echo "fdt set $path $var" >> $UBOOT_SOURCE
>>>>>>                fi
>>>>>> @@ -74,7 +74,7 @@ function dt_set()
>>>>>>                fdtput $FDTEDIT -p -t s $path $var $data
>>>>>>            elif test $data_type = "bool"
>>>>>>            then
>>>>>> -            if test "$data" -eq 1
>>>>>> +            if test "$data" == "1"
>>>>>>                then
>>>>>>                    fdtput $FDTEDIT -p $path $var
>>>>>>                fi
>>>>>> @@ -491,10 +491,6 @@ function xen_config()
>>>>>>            then
>>>>>>                DOMU_CMD[$i]="console=ttyAMA0"
>>>>>>            fi
>>>>>> -        if test -z "${DOMU_DIRECT_MAP[$i]}"
>>>>>> -        then
>>>>>> -             DOMU_DIRECT_MAP[$i]=1
>>>>>> -        fi
>>>>>>            i=$(( $i + 1 ))
>>>>>>        done
>>>>>>    }
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>