Re: [PATCH 1/4] guestfs: Specify host architecture to virt-builder

[Luis Chamberlain <mcgrof@kernel.org>]

On Tue, Mar 5, 2024 at 12:47 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> On Tue, Mar 05, 2024 at 12:29:29PM -0800, Luis Chamberlain wrote:
> > On Tue, Mar 05, 2024 at 03:22:35PM -0500, Chuck Lever wrote:
> > > On Tue, Mar 05, 2024 at 12:11:03PM -0800, Luis Chamberlain wrote:
> > > > On Tue, Mar 05, 2024 at 11:36:20AM -0500, Chuck Lever wrote:
> > > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > >
> > > > > According to documentation, the default architecture for
> > > > > virt-builder isn't the same as the host's platform architecture.
> > > > > To support platforms other than x86, specify the host's
> > > > > architecture explicitly on the command line.
> > > > >
> > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > > ---
> > > > >  scripts/bringup_guestfs.sh |    2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh
> > > > > index 34ad48cbe81f..51b5a07218ac 100755
> > > > > --- a/scripts/bringup_guestfs.sh
> > > > > +++ b/scripts/bringup_guestfs.sh
> > > > > @@ -67,7 +67,7 @@ _EOT
> > > > >         fi
> > > > >
> > > > >         echo "Generating new base image for ${OS_VERSION}"
> > > > > -       virt-builder ${OS_VERSION} -o $BASE_IMAGE --size 20G --format raw --commands-from-file $cmdfile
> > > > > +       virt-builder ${OS_VERSION} --arch `uname -m` -o $BASE_IMAGE --size 20G --format raw --commands-from-file $cmdfile
> > > >
> > > > Our current kconfig allows target architecture to be different, the
> > > > above seems to assume it matches the host arch so there is that
> > > > disrepancy in this patch. It would break the flexibility we are
> > > > providing.
> > > >
> > > > Although your commit 44f7e0515dac3c0e ("Kconfig: Remove unused Kconfig
> > > > option") removed TARGET_ARCH, here is an example where that could have
> > > > been used.
> > > >
> > > > config TARGET_ARCH
> > > >   string
> > > >   default "x86_64" if TARGET_ARCH_X86_64
> > > >   default "ppc64le" if TARGET_ARCH_PPC64LE
> > > >
> > > > Then the above would use ${CONFIG_TARGET_ARCH}
> > > >
> > > > A futher consideration is to add a new kconfig option which is disabled
> > > > by default (the default for all kconfig options are to be disabled by
> > > > default) which would *not* make it possible to override the target arch,
> > > > only if that bool would be enabled  would we allow the user to override
> > > > it. We want to discourage this as the performance would suck and the
> > > > results may vary as well.
> > >
> > > Then I'm a bit confused. You said before that the TARGET_ARCHITECTURE
> > > setting is supposed to select the set of tests to run, not the ISA.
> >
> > Sure, but when using virtualization it makes sense to match these since
> > if you break away from it your host CPU will only be able to emulate and
> > that's slow.
>
> Agreed: the "--arch" setting should match the host's ISA in most
> normal cases. That is the only behavior allowed by this patch, but
> I can make it settable, as you suggest.

I don't care for it to be settable myself as I realize it is insane to
do otherwise. I was just pointing out that today we do allow the
target arch to be modified by the user if using virtualization.

> > For cloud solutions the target arch can be anything and that should just
> > work fine.
>
> As far as I can tell, this patch changes only the behavior of
> guestfs-created guests. That excludes cloud environments, right?
>
> It's always possible I'm missing something.

Sure, I'm just saying if we modify the uname -m to be extracted via a
kconfig option it may be an option in kconfigs/arch/Kconfig which is
shared with cloud.

  Luis