| Message ID | 20230626033443.2943270-29-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
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,
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, >
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.