Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Jan Beulich <jbeulich@xxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel