| Message ID | 57e892d5b2c526dd44b68d4976796a25c0feca20.1724256027.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV device tree mapping | expand |
On 21.08.2024 18:06, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -41,8 +41,10 @@ > * Start addr | End addr | Slot | area description > * ============================================================================ > * ..... L2 511 Unused > - * 0xffffffffc0600000 0xffffffffc0800000 L2 511 Fixmap > - * 0xffffffffc0200000 0xffffffffc0600000 L2 511 FDT > + * 0xffffffffc0A00000 0xffffffffc0C00000 L2 511 Fixmap Nit: Please can you avoid using mixed case in numbers? > @@ -74,6 +76,15 @@ > #error "unsupported RV_STAGE1_MODE" > #endif > > +#define GAP_SIZE MB(2) > + > +#define XEN_VIRT_SIZE MB(2) > + > +#define BOOT_FDT_VIRT_START (XEN_VIRT_START + XEN_VIRT_SIZE + GAP_SIZE) > +#define BOOT_FDT_VIRT_SIZE MB(4) > + > +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE + GAP_SIZE) Nit: Overly long line. > --- /dev/null > +++ b/xen/arch/riscv/include/asm/fixmap.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * fixmap.h: compile-time virtual memory allocation > + */ > +#ifndef ASM_FIXMAP_H > +#define ASM_FIXMAP_H > + > +#include <xen/bug.h> > +#include <xen/page-size.h> > +#include <xen/pmap.h> > + > +#include <asm/page.h> > + > +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) > + > +/* Fixmap slots */ > +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ > +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */ > +#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of hardware */ > + > +#define FIX_LAST FIX_MISC > + > +#define FIXADDR_START FIXMAP_ADDR(0) > +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1) > + > +#ifndef __ASSEMBLY__ > + > +/* > + * Direct access to xen_fixmap[] should only happen when {set, > + * clear}_fixmap() is unusable (e.g. where we would end up to > + * recursively call the helpers). > + */ > +extern pte_t xen_fixmap[]; I'm afraid I keep being irritated by the comment: What recursive use of helpers is being talked about here? I can't see anything recursive in this patch. If this starts happening with a subsequent patch, then you have two options: Move the declaration + comment there, or clarify in the description (in enough detail) what this is about. > @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) > BUG_ON("unimplemented"); > } > > +/* Write a pagetable entry. */ > +static inline void write_pte(pte_t *p, pte_t pte) > +{ > + write_atomic(p, pte); > +} > + > +/* Read a pagetable entry. */ > +static inline pte_t read_pte(pte_t *p) > +{ > + return read_atomic(p); This only works because of the strange type trickery you're playing in read_atomic(). Look at x86 code - there's a strict expectation that the type can be converted to/from unsigned long. And page table accessors are written with that taken into consideration. Same goes for write_pte() of course, with the respective comment on the earlier patch in mind. Otoh I see that Arm does something very similar. If you have a strong need / desire to follow that, then please at least split the two entirely separate aspects that patch 1 presently changes both in one go. Jan
On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: > > > > + > > +/* > > + * Direct access to xen_fixmap[] should only happen when {set, > > + * clear}_fixmap() is unusable (e.g. where we would end up to > > + * recursively call the helpers). > > + */ > > +extern pte_t xen_fixmap[]; > > I'm afraid I keep being irritated by the comment: What recursive use > of > helpers is being talked about here? I can't see anything recursive in > this > patch. If this starts happening with a subsequent patch, then you > have > two options: Move the declaration + comment there, or clarify in the > description (in enough detail) what this is about. This comment is added because of: ``` void *__init pmap_map(mfn_t mfn) ... /* * We cannot use set_fixmap() here. We use PMAP when the domain map * page infrastructure is not yet initialized, so map_pages_to_xen() called * by set_fixmap() needs to map pages on demand, which then calls pmap() * again, resulting in a loop. Modify the PTEs directly instead. The same * is true for pmap_unmap(). */ arch_pmap_map(slot, mfn); ... ``` And it happens because set_fixmap() could be defined using generic PT helpers so what will lead to recursive behaviour when when there is no direct map: static pte_t *map_table(mfn_t mfn) { /* * During early boot, map_domain_page() may be unusable. Use the * PMAP to map temporarily a page-table. */ if ( system_state == SYS_STATE_early_boot ) return pmap_map(mfn); ... } > > > @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned > > long mfn, bool sync_icache) > > BUG_ON("unimplemented"); > > } > > > > +/* Write a pagetable entry. */ > > +static inline void write_pte(pte_t *p, pte_t pte) > > +{ > > + write_atomic(p, pte); > > +} > > + > > +/* Read a pagetable entry. */ > > +static inline pte_t read_pte(pte_t *p) > > +{ > > + return read_atomic(p); > > This only works because of the strange type trickery you're playing > in > read_atomic(). Look at x86 code - there's a strict expectation that > the > type can be converted to/from unsigned long. And page table accessors > are written with that taken into consideration. Same goes for > write_pte() > of course, with the respective comment on the earlier patch in mind. I will check x86 code. Probably my answer on the patch with read/write_atomic() suggest that too. Based on the answers to that patch I think we can go with x86 approach. Thanks. ~ Oleksii > > Otoh I see that Arm does something very similar. If you have a strong > need / desire to follow that, then please at least split the two > entirely separate aspects that patch 1 presently changes both in one > go. > > Jan
On 28.08.2024 11:53, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: >>> >>> + >>> +/* >>> + * Direct access to xen_fixmap[] should only happen when {set, >>> + * clear}_fixmap() is unusable (e.g. where we would end up to >>> + * recursively call the helpers). >>> + */ >>> +extern pte_t xen_fixmap[]; >> >> I'm afraid I keep being irritated by the comment: What recursive use >> of >> helpers is being talked about here? I can't see anything recursive in >> this >> patch. If this starts happening with a subsequent patch, then you >> have >> two options: Move the declaration + comment there, or clarify in the >> description (in enough detail) what this is about. > This comment is added because of: > ``` > void *__init pmap_map(mfn_t mfn) > ... > /* > * We cannot use set_fixmap() here. We use PMAP when the domain map > * page infrastructure is not yet initialized, so > map_pages_to_xen() called > * by set_fixmap() needs to map pages on demand, which then calls > pmap() > * again, resulting in a loop. Modify the PTEs directly instead. > The same > * is true for pmap_unmap(). > */ > arch_pmap_map(slot, mfn); > ... > ``` > And it happens because set_fixmap() could be defined using generic PT > helpers As you say - could be. If I'm not mistaken no set_fixmap() implementation exists even by the end of the series. Fundamentally I'd expect set_fixmap() to (possibly) use xen_fixmap[] directly. That in turn ... > so what will lead to recursive behaviour when when there is no > direct map: ... would mean no recursion afaict. Hence why clarification is needed as to what's going on here _and_ what's planned. Jan > static pte_t *map_table(mfn_t mfn) > { > /* > * During early boot, map_domain_page() may be unusable. Use the > * PMAP to map temporarily a page-table. > */ > if ( system_state == SYS_STATE_early_boot ) > return pmap_map(mfn); > ... > }
On Wed, 2024-08-28 at 12:44 +0200, Jan Beulich wrote: > On 28.08.2024 11:53, oleksii.kurochko@gmail.com wrote: > > On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: > > > > > > > > + > > > > +/* > > > > + * Direct access to xen_fixmap[] should only happen when {set, > > > > + * clear}_fixmap() is unusable (e.g. where we would end up to > > > > + * recursively call the helpers). > > > > + */ > > > > +extern pte_t xen_fixmap[]; > > > > > > I'm afraid I keep being irritated by the comment: What recursive > > > use > > > of > > > helpers is being talked about here? I can't see anything > > > recursive in > > > this > > > patch. If this starts happening with a subsequent patch, then you > > > have > > > two options: Move the declaration + comment there, or clarify in > > > the > > > description (in enough detail) what this is about. We can't move declaration of xen_fixmap[] to the patch where set_fixmap() will be introduced ( which uses PMAP for domain map page infrastructure ) as xen_fixmap[] is used in the current patch. And we can't properly provide the proper description with the function which will be introduced one day in the future ( what can be not good too ). I came up with the following description in the comment above xen_fixmap[] declaration: /* * Direct access to xen_fixmap[] should only happen when {set, * clear}_fixmap() is unusable (e.g. where we would end up to * recursively call the helpers). * * One such case is pmap_map() where set_fixmap() can not be used. * It happens because PMAP is used when the domain map page infrastructure * is not yet initialized, so map_pages_to_xen() called by set_fixmap() needs * to map pages on demand, which then calls pmap() again, resulting in a loop. * Modification of the PTEs directly instead in arch_pmap_map(). * The same is true for pmap_unmap(). */ Could it be an option just to drop the comment for now at all as at the moment there is no such restriction with the usage of {set,clear}_fixmap() and xen_fixmap[]? > > This comment is added because of: > > ``` > > void *__init pmap_map(mfn_t mfn) > > ... > > /* > > * We cannot use set_fixmap() here. We use PMAP when the > > domain map > > * page infrastructure is not yet initialized, so > > map_pages_to_xen() called > > * by set_fixmap() needs to map pages on demand, which then > > calls > > pmap() > > * again, resulting in a loop. Modify the PTEs directly > > instead. > > The same > > * is true for pmap_unmap(). > > */ > > arch_pmap_map(slot, mfn); > > ... > > ``` > > And it happens because set_fixmap() could be defined using generic > > PT > > helpers > > As you say - could be. If I'm not mistaken no set_fixmap() > implementation > exists even by the end of the series. Fundamentally I'd expect > set_fixmap() > to (possibly) use xen_fixmap[] directly. That in turn ... > > > so what will lead to recursive behaviour when when there is no > > direct map: > > ... would mean no recursion afaict. Hence why clarification is needed > as > to what's going on here _and_ what's planned. Yes, it is true. No recursion will happen in this case but if to look at the implementation of set_fixmap() for other Arm or x86 ( but I am not sure that x86 uses PMAP inside map_pages_to_xen() ) they are using map_pages_to_xen(). ~ Oleksii > > > static pte_t *map_table(mfn_t mfn) > > { > > /* > > * During early boot, map_domain_page() may be unusable. Use > > the > > * PMAP to map temporarily a page-table. > > */ > > if ( system_state == SYS_STATE_early_boot ) > > return pmap_map(mfn); > > ... > > } >
On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: > > @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned > > long mfn, bool sync_icache) > > BUG_ON("unimplemented"); > > } > > > > +/* Write a pagetable entry. */ > > +static inline void write_pte(pte_t *p, pte_t pte) > > +{ > > + write_atomic(p, pte); > > +} > > + > > +/* Read a pagetable entry. */ > > +static inline pte_t read_pte(pte_t *p) > > +{ > > + return read_atomic(p); > > This only works because of the strange type trickery you're playing > in > read_atomic(). Look at x86 code - there's a strict expectation that > the > type can be converted to/from unsigned long. And page table accessors > are written with that taken into consideration. Same goes for > write_pte() > of course, with the respective comment on the earlier patch in mind. > > Otoh I see that Arm does something very similar. If you have a strong > need / desire to follow that, then please at least split the two > entirely separate aspects that patch 1 presently changes both in one > go. I am not 100% sure that type trick could be dropped easily for RISC-V: 1. I still need the separate C function for proper #ifdef-ing: #ifndef CONFIG_RISCV_32 case 8: *(uint32_t *)res = readq_cpu(p); break; #endif 2. Because of the point 1 the change should be as following: -#define read_atomic(p) ({ \ - union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_; \ - read_atomic_size(p, x_.c, sizeof(*(p))); \ - x_.val; \ +#define read_atomic(p) ({ \ + unsigned long x_; \ + read_atomic_size(p, &x_, sizeof(*(p))); \ + (typeof(*(p)))x_; \ }) But after that I think it will be an error: "conversion to non-scalar type requested" in the last line as *p points to pte_t. and we can't just in read_pte() change to: static inline pte_t read_pte(pte_t *p) { return read_atomic(&p->pte); } As in this cases it started it will return unsigned long but function expects pte_t. As an option read_pte() can be updated to: /* Read a pagetable entry. */ static inline pte_t read_pte(pte_t *p) { return (pte_t) { .pte = read_atomic(&p->pte) }; } But I am not sure that it is better then just have a union trick inside read_atomic() and then just have read_atomic(p) for read_pte(). Am I missing something? ~ Oleksii
On 30.08.2024 13:55, oleksii.kurochko@gmail.com wrote: > On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: >>> @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned >>> long mfn, bool sync_icache) >>> BUG_ON("unimplemented"); >>> } >>> >>> +/* Write a pagetable entry. */ >>> +static inline void write_pte(pte_t *p, pte_t pte) >>> +{ >>> + write_atomic(p, pte); >>> +} >>> + >>> +/* Read a pagetable entry. */ >>> +static inline pte_t read_pte(pte_t *p) >>> +{ >>> + return read_atomic(p); >> >> This only works because of the strange type trickery you're playing >> in >> read_atomic(). Look at x86 code - there's a strict expectation that >> the >> type can be converted to/from unsigned long. And page table accessors >> are written with that taken into consideration. Same goes for >> write_pte() >> of course, with the respective comment on the earlier patch in mind. >> >> Otoh I see that Arm does something very similar. If you have a strong >> need / desire to follow that, then please at least split the two >> entirely separate aspects that patch 1 presently changes both in one >> go. > I am not 100% sure that type trick could be dropped easily for RISC-V: > 1. I still need the separate C function for proper #ifdef-ing: > #ifndef CONFIG_RISCV_32 > case 8: *(uint32_t *)res = readq_cpu(p); break; > #endif > > 2. Because of the point 1 the change should be as following: > -#define read_atomic(p) ({ \ > - union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_; \ > - read_atomic_size(p, x_.c, sizeof(*(p))); \ > - x_.val; \ > +#define read_atomic(p) ({ \ > + unsigned long x_; \ > + read_atomic_size(p, &x_, sizeof(*(p))); \ > + (typeof(*(p)))x_; \ > }) > But after that I think it will be an error: "conversion to non-scalar > type requested" in the last line as *p points to pte_t. > > and we can't just in read_pte() change to: > static inline pte_t read_pte(pte_t *p) > { > return read_atomic(&p->pte); > } > As in this cases it started it will return unsigned long but function > expects pte_t. Of course. > As an option read_pte() can be updated to: > /* Read a pagetable entry. */ > static inline pte_t read_pte(pte_t *p) > { > return (pte_t) { .pte = read_atomic(&p->pte) }; > } That's what's needed. > But I am not sure that it is better then just have a union trick inside > read_atomic() and then just have read_atomic(p) for read_pte(). It's largely up to you. My main request is that things end up / remain consistent. Which way round is secondary, and often merely a matter of suitably justifying the choice made. Jan
On 30.08.2024 13:01, oleksii.kurochko@gmail.com wrote: > On Wed, 2024-08-28 at 12:44 +0200, Jan Beulich wrote: >> On 28.08.2024 11:53, oleksii.kurochko@gmail.com wrote: >>> On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: >>>>> >>>>> + >>>>> +/* >>>>> + * Direct access to xen_fixmap[] should only happen when {set, >>>>> + * clear}_fixmap() is unusable (e.g. where we would end up to >>>>> + * recursively call the helpers). >>>>> + */ >>>>> +extern pte_t xen_fixmap[]; >>>> >>>> I'm afraid I keep being irritated by the comment: What recursive >>>> use >>>> of >>>> helpers is being talked about here? I can't see anything >>>> recursive in >>>> this >>>> patch. If this starts happening with a subsequent patch, then you >>>> have >>>> two options: Move the declaration + comment there, or clarify in >>>> the >>>> description (in enough detail) what this is about. > We can't move declaration of xen_fixmap[] to the patch where > set_fixmap() will be introduced ( which uses PMAP for domain map page > infrastructure ) as xen_fixmap[] is used in the current patch. > > And we can't properly provide the proper description with the function > which will be introduced one day in the future ( what can be not good > too ). I came up with the following description in the comment above > xen_fixmap[] declaration: > /* > * Direct access to xen_fixmap[] should only happen when {set, > * clear}_fixmap() is unusable (e.g. where we would end up to > * recursively call the helpers). > * > * One such case is pmap_map() where set_fixmap() can not be used. > * It happens because PMAP is used when the domain map page > infrastructure > * is not yet initialized, so map_pages_to_xen() called by > set_fixmap() needs > * to map pages on demand, which then calls pmap() again, resulting > in a loop. > * Modification of the PTEs directly instead in arch_pmap_map(). > * The same is true for pmap_unmap(). > */ > > Could it be an option just to drop the comment for now at all as at the > moment there is no such restriction with the usage of > {set,clear}_fixmap() and xen_fixmap[]? The comment isn't the right place to explain things here, imo. It's the patch description where unexpected aspects need shedding light on. >>> This comment is added because of: >>> ``` >>> void *__init pmap_map(mfn_t mfn) >>> ... >>> /* >>> * We cannot use set_fixmap() here. We use PMAP when the >>> domain map >>> * page infrastructure is not yet initialized, so >>> map_pages_to_xen() called >>> * by set_fixmap() needs to map pages on demand, which then >>> calls >>> pmap() >>> * again, resulting in a loop. Modify the PTEs directly >>> instead. >>> The same >>> * is true for pmap_unmap(). >>> */ >>> arch_pmap_map(slot, mfn); >>> ... >>> ``` >>> And it happens because set_fixmap() could be defined using generic >>> PT >>> helpers >> >> As you say - could be. If I'm not mistaken no set_fixmap() >> implementation >> exists even by the end of the series. Fundamentally I'd expect >> set_fixmap() >> to (possibly) use xen_fixmap[] directly. That in turn ... >> >>> so what will lead to recursive behaviour when when there is no >>> direct map: >> >> ... would mean no recursion afaict. Hence why clarification is needed >> as >> to what's going on here _and_ what's planned. > Yes, it is true. No recursion will happen in this case but if to look > at the implementation of set_fixmap() for other Arm or x86 ( but I am > not sure that x86 uses PMAP inside map_pages_to_xen() ) they are using > map_pages_to_xen(). There's no PMAP so far on x86. Jan
diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h index 50583aafdc..f55d6c45da 100644 --- a/xen/arch/riscv/include/asm/config.h +++ b/xen/arch/riscv/include/asm/config.h @@ -41,8 +41,10 @@ * Start addr | End addr | Slot | area description * ============================================================================ * ..... L2 511 Unused - * 0xffffffffc0600000 0xffffffffc0800000 L2 511 Fixmap - * 0xffffffffc0200000 0xffffffffc0600000 L2 511 FDT + * 0xffffffffc0A00000 0xffffffffc0C00000 L2 511 Fixmap + * ..... ( 2 MB gap ) + * 0xffffffffc0400000 0xffffffffc0800000 L2 511 FDT + * ..... ( 2 MB gap ) * 0xffffffffc0000000 0xffffffffc0200000 L2 511 Xen * ..... L2 510 Unused * 0x3200000000 0x7f40000000 L2 200-509 Direct map @@ -74,6 +76,15 @@ #error "unsupported RV_STAGE1_MODE" #endif +#define GAP_SIZE MB(2) + +#define XEN_VIRT_SIZE MB(2) + +#define BOOT_FDT_VIRT_START (XEN_VIRT_START + XEN_VIRT_SIZE + GAP_SIZE) +#define BOOT_FDT_VIRT_SIZE MB(4) + +#define FIXMAP_BASE (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE + GAP_SIZE) + #define DIRECTMAP_SLOT_END 509 #define DIRECTMAP_SLOT_START 200 #define DIRECTMAP_VIRT_START SLOTN(DIRECTMAP_SLOT_START) diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h new file mode 100644 index 0000000000..63732df36c --- /dev/null +++ b/xen/arch/riscv/include/asm/fixmap.h @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * fixmap.h: compile-time virtual memory allocation + */ +#ifndef ASM_FIXMAP_H +#define ASM_FIXMAP_H + +#include <xen/bug.h> +#include <xen/page-size.h> +#include <xen/pmap.h> + +#include <asm/page.h> + +#define FIXMAP_ADDR(n) (FIXMAP_BASE + (n) * PAGE_SIZE) + +/* Fixmap slots */ +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */ +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */ +#define FIX_MISC (FIX_PMAP_END + 1) /* Ephemeral mappings of hardware */ + +#define FIX_LAST FIX_MISC + +#define FIXADDR_START FIXMAP_ADDR(0) +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1) + +#ifndef __ASSEMBLY__ + +/* + * Direct access to xen_fixmap[] should only happen when {set, + * clear}_fixmap() is unusable (e.g. where we would end up to + * recursively call the helpers). + */ +extern pte_t xen_fixmap[]; + +#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot)) + +static inline unsigned int virt_to_fix(vaddr_t vaddr) +{ + BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START); + + return ((vaddr - FIXADDR_START) >> PAGE_SHIFT); +} + +#endif /* __ASSEMBLY__ */ + +#endif /* ASM_FIXMAP_H */ diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h index 25af9e1aaa..a0bdc2bc3a 100644 --- a/xen/arch/riscv/include/asm/mm.h +++ b/xen/arch/riscv/include/asm/mm.h @@ -255,4 +255,6 @@ static inline unsigned int arch_get_dma_bitsize(void) return 32; /* TODO */ } +void setup_fixmap_mappings(void); + #endif /* _ASM_RISCV_MM_H */ diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h index c831e16417..a7419b93b2 100644 --- a/xen/arch/riscv/include/asm/page.h +++ b/xen/arch/riscv/include/asm/page.h @@ -9,6 +9,7 @@ #include <xen/bug.h> #include <xen/types.h> +#include <asm/atomic.h> #include <asm/mm.h> #include <asm/page-bits.h> @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) BUG_ON("unimplemented"); } +/* Write a pagetable entry. */ +static inline void write_pte(pte_t *p, pte_t pte) +{ + write_atomic(p, pte); +} + +/* Read a pagetable entry. */ +static inline pte_t read_pte(pte_t *p) +{ + return read_atomic(p); +} + #endif /* __ASSEMBLY__ */ #endif /* _ASM_RISCV_PAGE_H */ diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c index 7d09e781bf..b8ff91cf4e 100644 --- a/xen/arch/riscv/mm.c +++ b/xen/arch/riscv/mm.c @@ -12,6 +12,7 @@ #include <asm/early_printk.h> #include <asm/csr.h> #include <asm/current.h> +#include <asm/fixmap.h> #include <asm/page.h> #include <asm/processor.h> @@ -49,6 +50,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES]; pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT * PAGETABLE_ENTRIES]; +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) +xen_fixmap[PAGETABLE_ENTRIES]; + #define HANDLE_PGTBL(curr_lvl_num) \ index = pt_index(curr_lvl_num, page_addr); \ if ( pte_is_valid(pgtbl[index]) ) \ @@ -191,6 +195,45 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc, return is_mode_supported; } +void __init setup_fixmap_mappings(void) +{ + pte_t *pte, tmp; + unsigned int i; + + BUILD_BUG_ON(FIX_LAST >= PAGETABLE_ENTRIES); + + pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))]; + + /* + * In RISC-V page table levels are numbered from Lx to L0 where + * x is the highest page table level for currect MMU mode ( for example, + * for Sv39 has 3 page tables so the x = 2 (L2 -> L1 -> L0) ). + * + * In this cycle we want to find L1 page table because as L0 page table + * xen_fixmap[] will be used. + */ + for ( i = HYP_PT_ROOT_LEVEL; i-- > 1; ) + { + BUG_ON(!pte_is_valid(*pte)); + + pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte)); + pte = &pte[pt_index(i, FIXMAP_ADDR(0))]; + } + + BUG_ON(pte_is_valid(*pte)); + + tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE); + write_pte(pte, tmp); + + RISCV_FENCE(rw, rw); + sfence_vma(); + + /* + * We only need the zeroeth table allocated, but not the PTEs set, because + * set_fixmap() will set them on the fly. + */ +} + /* * setup_initial_pagetables: * diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 4defad68f4..13f0e8c77d 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -46,6 +46,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id, test_macros_from_bug_h(); #endif + setup_fixmap_mappings(); + printk("All set up\n"); for ( ;; ) diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S index 070b19d915..7a683f6065 100644 --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -181,6 +181,6 @@ ASSERT(!SIZEOF(.got.plt), ".got.plt non-empty") * Changing the size of Xen binary can require an update of * PGTBL_INITIAL_COUNT. */ -ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions") +ASSERT(_end - _start <= XEN_VIRT_SIZE, "Xen too large for early-boot assumptions") ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big");
Set up fixmap mappings and the L0 page table for fixmap support. Define new macros in riscv/config.h for calculating the FIXMAP_BASE address, including BOOT_FDT_VIRT_{START, SIZE}, XEN_VIRT_SIZE, and XEN_VIRT_END. Update the check for Xen size in riscv/lds.S to use XEN_VIRT_SIZE instead of a hardcoded constant. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V5: - move definition of FIXMAP_ADDR() to asm/fixmap.h - add gap size equal to 2 MB ( 512 * 4K one page table entry in L1 page table ) between Xen, FDT and Fixmap. - drop the comment for FIX_LAST. - move +1 from FIX_LAST definition to FIXADDR_TOP to be aligned with Arm. ( probably everything below FIX_LAST will be moved to a separate header in asm/generic.h ) - correct the "changes in V4: s/'fence r,r'/'fence rw, rw' - use write_atomic() in set_pte(). - introduce read_pte(). --- Changes in V4: - move definitions of XEN_VIRT_SIZE, BOOT_FDT_VIRT_{START,SIZE}, FIXMAP_{BASE,ADDR} below XEN_VIRT_START to have definitions appear in order. - define FIX_LAST as (FIX_MISC + 1) to have a guard slot at the end. - s/enumerated/numbered in the comment - update the cycle which looks for L1 page table in setup_fixmap_mapping_function() and the comment above him. - drop fences inside write_pte() and put 'fence rw,rw' in setup_fixmap() before sfence_vma(). - update the commit message - drop printk message inside setup_fixmap(). --- Changes in V3: - s/XEN_SIZE/XEN_VIRT_SIZE - drop usage of XEN_VIRT_END. - sort newly introduced defines in config.h by address - code style fixes - drop runtime check of that pte is valid as it was checked in L1 page table finding cycle by BUG_ON(). - update implementation of write_pte() with FENCE rw, rw. - add BUILD_BUG_ON() to check that amount of entries aren't bigger then entries in page table. - drop set_fixmap, clear_fixmap declarations as they aren't used and defined now - update the commit message. - s/__ASM_FIXMAP_H/ASM_FIXMAP_H - add SPDX-License-Identifier: GPL-2.0 --- xen/arch/riscv/include/asm/config.h | 15 ++++++++-- xen/arch/riscv/include/asm/fixmap.h | 46 +++++++++++++++++++++++++++++ xen/arch/riscv/include/asm/mm.h | 2 ++ xen/arch/riscv/include/asm/page.h | 13 ++++++++ xen/arch/riscv/mm.c | 43 +++++++++++++++++++++++++++ xen/arch/riscv/setup.c | 2 ++ xen/arch/riscv/xen.lds.S | 2 +- 7 files changed, 120 insertions(+), 3 deletions(-) create mode 100644 xen/arch/riscv/include/asm/fixmap.h