| Message ID | ec04dca87144194f5b3ecb9a78d142d370c9193f.1729068334.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Setup memory management | expand |
On Wed Oct 16, 2024 at 10:15 AM BST, Oleksii Kurochko wrote: > Implement the `maddr_to_virt()` function to convert a machine address > to a virtual address. This function is specifically designed to be used > only for the DIRECTMAP region, so a check has been added to ensure that > the address does not exceed `DIRECTMAP_SIZE`. > nit: Worth mentioning this comes from the x86 side of things. > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > xen/arch/riscv/include/asm/mm.h | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h > index ebb142502e..0396e66f47 100644 > --- a/xen/arch/riscv/include/asm/mm.h > +++ b/xen/arch/riscv/include/asm/mm.h > @@ -25,8 +25,12 @@ > > static inline void *maddr_to_virt(paddr_t ma) > { > - BUG_ON("unimplemented"); > - return NULL; > + /* Offset in the direct map, accounting for pdx compression */ > + unsigned long va_offset = maddr_to_directmapoff(ma); > + > + ASSERT(va_offset < DIRECTMAP_SIZE); > + > + return (void *)(DIRECTMAP_VIRT_START + va_offset); > } > > /*
On 16.10.2024 11:15, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/mm.h > +++ b/xen/arch/riscv/include/asm/mm.h > @@ -25,8 +25,12 @@ > > static inline void *maddr_to_virt(paddr_t ma) > { > - BUG_ON("unimplemented"); > - return NULL; > + /* Offset in the direct map, accounting for pdx compression */ > + unsigned long va_offset = maddr_to_directmapoff(ma); Why the mentioning of PDX compression? At least right now it's unavailable for RISC-V afaics. Are there plans to change that any time soon? Jan
On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote: > On 16.10.2024 11:15, Oleksii Kurochko wrote: > > --- a/xen/arch/riscv/include/asm/mm.h > > +++ b/xen/arch/riscv/include/asm/mm.h > > @@ -25,8 +25,12 @@ > > > > static inline void *maddr_to_virt(paddr_t ma) > > { > > - BUG_ON("unimplemented"); > > - return NULL; > > + /* Offset in the direct map, accounting for pdx compression */ > > + unsigned long va_offset = maddr_to_directmapoff(ma); > > Why the mentioning of PDX compression? It was mentioned because if PDX will be enabled maddr_to_directmapoff() will take into account PDX stuff. > At least right now it's unavailable > for RISC-V afaics. Are there plans to change that any time soon? At the moment, I don't have such plans, looking at available platform there are no a lot of benefits of having PDX compression now. Perhaps it would be good to add BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which should be updated when CONFIG_PDX will be enabled. ~ Oleksii
On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote: > On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote: > > On 16.10.2024 11:15, Oleksii Kurochko wrote: > > > --- a/xen/arch/riscv/include/asm/mm.h > > > +++ b/xen/arch/riscv/include/asm/mm.h > > > @@ -25,8 +25,12 @@ > > > > > > static inline void *maddr_to_virt(paddr_t ma) > > > { > > > - BUG_ON("unimplemented"); > > > - return NULL; > > > + /* Offset in the direct map, accounting for pdx compression */ > > > + unsigned long va_offset = maddr_to_directmapoff(ma); > > > > Why the mentioning of PDX compression? > It was mentioned because if PDX will be enabled maddr_to_directmapoff() > will take into account PDX stuff. > > > At least right now it's unavailable > > for RISC-V afaics. Are there plans to change that any time soon? > At the moment, I don't have such plans, looking at available platform > there are no a lot of benefits of having PDX compression now. > > Perhaps it would be good to add > BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which should > be updated when CONFIG_PDX will be enabled. > > ~ Oleksii I'd just forget about it unless you ever notice you're wasting a lot of entries in the frame table due to empty space in the memory map. Julien measured the effect on Amazon's Live Migration as a 10% improvement in downtime with PDX off. PDX compression shines when you have separate RAM banks at very, very disparately far addresses (specifics in pdx.h). Unfortunately the flip side of this compression is that you get several memory accesses for each single pdx-(to/from)-mfn conversion. And we do a lot of those. One possible solution would be to alt-patch the values in the code-stream and avoid the perf-hit, but that's not merged. Jan had some patches but that didn't make it to staging, IIRC. Cheers, Alejandro
On Mon, 2024-10-21 at 08:56 +0100, Alejandro Vallejo wrote: > On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote: > > On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote: > > > On 16.10.2024 11:15, Oleksii Kurochko wrote: > > > > --- a/xen/arch/riscv/include/asm/mm.h > > > > +++ b/xen/arch/riscv/include/asm/mm.h > > > > @@ -25,8 +25,12 @@ > > > > > > > > static inline void *maddr_to_virt(paddr_t ma) > > > > { > > > > - BUG_ON("unimplemented"); > > > > - return NULL; > > > > + /* Offset in the direct map, accounting for pdx > > > > compression */ > > > > + unsigned long va_offset = maddr_to_directmapoff(ma); > > > > > > Why the mentioning of PDX compression? > > It was mentioned because if PDX will be enabled > > maddr_to_directmapoff() > > will take into account PDX stuff. > > > > > At least right now it's unavailable > > > for RISC-V afaics. Are there plans to change that any time soon? > > At the moment, I don't have such plans, looking at available > > platform > > there are no a lot of benefits of having PDX compression now. > > > > Perhaps it would be good to add > > BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which > > should > > be updated when CONFIG_PDX will be enabled. > > > > ~ Oleksii > > I'd just forget about it unless you ever notice you're wasting a lot > of entries > in the frame table due to empty space in the memory map. Julien > measured the > effect on Amazon's Live Migration as a 10% improvement in downtime > with PDX > off. > > PDX compression shines when you have separate RAM banks at very, very > disparately far addresses (specifics in pdx.h). Unfortunately the > flip side of > this compression is that you get several memory accesses for each > single > pdx-(to/from)-mfn conversion. And we do a lot of those. One possible > solution > would be to alt-patch the values in the code-stream and avoid the > perf-hit, but > that's not merged. Jan had some patches but that didn't make it to > staging, > IIRC. Could you please give me some links in the mailing list with mentioned patches? ~ Oleksii
On Mon Oct 21, 2024 at 10:17 AM BST, oleksii.kurochko wrote: > On Mon, 2024-10-21 at 08:56 +0100, Alejandro Vallejo wrote: > > On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote: > > > On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote: > > > > On 16.10.2024 11:15, Oleksii Kurochko wrote: > > > > > --- a/xen/arch/riscv/include/asm/mm.h > > > > > +++ b/xen/arch/riscv/include/asm/mm.h > > > > > @@ -25,8 +25,12 @@ > > > > > > > > > > static inline void *maddr_to_virt(paddr_t ma) > > > > > { > > > > > - BUG_ON("unimplemented"); > > > > > - return NULL; > > > > > + /* Offset in the direct map, accounting for pdx > > > > > compression */ > > > > > + unsigned long va_offset = maddr_to_directmapoff(ma); > > > > > > > > Why the mentioning of PDX compression? > > > It was mentioned because if PDX will be enabled > > > maddr_to_directmapoff() > > > will take into account PDX stuff. > > > > > > > At least right now it's unavailable > > > > for RISC-V afaics. Are there plans to change that any time soon? > > > At the moment, I don't have such plans, looking at available > > > platform > > > there are no a lot of benefits of having PDX compression now. > > > > > > Perhaps it would be good to add > > > BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which > > > should > > > be updated when CONFIG_PDX will be enabled. > > > > > > ~ Oleksii > > > > I'd just forget about it unless you ever notice you're wasting a lot > > of entries > > in the frame table due to empty space in the memory map. Julien > > measured the > > effect on Amazon's Live Migration as a 10% improvement in downtime > > with PDX > > off. > > > > PDX compression shines when you have separate RAM banks at very, very > > disparately far addresses (specifics in pdx.h). Unfortunately the > > flip side of > > this compression is that you get several memory accesses for each > > single > > pdx-(to/from)-mfn conversion. And we do a lot of those. One possible > > solution > > would be to alt-patch the values in the code-stream and avoid the > > perf-hit, but > > that's not merged. Jan had some patches but that didn't make it to > > staging, > > IIRC. > Could you please give me some links in the mailing list with mentioned > patches? > > ~ Oleksii Sure. Much of this was discussed in the "Make PDX compression optional" series. This link is v1, but there were 3 in total and a pre-patch documenting pdx.h explaining what the technique actually does to make sure we were all on the same page (pun intended) and the pdx-off case wouldn't break the world. https://lore.kernel.org/xen-devel/20230717160318.2113-1-alejandro.vallejo@cloud.com/ This was Jan's 2018 take to turn PDX into alternatives. He mentioned it somewhere in those threads, but I can't find that message anymore. https://lore.kernel.org/xen-devel/5B76740802000078001DF345@prv1-mh.provo.novell.com/ Cheers, Alejandro
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index ebb142502e..0396e66f47 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -25,8 +25,12 @@ static inline void *maddr_to_virt(paddr_t ma) { - BUG_ON("unimplemented"); - return NULL; + /* Offset in the direct map, accounting for pdx compression */ + unsigned long va_offset = maddr_to_directmapoff(ma); + + ASSERT(va_offset < DIRECTMAP_SIZE); + + return (void *)(DIRECTMAP_VIRT_START + va_offset); } /*
Implement the `maddr_to_virt()` function to convert a machine address to a virtual address. This function is specifically designed to be used only for the DIRECTMAP region, so a check has been added to ensure that the address does not exceed `DIRECTMAP_SIZE`. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/include/asm/mm.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)