RE: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions

[Mark Brown <mark.brown@xxxxxxxxxxxxx>]

Hello all,
This is my first post to xen-devel. Since I'm new, let me give a brief intro 
about "Why?"

1. I am interested in easing the ongoing burden of repeatedly obtaining safety
certification for Xen for eventual use in high assurance operational *systems*.
Call this an evergreen certification effort.

2. When I read the ISO26262 and avionics cert standards, I note that there is a 
system
architecture (it has its own "System V" lifecycle) that encompasses a set-of-n
hardware items (e.g. fabricated SoC analysis and also FPGA soft IP development) 
and
also a set-of-m software items (e.g., bootloader, hypervisor, guest OSes, apps, 
containers,
etc.). For high assurance, we must evaluate the entire system and not just some 
of its items.

3. Also from the standards the govern the lifecycle and processes for developing
high assurance systems, hardware, and software, maintaining (automated) 
bidirectional
requirements identifier traceability from safety requirements "down" (i.e., 
through
levels of requirements decomposition, into *-architecture level designs, into 
software
unit designs, into software unit code, and into software verification tests at 
the unit,
library, module / architecture, and system level. From my perspective, there's 
a lot
riding on these requirements IDs. I would guess that many in this community
would feel concerned about dumping many (hundreds?) of requirements IDs
directly into the Xen code base, even if these were located within comments.

4. Because of 1, 2, and 3 (above), when I look to understand Xen requirements 
sources,
my first pass is to remember that Xen is just one software item, and it will 
need to accept
integration requirements flowing in from the system (i.e., via KConfig settings 
and Device
Tree / SDT language) and also from the core types that it relies upon (i.e., 
scoped down
to the operationally-deployed core types including Arm Cortex-R52 during boot,
but extending to co-processors it uses and then to IO devices that are used, 
etc.)
Finally, sometimes other software items flow requirements into Xen, e.g., when
Linux  paravirtualization or virtio implementations are scoped-in.

In summary, finding an agreeable way to embed requirements IDs into the Xen
source code seems essential to me, so that we can achieve evergreen high
assurance certifications for Xen.

Proposal 1: Rather than start by adding many requirements IDs into comments,
we might instead start out by using a less-impactful approach.
I propose that we could hand-pick some C data types (CDTs) that were known to be
critically important to Xen's high assurance, and simply (re)name these CDTs,
for example, structs and unions.

This proposal is to adopt a naming convention for handpicked safety-relevant
CDTs, starting with the first two identified below.

For example in this patch, e.g. in xen/arch/arm/include/asm/arm32/mpu.h,
two CDT structs convey the bitfield requirements from the new MPU co-processors.
Specifically, in the typedefs "prbar_t" and "prlar_t" no struct tag (name) is 
used
for either of the bitfield struct. I propose adding struct tag names to both, 
and
then leveraging these struct tag names as requirements IDs.

Suggested Naming Convention:
 (a) E.g., For prbar_t, tag its bitfield struct with the name "PRBAR_BF"
 (b) E.g., For prlar_t, tag its bitfied struct with the name "PRLAR_BF"
 (c) Proposed bitfield struct naming convention: <register>_BF
 (d) Going forward, for bitfield structs that flow-in safety-relevant
  requirements from the core's Technical Reference Manual (TRM)
  and registers, to use the suffix "_BF" to identify these structs. And
  to faithfully reuse the TRM's name for the register.  (application to structs)
(e) Going forward, for unions that wrap "_BF" structs, to name any
  union member name corresponding to the "_BF" as "bf" and any
  union member name corresponding to the whole as "whole".
  (application to unions that immediately wrap "_BF" structs)

The benefits of systematically following these naming conventions will be:
1) Even simple tools like grep -R can find all "_BF" requirements IDs that flow 
from
    hardware registers that have been handpicked due to their safety-relevance.
2) Any CDT-parsing tool such as Doxygen, etc., can automatically find and trace
   all usages of "_BF" structs at several levels
   (a) Dependent structs such as "pr_t" can be automatically traced "up" to
   their "_BF" ancestor CDTs.
   (b) C function argument CDTs can be automatically traced up to any
   ancestor "_BF" CDTs, with the implication that the function may be 
