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

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

On 11/04/16 13:51, George Dunlap wrote:
> 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?

I suppose I could change the entry requirements and have "nf &=
FLAGS_MASK" rather than the ASSERT()?

That would allow the caller to pass the regular PAGE_HYPERVISOR_xxx,
which I guess is what you mean by 2)

1) is a little nastly. 3) is hard.  Most other bits you can't safely do
blanket changes like this with.  4 is not possible).  Over the range you
might have different caching, different A/D bits or different global
settings, which want to be kept as-are.

~Andrew

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