| Message ID | 20230728075903.7838-3-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make PDX compression optional | expand |
Hi, On 28/07/2023 08:59, Alejandro Vallejo wrote: > This patch factors out the pdx compression logic hardcoded in both ports > for the maddr<->vaddr conversion functions. > > Touches both x86 and arm ports. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 28.07.2023 09:59, Alejandro Vallejo wrote: > --- a/xen/include/xen/pdx.h > +++ b/xen/include/xen/pdx.h > @@ -160,6 +160,31 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx) > #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn)) > #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx)) > > +/** > + * Computes the offset into the direct map of an maddr > + * > + * @param ma Machine address > + * @return Offset on the direct map where that > + * machine address can be accessed > + */ > +static inline unsigned long maddr_to_directmapoff(uint64_t ma) Was there prior agreement to use uint64_t here and ... > +{ > + return ((ma & ma_top_mask) >> pfn_pdx_hole_shift) | > + (ma & ma_va_bottom_mask); > +} > + > +/** > + * Computes a machine address given a direct map offset > + * > + * @param offset Offset into the direct map > + * @return Corresponding machine address of that virtual location > + */ > +static inline uint64_t directmapoff_to_maddr(unsigned long offset) ... here, not paddr_t? Also you use unsigned long for the offset here, but size_t for maddr_to_directmapoff()'s return value in __maddr_to_virt(). Would be nice if this was consistent within the patch. Especially since the names of the helper functions are longish, I'm afraid I'm not fully convinced of the transformation. But I'm also not meaning to stand in the way, if everyone else wants to move in that direction. Jan
On Mon, Jul 31, 2023 at 05:15:19PM +0200, Jan Beulich wrote: > On 28.07.2023 09:59, Alejandro Vallejo wrote: > > --- a/xen/include/xen/pdx.h > > +++ b/xen/include/xen/pdx.h > > @@ -160,6 +160,31 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx) > > #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn)) > > #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx)) > > > > +/** > > + * Computes the offset into the direct map of an maddr > > + * > > + * @param ma Machine address > > + * @return Offset on the direct map where that > > + * machine address can be accessed > > + */ > > +static inline unsigned long maddr_to_directmapoff(uint64_t ma) > > Was there prior agreement to use uint64_t here and ... > > > +{ > > + return ((ma & ma_top_mask) >> pfn_pdx_hole_shift) | > > + (ma & ma_va_bottom_mask); > > +} > > + > > +/** > > + * Computes a machine address given a direct map offset > > + * > > + * @param offset Offset into the direct map > > + * @return Corresponding machine address of that virtual location > > + */ > > +static inline uint64_t directmapoff_to_maddr(unsigned long offset) > > ... here, not paddr_t? The whole file uses uint64_t rather than paddr_t so I added the new code using the existing convention. I can just as well make it paddr_t, it's not a problem. > > Also you use unsigned long for the offset here, but size_t for > maddr_to_directmapoff()'s return value in __maddr_to_virt(). > Would be nice if this was consistent within the patch. That's fair. I can leave it as "unsigned long" (seeing that a previous nit was preferring that type over size_t). > Especially since the names of the helper functions are longish, > I'm afraid I'm not fully convinced of the transformation. In what sense? If it's just naming style I'm happy to consider other names, but taking compression logic out of non-pdx code is essential to removing compiling it out. > But I'm also not meaning to stand in the way, if everyone else wants to > move in that direction. > > Jan Thanks, Alejandro
On 07.08.2023 18:26, Alejandro Vallejo wrote: > On Mon, Jul 31, 2023 at 05:15:19PM +0200, Jan Beulich wrote: >> Especially since the names of the helper functions are longish, >> I'm afraid I'm not fully convinced of the transformation. > In what sense? If it's just naming style I'm happy to consider other names, > but taking compression logic out of non-pdx code is essential to removing > compiling it out. I understand that, but my dislike for long names of functions doing basic transformations remains. I'm afraid I have no good alternative suggestion, though, and while it's been a long time, this may also have been one reason why I didn't go with helpers back at the time. Plus of course I remain unconvinced that compiling out really is the better option compared to patching out, as was done by my series. Jan
Hi Jan, On 08/08/2023 07:05, Jan Beulich wrote: > On 07.08.2023 18:26, Alejandro Vallejo wrote: >> On Mon, Jul 31, 2023 at 05:15:19PM +0200, Jan Beulich wrote: >>> Especially since the names of the helper functions are longish, >>> I'm afraid I'm not fully convinced of the transformation. >> In what sense? If it's just naming style I'm happy to consider other names, >> but taking compression logic out of non-pdx code is essential to removing >> compiling it out. > > I understand that, but my dislike for long names of functions doing > basic transformations remains. I'm afraid I have no good alternative > suggestion, though, and while it's been a long time, this may also > have been one reason why I didn't go with helpers back at the time. > > Plus of course I remain unconvinced that compiling out really is the > better option compared to patching out, as was done by my series. I am with Alejandro here. The altpatching is more complex and would likely require arch specific code. I don't exactly see the benefits of such approach given that are no known x86 platform where PDX might be useful. Even for Arm, I am not aware of a platform where PDX could be disabled. I agree this could change in the future, but this could be revisit if needed. Cheers,
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 5b530f0f40..c0d7f0f181 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -319,8 +319,7 @@ static inline void *maddr_to_virt(paddr_t ma) (DIRECTMAP_SIZE >> PAGE_SHIFT)); return (void *)(XENHEAP_VIRT_START - (directmap_base_pdx << PAGE_SHIFT) + - ((ma & ma_va_bottom_mask) | - ((ma & ma_top_mask) >> pfn_pdx_hole_shift))); + maddr_to_directmapoff(ma)); } #endif diff --git a/xen/arch/x86/include/asm/x86_64/page.h b/xen/arch/x86/include/asm/x86_64/page.h index 53faa7875b..b589c93e77 100644 --- a/xen/arch/x86/include/asm/x86_64/page.h +++ b/xen/arch/x86/include/asm/x86_64/page.h @@ -36,26 +36,22 @@ static inline unsigned long __virt_to_maddr(unsigned long va) { ASSERT(va < DIRECTMAP_VIRT_END); if ( va >= DIRECTMAP_VIRT_START ) - va -= DIRECTMAP_VIRT_START; - else - { - BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1)); - /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */ - ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) == - ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT))); - - va += xen_phys_start - XEN_VIRT_START; - } - return (va & ma_va_bottom_mask) | - ((va << pfn_pdx_hole_shift) & ma_top_mask); + return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START); + + BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1)); + /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */ + ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) == + ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT))); + + return xen_phys_start + va - XEN_VIRT_START; } static inline void *__maddr_to_virt(unsigned long ma) { - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); - return (void *)(DIRECTMAP_VIRT_START + - ((ma & ma_va_bottom_mask) | - ((ma & ma_top_mask) >> pfn_pdx_hole_shift))); + /* Offset in the direct map, accounting for pdx compression */ + size_t va_offset = maddr_to_directmapoff(ma); + ASSERT(va_offset < DIRECTMAP_SIZE); + return (void *)(DIRECTMAP_VIRT_START + va_offset); } /* read access (should only be used for debug printk's) */ diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h index de5439a5e5..d96f03d6e6 100644 --- a/xen/include/xen/pdx.h +++ b/xen/include/xen/pdx.h @@ -160,6 +160,31 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx) #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn)) #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx)) +/** + * Computes the offset into the direct map of an maddr + * + * @param ma Machine address + * @return Offset on the direct map where that + * machine address can be accessed + */ +static inline unsigned long maddr_to_directmapoff(uint64_t ma) +{ + return ((ma & ma_top_mask) >> pfn_pdx_hole_shift) | + (ma & ma_va_bottom_mask); +} + +/** + * Computes a machine address given a direct map offset + * + * @param offset Offset into the direct map + * @return Corresponding machine address of that virtual location + */ +static inline uint64_t directmapoff_to_maddr(unsigned long offset) +{ + return (((uint64_t)offset << pfn_pdx_hole_shift) & ma_top_mask) | + (offset & ma_va_bottom_mask); +} + /** * Initializes global variables with information about the compressible * range of the current memory regions.
This patch factors out the pdx compression logic hardcoded in both ports for the maddr<->vaddr conversion functions. Touches both x86 and arm ports. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v2: * Cast variable to u64 before shifting left to avoid overflow (Julien) --- xen/arch/arm/include/asm/mm.h | 3 +-- xen/arch/x86/include/asm/x86_64/page.h | 28 +++++++++++--------------- xen/include/xen/pdx.h | 25 +++++++++++++++++++++++ 3 files changed, 38 insertions(+), 18 deletions(-)