| Message ID | 1d4270af6469af2f95ede34abd8e9f98f775c1f1.1727708665.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Register Xen's load address as a boot module | expand |
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
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
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
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)
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(-)