Re: [PATCH v3 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Luca Fancellu]

Hi Ayan,

> On 10 Oct 2024, at 15:03, Ayan Kumar Halder  wrote:
> 
> Secondary cpus initialization is not yet supported. Thus, we print an
> appropriate message and put the secondary cpus in WFE state.
> And we introduce to BUILD_BUG_ON to prevent users using from building Xen
> on multiprocessor based MPU systems.
> 
> In Arm, there is no clean way to disable SMP. As of now, we wish to support
> MPU on UNP only. So, we have defined the default range of NR_CPUs to be 1 for
> MPU.
> 
> Signed-off-by: Ayan Kumar Halder 

Apart from Jan’s comment, the rest of the changes to the arch/arm part looks ok 
to me:
Reviewed-by: Luca Fancellu 

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Ayan,

> /*
>  * Maps the various sections of Xen (described in xen.lds.S) as different MPU
>  * regions.
> @@ -68,10 +92,11 @@
>  * Inputs:
>  *   lr : Address to return to.
>  *
> - * Clobbers x0 - x5
> + * Clobbers x0 - x6
>  *
>  */
> FUNC(enable_boot_cpu_mm)
> +mov   x6, lr
> 
> /* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> mrs   x5, MPUIR_EL2
> @@ -113,7 +138,7 @@ FUNC(enable_boot_cpu_mm)
> beq 5f
> prepare_xen_region x0, x1, x2, x3, x4, x5
> 
> -5:
> +5:  mov   lr, x6

Shall these changes to enable_boot_cpu_mm be part of the previous commit?

The rest looks good to me:
Reviewed-by: Luca Fancellu 

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Ayan,

>>> + */
>>> +FUNC_LOCAL(enable_mpu)
>>> +mrs   x0, SCTLR_EL2
>>> +bic   x0, x0, #SCTLR_ELx_BR   /* Disable Background region */
>>> +orr   x0, x0, #SCTLR_Axx_ELx_M/* Enable MPU */
>>> +orr   x0, x0, #SCTLR_Axx_ELx_C/* Enable D-cache */
>>> +orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
>> 
>> NIT: Can't we have a single "orr" instruction to set all the flags?
> Yes, I will change this.
>> 
>>> +dsb   sy
>> 
>> I still question this use of "dsb sy"...
> 
> Actually I kept this to ensure that all outstanding memory access are 
> completed before MPU is enabled.
> 
> However, prepare_xen_region() is invoked before this and it has a 'dsb sy' at 
> the end.
> 
> So we can drop this barrier.

I suggest to keep the barrier here and drop the one in prepare_xen_region 
instead,
have a look in my branch: 
https://gitlab.com/xen-project/xen/-/merge_requests/7/diffs?commit_id=f42a2816f9bd95f2f6957887be910cb9acd140b5

During my testing I was having trouble without this barrier.

Cheers,
Luca

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Julien,

> On 22 Oct 2024, at 14:13, Julien Grall  wrote:
> 
> 
> 
> On 22/10/2024 10:56, Luca Fancellu wrote:
>>> On 22 Oct 2024, at 10:47, Julien Grall  wrote:
>>> 
>>> Hi Luca,
>>> 
>>> On 22/10/2024 10:41, Luca Fancellu wrote:
>>>>> On 21 Oct 2024, at 17:27, Julien Grall  wrote:
>>>> 2) dsb+isb barrier after enabling the MPU, since we are enabling the MPU 
>>>> but also because we are disabling the background region
>>> 
>>> TBH, I don't understand this one. Why would disabling the background region 
>>> requires a dsb + isb? Do you have any pointer in the Armv8-R specification?
>> I’m afraid this is only my deduction, Section C1.4 of the Arm® Architecture 
>> Reference Manual Supplement Armv8, for R-profile AArch64 architecture,
>> (DDI 0600B.a) explains what is the background region, it says it implements 
>> physical address range(s), so when we disable it, we would like any data
>> access to complete before changing this implementation defined range with 
>> the ranges defined by us tweaking PRBAR/PRLAR, am I right?
> 
> My mental model for the ordering is similar to a TLB flush which is:
>   1/ dsb nsh
>   2/ tlbi
>   3/ dsb nsh
>   4/ isb
> 
> Enabling the MPU is effectively 2. AFAIK, 1 is only necessary to ensure the 
> update to the page-tables. But it is not a requirement to ensure any data 
> access are completed (otherwise, we would have needed a dsb sy because we 
> don't know how far the access are going...).

Uhm… I’m not sure we are on the same page, probably I explained that wrongly, 
so my point is that:

FUNC_LOCAL(enable_mpu)
mrs   x0, SCTLR_EL2
bic   x0, x0, #SCTLR_ELx_BR   /* Disable Background region */
orr   x0, x0, #SCTLR_Axx_ELx_M/* Enable MPU */
orr   x0, x0, #SCTLR_Axx_ELx_C/* Enable D-cache */
orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
dsb   sy
^— This data barrier is needed in order to complete any data access, which 
guarantees that all explicit memory accesses before
   this instruction complete, so we can safely turn ON the MPU and 
disable the background region.
msr   SCTLR_EL2, x0
isb

ret
END(enable_mpu)

I didn’t understand if your point is that it is not needed or if it is needed 
but in a different location.

> 
> Cheers,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Julien,

> On 21 Oct 2024, at 17:27, Julien Grall  wrote:
> 
> 
> 
> On 21/10/2024 17:24, Luca Fancellu wrote:
>> Hi Ayan,
>>>>> + */
>>>>> +FUNC_LOCAL(enable_mpu)
>>>>> +mrs   x0, SCTLR_EL2
>>>>> +bic   x0, x0, #SCTLR_ELx_BR   /* Disable Background region */
>>>>> +orr   x0, x0, #SCTLR_Axx_ELx_M/* Enable MPU */
>>>>> +orr   x0, x0, #SCTLR_Axx_ELx_C/* Enable D-cache */
>>>>> +orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
>>>> 
>>>> NIT: Can't we have a single "orr" instruction to set all the flags?
>>> Yes, I will change this.
>>>> 
>>>>> +dsb   sy
>>>> 
>>>> I still question this use of "dsb sy"...
>>> 
>>> Actually I kept this to ensure that all outstanding memory access are 
>>> completed before MPU is enabled.
>>> 
>>> However, prepare_xen_region() is invoked before this and it has a 'dsb sy' 
>>> at the end.
>>> 
>>> So we can drop this barrier.
>> I suggest to keep the barrier here and drop the one in prepare_xen_region 
>> instead,
> 
> I think the barriers in prepare_xen_region() should be kept because we may 
> want to use the helper *after* the MPU is turned on.

Sure I agree, given that the current code was only called before enabling the 
MPU I was ok to drop the barrier in prepare_xen_region,
but given that the macro could be reused, it’s safer to keep them both.

> 
>> have a look in my branch: 
>> https://gitlab.com/xen-project/xen/-/merge_requests/7/diffs?commit_id=f42a2816f9bd95f2f6957887be910cb9acd140b5
>> During my testing I was having trouble without this barrier.
> 
> Was it before or after removing the barriers in prepare_xen_region()? If the 
> latter, then I am a bit puzzled why you need it. Did you investigate a bit 
> more?

I don’t recall unfortunately, I tried to reproduce the issue removing the 
barrier from enable_mpu and adding it into prepare_xen_region only but it’s 
working fine
or at least I’m not able to reproduce the issue I was having, of course I 
wouldn’t remove it from both since it goes against the arm arm, so I think the 
logic
here should be keeping both the barriers:

1) dsb+isb barrier after writing prbar/prlar as the arm arm recommends (in case 
the macro is used with MPU enabled in the future)
2) dsb+isb barrier after enabling the MPU, since we are enabling the MPU but 
also because we are disabling the background region

Cheers,
Luca

Re: [PATCH v3 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Luca Fancellu]

+ Frediano for suggestion about header protection define name

> +++ b/xen/arch/arm/include/asm/mpu/layout.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_MPU_LAYOUT_H__
> +#define __ARM_MPU_LAYOUT_H__

I think we are moving away from this notation:
https://patchwork.kernel.org/project/xen-devel/patch/20241004081713.749031-6-frediano.zig...@cloud.com/
Shall this be ASM___ARM__MPU__LAYOUT_H ? @Frediano


> +
> +#define XEN_START_ADDRESS CONFIG_XEN_START_ADDRESS
> +
> +/*
> + * All MPU platforms need to provide a XEN_START_ADDRESS for linker. This
> + * address indicates where Xen image will be loaded and run from. This
> + * address must be aligned to a PAGE_SIZE.
> + */
> +#if (XEN_START_ADDRESS % PAGE_SIZE) != 0
> +#error "XEN_START_ADDRESS must be aligned to 4KB"
> +#endif
> +
> +/*
> + * For MPU, XEN's virtual start address is same as the physical address.
> + * The reason being MPU treats VA == PA. IOW, it cannot map the physical
> + * address to a different fixed virtual address. So, the virtual start
> + * address is determined by the physical address at which Xen is loaded.
> + */
> +#define XEN_VIRT_START _AT(paddr_t, XEN_START_ADDRESS)
> +
> +#endif /* __ARM_MPU_LAYOUT_H__ */

       ^-- ASM___ARM__MPU__LAYOUT_H ? 

Apart from that, the rest looks ok to me:
Reviewed-by: Luca Fancellu 

Re: [PATCH v3 2/6] xen/arm: mpu: Introduce choice between MMU and MPU

[Luca Fancellu]

Hi Ayan,

> On 10 Oct 2024, at 15:03, Ayan Kumar Halder  wrote:
> 
> There are features in the forthcoming patches which are dependent on
> MPU. For eg fixed start address.
> Also, some of the Xen features (eg STATIC_MEMORY) will be selected
> by the MPU configuration.
> 
> Thus, this patch introduces a choice between MMU and MPU for the type
> of memory management system. By default, MMU is selected.
> MPU is now gated by UNSUPPORTED.
> 
> Update SUPPORT.md to state that the support for MPU is experimental.

Maybe I’m wrong, but shouldn’t we add this only when MPU is supported?
In this case we are just having the Kconfig setting.

Apart from that, the rest looks good to me:

Reviewed-by: Luca Fancellu 

Re: [PATCH v3 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Luca Fancellu]

Hi Ayan,

> On 10 Oct 2024, at 15:03, Ayan Kumar Halder  wrote:
> 
> If the BSS section is empty, then the function can just return.
> 
> Signed-off-by: Ayan Kumar Halder 

Reviewed-by: Luca Fancellu mailto:luca.fance...@arm.com>>

Re: [PATCH v3 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,


> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 00..4a21bc815c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include 
> +#include 
> +
> +#define REGION_TEXT_PRBAR   0x38/* SH=11 AP=10 XN=00 */
> +#define REGION_RO_PRBAR 0x3A/* SH=11 AP=10 XN=10 */

NIT alignment

> +#define REGION_DATA_PRBAR   0x32/* SH=11 AP=00 XN=10 */
> +
> +#define REGION_NORMAL_PRLAR 0x0f/* NS=0 ATTR=111 EN=1 */
> +
> +/*
> + * Macro to prepare and set a EL2 MPU memory region.
> + * We will also create an according MPU memory region entry, which
> + * is a structure of pr_t,  in table \prmap.
> + *
> + * Inputs:
> + * sel: region selector
> + * base:reg storing base address (should be page-aligned)
> + * limit:   reg storing limit address
> + * prbar:   store computed PRBAR_EL2 value
> + * prlar:   store computed PRLAR_EL2 value
> + * maxcount:maximum number of EL2 regions supported
> + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it 
> will be
> + *  REGION_DATA_PRBAR
> + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it 
> will be
> + *  REGION_NORMAL_PRLAR
> + */
> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, 
> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> +
> +/* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +add   \sel, \sel, #1

I think there is an issue adding 1 here, because the very first region we are 
going to fill will be the 1st even if we intended the 0th.
Probably moving this one at the end will fix the issue

> +cmp   \sel, \maxcount
> +bgt   fail
> +
> +/* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> +and   \base, \base, #MPU_REGION_MASK
> +mov   \prbar, #\attr_prbar
> +orr   \prbar, \prbar, \base
> +
> +/* Limit address should be inclusive */
> +sub   \limit, \limit, #1
> +and   \limit, \limit, #MPU_REGION_MASK
> +mov   \prlar, #\attr_prlar
> +orr   \prlar, \prlar, \limit
> +
> +msr   PRSELR_EL2, \sel
> +isb
> +msr   PRBAR_EL2, \prbar
> +msr   PRLAR_EL2, \prlar
> +dsb   sy
> +isb
> +.endm
> +
> +/* Load the physical address of a symbol into xb */
> +.macro load_paddr xb, sym
> +ldr \xb, =\sym
> +add \xb, \xb, x20   /* x20 - Phys offset */
> +.endm
> +
> +/*
> + * Maps the various sections of Xen (described in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Inputs:
> + *   lr : Address to return to.
> + *
> + * Clobbers x0 - x5
> + *
> + */
> +FUNC(enable_boot_cpu_mm)
> +
> +/* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +mrs   x5, MPUIR_EL2
> +
> +/* x0: region sel */
> +mov   x0, xzr
> +/* Xen text section. */
> +load_paddr x1, _stext
> +load_paddr x2, _etext
> +cmp x1, x2
> +beq 1f
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +1:  /* Xen read-only data section. */
> +load_paddr x1, _srodata
> +load_paddr x2, _erodata
> +cmp x1, x2
> +beq 2f
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +2:  /* Xen read-only after init and data section. (RW data) */
> +load_paddr x1, __ro_after_init_start
> +load_paddr x2, __init_begin
> +cmp x1, x2
> +beq 3f
> +prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +3:  /* Xen code section. */
> +load_paddr x1, __init_begin
> +load_paddr x2, __init_data_begin
> +cmp x1, x2
> +beq 4f
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +4:  /* Xen data and BSS section. */
> +load_paddr x1, __init_data_begin
> +load_paddr x2, __bss_end
> +cmp x1, x2
> +beq 5f
> +prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +5:
> +ret
> +
> +fail:
> +PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
> +wfe
> +b   1b
> +END(enable_boot_cpu_mm)
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h 
> b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> new file mode 100644
> index 00..b0c31a58ec
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_ARM_ARM64_MPU_SYSREGS_H
> +#define __ASM_ARM_ARM64_MPU_SYSREGS_H

Same comment about define name as in patch 3, here and in every
new file of this patch

Cheers,
Luca

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Luca Fancellu]

Hi Jan,

> On 29 Oct 2024, at 08:08, Jan Beulich  wrote:
> 
> On 28.10.2024 18:38, Ayan Kumar Halder wrote:
>> On 28/10/2024 15:01, Jan Beulich wrote:
>>> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
 On 28/10/2024 12:55, Jan Beulich wrote:
> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>> --- a/xen/arch/Kconfig
>> +++ b/xen/arch/Kconfig
>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>> 
>>   config NR_CPUS
>>  int "Maximum number of CPUs"
>> +range 1 1 if ARM && MPU
>>  range 1 16383
>>  default "256" if X86
>>  default "8" if ARM && RCAR3
>>  default "4" if ARM && QEMU
>>  default "4" if ARM && MPSOC
>> +default "1" if ARM && MPU
>>  default "128" if ARM
>>  help
>>Controls the build-time size of various arrays and bitmaps
> I'm afraid I can't easily tell whether MPU can be used together with any 
> of
> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to move
> up, as it's the first one that has a match on its condition which is being
> used.
 MPU cannot be used with any of the existing platforms.
>>> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
>>> don't go together?
>> 
>> Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
>> aware, there is no support for Aarch64 MPU in Qemu (ie R82).
>> 
>> Even for R52, I could not get the upstream Qemu working (emulating some 
>> Arm reference platform).
>> 
>> I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
>> working which emulates AMD's SoC using R52.
>> 
>> However, this should not impact the current patch. There is no Qemu in 
>> xen/arch/arm/platforms/*.
> 
> Aiui that's not relevant. There is a QEMU item in 
> xen/arch/arm/platforms/Kconfig.
> I continue to fail to see why that couldn't be selected together with MPU. 
> Yet if
> it can be, you'd end up with a default of 4, not 1, if it actually _is_ 
> selected.
> Alternatively QEMU (and maybe also RCAR3 and MPSOC) need to be mutually 
> exclusive
> with MPU. Hmm, looks like that's already the case, by patch 2 suppressing the
> "Platform Support" prompt. While that looks fragile to me, I'm sorry for the
> noise then.

Are you suggesting to move "default "1" if ARM && MPU” right after “default 
"256" if X86”?

Cheers,
Luca

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

> On 22 Oct 2024, at 10:47, Julien Grall  wrote:
> 
> Hi Luca,
> 
> On 22/10/2024 10:41, Luca Fancellu wrote:
>>> On 21 Oct 2024, at 17:27, Julien Grall  wrote:
>> 2) dsb+isb barrier after enabling the MPU, since we are enabling the MPU but 
>> also because we are disabling the background region
> 
> TBH, I don't understand this one. Why would disabling the background region 
> requires a dsb + isb? Do you have any pointer in the Armv8-R specification?

I’m afraid this is only my deduction, Section C1.4 of the Arm® Architecture 
Reference Manual Supplement Armv8, for R-profile AArch64 architecture,
(DDI 0600B.a) explains what is the background region, it says it implements 
physical address range(s), so when we disable it, we would like any data
access to complete before changing this implementation defined range with the 
ranges defined by us tweaking PRBAR/PRLAR, am I right?


> 
> Cheers,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v3 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Ayan,

> On 24 Oct 2024, at 09:02, Ayan Kumar Halder  wrote:
> 
> Hi Julien,
> 
> On 23/10/2024 17:30, Julien Grall wrote:
>> 
>> 
>> On 23/10/2024 17:18, Julien Grall wrote:
>>> 
>>> 
>>> On 23/10/2024 17:13, Julien Grall wrote:
>>>> 
>>>> 
>>>> On 23/10/2024 17:06, Ayan Kumar Halder wrote:
>>>>> Hi Luca/Julien,
>>>>> 
>>>>> On 22/10/2024 17:31, Luca Fancellu wrote:
>>>>>> Hi Julien,
>>>>>> 
>>>>>>> On 22 Oct 2024, at 14:13, Julien Grall  wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 22/10/2024 10:56, Luca Fancellu wrote:
>>>>>>>>> On 22 Oct 2024, at 10:47, Julien Grall  wrote:
>>>>>>>>> 
>>>>>>>>> Hi Luca,
>>>>>>>>> 
>>>>>>>>> On 22/10/2024 10:41, Luca Fancellu wrote:
>>>>>>>>>>> On 21 Oct 2024, at 17:27, Julien Grall  wrote:
>>>>>>>>>> 2) dsb+isb barrier after enabling the MPU, since we are enabling the 
>>>>>>>>>> MPU but also because we are disabling the background region
>>>>>>>>> TBH, I don't understand this one. Why would disabling the background 
>>>>>>>>> region requires a dsb + isb? Do you have any pointer in the Armv8-R 
>>>>>>>>> specification?
>>>>>>>> I’m afraid this is only my deduction, Section C1.4 of the Arm® 
>>>>>>>> Architecture Reference Manual Supplement Armv8, for R-profile AArch64 
>>>>>>>> architecture,
>>>>>>>> (DDI 0600B.a) explains what is the background region, it says it 
>>>>>>>> implements physical address range(s), so when we disable it, we would 
>>>>>>>> like any data
>>>>>>>> access to complete before changing this implementation defined range 
>>>>>>>> with the ranges defined by us tweaking PRBAR/PRLAR, am I right?
>>>>>>> My mental model for the ordering is similar to a TLB flush which is:
>>>>>>>1/ dsb nsh
>>>>>>>2/ tlbi
>>>>>>>3/ dsb nsh
>>>>>>>4/ isb
>>>>>>> 
>>>>>>> Enabling the MPU is effectively 2. AFAIK, 1 is only necessary to ensure 
>>>>>>> the update to the page-tables. But it is not a requirement to ensure 
>>>>>>> any data access are completed (otherwise, we would have needed a dsb sy 
>>>>>>> because we don't know how far the access are going...).
>>>>>> Uhm… I’m not sure we are on the same page, probably I explained that 
>>>>>> wrongly, so my point is that:
>>>>>> 
>>>>>> FUNC_LOCAL(enable_mpu)
>>>>>>  mrs   x0, SCTLR_EL2
>>>>>>  bic   x0, x0, #SCTLR_ELx_BR   /* Disable Background region */
>>>>>>  orr   x0, x0, #SCTLR_Axx_ELx_M/* Enable MPU */
>>>>>>  orr   x0, x0, #SCTLR_Axx_ELx_C/* Enable D-cache */
>>>>>>  orr   x0, x0, #SCTLR_Axx_ELx_WXN  /* Enable WXN */
>>>>>>  dsb   sy
>>>>>>  ^— This data barrier is needed in order to complete any data 
>>>>>> access, which guarantees that all explicit memory accesses before
>>>>>> this instruction complete, so we can safely turn ON the MPU 
>>>>>> and disable the background region.
>>>> 
>>>> I think
>>> 
>>> Sorry I fat fingered and pressed send too early. I think this is the part I 
>>> disagree with. All explicit accesses don't need to be complete (in the 
>>> sense observed by everyone in the system). They only need to have gone 
>>> through the permissions check.
>> 
>> I think I managed to find again the wording that would justify why a "dsb" 
>> is not necessary for the permission checks. From ARM DDI 0487K.a D23-7349:
>> 
>> ```
>> Direct writes using the instructions in Table D22-2 require synchronization 
>> before software can rely on the effects
>> of changes to the System registers to affect instructions appearing in 
>> program order after the direct write to the
>> System register. Direct writes to these registers are not allowed to affect 
>> any instructions appearing in program order
>> before the direct write.
>> ```
>> 
>> I understand the paragraph as enabling the MPU via SCTLR_EL2 will not affect 
>> any instruction before hand.
> 
> Yes, I agree.
> 
> However, as the line states
> 
> "Direct writes using the instructions in Table D22-2 require synchronization 
> before software can rely on the effects"
> 
> This means synchronization is required after the write to SCTLR_EL2.
> 
> However, this synchronization seems to imply dsb sy + isb.
> 
> FromARM DDI 0487K.a ID032224 B2-274
> 
> "A DSB instruction ordered after a direct write to a System PMU register does 
> not complete until all observers observe the direct write"

I think this is related only to the System PMU register, not to the registers 
in table D22-2 (which SCTLR_ELx are part)

Anyway we could discuss this in person at the Xen meet-up and write a summary 
in the thread later.

Cheers,
Luca

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

I forgot another thing:


> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 00..9377ae778c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include 
 ^— This feels suspicious, this header cannot be included by an assembly 
file

Re: [PATCH v4 6/6] xen/arm: mpu: Implement a dummy enable_secondary_cpu_mm

[Luca Fancellu]

> On 29 Oct 2024, at 09:41, Jan Beulich  wrote:
> 
> On 29.10.2024 10:30, Luca Fancellu wrote:
>> Hi Jan,
>> 
>>> On 29 Oct 2024, at 08:08, Jan Beulich  wrote:
>>> 
>>> On 28.10.2024 18:38, Ayan Kumar Halder wrote:
>>>> On 28/10/2024 15:01, Jan Beulich wrote:
>>>>> On 28.10.2024 15:39, Ayan Kumar Halder wrote:
>>>>>> On 28/10/2024 12:55, Jan Beulich wrote:
>>>>>>> On 28.10.2024 13:45, Ayan Kumar Halder wrote:
>>>>>>>> --- a/xen/arch/Kconfig
>>>>>>>> +++ b/xen/arch/Kconfig
>>>>>>>> @@ -6,11 +6,13 @@ config PHYS_ADDR_T_32
>>>>>>>> 
>>>>>>>>  config NR_CPUS
>>>>>>>>int "Maximum number of CPUs"
>>>>>>>> +  range 1 1 if ARM && MPU
>>>>>>>>range 1 16383
>>>>>>>>default "256" if X86
>>>>>>>>default "8" if ARM && RCAR3
>>>>>>>>default "4" if ARM && QEMU
>>>>>>>>default "4" if ARM && MPSOC
>>>>>>>> +  default "1" if ARM && MPU
>>>>>>>>default "128" if ARM
>>>>>>>>help
>>>>>>>>  Controls the build-time size of various arrays and bitmaps
>>>>>>> I'm afraid I can't easily tell whether MPU can be used together with 
>>>>>>> any of
>>>>>>> RCAR3, QEMU, or MPSOC. If it can, the new default line would need to 
>>>>>>> move
>>>>>>> up, as it's the first one that has a match on its condition which is 
>>>>>>> being
>>>>>>> used.
>>>>>> MPU cannot be used with any of the existing platforms.
>>>>> That is - qemu can't emulate such an environment, i.e. even QEMU and MPU
>>>>> don't go together?
>>>> 
>>>> Qemu has support for Aarch32 MPU at EL2 and EL1 (ie R52). As far as I am 
>>>> aware, there is no support for Aarch64 MPU in Qemu (ie R82).
>>>> 
>>>> Even for R52, I could not get the upstream Qemu working (emulating some 
>>>> Arm reference platform).
>>>> 
>>>> I could get the Xilinx fork of Qemu (https://github.com/Xilinx/qemu) 
>>>> working which emulates AMD's SoC using R52.
>>>> 
>>>> However, this should not impact the current patch. There is no Qemu in 
>>>> xen/arch/arm/platforms/*.
>>> 
>>> Aiui that's not relevant. There is a QEMU item in 
>>> xen/arch/arm/platforms/Kconfig.
>>> I continue to fail to see why that couldn't be selected together with MPU. 
>>> Yet if
>>> it can be, you'd end up with a default of 4, not 1, if it actually _is_ 
>>> selected.
>>> Alternatively QEMU (and maybe also RCAR3 and MPSOC) need to be mutually 
>>> exclusive
>>> with MPU. Hmm, looks like that's already the case, by patch 2 suppressing 
>>> the
>>> "Platform Support" prompt. While that looks fragile to me, I'm sorry for the
>>> noise then.
>> 
>> Are you suggesting to move "default "1" if ARM && MPU” right after “default 
>> "256" if X86”?
> 
> Yes.

Makes sense! 

With that:

Reviewed-by: Luca Fancellu 

> 
> Jan

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

While I rebased the branch on top of your patches, I saw you’ve changed the 
number of regions
mapped at boot time, can I ask why? 

Compared to 
https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-penny.zh...@arm.com/:

> +FUNC(enable_boot_cpu_mm)
> +
> +/* Get the number of regions specified in MPUIR_EL2 */
> +mrs   x5, MPUIR_EL2
> +
> +/* x0: region sel */
> +mov   x0, xzr
> +/* Xen text section. */
> +ldr   x1, =_stext
> +ldr   x2, =_etext
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +/* Xen read-only data section. */
> +ldr   x1, =_srodata
> +ldr   x2, =_erodata
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +/* Xen read-only after init and data section. (RW data) */
> +ldr   x1, =__ro_after_init_start
> +ldr   x2, =__init_begin
> +prepare_xen_region x0, x1, x2, x3, x4, x5

 ^— this, for example, will block Xen to call init_done(void) later, I 
understand this is earlyboot,
   but I guess we don’t want to make subsequent changes to this 
part when introducing the
   patches to support start_xen()

> +
> +/* Xen code section. */
> +ldr   x1, =__init_begin
> +ldr   x2, =__init_data_begin
> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +/* Xen data and BSS section. */
> +ldr   x1, =__init_data_begin
> +ldr   x2, =__bss_end
> +prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +ret
> +
> +END(enable_boot_cpu_mm)

I suggest to keep exactly the regions as are in Penny’s patch.

Cheers,
Luca

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Julien,

> On 30 Oct 2024, at 10:32, Julien Grall  wrote:
> 
> On 30/10/2024 10:08, Luca Fancellu wrote:
>> Hi Julien,
>>> On 30 Oct 2024, at 09:52, Julien Grall  wrote:
>>> 
>>> On Wed, 30 Oct 2024 at 09:17, Luca Fancellu  wrote:
>>>> 
>>>> Hi Ayan,
>>>> 
>>>> While I rebased the branch on top of your patches, I saw you’ve changed 
>>>> the number of regions
>>>> mapped at boot time, can I ask why?
>>> 
>>> I have asked the change. If you look at the layout...
>> Apologies, I didn’t see you’ve asked the change
> 
> No need to apologies! I think I asked a few revisions ago.
> 
>>> 
>>>> 
>>>> Compared to 
>>>> https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-penny.zh...@arm.com/:
>>> 
>>> 
>>> ... you have two sections with the same permissions:
>>> 
>>> xen_mpumap[1] : Xen read-only data
>>> xen_mpumap[2] : Xen read-only after init data
>>> xen_mpumap[3] : Xen read-write data
>>> 
>>> During boot, [2] and [3] will share the same permissions. After boot,
>>> this will be [1] and [2]. Given the number of MPU regions is limited,
>>> this is a bit of a waste.
>>> 
>>> We also don't want to have a hole in the middle of Xen sections. So
>>> folding seemed to be a good idea.
>>> 
>>>> 
>>>>> +FUNC(enable_boot_cpu_mm)
>>>>> +
>>>>> +/* Get the number of regions specified in MPUIR_EL2 */
>>>>> +mrs   x5, MPUIR_EL2
>>>>> +
>>>>> +/* x0: region sel */
>>>>> +mov   x0, xzr
>>>>> +/* Xen text section. */
>>>>> +ldr   x1, =_stext
>>>>> +ldr   x2, =_etext
>>>>> +prepare_xen_region x0, x1, x2, x3, x4, x5, 
>>>>> attr_prbar=REGION_TEXT_PRBAR
>>>>> +
>>>>> +/* Xen read-only data section. */
>>>>> +ldr   x1, =_srodata
>>>>> +ldr   x2, =_erodata
>>>>> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>>>>> +
>>>>> +/* Xen read-only after init and data section. (RW data) */
>>>>> +ldr   x1, =__ro_after_init_start
>>>>> +ldr   x2, =__init_begin
>>>>> +prepare_xen_region x0, x1, x2, x3, x4, x5
>>>> 
>>>> ^— this, for example, will block Xen to call init_done(void) 
>>>> later, I understand this is earlyboot,
>>>>   but I guess we don’t want to make subsequent changes to this 
>>>> part when introducing the
>>>>   patches to support start_xen()
>>> 
>>> Can you be a bit more descriptive... What will block?
>> This call in setup.c:
>> rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
>>  (unsigned long)&__ro_after_init_end,
>>  PAGE_HYPERVISOR_RO);
>> Cannot work anymore because xen_mpumap[2] is wider than only 
>> (__ro_after_init_start, __ro_after_init_end).
> 
> Is this because the implementation of modify_xen_mappings() is only able to 
> modify a full region? IOW, it would not be able to split regions and/or merge 
> them?