safety-relevant
   because it accesses one or more "_BF" CDTs.
   (c) Simple pointer-to-CDT usages should not obscure any facts that
   either the ".bf.*" (accessing a safety-relevant bitfield by name) or the
  ".whole" (accessing the entire register contents by value) were accessed.
3) This convention will naturally allow the new "_BF" requirements IDs
to flow into all future use / access of these safety-relevant registers, 
including
future verification tests.
4) Also, it will naturally allow the new "<register>_BF" requirements IDs
to be traced "up" into the TRM and automatically de-reference the semantics
and usage and safety-relevant notes about the bitfield, the register, and the
larger context of use for that register (e.g., at which instruction will the 
PMU's
enforcement of "HPRBAR10" / "p15,4,c6,c13,0" be activated?)
5) The xen-devel community may find that using this naming convention
provides an easier on-ramp to "start seeing safety and high assurance."
This is because engineers who read any future C source code,
or review any future patch series will see that specific lines in that C source 
code
are indelibly tattooed when they refer to a safety-relevant register. Whether 
any
line of code access the "_BF" register in part or in whole, that will become 
explicitly visible.
The goal is to raise community awareness by flagging access / use.

Thank you!

Sincerely,
--mark

-----Original Message-----
From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Julien 
Grall
Sent: Thursday, April 17, 2025 11:55 PM
To: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>; 
xen-devel@xxxxxxxxxxxxxxxxxxxx
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis 
<bertrand.marquis@xxxxxxx>; Michal Orzel <michal.orzel@xxxxxxx>; Volodymyr 
Babchuk <Volodymyr_Babchuk@xxxxxxxx>
Subject: Re: [PATCH v1] xen/arm: AArch32-V8R: Add MPU register definitions

[You don't often get email from julien@xxxxxxx. Learn why this is important at 
https://aka.ms/LearnAboutSenderIdentification ]

CAUTION: This email originated from outside of the organization. Do not click 
links or open attachments unless you recognize the sender and know the content 
is safe.


Hi Ayan,

On 18/04/2025 00:55, Ayan Kumar Halder wrote:
> Add the definitions for HPRBAR<0..31>, HPRLAR<0..31> and HPRENR.
> The definitions are taken from ARM DDI 0568A.c ID110520, E2.2.3
> HPRBAR<n>,
> E2.2.4 HPRENR and E2.2.6 HPRLAR<n>.
>
> Introduce pr_t typedef which is a structure having the prbar and prlar
> members, each being structured as the registers of the AArch32-V8R 
> architecture.
> This is the arm32 equivalent of
> "arm/mpu: Introduce MPU memory region map structure".
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> This patch should be applied after
> "[PATCH v3 0/7] First chunk for Arm R82 and MPU support" in order to
> enable compilation for AArch32.
>
>   xen/arch/arm/include/asm/arm32/mpu.h  |  59 +++++++++++
>   xen/arch/arm/include/asm/mpu.h        |   4 +
>   xen/arch/arm/include/asm/mpu/cpregs.h | 135 ++++++++++++++++++++++++++
>   3 files changed, 198 insertions(+)
>   create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h
> b/xen/arch/arm/include/asm/arm32/mpu.h
> new file mode 100644
> index 0000000000..4aabd93479
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * mpu.h: Arm Memory Protection Unit definitions for aarch64.
> + */
> +
> +#ifndef __ARM_ARM32_MPU_H
> +#define __ARM_ARM32_MPU_H
> +
> +#define XN_EL2_ENABLED  0x1

I understand the define is introduced in Luca's patch, but this a bit 
non-descriptive... Can we find a better name? Maybe by adding the name of the 
register and some documentation?

> +
> +#ifndef __ASSEMBLY__
> +
> +/* Hypervisor Protection Region Base Address Register */ typedef
> +union {
> +    struct {
> +        unsigned int xn:1;       /* Execute-Never */
> +        unsigned int ap:2;       /* Acess Permission */
> +        unsigned int sh:2;       /* Sharebility */
> +        unsigned int res0:1;     /* Reserved as 0 */
> +        unsigned int base:26;    /* Base Address */
> +    } reg;
> +    uint32_t bits;
> +} prbar_t;
> +
> +/* Hypervisor Protection Region Limit Address Register */ typedef
> +union {
> +    struct {
> +        unsigned int en:1;     /* Region enable */
> +        unsigned int ai:3;     /* Memory Attribute Index */
> +        /*
> +         * There is no actual ns bit in hardware. It is used here for
> +         * compatibility with Armr64 code. Thus, we are reusing a res0 bit 
> for ns.

typo: Arm64.

> +         */

Hmmmm, this would mean someone may mistakenly set the bit to 0 by mistake. If 
the field is always meant to be 0 on arm64, then I would consider to name is 
res0 on arm64 with an explanation.

This would make clear the bit is not supposed to have a value other than 0.

> +        unsigned int ns:1;     /* Reserved 0 by hardware */
> +        unsigned int res0:1;   /* Reserved 0 by hardware */
> +        unsigned int limit:26;  /* Limit Address */

NIT: Can we align the comments?

> +    } reg;
> +    uint32_t bits;
> +} prlar_t;
> +
> +/* Protection Region */
> +typedef struct {
> +    prbar_t prbar;
> +    prlar_t prlar;
> +    uint64_t p2m_type;          /* Used to store p2m types. */
> +} pr_t;

This looks to be common with arm64. If so, I would prefer if the structure is 
in a common header.

> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* __ARM_ARM32_MPU_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/mpu.h
> b/xen/arch/arm/include/asm/mpu.h index 77d0566f97..67127149c0 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -8,8 +8,12 @@
>
>   #if defined(CONFIG_ARM_64)
>   # include <asm/arm64/mpu.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/mpu.h>
>   #endif
>
> +#define PRENR_MASK  GENMASK(31, 0)
> +
>   #define MPU_REGION_SHIFT  6
>   #define MPU_REGION_ALIGN  (_AC(1, UL) << MPU_REGION_SHIFT)
>   #define MPU_REGION_MASK   (~(MPU_REGION_ALIGN - 1))
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h
> b/xen/arch/arm/include/asm/mpu/cpregs.h
> index d5cd0e04d5..7cf52aa09a 100644
> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -6,18 +6,153 @@
>   /* CP15 CR0: MPU Type Register */
>   #define HMPUIR          p15,4,c0,c0,4
>
> +/* CP15 CR6: Protection Region Enable Register */
> +#define HPRENR          p15,4,c6,c1,1
> +
>   /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
>   #define HPRSELR         p15,4,c6,c2,1
>   #define HPRBAR          p15,4,c6,c3,0
>   #define HPRLAR          p15,4,c6,c8,1
>
> +/* CP15 CR6: MPU Protection Region Base/Limit Address Register */
> +#define HPRBAR0         p15,4,c6,c8,0
> +#define HPRLAR0         p15,4,c6,c8,1
> +#define HPRBAR1         p15,4,c6,c8,4
> +#define HPRLAR1         p15,4,c6,c8,5
> +#define HPRBAR2         p15,4,c6,c9,0
> +#define HPRLAR2         p15,4,c6,c9,1
> +#define HPRBAR3         p15,4,c6,c9,4
> +#define HPRLAR3         p15,4,c6,c9,5
> +#define HPRBAR4         p15,4,c6,c10,0
> +#define HPRLAR4         p15,4,c6,c10,1
> +#define HPRBAR5         p15,4,c6,c10,4
> +#define HPRLAR5         p15,4,c6,c10,5
> +#define HPRBAR6         p15,4,c6,c11,0
> +#define HPRLAR6         p15,4,c6,c11,1
> +#define HPRBAR7         p15,4,c6,c11,4
> +#define HPRLAR7         p15,4,c6,c11,5
> +#define HPRBAR8         p15,4,c6,c12,0
> +#define HPRLAR8         p15,4,c6,c12,1
> +#define HPRBAR9         p15,4,c6,c12,4
> +#define HPRLAR9         p15,4,c6,c12,5
> +#define HPRBAR10        p15,4,c6,c13,0
> +#define HPRLAR10        p15,4,c6,c13,1
> +#define HPRBAR11        p15,4,c6,c13,4
> +#define HPRLAR11        p15,4,c6,c13,5
> +#define HPRBAR12        p15,4,c6,c14,0
> +#define HPRLAR12        p15,4,c6,c14,1
> +#define HPRBAR13        p15,4,c6,c14,4
> +#define HPRLAR13        p15,4,c6,c14,5
> +#define HPRBAR14        p15,4,c6,c15,0
> +#define HPRLAR14        p15,4,c6,c15,1
> +#define HPRBAR15        p15,4,c6,c15,4
> +#define HPRLAR15        p15,4,c6,c15,5
> +#define HPRBAR16        p15,5,c6,c8,0
> +#define HPRLAR16        p15,5,c6,c8,1
> +#define HPRBAR17        p15,5,c6,c8,4
> +#define HPRLAR17        p15,5,c6,c8,5
> +#define HPRBAR18        p15,5,c6,c9,0
> +#define HPRLAR18        p15,5,c6,c9,1
> +#define HPRBAR19        p15,5,c6,c9,4
> +#define HPRLAR19        p15,5,c6,c9,5
> +#define HPRBAR20        p15,5,c6,c10,0
> +#define HPRLAR20        p15,5,c6,c10,1
> +#define HPRBAR21        p15,5,c6,c10,4
> +#define HPRLAR21        p15,5,c6,c10,5
> +#define HPRBAR22        p15,5,c6,c11,0
> +#define HPRLAR22        p15,5,c6,c11,1
> +#define HPRBAR23        p15,5,c6,c11,4
> +#define HPRLAR23        p15,5,c6,c11,5
> +#define HPRBAR24        p15,5,c6,c12,0
> +#define HPRLAR24        p15,5,c6,c12,1
> +#define HPRBAR25        p15,5,c6,c12,4
> +#define HPRLAR25        p15,5,c6,c12,5
> +#define HPRBAR26        p15,5,c6,c13,0
> +#define HPRLAR26        p15,5,c6,c13,1
> +#define HPRBAR27        p15,5,c6,c13,4
> +#define HPRLAR27        p15,5,c6,c13,5
> +#define HPRBAR28        p15,5,c6,c14,0
> +#define HPRLAR28        p15,5,c6,c14,1
> +#define HPRBAR29        p15,5,c6,c14,4
> +#define HPRLAR29        p15,5,c6,c14,5
> +#define HPRBAR30        p15,5,c6,c15,0
> +#define HPRLAR30        p15,5,c6,c15,1
> +#define HPRBAR31        p15,5,c6,c15,4
> +#define HPRLAR31        p15,5,c6,c15,5

NIT: Is there any way we could generate the values using macros?

> +
>   /* Aliases of AArch64 names for use in common code */
>   #ifdef CONFIG_ARM_32
>   /* Alphabetically... */
>   #define MPUIR_EL2       HMPUIR
>   #define PRBAR_EL2       HPRBAR
> +#define PRBAR0_EL2      HPRBAR0

AFAIU, the alias will be mainly used in the macro generate the switch. Rather 
than open-coding all the PR*AR_EL2, can we provide two macros PR{B, L}AR_N that 
will be implemented as HPR{B,L}AR##n for arm32 and PR{B,L}AR##n for arm64?

>   #define PRSELR_EL2      HPRSELR
> +
>   #endif /* CONFIG_ARM_32 */
>
>   #endif /* __ARM_MPU_CPREGS_H */

Cheers,

--
Julien Grall


________________________________
The information contained in this e-mail and any attachments from Parry Labs 
may contain confidential and/or proprietary information, and is intended only 
for the named recipient to whom it was originally addressed. If you are not the 
intended recipient, any disclosure, distribution, or copying of this e-mail or 
its attachments is strictly prohibited.   If you have received this e-mail in 
error, please notify the sender immediately by return e-mail and permanently 
delete the e-mail and any attachments.