| Message ID | 25a0fa030db90c929379a799aa5e03bed0197665.1728057657.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Register Xen's load address as a boot module | expand |
On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote: > 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> > --- > Changes in V2: > - Drop casts in virt_to_maddr() for ASSERT which checks that VA is > in the range of where Xen is located. > - Add UL suffix for or XEN_VIRT_START by using _AC(..., UL) and add > inclusion > of <xen/const.h> > - Add the comment above return which explains why there is no need > to do " - XEN_VIRT_START. > --- > xen/arch/riscv/include/asm/config.h | 4 ++++ > xen/arch/riscv/include/asm/mm.h | 17 ++++++++++++++++- > xen/arch/riscv/mm.c | 2 +- > 3 files changed, 21 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..0f7879d685 100644 > --- a/xen/arch/riscv/include/asm/mm.h > +++ b/xen/arch/riscv/include/asm/mm.h > @@ -5,6 +5,7 @@ > > #include <public/xen.h> > #include <xen/bug.h> > +#include <xen/const.h> > #include <xen/mm-frame.h> > #include <xen/pdx.h> > #include <xen/types.h> > @@ -28,7 +29,21 @@ 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)); It should be ASSERT(va < (...)). Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as address from Xen's VA space ( XEN_VIRT_START ) are higher then (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE). Or as an option we could consider to drop this ASSERT() as if VA is from directmap range the if below will catch that; otherwise we have another one ASSERT() which checks that VA is from Xen VA range where it is sage to use (phys_offset + va). Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE))" or I am missing something? ~ Oleksii > + 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((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) == > + (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + > PAGE_SHIFT))); > + > + /* phys_offset = load_start - XEN_VIRT_START */ > + 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)
On 08.10.2024 12:26, oleksii.kurochko@gmail.com wrote: > On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote: >> @@ -28,7 +29,21 @@ 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)); > It should be ASSERT(va < (...)). > > Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as > address from Xen's VA space ( XEN_VIRT_START ) are higher then > (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE). > > Or as an option we could consider to drop this ASSERT() as if > VA is from directmap range the if below will catch that; otherwise > we have another one ASSERT() which checks that VA is from Xen VA range > where it is sage to use (phys_offset + va). > > Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START + > DIRECTMAP_SIZE))" or I am missing something? Counter question: Why did you put the ASSERT() there when - once corrected - it's actually pointless? What you want to make sure is that virt_to_maddr() can't be invoked with bad argument (without noticing). If that's achieved with just the other assertion (as it looks to be), then leaving out this assertion ought to be fine. Jan
On Tue, 2024-10-08 at 12:34 +0200, Jan Beulich wrote: > On 08.10.2024 12:26, oleksii.kurochko@gmail.com wrote: > > On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote: > > > @@ -28,7 +29,21 @@ 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)); > > It should be ASSERT(va < (...)). > > > > Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as > > address from Xen's VA space ( XEN_VIRT_START ) are higher then > > (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE). > > > > Or as an option we could consider to drop this ASSERT() as if > > VA is from directmap range the if below will catch that; otherwise > > we have another one ASSERT() which checks that VA is from Xen VA > > range > > where it is sage to use (phys_offset + va). > > > > Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START > > + > > DIRECTMAP_SIZE))" or I am missing something? > > Counter question: Why did you put the ASSERT() there when - once > corrected - it's actually pointless? What you want to make sure is > that virt_to_maddr() can't be invoked with bad argument (without > noticing). If that's achieved with just the other assertion (as it > looks to be), then leaving out this assertion ought to be fine. Originally, I didn’t include the part after 'if (...)'. The purpose of ASSERT(va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)) was to ensure that the virtual address fell within the expected (directmap) range. Later, I added the part after 'if (...)', which extended the acceptable virtual address range to also cover addresses from Xen’s linkage address space. At that point, I should have removed the original ASSERT() but overlooked it. I will drop the first ASSERT() and update the commit message / comment above virt_to_maddr() why it is enough to have only 1 ASSERT() after if (...). ~ 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..0f7879d685 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -5,6 +5,7 @@ #include <public/xen.h> #include <xen/bug.h> +#include <xen/const.h> #include <xen/mm-frame.h> #include <xen/pdx.h> #include <xen/types.h> @@ -28,7 +29,21 @@ 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((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) == + (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT))); + + /* phys_offset = load_start - XEN_VIRT_START */ + 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> --- Changes in V2: - Drop casts in virt_to_maddr() for ASSERT which checks that VA is in the range of where Xen is located. - Add UL suffix for or XEN_VIRT_START by using _AC(..., UL) and add inclusion of <xen/const.h> - Add the comment above return which explains why there is no need to do " - XEN_VIRT_START. --- xen/arch/riscv/include/asm/config.h | 4 ++++ xen/arch/riscv/include/asm/mm.h | 17 ++++++++++++++++- xen/arch/riscv/mm.c | 2 +- 3 files changed, 21 insertions(+), 2 deletions(-)