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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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