Re: [Xen-devel] [PATCHv3] x86/xen: avoid m2p lookup when setting early page table entries

[Juergen Gross <jgross@xxxxxxxx>]

On 23/06/16 15:37, David Vrabel wrote:
> On 23/06/16 14:27, Juergen Gross wrote:
>> On 23/06/16 15:06, David Vrabel wrote:
>>> On 23/06/16 13:13, Juergen Gross wrote:
>>>> On 23/06/16 11:51, David Vrabel wrote:
>>>>> When page tables entries are set using xen_set_pte_init() during early
>>>>> boot there is no page fault handler that could handle a fault when
>>>>> performing an M2P lookup.
>>>>>
>>>>> In 64 bit guests (usually dom0) early_ioremap() would fault in
>>>>> xen_set_pte_init() because an M2P lookup faults because the MFN is in
>>>>> MMIO space and not mapped in the M2P.  This lookup is done to see if
>>>>> the PFN in in the range used for the initial page table pages, so that
>>>>> the PTE may be set as read-only.
>>>>>
>>>>> The M2P lookup can be avoided by moving the check (and clear of RW)
>>>>> earlier when the PFN is still available.
>>>>>
>>>>> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
>>>>> ---
>>>>> Cc: Kevin Moraga <kmoragas@xxxxxxxxxx>
>>>>>
>>>>> v3:
>>>>> - fold mask_rw_pte()/mask_rw_pteval() into their callers.
>>>>>
>>>>> v2:
>>>>> - Remove __init annotation from xen_make_pte_init() since
>>>>>   PV_CALLEE_SAVE_REGS_THUNK always puts the thunk in .text.
>>>>>
>>>>> - mask_rw_pte() -> mask_rw_pteval() for x86-64.
>>>>> ---
>>>>>  arch/x86/xen/mmu.c | 76 
>>>>> +++++++++++++++++++++++++-----------------------------
>>>>>  1 file changed, 35 insertions(+), 41 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
>>>>> index 478a2de..64d8f0b 100644
>>>>> --- a/arch/x86/xen/mmu.c
>>>>> +++ b/arch/x86/xen/mmu.c
>>>>> @@ -1551,41 +1551,6 @@ static void xen_pgd_free(struct mm_struct *mm, 
>>>>> pgd_t *pgd)
>>>>>  #endif
>>>>>  }
>>>>>  
>>>>> -#ifdef CONFIG_X86_32
>>>>> -static pte_t __init mask_rw_pte(pte_t *ptep, pte_t pte)
>>>>> -{
>>>>> - /* If there's an existing pte, then don't allow _PAGE_RW to be set */
>>>>> - if (pte_val_ma(*ptep) & _PAGE_PRESENT)
>>>>> -         pte = __pte_ma(((pte_val_ma(*ptep) & _PAGE_RW) | ~_PAGE_RW) &
>>>>> -                        pte_val_ma(pte));
>>>>> -
>>>>> - return pte;
>>>>> -}
>>>>> -#else /* CONFIG_X86_64 */
>>>>> -static pte_t __init mask_rw_pte(pte_t *ptep, pte_t pte)
>>>>> -{
>>>>> - unsigned long pfn;
>>>>> -
>>>>> - if (xen_feature(XENFEAT_writable_page_tables) ||
>>>>> -     xen_feature(XENFEAT_auto_translated_physmap) ||
>>>>> -     xen_start_info->mfn_list >= __START_KERNEL_map)
>>>>> -         return pte;
>>>>> -
>>>>> - /*
>>>>> -  * Pages belonging to the initial p2m list mapped outside the default
>>>>> -  * address range must be mapped read-only. This region contains the
>>>>> -  * page tables for mapping the p2m list, too, and page tables MUST be
>>>>> -  * mapped read-only.
>>>>> -  */
>>>>> - pfn = pte_pfn(pte);
>>>>> - if (pfn >= xen_start_info->first_p2m_pfn &&
>>>>> -     pfn < xen_start_info->first_p2m_pfn + xen_start_info->nr_p2m_frames)
>>>>> -         pte = __pte_ma(pte_val_ma(pte) & ~_PAGE_RW);
>>>>> -
>>>>> - return pte;
>>>>> -}
>>>>> -#endif /* CONFIG_X86_64 */
>>>>> -
>>>>>  /*
>>>>>   * Init-time set_pte while constructing initial pagetables, which
>>>>>   * doesn't allow RO page table pages to be remapped RW.
>>>>> @@ -1600,13 +1565,41 @@ static pte_t __init mask_rw_pte(pte_t *ptep, 
>>>>> pte_t pte)
>>>>>   * so always write the PTE directly and rely on Xen trapping and
>>>>>   * emulating any updates as necessary.
>>>>>   */
>>>>> -static void __init xen_set_pte_init(pte_t *ptep, pte_t pte)
>>>>> +__visible pte_t xen_make_pte_init(pteval_t pte)
>>>>>  {
>>>>> - if (pte_mfn(pte) != INVALID_P2M_ENTRY)
>>>>> -         pte = mask_rw_pte(ptep, pte);
>>>>> - else
>>>>> -         pte = __pte_ma(0);
>>>>> +#ifdef CONFIG_X86_64
>>>>> + unsigned long pfn;
>>>>> +
>>>>> + /*
>>>>> +  * Pages belonging to the initial p2m list mapped outside the default
>>>>> +  * address range must be mapped read-only. This region contains the
>>>>> +  * page tables for mapping the p2m list, too, and page tables MUST be
>>>>> +  * mapped read-only.
>>>>> +  */
>>>>> + pfn = (pte & PTE_PFN_MASK) >> PAGE_SHIFT;
>>>>> + if (xen_start_info->mfn_list < __START_KERNEL_map &&
>>>>> +     pfn >= xen_start_info->first_p2m_pfn &&
>>>>> +     pfn < xen_start_info->first_p2m_pfn + xen_start_info->nr_p2m_frames)
>>>>> +         pte &= ~_PAGE_RW;
>>>>> +#endif
>>>>> + pte = pte_pfn_to_mfn(pte);
>>>>>  
>>>>> + if ((pte & PTE_PFN_MASK) >> PAGE_SHIFT == INVALID_P2M_ENTRY)
>>>>
>>>> How can this ever be true? I know this is just the open coded
>>>> variant form the original xen_set_pte_init(). Either the if isn't
>>>> needed at all or it should be corrected.
>>>
>>> The frame might be ballooned out.
>>
>> Okay. But how do you get the 64 bit value with all bits set by shifting
>> an unsigned value to the right (I admit that this is possible on a 32
>> bit dom0, but I doubt that was the intention of the test)?
> 
> pte_pfn_to_mfn() already does the right thing so this test is bogus.
> I've removed it.
> 
> Can I add your acked-by or reviewed-by with this change?

Yes, with the test removed: Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

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