1. Patchew
  2. Xen
  3. [PATCH v1 0/5] Setup memory management
  4. View patch

[PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

Oleksii Kurochko posted 5 patches 1 month, 1 week ago
[PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by Oleksii Kurochko 1 month, 1 week ago 
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(-)

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);
 }
 
 /*
-- 
2.47.0
Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by Jan Beulich 1 month, 1 week ago 
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
Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by oleksii.kurochko@gmail.com 1 month ago 
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
Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by Alejandro Vallejo 1 month ago 
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
Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by oleksii.kurochko@gmail.com 1 month ago 
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
Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by Alejandro Vallejo 1 month ago 
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
Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()
Posted by Alejandro Vallejo 1 month, 1 week ago 
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);
>  }
>  
>  /*

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.