On 13/05/2025 10:45, Luca Fancellu wrote:
> Provide a function that creates a pr_t object from a memory
> range and some attributes.
>
> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
> ---
> v5 changes:
> - removed AP_RW_EL2 used only by pr_of_xenaddr(), fixed
> comments and typos
> - Given some comments to the page.h flags and modifications
> to the prbar_t fields ap, xn, the constructor now takes
> flags instead of attr_idx, which I believe it's better,
> deleted PRBAR_EL2_XN_ENABLED since now the PAGE_XN_MASK()
> is used instead.
> - renamed to pr_of_addr since it will be used also in p2m
> v4 changes:
> - update helper comments
> - rename XN_EL2_ENABLED to PRBAR_EL2_XN_ENABLED
> - protected pr_of_xenaddr() with #ifdef Arm64 until Arm32
> can build with it
> ---
> xen/arch/arm/include/asm/mpu/mm.h | 10 +++++
> xen/arch/arm/mpu/mm.c | 66 +++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h
> b/xen/arch/arm/include/asm/mpu/mm.h
> index 2ee908801665..f0facee51692 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -75,6 +75,16 @@ void read_protection_region(pr_t *pr_read, uint8_t sel);
> */
> void write_protection_region(const pr_t *pr_write, uint8_t sel);
>
> +/*
> + * Creates a pr_t structure describing a protection region.
> + *
> + * @base: base address as base of the protection region.
> + * @limit: exclusive address as limit of the protection region.
> + * @flags: memory flags for the mapping.
> + * @return: pr_t structure describing a protection region.
> + */
> +pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
> +
> #endif /* __ARM_MPU_MM_H__ */
>
> /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 46883cbd4af9..ac83227e607e 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -9,6 +9,7 @@
> #include <xen/types.h>
> #include <asm/mpu.h>
> #include <asm/mpu/mm.h>
> +#include <asm/page.h>
> #include <asm/sysregs.h>
>
> struct page_info *frame_table;
> @@ -153,6 +154,71 @@ void write_protection_region(const pr_t *pr_write,
> uint8_t sel)
> BUG(); /* Can't happen */
> }
> }
> +
> +pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
> +{
> + unsigned int attr_idx = PAGE_AI_MASK(flags);
> + prbar_t prbar;
> + prlar_t prlar;
> + pr_t region;
> +
> + /* Build up value for PRBAR_EL2. */
> + prbar = (prbar_t) {
> + .reg = {
> + .ro = PAGE_RO_MASK(flags),
> + .xn = PAGE_XN_MASK(flags),
Shouldn't you also initialize .xn_0 and .ap_0 or you rely on compound literal
implicit zero initialization of unspecified fields? But then you do initialize
.ns to 0 fror prlar...
> + }};
> +
> + switch ( attr_idx )
> + {
> + /*
> + * ARM ARM: Shareable, Inner Shareable, and Outer Shareable Normal memory
> + * (DDI 0487L.a B2.10.1.1.1 Note section):
> + *
> + * Because all data accesses to Non-cacheable locations are data coherent
> + * to all observers, Non-cacheable locations are always treated as Outer
> + * Shareable
> + *
> + * ARM ARM: Device memory (DDI 0487L.a B2.10.2)
> + *
> + * All of these memory types have the following properties:
> + * [...]
> + * - Data accesses to memory locations are coherent for all observers in
> + * the system, and correspondingly are treated as being Outer
> Shareable
> + */
> + case MT_NORMAL_NC:
> + /* Fall through */
> + case MT_DEVICE_nGnRnE:
> + /* Fall through */
> + case MT_DEVICE_nGnRE:
> + prbar.reg.sh = LPAE_SH_OUTER;
> + break;
> + default:
> + /* Xen mappings are SMP coherent */
> + prbar.reg.sh = LPAE_SH_INNER;
> + break;
> + }
> +
> + /* Build up value for PRLAR_EL2. */
> + prlar = (prlar_t) {
> + .reg = {
> + .ns = 0, /* Hyp mode is in secure world */
> + .ai = attr_idx,
> + .en = 1, /* Region enabled */
> + }};
> +
> + /* Build up MPU memory region. */
> + region = (pr_t) {
> + .prbar = prbar,
> + .prlar = prlar,
> + };
> +
> + /* Set base address and limit address. */
> + pr_set_base(®ion, base);
> + pr_set_limit(®ion, limit);
> +
> + return region;
> +}
> #endif
>
> void __init setup_mm(void)
Other than that:
Reviewed-by: Michal Orzel <michal.or...@amd.com>
~Michal
