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

[Jan Beulich <jbeulich@xxxxxxxx>]

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