| Message ID | 20240513134046.82605-8-eliasely@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove the directmap | expand |
On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote: > From: Julien Grall <jgrall@amazon.com> > > PMAP will be used in a follow-up patch to bootstrap map domain > page infrastructure -- we need some way to map pages to setup the > mapcache without a direct map. > > The functions pmap_{map, unmap} open code {set, clear}_fixmap to break > the loop. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Elias El Yandouzi <eliasely@amazon.com> > > ---- > > The PMAP infrastructure was upstream separately for Arm since > Hongyan sent the secret-free hypervisor series. So this is a new > patch to plumb the feature on x86. > > Changes in v2: > * Declare PMAP entries earlier in fixed_addresses > * Reword the commit message > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index b4ec0e582e..56feb0c564 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -27,6 +27,7 @@ config X86 > select HAS_PCI > select HAS_PCI_MSI > select HAS_PIRQ > + select HAS_PMAP Shouldn't it be selected by HAS_SECRET_HIDING rather than being unconditionally selected? > select HAS_SCHED_GRANULARITY > select HAS_SECRET_HIDING > select HAS_UBSAN > diff --git a/xen/arch/x86/include/asm/fixmap.h b/xen/arch/x86/include/asm/fixmap.h > index 516ec3fa6c..a7ac365fc6 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> > @@ -53,6 +55,8 @@ enum fixed_addresses { > FIX_PV_CONSOLE, > FIX_XEN_SHARED_INFO, > #endif /* CONFIG_XEN_GUEST */ > + FIX_PMAP_BEGIN, > + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, This would better have #ifdef CONFIG_HAS_PMAP guards? > /* Everything else should go further down. */ > FIX_APIC_BASE, > FIX_IO_APIC_BASE_0, > diff --git a/xen/arch/x86/include/asm/pmap.h b/xen/arch/x86/include/asm/pmap.h > new file mode 100644 > index 0000000000..62746e191d > --- /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__ */ We usually add a: /* * Local variables: * mode: C * c-file-style: "BSD" * c-basic-offset: 4 * indent-tabs-mode: nil * End: */ Footer. No strict requirement, but it's nice so that your editor already picks the correct defaults for tabs &c. Thanks, Roger.
On 14.05.2024 11:40, Roger Pau Monné wrote: > On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote: >> @@ -53,6 +55,8 @@ enum fixed_addresses { >> FIX_PV_CONSOLE, >> FIX_XEN_SHARED_INFO, >> #endif /* CONFIG_XEN_GUEST */ >> + FIX_PMAP_BEGIN, >> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, > > This would better have > > #ifdef CONFIG_HAS_PMAP > > guards? That's useful only when the option can actually be off in certain configurations, isn't it? Jan
On Tue, May 14, 2024 at 11:43:14AM +0200, Jan Beulich wrote: > On 14.05.2024 11:40, Roger Pau Monné wrote: > > On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote: > >> @@ -53,6 +55,8 @@ enum fixed_addresses { > >> FIX_PV_CONSOLE, > >> FIX_XEN_SHARED_INFO, > >> #endif /* CONFIG_XEN_GUEST */ > >> + FIX_PMAP_BEGIN, > >> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, > > > > This would better have > > > > #ifdef CONFIG_HAS_PMAP > > > > guards? > > That's useful only when the option can actually be off in certain > configurations, isn't it? My comment earlier on this patch suggested to make CONFIG_HAS_PMAP be selected by HAS_SECRET_HIDING, rather than being unconditionally arch-selected (if that's possible, I certainly don't know the usage in further patches). Regards, Roger.
On 14.05.2024 12:22, Roger Pau Monné wrote: > On Tue, May 14, 2024 at 11:43:14AM +0200, Jan Beulich wrote: >> On 14.05.2024 11:40, Roger Pau Monné wrote: >>> On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote: >>>> @@ -53,6 +55,8 @@ enum fixed_addresses { >>>> FIX_PV_CONSOLE, >>>> FIX_XEN_SHARED_INFO, >>>> #endif /* CONFIG_XEN_GUEST */ >>>> + FIX_PMAP_BEGIN, >>>> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, >>> >>> This would better have >>> >>> #ifdef CONFIG_HAS_PMAP >>> >>> guards? >> >> That's useful only when the option can actually be off in certain >> configurations, isn't it? > > My comment earlier on this patch suggested to make CONFIG_HAS_PMAP be > selected by HAS_SECRET_HIDING, rather than being unconditionally > arch-selected (if that's possible, I certainly don't know the usage in > further patches). Right, but in patch 6 HAS_SECRET_HIDING is selected unconditionally, which would then also select HAS_PMAP. If, otoh, HAS_PMAP was selected only when SECRET_HIDING (or whatever its name is going to be), then an #ifdef would indeed be wanted here. Jan
On Tue, May 14, 2024 at 12:26:29PM +0200, Jan Beulich wrote: > On 14.05.2024 12:22, Roger Pau Monné wrote: > > On Tue, May 14, 2024 at 11:43:14AM +0200, Jan Beulich wrote: > >> On 14.05.2024 11:40, Roger Pau Monné wrote: > >>> On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote: > >>>> @@ -53,6 +55,8 @@ enum fixed_addresses { > >>>> FIX_PV_CONSOLE, > >>>> FIX_XEN_SHARED_INFO, > >>>> #endif /* CONFIG_XEN_GUEST */ > >>>> + FIX_PMAP_BEGIN, > >>>> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, > >>> > >>> This would better have > >>> > >>> #ifdef CONFIG_HAS_PMAP > >>> > >>> guards? > >> > >> That's useful only when the option can actually be off in certain > >> configurations, isn't it? > > > > My comment earlier on this patch suggested to make CONFIG_HAS_PMAP be > > selected by HAS_SECRET_HIDING, rather than being unconditionally > > arch-selected (if that's possible, I certainly don't know the usage in > > further patches). > > Right, but in patch 6 HAS_SECRET_HIDING is selected unconditionally, > which would then also select HAS_PMAP. If, otoh, HAS_PMAP was selected > only when SECRET_HIDING (or whatever its name is going to be), then an > #ifdef would indeed be wanted here. Oh, indeed, I was meant to tie to SECRET_HIDING and not HAS_SECRET_HIDING. I have to admit (as I've already commented on the patch) I don't much like those names, they are far too generic. Thanks, Roger.
On 14.05.2024 13:51, Roger Pau Monné wrote: > On Tue, May 14, 2024 at 12:26:29PM +0200, Jan Beulich wrote: >> On 14.05.2024 12:22, Roger Pau Monné wrote: >>> On Tue, May 14, 2024 at 11:43:14AM +0200, Jan Beulich wrote: >>>> On 14.05.2024 11:40, Roger Pau Monné wrote: >>>>> On Mon, May 13, 2024 at 01:40:34PM +0000, Elias El Yandouzi wrote: >>>>>> @@ -53,6 +55,8 @@ enum fixed_addresses { >>>>>> FIX_PV_CONSOLE, >>>>>> FIX_XEN_SHARED_INFO, >>>>>> #endif /* CONFIG_XEN_GUEST */ >>>>>> + FIX_PMAP_BEGIN, >>>>>> + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, >>>>> >>>>> This would better have >>>>> >>>>> #ifdef CONFIG_HAS_PMAP >>>>> >>>>> guards? >>>> >>>> That's useful only when the option can actually be off in certain >>>> configurations, isn't it? >>> >>> My comment earlier on this patch suggested to make CONFIG_HAS_PMAP be >>> selected by HAS_SECRET_HIDING, rather than being unconditionally >>> arch-selected (if that's possible, I certainly don't know the usage in >>> further patches). >> >> Right, but in patch 6 HAS_SECRET_HIDING is selected unconditionally, >> which would then also select HAS_PMAP. If, otoh, HAS_PMAP was selected >> only when SECRET_HIDING (or whatever its name is going to be), then an >> #ifdef would indeed be wanted here. > > Oh, indeed, I was meant to tie to SECRET_HIDING and not > HAS_SECRET_HIDING. I have to admit (as I've already commented on the > patch) I don't much like those names, they are far too generic. And I commented to this same effect on v2 already, without being heard. Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index b4ec0e582e..56feb0c564 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -27,6 +27,7 @@ config X86 select HAS_PCI select HAS_PCI_MSI select HAS_PIRQ + select HAS_PMAP select HAS_SCHED_GRANULARITY select HAS_SECRET_HIDING select HAS_UBSAN diff --git a/xen/arch/x86/include/asm/fixmap.h b/xen/arch/x86/include/asm/fixmap.h index 516ec3fa6c..a7ac365fc6 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> @@ -53,6 +55,8 @@ enum fixed_addresses { FIX_PV_CONSOLE, FIX_XEN_SHARED_INFO, #endif /* CONFIG_XEN_GUEST */ + FIX_PMAP_BEGIN, + FIX_PMAP_END = FIX_PMAP_BEGIN + NUM_FIX_PMAP, /* Everything else should go further down. */ FIX_APIC_BASE, FIX_IO_APIC_BASE_0, diff --git a/xen/arch/x86/include/asm/pmap.h b/xen/arch/x86/include/asm/pmap.h new file mode 100644 index 0000000000..62746e191d --- /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__ */