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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

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@xxxxxxxxx>
> > > > 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