Yes, the code is, at the moment, not smart enough to do that, it will only 
modify a full region.

> 
>> If that is what we want, then we could wrap the above call into something 
>> MMU specific that will execute the above call and
>> something MPU specific that will modify xen_mpumap[1] from (_srodata, 
>> _erodata) to (_srodata, __ro_after_init_end)
>> and xen_mpumap[2] from (__ro_after_init_start, __init_begin) to 
>> (__ro_after_init_end, __init_begin).
> 
> I think it would make sense to have the call mmu/mpu specific. This would 
> allow to limit the number of MPU regions used by Xen itself.
> 
> The only problem is IIRC the region is not fixed because we will skip empty 
> regions during earlyboot.

Yes, but I think we can assume that X(_srodata, _erodata) and 
Y(__ro_after_init_start, __init_begin) won’t never be empty for Xen?

In that case, the call to mpumap_contain_region() should be able to retrieve 
the full region X and the partial region Y and
a specific function could modify the ranges of both given the respective 
indexes.

Code in my branch: 
https://gitlab.com/xen-project/people/lucafancellu/xen/-/blob/r82_rebased/xen/arch/arm/mpu/mm.c#L382

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Julien,

> On 30 Oct 2024, at 09:52, Julien Grall  wrote:
> 
> On Wed, 30 Oct 2024 at 09:17, Luca Fancellu  wrote:
>> 
>> Hi Ayan,
>> 
>> While I rebased the branch on top of your patches, I saw you’ve changed the 
>> number of regions
>> mapped at boot time, can I ask why?
> 
> I have asked the change. If you look at the layout...

Apologies, I didn’t see you’ve asked the change

> 
>> 
>> Compared to 
>> https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-25-penny.zh...@arm.com/:
> 
> 
> ... you have two sections with the same permissions:
> 
> xen_mpumap[1] : Xen read-only data
> xen_mpumap[2] : Xen read-only after init data
> xen_mpumap[3] : Xen read-write data
> 
> During boot, [2] and [3] will share the same permissions. After boot,
> this will be [1] and [2]. Given the number of MPU regions is limited,
> this is a bit of a waste.
> 
> We also don't want to have a hole in the middle of Xen sections. So
> folding seemed to be a good idea.
> 
>> 
>>> +FUNC(enable_boot_cpu_mm)
>>> +
>>> +/* Get the number of regions specified in MPUIR_EL2 */
>>> +mrs   x5, MPUIR_EL2
>>> +
>>> +/* x0: region sel */
>>> +mov   x0, xzr
>>> +/* Xen text section. */
>>> +ldr   x1, =_stext
>>> +ldr   x2, =_etext
>>> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
>>> +
>>> +/* Xen read-only data section. */
>>> +ldr   x1, =_srodata
>>> +ldr   x2, =_erodata
>>> +prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
>>> +
>>> +/* Xen read-only after init and data section. (RW data) */
>>> +ldr   x1, =__ro_after_init_start
>>> +ldr   x2, =__init_begin
>>> +prepare_xen_region x0, x1, x2, x3, x4, x5
>> 
>> ^— this, for example, will block Xen to call init_done(void) later, 
>> I understand this is earlyboot,
>>   but I guess we don’t want to make subsequent changes to this 
>> part when introducing the
>>   patches to support start_xen()
> 
> Can you be a bit more descriptive... What will block?

This call in setup.c:
rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
 (unsigned long)&__ro_after_init_end,
 PAGE_HYPERVISOR_RO);

Cannot work anymore because xen_mpumap[2] is wider than only 
(__ro_after_init_start, __ro_after_init_end).

If that is what we want, then we could wrap the above call into something MMU 
specific that will execute the above call and
something MPU specific that will modify xen_mpumap[1] from (_srodata, _erodata) 
to (_srodata, __ro_after_init_end)
and xen_mpumap[2] from (__ro_after_init_start, __init_begin) to 
(__ro_after_init_end, __init_begin).

Please, let me know your thoughts.

Cheers,
Luca

Re: [PATCH] CI: Fix cppcheck parallel build more

[Luca Fancellu]

Hi Andrew,

