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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


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