virt_to_maddr and maddr_to_virt are used widely in Xen code. So
even there is no VMSA in MPU system, we keep the interface in MPU to
stay the same code flow.

The MPU version of virt/maddr conversion is simple, and we just return
the input address as the output with type conversion.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
---
v3:
- Fix typos
- Move the implementation from mm/mpu.h to mm.h, to share as much as
possible with MMU system.
---
 xen/arch/arm/include/asm/mm.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index eb520b49e3..ea4847c12b 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -267,13 +267,22 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 /* Page-align address and convert to frame number format */
 #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
 
+#ifndef CONFIG_HAS_MPU
 static inline paddr_t __virt_to_maddr(vaddr_t va)
 {
     uint64_t par = va_to_par(va);
     return (par & PADDR_MASK & PAGE_MASK) | (va & ~PAGE_MASK);
 }
+#else
+static inline paddr_t __virt_to_maddr(vaddr_t va)
+{
+    return (paddr_t)va;
+}
+#endif /* CONFIG_HAS_MPU */
+
 #define virt_to_maddr(va)   __virt_to_maddr((vaddr_t)(va))
 
+#ifndef CONFIG_HAS_MPU
 #ifdef CONFIG_ARM_32
 static inline void *maddr_to_virt(paddr_t ma)
 {
@@ -292,6 +301,12 @@ static inline void *maddr_to_virt(paddr_t ma)
                      ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
 }
 #endif
