| Message ID | 20221216114853.8227-14-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove the directmap | expand |
On 16.12.2022 12:48, Julien Grall wrote: > PMAP will be used in a follow-up patch to bootstap map domain > page infrastructure -- we need some way to map pages to setup the > mapcache without a direct map. But this isn't going to be needed overly early then, seeing that ... > --- a/xen/arch/x86/include/asm/fixmap.h > +++ b/xen/arch/x86/include/asm/fixmap.h > @@ -21,6 +21,8 @@ > > #include <xen/acpi.h> > #include <xen/pfn.h> > +#include <xen/pmap.h> > + > #include <asm/apicdef.h> > #include <asm/msi.h> > #include <acpi/apei.h> > @@ -54,6 +56,8 @@ enum fixed_addresses { > FIX_XEN_SHARED_INFO, > #endif /* CONFIG_XEN_GUEST */ > /* Everything else should go further down. */ > + FIX_PMAP_BEGIN, > + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, ... you've inserted the new entries after the respective comment? Is there a reason you don't insert farther towards the end of this enumeration? > --- /dev/null > +++ b/xen/arch/x86/include/asm/pmap.h > @@ -0,0 +1,25 @@ > +#ifndef __ASM_PMAP_H__ > +#define __ASM_PMAP_H__ > + > +#include <asm/fixmap.h> > + > +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) > +{ > + unsigned long linear = (unsigned long)fix_to_virt(slot); > + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; > + > + ASSERT(!(l1e_get_flags(*pl1e) & _PAGE_PRESENT)); > + > + l1e_write_atomic(pl1e, l1e_from_mfn(mfn, PAGE_HYPERVISOR)); > +} > + > +static inline void arch_pmap_unmap(unsigned int slot) > +{ > + unsigned long linear = (unsigned long)fix_to_virt(slot); > + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; > + > + l1e_write_atomic(pl1e, l1e_empty()); > + flush_tlb_one_local(linear); > +} You're effectively open-coding {set,clear}_fixmap(), just without the L1 table allocation (should such be necessary). If you depend on using the build-time L1 table, then you need to move your entries ahead of said comment. But independent of that you want to either use the existing macros / functions, or explain why you can't. Jan
Hi Jan, On 05/01/2023 16:46, Jan Beulich wrote: > On 16.12.2022 12:48, Julien Grall wrote: >> --- a/xen/arch/x86/include/asm/fixmap.h >> +++ b/xen/arch/x86/include/asm/fixmap.h >> @@ -21,6 +21,8 @@ >> >> #include <xen/acpi.h> >> #include <xen/pfn.h> >> +#include <xen/pmap.h> >> + >> #include <asm/apicdef.h> >> #include <asm/msi.h> >> #include <acpi/apei.h> >> @@ -54,6 +56,8 @@ enum fixed_addresses { >> FIX_XEN_SHARED_INFO, >> #endif /* CONFIG_XEN_GUEST */ >> /* Everything else should go further down. */ >> + FIX_PMAP_BEGIN, >> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, > > ... you've inserted the new entries after the respective comment? Is > there a reason you don't insert farther towards the end of this > enumeration? I will answer this below. > >> --- /dev/null >> +++ b/xen/arch/x86/include/asm/pmap.h >> @@ -0,0 +1,25 @@ >> +#ifndef __ASM_PMAP_H__ >> +#define __ASM_PMAP_H__ >> + >> +#include <asm/fixmap.h> >> + >> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) >> +{ >> + unsigned long linear = (unsigned long)fix_to_virt(slot); >> + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; >> + >> + ASSERT(!(l1e_get_flags(*pl1e) & _PAGE_PRESENT)); >> + >> + l1e_write_atomic(pl1e, l1e_from_mfn(mfn, PAGE_HYPERVISOR)); >> +} >> + >> +static inline void arch_pmap_unmap(unsigned int slot) >> +{ >> + unsigned long linear = (unsigned long)fix_to_virt(slot); >> + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; >> + >> + l1e_write_atomic(pl1e, l1e_empty()); >> + flush_tlb_one_local(linear); >> +} > > You're effectively open-coding {set,clear}_fixmap(), just without > the L1 table allocation (should such be necessary). If you depend > on using the build-time L1 table, then you need to move your > entries ahead of said comment. So the problem is less about the allocation be more the fact that we can't use map_pages_to_xen() because it would call pmap_map(). So we need to break the loop. Hence why set_fixmap()/clear_fixmap() are open-coded. And indeed, we would need to rely on the build-time L1 table in this case. So I will move the entries earlier. > But independent of that you want > to either use the existing macros / functions, or explain why you > can't. This is explained in the caller of arch_pmap*(): /* * 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(). */ The comment is valid for Arm, x86 and (I would expect in the future) RISC-V because the page-tables may be allocated in domheap (so not always mapped). So I don't feel this comment should be duplicated in the header. But I can certainly explain it in the commit message. Cheers,
On 05.01.2023 18:50, Julien Grall wrote: > On 05/01/2023 16:46, Jan Beulich wrote: >> On 16.12.2022 12:48, Julien Grall wrote: >>> --- a/xen/arch/x86/include/asm/fixmap.h >>> +++ b/xen/arch/x86/include/asm/fixmap.h >>> @@ -21,6 +21,8 @@ >>> >>> #include <xen/acpi.h> >>> #include <xen/pfn.h> >>> +#include <xen/pmap.h> >>> + >>> #include <asm/apicdef.h> >>> #include <asm/msi.h> >>> #include <acpi/apei.h> >>> @@ -54,6 +56,8 @@ enum fixed_addresses { >>> FIX_XEN_SHARED_INFO, >>> #endif /* CONFIG_XEN_GUEST */ >>> /* Everything else should go further down. */ >>> + FIX_PMAP_BEGIN, >>> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, >> >> ... you've inserted the new entries after the respective comment? Is >> there a reason you don't insert farther towards the end of this >> enumeration? > > I will answer this below. > >> >>> --- /dev/null >>> +++ b/xen/arch/x86/include/asm/pmap.h >>> @@ -0,0 +1,25 @@ >>> +#ifndef __ASM_PMAP_H__ >>> +#define __ASM_PMAP_H__ >>> + >>> +#include <asm/fixmap.h> >>> + >>> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) >>> +{ >>> + unsigned long linear = (unsigned long)fix_to_virt(slot); >>> + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; >>> + >>> + ASSERT(!(l1e_get_flags(*pl1e) & _PAGE_PRESENT)); >>> + >>> + l1e_write_atomic(pl1e, l1e_from_mfn(mfn, PAGE_HYPERVISOR)); >>> +} >>> + >>> +static inline void arch_pmap_unmap(unsigned int slot) >>> +{ >>> + unsigned long linear = (unsigned long)fix_to_virt(slot); >>> + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; >>> + >>> + l1e_write_atomic(pl1e, l1e_empty()); >>> + flush_tlb_one_local(linear); >>> +} >> >> You're effectively open-coding {set,clear}_fixmap(), just without >> the L1 table allocation (should such be necessary). If you depend >> on using the build-time L1 table, then you need to move your >> entries ahead of said comment. > > So the problem is less about the allocation be more the fact that we > can't use map_pages_to_xen() because it would call pmap_map(). > > So we need to break the loop. Hence why set_fixmap()/clear_fixmap() are > open-coded. > > And indeed, we would need to rely on the build-time L1 table in this > case. So I will move the entries earlier. Additionally we will now need to (finally) gain a build-time check that all "early" entries actually fit in the static L1 table. XHCI has pushed us quite a bit up here, and I could see us considering to alter (bump) the number of PMAP entries. >> But independent of that you want >> to either use the existing macros / functions, or explain why you >> can't. > > This is explained in the caller of arch_pmap*(): > > /* > * 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(). > */ > > The comment is valid for Arm, x86 and (I would expect in the future) > RISC-V because the page-tables may be allocated in domheap (so not > always mapped). > > So I don't feel this comment should be duplicated in the header. But I > can certainly explain it in the commit message. Right, that's what I was after; I'm sorry for not having worded this precisely enough. Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 6a7825f4ba3c..47b120f18497 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -24,6 +24,7 @@ config X86 select HAS_PCI select HAS_PCI_MSI select HAS_PDX + select HAS_PMAP select HAS_SCHED_GRANULARITY select HAS_UBSAN select HAS_VPCI if HVM diff --git a/xen/arch/x86/include/asm/fixmap.h b/xen/arch/x86/include/asm/fixmap.h index 516ec3fa6c95..38f079873418 100644 --- a/xen/arch/x86/include/asm/fixmap.h +++ b/xen/arch/x86/include/asm/fixmap.h @@ -21,6 +21,8 @@ #include <xen/acpi.h> #include <xen/pfn.h> +#include <xen/pmap.h> + #include <asm/apicdef.h> #include <asm/msi.h> #include <acpi/apei.h> @@ -54,6 +56,8 @@ enum fixed_addresses { FIX_XEN_SHARED_INFO, #endif /* CONFIG_XEN_GUEST */ /* Everything else should go further down. */ + FIX_PMAP_BEGIN, + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, FIX_APIC_BASE, FIX_IO_APIC_BASE_0, FIX_IO_APIC_BASE_END = FIX_IO_APIC_BASE_0 + MAX_IO_APICS-1, diff --git a/xen/arch/x86/include/asm/pmap.h b/xen/arch/x86/include/asm/pmap.h new file mode 100644 index 000000000000..62746e191d03 --- /dev/null +++ b/xen/arch/x86/include/asm/pmap.h @@ -0,0 +1,25 @@ +#ifndef __ASM_PMAP_H__ +#define __ASM_PMAP_H__ + +#include <asm/fixmap.h> + +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn) +{ + unsigned long linear = (unsigned long)fix_to_virt(slot); + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; + + ASSERT(!(l1e_get_flags(*pl1e) & _PAGE_PRESENT)); + + l1e_write_atomic(pl1e, l1e_from_mfn(mfn, PAGE_HYPERVISOR)); +} + +static inline void arch_pmap_unmap(unsigned int slot) +{ + unsigned long linear = (unsigned long)fix_to_virt(slot); + l1_pgentry_t *pl1e = &l1_fixmap[l1_table_offset(linear)]; + + l1e_write_atomic(pl1e, l1e_empty()); + flush_tlb_one_local(linear); +} + +#endif /* __ASM_PMAP_H__ */