1. Patchew
  2. Xen
  3. [PATCH v1 0/3] Register Xen's load address as a boot module
  4. View patch

[PATCH v1 1/3] xen/riscv: implement virt_to_maddr()

Oleksii Kurochko posted 3 patches 2 months, 3 weeks ago
There is a newer version of this series
[PATCH v1 1/3] xen/riscv: implement virt_to_maddr()
Posted by Oleksii Kurochko 2 months, 3 weeks ago 
Implement the virt_to_maddr() function to convert virtual addresses
to machine addresses, including checks for address ranges such as
the direct mapping area (DIRECTMAP_VIRT_START) and the Xen virtual
address space. To implement this, the phys_offset variable is made
accessible outside of riscv/mm.c.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/config.h |  4 ++++
 xen/arch/riscv/include/asm/mm.h     | 15 ++++++++++++++-
 xen/arch/riscv/mm.c                 |  2 +-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index 7dbb235685..8884aeab16 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -155,6 +155,10 @@
 
 #define IDENT_AREA_SIZE 64
 
+#ifndef __ASSEMBLY__
+extern unsigned long phys_offset;
+#endif
+
 #endif /* __RISCV_CONFIG_H__ */
 /*
  * Local variables:
diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
index 4b7b00b850..aa1f86cd5e 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -28,7 +28,20 @@ static inline void *maddr_to_virt(paddr_t ma)
     return NULL;
 }
 
-#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
+static inline unsigned long virt_to_maddr(unsigned long va)
+{
+    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
+    if ((va >= DIRECTMAP_VIRT_START) &&
+        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
+        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
+
+    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
+    ASSERT(((long)va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
+           ((long)XEN_VIRT_START >> (PAGETABLE_ORDER + PAGE_SHIFT)));
+
+    return phys_offset + va;
+}
+#define virt_to_maddr(va) virt_to_maddr((unsigned long)(va))
 
 /* Convert between Xen-heap virtual addresses and machine frame numbers. */
 #define __virt_to_mfn(va)  mfn_x(maddr_to_mfn(virt_to_maddr(va)))
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 4a628aef83..7a1919e07e 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -26,7 +26,7 @@ struct mmu_desc {
     pte_t *pgtbl_base;
 };
 
-static unsigned long __ro_after_init phys_offset;
+unsigned long __ro_after_init phys_offset;
 
 #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
 #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)
-- 
2.46.1
Re: [PATCH v1 1/3] xen/riscv: implement virt_to_maddr()
Posted by Jan Beulich 2 months, 2 weeks ago 
On 30.09.2024 17:08, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -28,7 +28,20 @@ static inline void *maddr_to_virt(paddr_t ma)
>      return NULL;
>  }
>  
> -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
> +static inline unsigned long virt_to_maddr(unsigned long va)
> +{
> +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
> +    if ((va >= DIRECTMAP_VIRT_START) &&
> +        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
> +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);

While the cover letter states a dependency on another series, I'm unable
to spot directmapoff_to_maddr() in the tree or in that other series.

> +    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
> +    ASSERT(((long)va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
> +           ((long)XEN_VIRT_START >> (PAGETABLE_ORDER + PAGE_SHIFT)));

What's the point of the casts here? va is unsigned long and XEN_VIRT_START
is a literal number (which probably ought to have a UL suffix).

> +    return phys_offset + va;

Don't you need to subtract XEN_VIRT_START here?

Jan
Re: [PATCH v1 1/3] xen/riscv: implement virt_to_maddr()
Posted by oleksii.kurochko@gmail.com 2 months, 2 weeks ago 
On Tue, 2024-10-01 at 17:41 +0200, Jan Beulich wrote:
> On 30.09.2024 17:08, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -28,7 +28,20 @@ static inline void *maddr_to_virt(paddr_t ma)
> >      return NULL;
> >  }
> >  
> > -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
> > +static inline unsigned long virt_to_maddr(unsigned long va)
> > +{
> > +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
> > +    if ((va >= DIRECTMAP_VIRT_START) &&
> > +        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
> > +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> 
> While the cover letter states a dependency on another series, I'm
> unable
> to spot directmapoff_to_maddr() in the tree or in that other series.
The definition of directmap_off_to_maddr() is in xen/pdx.h:

#ifdef CONFIG_PDX_COMPRESSION
...
/**
 * 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 paddr_t directmapoff_to_maddr(unsigned long offset)
{
    return ((((paddr_t)offset << pfn_pdx_hole_shift) & ma_top_mask) |
            (offset & ma_va_bottom_mask));
}
...
#else /* !CONFIG_PDX_COMPRESSION */
...
/* directmap is indexed by by maddr */
#define maddr_to_directmapoff(x) (x)
#define directmapoff_to_maddr(x) (x)
...
#endif

> 
> > +    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
> > +    ASSERT(((long)va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
> > +           ((long)XEN_VIRT_START >> (PAGETABLE_ORDER +
> > PAGE_SHIFT)));
> 
> What's the point of the casts here? va is unsigned long and
> XEN_VIRT_START
> is a literal number (which probably ought to have a UL suffix).
I thought that it could be the same as for x86 that:
 /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */
But checking the generated code for RISC-V casts could be dropped
as the generated code is the same.

Regarding UL, I will update to _AC(XEN_VIRT_START, UL).

> 
> > +    return phys_offset + va;
> 
> Don't you need to subtract XEN_VIRT_START here?
It shouldn't as XEN_VIRT_START is taken into account during phys_offset
calculation ( as you mentioned in the reply ).

Regarding the name of phys_offset variable, could it be better to
rename it to load_offset?

As an option I can just add the comment above return:
 /* phys_offset = load_start - XEN_VIRT_START */

~ Oleksii
Re: [PATCH v1 1/3] xen/riscv: implement virt_to_maddr()
Posted by Jan Beulich 2 months, 2 weeks ago 
On 01.10.2024 17:41, Jan Beulich wrote:
> On 30.09.2024 17:08, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/mm.h
>> +++ b/xen/arch/riscv/include/asm/mm.h
>> @@ -28,7 +28,20 @@ static inline void *maddr_to_virt(paddr_t ma)
>>      return NULL;
>>  }
>>  
>> -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
>> +static inline unsigned long virt_to_maddr(unsigned long va)
>> +{
>> +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
>> +    if ((va >= DIRECTMAP_VIRT_START) &&
>> +        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
>> +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> 
> While the cover letter states a dependency on another series, I'm unable
> to spot directmapoff_to_maddr() in the tree or in that other series.
> 
>> +    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
>> +    ASSERT(((long)va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
>> +           ((long)XEN_VIRT_START >> (PAGETABLE_ORDER + PAGE_SHIFT)));
> 
> What's the point of the casts here? va is unsigned long and XEN_VIRT_START
> is a literal number (which probably ought to have a UL suffix).
> 
>> +    return phys_offset + va;
> 
> Don't you need to subtract XEN_VIRT_START here?

Oh, that subtraction is part of the calculation of phys_offset. The variable's
name then doesn't really reflect its purpose, imo.

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.