Re: [PATCH v2 5/5] libxc: create p2m list outside of kernel mapping if supported

[Juergen Gross <jgross@suse.com>]

On 10/02/2015 03:16 PM, Ian Campbell wrote:
> On Fri, 2015-10-02 at 07:49 +0200, Juergen Gross wrote:
>>
>> +    /* Allocate p2m list if outside of initial kernel mapping. */
>> +    if ( dom->arch_hooks->alloc_p2m_list && dom->parms.p2m_base != UNSET_ADDR )
>> +    {
>> +        if ( dom->arch_hooks->alloc_p2m_list(dom) != 0 )
>> +            goto err;
>> +        dom->p2m_seg.vend = dom->p2m_seg.vend - dom->p2m_seg.vstart;
>> +        dom->p2m_seg.vstart = dom->parms.p2m_base;
>> +        dom->p2m_seg.vend += dom->p2m_seg.vstart;
>
> Here is this strange pattern again.
>
> It seems like you should be adding new APIs to the dom builder's VA/PA
> allocation stuff and using those instead of working around the behaviour of
> the existing ones.

Okay. I'll split allocation into two layers (virtual and physical)
and make the virtual one accessible for other functions. I'll need
to keep track of physical and virtual allocation boundaries, of
course.

>
>> +    }
>> +    /*
>> +     * Build the page tables for mapping the p2m list at an address
>> +     * specified by the to be loaded kernel.
>> +     * l1pfn holds the pfn of the next page table to allocate.
>> +     * At each level we might already have an entry filled when setting
>> +     * up the initial kernel mapping. This can happen for the last entry
>> +     * of each level only!
>> +     */
>> +    l3tab = NULL;
>> +    l2tab = NULL;
>> +    l1tab = NULL;
>> +    l1pfn = round_pfn(dom->p2m_size * dom->arch_hooks->sizeof_pfn) +
>> +            dom->p2m_seg.pfn;
>> +
>> +    for ( addr = dom->parms.p2m_base;
>> +          addr < dom->parms.p2m_base +
>> +                 dom->p2m_size * dom->arch_hooks->sizeof_pfn;
>> +          addr += PAGE_SIZE_X86 )
>
> This is replicating a bunch of existing setup_pgtable_* code.
>
> Please refactor into a helper (one per PT layout) to map a region and use
> that for the existing and new use cases.

Hmm, I thought about this, too. The problem is there are subtle
differences between both variants. I can give it a try and see how
the code looks like.

>
>> +    {
>> +        if ( l3tab == NULL )
>> +        {
>> +            l4off = l4_table_offset_x86_64(addr);
>> +            l3pfn = l4tab[l4off] ? l4pfn + dom->pg_l4 : l1pfn++;
>> +            l3tab = xc_dom_pfn_to_ptr(dom, l3pfn, 1);
>> +            if ( l3tab == NULL )
>> +                goto pfn_error;
>> +            l4tab[l4off] =
>> +                pfn_to_paddr(xc_dom_p2m_guest(dom, l3pfn)) | L4_PROT;
>> +        }
>> +
>> +        if ( l2tab == NULL )
>> +        {
>> +            l3off = l3_table_offset_x86_64(addr);
>> +            l2pfn = l3tab[l3off] ? l3pfn + dom->pg_l3 : l1pfn++;
>> +            l2tab = xc_dom_pfn_to_ptr(dom, l2pfn, 1);
>> +            if ( l2tab == NULL )
>> +                goto pfn_error;
>> +            l3tab[l3off] =
>> +                pfn_to_paddr(xc_dom_p2m_guest(dom, l2pfn)) | L3_PROT;
>> +        }
>> +
>> +        if ( l1tab == NULL )
>> +        {
>> +            l2off = l2_table_offset_x86_64(addr);
>> +            l1pfn = l2tab[l2off] ? l2pfn + dom->pg_l2 : l1pfn;
>> +            l1tab = xc_dom_pfn_to_ptr(dom, l1pfn, 1);
>> +            if ( l1tab == NULL )
>> +                goto pfn_error;
>> +            l2tab[l2off] =
>> +                pfn_to_paddr(xc_dom_p2m_guest(dom, l1pfn)) | L2_PROT;
>> +            l1pfn++;
>> +        }
>> +
>> +        l1off = l1_table_offset_x86_64(addr);
>> +        pgpfn = ((addr - dom->parms.p2m_base) >> PAGE_SHIFT_X86) +
>> +                dom->p2m_seg.pfn;
>> +        l1tab[l1off] =
>> +            pfn_to_paddr(xc_dom_p2m_guest(dom, pgpfn)) | L1_PROT;
>> +
>> +        if ( l1off == (L1_PAGETABLE_ENTRIES_X86_64 - 1) )
>> +        {
>> +            l1tab = NULL;
>> +            if ( l2off == (L2_PAGETABLE_ENTRIES_X86_64 - 1) )
>> +            {
>> +                l2tab = NULL;
>> +                if ( l3off == (L3_PAGETABLE_ENTRIES_X86_64 - 1) )
>> +                    l3tab = NULL;
>> +            }
>> +        }
>> +    }
>> +
>>       return 0;
>>
>>   pfn_error:
>> @@ -442,6 +519,27 @@ pfn_error:
>>   static int alloc_p2m_list(struct xc_dom_image *dom)
>>   {
>>       size_t p2m_alloc_size = dom->p2m_size * dom->arch_hooks->sizeof_pfn;
>> +    xen_vaddr_t from, to;
>> +    xen_pfn_t tables;
>> +
>> +    p2m_alloc_size = round_pg(p2m_alloc_size);
>> +    if ( dom->parms.p2m_base != UNSET_ADDR )
>> +    {
>> +        /* Add space for page tables, 64 bit only. */
>
> Please make an alloc_p2m_list_x86_64 which does this and then calls the
> common code and then use the appropriate hook for each sub arch.

Okay.

>> +        from = dom->parms.p2m_base;
>> +        to = from + p2m_alloc_size - 1;
>> +        tables = 0;
>> +        tables += nr_page_tables(dom, from, to,
>> L4_PAGETABLE_SHIFT_X86_64);
>> +        if ( to > (xen_vaddr_t)(~0ULL << L4_PAGETABLE_SHIFT_X86_64) )
>> +            tables--;
>> +        tables += nr_page_tables(dom, from, to,
>> L3_PAGETABLE_SHIFT_X86_64);
>> +        if ( to > (xen_vaddr_t)(~0ULL << L3_PAGETABLE_SHIFT_X86_64) )
>> +            tables--;
>> +        tables += nr_page_tables(dom, from, to,
>> L2_PAGETABLE_SHIFT_X86_64);
>> +        if ( to > (xen_vaddr_t)(~0ULL << L2_PAGETABLE_SHIFT_X86_64) )
>> +            tables--;
>> +        p2m_alloc_size += tables << PAGE_SHIFT_X86;
>> +    }
>>
>>       /* allocate phys2mach table */
>>       if ( xc_dom_alloc_segment(dom, &dom->p2m_seg, "phys2mach",


Juergen