Re: [Xen-devel] [PATCH v1 4/9] arm/mm: Introduce modify_xen_mappings

[Julien Grall <julien.grall@xxxxxxx>]

Hi Konrad,

On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
> Which is only used by Livepatch code. The purpose behind
> this call is to modify the page table entries flags.
>
> Specifically the .ro and .nx flags. The current mechanism
> puts cache attributes in the flags and the .ro and .nx are
> locked down and assumed to be .ro=0, nx=1.
>
> Livepatch needs .nx=0 and also .ro to be set to 1.
>
> We introduce a new 'flags' where various bits determine
> whether .ro and .nx bits are set or cleared. We can't use
> an enum as the function prototype would diverge from x86.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> --
> Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> [Added as he wrote the x86 modify_xen_mappings version]
>
> RFC: First submission.
> v1: Add #defines to make it simpler to understand the bit-shifting.
> ---
>  xen/arch/arm/mm.c          | 21 ++++++++++++++++++---
>  xen/include/asm-arm/page.h | 11 +++++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 4e256c2..520c0e0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -836,6 +836,7 @@ static int create_xen_table(lpae_t *entry)
>  enum xenmap_operation {
>      INSERT,
>      REMOVE,
> +    MODIFY,
>      RESERVE
>  };
>
> @@ -881,14 +882,22 @@ static int create_xen_entries(enum xenmap_operation op,
>                  pte.pt.table = 1;
>                  write_pte(&third[third_table_offset(addr)], pte);
>                  break;
> +            case MODIFY:
>              case REMOVE:
>                  if ( !third[third_table_offset(addr)].pt.valid )
>                  {
> -                    printk("create_xen_entries: trying to remove a non-existing 
> mapping addr=%lx\n",
> -                           addr);
> +                    printk("create_xen_entries: trying to %s a non-existing mapping 
> addr=%lx\n",
> +                           op == REMOVE ? "remove" : "modify", addr);
>                      return -EINVAL;
>                  }
> -                pte.bits = 0;
> +                if ( op == REMOVE )
> +                    pte.bits = 0;
> +                else
> +                {
> +                    pte = third[third_table_offset(addr)];
> +                    pte.pt.ro = PTE_RO_MASK(ai);
> +                    pte.pt.xn = PTE_NX_MASK(ai);
I would like to see some sanity check for read-only and execute-never 
bit. For instance, ro = 0 and xn = 0 would lead to a crash later on 
because Xen is enabling SCTLR_EL2.WXN.
I think the other combinations should be fine.

> +                }
>                  write_pte(&third[third_table_offset(addr)], pte);
>                  break;
>              default:
> @@ -922,6 +931,12 @@ void destroy_xen_mappings(unsigned long v, unsigned long e)
>      create_xen_entries(REMOVE, v, 0, (e - v) >> PAGE_SHIFT, 0);
>  }
>
> +void modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
create_xen_entries may fail, don't you want to report the error to the 
caller?
> +{
> +    ASSERT((flags & (PTE_NX | PTE_RO)) == flags);
> +    create_xen_entries(MODIFY, s, 0, (e - s) >> PAGE_SHIFT, flags);
> +}
> +
>  enum mg { mg_clear, mg_ro, mg_rw, mg_rx };
>  static void set_pte_flags_on_range(const char *p, unsigned long l, enum mg mg)
>  {
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 05d9f82..2f66740 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -66,6 +66,17 @@
>  #define PAGE_HYPERVISOR_WC      (DEV_WC)
>
>  /*
> + * Defines for changing the PTE .ro and .nx bits. This is only to be
> + * used with modify_xen_mappings.
> + */
> +#define _PTE_NX_BIT     0U
> +#define _PTE_RO_BIT     1U
> +#define PTE_NX          (1U << _PTE_NX_BIT)
> +#define PTE_RO          (1U << _PTE_RO_BIT)
> +#define PTE_NX_MASK(x)  (((x) >> _PTE_NX_BIT) & 0x1U)
> +#define PTE_RO_MASK(x)  (((x) >> _PTE_RO_BIT) & 0x1U)
> +
> +/*
>   * Stage 2 Memory Type.
>   *
>   * These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel