Re: [PATCH v4 2/4] xen: Add files needed for minimal ppc64le build

[Julien Grall <julien@xxxxxxx>]

Hi,

On 19/06/2023 17:25, Andrew Cooper wrote:
> On 19/06/2023 4:49 pm, Shawn Anastasio wrote:
> > On 6/16/23 3:24 PM, Andrew Cooper wrote:
> > > On 16/06/2023 6:48 pm, Shawn Anastasio wrote:
> > > > Add the build system changes required to build for ppc64le (POWER8+).
> > > > As of now the resulting image simply boots to an infinite loop.
> > > >
> > > > $ make XEN_TARGET_ARCH=ppc64 -C xen openpower_defconfig
> > > > $ make XEN_TARGET_ARCH=ppc64 SUBSYSTEMS=xen -C xen build
> > > I think the first of these isn't needed, given the config ARCH_DEFCONFIG
> > > default.  I'd suggest dropping it.
> > It seems like the build system expects an `$(ARCH)_defconfig` present if
> > you don't manually specify a defconfig target. I see riscv64 has a
> > tiny64_defconfig and a riscv64_defconfig that are idential, probably for
> > this same reason. Would you like me to take the same approach of
> > duplicating openpower_defconfig to ppc64_defconfig?
> Or just rename openpower_defconfig to ppc64_defconfig ?
>
> Is there any reason to keep openpower differently?
>
> > > If that's not feasible, then fine, but if it is, it ought to be the
> > > default.  Which might be as simple as saying "or later" somewhere in
> > > this text, or might be a giant can of worms that I shouldn't open...
> > Originally the help text for the two ISA config options ended in a "+"
> > but that was deemed ambiguous. Would adding "or later" to the help text
> > for the two options clarify it sufficiently?
> Yeah, that would definitely help.
>
> > > > diff --git a/xen/arch/ppc/include/asm/page-bits.h 
> > > > b/xen/arch/ppc/include/asm/page-bits.h
> > > > new file mode 100644
> > > > index 0000000000..4c01bf9716
> > > > --- /dev/null
> > > > +++ b/xen/arch/ppc/include/asm/page-bits.h
> > > > @@ -0,0 +1,7 @@
> > > > +#ifndef __PPC_PAGE_BITS_H__
> > > > +#define __PPC_PAGE_BITS_H__
> > > > +
> > > > +#define PAGE_SHIFT              16 /* 64 KiB Pages */
> > > > +#define PADDR_BITS              48
> > > Is 64k the minimum granularity?  Or is 4k an option?
> > Both 4K and 64K are supported by the hardware.
> >
> > > I ask because Xen has some very short sighted ABIs which we're still
> > > working on removing.  There are still quite a few expectations of
> > > PAGE_SHIFT being 12.
> > >
> > > To be clear, we're looking to fix all of these ABIs, but I suspect it
> > > will be an easier lift to begin with at 4k.  (Or perhaps the right thing
> > > is to double down and just get them fixed.)
> > Interesting. Given this I'm inclined to go with 4k just to reduce pain
> > points during initial bring up, though supporting 64k would definitely
> > be desirable going forward.
> Maybe keep as 64k for now, with 4k as a backup if major problems are
> encountered?
>
> I honestly don't know how much of Xen's common code is non-4k-clean, and
> this is the best opportunity to find out.
The hypervisor itself is probably alright. For the tools and the kernel, 
we did some work a few years ago so the code don't assume the kernel and 
the hypervisor are using the same page size.
The tools and kernel have hardcoded expectation for the tools. Have a 
look at XC_PAGE_SIZE (tools) and XEN_PAGE_SHIFT (linux).
There was an attempt from Costin Lupu a couple of years ago to clean-up 
the definition (see [1]) but this was never merged. I can't remember why...
Now regarding the default page-size for PPC. At the moment, the 
page-size of the ABI is tie to the hypervisor.
For the ABI, it is best to use the bigger size (i.e. 64KB) because with 
just some rework in the hypervisor, you could run the same guest image 
on both a 4KB and 64KB.
Therefore, I think the 64KB size for the hypervisor is probably the 
better choice for the initial port. This will avoid some external ABI 
issue in the future if you want to support more page-size (we have the 
problem on Arm as we started with 4KB). You would also not rely on a 
newer ABI.
Cheers,

[1] 
https://lore.kernel.org/xen-devel/cover.1628519855.git.costin.lupu@xxxxxxxxx/
--
Julien Grall