| Message ID | 7360b59e8fd39796fee56430a437b20c948d08c2.1588278317.git.hongyxia@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove the direct map | expand |
Hi Hongyan, On 30/04/2020 21:44, Hongyan Xia wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > Also add a helper function to retrieve it. Change arch_mfn_in_direct_map > to check this option before returning. > > This is added as a boot command line option, not a Kconfig. We do not > produce different builds for EC2 so this is not introduced as a > compile-time configuration. > > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > --- > docs/misc/xen-command-line.pandoc | 12 ++++++++++++ > xen/arch/x86/mm.c | 3 +++ > xen/arch/x86/setup.c | 2 ++ > xen/include/asm-arm/mm.h | 5 +++++ > xen/include/asm-x86/mm.h | 17 ++++++++++++++++- > 5 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc > index ee12b0f53f..7027e3a15c 100644 > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -652,6 +652,18 @@ Specify the size of the console debug trace buffer. By specifying `cpu:` > additionally a trace buffer of the specified size is allocated per cpu. > The debug trace feature is only enabled in debugging builds of Xen. > > +### directmap (x86) > +> `= <boolean>` > + > +> Default: `true` > + > +Enable or disable the direct map region in Xen. > + > +By default, Xen creates the direct map region which maps physical memory > +in that region. Setting this to no will remove the direct map, blocking > +exploits that leak secrets via speculative memory access in the direct > +map. > + > ### dma_bits > > `= <integer>` > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index b3530d2763..64da997764 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -162,6 +162,9 @@ l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) > l1_fixmap_x[L1_PAGETABLE_ENTRIES]; > > +bool __read_mostly opt_directmap = true; > +boolean_param("directmap", opt_directmap); > + > paddr_t __read_mostly mem_hotplug; > > /* Frame table size in pages. */ > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index faca8c9758..60fc4038be 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1282,6 +1282,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > if ( highmem_start ) > xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); > > + printk("Booting with directmap %s\n", arch_has_directmap() ? "on" : "off"); > + > /* > * Walk every RAM region and map it in its entirety (on x86/64, at least) > * and notify it to the boot allocator. > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index 7df91280bc..e6fd934113 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -366,6 +366,11 @@ int arch_acquire_resource(struct domain *d, unsigned int type, unsigned int id, > return -EOPNOTSUPP; > } > > +static inline bool arch_has_directmap(void) > +{ > + return true; arm32 doesn't have a directmap, so this needs to be false for arm32 and true for arm64. I would also like the implementation of the helper close to arch_mfn_in_directmap() in asm-arm/arm*/mm.h. Cheers,
On Thu, Apr 30, 2020 at 09:44:20PM +0100, Hongyan Xia wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > Also add a helper function to retrieve it. Change arch_mfn_in_direct_map > to check this option before returning. > > This is added as a boot command line option, not a Kconfig. We do not > produce different builds for EC2 so this is not introduced as a > compile-time configuration. Having a Kconfig will probably allow the compiler to eliminate dead code. This is not asking you to do the work, someone can come along and adjust arch_has_directmap easily. Wei.
On Fri, 2020-05-01 at 12:11 +0000, Wei Liu wrote: > On Thu, Apr 30, 2020 at 09:44:20PM +0100, Hongyan Xia wrote: > > From: Hongyan Xia <hongyxia@amazon.com> > > > > Also add a helper function to retrieve it. Change > > arch_mfn_in_direct_map > > to check this option before returning. > > > > This is added as a boot command line option, not a Kconfig. We do > > not > > produce different builds for EC2 so this is not introduced as a > > compile-time configuration. > > Having a Kconfig will probably allow the compiler to eliminate dead > code. > > This is not asking you to do the work, someone can come along and > adjust > arch_has_directmap easily. My original code added this as a CONFIG option, but I converted it into a boot-time switch, so I can just dig out history and convert it back. I wonder if we should get more opinions on this to make a decision. I would love Xen to have static key support though so that a boot-time switch costs no run-time performance. Hongyan
On Fri, May 01, 2020 at 01:59:24PM +0100, Hongyan Xia wrote: > On Fri, 2020-05-01 at 12:11 +0000, Wei Liu wrote: > > On Thu, Apr 30, 2020 at 09:44:20PM +0100, Hongyan Xia wrote: > > > From: Hongyan Xia <hongyxia@amazon.com> > > > > > > Also add a helper function to retrieve it. Change > > > arch_mfn_in_direct_map > > > to check this option before returning. > > > > > > This is added as a boot command line option, not a Kconfig. We do > > > not > > > produce different builds for EC2 so this is not introduced as a > > > compile-time configuration. > > > > Having a Kconfig will probably allow the compiler to eliminate dead > > code. > > > > This is not asking you to do the work, someone can come along and > > adjust > > arch_has_directmap easily. > > My original code added this as a CONFIG option, but I converted it into > a boot-time switch, so I can just dig out history and convert it back. > I wonder if we should get more opinions on this to make a decision. Form my perspective, you as a contributor has done the work to scratch your own itch, hence I said "not asking you to do the work". I don't want to turn every comment into a formal ask and eventually lead to feature creep. > > I would love Xen to have static key support though so that a boot-time > switch costs no run-time performance. > Yes that would be great. Wei. > Hongyan >
Hi, On 01/05/2020 14:11, Wei Liu wrote: > On Fri, May 01, 2020 at 01:59:24PM +0100, Hongyan Xia wrote: >> On Fri, 2020-05-01 at 12:11 +0000, Wei Liu wrote: >>> On Thu, Apr 30, 2020 at 09:44:20PM +0100, Hongyan Xia wrote: >>>> From: Hongyan Xia <hongyxia@amazon.com> >>>> >>>> Also add a helper function to retrieve it. Change >>>> arch_mfn_in_direct_map >>>> to check this option before returning. >>>> >>>> This is added as a boot command line option, not a Kconfig. We do >>>> not >>>> produce different builds for EC2 so this is not introduced as a >>>> compile-time configuration. >>> >>> Having a Kconfig will probably allow the compiler to eliminate dead >>> code. >>> >>> This is not asking you to do the work, someone can come along and >>> adjust >>> arch_has_directmap easily. >> >> My original code added this as a CONFIG option, but I converted it into >> a boot-time switch, so I can just dig out history and convert it back. >> I wonder if we should get more opinions on this to make a decision. > > Form my perspective, you as a contributor has done the work to scratch > your own itch, hence I said "not asking you to do the work". I don't > want to turn every comment into a formal ask and eventually lead to > feature creep. > >> >> I would love Xen to have static key support though so that a boot-time >> switch costs no run-time performance. >> > > Yes that would be great. From my understanding static key is very powerful as you could modify the value even at runtime. On Arm, I wrote a version that I would call static key for the poor. We are using alternative to select between 0 and 1 as an immediate value. #define CHECK_WORKAROUND_HELPER(erratum, feature, arch) \ static inline bool check_workaround_##erratum(void) \ { \ if ( !IS_ENABLED(arch) ) \ return false; \ else \ { \ register_t ret; \ \ asm volatile (ALTERNATIVE("mov %0, #0", \ "mov %0, #1", \ feature) \ : "=r" (ret)); \ \ return unlikely(ret); \ } \ } The code generated will still be using a conditional branch, but the processor should be able to always infer correctly whether the condition is true or not. The implementation is also very tailored to Arm as we consider workaround are not enabled by default. But I think this can be improved and made more generic. Cheers,
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index ee12b0f53f..7027e3a15c 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -652,6 +652,18 @@ Specify the size of the console debug trace buffer. By specifying `cpu:` additionally a trace buffer of the specified size is allocated per cpu. The debug trace feature is only enabled in debugging builds of Xen. +### directmap (x86) +> `= <boolean>` + +> Default: `true` + +Enable or disable the direct map region in Xen. + +By default, Xen creates the direct map region which maps physical memory +in that region. Setting this to no will remove the direct map, blocking +exploits that leak secrets via speculative memory access in the direct +map. + ### dma_bits > `= <integer>` diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index b3530d2763..64da997764 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -162,6 +162,9 @@ l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) l1_fixmap_x[L1_PAGETABLE_ENTRIES]; +bool __read_mostly opt_directmap = true; +boolean_param("directmap", opt_directmap); + paddr_t __read_mostly mem_hotplug; /* Frame table size in pages. */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index faca8c9758..60fc4038be 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1282,6 +1282,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) if ( highmem_start ) xenheap_max_mfn(PFN_DOWN(highmem_start - 1)); + printk("Booting with directmap %s\n", arch_has_directmap() ? "on" : "off"); + /* * Walk every RAM region and map it in its entirety (on x86/64, at least) * and notify it to the boot allocator. diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index 7df91280bc..e6fd934113 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -366,6 +366,11 @@ int arch_acquire_resource(struct domain *d, unsigned int type, unsigned int id, return -EOPNOTSUPP; } +static inline bool arch_has_directmap(void) +{ + return true; +} + #endif /* __ARCH_ARM_MM__ */ /* * Local variables: diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index ef7a20ac7d..7ff99ee8e3 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -454,6 +454,8 @@ int check_descriptor(const struct domain *d, seg_desc_t *desc); extern paddr_t mem_hotplug; +extern bool opt_directmap; + /****************************************************************************** * With shadow pagetables, the different kinds of address start * to get get confusing. @@ -637,13 +639,26 @@ extern const char zero_page[]; /* Build a 32bit PSE page table using 4MB pages. */ void write_32bit_pse_identmap(uint32_t *l2); +static inline bool arch_has_directmap(void) +{ + return opt_directmap; +} + /* * x86 maps part of physical memory via the directmap region. * Return whether the input MFN falls in that range. + * + * When boot command line sets directmap=no, we will not have a direct map at + * all so this will always return false. */ static inline bool arch_mfn_in_directmap(unsigned long mfn) { - unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); + unsigned long eva; + + if ( !arch_has_directmap() ) + return false; + + eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END); return mfn <= (virt_to_mfn(eva - 1) + 1); }