On 19.06.2023 18:44, Julien Grall wrote: > 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.
Just one remark here for consideration: Grant v2's sub-page grants use a 16-bit length and page offset. There would be a corner case which isn't representable (offset 0 length 64k). Jan
