[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 1/3] arm/mpu: Introduce MPU memory region map structure


  • To: Ayan Kumar Halder <ayankuma@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • From: "Orzel, Michal" <michal.orzel@xxxxxxx>
  • Date: Fri, 6 Jun 2025 12:13:33 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=ns4AgFo9XDRbp1s0mhDM41O9Qa5PyQc6OVVfa2WSwKI=; b=QLGjoolehJWQwLdmdN/TdVG2yF0ifo8P5ElQwCD+IcKjx6l2vtS3iz5sYonqFR7piFtriNYMvZ9suivJHDHOA29ZdSg0UcOJnTSAaH6C9yQlEtNXIObSSeO9iQmnTpGEpPyjUDIX8HQAdZ2Ah1iXnoIDuDCaUa59ludnB5/+VyQVltHj2cFyIDIIdvtlKnATQvCaLdtX8WuMfo1+c1PmesQM/ZP7U2q1ZUlsKK77gbXS2U4uT4aOvYKIMQuMBwnGxI60NVrgiCbP+btQ6nShfRjV5Jo4/E9p5eyvc7ITMGa8pXUXPazjw0nJaV7XMpgowBAt5JIly1y0wuqvnLdB0Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=NTbPGgJk7iRFbqOEqvMpEVpMcK2QQjIjO4wDn5e6KAXoQjVhsMLx4pNWOMsO1OOSqxninr4QWsgQ0ijdmOIxHNrZQDl+KkJP9PH2qyFZu9bYhsqgMMMbXA2Fw7vZZO6DfzwRDj+taGASHuPwmfLsxMTeirOswxyCLfNao3ilHqIG6/CtcMuKZV6gQF4SDOGJ04+LU5Xg1tUMnjMKK4xsISXYXP+5kmVqf6/CgTTqWAfT/VIYgH6k8w6IRl+9syLAol7mM3rvu+6ArkByFNSh4Iok178p0NEUm288R6S2C57/OTpj9K6FAAHvfp1gPaDa42VBPO3tCdDnW4xKnJs1IA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Fri, 06 Jun 2025 10:14:09 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>


On 05/06/2025 15:39, Ayan Kumar Halder wrote:
> Hi Michal,
> 
> On 05/06/2025 08:06, Orzel, Michal wrote:
>>
>> On 04/06/2025 19:43, Ayan Kumar Halder wrote:
>>> Introduce pr_t typedef which is a structure having the prbar and prlar 
>>> members,
>>> each being structured as the registers of the AArch32 Armv8-R architecture.
>>>
>>> Also, define MPU_REGION_RES0 to 0 as there are no reserved 0 bits beyond the
>>> BASE or LIMIT bitfields in prbar or prlar respectively.
>>>
>>> Move pr_t definition to common code.
>>> Also, enclose xn_0 within ARM64 as it is not present for ARM32.
>>>
>>> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
>>> ---
>>>   xen/arch/arm/include/asm/arm32/mpu.h | 30 +++++++++++++++++++++++-----
>>>   xen/arch/arm/include/asm/arm64/mpu.h |  6 ------
>>>   xen/arch/arm/include/asm/mpu.h       |  6 ++++++
>>>   xen/arch/arm/mpu/mm.c                |  2 ++
>>>   4 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
>>> b/xen/arch/arm/include/asm/arm32/mpu.h
>>> index f0d4d4055c..ae3b661fde 100644
>>> --- a/xen/arch/arm/include/asm/arm32/mpu.h
>>> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
>>> @@ -5,11 +5,31 @@
>>>   
>>>   #ifndef __ASSEMBLY__
>>>   
>>> -/* MPU Protection Region */
>>> -typedef struct {
>>> -    uint32_t prbar;
>>> -    uint32_t prlar;
>>> -} pr_t;
>>> +#define MPU_REGION_RES0       0x0
>> The name of the macro does not make a lot of sense in AArch32 context
>> and can create a confusion for the reader.
> 
> I know, but I want to avoid introducing ifdef or have separate 
> implementation (for arm32 and arm64) for the following
> 
> Refer xen/arch/arm/include/asm/mpu.h
> 
> static inline void pr_set_base(pr_t *pr, paddr_t base)
> {
>      pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
> }
> 
> Let me know your preference.
I did not mean #ifdef-ing. I was more like suggesting to use a different macro
name that would be more meaningful than this one.

> 
>>
>>> +
>>> +/* Hypervisor Protection Region Base Address Register */
>>> +typedef union {
>>> +    struct {
>>> +        unsigned int xn:1;       /* Execute-Never */
>>> +        unsigned int ap_0:1;     /* Acess Permission */
>> If you write AP[1] below, I would expect here AP[0]
> 
> Again same reason as before, let me know if you want to have additional 
> ifdef in pr_of_addr() or separate functions for arm32 and arm64
I don't understand. My comment was only about changing comment to say /* Access
Permission AP[0] */ because below you have a comment with AP[1].

~Michal




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.