Re: [PATCH v7 07/24] x86/mm: Introduce modify_xen_mappings()

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Sun, Apr 10, 2016 at 10:14 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> From: Andrew Cooper <andrew.cooper3@citrix.com>
>
> To simply change the permissions on existing Xen mappings.  The existing
> destroy_xen_mappings() is altered to support a change the PTE permissions.
>
> A new destroy_xen_mappings() is introduced, as the special case of not passing
> _PAGE_PRESENT to modify_xen_mappings().

This part looks correct to me.

> As cleanup (and an ideal functional test), the boot logic which remaps Xen's
> code and data with reduced permissions is altered to use
> modify_xen_mappings(), rather than map_pages_to_xen() passing the same mfn's
> back in.

This seems like a very good idea; I've just got one comment...

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 22dc148..b867242 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1220,23 +1220,19 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      if ( !using_2M_mapping() )
>      {
>          /* Mark .text as RX (avoiding the first 2M superpage). */
> -        map_pages_to_xen(XEN_VIRT_START + MB(2),
> -                         PFN_DOWN(__pa(XEN_VIRT_START + MB(2))),
> -                         PFN_DOWN(__2M_text_end -
> -                                  (const char *)(XEN_VIRT_START + MB(2))),
> -                         PAGE_HYPERVISOR_RX);
> +        modify_xen_mappings(XEN_VIRT_START + MB(2),
> +                            (unsigned long)&__2M_text_end,
> +                            _PAGE_PRESENT);
>
>          /* Mark .rodata as RO. */
> -        map_pages_to_xen((unsigned long)&__2M_rodata_start,
> -                         PFN_DOWN(__pa(__2M_rodata_start)),
> -                         PFN_DOWN(__2M_rodata_end - __2M_rodata_start),
> -                         PAGE_HYPERVISOR_RO);
> +        modify_xen_mappings((unsigned long)&__2M_rodata_start,
> +                            (unsigned long)&__2M_rodata_end,
> +                            _PAGE_NX | _PAGE_PRESENT);
>
>          /* Mark .data and .bss as RW. */
> -        map_pages_to_xen((unsigned long)&__2M_rwdata_start,
> -                         PFN_DOWN(__pa(__2M_rwdata_start)),
> -                         PFN_DOWN(__2M_rwdata_end - __2M_rwdata_start),
> -                         PAGE_HYPERVISOR_RW);
> +        modify_xen_mappings((unsigned long)&__2M_rwdata_start,
> +                            (unsigned long)&__2M_rwdata_end,
> +                            _PAGE_NX | _PAGE_RW | _PAGE_PRESENT);

The only thing here is that you're changing from the centrally-defined
PAGE_HYPERVISOR_$TYPE flags to what might be called "magic
combinations" of flags.  I realize this is because
modify_xen_mappings() only allows you to pass RW, PRESENT, and NX, but
it's still somewhat sub-optimal.

Alternatives that come to mind:
1. Mask out the other bits at the caller (i.e., PAGE_HYPERVISOR_RX &
MAP_PAGES_MASK)
2. Have modify_xen_mappings() filter out the bits it doesn't want to change
3. Extend modify_xen_mappings() to be able to modify other bits.
4. Have modify_xen_mappings() assert that the only bits which are
*changing* are the FLAGS_MASK bits

Thoughts?

 -George

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