> On 31 Oct 2024, at 16:55, Andrew Cooper  wrote:
> 
> A recent cppcheck run was found to fail:
> 
>  https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/8237167472
> 
> with:
> 
>  "type mismatch! call is() before get()" && is()
>  make[3]: *** [arch/x86/boot/Makefile:28: 
> arch/x86/boot/reloc-trampoline.32.o] Error 1
> 
> This turns out to be a parallel build issue, combined with a recent change to
> x86.  Notably, we now have a case where we build both:
> 
>  CC  arch/x86/boot/reloc-trampoline.32.o
>  CC  arch/x86/boot/reloc-trampoline.o
> 
> from the same original C file, and cppcheck uses the source C file as the key
> for generating it's intermediate files.
> 
> Switch cppcheck to use the object file as the unique key instead.
> 
> Fixes: 45bfff651173 ("xen/misra: xen-analysis.py: fix parallel analysis 
> Cppcheck errors")
> Fixes: db8acf31f96b ("x86/boot: Reuse code to relocate trampoline")
> Suggested-by: Luca Fancellu 
> Signed-off-by: Andrew Cooper 
> —

Looks good to me!
I’ve also checked with and without the patch and I can’t see any regression in 
terms of cppcheck
issues report.

Reviewed-by: Luca Fancellu 
Tested-by: Luca Fancellu 

Re: [PATCH v4 5/6] xen/arm: mpu: Enable MPU

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder  wrote:
> 
> After the regions have been created, now we enable the MPU. For this we 
> disable
> the background region so that the new memory map created for the regions take
> effect. Also, we treat all RW regions as non executable and the data cache is
> enabled.
> 
> As enable_mpu() is invoked from enable_boot_cpu_mm(), one needs to save and
> restore the lr.
> 
> Signed-off-by: Ayan Kumar Halder 
> ---
> 

It looks good to me:

Reviewed-by: Luca Fancellu 

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hy Ayan,

>> +
>> +#define REGION_TEXT_PRBAR   0x38/* SH=11 AP=10 XN=00 */
>> +#define REGION_RO_PRBAR 0x3A/* SH=11 AP=10 XN=10 */
>> +#define REGION_DATA_PRBAR   0x32/* SH=11 AP=00 XN=10 */
> 
> NIT: alignment
> 
>> +
>> +#define REGION_NORMAL_PRLAR 0x0f/* NS=0 ATTR=111 EN=1 */
>> +
>> +/*
>> + * Macro to prepare and set a EL2 MPU memory region.
>> + * We will also create an according MPU memory region entry, which
>> + * is a structure of pr_t,  in table \prmap.
>> + *
>> + * Inputs:
>> + * sel: region selector
>> + * base:reg storing base address (should be page-aligned)
>> + * limit:   reg storing limit address
>> + * prbar:   store computed PRBAR_EL2 value
>> + * prlar:   store computed PRLAR_EL2 value
>> + * maxcount:maximum number of EL2 regions supported
>> + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it 
>> will be
>> + *  REGION_DATA_PRBAR
>> + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it 
>> will be
>> + *  REGION_NORMAL_PRLAR
> 
> NIT: shall we also align the text after the colon?
> 

Please forget about these comments, I’ve applied your patches and everything 
looks good in terms of alignment,
I was misled by my mail client.

Cheers,
Luca 

Re: [PATCH v4 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder  wrote:
> 
> Define enable_boot_cpu_mm() for the AArch64-V8R system.

Could you use here "Armv8-R AArch64” instead of “AArch64-V8R"

> 
> Like boot-time page table in MMU system, we need a boot-time MPU protection
> region configuration in MPU system so Xen can fetch code and data from normal
> memory.
> 
> To do this, Xen maps the following sections of the binary as separate regions
> (with permissions) :-
> 1. Text (Read only at EL2, execution is permitted)
> 2. RO data (Read only at EL2)
> 3. RO after init data and RW data (Read/Write at EL2)
> 4. Init Text (Read only at EL2, execution is permitted)
> 5. Init data and BSS (Read/Write at EL2)
> 
> Before creating a region, we check if the count exceeds the number defined in
> MPUIR_EL2. If so, then the boot fails.
> 
> Also we check if the region is empty or not. IOW, if the start and end address
> are same, we skip mapping the region.
> 
> To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
> One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
> Registers", to get the definitions of these registers. Also, refer to G1.2
> "Accessing MPU memory region registers", the following
> 
> ```
> The MPU provides two register interfaces to program the MPU regions:
> - Access to any of the MPU regions via PRSELR_ELx, PRBAR_ELx, and
> PRLAR_ELx.
> ```
> 
> We use the above mechanism to create the MPU memory regions.
> 
> MPU specific registers are defined in
> xen/arch/arm/include/asm/arm64/mpu/sysregs.h.
> 
> Signed-off-by: Ayan Kumar Halder 
> ---
> Changes from :-
> 
> v1 - 1. Instead of mapping a (XEN_START_ADDRESS + 2MB) as a single MPU region,
> we have separate MPU regions for different parts of the Xen binary. The reason
> being different regions will nned different permissions (as mentioned in the
> linker script).
> 
> 2. Introduced a label (__init_data_begin) to mark the beginning of the init 
> data
> section.
> 
> 3. Moved MPU specific register definitions to mpu/sysregs.h.
> 
> 4. Fixed coding style issues.
> 
> 5. Included page.h in mpu/head.S as page.h includes sysregs.h.
> I haven't seen sysregs.h included directly from head.S or mmu/head.S.
> (Outstanding comment not addressed).
> 
> v2 - 1. Extracted "enable_mpu()" in a separate patch.
> 
> 2. Removed alignment for limit address.
> 
> 3. Merged some of the sections for preparing the early boot regions.
> 
> 4. Checked for the max limit of MPU regions before creating a new region.
> 
> 5. Checked for empty regions.
> 
> v3 :- 1. Modified prepare_xen_region() so that we check for empty region 
> within
> this. Also, index of regions (to be programmed in PRSELR_EL2) should start 
> from
> 0.
> 
> 2. Removed load_paddr() as the offset is 0.
> 
> 3. Introduced fail_insufficient_regions() to handle failure caused when the
> number of regions to be allocated is not sufficient.
> 
> xen/arch/arm/arm64/mpu/Makefile  |   1 +
> xen/arch/arm/arm64/mpu/head.S| 122 +++
> xen/arch/arm/include/asm/arm64/mpu/sysregs.h |  27 
> xen/arch/arm/include/asm/mm.h|   2 +
> xen/arch/arm/include/asm/mpu/arm64/mm.h  |  22 
> xen/arch/arm/include/asm/mpu/mm.h|  20 +++
> xen/arch/arm/xen.lds.S   |   1 +
> 7 files changed, 195 insertions(+)
> create mode 100644 xen/arch/arm/arm64/mpu/head.S
> create mode 100644 xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> create mode 100644 xen/arch/arm/include/asm/mpu/arm64/mm.h
> create mode 100644 xen/arch/arm/include/asm/mpu/mm.h
> 
> diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
> index b18cec4836..a8a750a3d0 100644
> --- a/xen/arch/arm/arm64/mpu/Makefile
> +++ b/xen/arch/arm/arm64/mpu/Makefile
> @@ -1 +1,2 @@
> +obj-y += head.o
> obj-y += mm.o
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 00..9377ae778c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include 
> +#include 
> +
> +#define REGION_TEXT_PRBAR   0x38/* SH=11 AP=10 XN=00 */
> +#define REGION_RO_PRBAR 0x3A/* SH=11 AP=10 XN=10 */
> +#define REGION_DATA_PRBAR   0x32/* SH=11 AP=00 XN=10 */

NIT: alignment

> +
> +#define REGION_NORMAL_PRLAR 0x0f/* NS=0 ATTR=111 EN=1 */
> +
> +/*
> + * Macro to prepare and set a EL2 MPU memory region.
> + * We will also create an according MPU memory region entry, which
> + * is a structure of pr_t,  in table \prmap.
> + *
> + * Inputs:
> + * sel: region selector
> + * base:reg storing base address (should be page-aligned)
> + * limit:   reg storing limit address
> + * prbar:   store computed PRBAR_EL2 value
> + * prlar:   store computed PRLAR_EL2 value
> + * maxcount:maximum number of E

Re: [PATCH v4 1/6] xen/arm: Skip initializing the BSS section when it is empty

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder  wrote:
> 
> If the BSS section is empty, then the function should return.
> If one does not check whether the BSS section is empty or not, then there is a
> risk of writing 0s outside of BSS section (which may contain critical data).
> 
> Fixes: dac84b66cc9a ("xen: arm64: initial build + config changes, start of 
> day code")
> Signed-off-by: Ayan Kumar Halder 
> —

Looks good to me

Reviewed-by: Luca Fancellu 

Re: [PATCH v4 3/6] xen/arm: mpu: Define Xen start address for MPU systems

[Luca Fancellu]

Hi Ayan,

> On 28 Oct 2024, at 12:45, Ayan Kumar Halder  wrote:
> 
> From: Wei Chen 
> 
> On Armv8-A, Xen has a fixed virtual start address (link address too) for all
> Armv8-A platforms. In an MMU based system, Xen can map its loaded address to
> this virtual start address. So, on Armv8-A platforms, the Xen start address 
> does
> not need to be configurable. But on Armv8-R platforms, there is no MMU to map
> loaded address to a fixed virtual address and different platforms will have 
> very
> different address space layout. So Xen cannot use a fixed physical address on
> MPU based system and need to have it configurable.
> 
> So, we introduce a Kconfig option for users to set the start address. The 
> start
> address needs to be aligned to 4KB. We have a check for this alignment.
> 
> MPU allows us to define regions which are 64 bits aligned. This restriction
> comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 extended to
> provide the base and limit address of a region). This means that the start
> address of Xen needs to be at least 64 bits aligned (as it will correspond to
> the start address of memory protection region).
> 
> As for now Xen on MPU tries to use the same memory alignment restrictions as 
> it
> has been for MMU. We have added a build assertion to ensure that the page size
> is 4KB. Unlike MMU where the starting virtual address is 2MB, Xen on MPU needs
> the start address to be 4KB (ie page size) aligned.
> 
> In case if the user forgets to set the start address, then 0x is used
> as default. This is to trigger the error (on alignment check) and thereby 
> prompt
> user to set the start address.
> 
> Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is
> defined.
> 
> Signed-off-by: Wei Chen 
> Signed-off-by: Jiamei.Xie 
> Signed-off-by: Ayan Kumar Halder 
> —

Looks good to me

Reviewed-by: Luca Fancellu 

Re: [PATCH v1 2/2] xen/mmu: enable SMMU subsystem only in MMU

[Luca Fancellu]

Hi Ayan,


> On 11 Nov 2024, at 16:00, Ayan Kumar Halder  wrote:
> 
> 
> On 11/11/2024 13:45, Julien Grall wrote:
>> Hi Ayan,
> Hi Julien,
>> 
>> On 11/11/2024 13:24, Ayan Kumar Halder wrote:
>>> On 11/11/2024 11:12, Julien Grall wrote:
 Hi,
>>> Hi Julien,
 
 On 08/11/2024 19:59, Ayan Kumar Halder wrote:
> From: Penny Zheng 
> 
> In Xen, SMMU subsystem is supported for MMU system only. The reason being 
> SMMU
> driver uses the same page tables as MMU.
> Thus, we make it dependent on CONFIG_MMU.
> 
> Signed-off-by: Penny Zheng 
> Signed-off-by: Ayan Kumar Halder 
> ---
>   xen/arch/arm/Kconfig| 2 +-
>   xen/drivers/passthrough/Kconfig | 3 ++-
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 15b2e4a227..3699e148e9 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -16,7 +16,7 @@ config ARM
>   select HAS_DEVICE_TREE
>   select HAS_PASSTHROUGH
>   select HAS_UBSAN
> -select IOMMU_FORCE_PT_SHARE
> +select IOMMU_FORCE_PT_SHARE if MMU
 
 Realistically, everything under drivers/passthrough is MMU specific. So 
 does it actually make any sense to select HAS_PASSTHROUGH right now?
>>> 
>>> Actually we are able to assign devices to different DomUs (eg UART1 to 
>>> domU1) as long as the device isn't behind an IOMMU. So in our case, the 
>>> passthrough device tree has this node
>>> 
>>>  uart@9c0b {
>>>  compatible = "arm,pl011\0arm,primecell";
>>>  reg = <0x00 0x9c0b 0x00 0x1>;
>>>  interrupt-parent = <0x01>;
>>>  interrupts = <0x00 0x07 0x04>;
>>>  clock-names = "uartclk\0apb_pclk";
>>>  clocks = <0x06 0x07>;
>>>  xen,path = "/uart@9c0b";
>>>  xen,reg = <0x00 0x9c0b 0x00 0x1 0x00 0x9c0b>;
>>>  xen,force-assign-without-iommu;
>> 
>> So how devices will be protected on an MPU systems?
>> 
>> >  };> So, should we still disable HAS_PASSTHROUGH for MPU ?
>> 
>> While it may work, a lot of code in drivers/passthrough is IOMMU specific 
>> (see all the function named iommu_*). So I find really odd that you disable 
>> IOMMU_FORCE_PT_SHARE but all the rest is still present...
>> 
>> I think we need some consistency. If you are planning to do device 
>> passthrough without any protection, then I don't think you need any code 
>> within drivers/passthrough/ (at least for platform devices).
>> 
>> Overall, for this patch, I think it would be better to simply select 
>> HAS_PASSTHROUGH when MMU is enabled. We can revisit device passthrough once 
>> we have the patches on the ML.
> Yes, this makes sense. I will wait for Luca to confirm as well.

It makes sense to don’t compile all that stuff, anyway we are using some 
functions from drivers/passthrough/device_tree.c to pass the pl011 to the 
guests, we will try to handle them later in the series then.

Cheers,
Luca

Re: [PATCH v1 2/2] xen/mmu: enable SMMU subsystem only in MMU

[Luca Fancellu]

> On 8 Nov 2024, at 19:59, Ayan Kumar Halder  wrote:
> 
> From: Penny Zheng 
> 
> In Xen, SMMU subsystem is supported for MMU system only. The reason being SMMU
> driver uses the same page tables as MMU.
> Thus, we make it dependent on CONFIG_MMU.
> 
> Signed-off-by: Penny Zheng 
> Signed-off-by: Ayan Kumar Halder 
> —

Reviewed-by: Luca Fancellu 

Re: [PATCH v5 1/3] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu]

Hi Ayan,

On 7 Nov 2024, at 15:03, Ayan Kumar Halder  wrote:

Define enable_boot_cpu_mm() for the Armv8-R AArch64.

Like boot-time page table in MMU system, we need a boot-time MPU protection
region configuration in MPU system so Xen can fetch code and data from normal
memory.

To do this, Xen maps the following sections of the binary as separate regions
(with permissions) :-
1. Text (Read only at EL2, execution is permitted)
2. RO data (Read only at EL2)
3. RO after init data and RW data (Read/Write at EL2)
4. Init Text (Read only at EL2, execution is permitted)
5. Init data and BSS (Read/Write at EL2)

Before creating a region, we check if the count exceeds the number defined in
MPUIR_EL2. If so, then the boot fails.

Also we check if the region is empty or not. IOW, if the start and end address
are same, we skip mapping the region.

To map a region, Xen uses the PRBAR_EL2, PRLAR_EL2 and PRSELR_EL2 registers.
One can refer to ARM DDI 0600B.a ID062922 G1.3  "General System Control
Registers", to get the definitions of these registers. Also, refer to G1.2
"Accessing MPU memory region registers", the following

```
The MPU provides two register interfaces to program the MPU regions:
- Access to any of the MPU regions via PRSELR_ELx, PRBAR_ELx, and
PRLAR_ELx.
```

We use the above mechanism to create the MPU memory regions.

Also, the compiler needs the flag ("-march=armv8-r") in order to build Xen for
Armv8-R AArch64 MPU based systems. There will be no need for us to explicitly
define MPU specific registers.

Signed-off-by: Ayan Kumar Halder 
---

Changes looks ok to me and I’ve also built and tested, maybe one NIT below


diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 5abd4b0d1c..59b774b7b8 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -16,7 +16,7 @@

#if defined(CONFIG_MMU)
# include 
-#else
+#elif !defined(CONFIG_MPU)
# error "Unknown memory management layout"
#endif


^— maybe this change is not needed at this stage, it will be soon though

Anyway:

Reviewed-by: Luca Fancellu mailto:fance...@arm.com>>

Cheers,
Luca

Re: [PATCH v1 1/2] xen/mpu: Map early uart when earlyprintk on

[Luca Fancellu]

> On 8 Nov 2024, at 20:00, Ayan Kumar Halder  wrote:
> 
> CONFIG_EARLY_UART_SIZE is introduced to let user provide physical size of
> early UART. Unlike MMU where we map a page in the virtual address space,
> here we need to know the exact physical size to be mapped.
> As VA == PA in case of MPU, the memory layout follows exactly the hardware
> configuration. As a consequence, we set  EARLY_UART_VIRTUAL_ADDRESS as 
> physical
> address.
> 
> Further, we check whether user-defined EARLY_UART_SIZE is aligned to PAGE_SIZE
> (4KB). This is partly because we intend to map a minimum of 1 page(ie 4KB) and
> the limit address is set as "EARLY_UART_SIZE-1". The limit address needs to 
> end
> with 0x3f (as required by PRLAR register).
> 
> UART is mapped as nGnRE region (as specified by ATTR=100 , refer G1.3.13,
> MAIR_EL2, "---0100 Device memory nGnRE") and Doc ID - 102670_0101_02_en
> Table 4-3, Armv8 architecture memory types (nGnRE - Corresponds to Device in
> Armv7 architecture). Also, it is mapped as outer shareable, RW at EL2 only
> and execution of instructions from the region is not permitted.
> 
> Signed-off-by: Ayan Kumar Halder 
> —

This looks ok to me

Reviewed-by: Luca Fancellu 

Re: [ARM] preparations for 4.19.1

[Luca Fancellu]

Hi all,

On 12 Nov 2024, at 16:11, Julien Grall  wrote:

Hi,

On 12/11/2024 16:00, Stewart Hildebrand wrote:
On 11/12/24 08:00, Jan Beulich wrote:
All,

the release is due by the end of the month. Please point out backports you find
missing from the respective staging branch, but which you consider relevant.

Advance notice: 4.18.4 ought to follow about two weeks later.

Jan

Looking for Julien's input on this one:
35c64c3dce01 ("xen/arm64: entry: Actually skip do_trap_*() when an SError is 
triggered")
As mentioned in the post-commit notes [0] it's a candidate for backport.

In the past, Stefano handled backports for Arm. I am not sure if this is still 
case. Stefano?

[0] https://lore.kernel.org/xen-devel/20240806124815.53492-1-jul...@xen.org/

Cheers,

--
Julien Grall



Regarding back porting to the 4.19, there is a regression for Arm discussed in 
the ML and on Matrix, do we want to address it now?


[v5,1/3] xen/device-tree: Let DT reserve map entries overlap reserved-memory - 
Patchwork
patchwork.kernel.org
[X]



xen/device-tree: Allow exact match for overlapping regions - 
Patchwork
patchwork.kernel.org
[X]


Cheers,
Luca

Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions

[Luca Fancellu]

Hi Julien,

> On 13 Nov 2024, at 14:01, Julien Grall  wrote:
> 
> Hi Luca,
> 
> On 06/11/2024 13:41, Luca Fancellu wrote:
>> There are some cases where the device tree exposes a memory range
>> in both /memreserve/ and reserved-memory node, in this case the
>> current code will stop Xen to boot since it will find that the
>> latter range is clashing with the already recorded /memreserve/
>> ranges.
>> Furthermore, u-boot lists boot modules ranges, such as ramdisk,
>> in the /memreserve/ part and even in this case this will prevent
>> Xen to boot since it will see that the module memory range that
>> it is going to add in 'add_boot_module' clashes with a /memreserve/
>> range.
>> When Xen populate the data structure that tracks the memory ranges,
>> it also adds a memory type described in 'enum membank_type', so
>> in order to fix this behavior, allow the 'check_reserved_regions_overlap'
>> function to check for exact memory range match given a specific memory
>> type; allowing reserved-memory node ranges and boot modules to have an
>> exact match with ranges from /memreserve/.
>> While there, set a type for the memory recorded during ACPI boot.
> 
> I am a bit confused which you mention only ACPI boot. Isn't the path also 
> used when booting using Device-Tree?

right, maybe this should be:

“While there, set a type for the memory recorded using meminfo_add_bank() from 
eft-boot.h."

>> 
>>  static bool __init meminfo_overlap_check(const struct membanks *mem,
>>   paddr_t region_start,
>> - paddr_t region_size)
>> + paddr_t region_size,
>> + enum membank_type allow_match_type)
> 
> Looking at the callers, you only seem to pass MEMBANK_FDT_RESVMEM or 
> MEMBANK_NONE. So I wonder whether it actually make sense to introduce 
> MEMBANK_NONE. Wouldn't it be better to have a boolean indicating whether we 
> are looking for FDT_RESVMEM?

I wanted to give a more generic approach, but you are right, we could have a 
boolean like allow_match_memreserve.


> 
>>  {
>>  paddr_t bank_start = INVALID_PADDR, bank_end = 0;
>>  paddr_t region_end = region_start + region_size;
>> @@ -113,12 +114,16 @@ static bool __init meminfo_overlap_check(const struct 
>> membanks *mem,
>>  if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
>>   region_start >= bank_end )
>>  continue;
>> -else
>> -{
>> -printk("Region: [%#"PRIpaddr", %#"PRIpaddr") overlapping with 
>> bank[%u]: [%#"PRIpaddr", %#"PRIpaddr")\n",
>> -   region_start, region_end, i, bank_start, bank_end);
>> -return true;
>> -}
>> +
>> +if ( (allow_match_type != MEMBANK_NONE) &&
>> + (region_start == bank_start) && (region_end == bank_end) &&
> 
> Why is this only an exact match?

Apparently what we are fixing is only a case where memreserve regions matches 
exactly modules or reserved_mem nodes.

> 
>> + (allow_match_type == mem->bank[i].type) )
>> +continue;
>> +
>> +printk("Region: [%#"PRIpaddr", %#"PRIpaddr") overlapping with 
>> bank[%u]: [%#"PRIpaddr", %#"PRIpaddr")\n",
>> +region_start, region_end, i, bank_start, bank_end);
>> +return true;
>> +
>>  }
>>return false;
>> @@ -169,9 +174,14 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>>   * with the existing reserved memory regions defined in bootinfo.
>>   * Return true if the input physical address range is overlapping with any
>>   * existing reserved memory regions, otherwise false.
>> + * The 'allow_match_type' parameter can be used to allow exact match of a
>> + * region with another memory region already recorded, but it needs to be 
>> used
>> + * only on memory regions that allows a type (reserved_mem, acpi). For all 
>> the
>> + * other cases, passing 'MEMBANK_NONE' will disable the exact match.
>>   */
>>  bool __init check_reserved_regions_overlap(paddr_t region_start,
>> -   paddr_t region_size)
>> +   paddr_t region_size,
>> +   enum membank_type 
>> allow_match_typ

Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions

[Luca Fancellu]

Hi Julien,


 -}
 +
 +if ( (allow_match_type != MEMBANK_NONE) &&
 + (region_start == bank_start) && (region_end == bank_end) &&
>>> 
>>> Why is this only an exact match?
>> Apparently what we are fixing is only a case where memreserve regions 
>> matches exactly modules or reserved_mem nodes.
> 
> TBH, as I replied to Michal, I don't understand why we are just focusing on 
> just one issue. It would be good to provide some rationale.

I’m not sure why some DT out there are providing memory ranges reserved in both 
resvmem and reserved_memory node, could it be
that it was needed because some firmware was reading only one of the two before 
starting linux? I’m just thinking out loud.

Is your point that we should allow instead not only exact matching but also 
partial matching?

 
 +
 +#ifdef CONFIG_STATIC_SHM
 +/*
 + * Static shared memory banks don't have a type, so for them 
 disable
 + * the exact match check.
 + */
>>> 
>>> This feels a bit of a hack. Why can't we had a default type like you did in 
>>> meminfo_add_bank()?
>> This is the structure:
>> struct membank {
>> paddr_t start;
>> paddr_t size;
>> union {
>> enum membank_type type;
>> #ifdef CONFIG_STATIC_SHM
>> struct shmem_membank_extra *shmem_extra;
>> #endif
>> };
>> };
> 
> Anonymous union are really annoying... So effectively in 
> check_reserved_regions_overlap() we are hoping that the caller will not set 
> allow_match_type to another value than MEMBANK_NONE for static memory. This 
> is extremely fragile.
> 
> I can't think of another solution right now, but I am definitely not happy 
> with this approach.

I agree it’s not nice, but the issue is not in 
check_reserved_regions_overlap(), it is in meminfo_overlap_check() which is 
static and so confined to that module.

Anyway I’m trying to think about another solution.

> 
>> we did that in order to save space when static shared memory is not enabled, 
>> so we can’t have the
>> type for these banks because we are already writing shmem_extra.
>> We could remove the union but in that case we would waste space when static 
>> shared memory is
>> enabled.
> 
> Can you remind me how much memory this is going to save?

The space issue comes from:

#define NR_MEM_BANKS 256

[…]

struct meminfo {
struct membanks_hdr common;
struct membank bank[NR_MEM_BANKS];
};

struct bootinfo {
struct meminfo mem;
/* The reserved regions are only used when booting using Device-Tree */
struct meminfo reserved_mem;

[…]

#ifdef CONFIG_ACPI
struct meminfo acpi;
#endif
[…]
};

So in case we remove the union and CONFIG_STATIC_SHM=y and so we allow type and 
*shmem_extra to coexist, we have 256 * 2 * sizeof(pointer) byte.
Additionaly if CONFIG_ACPI=y, we have 256 * sizeof(pointer) byte more not used 
space.

Cheers,
Luca

Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions

[Luca Fancellu]

Hi Michal,

> So we have 2 separate issues. I don't particularly like the concept of 
> introducing MEMBANK_NONE
> and the changes below look a bit too much for me, given that for boot modules 
> we can only have
> /memreserve/ matching initrd.
> 
> Shawn patch fixes the first issue. AFAICT the second issue can be fixed by 
> below simple patch:
> diff --git a/xen/common/device-tree/bootfdt.c 
> b/xen/common/device-tree/bootfdt.c
> index 927f59c64b0d..d8bd8c44bd35 100644
> --- a/xen/common/device-tree/bootfdt.c
> +++ b/xen/common/device-tree/bootfdt.c
> @@ -586,6 +586,10 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
> paddr)
> 
> add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
> 
> +ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
> +if ( ret )
> +panic("Early FDT parsing failed (%d)\n", ret);
> +
> nr_rsvd = fdt_num_mem_rsv(fdt);
> if ( nr_rsvd < 0 )
> panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
> @@ -594,10 +598,14 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
> paddr)
> {
> struct membank *bank;
> paddr_t s, sz;
> +const struct bootmodule *mod = 
> boot_module_find_by_kind(BOOTMOD_RAMDISK);
> 
> if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
> continue;
> 
> +if ( mod && (mod->start == s) && (mod->size == sz) )
> +continue;

Ok I see, we skip the /memreserve/ entry if it matches the ramdisk, fair 
enough, I don’t have
a strong opinion on how we do that, the important thing is just to unblock the 
users experiencing
this issue.

Cheers,
Luca

[PATCH] xen/device-tree: Allow exact match for overlapping regions

[Luca Fancellu]

There are some cases where the device tree exposes a memory range
in both /memreserve/ and reserved-memory node, in this case the
current code will stop Xen to boot since it will find that the
latter range is clashing with the already recorded /memreserve/
ranges.

Furthermore, u-boot lists boot modules ranges, such as ramdisk,
in the /memreserve/ part and even in this case this will prevent
Xen to boot since it will see that the module memory range that
it is going to add in 'add_boot_module' clashes with a /memreserve/
range.

When Xen populate the data structure that tracks the memory ranges,
it also adds a memory type described in 'enum membank_type', so
in order to fix this behavior, allow the 'check_reserved_regions_overlap'
function to check for exact memory range match given a specific memory
type; allowing reserved-memory node ranges and boot modules to have an
exact match with ranges from /memreserve/.

While there, set a type for the memory recorded during ACPI boot.

Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to 
bootinfo.reserved_mem")
Reported-by: Shawn Anastasio 
Reported-by: Grygorii Strashko 
Signed-off-by: Luca Fancellu 
---
I tested this patch adding the same range in a /memreserve/ entry and
/reserved-memory node, and by letting u-boot pass a ramdisk.
I've also tested that a configuration running static shared memory still works
fine.
---
 xen/arch/arm/efi/efi-boot.h   |  3 +-
 xen/arch/arm/static-shmem.c   |  2 +-
 xen/common/device-tree/bootfdt.c  |  9 +-
 xen/common/device-tree/bootinfo.c | 48 ---
 xen/include/xen/bootfdt.h |  9 +-
 5 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 199f5260229d..d35c991c856f 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -167,13 +167,14 @@ static bool __init meminfo_add_bank(struct membanks *mem,
 if ( mem->nr_banks >= mem->max_banks )
 return false;
 #ifdef CONFIG_ACPI
-if ( check_reserved_regions_overlap(start, size) )
+if ( check_reserved_regions_overlap(start, size, MEMBANK_NONE) )
 return false;
 #endif
 
 bank = &mem->bank[mem->nr_banks];
 bank->start = start;
 bank->size = size;
+bank->type = MEMBANK_DEFAULT;
 
 mem->nr_banks++;
 
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index aa80756c3ca5..149ed4b0a5ba 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -696,7 +696,7 @@ int __init process_shm_node(const void *fdt, int node, 
uint32_t address_cells,
 if (i < mem->max_banks)
 {
 if ( (paddr != INVALID_PADDR) &&
- check_reserved_regions_overlap(paddr, size) )
+ check_reserved_regions_overlap(paddr, size, MEMBANK_NONE) )
 return -EINVAL;
 
 /* Static shared memory shall be reserved from any other use. */
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..fb3a6ab95a22 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -176,8 +176,15 @@ static int __init device_tree_get_meminfo(const void *fdt, 
int node,
 for ( i = 0; i < banks && mem->nr_banks < mem->max_banks; i++ )
 {
 device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+/*
+ * Some valid device trees, such as those generated by OpenPOWER
+ * skiboot firmware, expose all reserved memory regions in the
+ * FDT memory reservation block AND in the reserved-memory node which
+ * has already been parsed. Thus, any matching overlaps in the
+ * reserved_mem banks should be ignored.
+ */
 if ( mem == bootinfo_get_reserved_mem() &&
- check_reserved_regions_overlap(start, size) )
+ check_reserved_regions_overlap(start, size, MEMBANK_FDT_RESVMEM) )
 return -EINVAL;
 /* Some DT may describe empty bank, ignore them */
 if ( !size )
diff --git a/xen/common/device-tree/bootinfo.c 
b/xen/common/device-tree/bootinfo.c
index f2e6a1145b7c..05038075e835 100644
--- a/xen/common/device-tree/bootinfo.c
+++ b/xen/common/device-tree/bootinfo.c
@@ -99,7 +99,8 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
  */
 static bool __init meminfo_overlap_check(const struct membanks *mem,
  paddr_t region_start,
- paddr_t region_size)
+ paddr_t region_size,
+ enum membank_type allow_match_type)
 {
 paddr_t bank_start = INVALID_PADDR, bank_end = 0;
 paddr_t region_end = region_start + region_size;
@@ -113,12 +114,1

[PATCH v3 2/5] arm/setup: Move MMU specific extern declarations to mmu/setup.h

[Luca Fancellu]

Move some extern declarations related to MMU structures and define
from asm/setup.h to asm/mmu/setup.h, in order to increase encapsulation
and allow the MPU part to build, since it has no clue about them.

Signed-off-by: Luca Fancellu 
---
Changes from v2:
 - No changes
Changes from v1:
 - Moved extern to mmu/setup.h instead of mmu/mm.h
 - moved also pte_of_xenaddr()
---
---
 xen/arch/arm/include/asm/mmu/setup.h | 31 
 xen/arch/arm/include/asm/setup.h | 20 ++
 2 files changed, 37 insertions(+), 14 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/mmu/setup.h

diff --git a/xen/arch/arm/include/asm/mmu/setup.h 
b/xen/arch/arm/include/asm/mmu/setup.h
new file mode 100644
index ..3fe752b04c63
--- /dev/null
+++ b/xen/arch/arm/include/asm/mmu/setup.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __ARM_MMU_SETUP_H__
+#define __ARM_MMU_SETUP_H__
+
+#include 
+#include 
+
+extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
+
+#ifdef CONFIG_ARM_64
+extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
+#endif
+extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
+extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
+
+/* Find where Xen will be residing at runtime and return a PT entry */
+lpae_t pte_of_xenaddr(vaddr_t va);
+
+#endif /* __ARM_MMU_SETUP_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 64c227d171fc..a5a80d9b477f 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -6,6 +6,12 @@
 #include 
 #include 
 
+#if defined(CONFIG_MMU)
+# include 
+#elif !defined(CONFIG_MPU)
+# error "Unknown memory management layout"
+#endif
+
 #define MAX_FDT_SIZE SZ_2M
 
 struct map_range_data
@@ -65,20 +71,6 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
 int map_range_to_domain(const struct dt_device_node *dev,
 uint64_t addr, uint64_t len, void *data);
 
-extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
-
-#ifdef CONFIG_ARM_64
-extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
-#endif
-extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
-extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
-
-/* Find where Xen will be residing at runtime and return a PT entry */
-lpae_t pte_of_xenaddr(vaddr_t va);
-
 extern const char __ro_after_init_start[], __ro_after_init_end[];
 
 struct init_info
-- 
2.34.1

[PATCH v3 4/5] xen/arm: Check for Static Heap feature when freeing resources

[Luca Fancellu]

From: Penny Zheng 

If the Xen heap is statically configured in Device Tree, its size is
definite, so only the defined memory shall be given to the boot
allocator. Have a check where init_domheap_pages() is called
which takes into account if static heap feature is used.

Extract static_heap flag from init data bootinfo, as it will be needed
after destroying the init data section.

Introduce a new helper using_static_heap() to tell whether Xen Heap
is statically configured in the Device Tree.

Signed-off-by: Penny Zheng 
Signed-off-by: Wei Chen 
Signed-off-by: Luca Fancellu 
---
Changes from v2:
 - Change xen_is_using_staticheap() to using_static_heap()
 - Move declaration of static_heap and using_static_heap() to xen/mm.h
 - Reprased first part of the commit message and title
Changes from v1:
 - moved static_heap to common/page_alloc.c
 - protect static_heap access with CONFIG_STATIC_MEMORY
 - update comment in arm/kernel.c kernel_decompress()
---
---
 xen/arch/arm/arm32/mmu/mm.c   |  4 ++--
 xen/arch/arm/kernel.c |  7 ---
 xen/arch/arm/mmu/setup.c  |  8 ++--
 xen/arch/arm/setup.c  | 27 ++-
 xen/common/device-tree/bootfdt.c  |  4 +++-
 xen/common/device-tree/bootinfo.c |  2 +-
 xen/common/page_alloc.c   |  5 +
 xen/include/xen/bootfdt.h |  1 -
 xen/include/xen/mm.h  | 13 +
 9 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 063611412be0..905565e1b528 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -199,7 +199,7 @@ void __init setup_mm(void)
 
 total_pages = ram_size >> PAGE_SHIFT;
 
-if ( bootinfo.static_heap )
+if ( using_static_heap() )
 {
 const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 
@@ -246,7 +246,7 @@ void __init setup_mm(void)
 
 do
 {
-e = bootinfo.static_heap ?
+e = using_static_heap() ?
 fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
 consider_modules(ram_start, ram_end,
  pfn_to_paddr(xenheap_pages),
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 293d7efaed9c..5dc367951113 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -244,10 +244,11 @@ static __init int kernel_decompress(struct bootmodule 
*mod, uint32_t offset)
 size += offset;
 
 /*
- * Free the original kernel, update the pointers to the
- * decompressed kernel
+ * In case Xen is not using the static heap feature, free the original
+ * kernel, update the pointers to the decompressed kernel
  */
-fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+if ( !using_static_heap() )
+fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
 return 0;
 }
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 9664e85ee6c0..36d0efd16c29 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -341,8 +341,12 @@ void free_init_memory(void)
 if ( rc )
 panic("Unable to remove the init section (rc = %d)\n", rc);
 
-init_domheap_pages(pa, pa + len);
-printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+if ( !using_static_heap() )
+{
+init_domheap_pages(pa, pa + len);
+printk("Freed %ldkB init memory.\n",
+   (long)(__init_end-__init_begin) >> 10);
+}
 }
 
 /**
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca94..46689d13072e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,24 +206,25 @@ void __init discard_initial_modules(void)
 struct bootmodules *mi = &bootinfo.modules;
 int i;
 
-for ( i = 0; i < mi->nr_mods; i++ )
+if ( !using_static_heap() )
 {
-paddr_t s = mi->module[i].start;
-paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
-
-if ( mi->module[i].kind == BOOTMOD_XEN )
-continue;
+for ( i = 0; i < mi->nr_mods; i++ )
+{
+paddr_t s = mi->module[i].start;
+paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-if ( !mfn_valid(maddr_to_mfn(s)) ||
- !mfn_valid(maddr_to_mfn(e)) )
-continue;
+if ( mi->module[i].kind == BOOTMOD_XEN )
+continue;
 
-fw_unreserved_regions(s, e, init_domheap_pages, 0);
-}
+if ( !mfn_valid(maddr_to_mfn(s)) ||
+ !mfn_valid(maddr_to_mfn(e)) )
+continue;
 
-mi->nr_mods = 0;
+fw_unreserved_regions(s, e, init_domheap_pages, 0);
+}
 
-remove_early_mappings();
+mi->nr_mods = 0;
+}
 }
 
 /* Relocate the FDT in Xen heap */
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/

[PATCH v3 0/5] Prerequisite patches for R82 upstreaming

[Luca Fancellu]

In this serie I've taken out patches from the R82 branch already in the ML[1]
and some new patches I've done based on the current status of staging that will
not impact the current Armv8-R earlyboot work.

[1] 
https://patchwork.kernel.org/project/xen-devel/cover/20230626033443.2943270-1-penny.zh...@arm.com/

Changes between v2 and v3:
 - New patch
 - changes to previous patch are listed inside them

Luca Fancellu (4):
  common/vmap: Fall back to simple allocator when !HAS_VMAP
  arm/setup: Move MMU specific extern declarations to mmu/setup.h
  xen/arm: Use vmap_contig instead of __vmap where it's possible
  xen/arm: Move setup_frametable_mappings to arm/mmu

Penny Zheng (1):
  xen/arm: Check for Static Heap feature when freeing resources

 xen/arch/arm/alternative.c   |  3 +-
 xen/arch/arm/arm32/mmu/mm.c  |  4 +-
 xen/arch/arm/cpuerrata.c |  5 +--
 xen/arch/arm/include/asm/mmu/setup.h | 31 ++
 xen/arch/arm/include/asm/setup.h | 20 +++--
 xen/arch/arm/kernel.c|  9 ++--
 xen/arch/arm/livepatch.c |  3 +-
 xen/arch/arm/mm.c| 40 --
 xen/arch/arm/mmu/Makefile|  1 +
 xen/arch/arm/mmu/mm.c| 61 
 xen/arch/arm/mmu/setup.c |  8 +++-
 xen/arch/arm/setup.c | 27 ++--
 xen/common/device-tree/bootfdt.c |  4 +-
 xen/common/device-tree/bootinfo.c|  2 +-
 xen/common/page_alloc.c  |  5 +++
 xen/include/xen/bootfdt.h|  1 -
 xen/include/xen/mm.h | 13 ++
 xen/include/xen/vmap.h   |  2 +-
 xen/include/xen/xvmalloc.h   | 36 +---
 19 files changed, 184 insertions(+), 91 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/mmu/setup.h
 create mode 100644 xen/arch/arm/mmu/mm.c

-- 
2.34.1

[PATCH v3 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

When HAS_VMAP is disabled, the xv{malloc,zalloc,...} functions
should fall back to the simple x{malloc,zalloc,...} variant,
implement that because MPU systems won't have virtual memory.

Additionally remove VMAP_VIRT_START from vmap.h guards since
MPU systems won't have it defined.

Signed-off-by: Luca Fancellu 
---
Changes from v2:
 - Don't protect declarations.
Changes from v1:
 - put back static inline iounmap
 - changed commit message
 - hide not used declaration for system with !HAS_VMAP
 - correct function declared in xvmalloc.h to be static inline
 - prefer '#ifdef' instead of '#if defined' where possible
---
---
 xen/include/xen/vmap.h |  2 +-
 xen/include/xen/xvmalloc.h | 36 +++-
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..26c831757a11 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -5,7 +5,7 @@
  * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
  * latter is used when loading livepatches and the former for everything else.
  */
-#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
+#ifndef __XEN_VMAP_H__
 #define __XEN_VMAP_H__
 
 #include 
diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
index 440d85a284bb..e97a30f61e96 100644
--- a/xen/include/xen/xvmalloc.h
+++ b/xen/include/xen/xvmalloc.h
@@ -40,20 +40,46 @@
 ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
  __alignof__(typeof(*(ptr)
 
+#ifdef CONFIG_HAS_VMAP
+
 /* Free any of the above. */
 void xvfree(void *va);
 
+/* Underlying functions */
+void *_xvmalloc(size_t size, unsigned int align);
+void *_xvzalloc(size_t size, unsigned int align);
+void *_xvrealloc(void *va, size_t size, unsigned int align);
+
+#else /* !CONFIG_HAS_VMAP */
+
+static inline void xvfree(void *va)
+{
+xfree(va);
+}
+
+static inline void *_xvmalloc(size_t size, unsigned int align)
+{
+return _xmalloc(size, align);
+}
+
+static inline void *_xvzalloc(size_t size, unsigned int align)
+{
+return _xzalloc(size, align);
+}
+
+static inline void *_xvrealloc(void *va, size_t size, unsigned int align)
+{
+return _xrealloc(va, size, align);
+}
+
+#endif /* CONFIG_HAS_VMAP */
+
 /* Free an allocation, and zero the pointer to it. */
 #define XVFREE(p) do { \
 xvfree(p); \
 (p) = NULL;\
 } while ( false )
 
-/* Underlying functions */
-void *_xvmalloc(size_t size, unsigned int align);
-void *_xvzalloc(size_t size, unsigned int align);
-void *_xvrealloc(void *va, size_t size, unsigned int align);
-
 static inline void *_xvmalloc_array(
 size_t size, unsigned int align, unsigned long num)
 {
-- 
2.34.1

[PATCH v3 5/5] xen/arm: Move setup_frametable_mappings to arm/mmu

[Luca Fancellu]

Move the current setup_frametable_mappings implementation to
arm/mmu under a new file mm.c, this implementation depends on
virtual memory and won't be used as it is for MPU systems.

Take the occasion to fix code style issues related to the line
length.

Moved also frametable_virt_end since it is used only on MMU
systems.

Signed-off-by: Luca Fancellu 
---
Changes to v2:
 - New patch
---
---
 xen/arch/arm/mm.c | 40 -
 xen/arch/arm/mmu/Makefile |  1 +
 xen/arch/arm/mmu/mm.c | 61 +++
 3 files changed, 62 insertions(+), 40 deletions(-)
 create mode 100644 xen/arch/arm/mmu/mm.c

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index def939172cc5..a56e20ba2bdc 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -22,7 +22,6 @@
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
 
 unsigned long frametable_base_pdx __read_mostly;
-unsigned long frametable_virt_end __read_mostly;
 
 void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 {
@@ -43,45 +42,6 @@ void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 invalidate_icache();
 }
 
-/* Map a frame table to cover physical addresses ps through pe */
-void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
-{
-unsigned long nr_pdxs = mfn_to_pdx(mfn_add(maddr_to_mfn(pe), -1)) -
-mfn_to_pdx(maddr_to_mfn(ps)) + 1;
-unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
-mfn_t base_mfn;
-const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : 
MB(32);
-int rc;
-
-/*
- * The size of paddr_t should be sufficient for the complete range of
- * physical address.
- */
-BUILD_BUG_ON((sizeof(paddr_t) * BITS_PER_BYTE) < PADDR_BITS);
-BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
-
-if ( frametable_size > FRAMETABLE_SIZE )
-panic("The frametable cannot cover the physical region %#"PRIpaddr" - 
%#"PRIpaddr"\n",
-  ps, pe);
-
-frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
-/* Round up to 2M or 32M boundary, as appropriate. */
-frametable_size = ROUNDUP(frametable_size, mapping_size);
-base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
-
-rc = map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
-  frametable_size >> PAGE_SHIFT,
-  PAGE_HYPERVISOR_RW | _PAGE_BLOCK);
-if ( rc )
-panic("Unable to setup the frametable mappings.\n");
-
-memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
-memset(&frame_table[nr_pdxs], -1,
-   frametable_size - (nr_pdxs * sizeof(struct page_info)));
-
-frametable_virt_end = FRAMETABLE_VIRT_START + (nr_pdxs * sizeof(struct 
page_info));
-}
-
 int steal_page(
 struct domain *d, struct page_info *page, unsigned int memflags)
 {
diff --git a/xen/arch/arm/mmu/Makefile b/xen/arch/arm/mmu/Makefile
index 2cb44b857dd2..1c89602947de 100644
--- a/xen/arch/arm/mmu/Makefile
+++ b/xen/arch/arm/mmu/Makefile
@@ -1,3 +1,4 @@
+obj-y += mm.o
 obj-y += p2m.o
 obj-y += pt.o
 obj-y += setup.o
diff --git a/xen/arch/arm/mmu/mm.c b/xen/arch/arm/mmu/mm.c
new file mode 100644
index ..aefec908b7f2
--- /dev/null
+++ b/xen/arch/arm/mmu/mm.c
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+unsigned long frametable_virt_end __read_mostly;
+
+/* Map a frame table to cover physical addresses ps through pe */
+void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
+{
+unsigned long nr_pdxs = mfn_to_pdx(mfn_add(maddr_to_mfn(pe), -1)) -
+mfn_to_pdx(maddr_to_mfn(ps)) + 1;
+unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
+mfn_t base_mfn;
+const unsigned long mapping_size = frametable_size < MB(32) ? MB(2)
+: MB(32);
+int rc;
+
+/*
+ * The size of paddr_t should be sufficient for the complete range of
+ * physical address.
+ */
+BUILD_BUG_ON((sizeof(paddr_t) * BITS_PER_BYTE) < PADDR_BITS);
+BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
+
+if ( frametable_size > FRAMETABLE_SIZE )
+panic("The frametable cannot cover the physical region %#"PRIpaddr" - 
%#"PRIpaddr"\n",
+  ps, pe);
+
+frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps));
+/* Round up to 2M or 32M boundary, as appropriate. */
+frametable_size = ROUNDUP(frametable_size, mapping_size);
+base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
+
+rc = map_pages_to_xen(FRAMETABLE_VIRT_START, base_mfn,
+  frametable_size >> PAGE_SHIFT,
+  PAG

[PATCH v3 3/5] xen/arm: Use vmap_contig instead of __vmap where it's possible

[Luca Fancellu]

Currently the arm code uses __vmap function in few parts to map
physically contiguous pages, vmap_contig was introduced recently
and does the same because it's a wrapper for __vmap, so use the
latter instead of the direct __vmap function.

Signed-off-by: Luca Fancellu 
Acked-by: Julien Grall 
Acked-by: Roger Pau Monné 
---
Changes from v2:
 - Add ack-by Roger
Changes from v1:
 - Add ack-by Julien
---
---
 xen/arch/arm/alternative.c | 3 +--
 xen/arch/arm/cpuerrata.c   | 5 ++---
 xen/arch/arm/kernel.c  | 2 +-
 xen/arch/arm/livepatch.c   | 3 +--
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index d99b5070937d..fec7dbd2cde9 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -209,8 +209,7 @@ void __init apply_alternatives_all(void)
  * The text and inittext section are read-only. So re-map Xen to
  * be able to patch the code.
  */
-xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
-VMAP_DEFAULT);
+xenmap = vmap_contig(xen_mfn, 1U << xen_order);
 /* Re-mapping Xen is not expected to fail during boot. */
 BUG_ON(!xenmap);
 
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index eef9c0ea0e21..17cf134f1b0d 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -61,9 +61,8 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char 
*hyp_vec_start,
  * Vectors are part of the text that are mapped read-only. So re-map
  * the vector table to be able to update vectors.
  */
-dst_remapped = __vmap(&dst_mfn,
-  1UL << get_order_from_bytes(VECTOR_TABLE_SIZE),
-  1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+dst_remapped = vmap_contig(dst_mfn,
+   1UL << get_order_from_bytes(VECTOR_TABLE_SIZE));
 if ( !dst_remapped )
 return false;
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 669d143cee1b..293d7efaed9c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -211,7 +211,7 @@ static __init int kernel_decompress(struct bootmodule *mod, 
uint32_t offset)
 return -ENOMEM;
 }
 mfn = page_to_mfn(pages);
-output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, 
VMAP_DEFAULT);
+output = vmap_contig(mfn, 1 << kernel_order_out);
 
 rc = perform_gunzip(output, input, size);
 clean_dcache_va_range(output, output_size);
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 037746d9528d..3805b2974663 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -39,8 +39,7 @@ int arch_livepatch_quiesce(void)
  * The text section is read-only. So re-map Xen to be able to patch
  * the code.
  */
-vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, 
PAGE_HYPERVISOR,
-  VMAP_DEFAULT);
+vmap_of_xen_text = vmap_contig(text_mfn, 1U << text_order);
 
 if ( !vmap_of_xen_text )
 {
-- 
2.34.1

Re: [PATCH v3 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

Hi Jan,

> On 29 Nov 2024, at 11:06, Jan Beulich  wrote:
> 
> On 29.11.2024 10:12, Luca Fancellu wrote:
>> --- a/xen/include/xen/xvmalloc.h
>> +++ b/xen/include/xen/xvmalloc.h
>> @@ -40,20 +40,46 @@
>> ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
>>  __alignof__(typeof(*(ptr)
>> 
>> +#ifdef CONFIG_HAS_VMAP
>> +
>> /* Free any of the above. */
>> void xvfree(void *va);
>> 
>> +/* Underlying functions */
>> +void *_xvmalloc(size_t size, unsigned int align);
>> +void *_xvzalloc(size_t size, unsigned int align);
>> +void *_xvrealloc(void *va, size_t size, unsigned int align);
>> +
>> +#else /* !CONFIG_HAS_VMAP */
>> +
>> +static inline void xvfree(void *va)
>> +{
>> +xfree(va);
>> +}
>> +
>> +static inline void *_xvmalloc(size_t size, unsigned int align)
>> +{
>> +return _xmalloc(size, align);
>> +}
>> +
>> +static inline void *_xvzalloc(size_t size, unsigned int align)
>> +{
>> +return _xzalloc(size, align);
>> +}
>> +
>> +static inline void *_xvrealloc(void *va, size_t size, unsigned int align)
>> +{
>> +return _xrealloc(va, size, align);
>> +}
> 
> Just to double check: Was it at least considered to use simple #define-s
> to effect the aliasing? Wrapper functions like the above ones have the
> downside of needing touching (easy to miss) when the wrapped function
> types change in whichever minor way. (And yes, I do understand that we
> generally aim at using inline functions in preference to macros.)

Yes, I think I tried and I didn’t have issues using #define-s, I asked here
https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-2-luca.fance...@arm.com/#26123448
about a preferred approach, but I didn’t have any reply, so I went for what
I believed was preferred as you said, static inline-s instead of macros.

Cheers,
Luca

Re: [PATCH v3 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

>>> 
>>> Just to double check: Was it at least considered to use simple #define-s
>>> to effect the aliasing? Wrapper functions like the above ones have the
>>> downside of needing touching (easy to miss) when the wrapped function
>>> types change in whichever minor way. (And yes, I do understand that we
>>> generally aim at using inline functions in preference to macros.)
>> 
>> Yes, I think I tried and I didn’t have issues using #define-s, I asked here
>> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-2-luca.fance...@arm.com/#26123448
>> about a preferred approach, but I didn’t have any reply, so I went for what
>> I believed was preferred as you said, static inline-s instead of macros.
> 
> As Andrew's idea didn't work out, personally I think the simple #define
> approach you suggested would be preferable in this case. There is in
> particular no type-safety concern here, as the wrapped functions will
> all have the intended type checking applied.

ok, I’ll re-spin this one with #defines.

> 
> Jan

[PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu]

From: Penny Zheng 

If Xenheap is statically configured in Device Tree, its size is
definite. So, the memory shall not be given back into static heap, like
it's normally done in free_init_memory, etc, once the initialization
is finished.

Extract static_heap flag from init data bootinfo, as it will be needed
after destroying the init data section.
Introduce a new helper xen_is_using_staticheap() to tell whether Xenheap
is statically configured in the Device Tree.

Signed-off-by: Penny Zheng 
Signed-off-by: Wei Chen 
Signed-off-by: Luca Fancellu 
---
Changes from v1:
 - moved static_heap to common/page_alloc.c
 - protect static_heap access with CONFIG_STATIC_MEMORY
 - update comment in arm/kernel.c kernel_decompress()
---
---
 xen/arch/arm/arm32/mmu/mm.c   |  4 ++--
 xen/arch/arm/kernel.c |  7 ---
 xen/arch/arm/mmu/setup.c  |  8 ++--
 xen/arch/arm/setup.c  | 27 ++-
 xen/common/device-tree/bootfdt.c  |  4 +++-
 xen/common/device-tree/bootinfo.c |  2 +-
 xen/common/page_alloc.c   |  5 +
 xen/include/xen/bootfdt.h | 14 +-
 8 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 063611412be0..b7ca7c94c9ca 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -199,7 +199,7 @@ void __init setup_mm(void)
 
 total_pages = ram_size >> PAGE_SHIFT;
 
-if ( bootinfo.static_heap )
+if ( xen_is_using_staticheap() )
 {
 const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 
@@ -246,7 +246,7 @@ void __init setup_mm(void)
 
 do
 {
-e = bootinfo.static_heap ?
+e = xen_is_using_staticheap() ?
 fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
 consider_modules(ram_start, ram_end,
  pfn_to_paddr(xenheap_pages),
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 293d7efaed9c..d2245ec9d2ef 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -244,10 +244,11 @@ static __init int kernel_decompress(struct bootmodule 
*mod, uint32_t offset)
 size += offset;
 
 /*
- * Free the original kernel, update the pointers to the
- * decompressed kernel
+ * In case Xen is not using the static heap feature, free the original
+ * kernel, update the pointers to the decompressed kernel
  */
-fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+if ( !xen_is_using_staticheap() )
+fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
 return 0;
 }
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 9664e85ee6c0..83c0a1480447 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -341,8 +341,12 @@ void free_init_memory(void)
 if ( rc )
 panic("Unable to remove the init section (rc = %d)\n", rc);
 
-init_domheap_pages(pa, pa + len);
-printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+if ( !xen_is_using_staticheap() )
+{
+init_domheap_pages(pa, pa + len);
+printk("Freed %ldkB init memory.\n",
+   (long)(__init_end-__init_begin) >> 10);
+}
 }
 
 /**
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca94..91340d5dc201 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,24 +206,25 @@ void __init discard_initial_modules(void)
 struct bootmodules *mi = &bootinfo.modules;
 int i;
 
-for ( i = 0; i < mi->nr_mods; i++ )
+if ( !xen_is_using_staticheap() )
 {
-paddr_t s = mi->module[i].start;
-paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
-
-if ( mi->module[i].kind == BOOTMOD_XEN )
-continue;
+for ( i = 0; i < mi->nr_mods; i++ )
+{
+paddr_t s = mi->module[i].start;
+paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-if ( !mfn_valid(maddr_to_mfn(s)) ||
- !mfn_valid(maddr_to_mfn(e)) )
-continue;
+if ( mi->module[i].kind == BOOTMOD_XEN )
+continue;
 
-fw_unreserved_regions(s, e, init_domheap_pages, 0);
-}
+if ( !mfn_valid(maddr_to_mfn(s)) ||
+ !mfn_valid(maddr_to_mfn(e)) )
+continue;
 
-mi->nr_mods = 0;
+fw_unreserved_regions(s, e, init_domheap_pages, 0);
+}
 
-remove_early_mappings();
+mi->nr_mods = 0;
+}
 }
 
 /* Relocate the FDT in Xen heap */
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..6cc9ae146a97 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -403,7 +403,9 @@ static int __init process_chosen_node(const void *fdt, int 
node,
 if ( rc )

[PATCH v2 1/4] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

When HAS_VMAP is disabled, the xv{malloc,zalloc,...} functions
should fall back to the simple x{malloc,zalloc,...} variant,
implement that because MPU systems won't have virtual memory.

Additionally remove VMAP_VIRT_START from vmap.h guards since
MPU systems won't have it defined and protect with #ifdef
CONFIG_HAS_VMAP all the declaration that won't be used in a
MPU system built without HAS_VMAP.

Signed-off-by: Luca Fancellu 
---
Changes from v1:
 - put back static inline iounmap
 - changed commit message
 - hide not used declaration for system with !HAS_VMAP
 - correct function declared in xvmalloc.h to be static inline
 - prefer '#ifdef' instead of '#if defined' where possible
---
---
 xen/include/xen/vmap.h | 61 ++
 xen/include/xen/xvmalloc.h | 36 ++
 2 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..a9f4a07bbb65 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -5,12 +5,19 @@
  * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
  * latter is used when loading livepatches and the former for everything else.
  */
-#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
+#ifndef __XEN_VMAP_H__
 #define __XEN_VMAP_H__
 
 #include 
 #include 
 
+/*
+ * MPU systems won't have HAS_VMAP enabled, but will provide implementation
+ * only for some of the functions of this module. So hide the definition for
+ * some of these function to systems where !HAS_VMAP
+ */
+#ifdef CONFIG_HAS_VMAP
+
 /* Identifiers for the linear ranges tracked by vmap */
 enum vmap_region {
 /*
@@ -68,25 +75,6 @@ void *__vmap(const mfn_t *mfn, unsigned int granularity, 
unsigned int nr,
  */
 void *vmap(const mfn_t *mfn, unsigned int nr);
 
-/*
- * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
- *
- * @param mfn Base mfn of the physical region
- * @param nr  Number of mfns in the physical region
- * @return Pointer to the mapped area on success; NULL otherwise.
- */
-void *vmap_contig(mfn_t mfn, unsigned int nr);
-
-/*
- * Unmaps a range of virtually contiguous memory from one of the vmap regions
- *
- * The system remembers internally how wide the mapping is and unmaps it all.
- * It also can determine the vmap region type from the `va`.
- *
- * @param va Virtual base address of the range to unmap
- */
-void vunmap(const void *va);
-
 /*
  * Allocate `size` octets of possibly non-contiguous physical memory and map
  * them contiguously in the VMAP_DEFAULT vmap region
@@ -112,6 +100,33 @@ void *vzalloc(size_t size);
  */
 void vfree(void *va);
 
+/* Return the number of pages in the mapping starting at address 'va' */
+unsigned int vmap_size(const void *va);
+
+/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
+void *arch_vmap_virt_end(void);
+
+#endif /* CONFIG_HAS_VMAP */
+
+/*
+ * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
+ *
+ * @param mfn Base mfn of the physical region
+ * @param nr  Number of mfns in the physical region
+ * @return Pointer to the mapped area on success; NULL otherwise.
+ */
+void *vmap_contig(mfn_t mfn, unsigned int nr);
+
+/*
+ * Unmaps a range of virtually contiguous memory from one of the vmap regions
+ *
+ * The system remembers internally how wide the mapping is and unmaps it all.
+ * It also can determine the vmap region type from the `va`.
+ *
+ * @param va Virtual base address of the range to unmap
+ */
+void vunmap(const void *va);
+
 /*
  * Analogous to vmap_contig(), but for IO memory
  *
@@ -124,9 +139,6 @@ void vfree(void *va);
  */
 void __iomem *ioremap(paddr_t pa, size_t len);
 
-/* Return the number of pages in the mapping starting at address 'va' */
-unsigned int vmap_size(const void *va);
-
 /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
 static inline void iounmap(void __iomem *va)
 {
@@ -135,9 +147,6 @@ static inline void iounmap(void __iomem *va)
 vunmap((void *)(addr & PAGE_MASK));
 }
 
-/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
-void *arch_vmap_virt_end(void);
-
 /* Initialises the VMAP_DEFAULT virtual range */
 static inline void vm_init(void)
 {
diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
index 440d85a284bb..e97a30f61e96 100644
--- a/xen/include/xen/xvmalloc.h
+++ b/xen/include/xen/xvmalloc.h
@@ -40,20 +40,46 @@
 ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
  __alignof__(typeof(*(ptr)
 
+#ifdef CONFIG_HAS_VMAP
+
 /* Free any of the above. */
 void xvfree(void *va);
 
+/* Underlying functions */
+void *_xvmalloc(size_t size, unsigned int align);
+void *_xvzalloc(size_t size, unsigned int align);
+void *_xvrealloc(void *va, size_t size, unsigned int align);
+
+#else /* !CONFIG_HAS_VMAP */
+
+static inline void xvfree(void

[PATCH v2 3/4] xen/arm: Use vmap_contig instead of __vmap where it's possible

[Luca Fancellu]

Currently the arm code uses __vmap function in few parts to map
physically contiguous pages, vmap_contig was introduced recently
and does the same because it's a wrapper for __vmap, so use the
latter instead of the direct __vmap function.

Signed-off-by: Luca Fancellu 
Acked-by: Julien Grall 
---
Changes v1:
 - Add ack-by Julien
---
---
 xen/arch/arm/alternative.c | 3 +--
 xen/arch/arm/cpuerrata.c   | 5 ++---
 xen/arch/arm/kernel.c  | 2 +-
 xen/arch/arm/livepatch.c   | 3 +--
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index d99b5070937d..fec7dbd2cde9 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -209,8 +209,7 @@ void __init apply_alternatives_all(void)
  * The text and inittext section are read-only. So re-map Xen to
  * be able to patch the code.
  */
-xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
-VMAP_DEFAULT);
+xenmap = vmap_contig(xen_mfn, 1U << xen_order);
 /* Re-mapping Xen is not expected to fail during boot. */
 BUG_ON(!xenmap);
 
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index eef9c0ea0e21..17cf134f1b0d 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -61,9 +61,8 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char 
*hyp_vec_start,
  * Vectors are part of the text that are mapped read-only. So re-map
  * the vector table to be able to update vectors.
  */
-dst_remapped = __vmap(&dst_mfn,
-  1UL << get_order_from_bytes(VECTOR_TABLE_SIZE),
-  1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+dst_remapped = vmap_contig(dst_mfn,
+   1UL << get_order_from_bytes(VECTOR_TABLE_SIZE));
 if ( !dst_remapped )
 return false;
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 669d143cee1b..293d7efaed9c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -211,7 +211,7 @@ static __init int kernel_decompress(struct bootmodule *mod, 
uint32_t offset)
 return -ENOMEM;
 }
 mfn = page_to_mfn(pages);
-output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, 
VMAP_DEFAULT);
+output = vmap_contig(mfn, 1 << kernel_order_out);
 
 rc = perform_gunzip(output, input, size);
 clean_dcache_va_range(output, output_size);
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 037746d9528d..3805b2974663 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -39,8 +39,7 @@ int arch_livepatch_quiesce(void)
  * The text section is read-only. So re-map Xen to be able to patch
  * the code.
  */
-vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, 
PAGE_HYPERVISOR,
-  VMAP_DEFAULT);
+vmap_of_xen_text = vmap_contig(text_mfn, 1U << text_order);
 
 if ( !vmap_of_xen_text )
 {
-- 
2.34.1

[PATCH v2 2/4] arm/setup: Move MMU specific extern declarations to mmu/setup.h

[Luca Fancellu]

Move some extern declarations related to MMU structures and define
from asm/setup.h to asm/mmu/setup.h, in order to increase encapsulation
and allow the MPU part to build, since it has no clue about them.

Signed-off-by: Luca Fancellu 
---
Changes from v1:
 - Moved extern to mmu/setup.h instead of mmu/mm.h
 - moved also pte_of_xenaddr()
---
---
 xen/arch/arm/include/asm/mmu/setup.h | 31 
 xen/arch/arm/include/asm/setup.h | 20 ++
 2 files changed, 37 insertions(+), 14 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/mmu/setup.h

diff --git a/xen/arch/arm/include/asm/mmu/setup.h 
b/xen/arch/arm/include/asm/mmu/setup.h
new file mode 100644
index ..3fe752b04c63
--- /dev/null
+++ b/xen/arch/arm/include/asm/mmu/setup.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __ARM_MMU_SETUP_H__
+#define __ARM_MMU_SETUP_H__
+
+#include 
+#include 
+
+extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
+
+#ifdef CONFIG_ARM_64
+extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
+#endif
+extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
+extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
+
+/* Find where Xen will be residing at runtime and return a PT entry */
+lpae_t pte_of_xenaddr(vaddr_t va);
+
+#endif /* __ARM_MMU_SETUP_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 64c227d171fc..a5a80d9b477f 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -6,6 +6,12 @@
 #include 
 #include 
 
+#if defined(CONFIG_MMU)
+# include 
+#elif !defined(CONFIG_MPU)
+# error "Unknown memory management layout"
+#endif
+
 #define MAX_FDT_SIZE SZ_2M
 
 struct map_range_data
@@ -65,20 +71,6 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
 int map_range_to_domain(const struct dt_device_node *dev,
 uint64_t addr, uint64_t len, void *data);
 
-extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
-
-#ifdef CONFIG_ARM_64
-extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
-#endif
-extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
-extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
-
-/* Find where Xen will be residing at runtime and return a PT entry */
-lpae_t pte_of_xenaddr(vaddr_t va);
-
 extern const char __ro_after_init_start[], __ro_after_init_end[];
 
 struct init_info
-- 
2.34.1

[PATCH v2 0/4] Prerequisite patches for R82 upstreaming

[Luca Fancellu]

In this serie I've taken out patches from the R82 branch already in the ML[1]
and some new patches I've done based on the current status of staging that will
not impact the current Armv8-R earlyboot work.

[1] 
https://patchwork.kernel.org/project/xen-devel/cover/20230626033443.2943270-1-penny.zh...@arm.com/

Luca Fancellu (3):
  common/vmap: Fall back to simple allocator when !HAS_VMAP
  arm/setup: Move MMU specific extern declarations to mmu/setup.h
  xen/arm: Use vmap_contig instead of __vmap where it's possible

Penny Zheng (1):
  xen/arm: do not give memory back to static heap

 xen/arch/arm/alternative.c   |  3 +-
 xen/arch/arm/arm32/mmu/mm.c  |  4 +-
 xen/arch/arm/cpuerrata.c |  5 +--
 xen/arch/arm/include/asm/mmu/setup.h | 31 ++
 xen/arch/arm/include/asm/setup.h | 20 +++--
 xen/arch/arm/kernel.c|  9 ++--
 xen/arch/arm/livepatch.c |  3 +-
 xen/arch/arm/mmu/setup.c |  8 +++-
 xen/arch/arm/setup.c | 27 ++--
 xen/common/device-tree/bootfdt.c |  4 +-
 xen/common/device-tree/bootinfo.c|  2 +-
 xen/common/page_alloc.c  |  5 +++
 xen/include/xen/bootfdt.h| 14 ++-
 xen/include/xen/vmap.h   | 61 
 xen/include/xen/xvmalloc.h   | 36 +---
 15 files changed, 156 insertions(+), 76 deletions(-)
 create mode 100644 xen/arch/arm/include/asm/mmu/setup.h

-- 
2.34.1

Re: [PATCH] bootfdt: Unify early printing of memory ranges endpoints

[Luca Fancellu]

Hi Michal,

> On 19 Nov 2024, at 11:51, Michal Orzel  wrote:
> 
> At the moment, when printing memory ranges during early boot, endpoints
> of some ranges are printed as inclusive (RAM, RESVD, SHMEM) and some
> as exclusive (Initrd, MODULE). Make the behavior consistent and print
> all the endpoints as inclusive.
> 
> Signed-off-by: Michal Orzel 
> ---

Looks good to me!

Reviewed-by: Luca Fancellu 

Outside early boot there are other places where we print exclusive ranges,
do you know if there is any general style we should apply or does it depend on
case by case?

Cheers,
Luca

Re: [PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

Hi Andrew,

 +
 +void *_xvrealloc(void *va, size_t size, unsigned int align)
 +{
 +return _xrealloc(va, size, align);
 +}
 +
 +#endif
>>> Does this really compile with the wrappers not being static inline ?
>>> 
>>> That aside, could we not do this using conditional aliases, rather than
>>> wrapping the functions?  It would certainly be shorter, code wise.
>> Do you mean something like below?
>> 
>> #define xvfree xfree
>> #define _xvmalloc _xmalloc
>> […]
> 
> I mean __attribute__((__alias__("")))
> 
> There are two examples in tree already.  See efi_compat_get_info() being
> aliased to efi_get_info()
> 
> In this case, in the !HAS_VMAP case, we'd just declare _xmalloc() to
> have an alias called _xvmalloc() too.
> 
> This avoids needing to wrap every function in the headers.

I’m getting:

error: ‘xvfree’ aliased to undefined symbol ‘xfree’

looking into the documentation it says:

“It is an error if the alias target is not defined in the same translation unit 
as the alias."

So I think I can’t use this one here.

Should I continue to do in the other way? Or using #define?

> 
> ~Andrew

[PATCH 2/5] arm/setup: Move MMU specific extern declarations to mmu/mm.h

[Luca Fancellu]

Move some extern declarations related to MMU structures and define
from asm/setup.h to asm/mm.h, in order to increase encapsulation and
allow the MPU part to build, since it has no clue about them.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/include/asm/mmu/mm.h | 11 +++
 xen/arch/arm/include/asm/setup.h  | 11 ---
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/include/asm/mmu/mm.h 
b/xen/arch/arm/include/asm/mmu/mm.h
index c5e03a66bf9e..69b72d671012 100644
--- a/xen/arch/arm/include/asm/mmu/mm.h
+++ b/xen/arch/arm/include/asm/mmu/mm.h
@@ -12,6 +12,17 @@ extern vaddr_t directmap_virt_start;
 extern unsigned long directmap_base_pdx;
 #endif
 
+extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
+
+#ifdef CONFIG_ARM_64
+extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
+#endif
+extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
+extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
+extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
+
 #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
 
 /*
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 64c227d171fc..3f5c6cf9a08b 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -65,17 +65,6 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
 int map_range_to_domain(const struct dt_device_node *dev,
 uint64_t addr, uint64_t len, void *data);
 
-extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
-
-#ifdef CONFIG_ARM_64
-extern lpae_t boot_first[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_first_id[XEN_PT_LPAE_ENTRIES];
-#endif
-extern lpae_t boot_second[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_second_id[XEN_PT_LPAE_ENTRIES];
-extern lpae_t boot_third[XEN_PT_LPAE_ENTRIES * XEN_NR_ENTRIES(2)];
-extern lpae_t boot_third_id[XEN_PT_LPAE_ENTRIES];
-
 /* Find where Xen will be residing at runtime and return a PT entry */
 lpae_t pte_of_xenaddr(vaddr_t va);
 
-- 
2.34.1

[PATCH 0/5] Prerequisite patches for R82 upstreaming

[Luca Fancellu]

In this serie I've taken out patches from the R82 branch already in the ML[1]
and some new patches I've done based on the current status of staging that will
not impact the current Armv8-R earlyboot work.

[1] 
https://patchwork.kernel.org/project/xen-devel/cover/20230626033443.2943270-1-penny.zh...@arm.com/

Luca Fancellu (3):
  common/vmap: Fall back to simple allocator when !HAS_VMAP
  arm/setup: Move MMU specific extern declarations to mmu/mm.h
  xen/arm: Use vmap_contig instead of __vmap where it's possible

Penny Zheng (2):
  xen/arm: only map the init text section RW in free_init_memory
  xen/arm: do not give memory back to static heap

 xen/arch/arm/alternative.c   |  3 +--
 xen/arch/arm/arm32/mmu/mm.c  |  4 ++--
 xen/arch/arm/cpuerrata.c |  5 ++--
 xen/arch/arm/include/asm/mmu/mm.h| 11 +
 xen/arch/arm/include/asm/setup.h | 11 -
 xen/arch/arm/kernel.c|  5 ++--
 xen/arch/arm/livepatch.c |  3 +--
 xen/arch/arm/mmu/setup.c | 16 +
 xen/arch/arm/setup.c | 27 +++--
 xen/common/device-tree/bootfdt.c |  2 +-
 xen/common/device-tree/bootinfo.c|  2 +-
 xen/common/device-tree/device-tree.c |  3 +++
 xen/common/vmap.c|  7 ++
 xen/include/xen/bootfdt.h| 11 -
 xen/include/xen/vmap.h   |  9 ++-
 xen/include/xen/xvmalloc.h   | 36 
 16 files changed, 100 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH 4/5] xen/arm: Use vmap_contig instead of __vmap where it's possible

[Luca Fancellu]

Currently the arm code uses __vmap function in few parts to map
physically contiguous pages, vmap_contig was introduced recently
and does the same because it's a wrapper for __vmap, so use the
latter instead of the direct __vmap function.

Signed-off-by: Luca Fancellu 
---
 xen/arch/arm/alternative.c | 3 +--
 xen/arch/arm/cpuerrata.c   | 5 ++---
 xen/arch/arm/kernel.c  | 2 +-
 xen/arch/arm/livepatch.c   | 3 +--
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index d99b5070937d..fec7dbd2cde9 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -209,8 +209,7 @@ void __init apply_alternatives_all(void)
  * The text and inittext section are read-only. So re-map Xen to
  * be able to patch the code.
  */
-xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
-VMAP_DEFAULT);
+xenmap = vmap_contig(xen_mfn, 1U << xen_order);
 /* Re-mapping Xen is not expected to fail during boot. */
 BUG_ON(!xenmap);
 
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index eef9c0ea0e21..17cf134f1b0d 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -61,9 +61,8 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char 
*hyp_vec_start,
  * Vectors are part of the text that are mapped read-only. So re-map
  * the vector table to be able to update vectors.
  */
-dst_remapped = __vmap(&dst_mfn,
-  1UL << get_order_from_bytes(VECTOR_TABLE_SIZE),
-  1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
+dst_remapped = vmap_contig(dst_mfn,
+   1UL << get_order_from_bytes(VECTOR_TABLE_SIZE));
 if ( !dst_remapped )
 return false;
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 669d143cee1b..293d7efaed9c 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -211,7 +211,7 @@ static __init int kernel_decompress(struct bootmodule *mod, 
uint32_t offset)
 return -ENOMEM;
 }
 mfn = page_to_mfn(pages);
-output = __vmap(&mfn, 1 << kernel_order_out, 1, 1, PAGE_HYPERVISOR, 
VMAP_DEFAULT);
+output = vmap_contig(mfn, 1 << kernel_order_out);
 
 rc = perform_gunzip(output, input, size);
 clean_dcache_va_range(output, output_size);
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 037746d9528d..3805b2974663 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -39,8 +39,7 @@ int arch_livepatch_quiesce(void)
  * The text section is read-only. So re-map Xen to be able to patch
  * the code.
  */
-vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, 
PAGE_HYPERVISOR,
-  VMAP_DEFAULT);
+vmap_of_xen_text = vmap_contig(text_mfn, 1U << text_order);
 
 if ( !vmap_of_xen_text )
 {
-- 
2.34.1

[PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

When HAS_VMAP is disabled, the xv{malloc,zalloc,...} functions
should fall back to the simple x{malloc,zalloc,...} variant,
implement that because MPU systems won't have virtual memory.

Additionally remove VMAP_VIRT_START from vmap.h guards since
MPU systems won't have it defined and move iounmap function
to the vmap.c file, because it uses the vunmap function that
won't be compiled in when !HAS_VMAP.

Signed-off-by: Luca Fancellu 
---
This is a rework of this one: 
https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-16-penny.zh...@arm.com/
where I hope I've understood correctly what Jan Beulich was suggesting here:
https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-16-penny.zh...@arm.com/#25409119
---
 xen/common/vmap.c  |  7 +++
 xen/include/xen/vmap.h |  9 ++---
 xen/include/xen/xvmalloc.h | 36 +++-
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index 47225fecc067..294280dcd08c 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -426,3 +426,10 @@ void *_xvrealloc(void *va, size_t size, unsigned int align)
 
 return ptr;
 }
+
+void iounmap(void __iomem *va)
+{
+unsigned long addr = (unsigned long)(void __force *)va;
+
+vunmap((void *)(addr & PAGE_MASK));
+}
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..9e1d794c2548 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -5,7 +5,7 @@
  * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
  * latter is used when loading livepatches and the former for everything else.
  */
-#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
+#if !defined(__XEN_VMAP_H__)
 #define __XEN_VMAP_H__
 
 #include 
@@ -128,12 +128,7 @@ void __iomem *ioremap(paddr_t pa, size_t len);
 unsigned int vmap_size(const void *va);
 
 /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
-static inline void iounmap(void __iomem *va)
-{
-unsigned long addr = (unsigned long)(void __force *)va;
-
-vunmap((void *)(addr & PAGE_MASK));
-}
+void iounmap(void __iomem *va);
 
 /* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
 void *arch_vmap_virt_end(void);
diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
index 440d85a284bb..802be6687085 100644
--- a/xen/include/xen/xvmalloc.h
+++ b/xen/include/xen/xvmalloc.h
@@ -40,20 +40,46 @@
 ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
  __alignof__(typeof(*(ptr)
 
+#if defined(CONFIG_HAS_VMAP)
+
 /* Free any of the above. */
 void xvfree(void *va);
 
+/* Underlying functions */
+void *_xvmalloc(size_t size, unsigned int align);
+void *_xvzalloc(size_t size, unsigned int align);
+void *_xvrealloc(void *va, size_t size, unsigned int align);
+
+#else
+
+static inline void xvfree(void *va)
+{
+xfree(va);
+}
+
+void *_xvmalloc(size_t size, unsigned int align)
+{
+return _xmalloc(size, align);
+}
+
+void *_xvzalloc(size_t size, unsigned int align)
+{
+return _xzalloc(size, align);
+}
+
+void *_xvrealloc(void *va, size_t size, unsigned int align)
+{
+return _xrealloc(va, size, align);
+}
+
+#endif
+
 /* Free an allocation, and zero the pointer to it. */
 #define XVFREE(p) do { \
 xvfree(p); \
 (p) = NULL;\
 } while ( false )
 
-/* Underlying functions */
-void *_xvmalloc(size_t size, unsigned int align);
-void *_xvzalloc(size_t size, unsigned int align);
-void *_xvrealloc(void *va, size_t size, unsigned int align);
-
 static inline void *_xvmalloc_array(
 size_t size, unsigned int align, unsigned long num)
 {
-- 
2.34.1

[PATCH 5/5] xen/arm: do not give memory back to static heap

[Luca Fancellu]

From: Penny Zheng 

If Xenheap is statically configured in Device Tree, its size is
definite. So, the memory shall not be given back into static heap, like
it's normally done in free_init_memory, etc, once the initialization
is finished.

Extract static_heap flag from init data bootinfo, as it will be needed
after destroying the init data section.
Introduce a new helper xen_is_using_staticheap() to tell whether Xenheap
is statically configured in the Device Tree.

Signed-off-by: Luca Fancellu 
---
This is a rebase of this one: 
https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-18-penny.zh...@arm.com/
---
 xen/arch/arm/arm32/mmu/mm.c  |  4 ++--
 xen/arch/arm/kernel.c|  3 ++-
 xen/arch/arm/mmu/setup.c |  8 ++--
 xen/arch/arm/setup.c | 27 ++-
 xen/common/device-tree/bootfdt.c |  2 +-
 xen/common/device-tree/bootinfo.c|  2 +-
 xen/common/device-tree/device-tree.c |  3 +++
 xen/include/xen/bootfdt.h| 11 ++-
 8 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 063611412be0..b7ca7c94c9ca 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -199,7 +199,7 @@ void __init setup_mm(void)
 
 total_pages = ram_size >> PAGE_SHIFT;
 
-if ( bootinfo.static_heap )
+if ( xen_is_using_staticheap() )
 {
 const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
 
@@ -246,7 +246,7 @@ void __init setup_mm(void)
 
 do
 {
-e = bootinfo.static_heap ?
+e = xen_is_using_staticheap() ?
 fit_xenheap_in_static_heap(pfn_to_paddr(xenheap_pages), MB(32)) :
 consider_modules(ram_start, ram_end,
  pfn_to_paddr(xenheap_pages),
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 293d7efaed9c..a4a99607b668 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -247,7 +247,8 @@ static __init int kernel_decompress(struct bootmodule *mod, 
uint32_t offset)
  * Free the original kernel, update the pointers to the
  * decompressed kernel
  */
-fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
+if ( !xen_is_using_staticheap() )
+fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
 
 return 0;
 }
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 1b1d302c8788..d0775793f4b4 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -343,8 +343,12 @@ void free_init_memory(void)
 if ( rc )
 panic("Unable to remove the init section (rc = %d)\n", rc);
 
-init_domheap_pages(pa, pa + len);
-printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);
+if ( !xen_is_using_staticheap() )
+{
+init_domheap_pages(pa, pa + len);
+printk("Freed %ldkB init memory.\n",
+   (long)(__init_end-__init_begin) >> 10);
+}
 }
 
 /**
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 71ebaa77ca94..91340d5dc201 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,24 +206,25 @@ void __init discard_initial_modules(void)
 struct bootmodules *mi = &bootinfo.modules;
 int i;
 
-for ( i = 0; i < mi->nr_mods; i++ )
+if ( !xen_is_using_staticheap() )
 {
-paddr_t s = mi->module[i].start;
-paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
-
-if ( mi->module[i].kind == BOOTMOD_XEN )
-continue;
+for ( i = 0; i < mi->nr_mods; i++ )
+{
+paddr_t s = mi->module[i].start;
+paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
 
-if ( !mfn_valid(maddr_to_mfn(s)) ||
- !mfn_valid(maddr_to_mfn(e)) )
-continue;
+if ( mi->module[i].kind == BOOTMOD_XEN )
+continue;
 
-fw_unreserved_regions(s, e, init_domheap_pages, 0);
-}
+if ( !mfn_valid(maddr_to_mfn(s)) ||
+ !mfn_valid(maddr_to_mfn(e)) )
+continue;
 
-mi->nr_mods = 0;
+fw_unreserved_regions(s, e, init_domheap_pages, 0);
+}
 
-remove_early_mappings();
+mi->nr_mods = 0;
+}
 }
 
 /* Relocate the FDT in Xen heap */
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..ccb150b34a63 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -403,7 +403,7 @@ static int __init process_chosen_node(const void *fdt, int 
node,
 if ( rc )
 return rc;
 
-bootinfo.static_heap = true;
+static_heap = true;
 }
 
 printk("Checking for initrd in /chosen\n");
diff --git a/xen/common/device-tree/bootinfo.c 
b/xen/common/device-tree/bootinfo.c
index

[PATCH 3/5] xen/arm: only map the init text section RW in free_init_memory

[Luca Fancellu]

From: Penny Zheng 

In free_init_memory, we do not need to map the whole init section RW,
as only init text section is mapped RO in boot time.

Signed-off-by: Luca Fancellu 
---
This is this one: 
https://patchwork.kernel.org/project/xen-devel/patch/20230626033443.2943270-19-penny.zh...@arm.com/
---
 xen/arch/arm/mmu/setup.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 9664e85ee6c0..1b1d302c8788 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -7,6 +7,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -309,16 +310,17 @@ void *__init arch_vmap_virt_end(void)
 void free_init_memory(void)
 {
 paddr_t pa = virt_to_maddr(__init_begin);
+unsigned long inittext_end = round_pgup((unsigned long)_einittext);
 unsigned long len = __init_end - __init_begin;
 uint32_t insn;
 unsigned int i, nr = len / sizeof(insn);
 uint32_t *p;
 int rc;
 
-rc = modify_xen_mappings((unsigned long)__init_begin,
- (unsigned long)__init_end, PAGE_HYPERVISOR_RW);
+rc = modify_xen_mappings((unsigned long)__init_begin, inittext_end,
+ PAGE_HYPERVISOR_RW);
 if ( rc )
-panic("Unable to map RW the init section (rc = %d)\n", rc);
+panic("Unable to map RW the init text section (rc = %d)\n", rc);
 
 /*
  * From now on, init will not be used for execution anymore,
-- 
2.34.1

Re: [PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

> On 15 Nov 2024, at 12:00, Andrew Cooper  wrote:
> 
> On 15/11/2024 10:50 am, Luca Fancellu wrote:
>> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
>> index 440d85a284bb..802be6687085 100644
>> --- a/xen/include/xen/xvmalloc.h
>> +++ b/xen/include/xen/xvmalloc.h
>> @@ -40,20 +40,46 @@
>> ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
>>  __alignof__(typeof(*(ptr)
>> 
>> +#if defined(CONFIG_HAS_VMAP)
>> +
>> /* Free any of the above. */
>> void xvfree(void *va);
>> 
>> +/* Underlying functions */
>> +void *_xvmalloc(size_t size, unsigned int align);
>> +void *_xvzalloc(size_t size, unsigned int align);
>> +void *_xvrealloc(void *va, size_t size, unsigned int align);
>> +
>> +#else
>> +
>> +static inline void xvfree(void *va)
>> +{
>> +xfree(va);
>> +}
>> +
>> +void *_xvmalloc(size_t size, unsigned int align)
>> +{
>> +return _xmalloc(size, align);
>> +}
>> +
>> +void *_xvzalloc(size_t size, unsigned int align)
>> +{
>> +return _xzalloc(size, align);
>> +}
>> +
>> +void *_xvrealloc(void *va, size_t size, unsigned int align)
>> +{
>> +return _xrealloc(va, size, align);
>> +}
>> +
>> +#endif
> 
> Does this really compile with the wrappers not being static inline ?
> 
> That aside, could we not do this using conditional aliases, rather than
> wrapping the functions?  It would certainly be shorter, code wise.

Do you mean something like below?

#define xvfree xfree
#define _xvmalloc _xmalloc
[…]

> 
> ~Andrew

Re: [PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

> On 15 Nov 2024, at 12:00, Andrew Cooper  wrote:
> 
> On 15/11/2024 10:50 am, Luca Fancellu wrote:
>> diff --git a/xen/include/xen/xvmalloc.h b/xen/include/xen/xvmalloc.h
>> index 440d85a284bb..802be6687085 100644
>> --- a/xen/include/xen/xvmalloc.h
>> +++ b/xen/include/xen/xvmalloc.h
>> @@ -40,20 +40,46 @@
>> ((typeof(ptr))_xvrealloc(ptr, offsetof(typeof(*(ptr)), field[nr]), \
>>  __alignof__(typeof(*(ptr)
>> 
>> +#if defined(CONFIG_HAS_VMAP)
>> +
>> /* Free any of the above. */
>> void xvfree(void *va);
>> 
>> +/* Underlying functions */
>> +void *_xvmalloc(size_t size, unsigned int align);
>> +void *_xvzalloc(size_t size, unsigned int align);
>> +void *_xvrealloc(void *va, size_t size, unsigned int align);
>> +
>> +#else
>> +
>> +static inline void xvfree(void *va)
>> +{
>> +xfree(va);
>> +}
>> +
>> +void *_xvmalloc(size_t size, unsigned int align)
>> +{
>> +return _xmalloc(size, align);
>> +}
>> +
>> +void *_xvzalloc(size_t size, unsigned int align)
>> +{
>> +return _xzalloc(size, align);
>> +}
>> +
>> +void *_xvrealloc(void *va, size_t size, unsigned int align)
>> +{
>> +return _xrealloc(va, size, align);
>> +}
>> +
>> +#endif
> 
> Does this really compile with the wrappers not being static inline ?

wow, yes it is compiling and I’m surprised about that, this was clearly a 
mistake, I’ll fix

> 
> That aside, could we not do this using conditional aliases, rather than
> wrapping the functions?  It would certainly be shorter, code wise.

I’ll ping you to understand better what you mean here


> 
> ~Andrew

Re: [PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

>> 
>> Sure, I’ll wrap what can’t be used in MPU case with HAS_VMAP, I would like 
>> to keep out:
>> 
>> void *vmap_contig(mfn_t mfn, unsigned int nr);
>> 
>> void vunmap(const void *va);
>> 
>> void __iomem *ioremap(paddr_t pa, size_t len);
>> 
>> static inline void iounmap(void __iomem *va)
>> 
>> static inline void vm_init(void)
>> 
>> In order to don’t put too many #ifdef, are you ok if I move the declarations 
>> in order to have these close to each other. like below:
> 
> Some re-arrangement ought to be fine, especially when the #ifdef is
> accompanied by a comment. I can't see how there can be #else though.

Yes right, clearly not tested, thanks I’ll do the changes

> 
> Jan

Re: [PATCH 1/5] common/vmap: Fall back to simple allocator when !HAS_VMAP

[Luca Fancellu]

> On 15 Nov 2024, at 11:12, Jan Beulich  wrote:
> 
> On 15.11.2024 11:50, Luca Fancellu wrote:
>> --- a/xen/common/vmap.c
>> +++ b/xen/common/vmap.c
>> @@ -426,3 +426,10 @@ void *_xvrealloc(void *va, size_t size, unsigned int 
>> align)
>> 
>> return ptr;
>> }
>> +
>> +void iounmap(void __iomem *va)
>> +{
>> +unsigned long addr = (unsigned long)(void __force *)va;
>> +
>> +vunmap((void *)(addr & PAGE_MASK));
>> +}
> 
> Why is this being moved here, and converted from inline to out-of-line?
> What the description says is insufficient imo, as even if you mean to
> only support vmap_contig() and ioremap() on MPU systems, you'll still
> need both vunmap() and iounmap().
> 
> Plus, if it really needs converting, I don't think it should live at the
> very end of the file, past _xvmalloc() and friends. Better suitable places
> may then be next to vunmap() itself, or between vfree() and xvfree().

I’ll try to keep it as it was originally, I gave a brief look into the R82 
branch and it should be fine.
I’m planning to define vmap_config(), vunmap(), ioremap(), iounmap() in a 
vmap-mpu.c under arch/arm/mpu

> 
>> --- a/xen/include/xen/vmap.h
>> +++ b/xen/include/xen/vmap.h
>> @@ -5,7 +5,7 @@
>>  * purpose area (VMAP_DEFAULT) and a livepatch-specific area (VMAP_XEN). The
>>  * latter is used when loading livepatches and the former for everything 
>> else.
>>  */
>> -#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
>> +#if !defined(__XEN_VMAP_H__)
>> #define __XEN_VMAP_H__
> 
> With this adjustment, where are the functions defined that you "unhide"
> the declarations of, in the MPU case? As you say in the description,
> vmap.c won't be built in that case.

Sure, I’ll wrap what can’t be used in MPU case with HAS_VMAP, I would like to 
keep out:

void *vmap_contig(mfn_t mfn, unsigned int nr);

void vunmap(const void *va);

void __iomem *ioremap(paddr_t pa, size_t len);

static inline void iounmap(void __iomem *va)

static inline void vm_init(void)

In order to don’t put too many #ifdef, are you ok if I move the declarations in 
order to have these close to each other. like below:

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index c1dd7ac22f30..940b7655ed8f 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+#ifdef CONFIG_HAS_VMAP
+
 /* Identifiers for the linear ranges tracked by vmap */
 enum vmap_region {
 /*
@@ -68,25 +70,6 @@ void *__vmap(const mfn_t *mfn, unsigned int granularity, 
unsigned int nr,
  */
 void *vmap(const mfn_t *mfn, unsigned int nr);
 
-/*
- * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
- *
- * @param mfn Base mfn of the physical region
- * @param nr  Number of mfns in the physical region
- * @return Pointer to the mapped area on success; NULL otherwise.
- */
-void *vmap_contig(mfn_t mfn, unsigned int nr);
-
-/*
- * Unmaps a range of virtually contiguous memory from one of the vmap regions
- *
- * The system remembers internally how wide the mapping is and unmaps it all.
- * It also can determine the vmap region type from the `va`.
- *
- * @param va Virtual base address of the range to unmap
- */
-void vunmap(const void *va);
-
 /*
  * Allocate `size` octets of possibly non-contiguous physical memory and map
  * them contiguously in the VMAP_DEFAULT vmap region
@@ -112,6 +95,33 @@ void *vzalloc(size_t size);
  */
 void vfree(void *va);
 
+/* Return the number of pages in the mapping starting at address 'va' */
+unsigned int vmap_size(const void *va);
+
+/* Pointer to 1 octet past the end of the VMAP_DEFAULT virtual area */
+void *arch_vmap_virt_end(void);
+
+#else /* !CONFIG_HAS_VMAP */
+
+/*
+ * Maps physically contiguous pages onto the VMAP_DEFAULT vmap region
+ *
+ * @param mfn Base mfn of the physical region
+ * @param nr  Number of mfns in the physical region
+ * @return Pointer to the mapped area on success; NULL otherwise.
+ */
+void *vmap_contig(mfn_t mfn, unsigned int nr);
+
+/*
+ * Unmaps a range of virtually contiguous memory from one of the vmap regions
+ *
+ * The system remembers internally how wide the mapping is and unmaps it all.
+ * It also can determine the vmap region type from the `va`.
+ *
+ * @param va Virtual base address of the range to unmap
+ */
+void vunmap(const void *va);
+
 /*
  * Analogous to vmap_contig(), but for IO memory
  *
@@ -124,9 +134,6 @@ void vfree(void *va);
  */
 void __iomem *ioremap(paddr_t pa, size_t len);
 
-/* Return the number of pages in the mapping starting at address 'va' */
-unsigned int vmap_size(const void *va);
-
 /* Analogous to vunmap(), but for IO memory mapped via ioremap() */
 static inline void iounmap(void __iomem *va)
 {
@@ -135,9 +142,6 @@ static inline void ioun

Re: [PATCH 5/5] xen/arm: do not give memory back to static heap

[Luca Fancellu]

Hi Julien,

>> -fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0);
>> +if ( !xen_is_using_staticheap() )
> 
> The comment on top needs to be updated.

I’ll update, is this ok:

/*
 * In case Xen is not using the static heap feature, free the original
 * kernel, update the pointers to the decompressed kernel
 */

>> 
>> diff --git a/xen/common/device-tree/device-tree.c 
>> b/xen/common/device-tree/device-tree.c
>> index d0528c582565..22b69c49171b 100644
>> --- a/xen/common/device-tree/device-tree.c
>> +++ b/xen/common/device-tree/device-tree.c
>> @@ -25,6 +25,9 @@
>>  #include 
>>  #include 
>>  +/* Flag saved when Xen is using the static heap feature (xen,static-heap) 
>> */
>> +bool __read_mostly static_heap;
> 
> Strictly speaking, static_heap could be used with ACPI (even though there is 
> not binding today). So I think it should not belong to device-tree.c. I think 
> page_alloc.c may be more suitable. Also, I think static_heap will not be 
> touched after init. So this likely wants to be __ro_after_init.

Sure, I’ll do the modifications and I’ll move to common/page_alloc.c

> 
> Lastly, shouldn't this be protected by #ifdef? Otherwise...

Could you clarify if I understood correctly?

If I protect static_heap with CONFIG_STATIC_MEMORY, then I have to protect also 
the code in 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/common/device-tree/bootfdt.c;h=927f59c64b0d64842e2a0fd09562ac919c204e6e;hb=refs/heads/staging#l393,
is this what you are expecting?

And in that case, should it be only to protect the access to the variable or 
the all block? For example now if CONFIG_STATIC_MEMORY is not set, I think we 
parse anyway the xen,static-mem, so this tends me to think I should protect 
only the variable.

Cheers,
Luca

Re: [PATCH 2/5] arm/setup: Move MMU specific extern declarations to mmu/mm.h

[Luca Fancellu]

Hi Julien,

> On 17 Nov 2024, at 17:46, Julien Grall  wrote:
> 
> Hi Luca,
> 
> On 15/11/2024 10:50, Luca Fancellu wrote:
>> Move some extern declarations related to MMU structures and define
>> from asm/setup.h to asm/mm.h, in order to increase encapsulation and
> 
> You are moving them to asm/mmu/mm.h. But I think I would prefer if they are 
> moved to asm/mmu/setup.h because boot_* are not supposed to be used outside 
> of boot. So it is clearer if they are still defined in a setup.h.

Sure I’ll move them to arm/mmu/setup.h

>> 
>> -
>>  /* Find where Xen will be residing at runtime and return a PT entry */
>>  lpae_t pte_of_xenaddr(vaddr_t va);
> 
> Shouldn't we move this function as well?

This one was not problematic, but I can move this one as well.

Cheers,
Luca

Re: [PATCH 3/5] xen/arm: only map the init text section RW in free_init_memory

[Luca Fancellu]

Hi Julien,

> On 17 Nov 2024, at 17:58, Julien Grall  wrote:
> 
> Hi Luca,
> 
> On 15/11/2024 10:50, Luca Fancellu wrote:
>> From: Penny Zheng 
>> In free_init_memory, we do not need to map the whole init section RW,
>> as only init text section is mapped RO in boot time.
> 
> So originally, this was done because the function was generic. But now this 
> is MMU specific, we don't really gain that much during boot but will impair 
> any work that would restrict some init further (for instance .init.rodata 
> could be RO). So is it actually worth it?

Probably not, I’ll drop this one.

> 
>> Signed-off-by: Luca Fancellu 
> 
> Given the link below, why are Penny and Wei's signed-off-by are missing?

Internal policy, authorship is preserved though.

Cheers,
Luca

Re: [PATCH v2 4/4] xen/arm: do not give memory back to static heap

[Luca Fancellu]

> On 26 Nov 2024, at 13:29, Jan Beulich  wrote:
> 
> On 26.11.2024 14:25, Luca Fancellu wrote:
>>> This reads better, thanks. Follow-on question: Is what is statically
>>> configured for the heap guaranteed to never overlap with anything passed
>>> to init_domheap_pages() in those places that you touch?
>> 
>> I think so, the places of the check are mainly memory regions related to 
>> boot modules,
>> when we add a boot module we also do a check in order to see if it clashes 
>> with any
>> reserved regions already defined, which the static heap is part of.
>> 
>> Could you explain me why the question?
> 
> Well, if there was a chance of overlap, then parts of the free region would
> need to go one way, and the rest the other way.

oh ok, sure of course, thanks for answering.

> 
>>>>>> --- a/xen/include/xen/bootfdt.h
>>>>>> +++ b/xen/include/xen/bootfdt.h
>>>>>> @@ -132,7 +132,6 @@ struct bootinfo {
>>>>>> #ifdef CONFIG_STATIC_SHM
>>>>>>   struct shared_meminfo shmem;
>>>>>> #endif
>>>>>> -bool static_heap;
>>>>>> };
>>>>>> 
>>>>>> #ifdef CONFIG_ACPI
>>>>>> @@ -157,6 +156,10 @@ struct bootinfo {
>>>>>> 
>>>>>> extern struct bootinfo bootinfo;
>>>>>> 
>>>>>> +#ifdef CONFIG_STATIC_MEMORY
>>>>>> +extern bool static_heap;
>>>>>> +#endif
>>>>> 
>>>>> Just to double check Misra-wise: Is there a guarantee that this header 
>>>>> will
>>>>> always be included by page-alloc.c, so that the definition of the symbol
>>>>> has an earlier declaration already visible? I ask because this header
>>>>> doesn't look like one where symbols defined in page-alloc.c would normally
>>>>> be declared. And I sincerely hope that this header isn't one of those that
>>>>> end up being included virtually everywhere.
>>>> 
>>>> I’ve read again MISRA rule 8.4 and you are right, I should have included 
>>>> bootfdt.h in
>>>> page-alloc.c in order to have the declaration visible before defining 
>>>> static_heap.
>>>> 
>>>> I will include the header in page-alloc.c
>>> 
>>> Except that, as said, I don't think that header should be included in this 
>>> file.
>>> Instead I think the declaration wants to move elsewhere.
>> 
>> Ok sorry, I misunderstood your comment, I thought you were suggesting to 
>> have the
>> declaration visible in that file since we are defining there the variable.
>> 
>> So Julien suggested that file, it was hosted before in 
>> common/device-tree/device-tree.c,
>> see the comment here:
>> https://patchwork.kernel.org/project/xen-devel/patch/20241115105036.218418-6-luca.fance...@arm.com/#26125054
>> 
>> Since it seems you disagree with Julien, could you suggest a more suitable 
>> place?
> 
> Anything defined in page-alloc.c should by default have its declaration in
> xen/mm.h, imo. Exceptions would need justification.

I would be fine to have the declaration in xen/mm.h, I just need to import 
xen/mm.h in bootfdt.h so that it is visible to
“using_static_heap”, @Julien would you be ok with that?

> 
> Obviously a possible alternative is to move the definition, not the 
> declaration.
> 
> Jan

Cheers,
Luca

[PATCH v2 0/6] Boot time cpupools

[Luca Fancellu]

This serie introduces a feature for Xen to create cpu pools at boot time, the
feature is enabled using a configurable that is disabled by default.
The boot time cpupool feature relies on the device tree to describe the cpu
pools.
Another feature is introduced by the serie, the possibility to assign a
dom0less guest to a cpupool at boot time.

Here follows an example, Xen is built with CONFIG_BOOT_TIME_CPUPOOLS=y.

>From the DT:

  [...]

  a72_0: cpu@0 {
compatible = "arm,cortex-a72";
reg = <0x0 0x0>;
device_type = "cpu";
[...]
  };

  a72_1: cpu@1 {
compatible = "arm,cortex-a72";
reg = <0x0 0x1>;
device_type = "cpu";
[...]
  };

  a53_0: cpu@100 {
compatible = "arm,cortex-a53";
reg = <0x0 0x100>;
device_type = "cpu";
[...]
  };

  a53_1: cpu@101 {
compatible = "arm,cortex-a53";
reg = <0x0 0x101>;
device_type = "cpu";
[...]
  };

  a53_2: cpu@102 {
compatible = "arm,cortex-a53";
reg = <0x0 0x102>;
device_type = "cpu";
[...]
  };

  a53_3: cpu@103 {
compatible = "arm,cortex-a53";
reg = <0x0 0x103>;
device_type = "cpu";
[...]
  };

  chosen {
#size-cells = <0x1>;
#address-cells = <0x1>;
xen,dom0-bootargs = "...";
xen,xen-bootargs = "...";

s1: sched_a {
compatible = "xen,scheduler";
sched-name = "credit2";
};
s2: sched_b {
compatible = "xen,scheduler";
sched-name = "null";
};

cpupool0 {
  compatible = "xen,cpupool";
  cpupool-cpus = <&a72_0 &a72_1>;
  cpupool-sched = <&1>;
};

cp1: cpupool1 {
  compatible = "xen,cpupool";
  cpupool-cpus = <&a53_0 &a53_1 &a53_2 &a53_3>;
  cpupool-sched = <&s2>;
};

module@0 {
  reg = <0x8008 0x130>;
  compatible = "multiboot,module";
};

domU1 {
  #size-cells = <0x1>;
  #address-cells = <0x1>;
  compatible = "xen,domain";
  cpus = <1>;
  memory = <0 0xC>;
  vpl011;
  domain-cpupool = <&cp1>;

  module@9200 {
compatible = "multiboot,kernel", "multiboot,module";
reg = <0x9200 0x1ff>;
bootargs = "...";
  };
};
  };

  [...]

The example DT is instructing Xen to have two cpu pools, the one with id 0
having two phisical cpus and the one with id 1 having 4 phisical cpu, the
second cpu pool uses the null scheduler and from the /chosen node we can see
that a dom0less guest will be started on that cpu pool.

In this particular case Xen must boot with different type of cpus, so the
boot argument hmp_unsafe must be enabled.


Luca Fancellu (6):
  tools/cpupools: Give a name to unnamed cpupools
  xen/sched: create public function for cpupools creation
  xen/sched: retrieve scheduler id by name
  xen/cpupool: Create different cpupools at boot time
  arm/dom0less: assign dom0less guests to cpupools
  xen/cpupool: Allow cpupool0 to use different scheduler

 docs/misc/arm/device-tree/booting.txt  |   5 +
 docs/misc/arm/device-tree/cpupools.txt | 156 +
 tools/helpers/xen-init-dom0.c  |  35 +++-
 tools/libs/light/libxl_utils.c |   3 +-
 xen/arch/arm/domain_build.c|  14 +-
 xen/common/Kconfig |   8 +
 xen/common/Makefile|   1 +
 xen/common/boot_cpupools.c | 233 +
 xen/common/domain.c|   2 +-
 xen/common/sched/core.c|  40 +++--
 xen/common/sched/cpupool.c |  29 ++-
 xen/include/public/domctl.h|   4 +-
 xen/include/xen/sched.h|  58 ++
 13 files changed, 560 insertions(+), 28 deletions(-)
 create mode 100644 docs/misc/arm/device-tree/cpupools.txt
 create mode 100644 xen/common/boot_cpupools.c

-- 
2.17.1

[PATCH v2 1/6] tools/cpupools: Give a name to unnamed cpupools

[Luca Fancellu]

With the introduction of boot time cpupools, Xen can create many
different cpupools at boot time other than cpupool with id 0.

Since these newly created cpupools can't have an
entry in Xenstore, create the entry using xen-init-dom0
helper with the usual convention: Pool-.

Given the change, remove the check for poolid == 0 from
libxl_cpupoolid_to_name(...).

Signed-off-by: Luca Fancellu 
---
Changes in v2:
 - Remove unused variable, moved xc_cpupool_infofree
   ahead to simplify the code, use asprintf (Juergen)
---
 tools/helpers/xen-init-dom0.c  | 35 +-
 tools/libs/light/libxl_utils.c |  3 +--
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
index c99224a4b607..84286617790f 100644
--- a/tools/helpers/xen-init-dom0.c
+++ b/tools/helpers/xen-init-dom0.c
@@ -43,7 +43,9 @@ int main(int argc, char **argv)
 int rc;
 struct xs_handle *xsh = NULL;
 xc_interface *xch = NULL;
-char *domname_string = NULL, *domid_string = NULL;
+char *domname_string = NULL, *domid_string = NULL, *pool_path, *pool_name;
+xc_cpupoolinfo_t *xcinfo;
+unsigned int pool_id = 0;
 libxl_uuid uuid;
 
 /* Accept 0 or 1 argument */
@@ -114,6 +116,37 @@ int main(int argc, char **argv)
 goto out;
 }
 
+/* Create an entry in xenstore for each cpupool on the system */
+do {
+xcinfo = xc_cpupool_getinfo(xch, pool_id);
+if (xcinfo != NULL) {
+if (xcinfo->cpupool_id != pool_id)
+pool_id = xcinfo->cpupool_id;
+xc_cpupool_infofree(xch, xcinfo);
+if (asprintf(&pool_path, "/local/pool/%d/name", pool_id) <= 0) {
+fprintf(stderr, "cannot allocate memory for pool path\n");
+rc = 1;
+goto out;
+}
+if (asprintf(&pool_name, "Pool-%d", pool_id) <= 0) {
+fprintf(stderr, "cannot allocate memory for pool name\n");
+rc = 1;
+goto out_err;
+}
+pool_id++;
+if (!xs_write(xsh, XBT_NULL, pool_path, pool_name,
+  strlen(pool_name))) {
+fprintf(stderr, "cannot set pool name\n");
+rc = 1;
+}
+free(pool_name);
+out_err:
+free(pool_path);
+if ( rc )
+goto out;
+}
+} while(xcinfo != NULL);
+
 printf("Done setting up Dom0\n");
 
 out:
diff --git a/tools/libs/light/libxl_utils.c b/tools/libs/light/libxl_utils.c
index b91c2cafa223..81780da3ff40 100644
--- a/tools/libs/light/libxl_utils.c
+++ b/tools/libs/light/libxl_utils.c
@@ -151,8 +151,7 @@ char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t 
poolid)
 
 snprintf(path, sizeof(path), "/local/pool/%d/name", poolid);
 s = xs_read(ctx->xsh, XBT_NULL, path, &len);
-if (!s && (poolid == 0))
-return strdup("Pool-0");
+
 return s;
 }
 
-- 
2.17.1

[PATCH v2 2/6] xen/sched: create public function for cpupools creation

[Luca Fancellu]

Create new public function to create cpupools, can take as parameter
the scheduler id or a negative value that means the default Xen
scheduler will be used.

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- cpupool_create_pool doesn't check anymore for pool id uniqueness
  before calling cpupool_create. Modified commit message accordingly
---
 xen/common/sched/cpupool.c | 15 +++
 xen/include/xen/sched.h| 16 
 2 files changed, 31 insertions(+)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index a6da4970506a..89a891af7076 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -1219,6 +1219,21 @@ static void cpupool_hypfs_init(void)
 
 #endif /* CONFIG_HYPFS */
 
+struct cpupool *__init cpupool_create_pool(unsigned int pool_id, int sched_id)
+{
+struct cpupool *pool;
+
+if ( sched_id < 0 )
+sched_id = scheduler_get_default()->sched_id;
+
+pool = cpupool_create(pool_id, sched_id);
+
+BUG_ON(IS_ERR(pool));
+cpupool_put(pool);
+
+return pool;
+}
+
 static int __init cf_check cpupool_init(void)
 {
 unsigned int cpu;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 10ea969c7af9..47fc856e0fe0 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct cpupool 
*c);
 int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
 unsigned int cpupool_get_id(const struct domain *d);
 const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
+
+/*
+ * cpupool_create_pool - Creates a cpupool
+ * @pool_id: id of the pool to be created
+ * @sched_id: id of the scheduler to be used for the pool
+ *
+ * Creates a cpupool with pool_id id.
+ * The sched_id parameter identifies the scheduler to be used, if it is
+ * negative, the default scheduler of Xen will be used.
+ *
+ * returns:
+ * pointer to the struct cpupool just created, on success
+ * NULL, on cpupool creation error
+ */
+struct cpupool *cpupool_create_pool(unsigned int pool_id, int sched_id);
+
 extern void cf_check dump_runq(unsigned char key);
 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
-- 
2.17.1

[PATCH v2 3/6] xen/sched: retrieve scheduler id by name

[Luca Fancellu]

Add a static function to retrieve the scheduler pointer using the
scheduler name.

Add a public function to retrieve the scheduler id by the scheduler
name that makes use of the new static function.

Take the occasion to replace open coded scheduler search with the
new static function in scheduler_init.

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- replace open coded scheduler search in scheduler_init (Juergen)
---
 xen/common/sched/core.c | 40 ++--
 xen/include/xen/sched.h | 11 +++
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 19ab67818106..48ee01420fb8 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2947,10 +2947,30 @@ void scheduler_enable(void)
 scheduler_active = true;
 }
 
+static inline
+const struct scheduler *__init sched_get_by_name(const char *sched_name)
+{
+unsigned int i;
+
+for ( i = 0; i < NUM_SCHEDULERS; i++ )
+if ( schedulers[i] && !strcmp(schedulers[i]->opt_name, sched_name) )
+return schedulers[i];
+
+return NULL;
+}
+
+int __init sched_get_id_by_name(const char *sched_name)
+{
+const struct scheduler *scheduler = sched_get_by_name(sched_name);
+
+return scheduler ? scheduler->sched_id : -1;
+}
+
 /* Initialise the data structures. */
 void __init scheduler_init(void)
 {
 struct domain *idle_domain;
+const struct scheduler *scheduler;
 int i;
 
 scheduler_enable();
@@ -2981,25 +3001,17 @@ void __init scheduler_init(void)
schedulers[i]->opt_name);
 schedulers[i] = NULL;
 }
-
-if ( schedulers[i] && !ops.name &&
- !strcmp(schedulers[i]->opt_name, opt_sched) )
-ops = *schedulers[i];
 }
 
-if ( !ops.name )
+scheduler = sched_get_by_name(opt_sched);
+if ( !scheduler )
 {
 printk("Could not find scheduler: %s\n", opt_sched);
-for ( i = 0; i < NUM_SCHEDULERS; i++ )
-if ( schedulers[i] &&
- !strcmp(schedulers[i]->opt_name, CONFIG_SCHED_DEFAULT) )
-{
-ops = *schedulers[i];
-break;
-}
-BUG_ON(!ops.name);
-printk("Using '%s' (%s)\n", ops.name, ops.opt_name);
+scheduler = sched_get_by_name(CONFIG_SCHED_DEFAULT);
+BUG_ON(!scheduler);
+printk("Using '%s' (%s)\n", scheduler->name, scheduler->opt_name);
 }
+ops = *scheduler;
 
 if ( cpu_schedule_up(0) )
 BUG();
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 47fc856e0fe0..2c10303f0187 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -756,6 +756,17 @@ void sched_destroy_domain(struct domain *d);
 long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
 long sched_adjust_global(struct xen_sysctl_scheduler_op *);
 int  sched_id(void);
+
+/*
+ * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
+ * @sched_name: scheduler name as a string
+ *
+ * returns:
+ * positive value being the scheduler id, on success
+ * negative value if the scheduler name is not found.
+ */
+int sched_get_id_by_name(const char *sched_name);
+
 void vcpu_wake(struct vcpu *v);
 long vcpu_yield(void);
 void vcpu_sleep_nosync(struct vcpu *v);
-- 
2.17.1

[PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

Introduce a way to create different cpupools at boot time, this is
particularly useful on ARM big.LITTLE system where there might be the
need to have different cpupools for each type of core, but also
systems using NUMA can have different cpu pools for each node.

The feature on arm relies on a specification of the cpupools from the
device tree to build pools and assign cpus to them.

Documentation is created to explain the feature.

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- Move feature to common code (Juergen)
- Try to decouple dtb parse and cpupool creation to allow
  more way to specify cpupools (for example command line)
- Created standalone dt node for the scheduler so it can
  be used in future work to set scheduler specific
  parameters
- Use only auto generated ids for cpupools
---
 docs/misc/arm/device-tree/cpupools.txt | 156 ++
 xen/common/Kconfig |   8 +
 xen/common/Makefile|   1 +
 xen/common/boot_cpupools.c | 212 +
 xen/common/sched/cpupool.c |   6 +-
 xen/include/xen/sched.h|  19 +++
 6 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 docs/misc/arm/device-tree/cpupools.txt
 create mode 100644 xen/common/boot_cpupools.c

diff --git a/docs/misc/arm/device-tree/cpupools.txt 
b/docs/misc/arm/device-tree/cpupools.txt
new file mode 100644
index ..d5a82ed0d45a
--- /dev/null
+++ b/docs/misc/arm/device-tree/cpupools.txt
@@ -0,0 +1,156 @@
+Boot time cpupools
+==
+
+When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to
+create cpupools during boot phase by specifying them in the device tree.
+
+Cpupools specification nodes shall be direct childs of /chosen node.
+Each cpupool node contains the following properties:
+
+- compatible (mandatory)
+
+Must always include the compatiblity string: "xen,cpupool".
+
+- cpupool-cpus (mandatory)
+
+Must be a list of device tree phandle to nodes describing cpus (e.g. having
+device_type = "cpu"), it can't be empty.
+
+- cpupool-sched (optional)
+
+Must be a device tree phandle to a node having "xen,scheduler" compatible
+(description below), it has no effect when the cpupool refers to the 
cpupool
+number zero, in that case the default Xen scheduler is selected 
(sched=<...>
+boot argument).
+
+
+A scheduler specification node is a device tree node that contains the 
following
+properties:
+
+- compatible (mandatory)
+
+Must always include the compatiblity string: "xen,scheduler".
+
+- sched-name (mandatory)
+
+Must be a string having the name of a Xen scheduler, check the sched=<...>
+boot argument for allowed values.
+
+
+Constraints
+===
+
+If no cpupools are specified, all cpus will be assigned to one cpupool
+implicitly created (Pool-0).
+
+If cpupools node are specified, but not every cpu brought up by Xen is 
assigned,
+all the not assigned cpu will be assigned to an additional cpupool.
+
+If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will
+stop.
+
+
+Examples
+
+
+A system having two types of core, the following device tree specification will
+instruct Xen to have two cpupools:
+
+- The cpupool with id 0 will have 4 cpus assigned.
+- The cpupool with id 1 will have 2 cpus assigned.
+
+The following example can work only if hmp-unsafe=1 is passed to Xen boot
+arguments, otherwise not all cores will be brought up by Xen and the cpupool
+creation process will stop Xen.
+
+
+a72_1: cpu@0 {
+compatible = "arm,cortex-a72";
+reg = <0x0 0x0>;
+device_type = "cpu";
+[...]
+};
+
+a72_2: cpu@1 {
+compatible = "arm,cortex-a72";
+reg = <0x0 0x1>;
+device_type = "cpu";
+[...]
+};
+
+a53_1: cpu@100 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x100>;
+device_type = "cpu";
+[...]
+};
+
+a53_2: cpu@101 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x101>;
+device_type = "cpu";
+[...]
+};
+
+a53_3: cpu@102 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x102>;
+device_type = "cpu";
+[...]
+};
+
+a53_4: cpu@103 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x103>;
+device_type = "cpu";
+[...]
+};
+
+chosen {
+
+sched: sched_a {
+compatible = "xen,scheduler";
+sched-name = "credit2";
+};
+cpupool_a {
+compatible = "xen,cpupool";
+cpupool-cpus = <&a53_1 &a53_2 &a53_3 &a53_4>;
+};
+cpupool_b {
+compatible = "xen,cpupool";
+cpupool-cpus = <&a72_1 &a72_2>;

[PATCH v2 6/6] xen/cpupool: Allow cpupool0 to use different scheduler

[Luca Fancellu]

Currently cpupool0 can use only the default scheduler, and
cpupool_create has an harcoded behavior when creating the pool 0
that doesn't allocate new memory for the scheduler, but uses the
default scheduler structure in memory.

With this commit it is possible to allocate a different scheduler for
the cpupool0 when using the boot time cpupool.
To achieve this the hardcoded behavior in cpupool_create is removed
and the cpupool0 creation is moved.

When compiling without boot time cpupools enabled, the current
behavior is maintained (except that cpupool0 scheduler memory will be
allocated).

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- new patch
---
 xen/common/boot_cpupools.c | 39 ++
 xen/common/sched/cpupool.c |  8 +---
 xen/include/xen/sched.h|  5 -
 3 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c
index 01a69f894f14..a8ae8c5b7852 100644
--- a/xen/common/boot_cpupools.c
+++ b/xen/common/boot_cpupools.c
@@ -189,31 +189,28 @@ void __init btcpupools_allocate_pools(const cpumask_t 
*cpu_online_map)
 {
 struct cpupool *pool = NULL;
 int pool_id, sched_id;
+unsigned int i;
 
 pool_id = pool_cpu_map[cpu_num].pool_id;
 sched_id = pool_cpu_map[cpu_num].sched_id;
 
-if ( pool_id )
-{
-unsigned int i;
-
-/* Look for previously created pool with id pool_id */
-for ( i = 0; i < cpu_num; i++ )
-if ( (pool_cpu_map[i].pool_id == pool_id) &&
- pool_cpu_map[i].pool )
-{
-pool = pool_cpu_map[i].pool;
-break;
-}
-
-/* If no pool was created before, create it */
-if ( !pool )
-pool = cpupool_create_pool(pool_id, sched_id);
-if ( !pool )
-panic("Error creating pool id %u!\n", pool_id);
-}
-else
-pool = cpupool0;
+/* Look for previously created pool with id pool_id */
+for ( i = 0; i < cpu_num; i++ )
+if ( (pool_cpu_map[i].pool_id == pool_id) && pool_cpu_map[i].pool )
+{
+pool = pool_cpu_map[i].pool;
+break;
+}
+
+/* If no pool was created before, create it */
+if ( !pool )
+pool = cpupool_create_pool(pool_id, sched_id);
+if ( !pool )
+panic("Error creating pool id %u!\n", pool_id);
+
+/* Keep track of cpupool id 0 with the global cpupool0 */
+if ( !pool_id )
+cpupool0 = pool;
 
 pool_cpu_map[cpu_num].pool = pool;
 printk(XENLOG_INFO "Logical CPU %u in Pool-%u.\n", cpu_num, pool_id);
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index b2495ad6d03e..3d458a4932b2 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -312,10 +312,7 @@ static struct cpupool *cpupool_create(unsigned int poolid,
 c->cpupool_id = q->cpupool_id + 1;
 }
 
-if ( poolid == 0 )
-c->sched = scheduler_get_default();
-else
-c->sched = scheduler_alloc(sched_id);
+c->sched = scheduler_alloc(sched_id);
 if ( IS_ERR(c->sched) )
 {
 ret = PTR_ERR(c->sched);
@@ -1242,9 +1239,6 @@ static int __init cf_check cpupool_init(void)
 
 cpupool_hypfs_init();
 
-cpupool0 = cpupool_create(0, 0);
-BUG_ON(IS_ERR(cpupool0));
-cpupool_put(cpupool0);
 register_cpu_notifier(&cpu_nfb);
 
 btcpupools_dtb_parse();
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 30a6538452bc..4007a3df4c1c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1188,7 +1188,10 @@ static inline void btcpupools_dtb_parse(void) {}
 #endif
 
 #else
-static inline void btcpupools_allocate_pools(const cpumask_t *cpu_online_map) 
{}
+static inline void btcpupools_allocate_pools(const cpumask_t *cpu_online_map)
+{
+cpupool0 = cpupool_create_pool(0, -1);
+}
 static inline void btcpupools_dtb_parse(void) {}
 static inline struct cpupool *btcpupools_get_cpupool(unsigned int cpu)
 {
-- 
2.17.1

[PATCH v2 5/6] arm/dom0less: assign dom0less guests to cpupools

[Luca Fancellu]

Introduce domain-cpupool property of a xen,domain device tree node,
that specifies the cpupool device tree handle of a xen,cpupool node
that identifies a cpupool created at boot time where the guest will
be assigned on creation.

Add member to the xen_domctl_createdomain public interface so the
XEN_DOMCTL_INTERFACE_VERSION version is bumped.

Add public function to retrieve a pool id from the device tree
cpupool node.

Update documentation about the property.

Signed-off-by: Luca Fancellu 
---
Changes in v2:
- Moved cpupool_id from arch specific to common part (Juergen)
- Implemented functions to retrieve the cpupool id from the
  cpupool dtb node.
---
 docs/misc/arm/device-tree/booting.txt |  5 +
 xen/arch/arm/domain_build.c   | 14 +-
 xen/common/boot_cpupools.c| 24 
 xen/common/domain.c   |  2 +-
 xen/include/public/domctl.h   |  4 +++-
 xen/include/xen/sched.h   |  9 +
 6 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt 
b/docs/misc/arm/device-tree/booting.txt
index a94125394e35..7b4a29a2c293 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -188,6 +188,11 @@ with the following properties:
 An empty property to request the memory of the domain to be
 direct-map (guest physical address == physical address).
 
+- domain-cpupool
+
+Optional. Handle to a xen,cpupool device tree node that identifies the
+cpupool where the guest will be started at boot.
+
 Under the "xen,domain" compatible node, one or more sub-nodes are present
 for the DomU kernel and ramdisk.
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de05..9c67a483d4a4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3172,7 +3172,8 @@ static int __init construct_domU(struct domain *d,
 void __init create_domUs(void)
 {
 struct dt_device_node *node;
-const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+const struct dt_device_node *cpupool_node,
+*chosen = dt_find_node_by_path("/chosen");
 
 BUG_ON(chosen == NULL);
 dt_for_each_child_node(chosen, node)
@@ -3241,6 +3242,17 @@ void __init create_domUs(void)
  vpl011_virq - 32 + 1);
 }
 
+/* Get the optional property domain-cpupool */
+cpupool_node = dt_parse_phandle(node, "domain-cpupool", 0);
+if ( cpupool_node )
+{
+int pool_id = btcpupools_get_domain_pool_id(cpupool_node);
+if ( pool_id < 0 )
+panic("Error getting cpupool id from domain-cpupool (%d)\n",
+  pool_id);
+d_cfg.cpupool_id = pool_id;
+}
+
 /*
  * The variable max_init_domid is initialized with zero, so here it's
  * very important to use the pre-increment operator to call
diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c
index e8529a902d21..01a69f894f14 100644
--- a/xen/common/boot_cpupools.c
+++ b/xen/common/boot_cpupools.c
@@ -11,6 +11,8 @@
 
 #define BTCPUPOOLS_DT_NODE_NO_REG (-1)
 #define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2)
+#define BTCPUPOOLS_DT_WRONG_NODE  (-3)
+#define BTCPUPOOLS_DT_CORRUPTED_NODE  (-4)
 
 struct pool_map {
 int pool_id;
@@ -53,6 +55,28 @@ get_logical_cpu_from_cpu_node(const struct dt_device_node 
*cpu_node)
 return cpu_num;
 }
 
+int __init btcpupools_get_domain_pool_id(const struct dt_device_node *node)
+{
+const struct dt_device_node *phandle_node;
+int cpu_num;
+
+if ( !dt_device_is_compatible(node, "xen,cpupool") )
+return BTCPUPOOLS_DT_WRONG_NODE;
+/*
+ * Get first cpu listed in the cpupool, from its reg it's possible to
+ * retrieve the cpupool id.
+ */
+phandle_node = dt_parse_phandle(node, "cpupool-cpus", 0);
+if ( !phandle_node )
+return BTCPUPOOLS_DT_CORRUPTED_NODE;
+
+cpu_num = get_logical_cpu_from_cpu_node(phandle_node);
+if ( cpu_num < 0 )
+return cpu_num;
+
+return pool_cpu_map[cpu_num].pool_id;
+}
+
 static int __init check_and_get_sched_id(const char* scheduler_name)
 {
 int sched_id = sched_get_id_by_name(scheduler_name);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 351029f8b239..0827400f4f49 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -698,7 +698,7 @@ struct domain *domain_create(domid_t domid,
 if ( !d->pbuf )
 goto fail;
 
-if ( (err = sched_init_domain(d, 0)) != 0 )
+if ( (err = sched_init_domain(d, config->cpupool_id)) != 0 )
 goto fail;
 
 if ( (err = late_hwdom_init(d)) != 0 )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index b85e6170b0aa..3d43

Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

Hi Juergen,

Thanks for your review

> On 11 Mar 2022, at 08:09, Juergen Gross  wrote:
> 
> On 10.03.22 18:10, Luca Fancellu wrote:
>> Introduce a way to create different cpupools at boot time, this is
>> particularly useful on ARM big.LITTLE system where there might be the
>> need to have different cpupools for each type of core, but also
>> systems using NUMA can have different cpu pools for each node.
>> The feature on arm relies on a specification of the cpupools from the
>> device tree to build pools and assign cpus to them.
>> Documentation is created to explain the feature.
>> Signed-off-by: Luca Fancellu 
>> ---
>> Changes in v2:
>> - Move feature to common code (Juergen)
>> - Try to decouple dtb parse and cpupool creation to allow
>>   more way to specify cpupools (for example command line)
>> - Created standalone dt node for the scheduler so it can
>>   be used in future work to set scheduler specific
>>   parameters
>> - Use only auto generated ids for cpupools
>> ---
>>  docs/misc/arm/device-tree/cpupools.txt | 156 ++
>>  xen/common/Kconfig |   8 +
>>  xen/common/Makefile|   1 +
>>  xen/common/boot_cpupools.c | 212 +
>>  xen/common/sched/cpupool.c |   6 +-
>>  xen/include/xen/sched.h|  19 +++
>>  6 files changed, 401 insertions(+), 1 deletion(-)
>>  create mode 100644 docs/misc/arm/device-tree/cpupools.txt
>>  create mode 100644 xen/common/boot_cpupools.c
>> diff --git a/docs/misc/arm/device-tree/cpupools.txt 
>> b/docs/misc/arm/device-tree/cpupools.txt
>> new file mode 100644
>> index ..d5a82ed0d45a
>> --- /dev/null
>> +++ b/docs/misc/arm/device-tree/cpupools.txt
>> @@ -0,0 +1,156 @@
>> +Boot time cpupools
>> +==
>> +
>> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible 
>> to
>> +create cpupools during boot phase by specifying them in the device tree.
>> +
>> +Cpupools specification nodes shall be direct childs of /chosen node.
>> +Each cpupool node contains the following properties:
>> +
>> +- compatible (mandatory)
>> +
>> +Must always include the compatiblity string: "xen,cpupool".
>> +
>> +- cpupool-cpus (mandatory)
>> +
>> +Must be a list of device tree phandle to nodes describing cpus (e.g. 
>> having
>> +device_type = "cpu"), it can't be empty.
>> +
>> +- cpupool-sched (optional)
>> +
>> +Must be a device tree phandle to a node having "xen,scheduler" 
>> compatible
>> +(description below), it has no effect when the cpupool refers to the 
>> cpupool
>> +number zero, in that case the default Xen scheduler is selected 
>> (sched=<...>
>> +boot argument).
>> +
>> +
>> +A scheduler specification node is a device tree node that contains the 
>> following
>> +properties:
>> +
>> +- compatible (mandatory)
>> +
>> +Must always include the compatiblity string: "xen,scheduler".
>> +
>> +- sched-name (mandatory)
>> +
>> +Must be a string having the name of a Xen scheduler, check the 
>> sched=<...>
>> +boot argument for allowed values.
>> +
>> +
>> +Constraints
>> +===
>> +
>> +If no cpupools are specified, all cpus will be assigned to one cpupool
>> +implicitly created (Pool-0).
>> +
>> +If cpupools node are specified, but not every cpu brought up by Xen is 
>> assigned,
>> +all the not assigned cpu will be assigned to an additional cpupool.
>> +
>> +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen 
>> will
>> +stop.
>> +
>> +
>> +Examples
>> +
>> +
>> +A system having two types of core, the following device tree specification 
>> will
>> +instruct Xen to have two cpupools:
>> +
>> +- The cpupool with id 0 will have 4 cpus assigned.
>> +- The cpupool with id 1 will have 2 cpus assigned.
>> +
>> +The following example can work only if hmp-unsafe=1 is passed to Xen boot
>> +arguments, otherwise not all cores will be brought up by Xen and the cpupool
>> +creation process will stop Xen.
>> +
>> +
>> +a72_1: cpu@0 {
>> +compatible = "arm,cortex-a72";
>> +reg = <0x0 0x0>;
>> +device_type = "cpu";
>> +[...]
>> +};
>>

Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

> On 11 Mar 2022, at 10:18, Juergen Gross  wrote:
> 
> On 11.03.22 10:46, Jan Beulich wrote:
>> On 11.03.2022 10:29, Juergen Gross wrote:
>>> On 11.03.22 09:56, Luca Fancellu wrote:
>>>>> On 11 Mar 2022, at 08:09, Juergen Gross  wrote:
>>>>> On 10.03.22 18:10, Luca Fancellu wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/common/boot_cpupools.c
>>>>>> @@ -0,0 +1,212 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>>> +/*
>>>>>> + * xen/common/boot_cpupools.c
>>>>>> + *
>>>>>> + * Code to create cpupools at boot time for arm architecture.
>>>>> 
>>>>> Please drop the arm reference here.
>>>>> 
>>>>>> + *
>>>>>> + * Copyright (C) 2022 Arm Ltd.
>>>>>> + */
>>>>>> +
>>>>>> +#include 
>>>>>> +
>>>>>> +#define BTCPUPOOLS_DT_NODE_NO_REG (-1)
>>>>>> +#define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2)
>>>>> 
>>>>> Move those inside the #ifdef below, please
>>>>> 
>>>>>> +
>>>>>> +struct pool_map {
>>>>>> +int pool_id;
>>>>>> +int sched_id;
>>>>>> +struct cpupool *pool;
>>>>>> +};
>>>>>> +
>>>>>> +static struct pool_map __initdata pool_cpu_map[NR_CPUS] =
>>>>>> +{ [0 ... NR_CPUS-1] = {.pool_id = -1, .sched_id = -1, .pool = NULL} 
>>>>>> };
>>>>>> +static unsigned int __initdata next_pool_id;
>>>>>> +
>>>>>> +#ifdef CONFIG_ARM
>>>>> 
>>>>> Shouldn't this be CONFIG_HAS_DEVICE_TREE?
>>>> 
>>>> Yes, the only problem is that in get_logical_cpu_from_hw_id I use the arm 
>>>> specific
>>>> cpu_logical_map(…), so what do you think it’s the better way here?
>>>> Do you think I should have everything under CONFIG_HAS_DEVICE_TREE
>>>> and get_logical_cpu_from_hw_id under CONFIG_ARM like in this way below?
>>> 
>>> Hmm, what is the hwid used for on Arm? I guess this could be similar
>>> to the x86 acpi-id?
>> Since there's going to be only one of DT or ACPI, if anything this could
>> be the APIC ID and then ...
>>> So I'd rather put get_logical_cpu_from_hw_id() into Arm specific code
>>> and add a related x86 function to x86 code. Depending on the answer to
>>> above question this could either be get_cpu_id(), or maybe an identity
>>> function.
>> ... a lookup loop over x86_cpu_to_apicid[] (I thought we had a function
>> doing so, but right now I can't find one).
> 
> It is the second half of get_cpu_id().

I was going to say, maybe I can do something like this:

#ifdef CONFIG_ARM
#define hwid_from_logical_cpu_id(x) cpu_logical_map(x)
#elif defined(CONFIG_X86)
#define hwid_from_logical_cpu_id(x) x86_cpu_to_apicid(x)
#else
#define hwid_from_logical_cpu_id(x) (-1)
#end

static int __init get_logical_cpu_from_hw_id(unsigned int hwid)
{
unsigned int i;

for ( i = 0; i < nr_cpu_ids; i++ )
if ( hwid_from_logical_cpu_id(i) == hwid )
return i;

return -1;
}

Do you think it is acceptable?

I see the current get_cpu_id(…) from x86 code is starting from an acpi id to
lookup the apicid and then it is looking for the logical cpu number.
In the x86 context, eventually, the reg property of a cpu node would hold an
Acpi id or an apicid? I would have say the last one but I’m not sure now.

Cheers,
Luca

> 
> 
> Juergen
> 

Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

Hi Stefano,

> On 11 Mar 2022, at 03:57, Stefano Stabellini  wrote:
> 
> On Thu, 10 Mar 2022, Luca Fancellu wrote:
>> Introduce a way to create different cpupools at boot time, this is
>> particularly useful on ARM big.LITTLE system where there might be the
>> need to have different cpupools for each type of core, but also
>> systems using NUMA can have different cpu pools for each node.
>> 
>> The feature on arm relies on a specification of the cpupools from the
>> device tree to build pools and assign cpus to them.
>> 
>> Documentation is created to explain the feature.
>> 
>> Signed-off-by: Luca Fancellu 
>> ---
>> Changes in v2:
>> - Move feature to common code (Juergen)
>> - Try to decouple dtb parse and cpupool creation to allow
>>  more way to specify cpupools (for example command line)
>> - Created standalone dt node for the scheduler so it can
>>  be used in future work to set scheduler specific
>>  parameters
>> - Use only auto generated ids for cpupools
>> ---
>> docs/misc/arm/device-tree/cpupools.txt | 156 ++
>> xen/common/Kconfig |   8 +
>> xen/common/Makefile|   1 +
>> xen/common/boot_cpupools.c | 212 +
>> xen/common/sched/cpupool.c |   6 +-
>> xen/include/xen/sched.h|  19 +++
>> 6 files changed, 401 insertions(+), 1 deletion(-)
>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt
>> create mode 100644 xen/common/boot_cpupools.c
>> 
>> diff --git a/docs/misc/arm/device-tree/cpupools.txt 
>> b/docs/misc/arm/device-tree/cpupools.txt
>> new file mode 100644
>> index ..d5a82ed0d45a
>> --- /dev/null
>> +++ b/docs/misc/arm/device-tree/cpupools.txt
>> @@ -0,0 +1,156 @@
>> +Boot time cpupools
>> +==
>> +
>> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible 
>> to
>> +create cpupools during boot phase by specifying them in the device tree.
>> +
>> +Cpupools specification nodes shall be direct childs of /chosen node.
>> +Each cpupool node contains the following properties:
>> +
>> +- compatible (mandatory)
>> +
>> +Must always include the compatiblity string: "xen,cpupool".
>> +
>> +- cpupool-cpus (mandatory)
>> +
>> +Must be a list of device tree phandle to nodes describing cpus (e.g. 
>> having
>> +device_type = "cpu"), it can't be empty.
>> +
>> +- cpupool-sched (optional)
>> +
>> +Must be a device tree phandle to a node having "xen,scheduler" 
>> compatible
>> +(description below), it has no effect when the cpupool refers to the 
>> cpupool
>> +number zero, in that case the default Xen scheduler is selected 
>> (sched=<...>
>> +boot argument).
> 
> This is *a lot* better.
> 
> The device tree part is nice. I have only one question left on it: why
> do we need a separate scheduler node? Could the "cpupool-sched" property
> be a simple string with the scheduler name?
> 
> E.g.:
> 
>cpupool_a {
>compatible = "xen,cpupool";
>cpupool-cpus = <&a53_1 &a53_2>;
>};
>cpupool_b {
>compatible = "xen,cpupool";
>cpupool-cpus = <&a72_1 &a72_2>;
>cpupool-sched = "null";
>};
> 
> 
> To me, it doesn't look like these new "scheduler specification nodes"
> bring any benefits. I would just get rid of them.

From a comment of Juergen on the second patch I thought someone sees the need to
have a way to set scheduling parameters:

“you are allowing to use another scheduler,
but what if someone wants to set non-standard scheduling parameters
(e.g. another time slice)?”

So I thought I could introduce a scheduler specification node that could in the 
future be
extended and used to set scheduling parameter.

If it is something that is not needed, I will get rid of it.

> 
> 
>> +A scheduler specification node is a device tree node that contains the 
>> following
>> +properties:
>> +
>> +- compatible (mandatory)
>> +
>> +Must always include the compatiblity string: "xen,scheduler".
>> +
>> +- sched-name (mandatory)
>> +
>> +Must be a string having the name of a Xen scheduler, check the 
>> sched=<...>
>> +boot argument for allowed values.
>> +
>> +
>> +Constraints
>> +===
>> +
>> +If no cpupools are specified, all cpus 

Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

> On 11 Mar 2022, at 14:11, Luca Fancellu  wrote:
> 
> Hi Stefano,
> 
>> On 11 Mar 2022, at 03:57, Stefano Stabellini  wrote:
>> 
>> On Thu, 10 Mar 2022, Luca Fancellu wrote:
>>> Introduce a way to create different cpupools at boot time, this is
>>> particularly useful on ARM big.LITTLE system where there might be the
>>> need to have different cpupools for each type of core, but also
>>> systems using NUMA can have different cpu pools for each node.
>>> 
>>> The feature on arm relies on a specification of the cpupools from the
>>> device tree to build pools and assign cpus to them.
>>> 
>>> Documentation is created to explain the feature.
>>> 
>>> Signed-off-by: Luca Fancellu 
>>> ---
>>> Changes in v2:
>>> - Move feature to common code (Juergen)
>>> - Try to decouple dtb parse and cpupool creation to allow
>>> more way to specify cpupools (for example command line)
>>> - Created standalone dt node for the scheduler so it can
>>> be used in future work to set scheduler specific
>>> parameters
>>> - Use only auto generated ids for cpupools
>>> ---
>>> docs/misc/arm/device-tree/cpupools.txt | 156 ++
>>> xen/common/Kconfig |   8 +
>>> xen/common/Makefile|   1 +
>>> xen/common/boot_cpupools.c | 212 +
>>> xen/common/sched/cpupool.c |   6 +-
>>> xen/include/xen/sched.h|  19 +++
>>> 6 files changed, 401 insertions(+), 1 deletion(-)
>>> create mode 100644 docs/misc/arm/device-tree/cpupools.txt
>>> create mode 100644 xen/common/boot_cpupools.c
>>> 
>>> diff --git a/docs/misc/arm/device-tree/cpupools.txt 
>>> b/docs/misc/arm/device-tree/cpupools.txt
>>> new file mode 100644
>>> index ..d5a82ed0d45a
>>> --- /dev/null
>>> +++ b/docs/misc/arm/device-tree/cpupools.txt
>>> @@ -0,0 +1,156 @@
>>> +Boot time cpupools
>>> +==
>>> +
>>> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is 
>>> possible to
>>> +create cpupools during boot phase by specifying them in the device tree.
>>> +
>>> +Cpupools specification nodes shall be direct childs of /chosen node.
>>> +Each cpupool node contains the following properties:
>>> +
>>> +- compatible (mandatory)
>>> +
>>> +Must always include the compatiblity string: "xen,cpupool".
>>> +
>>> +- cpupool-cpus (mandatory)
>>> +
>>> +Must be a list of device tree phandle to nodes describing cpus (e.g. 
>>> having
>>> +device_type = "cpu"), it can't be empty.
>>> +
>>> +- cpupool-sched (optional)
>>> +
>>> +Must be a device tree phandle to a node having "xen,scheduler" 
>>> compatible
>>> +(description below), it has no effect when the cpupool refers to the 
>>> cpupool
>>> +number zero, in that case the default Xen scheduler is selected 
>>> (sched=<...>
>>> +boot argument).
>> 
>> This is *a lot* better.
>> 
>> The device tree part is nice. I have only one question left on it: why
>> do we need a separate scheduler node? Could the "cpupool-sched" property
>> be a simple string with the scheduler name?
>> 
>> E.g.:
>> 
>>   cpupool_a {
>>   compatible = "xen,cpupool";
>>   cpupool-cpus = <&a53_1 &a53_2>;
>>   };
>>   cpupool_b {
>>   compatible = "xen,cpupool";
>>   cpupool-cpus = <&a72_1 &a72_2>;
>>   cpupool-sched = "null";
>>   };
>> 
>> 
>> To me, it doesn't look like these new "scheduler specification nodes"
>> bring any benefits. I would just get rid of them.
> 
> From a comment of Juergen on the second patch I thought someone sees the need 
> to
> have a way to set scheduling parameters:
> 
> “you are allowing to use another scheduler,
> but what if someone wants to set non-standard scheduling parameters
> (e.g. another time slice)?”
> 
> So I thought I could introduce a scheduler specification node that could in 
> the future be
> extended and used to set scheduling parameter.
> 
> If it is something that is not needed, I will get rid of it.
> 
>> 
>> 
>>> +A scheduler specification node is a device tree node that contains the 
>&g

Re: [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type

[Luca Fancellu]

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Change function type of following function to access during runtime:
>1. map_irq_to_domain()
>2. handle_device_interrupt()
>3. map_range_to_domain()
>4. unflatten_dt_node()
>5. unflatten_device_tree()
> 
> Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() 
> to
> device.c.
> 
> These changes are done to support the dynamic programming of a nodes where an
> overlay node will be added to fdt and unflattened node will be added to 
> dt_host.
> Furthermore, IRQ and mmio mapping will be done for the added node.
> 
> Signed-off-by: Vikram Garhwal 
> ---
> xen/arch/arm/device.c| 136 +
> xen/arch/arm/domain_build.c  | 142 ---
> xen/arch/arm/include/asm/setup.h |   3 +
> xen/common/device_tree.c |  20 ++---
> xen/include/xen/device_tree.h|   5 ++
> 5 files changed, 154 insertions(+), 152 deletions(-)
> 
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 70cd6c1a19..0dfd33b33e 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -21,6 +21,9 @@
> #include 
> #include 
> #include 
> +#include 
> +#include 
> +#include 
> 
> extern const struct device_desc _sdevice[], _edevice[];
> extern const struct acpi_device_desc _asdevice[], _aedevice[];
> @@ -84,6 +87,139 @@ enum device_class device_get_class(const struct 
> dt_device_node *dev)
> return DEVICE_UNKNOWN;
> }
> 
> +int map_irq_to_domain(struct domain *d, unsigned int irq,
> +  bool need_mapping, const char *devname)
> +{
> +int res;
> +
> +res = irq_permit_access(d, irq);
> +if ( res )
> +{
> +printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> +   d->domain_id, irq);
> +return res;
> +}
> +
> +if ( need_mapping )
> +{
> +/*
> + * Checking the return of vgic_reserve_virq is not
> + * necessary. It should not fail except when we try to map
> + * the IRQ twice. This can legitimately happen if the IRQ is shared
> + */
> +vgic_reserve_virq(d, irq);
> +
> +res = route_irq_to_guest(d, irq, irq, devname);
> +if ( res < 0 )
> +{
> +printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> +   irq, d->domain_id);
> +return res;
> +}
> +}
> +
> +dt_dprintk("  - IRQ: %u\n", irq);
> +return 0;
> +}
> +
> +int map_range_to_domain(const struct dt_device_node *dev,
> +u64 addr, u64 len, void *data)
> +{
> +struct map_range_data *mr_data = data;
> +struct domain *d = mr_data->d;
> +int res;
> +
> +res = iomem_permit_access(d, paddr_to_pfn(addr),
> +paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));

Hi Vikram,

Why the if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
strlen("/reserved-memory/")) != 0 ) was dropped?


> +if ( res )
> +{
> +printk(XENLOG_ERR "Unable to permit to dom%d access to"
> +" 0x%"PRIx64" - 0x%"PRIx64"\n",
> +d->domain_id,
> +addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> +return res;
> +}
> +
> +if ( !mr_data->skip_mapping )
> +{
> +res = map_regions_p2mt(d,
> +   gaddr_to_gfn(addr),
> +   PFN_UP(len),
> +   maddr_to_mfn(addr),
> +   mr_data->p2mt);
> +
> +if ( res < 0 )
> +{
> +printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> +   " - 0x%"PRIx64" in domain %d\n",
> +   addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> +   d->domain_id);
> +return res;
> +}
> +}
> +
> +dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> +   addr, addr + len, mr_data->p2mt);
> +
> +return 0;
> +}
> +
> +/*
> + * handle_device_interrupts retrieves the interrupts configuration from
> + * a device tree node and maps those interrupts to the target domain.
> + *
> + * Returns:
> + *   < 0 error
> + *   0   success
> + */
> +int handle_device_interrupts(struct domain *d,
> + struct dt_device_node *dev,
> + bool need_mapping)
> +{
> +unsigned int i, nirq;
> +int res;
> +struct dt_raw_irq rirq;
> +
> +nirq = dt_number_of_irq(dev);
> +
> +/* Give permission and map IRQs */
> +for ( i = 0; i < nirq; i++ )
> +{
> +res = dt_device_get_raw_irq(dev, i, &rirq);
> +if ( res )
> +{
> +printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> +   i, dt_node_full_name(dev));
> +return res;
> +}
> +
> +/*
> + * Don't map IRQ that have no physical meaning
> + * ie: IRQ whose controller

Re: [XEN][RFC PATCH v3 02/14] xen/arm: Add CONFIG_OVERLAY_DTB

[Luca Fancellu]

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Introduce a config option where the user can enable support for 
> adding/removing
> device tree nodes using a device tree binary overlay.
> 
> Signed-off-by: Vikram Garhwal 
> ---
> xen/arch/arm/Kconfig | 6 ++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..0159fbe27a 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -46,6 +46,12 @@ config HAS_ITS
> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> depends on GICV3 && !NEW_VGIC
> 
> +config OVERLAY_DTB
> +bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> +---help---
> +
> +Dynamic addition/removal of Xen device tree nodes using a dtbo.
> +

Many recents entries in this file uses a different style from this one, using 
“help” instead of
“—help—“ and omitting the blank line, I would continue to use the more recent 
style if no
one object with it

Cheers,
Luca

> config HVM
> def_bool y
> 
> -- 
> 2.17.1
> 
> 

Re: [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target()

[Luca Fancellu]

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
> function type.
> 
> This is done to get the target path for the overlay nodes which is very useful
> in many cases. For example, Xen hypervisor needs it when applying overlays
> because Xen needs to do further processing of the overlay nodes, e.g. mapping 
> of
> resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
> 
> This commit is also applied to git://github.com/dgibson/dtc:
>commit: ad9cf6bde5b90d4c1e5a79a2803e98d6344c27d7.

Hi Vikram,

please correct me if I’m wrong, I’ve cloned the repository and I found your 
commit with
this SHA: 45f3d1a095dd3440578d5c6313eba555a791f3fb

Cheers,
Luca

Re: [XEN][RFC PATCH v3 05/14] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree

[Luca Fancellu]

Hi Vikram,

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Add _dt_find_by_path() to find a matching node with path for a dt_device_node.
> 
> Signed-off-by: Vikram Garhwal 
> ---
> xen/common/device_tree.c  | 10 --
> xen/include/xen/device_tree.h |  9 +
> 2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f43d66a501..2e334f949e 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -358,17 +358,23 @@ struct dt_device_node *dt_find_node_by_type(struct 
> dt_device_node *from,
> return np;
> }
> 
> -struct dt_device_node *dt_find_node_by_path(const char *path)
> +struct dt_device_node *_dt_find_node_by_path(struct dt_device_node *dt,
> + const char *path)

A function with a name starting with an underscore feels to me more an internal 
function rather than public,
I suggest to find another name, maybe device_tree_find_node_by_path? 

> {
> struct dt_device_node *np;
> 
> -dt_for_each_device_node(dt_host, np)
> +dt_for_each_device_node(dt, np)
> if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
> break;
> 
> return np;
> }
> 
> +struct dt_device_node *dt_find_node_by_path(const char *path)
> +{
> +return _dt_find_node_by_path(dt_host, path);
> +}

This is public but it’s not declared in device_tree.h. Maybe this can become a 
static inline defined in the header?

Cheers,
Luca

Re: [XEN][RFC PATCH v3 06/14] xen/smmu: Add remove_device callback for smmu_iommu ops

[Luca Fancellu]

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Add remove_device callback for removing the device entry from smmu-master 
> using
> following steps:
> 1. Find if SMMU master exists for the device node.
> 2. Remove the SMMU master
> 
> Signed-off-by: Vikram Garhwal 

Seems ok to me,

Reviewed-by: Luca Fancellu 

Re: [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller

[Luca Fancellu]

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_lock().
> 
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign.
> 
> Signed-off-by: Vikram Garhwal 
> ---
> xen/drivers/passthrough/device_tree.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0dad..b3b04f8e03 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -83,16 +83,14 @@ fail:
> return rc;
> }
> 
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +static bool_t iommu_dt_device_is_assigned_lock(const struct dt_device_node 
> *dev)
> {
> bool_t assigned = 0;
> 

You can add an ASSERT(spin_is_locked(&dtdevs_lock)); to be sure, however the 
name is pretty clear,
so for me with or without it:

Reviewed-by: Luca Fancellu 

> if ( !dt_device_is_protected(dev) )
> return 0;
> 
> -spin_lock(&dtdevs_lock);
> assigned = !list_empty(&dev->domain_list);
> -spin_unlock(&dtdevs_lock);
> 
> return assigned;
> }
> @@ -225,12 +223,17 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, 
> struct domain *d,
> 
> if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> {
> -if ( iommu_dt_device_is_assigned(dev) )
> +spin_lock(&dtdevs_lock);
> +
> +if ( iommu_dt_device_is_assigned_lock(dev) )
> {
> printk(XENLOG_G_ERR "%s already assigned.\n",
>dt_node_full_name(dev));
> ret = -EINVAL;
> }
> +
> +spin_unlock(&dtdevs_lock);
> +
> break;
> }
> 
> 

Re: [XEN][RFC PATCH v3 08/14] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock

[Luca Fancellu]

> On 8 Mar 2022, at 19:46, Vikram Garhwal  wrote:
> 
> Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access 
> add.
> 
> Signed-off-by: Vikram Garhwal 

Reviewed-by: Luca Fancellu 

> ---
> xen/drivers/passthrough/device_tree.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index b3b04f8e03..776809a8f2 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -145,6 +145,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
> if ( dev_iommu_fwspec_get(dev) )
> return 0;
> 
> +spin_lock(&dtdevs_lock);
> +
> /*
>  * According to the Documentation/devicetree/bindings/iommu/iommu.txt
>  * from Linux.
> @@ -157,7 +159,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
>  * these callback implemented.
>  */
> if ( !ops->add_device || !ops->dt_xlate )
> -return -EINVAL;
> +{
> +rc = -EINVAL;
> +goto fail;
> +}
> 
> if ( !dt_device_is_available(iommu_spec.np) )
> break;
> @@ -188,6 +193,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
> if ( rc < 0 )
> iommu_fwspec_free(dev);
> 
> +fail:
> +spin_unlock(&dtdevs_lock);
> return rc;
> }
> 
> 

Re: [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device()

[Luca Fancellu]

> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> +const struct iommu_ops *ops = iommu_get_ops();
> +struct device *dev = dt_to_dev(np);
> +int rc;
> +
> +if ( !ops )
> +return -EOPNOTSUPP;

Here we have that the counterpart iommu_add_dt_device returns EINVAL here and...

> +
> +spin_lock(&dtdevs_lock);
> +
> +if ( iommu_dt_device_is_assigned_lock(np) ) {
> +rc = -EBUSY;
> +goto fail;
> +}
> +
> +/*
> + * The driver which supports generic IOMMU DT bindings must have
> + * these callback implemented.
> + */
> +if ( !ops->remove_device ) {
> +rc = -EOPNOTSUPP;

… here (for !ops->add_device), so I’m wondering if there is a mistake.

> +goto fail;
> +}
> +

Re: [PATCH v2 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

> Hmm, this will fail the ASSERT(spin_is_locked(&cpupool_lock)) in
> __cpupool_find_by_id().
> 
> I think you need to use cpupool_get_by_id() and cpupool_put() by making them
> globally visible (move their prototypes to sched.h).

Hi Juergen,

Yes I was thinking more to a __init wrapper that takes the lock and calls 
__cpupool_find_by_id,
this to avoid exporting cpupool_put outside since we would be the only user.
But I would like your opinion on that.

Cheers,
Luca

> 
> 
> Juergen
> 

Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities

[Luca Fancellu]

> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index dc8d3a13f5..2eb5734f8e 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -54,6 +54,7 @@ obj-y += wait.o
> obj-bin-y += warning.init.o
> obj-$(CONFIG_XENOPROF) += xenoprof.o
> obj-y += xmalloc_tlsf.o
> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o

I think the entries are in alphabetical order, this should be added after += 
domain.o

> +/*
> + * overlay_get_nodes_info will get the all node's full name with path. This 
> is
> + * useful when checking node for duplication i.e. dtbo tries to add nodes 
> which
> + * already exists in device tree.
> + */
> +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
> +  unsigned int num_overlay_nodes)
> +{
> +int fragment;
> +unsigned int node_num = 0;
> +
> +*nodes_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));

Here you can use xzalloc_bytes and...

> +
> +if ( *nodes_full_path == NULL )
> +return -ENOMEM;
> +memset(*nodes_full_path, 0x0, num_overlay_nodes * sizeof(char *));

Get rid of this memset

> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(char **full_dt_node_path, int **nodes_irq,
> +int *node_num_irq, unsigned int num_nodes)
> +{
> +struct domain *d = hardware_domain;
> +int rc = 0;
> +struct dt_device_node *overlay_node;
> +unsigned int naddr;
> +unsigned int i, j, nirq;
> +u64 addr, size;
> +domid_t domid = 0;
> +
> +for ( j = 0; j < num_nodes; j++ )
> +{
> +dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[j]);
> +
> +overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
> +
> +if ( overlay_node == NULL )
> +{
> +printk(XENLOG_ERR "Device %s is not present in the tree. 
> Removing nodes failed\n",
> +   full_dt_node_path[j]);
> +return -EINVAL;
> +}
> +
> +domid = dt_device_used_by(overlay_node);
> +
> +dt_dprintk("Checking if node %s is used by any domain\n",
> +   full_dt_node_path[j]);
> +
> +/* Remove the node iff it's assigned to domain 0 or domain io. */
> +if ( domid != 0 && domid != DOMID_IO )
> +{
> +printk(XENLOG_ERR "Device %s as it is being used by domain %d. 
> Removing nodes failed\n",
> +   full_dt_node_path[j], domid);
> +return -EINVAL;
> +}
> +
> +dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> +
> +nirq = node_num_irq[j];
> +
> +/* Remove IRQ permission */
> +for ( i = 0; i < nirq; i++ )
> +{
> +rc = nodes_irq[j][i];
> +/*
> + * TODO: We don't handle shared IRQs for now. So, it is assumed 
> that
> + * the IRQs was not shared with another domain.
> + */
> +rc = irq_deny_access(d, rc);
> +if ( rc )
> +{
> +printk(XENLOG_ERR "unable to revoke access for irq %u for 
> %s\n",
> +   i, dt_node_full_name(overlay_node));
> +return rc;
> +}
> +}
> +
> +rc = iommu_remove_dt_device(overlay_node);
> +if ( rc != 0 && rc != -ENXIO )
> +return rc;
> +
> +naddr = dt_number_of_address(overlay_node);
> +
> +/* Remove mmio access. */
> +for ( i = 0; i < naddr; i++ )
> +{
> +rc = dt_device_get_address(overlay_node, i, &addr, &size);
> +if ( rc )
> +{
> +printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> +   i, dt_node_full_name(overlay_node));
> +return rc;
> +}
> +
> +rc = iomem_deny_access(d, paddr_to_pfn(addr),
> +   paddr_to_pfn(PAGE_ALIGN(addr + size - 
> 1)));
> +if ( rc )
> +{
> +printk(XENLOG_ERR "Unable to remove dom%d access to"
> +" 0x%"PRIx64" - 0x%"PRIx64"\n",
> +d->domain_id,
> +addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);

NIT: here in each line under XENLOG_ERR, there is an extra space, these lines
Could be aligned to XENLOG_ERR, just for code style purpose.

> +return rc;
> +}
> +}
> +
> +rc = dt_overlay_remove_node(overlay_node);
> +if ( rc )
> +return rc;
> +}
> +
> +return rc;
> +}
> +
> 
> +long dt_sysctl(struct xen_sysctl *op)
> +{
> +long ret = 0;
> +void *overlay_fdt;
> +char **nodes_full_path = NULL;
> +unsigned int num_overlay_nodes = 0;
> +
> +if ( op->u.dt_overlay.overlay_fdt_size <= 0 )
> +return -EINVAL;
> +
> +overlay_fdt = xmalloc_bytes(op->u.dt_overlay.overlay_fdt_size);
> +
> +if ( overlay_fdt == NULL )
> +ret

Re: [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities

[Luca Fancellu]

> 
> +static int dt_overlay_add_node(struct dt_device_node *device_node,
> +  const char *parent_node_path)

Const should be indented at the same level of struct

> +/*
> + * Adds device tree nodes under target node.
> + * We use dt_host_new to unflatten the updated device_tree_flattened. This is
> + * done to avoid the removal of device_tree generation, iomem regions 
> mapping to
> + * hardware domain done by handle_node().
> + */
> +static long handle_add_overlay_nodes(void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> +int rc = 0;
> +struct dt_device_node *overlay_node;
> +char **nodes_full_path = NULL;
> +int **nodes_irq = NULL;
> +int *node_num_irq = NULL;
> +void *fdt = NULL;
> +struct dt_device_node *dt_host_new = NULL;
> +struct domain *d = hardware_domain;
> +struct overlay_track *tr = NULL;
> +unsigned int naddr;
> +unsigned int num_irq;
> +unsigned int i, j, k;
> +unsigned int num_overlay_nodes;

All unsigned int can stay in the same line

> +u64 addr, size;
> +
> +fdt = xmalloc_bytes(fdt_totalsize(device_tree_flattened));
> +if ( fdt == NULL )
> +return -ENOMEM;
> +
> +num_overlay_nodes = overlay_node_count(overlay_fdt);
> +if ( num_overlay_nodes == 0 )
> +{
> +xfree(fdt);
> +return -ENOMEM;
> +}
> +
> +spin_lock(&overlay_lock);
> +
> +memcpy(fdt, device_tree_flattened, fdt_totalsize(device_tree_flattened));
> +
> +rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> +if ( rc )
> +{
> +xfree(fdt);
> +return rc;
> +}
> +
> +/*
> + * overlay_get_nodes_info is called to get the node information from 
> dtbo.
> + * This is done before fdt_overlay_apply() because the overlay apply will
> + * erase the magic of overlay_fdt.
> + */
> +rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> +num_overlay_nodes);
> +if ( rc )
> +{
> +printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
> +   rc);
> +goto err;
> +}
> +
> +nodes_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int *));

You can use xzalloc_bytes and remove the memset below here and...

> +
> +if ( nodes_irq == NULL )
> +{
> +rc = -ENOMEM;
> +goto err;
> +}
> +memset(nodes_irq, 0x0, num_overlay_nodes * sizeof(int *));
> +
> +node_num_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int));

Here

> +if ( node_num_irq == NULL )
> +{
> +rc = -ENOMEM;
> +goto err;
> +}
> +memset(node_num_irq, 0x0, num_overlay_nodes * sizeof(int));
> +
> +rc = fdt_overlay_apply(fdt, overlay_fdt);
> +if ( rc )
> +{
> +printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
> +goto err;
> +}
> +
> +   […]
> +err:
> +spin_unlock(&overlay_lock);
> +
> +xfree(dt_host_new);
> +xfree(fdt);
> +
> +if ( nodes_full_path != NULL )
> +{
> +for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; 
> i++ )
> +{
> +xfree(nodes_full_path[i]);
> +}
> +xfree(nodes_full_path);
> +}
> +
> +if ( nodes_irq != NULL )
> +{
> +for ( i = 0; i < num_overlay_nodes && nodes_irq[i] != NULL; i++ )
> +{
> +xfree(nodes_irq[i]);
> +}
> +xfree(nodes_irq);
> +}

I see you use this operation quite a bit in this module, perhaps you can create 
a function to
do that

Re: [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay

[Luca Fancellu]

> 
> diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
> index ef7362327f..848a8737c7 100644
> --- a/tools/libs/ctrl/Makefile
> +++ b/tools/libs/ctrl/Makefile
> @@ -3,6 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
> 
> SRCS-y   += xc_altp2m.c
> SRCS-y   += xc_cpupool.c
> +SRCS-y   += xc_overlay.c

I think these entries are in alphabetical order

> SRCS-y   += xc_domain.c
> SRCS-y   += xc_evtchn.c
> SRCS-y   += xc_gnttab.c
> diff --git a/tools/libs/ctrl/xc_overlay.c b/tools/libs/ctrl/xc_overlay.c
> new file mode 100644
> index 00..8fe780d75a
> --- /dev/null
> +++ b/tools/libs/ctrl/xc_overlay.c
> @@ -0,0 +1,51 @@
> +/*
> + *

This blank line can be removed 

> + * Overlay control functions.
> + * Copyright (C) 2021 Xilinx Inc.
> + * Author Vikram Garhwal 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see 
> .
> + */
> +
> +#include "xc_bitops.h"
> +#include "xc_private.h"
> +#include 
> +#include 
> +
> +int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
> +  uint8_t overlay_op)
> +{
> +int err;
> +DECLARE_SYSCTL;
> +
> +DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
> +XC_HYPERCALL_BUFFER_BOUNCE_IN);

XC_HYPERCALL_BUFFER_BOUNCE_IN can stay at the same level of overlay_fdt

Re: [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops

[Luca Fancellu]

> On 8 Mar 2022, at 19:47, Vikram Garhwal  wrote:
> 
> Signed-off-by: Vikram Garhwal 

I don’t know if an empty commit message is ok here, will leave to the maintainer
the choice, for me the code looks ok

Reviewed-by: Luca Fancellu 

Re: [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support

[Luca Fancellu]

> On 8 Mar 2022, at 19:47, Vikram Garhwal  wrote:
> 
> Signed-off-by: Vikram Garhwal 
> ---
> tools/xl/xl.h   |  4 
> tools/xl/xl_cmdtable.c  |  6 ++
> tools/xl/xl_vmcontrol.c | 45 +
> 3 files changed, 55 insertions(+)
> 
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..604fd5bb94 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -97,6 +97,9 @@ struct save_file_header {
> 
> #define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
> 
> +#define XL_DT_OVERLAY_ADD   1
> +#define XL_DT_OVERLAY_REMOVE2

Maybe you can get rid of them and ...

> +
> void save_domain_core_begin(uint32_t domid,
> int preserve_domid,
> const char *override_config_file,
> @@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv);
> int main_reboot(int argc, char **argv);
> int main_list(int argc, char **argv);
> int main_vm_list(int argc, char **argv);
> +int main_dt_overlay(int argc, char **argv);
> int main_create(int argc, char **argv);
> int main_config_update(int argc, char **argv);
> int main_button_press(int argc, char **argv);
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 661323d488..5812d19db8 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -20,6 +20,12 @@
> #include "xl.h"
> 
> const struct cmd_spec cmd_table[] = {
> +{ "overlay",
> +  &main_dt_overlay, 1, 1,
> +  "Add/Remove a device tree overlay",
> +  "add/remove <.dtbo>"
> +  "-h print this help\n"
> +},
> { "create",
>   &main_create, 1, 1,
>   "Create a domain from config file ",
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 435155a033..76b969dc33 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv)
> return 0;
> }
> 
> +int main_dt_overlay(int argc, char **argv)
> +{
> +const char *overlay_ops = argv[1];
> +const char *overlay_config_file = argv[2];
> +void *overlay_dtb = NULL;
> +int rc;
> +uint8_t op;
> +int overlay_dtb_size = 0;
> +
> +if (overlay_ops == NULL) {
> +fprintf(stderr, "No overlay operation mode provided\n");
> +return ERROR_FAIL;
> +}
> +
> +if (strcmp(overlay_ops, "add") == 0)
> +op = XL_DT_OVERLAY_ADD;
> +else if (strcmp(overlay_ops, "remove") == 0)
> +op = XL_DT_OVERLAY_REMOVE;

Use them there, maybe defining const int  = 

Since these values are only used there

Re: [PATCH v2 2/6] xen/sched: create public function for cpupools creation

[Luca Fancellu]

> On 15 Mar 2022, at 17:45, Andrew Cooper  wrote:
> 
> On 10/03/2022 17:10, Luca Fancellu wrote:
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 10ea969c7af9..47fc856e0fe0 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct 
>> cpupool *c);
>> int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
>> unsigned int cpupool_get_id(const struct domain *d);
>> const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
>> +
>> +/*
>> + * cpupool_create_pool - Creates a cpupool
>> + * @pool_id: id of the pool to be created
>> + * @sched_id: id of the scheduler to be used for the pool
>> + *
>> + * Creates a cpupool with pool_id id.
>> + * The sched_id parameter identifies the scheduler to be used, if it is
>> + * negative, the default scheduler of Xen will be used.
>> + *
>> + * returns:
>> + * pointer to the struct cpupool just created, on success
>> + * NULL, on cpupool creation error
> 
> What makes you say this?  Your new function will fall over a NULL
> pointer before it returns one...

You are right, it’s a leftover from the v1, I will change it and review the 
code that uses it.

Cheers,
Luca

> 
> ~Andrew

Re: [PATCH 1/3] tools/xenstore: add documentation for new set/get-feature commands

[Luca Fancellu]

> On 16 Mar 2022, at 16:10, Juergen Gross  wrote:
> 
> Add documentation for two new Xenstore wire commands SET_FEATURE and
> GET_FEATURE used to set or query the Xenstore features visible in the
> ring page of a given domain.
> 
> Signed-off-by: Juergen Gross 

Reviewed-by: Luca Fancellu 

Cheers,
Luca

> ---
> docs/misc/xenstore-ring.txt |  1 +
> docs/misc/xenstore.txt  | 12 
> 2 files changed, 13 insertions(+)
> 
> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> index f91accb5b0..bd000f694e 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -68,6 +68,7 @@ MaskDescription
> -
> 1   Ring reconnection (see the ring reconnection feature below)
> 2   Connection error indicator (see connection error feature below)
> +4   GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> 
> The "Connection state" field is used to request a ring close and reconnect.
> The "Connection state" field only contains valid data if the server has
> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> index ea3d8be177..31e3d53c52 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -332,6 +332,18 @@ SET_TARGET   ||
> 
>   xenstored prevents the use of SET_TARGET other than by dom0.
> 
> +GET_FEATURE  ||
> +SET_FEATURE  ||
> + Returns or sets the contents of the "feature" field located at
> + offset 2064 of the Xenstore ring page of the domain specified by
> + .  is a decimal number being a logical or of the
> + feature bits as defined in docs/misc/xenstore-ring.txt. Trying
> + to set a bit for a feature not being supported by the running
> + Xenstore will be denied.
> +
> + xenstored prevents the use of GET_FEATURE and SET_FEATURE other
> + than by dom0.
> +
> -- Miscellaneous --
> 
> CONTROL   |[|]
> -- 
> 2.34.1
> 
> 

Re: [PATCH 2/3] tools/xenstore: add documentation for new set/get-quota commands

[Luca Fancellu]

> On 16 Mar 2022, at 16:10, Juergen Gross  wrote:
> 
> Add documentation for two new Xenstore wire commands SET_QUOTA and
> GET_QUOTA used to set or query the Xenstore quota of a given domain.
> 
> Signed-off-by: Juergen Gross 

Reviewed-by: Luca Fancellu 

Cheers,
Luca

> ---
> docs/misc/xenstore-ring.txt |  1 +
> docs/misc/xenstore.txt  | 12 
> 2 files changed, 13 insertions(+)
> 
> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> index bd000f694e..0cb72a3e35 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -69,6 +69,7 @@ MaskDescription
> 1   Ring reconnection (see the ring reconnection feature below)
> 2   Connection error indicator (see connection error feature below)
> 4   GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> +8   GET_QUOTA and SET_QUOTA Xenstore wire commands are available
> 
> The "Connection state" field is used to request a ring close and reconnect.
> The "Connection state" field only contains valid data if the server has
> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> index 31e3d53c52..dd75a81328 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -344,6 +344,18 @@ SET_FEATURE  ||
>   xenstored prevents the use of GET_FEATURE and SET_FEATURE other
>   than by dom0.
> 
> +GET_QUOTA|||
> +SET_QUOTA|||
> + Returns or sets a quota value for the domain being specified by
> + .  is one of "nodes", "watches", "transactions",
> + "node-size" or "permissions".  is a decimal number
> + specifying the quota value, with "0" having the special meaning
> + of quota checks being disabled. The initial quota settings for
> + a domain are the global ones of Xenstore.
> +
> + xenstored prevents the use of GET_QUOTA and SET_QUOTA other
> + than by dom0.
> +
> -- Miscellaneous --
> 
> CONTROL   |[|]
> -- 
> 2.34.1
> 
> 

Re: [PATCH 3/3] tools/xenstore: add documentation for extended watch command

[Luca Fancellu]

> On 16 Mar 2022, at 16:10, Juergen Gross  wrote:
> 
> Add documentation for an extension of the WATCH command used to limit
> the scope of watched paths. Additionally it enables to receive more
> information in the events related to special watches (@introduceDomain
> or @releaseDomain).
> 
> Signed-off-by: Juergen Gross 
> ---
> docs/misc/xenstore-ring.txt |  1 +
> docs/misc/xenstore.txt  | 16 +---
> 2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt
> index 0cb72a3e35..eaa6d0a1a3 100644
> --- a/docs/misc/xenstore-ring.txt
> +++ b/docs/misc/xenstore-ring.txt
> @@ -70,6 +70,7 @@ MaskDescription
> 2   Connection error indicator (see connection error feature below)
> 4   GET_FEATURE and SET_FEATURE Xenstore wire commands are available
> 8   GET_QUOTA and SET_QUOTA Xenstore wire commands are available
> +16  WATCH can take a third parameter limiting its scope
> 
> The "Connection state" field is used to request a ring close and reconnect.
> The "Connection state" field only contains valid data if the server has
> diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt
> index dd75a81328..f86c6d9757 100644
> --- a/docs/misc/xenstore.txt
> +++ b/docs/misc/xenstore.txt
> @@ -188,7 +188,7 @@ SET_PERMS ||+?
> 
> -- Watches --
> 
> -WATCH||?
> +WATCH||[|]?
>   Adds a watch.
> 
>   When a  is modified (including path creation, removal,
> @@ -199,7 +199,11 @@ WATCH||?
>   matching watch results in a WATCH_EVENT message (see below).
> 
>   The event's path matches the watch's  if it is an child
> - of .
> + of . This match can be limited by specifying  (a
> + decimal value of 0 or larger): it denotes the directory levels
> + below  to consider for a match ("0" would not match for
> + a child of , "1" would match only for a direct child,
> + etc.).
> 
>can be a  to watch or @.  In the
>   latter case  may have any syntax but it matches
> @@ -210,7 +214,13 @@ WATCH||?
>   shutdown, and also on RELEASE
>   and domain destruction
>events are sent to privileged callers or explicitly
> - via SET_PERMS enabled domains only.
> + via SET_PERMS enabled domains only. The semantics for a
> + specification of  differ for generating 

Typo: s/differ/differs/?

> + events: specifying "1" will report the related domid by using
> + @/ for the reported path. Other 
> + values are not supported.
> + For @releaseDomain it is possible to watch only for a specific
> + domain by specifying @releaseDomain/ for the path.
> 
>   When a watch is first set up it is triggered once straight
>   away, with  equal to .  Watches may be triggered
> -- 
> 2.34.1
> 
> 

I’m not an English native speaker so apologies if there is no mistake.

With that fixed (if it’s wrong):

Reviewed-by: Luca Fancellu 

Cheers,
Luca

Re: [XEN PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves

[Luca Fancellu]

> On 16 Mar 2022, at 18:58, Andrew Cooper  wrote:
> 
> On 16/03/2022 18:38, Raphael Ning wrote:
>> From: Raphael Ning 
>> 
>> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
>> if it fails to lock the FIFO event queue(s), or if the guest has not
>> initialized the FIFO control block for the target vCPU. A well-behaved
>> guest should never trigger either of these cases.
>> 
>> There is no good reason to set the PENDING bit (the guest should not
>> depend on this behaviour anyway) or check for pollers in such corner
>> cases, so skip that. In fact, both the comment above the for loop and
>> the commit message for
>> 
>> 41a822c39263 xen/events: rework fifo queue locking
>> 
>> suggest that the bit should be set after the FIFO queue(s) are locked.
>> 
>> Take the opportunity to rename the was_pending variable (flipping its
>> sense) and switch to the standard bool type.
>> 
>> Suggested-by: David Vrabel 
>> Signed-off-by: Raphael Ning 
>> ---
>> xen/common/event_fifo.c | 8 
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>> index ed4d3beb10f3..6c74ccebebb7 100644
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
>> unsigned int port;
>> event_word_t *word;
>> unsigned long flags;
>> -bool_t was_pending;
>> +bool_t check_pollers = false;
> 
> Considering your commit message, did you intend to change this to bool?
> 
> Can be fixed on commit.  Acked-by: Andrew Cooper 
> 

I’ve tested on the ARM side, I’ve started/destroyed few guests from Dom0, 
connect to the console, run
some network communications between guest and Dom0, everything works:

Tested-by: Luca Fancellu 

Cheers,
Luca

> ~Andrew
> 
> P.S. David - do you want your maintainership back?  None of this code
> has undergone any major changes since you wrote it.
> 

Re: [PATCH] Livepatch: fix typos

[Luca Fancellu]

> On 11 Mar 2022, at 19:11, Bjoern Doebel  wrote:
> 
> Fix a couple of typos in livepatch code.

NIT: I would say: [...] in livepatch code comments.

> 
> Signed-off-by: Bjoern Doebel 

With or without it:

Reviewed-by: Luca Fancellu 

Cheers,
Luca

> CC: Konrad Rzeszutek Wilk 
> CC: Ross Lagerwall 

Re: [PATCH] xen/arm: fix typos in comments

[Luca Fancellu]

> On 18 Mar 2022, at 10:37, Julia Lawall  wrote:
> 
> Various spelling mistakes in comments.
> Detected with the help of Coccinelle.
> 
> Signed-off-by: Julia Lawall 

Reviewed-by: Luca Fancellu 

Cheers,
Luca

> 
> ---
> arch/arm/xen/mm.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index a7e54a087b80..607c1a557ccc 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -105,7 +105,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
>*  - The Linux page refers to foreign memory
>*  - The device doesn't support coherent DMA request
>*
> -  * The Linux page may be spanned acrros multiple Xen page, although
> +  * The Linux page may be spanned across multiple Xen page, although
>* it's not possible to have a mix of local and foreign Xen page.
>* Furthermore, range_straddles_page_boundary is already checking
>* if buffer is physically contiguous in the host RAM.
> 
> 

[PATCH v3 1/6] tools/cpupools: Give a name to unnamed cpupools

[Luca Fancellu]

With the introduction of boot time cpupools, Xen can create many
different cpupools at boot time other than cpupool with id 0.

Since these newly created cpupools can't have an
entry in Xenstore, create the entry using xen-init-dom0
helper with the usual convention: Pool-.

Given the change, remove the check for poolid == 0 from
libxl_cpupoolid_to_name(...).

Signed-off-by: Luca Fancellu 
Reviewed-by: Juergen Gross 
---
Changes in v3:
- no changes, add R-by
Changes in v2:
 - Remove unused variable, moved xc_cpupool_infofree
   ahead to simplify the code, use asprintf (Juergen)
---
 tools/helpers/xen-init-dom0.c  | 35 +-
 tools/libs/light/libxl_utils.c |  3 +--
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
index c99224a4b607..84286617790f 100644
--- a/tools/helpers/xen-init-dom0.c
+++ b/tools/helpers/xen-init-dom0.c
@@ -43,7 +43,9 @@ int main(int argc, char **argv)
 int rc;
 struct xs_handle *xsh = NULL;
 xc_interface *xch = NULL;
-char *domname_string = NULL, *domid_string = NULL;
+char *domname_string = NULL, *domid_string = NULL, *pool_path, *pool_name;
+xc_cpupoolinfo_t *xcinfo;
+unsigned int pool_id = 0;
 libxl_uuid uuid;
 
 /* Accept 0 or 1 argument */
@@ -114,6 +116,37 @@ int main(int argc, char **argv)
 goto out;
 }
 
+/* Create an entry in xenstore for each cpupool on the system */
+do {
+xcinfo = xc_cpupool_getinfo(xch, pool_id);
+if (xcinfo != NULL) {
+if (xcinfo->cpupool_id != pool_id)
+pool_id = xcinfo->cpupool_id;
+xc_cpupool_infofree(xch, xcinfo);
+if (asprintf(&pool_path, "/local/pool/%d/name", pool_id) <= 0) {
+fprintf(stderr, "cannot allocate memory for pool path\n");
+rc = 1;
+goto out;
+}
+if (asprintf(&pool_name, "Pool-%d", pool_id) <= 0) {
+fprintf(stderr, "cannot allocate memory for pool name\n");
+rc = 1;
+goto out_err;
+}
+pool_id++;
+if (!xs_write(xsh, XBT_NULL, pool_path, pool_name,
+  strlen(pool_name))) {
+fprintf(stderr, "cannot set pool name\n");
+rc = 1;
+}
+free(pool_name);
+out_err:
+free(pool_path);
+if ( rc )
+goto out;
+}
+} while(xcinfo != NULL);
+
 printf("Done setting up Dom0\n");
 
 out:
diff --git a/tools/libs/light/libxl_utils.c b/tools/libs/light/libxl_utils.c
index b91c2cafa223..81780da3ff40 100644
--- a/tools/libs/light/libxl_utils.c
+++ b/tools/libs/light/libxl_utils.c
@@ -151,8 +151,7 @@ char *libxl_cpupoolid_to_name(libxl_ctx *ctx, uint32_t 
poolid)
 
 snprintf(path, sizeof(path), "/local/pool/%d/name", poolid);
 s = xs_read(ctx->xsh, XBT_NULL, path, &len);
-if (!s && (poolid == 0))
-return strdup("Pool-0");
+
 return s;
 }
 
-- 
2.17.1

[PATCH v3 5/6] arm/dom0less: assign dom0less guests to cpupools

[Luca Fancellu]

Introduce domain-cpupool property of a xen,domain device tree node,
that specifies the cpupool device tree handle of a xen,cpupool node
that identifies a cpupool created at boot time where the guest will
be assigned on creation.

Add member to the xen_domctl_createdomain public interface so the
XEN_DOMCTL_INTERFACE_VERSION version is bumped.

Add public function to retrieve a pool id from the device tree
cpupool node.

Update documentation about the property.

Signed-off-by: Luca Fancellu 
---
Changes in v3:
- Use explicitely sized integer for struct xen_domctl_createdomain
  cpupool_id member. (Stefano)
- Changed code due to previous commit code changes
Changes in v2:
- Moved cpupool_id from arch specific to common part (Juergen)
- Implemented functions to retrieve the cpupool id from the
  cpupool dtb node.
---
 docs/misc/arm/device-tree/booting.txt |  5 +
 xen/arch/arm/domain_build.c   | 14 +-
 xen/common/boot_cpupools.c| 24 
 xen/common/domain.c   |  2 +-
 xen/include/public/domctl.h   |  4 +++-
 xen/include/xen/sched.h   |  9 +
 6 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt 
b/docs/misc/arm/device-tree/booting.txt
index a94125394e35..7b4a29a2c293 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -188,6 +188,11 @@ with the following properties:
 An empty property to request the memory of the domain to be
 direct-map (guest physical address == physical address).
 
+- domain-cpupool
+
+Optional. Handle to a xen,cpupool device tree node that identifies the
+cpupool where the guest will be started at boot.
+
 Under the "xen,domain" compatible node, one or more sub-nodes are present
 for the DomU kernel and ramdisk.
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de05..9c67a483d4a4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3172,7 +3172,8 @@ static int __init construct_domU(struct domain *d,
 void __init create_domUs(void)
 {
 struct dt_device_node *node;
-const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+const struct dt_device_node *cpupool_node,
+*chosen = dt_find_node_by_path("/chosen");
 
 BUG_ON(chosen == NULL);
 dt_for_each_child_node(chosen, node)
@@ -3241,6 +3242,17 @@ void __init create_domUs(void)
  vpl011_virq - 32 + 1);
 }
 
+/* Get the optional property domain-cpupool */
+cpupool_node = dt_parse_phandle(node, "domain-cpupool", 0);
+if ( cpupool_node )
+{
+int pool_id = btcpupools_get_domain_pool_id(cpupool_node);
+if ( pool_id < 0 )
+panic("Error getting cpupool id from domain-cpupool (%d)\n",
+  pool_id);
+d_cfg.cpupool_id = pool_id;
+}
+
 /*
  * The variable max_init_domid is initialized with zero, so here it's
  * very important to use the pre-increment operator to call
diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c
index f6f2fa8f2701..feba93a243fc 100644
--- a/xen/common/boot_cpupools.c
+++ b/xen/common/boot_cpupools.c
@@ -23,6 +23,8 @@ static unsigned int __initdata next_pool_id;
 
 #define BTCPUPOOLS_DT_NODE_NO_REG (-1)
 #define BTCPUPOOLS_DT_NODE_NO_LOG_CPU (-2)
+#define BTCPUPOOLS_DT_WRONG_NODE  (-3)
+#define BTCPUPOOLS_DT_CORRUPTED_NODE  (-4)
 
 static int __init get_logical_cpu_from_hw_id(unsigned int hwid)
 {
@@ -55,6 +57,28 @@ get_logical_cpu_from_cpu_node(const struct dt_device_node 
*cpu_node)
 return cpu_num;
 }
 
+int __init btcpupools_get_domain_pool_id(const struct dt_device_node *node)
+{
+const struct dt_device_node *phandle_node;
+int cpu_num;
+
+if ( !dt_device_is_compatible(node, "xen,cpupool") )
+return BTCPUPOOLS_DT_WRONG_NODE;
+/*
+ * Get first cpu listed in the cpupool, from its reg it's possible to
+ * retrieve the cpupool id.
+ */
+phandle_node = dt_parse_phandle(node, "cpupool-cpus", 0);
+if ( !phandle_node )
+return BTCPUPOOLS_DT_CORRUPTED_NODE;
+
+cpu_num = get_logical_cpu_from_cpu_node(phandle_node);
+if ( cpu_num < 0 )
+return cpu_num;
+
+return pool_cpu_map[cpu_num];
+}
+
 static int __init check_and_get_sched_id(const char* scheduler_name)
 {
 int sched_id = sched_get_id_by_name(scheduler_name);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 351029f8b239..0827400f4f49 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -698,7 +698,7 @@ struct domain *domain_create(domid_t domid,
 if ( !d->pbuf )
 goto fail;
 
-if ( (err = sched_init_domain(d, 0)) != 0 )
+  

[PATCH v3 4/6] xen/cpupool: Create different cpupools at boot time

[Luca Fancellu]

Introduce a way to create different cpupools at boot time, this is
particularly useful on ARM big.LITTLE system where there might be the
need to have different cpupools for each type of core, but also
systems using NUMA can have different cpu pools for each node.

The feature on arm relies on a specification of the cpupools from the
device tree to build pools and assign cpus to them.

Documentation is created to explain the feature.

Signed-off-by: Luca Fancellu 
---
Changes in v3:
- Add newline to cpupools.txt and removed "default n" from Kconfig (Jan)
- Fixed comment, moved defines, used global cpu_online_map, use
  HAS_DEVICE_TREE instead of ARM and place arch specific code in header
  (Juergen)
- Fix brakets, x86 code only panic, get rid of scheduler dt node, don't
  save pool pointer and look for it from the pool list (Stefano)
- Changed data structures to allow modification to the code.
Changes in v2:
- Move feature to common code (Juergen)
- Try to decouple dtb parse and cpupool creation to allow
  more way to specify cpupools (for example command line)
- Created standalone dt node for the scheduler so it can
  be used in future work to set scheduler specific
  parameters
- Use only auto generated ids for cpupools
---
 docs/misc/arm/device-tree/cpupools.txt | 135 +++
 xen/arch/arm/include/asm/smp.h |   3 +
 xen/common/Kconfig |   7 +
 xen/common/Makefile|   1 +
 xen/common/boot_cpupools.c | 178 +
 xen/common/sched/cpupool.c |   9 +-
 xen/include/xen/sched.h|  19 +++
 7 files changed, 351 insertions(+), 1 deletion(-)
 create mode 100644 docs/misc/arm/device-tree/cpupools.txt
 create mode 100644 xen/common/boot_cpupools.c

diff --git a/docs/misc/arm/device-tree/cpupools.txt 
b/docs/misc/arm/device-tree/cpupools.txt
new file mode 100644
index ..6d7463736b48
--- /dev/null
+++ b/docs/misc/arm/device-tree/cpupools.txt
@@ -0,0 +1,135 @@
+Boot time cpupools
+==
+
+When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible to
+create cpupools during boot phase by specifying them in the device tree.
+
+Cpupools specification nodes shall be direct childs of /chosen node.
+Each cpupool node contains the following properties:
+
+- compatible (mandatory)
+
+Must always include the compatiblity string: "xen,cpupool".
+
+- cpupool-cpus (mandatory)
+
+Must be a list of device tree phandle to nodes describing cpus (e.g. having
+device_type = "cpu"), it can't be empty.
+
+- cpupool-sched (optional)
+
+Must be a string having the name of a Xen scheduler, it has no effect when
+used in conjunction of a cpupool-id equal to zero, in that case the
+default Xen scheduler is selected (sched=<...> boot argument).
+Check the sched=<...> boot argument for allowed values.
+
+
+Constraints
+===
+
+If no cpupools are specified, all cpus will be assigned to one cpupool
+implicitly created (Pool-0).
+
+If cpupools node are specified, but not every cpu brought up by Xen is 
assigned,
+all the not assigned cpu will be assigned to an additional cpupool.
+
+If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen will
+stop.
+
+
+Examples
+
+
+A system having two types of core, the following device tree specification will
+instruct Xen to have two cpupools:
+
+- The cpupool with id 0 will have 4 cpus assigned.
+- The cpupool with id 1 will have 2 cpus assigned.
+
+The following example can work only if hmp-unsafe=1 is passed to Xen boot
+arguments, otherwise not all cores will be brought up by Xen and the cpupool
+creation process will stop Xen.
+
+
+a72_1: cpu@0 {
+compatible = "arm,cortex-a72";
+reg = <0x0 0x0>;
+device_type = "cpu";
+[...]
+};
+
+a72_2: cpu@1 {
+compatible = "arm,cortex-a72";
+reg = <0x0 0x1>;
+device_type = "cpu";
+[...]
+};
+
+a53_1: cpu@100 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x100>;
+device_type = "cpu";
+[...]
+};
+
+a53_2: cpu@101 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x101>;
+device_type = "cpu";
+[...]
+};
+
+a53_3: cpu@102 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x102>;
+device_type = "cpu";
+[...]
+};
+
+a53_4: cpu@103 {
+compatible = "arm,cortex-a53";
+reg = <0x0 0x103>;
+device_type = "cpu";
+[...]
+};
+
+chosen {
+
+cpupool_a {
+compatible = "xen,cpupool";
+cpupool-cpus = <&a53_1 &a53_2 &a53_3 &a53_4>;
+};
+cpupool_b {
+compatible = "xen,cpupool";
+cpupool-cpus = <&

[PATCH v3 6/6] xen/cpupool: Allow cpupool0 to use different scheduler

[Luca Fancellu]

Currently cpupool0 can use only the default scheduler, and
cpupool_create has an hardcoded behavior when creating the pool 0
that doesn't allocate new memory for the scheduler, but uses the
default scheduler structure in memory.

With this commit it is possible to allocate a different scheduler for
the cpupool0 when using the boot time cpupool.
To achieve this the hardcoded behavior in cpupool_create is removed
and the cpupool0 creation is moved.

When compiling without boot time cpupools enabled, the current
behavior is maintained (except that cpupool0 scheduler memory will be
allocated).

Signed-off-by: Luca Fancellu 
---
Changes in v3:
- fix typo in commit message (Juergen)
- rebase changes
Changes in v2:
- new patch
---
 xen/common/boot_cpupools.c | 5 -
 xen/common/sched/cpupool.c | 8 +---
 xen/include/xen/sched.h| 5 -
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/common/boot_cpupools.c b/xen/common/boot_cpupools.c
index feba93a243fc..af5bea0c1113 100644
--- a/xen/common/boot_cpupools.c
+++ b/xen/common/boot_cpupools.c
@@ -175,8 +175,11 @@ void __init btcpupools_allocate_pools(void)
 if ( add_extra_cpupool )
 next_pool_id++;
 
+/* Keep track of cpupool id 0 with the global cpupool0 */
+cpupool0 = cpupool_create_pool(0, pool_sched_map[0]);
+
 /* Create cpupools with selected schedulers */
-for ( i = 0; i < next_pool_id; i++ )
+for ( i = 1; i < next_pool_id; i++ )
 cpupool_create_pool(i, pool_sched_map[i]);
 
 #ifdef CONFIG_X86
diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index e5189c53a321..f717ee844e91 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -312,10 +312,7 @@ static struct cpupool *cpupool_create(unsigned int poolid,
 c->cpupool_id = q->cpupool_id + 1;
 }
 
-if ( poolid == 0 )
-c->sched = scheduler_get_default();
-else
-c->sched = scheduler_alloc(sched_id);
+c->sched = scheduler_alloc(sched_id);
 if ( IS_ERR(c->sched) )
 {
 ret = PTR_ERR(c->sched);
@@ -1242,9 +1239,6 @@ static int __init cf_check cpupool_init(void)
 
 cpupool_hypfs_init();
 
-cpupool0 = cpupool_create(0, 0);
-BUG_ON(IS_ERR(cpupool0));
-cpupool_put(cpupool0);
 register_cpu_notifier(&cpu_nfb);
 
 btcpupools_dtb_parse();
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 4e749a604f25..215b4f45609a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1188,7 +1188,10 @@ static inline void btcpupools_dtb_parse(void) {}
 #endif
 
 #else /* !CONFIG_BOOT_TIME_CPUPOOLS */
-static inline void btcpupools_allocate_pools(void) {}
+static inline void btcpupools_allocate_pools(void)
+{
+cpupool0 = cpupool_create_pool(0, -1);
+}
 static inline void btcpupools_dtb_parse(void) {}
 static inline unsigned int btcpupools_get_cpupool_id(unsigned int cpu)
 {
-- 
2.17.1

[PATCH v3 2/6] xen/sched: create public function for cpupools creation

[Luca Fancellu]

Create new public function to create cpupools, can take as parameter
the scheduler id or a negative value that means the default Xen
scheduler will be used.

Signed-off-by: Luca Fancellu 
---
Changes in v3:
- Fixed comment (Andrew)
Changes in v2:
- cpupool_create_pool doesn't check anymore for pool id uniqueness
  before calling cpupool_create. Modified commit message accordingly
---
 xen/common/sched/cpupool.c | 15 +++
 xen/include/xen/sched.h| 16 
 2 files changed, 31 insertions(+)

diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
index a6da4970506a..89a891af7076 100644
--- a/xen/common/sched/cpupool.c
+++ b/xen/common/sched/cpupool.c
@@ -1219,6 +1219,21 @@ static void cpupool_hypfs_init(void)
 
 #endif /* CONFIG_HYPFS */
 
+struct cpupool *__init cpupool_create_pool(unsigned int pool_id, int sched_id)
+{
+struct cpupool *pool;
+
+if ( sched_id < 0 )
+sched_id = scheduler_get_default()->sched_id;
+
+pool = cpupool_create(pool_id, sched_id);
+
+BUG_ON(IS_ERR(pool));
+cpupool_put(pool);
+
+return pool;
+}
+
 static int __init cf_check cpupool_init(void)
 {
 unsigned int cpu;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 10ea969c7af9..415b939ba8ae 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1145,6 +1145,22 @@ int cpupool_move_domain(struct domain *d, struct cpupool 
*c);
 int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op);
 unsigned int cpupool_get_id(const struct domain *d);
 const cpumask_t *cpupool_valid_cpus(const struct cpupool *pool);
+
+/*
+ * cpupool_create_pool - Creates a cpupool
+ * @pool_id: id of the pool to be created
+ * @sched_id: id of the scheduler to be used for the pool
+ *
+ * Creates a cpupool with pool_id id.
+ * The sched_id parameter identifies the scheduler to be used, if it is
+ * negative, the default scheduler of Xen will be used.
+ *
+ * returns:
+ * pointer to the struct cpupool just created, or Xen will panic in case of
+ * error
+ */
+struct cpupool *cpupool_create_pool(unsigned int pool_id, int sched_id);
+
 extern void cf_check dump_runq(unsigned char key);
 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi);
-- 
2.17.1

[PATCH v3 3/6] xen/sched: retrieve scheduler id by name

[Luca Fancellu]

Add a static function to retrieve the scheduler pointer using the
scheduler name.

Add a public function to retrieve the scheduler id by the scheduler
name that makes use of the new static function.

Take the occasion to replace open coded scheduler search with the
new static function in scheduler_init.

Signed-off-by: Luca Fancellu 
Reviewed-by: Juergen Gross 
---
Changes in v3:
- add R-by
Changes in v2:
- replace open coded scheduler search in scheduler_init (Juergen)
---
 xen/common/sched/core.c | 40 ++--
 xen/include/xen/sched.h | 11 +++
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 19ab67818106..48ee01420fb8 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -2947,10 +2947,30 @@ void scheduler_enable(void)
 scheduler_active = true;
 }
 
+static inline
+const struct scheduler *__init sched_get_by_name(const char *sched_name)
+{
+unsigned int i;
+
+for ( i = 0; i < NUM_SCHEDULERS; i++ )
+if ( schedulers[i] && !strcmp(schedulers[i]->opt_name, sched_name) )
+return schedulers[i];
+
+return NULL;
+}
+
+int __init sched_get_id_by_name(const char *sched_name)
+{
+const struct scheduler *scheduler = sched_get_by_name(sched_name);
+
+return scheduler ? scheduler->sched_id : -1;
+}
+
 /* Initialise the data structures. */
 void __init scheduler_init(void)
 {
 struct domain *idle_domain;
+const struct scheduler *scheduler;
 int i;
 
 scheduler_enable();
@@ -2981,25 +3001,17 @@ void __init scheduler_init(void)
schedulers[i]->opt_name);
 schedulers[i] = NULL;
 }
-
-if ( schedulers[i] && !ops.name &&
- !strcmp(schedulers[i]->opt_name, opt_sched) )
-ops = *schedulers[i];
 }
 
-if ( !ops.name )
+scheduler = sched_get_by_name(opt_sched);
+if ( !scheduler )
 {
 printk("Could not find scheduler: %s\n", opt_sched);
-for ( i = 0; i < NUM_SCHEDULERS; i++ )
-if ( schedulers[i] &&
- !strcmp(schedulers[i]->opt_name, CONFIG_SCHED_DEFAULT) )
-{
-ops = *schedulers[i];
-break;
-}
-BUG_ON(!ops.name);
-printk("Using '%s' (%s)\n", ops.name, ops.opt_name);
+scheduler = sched_get_by_name(CONFIG_SCHED_DEFAULT);
+BUG_ON(!scheduler);
+printk("Using '%s' (%s)\n", scheduler->name, scheduler->opt_name);
 }
+ops = *scheduler;
 
 if ( cpu_schedule_up(0) )
 BUG();
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 415b939ba8ae..4050e22544f9 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -756,6 +756,17 @@ void sched_destroy_domain(struct domain *d);
 long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
 long sched_adjust_global(struct xen_sysctl_scheduler_op *);
 int  sched_id(void);
+
+/*
+ * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
+ * @sched_name: scheduler name as a string
+ *
+ * returns:
+ * positive value being the scheduler id, on success
+ * negative value if the scheduler name is not found.
+ */
+int sched_get_id_by_name(const char *sched_name);
+
 void vcpu_wake(struct vcpu *v);
 long vcpu_yield(void);
 void vcpu_sleep_nosync(struct vcpu *v);
-- 
2.17.1

[PATCH v3 0/6] Boot time cpupools

[Luca Fancellu]

This serie introduces a feature for Xen to create cpu pools at boot time, the
feature is enabled using a configurable that is disabled by default.
The boot time cpupool feature relies on the device tree to describe the cpu
pools.
Another feature is introduced by the serie, the possibility to assign a
dom0less guest to a cpupool at boot time.

Here follows an example, Xen is built with CONFIG_BOOT_TIME_CPUPOOLS=y.

>From the DT:

  [...]

  a72_0: cpu@0 {
compatible = "arm,cortex-a72";
reg = <0x0 0x0>;
device_type = "cpu";
[...]
  };

  a72_1: cpu@1 {
compatible = "arm,cortex-a72";
reg = <0x0 0x1>;
device_type = "cpu";
[...]
  };

  a53_0: cpu@100 {
compatible = "arm,cortex-a53";
reg = <0x0 0x100>;
device_type = "cpu";
[...]
  };

  a53_1: cpu@101 {
compatible = "arm,cortex-a53";
reg = <0x0 0x101>;
device_type = "cpu";
[...]
  };

  a53_2: cpu@102 {
compatible = "arm,cortex-a53";
reg = <0x0 0x102>;
device_type = "cpu";
[...]
  };

  a53_3: cpu@103 {
compatible = "arm,cortex-a53";
reg = <0x0 0x103>;
device_type = "cpu";
[...]
  };

  chosen {
#size-cells = <0x1>;
#address-cells = <0x1>;
xen,dom0-bootargs = "...";
xen,xen-bootargs = "...";

cpupool0 {
  compatible = "xen,cpupool";
  cpupool-cpus = <&a72_0 &a72_1>;
  cpupool-sched = "credit2";
};

cp1: cpupool1 {
  compatible = "xen,cpupool";
  cpupool-cpus = <&a53_0 &a53_1 &a53_2 &a53_3>;
};

module@0 {
  reg = <0x8008 0x130>;
  compatible = "multiboot,module";
};

domU1 {
  #size-cells = <0x1>;
  #address-cells = <0x1>;
  compatible = "xen,domain";
  cpus = <1>;
  memory = <0 0xC>;
  vpl011;
  domain-cpupool = <&cp1>;

  module@9200 {
compatible = "multiboot,kernel", "multiboot,module";
reg = <0x9200 0x1ff>;
bootargs = "...";
  };
};
  };

  [...]

The example DT is instructing Xen to have two cpu pools, the one with id 0
having two phisical cpus and the one with id 1 having 4 phisical cpu, the
second cpu pool uses the null scheduler and from the /chosen node we can see
that a dom0less guest will be started on that cpu pool.

In this particular case Xen must boot with different type of cpus, so the
boot argument hmp_unsafe must be enabled.


Luca Fancellu (6):
  tools/cpupools: Give a name to unnamed cpupools
  xen/sched: create public function for cpupools creation
  xen/sched: retrieve scheduler id by name
  xen/cpupool: Create different cpupools at boot time
  arm/dom0less: assign dom0less guests to cpupools
  xen/cpupool: Allow cpupool0 to use different scheduler

 docs/misc/arm/device-tree/booting.txt  |   5 +
 docs/misc/arm/device-tree/cpupools.txt | 135 
 tools/helpers/xen-init-dom0.c  |  35 -
 tools/libs/light/libxl_utils.c |   3 +-
 xen/arch/arm/domain_build.c|  14 +-
 xen/arch/arm/include/asm/smp.h |   3 +
 xen/common/Kconfig |   7 +
 xen/common/Makefile|   1 +
 xen/common/boot_cpupools.c | 205 +
 xen/common/domain.c|   2 +-
 xen/common/sched/core.c|  40 +++--
 xen/common/sched/cpupool.c |  32 +++-
 xen/include/public/domctl.h|   4 +-
 xen/include/xen/sched.h|  58 +++
 14 files changed, 516 insertions(+), 28 deletions(-)
 create mode 100644 docs/misc/arm/device-tree/cpupools.txt
 create mode 100644 xen/common/boot_cpupools.c

-- 
2.17.1