+#else /* CONFIG_HAS_MPU */
+static inline void *maddr_to_virt(paddr_t ma)
+{
+    return (void *)(unsigned long)ma;
+}
+#endif
 
 /*
  * Translate a guest virtual address to a machine address.
-- 
2.25.1

On 26/06/2023 04:34, Penny Zheng wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> virt_to_maddr and maddr_to_virt are used widely in Xen code. So
> even there is no VMSA in MPU system, we keep the interface in MPU to
> stay the same code flow.
>
> The MPU version of virt/maddr conversion is simple, and we just return
> the input address as the output with type conversion.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
> v3:
> - Fix typos
> - Move the implementation from mm/mpu.h to mm.h, to share as much as
> possible with MMU system.
> ---
>   xen/arch/arm/include/asm/mm.h | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index eb520b49e3..ea4847c12b 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -267,13 +267,22 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
>   /* Page-align address and convert to frame number format */
>   #define paddr_to_pfn_aligned(paddr)    paddr_to_pfn(PAGE_ALIGN(paddr))
>
> +#ifndef CONFIG_HAS_MPU
>   static inline paddr_t __virt_to_maddr(vaddr_t va)
>   {
>       uint64_t par = va_to_par(va);
>       return (par & PADDR_MASK & PAGE_MASK) | (va & ~PAGE_MASK);
>   }
> +#else
> +static inline paddr_t __virt_to_maddr(vaddr_t va)
> +{
> +    return (paddr_t)va;
> +}
> +#endif /* CONFIG_HAS_MPU */
> +
>   #define virt_to_maddr(va)   __virt_to_maddr((vaddr_t)(va))
>
> +#ifndef CONFIG_HAS_MPU
>   #ifdef CONFIG_ARM_32
>   static inline void *maddr_to_virt(paddr_t ma)
>   {
> @@ -292,6 +301,12 @@ static inline void *maddr_to_virt(paddr_t ma)
>                        ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
>   }
>   #endif
> +#else /* CONFIG_HAS_MPU */
> +static inline void *maddr_to_virt(paddr_t ma)
> +{
> +    return (void *)(unsigned long)ma;

Why do you need "(unsigned long)ma" ? Both "unsigned long" and "paddr_t" 
are u64.

- Ayan

> +}
> +#endif
>
>   /*
>    * Translate a guest virtual address to a machine address.
> --
> 2.25.1
>
>

On 29/06/2023 3:20 pm, Ayan Kumar Halder wrote:
> On 26/06/2023 04:34, Penny Zheng wrote:
>> diff --git a/xen/arch/arm/include/asm/mm.h
>> b/xen/arch/arm/include/asm/mm.h
>> index eb520b49e3..ea4847c12b 100644
>> --- a/xen/arch/arm/include/asm/mm.h
>> +++ b/xen/arch/arm/include/asm/mm.h
>> @@ -292,6 +301,12 @@ static inline void *maddr_to_virt(paddr_t ma)
>>                        ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
>>   }
>>   #endif
>> +#else /* CONFIG_HAS_MPU */
>> +static inline void *maddr_to_virt(paddr_t ma)
>> +{
>> +    return (void *)(unsigned long)ma;
>
> Why do you need "(unsigned long)ma" ? Both "unsigned long" and
> "paddr_t" are u64.

For when paddr_t really isn't u64.

The logic is correct, but it is an opencoding of the _p() macro for
turning an arbitrary integer into a pointer.

~Andrew

On 29/06/2023 15:23, Andrew Cooper wrote:
> On 29/06/2023 3:20 pm, Ayan Kumar Halder wrote:
>> On 26/06/2023 04:34, Penny Zheng wrote:
>>> diff --git a/xen/arch/arm/include/asm/mm.h
>>> b/xen/arch/arm/include/asm/mm.h
>>> index eb520b49e3..ea4847c12b 100644
>>> --- a/xen/arch/arm/include/asm/mm.h
>>> +++ b/xen/arch/arm/include/asm/mm.h
>>> @@ -292,6 +301,12 @@ static inline void *maddr_to_virt(paddr_t ma)
>>>                         ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
>>>    }
>>>    #endif
>>> +#else /* CONFIG_HAS_MPU */
>>> +static inline void *maddr_to_virt(paddr_t ma)
>>> +{
>>> +    return (void *)(unsigned long)ma;
>> Why do you need "(unsigned long)ma" ? Both "unsigned long" and
>> "paddr_t" are u64.
> For when paddr_t really isn't u64.

Sorry, I am missing something

From 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/include/asm/types.h;h=fb6618ef247fe8e3abe472e50b4877e11cc8a96c;hb=refs/heads/staging

In CONFIG_ARM_64

typedef unsigned long u64;

typedef u64 paddr_t;

So, why do we need to typecast "paddr_t" to "unsigned long" as they are 
the same ?

- Ayan

>
> The logic is correct, but it is an opencoding of the _p() macro for
> turning an arbitrary integer into a pointer.
>
> ~Andrew

Hi,

On 29/06/2023 15:44, Ayan Kumar Halder wrote:
> 
> On 29/06/2023 15:23, Andrew Cooper wrote:
>> On 29/06/2023 3:20 pm, Ayan Kumar Halder wrote:
>>> On 26/06/2023 04:34, Penny Zheng wrote:
>>>> diff --git a/xen/arch/arm/include/asm/mm.h
>>>> b/xen/arch/arm/include/asm/mm.h
>>>> index eb520b49e3..ea4847c12b 100644
>>>> --- a/xen/arch/arm/include/asm/mm.h
>>>> +++ b/xen/arch/arm/include/asm/mm.h
>>>> @@ -292,6 +301,12 @@ static inline void *maddr_to_virt(paddr_t ma)
>>>>                         ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
>>>>    }
>>>>    #endif
>>>> +#else /* CONFIG_HAS_MPU */
>>>> +static inline void *maddr_to_virt(paddr_t ma)
>>>> +{
>>>> +    return (void *)(unsigned long)ma;
>>> Why do you need "(unsigned long)ma" ? Both "unsigned long" and
>>> "paddr_t" are u64.
>> For when paddr_t really isn't u64.
> 
> Sorry, I am missing something
> 
>  From 
> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/include/asm/types.h;h=fb6618ef247fe8e3abe472e50b4877e11cc8a96c;hb=refs/heads/staging
> 
> In CONFIG_ARM_64
> 
> typedef unsigned long u64;
> 
> typedef u64 paddr_t;
> 
> So, why do we need to typecast "paddr_t" to "unsigned long" as they are 
> the same ?
We may decide to restrict paddr_t to 32-bit in the future on Arm64.

Also, when CONFIG_PHYS_ADDR_T_32=n, paddr_t is 64-bit on 32-bit Xen. So 
casting directly to (void *) is not possible. Although, this is not a 
problem here because for the MPU on 32-bit, you would select 
CONFIG_PHYS_ADDR_T_32.

On a side note, I agree with Andrew that we should switch to _p().

Cheers,

-- 
Julien Grall

Hi,

On 2023/6/29 23:14, Julien Grall wrote:
> Hi,
> 
> On 29/06/2023 15:44, Ayan Kumar Halder wrote:
>>
>> On 29/06/2023 15:23, Andrew Cooper wrote:
>>> On 29/06/2023 3:20 pm, Ayan Kumar Halder wrote:
>>>> On 26/06/2023 04:34, Penny Zheng wrote:
>>>>> diff --git a/xen/arch/arm/include/asm/mm.h
>>>>> b/xen/arch/arm/include/asm/mm.h
>>>>> index eb520b49e3..ea4847c12b 100644
>>>>> --- a/xen/arch/arm/include/asm/mm.h
>>>>> +++ b/xen/arch/arm/include/asm/mm.h
>>>>> @@ -292,6 +301,12 @@ static inline void *maddr_to_virt(paddr_t ma)
>>>>>                         ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
>>>>>    }
>>>>>    #endif
>>>>> +#else /* CONFIG_HAS_MPU */
>>>>> +static inline void *maddr_to_virt(paddr_t ma)
>>>>> +{
>>>>> +    return (void *)(unsigned long)ma;
>>>> Why do you need "(unsigned long)ma" ? Both "unsigned long" and
>>>> "paddr_t" are u64.
>>> For when paddr_t really isn't u64.
>>
>> Sorry, I am missing something
>>
>>  From 
>> https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/include/asm/types.h;h=fb6618ef247fe8e3abe472e50b4877e11cc8a96c;hb=refs/heads/staging
>>
>> In CONFIG_ARM_64
>>
>> typedef unsigned long u64;
>>
>> typedef u64 paddr_t;
>>
>> So, why do we need to typecast "paddr_t" to "unsigned long" as they 
>> are the same ?
> We may decide to restrict paddr_t to 32-bit in the future on Arm64.
> 
> Also, when CONFIG_PHYS_ADDR_T_32=n, paddr_t is 64-bit on 32-bit Xen. So 
> casting directly to (void *) is not possible. Although, this is not a 
> problem here because for the MPU on 32-bit, you would select 
> CONFIG_PHYS_ADDR_T_32.
> 
> On a side note, I agree with Andrew that we should switch to _p().

Sure. I'll switch to _p().

> 
> Cheers,
>