| Message ID | 20200122202343.5703-2-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More Hyper-V infrastructure | expand |
On 22/01/2020 20:23, Wei Liu wrote: > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S > index 1cbf5acdfb..605d01f1dd 100644 > --- a/xen/arch/x86/boot/x86_64.S > +++ b/xen/arch/x86/boot/x86_64.S > @@ -85,7 +85,15 @@ GLOBAL(l2_directmap) > * 4k page. > */ Adjust this comment as well? > diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h > index d0cfbb70a8..4fa56ea0a9 100644 > --- a/xen/include/asm-x86/config.h > +++ b/xen/include/asm-x86/config.h > @@ -218,7 +218,7 @@ extern unsigned char boot_edid_info[128]; > /* Slot 261: high read-only compat machine-to-phys conversion table (1GB). */ > #define HIRO_COMPAT_MPT_VIRT_START RDWR_COMPAT_MPT_VIRT_END > #define HIRO_COMPAT_MPT_VIRT_END (HIRO_COMPAT_MPT_VIRT_START + GB(1)) > -/* Slot 261: xen text, static data and bss (1GB). */ > +/* Slot 261: xen text, static data, bss and executable fixmap (1GB). */ And per-cpu stubs. Might as well fix the comment while editing. > #define XEN_VIRT_START (HIRO_COMPAT_MPT_VIRT_END) > #define XEN_VIRT_END (XEN_VIRT_START + GB(1)) > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > index 9fb2f47946..c2a9d2b50a 100644 > --- a/xen/include/asm-x86/fixmap.h > +++ b/xen/include/asm-x86/fixmap.h > @@ -15,6 +15,9 @@ > #include <asm/page.h> > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > +/* This constant is derived from enum fixed_addresses_x below */ > +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) Answering slightly out of order, for clarity: FIXADDR_X_SIZE should be 0 or 1 by the end of this patch. As for MAX_FIXADDR_X_SIZE, how about simply IS_ENABLED(CONFIG_HYPERV_GUEST) ? That should work, even in a linker script. Somewhere, there should be a BUILD_BUG_ON() cross-checking MAX_FIXADDR_X_SIZE and __end_of_fixed_addresses_x. We don't yet have a build_assertions() in x86/mm.c, so I guess now is the time to gain one. > > #ifndef __ASSEMBLY__ > > @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) > return __virt_to_fix(vaddr); > } > > +enum fixed_addresses_x { > + /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */ > + FIX_X_RESERVED, > +#ifdef CONFIG_HYPERV_GUEST > + FIX_X_HYPERV_HCALL, > +#endif > + __end_of_fixed_addresses_x > +}; > + > +#define FIXADDR_X_SIZE (__end_of_fixed_addresses_x << PAGE_SHIFT) -1, seeing as 0 is reserved. > +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE) > + > +extern void __set_fixmap_x( > + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags); > + > +#define set_fixmap_x(idx, phys) \ > + __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) > + > +#define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > + > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > +#define __virt_to_fix_x(x) ((FIXADDR_X_TOP - ((x)&PAGE_MASK)) >> PAGE_SHIFT) The &PAGE_MASK is redundant, given the following shift, but can't be optimised out because of its effect on the high 12 bits of the address as well. These helpers aren't safe to wild inputs, even with the &PAGE_MASK, so I'd just drop it. Otherwise, LGTM. There is some cleanup we ought to do to the fixmap infrastructure, but that isn't appropriate for this series. ~Andrew
On 22.01.2020 21:23, Wei Liu wrote: > This allows us to set aside some address space for executable mapping. > This fixed map range starts from XEN_VIRT_END so that it is within reach > of the .text section. > > Shift the percpu stub range and livepatch range accordingly. Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there a particular reason why you move the stubs area down? It looks as if the patch would be smaller overall if you didn't. (Possibly down the road the stubs area could be made part of the FIXADDR_X range anyway.) > @@ -5763,6 +5765,13 @@ void __set_fixmap( > map_pages_to_xen(__fix_to_virt(idx), _mfn(mfn), 1, flags); > } > > +void __set_fixmap_x( > + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags) > +{ > + BUG_ON(idx >= __end_of_fixed_addresses_x); Also check for FIX_X_RESERVED? > --- a/xen/include/asm-x86/fixmap.h > +++ b/xen/include/asm-x86/fixmap.h > @@ -15,6 +15,9 @@ > #include <asm/page.h> > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > +/* This constant is derived from enum fixed_addresses_x below */ > +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) If this can't be properly derived, then a BUILD_BUG_ON() is needed. But didn't we discuss on irc already possible approaches of how to derive it from the enum? Did none of this work? > @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) > return __virt_to_fix(vaddr); > } > > +enum fixed_addresses_x { > + /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */ > + FIX_X_RESERVED, > +#ifdef CONFIG_HYPERV_GUEST > + FIX_X_HYPERV_HCALL, > +#endif > + __end_of_fixed_addresses_x > +}; > + > +#define FIXADDR_X_SIZE (__end_of_fixed_addresses_x << PAGE_SHIFT) > +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE) > + > +extern void __set_fixmap_x( > + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags); > + > +#define set_fixmap_x(idx, phys) \ > + __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) Can't __set_fixmap() be used here, making its implementation derive which one is mean from whether _PAGE_NX is set in the passed in flags? Jan
On Wed, Jan 22, 2020 at 08:56:55PM +0000, Andrew Cooper wrote: > On 22/01/2020 20:23, Wei Liu wrote: > > diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S > > index 1cbf5acdfb..605d01f1dd 100644 > > --- a/xen/arch/x86/boot/x86_64.S > > +++ b/xen/arch/x86/boot/x86_64.S > > @@ -85,7 +85,15 @@ GLOBAL(l2_directmap) > > * 4k page. > > */ > > Adjust this comment as well? I thought it was still accurate, so I didn't touch it. Now it reads: /* * L2 mapping the Xen text/data/bss region, constructed dynamically. * Executable fixmap region is hooked up statically. * Uses 1x * 4k page. */ Does this sound good to you? > > > diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h > > index d0cfbb70a8..4fa56ea0a9 100644 > > --- a/xen/include/asm-x86/config.h > > +++ b/xen/include/asm-x86/config.h > > @@ -218,7 +218,7 @@ extern unsigned char boot_edid_info[128]; > > /* Slot 261: high read-only compat machine-to-phys conversion table (1GB). */ > > #define HIRO_COMPAT_MPT_VIRT_START RDWR_COMPAT_MPT_VIRT_END > > #define HIRO_COMPAT_MPT_VIRT_END (HIRO_COMPAT_MPT_VIRT_START + GB(1)) > > -/* Slot 261: xen text, static data and bss (1GB). */ > > +/* Slot 261: xen text, static data, bss and executable fixmap (1GB). */ > > And per-cpu stubs. Might as well fix the comment while editing. Ack. > > > #define XEN_VIRT_START (HIRO_COMPAT_MPT_VIRT_END) > > #define XEN_VIRT_END (XEN_VIRT_START + GB(1)) > > > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h > > index 9fb2f47946..c2a9d2b50a 100644 > > --- a/xen/include/asm-x86/fixmap.h > > +++ b/xen/include/asm-x86/fixmap.h > > @@ -15,6 +15,9 @@ > > #include <asm/page.h> > > > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > > +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > > +/* This constant is derived from enum fixed_addresses_x below */ > > +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) > > Answering slightly out of order, for clarity: > > FIXADDR_X_SIZE should be 0 or 1 by the end of this patch. > > As for MAX_FIXADDR_X_SIZE, how about simply > IS_ENABLED(CONFIG_HYPERV_GUEST) ? That should work, even in a linker > script. > It should be at least 1<<PAGE_SHIFT because __end_of_fixed_addresses_x is at least 1. So for now it can be #define MAX_FIXADDR_X_SIZE ((IS_ENABLED(CONFIG_HYPERV_GUEST) + 1) << PAGE_SHIFT) > Somewhere, there should be a BUILD_BUG_ON() cross-checking > MAX_FIXADDR_X_SIZE and __end_of_fixed_addresses_x. We don't yet have a > build_assertions() in x86/mm.c, so I guess now is the time to gain one. No, the build_assertions shouldn't be necessary. MAX_FIXADDR_X_SIZE is intentionally set to the maximum possible value, while __end_of_fixed_addresses_x is subject to change according to configuration. They can differ. Unless you're talking about adding CONFIG_HYPERV_GUEST to MAX_FIXADDR_X_SIZE like I mentioned above? > > > > > #ifndef __ASSEMBLY__ > > > > @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) > > return __virt_to_fix(vaddr); > > } > > > > +enum fixed_addresses_x { > > + /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */ > > + FIX_X_RESERVED, > > +#ifdef CONFIG_HYPERV_GUEST > > + FIX_X_HYPERV_HCALL, > > +#endif > > + __end_of_fixed_addresses_x > > +}; > > + > > +#define FIXADDR_X_SIZE (__end_of_fixed_addresses_x << PAGE_SHIFT) > > -1, seeing as 0 is reserved. > What does -1 mean here? > > +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE) > > + > > +extern void __set_fixmap_x( > > + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags); > > + > > +#define set_fixmap_x(idx, phys) \ > > + __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) > > + > > +#define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) > > + > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) > > +#define __virt_to_fix_x(x) ((FIXADDR_X_TOP - ((x)&PAGE_MASK)) >> PAGE_SHIFT) > > The &PAGE_MASK is redundant, given the following shift, but can't be > optimised out because of its effect on the high 12 bits of the address > as well. These helpers aren't safe to wild inputs, even with the > &PAGE_MASK, so I'd just drop it. > OK. I will drop it. Wei.
On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote: > On 22.01.2020 21:23, Wei Liu wrote: > > This allows us to set aside some address space for executable mapping. > > This fixed map range starts from XEN_VIRT_END so that it is within reach > > of the .text section. > > > > Shift the percpu stub range and livepatch range accordingly. > > Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there > a particular reason why you move the stubs area down? It looks as if > the patch would be smaller overall if you didn't. (Possibly down > the road the stubs area could be made part of the FIXADDR_X range > anyway.) I think having a well-known fixed address is more useful for debugging. Going the other way around would mean the hypercall page location becomes dependent on the number of CPUs configured. > > > @@ -5763,6 +5765,13 @@ void __set_fixmap( > > map_pages_to_xen(__fix_to_virt(idx), _mfn(mfn), 1, flags); > > } > > > > +void __set_fixmap_x( > > + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags) > > +{ > > + BUG_ON(idx >= __end_of_fixed_addresses_x); > > Also check for FIX_X_RESERVED? Ack. In that case the same should be done for __set_fixmap. > > > --- a/xen/include/asm-x86/fixmap.h > > +++ b/xen/include/asm-x86/fixmap.h > > @@ -15,6 +15,9 @@ > > #include <asm/page.h> > > > > #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > > +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > > +/* This constant is derived from enum fixed_addresses_x below */ > > +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) > > If this can't be properly derived, then a BUILD_BUG_ON() is needed. > But didn't we discuss on irc already possible approaches of how to > derive it from the enum? Did none of this work? > The only option I remember discussing was to define macros instead of using enum. I said at the time at would make us lose the ability to dynamically size this area. If there are other ways that I missed, let me know. > > @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) > > return __virt_to_fix(vaddr); > > } > > > > +enum fixed_addresses_x { > > + /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */ > > + FIX_X_RESERVED, > > +#ifdef CONFIG_HYPERV_GUEST > > + FIX_X_HYPERV_HCALL, > > +#endif > > + __end_of_fixed_addresses_x > > +}; > > + > > +#define FIXADDR_X_SIZE (__end_of_fixed_addresses_x << PAGE_SHIFT) > > +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE) > > + > > +extern void __set_fixmap_x( > > + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags); > > + > > +#define set_fixmap_x(idx, phys) \ > > + __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) > > Can't __set_fixmap() be used here, making its implementation derive > which one is mean from whether _PAGE_NX is set in the passed in flags? __set_fixmap and __set_fixmap_x take different enum types for their first argument. I would prefer type safety and explicitness here. Wei. > > Jan
On 28.01.2020 16:15, Wei Liu wrote: > On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote: >> On 22.01.2020 21:23, Wei Liu wrote: >>> This allows us to set aside some address space for executable mapping. >>> This fixed map range starts from XEN_VIRT_END so that it is within reach >>> of the .text section. >>> >>> Shift the percpu stub range and livepatch range accordingly. >> >> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there >> a particular reason why you move the stubs area down? It looks as if >> the patch would be smaller overall if you didn't. (Possibly down >> the road the stubs area could be made part of the FIXADDR_X range >> anyway.) > > I think having a well-known fixed address is more useful for debugging. > > Going the other way around would mean the hypercall page location > becomes dependent on the number of CPUs configured. Depending on how future insertions are done into enum fixed_addresses_x, the address also won't be "well-known fixed". >>> --- a/xen/include/asm-x86/fixmap.h >>> +++ b/xen/include/asm-x86/fixmap.h >>> @@ -15,6 +15,9 @@ >>> #include <asm/page.h> >>> >>> #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) >>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) >>> +/* This constant is derived from enum fixed_addresses_x below */ >>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) >> >> If this can't be properly derived, then a BUILD_BUG_ON() is needed. >> But didn't we discuss on irc already possible approaches of how to >> derive it from the enum? Did none of this work? > > The only option I remember discussing was to define macros instead of > using enum. I said at the time at would make us lose the ability to > dynamically size this area. > > If there are other ways that I missed, let me know. I seem to recall recommending to export absolute symbols from assembly code. The question is how easily usable they would be from C, or how clumsy the resulting code would look. >>> @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) >>> return __virt_to_fix(vaddr); >>> } >>> >>> +enum fixed_addresses_x { >>> + /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */ >>> + FIX_X_RESERVED, >>> +#ifdef CONFIG_HYPERV_GUEST >>> + FIX_X_HYPERV_HCALL, >>> +#endif >>> + __end_of_fixed_addresses_x >>> +}; >>> + >>> +#define FIXADDR_X_SIZE (__end_of_fixed_addresses_x << PAGE_SHIFT) >>> +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE) >>> + >>> +extern void __set_fixmap_x( >>> + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags); >>> + >>> +#define set_fixmap_x(idx, phys) \ >>> + __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) >> >> Can't __set_fixmap() be used here, making its implementation derive >> which one is mean from whether _PAGE_NX is set in the passed in flags? > > __set_fixmap and __set_fixmap_x take different enum types for their > first argument. I would prefer type safety and explicitness here. Well, okay then. Duplication like this simply makes me a little nervous, and even more so when it extends our set of name space violations. Jan
On Tue, Jan 28, 2020 at 04:38:42PM +0100, Jan Beulich wrote: > On 28.01.2020 16:15, Wei Liu wrote: > > On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote: > >> On 22.01.2020 21:23, Wei Liu wrote: > >>> This allows us to set aside some address space for executable mapping. > >>> This fixed map range starts from XEN_VIRT_END so that it is within reach > >>> of the .text section. > >>> > >>> Shift the percpu stub range and livepatch range accordingly. > >> > >> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there > >> a particular reason why you move the stubs area down? It looks as if > >> the patch would be smaller overall if you didn't. (Possibly down > >> the road the stubs area could be made part of the FIXADDR_X range > >> anyway.) > > > > I think having a well-known fixed address is more useful for debugging. > > > > Going the other way around would mean the hypercall page location > > becomes dependent on the number of CPUs configured. > > Depending on how future insertions are done into > enum fixed_addresses_x, the address also won't be "well-known fixed". Going back to this, not moving stubs will make the change to alloc_stub_page become unnecessary (one line); on the other hand it makes FIX_X_ADDR_START become XEN_VIRT_END - NR_CPUS * PAGE_SIZE - PAGE_SIZE. Are you really concerned about this? I can make the change if you really want that, but it is just work with no apparent benefit. > > >>> --- a/xen/include/asm-x86/fixmap.h > >>> +++ b/xen/include/asm-x86/fixmap.h > >>> @@ -15,6 +15,9 @@ > >>> #include <asm/page.h> > >>> > >>> #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) > >>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) > >>> +/* This constant is derived from enum fixed_addresses_x below */ > >>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) > >> > >> If this can't be properly derived, then a BUILD_BUG_ON() is needed. > >> But didn't we discuss on irc already possible approaches of how to > >> derive it from the enum? Did none of this work? > > > > The only option I remember discussing was to define macros instead of > > using enum. I said at the time at would make us lose the ability to > > dynamically size this area. > > > > If there are other ways that I missed, let me know. > > I seem to recall recommending to export absolute symbols from > assembly code. The question is how easily usable they would > be from C, or how clumsy the resulting code would look. Even if I use absolute symbol I would still need to define a macro for it. There is no way around it, because enum can't be used in asm or linker script. I want to keep using enum because that would allow us to size the area according to Kconfig. Wei.
On 29.01.2020 15:42, Wei Liu wrote: > On Tue, Jan 28, 2020 at 04:38:42PM +0100, Jan Beulich wrote: >> On 28.01.2020 16:15, Wei Liu wrote: >>> On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote: >>>> On 22.01.2020 21:23, Wei Liu wrote: >>>>> This allows us to set aside some address space for executable mapping. >>>>> This fixed map range starts from XEN_VIRT_END so that it is within reach >>>>> of the .text section. >>>>> >>>>> Shift the percpu stub range and livepatch range accordingly. >>>> >>>> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there >>>> a particular reason why you move the stubs area down? It looks as if >>>> the patch would be smaller overall if you didn't. (Possibly down >>>> the road the stubs area could be made part of the FIXADDR_X range >>>> anyway.) >>> >>> I think having a well-known fixed address is more useful for debugging. >>> >>> Going the other way around would mean the hypercall page location >>> becomes dependent on the number of CPUs configured. >> >> Depending on how future insertions are done into >> enum fixed_addresses_x, the address also won't be "well-known fixed". > > Going back to this, not moving stubs will make the change to > alloc_stub_page become unnecessary (one line); on the other hand it > makes FIX_X_ADDR_START become XEN_VIRT_END - NR_CPUS * PAGE_SIZE - > PAGE_SIZE. > > Are you really concerned about this? I can make the change if you really > want that, but it is just work with no apparent benefit. Hmm, indeed, it's just one line. Not sure why I thought there would be more of an effect. Leave it as is, and sorry for the noise. >>>>> --- a/xen/include/asm-x86/fixmap.h >>>>> +++ b/xen/include/asm-x86/fixmap.h >>>>> @@ -15,6 +15,9 @@ >>>>> #include <asm/page.h> >>>>> >>>>> #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) >>>>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) >>>>> +/* This constant is derived from enum fixed_addresses_x below */ >>>>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) >>>> >>>> If this can't be properly derived, then a BUILD_BUG_ON() is needed. >>>> But didn't we discuss on irc already possible approaches of how to >>>> derive it from the enum? Did none of this work? >>> >>> The only option I remember discussing was to define macros instead of >>> using enum. I said at the time at would make us lose the ability to >>> dynamically size this area. >>> >>> If there are other ways that I missed, let me know. >> >> I seem to recall recommending to export absolute symbols from >> assembly code. The question is how easily usable they would >> be from C, or how clumsy the resulting code would look. > > Even if I use absolute symbol I would still need to define a macro for > it. There is no way around it, because enum can't be used in asm or > linker script. I'm afraid I don't understand. Why a macro? The absolute symbol would be there to communicate the relevant (enum-derived) value to the linker script. I.e. with enum { e0, e1, e2 }; in some C file asm ( ".equ GBL_e2, %c0; .global GBL_e2" :: "i" (e2) ); which I then hope would allow you to use GBL_e2 in the linker script ASSERT(). > I want to keep using enum because that would allow us to size the area > according to Kconfig. Of course, I fully agree with this goal. Jan
On Wed, Jan 29, 2020 at 03:59:32PM +0100, Jan Beulich wrote: [...] > >> I seem to recall recommending to export absolute symbols from > >> assembly code. The question is how easily usable they would > >> be from C, or how clumsy the resulting code would look. > > > > Even if I use absolute symbol I would still need to define a macro for > > it. There is no way around it, because enum can't be used in asm or > > linker script. > > I'm afraid I don't understand. Why a macro? The absolute symbol would > be there to communicate the relevant (enum-derived) value to the > linker script. I.e. with > > enum { e0, e1, e2 }; > > in some C file > > asm ( ".equ GBL_e2, %c0; .global GBL_e2" :: "i" (e2) ); > > which I then hope would allow you to use GBL_e2 in the linker > script ASSERT(). > OK. Let me see if this is possible. Wei.
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index 1cbf5acdfb..605d01f1dd 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -85,7 +85,15 @@ GLOBAL(l2_directmap) * 4k page. */ GLOBAL(l2_xenmap) - .fill L2_PAGETABLE_ENTRIES, 8, 0 + idx = 0 + .rept L2_PAGETABLE_ENTRIES + .if idx == l2_table_offset(FIXADDR_X_TOP - 1) + .quad sym_offs(l1_fixmap_x) + __PAGE_HYPERVISOR + .else + .quad 0 + .endif + idx = idx + 1 + .endr .size l2_xenmap, . - l2_xenmap /* L2 mapping the fixmap. Uses 1x 4k page. */ diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 2749cbc5cf..513b0f3841 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -12,6 +12,7 @@ #include <xen/livepatch.h> #include <xen/sched.h> +#include <asm/fixmap.h> #include <asm/nmi.h> #include <asm/livepatch.h> @@ -311,7 +312,7 @@ void __init arch_livepatch_init(void) void *start, *end; start = (void *)xen_virt_end; - end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE); + end = (void *)(XEN_VIRT_END - FIXADDR_X_SIZE - NR_CPUS * PAGE_SIZE); BUG_ON(end <= start); diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 654190e9e9..aabe1a4c64 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -157,6 +157,8 @@ /* Mapping of the fixmap space needed early. */ l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) l1_fixmap[L1_PAGETABLE_ENTRIES]; +l1_pgentry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE) + l1_fixmap_x[L1_PAGETABLE_ENTRIES]; paddr_t __read_mostly mem_hotplug; @@ -5763,6 +5765,13 @@ void __set_fixmap( map_pages_to_xen(__fix_to_virt(idx), _mfn(mfn), 1, flags); } +void __set_fixmap_x( + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags) +{ + BUG_ON(idx >= __end_of_fixed_addresses_x); + map_pages_to_xen(__fix_x_to_virt(idx), _mfn(mfn), 1, flags); +} + void *__init arch_vmap_virt_end(void) { return fix_to_virt(__end_of_fixed_addresses); diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c index c9d1ab4423..2da42fb691 100644 --- a/xen/arch/x86/smpboot.c +++ b/xen/arch/x86/smpboot.c @@ -640,7 +640,7 @@ unsigned long alloc_stub_page(unsigned int cpu, unsigned long *mfn) unmap_domain_page(memset(__map_domain_page(pg), 0xcc, PAGE_SIZE)); } - stub_va = XEN_VIRT_END - (cpu + 1) * PAGE_SIZE; + stub_va = XEN_VIRT_END - FIXADDR_X_SIZE - (cpu + 1) * PAGE_SIZE; if ( map_pages_to_xen(stub_va, page_to_mfn(pg), 1, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) ) { diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 07c6448dbb..cbc5701214 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -3,6 +3,8 @@ #include <xen/cache.h> #include <xen/lib.h> + +#include <asm/fixmap.h> #include <asm/page.h> #undef ENTRY #undef ALIGN @@ -353,6 +355,7 @@ SECTIONS } ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START - + MAX_FIXADDR_X_SIZE - DIV_ROUND_UP(NR_CPUS, STUBS_PER_PAGE) * PAGE_SIZE, "Xen image overlaps stubs area") diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h index d0cfbb70a8..4fa56ea0a9 100644 --- a/xen/include/asm-x86/config.h +++ b/xen/include/asm-x86/config.h @@ -218,7 +218,7 @@ extern unsigned char boot_edid_info[128]; /* Slot 261: high read-only compat machine-to-phys conversion table (1GB). */ #define HIRO_COMPAT_MPT_VIRT_START RDWR_COMPAT_MPT_VIRT_END #define HIRO_COMPAT_MPT_VIRT_END (HIRO_COMPAT_MPT_VIRT_START + GB(1)) -/* Slot 261: xen text, static data and bss (1GB). */ +/* Slot 261: xen text, static data, bss and executable fixmap (1GB). */ #define XEN_VIRT_START (HIRO_COMPAT_MPT_VIRT_END) #define XEN_VIRT_END (XEN_VIRT_START + GB(1)) diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h index 9fb2f47946..c2a9d2b50a 100644 --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -15,6 +15,9 @@ #include <asm/page.h> #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE) +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE) +/* This constant is derived from enum fixed_addresses_x below */ +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT) #ifndef __ASSEMBLY__ @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr) return __virt_to_fix(vaddr); } +enum fixed_addresses_x { + /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */ + FIX_X_RESERVED, +#ifdef CONFIG_HYPERV_GUEST + FIX_X_HYPERV_HCALL, +#endif + __end_of_fixed_addresses_x +}; + +#define FIXADDR_X_SIZE (__end_of_fixed_addresses_x << PAGE_SHIFT) +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE) + +extern void __set_fixmap_x( + enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags); + +#define set_fixmap_x(idx, phys) \ + __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES) + +#define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0) + +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT)) +#define __virt_to_fix_x(x) ((FIXADDR_X_TOP - ((x)&PAGE_MASK)) >> PAGE_SHIFT) + +#define fix_x_to_virt(x) ((void *)__fix_x_to_virt(x)) + #endif /* __ASSEMBLY__ */ #endif
This allows us to set aside some address space for executable mapping. This fixed map range starts from XEN_VIRT_END so that it is within reach of the .text section. Shift the percpu stub range and livepatch range accordingly. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- xen/arch/x86/boot/x86_64.S | 10 +++++++++- xen/arch/x86/livepatch.c | 3 ++- xen/arch/x86/mm.c | 9 +++++++++ xen/arch/x86/smpboot.c | 2 +- xen/arch/x86/xen.lds.S | 3 +++ xen/include/asm-x86/config.h | 2 +- xen/include/asm-x86/fixmap.h | 28 ++++++++++++++++++++++++++++ 7 files changed, 53 insertions(+), 4 deletions(-)