Re: [Xen-devel] [PATCH v3 07/13] xen/arm: compile and initialize vmap

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 25 Apr 2013, Ian Campbell wrote:
> On Wed, 2013-04-24 at 20:07 +0100, Stefano Stabellini wrote:
> > Rename EARLY_VMAP_VIRT_END and EARLY_VMAP_VIRT_START to
> > VMAP_VIRT_END and VMAP_VIRT_START.
> > 
> > Defining VMAP_VIRT_START triggers the compilation of common/vmap.c.
> > 
> > Define PAGE_HYPERVISOR and MAP_SMALL_PAGES (unused on ARM).
> 
> So our vmap is 2MB mappings only? I suppose that's ok, at least for now.

No, I explained myself poorly, I meant that we only support 4K mappings
so MAP_SMALL_PAGES is useless (always set).


> > Implement map_pages_to_xen and destroy_xen_mappings.
> 
> This involves moving the prototypes from x86 to generic, so needs Jan +
> Keir, CCd.
> 
> > 
> > Call vm_init from start_xen.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> > +static int create_xen_table(lpae_t *entry)
> > +{
> > +    void *p;
> > +    lpae_t pte;
> > +
> > +    p = alloc_xenheap_pages(0, 0);
> > +    if ( p == NULL )
> > +        return -ENOMEM;
> > +    clear_page(p);
> > +    pte = mfn_to_xen_entry(virt_to_mfn(p));
> > +    pte.pt.table = 1;
> > +    write_pte(entry, pte);
> > +    return 0;
> > +}
> > +
> > +enum xenmap_operation {
> > +    INSERT,
> > +    REMOVE
> > +};
> > +
> > +static int create_xen_entries(enum xenmap_operation op,
> > +                              unsigned long virt,
> > +                              unsigned long mfn,
> > +                              unsigned long nr_mfns)
> 
> Shame this can't be combined with create_p2m_entries, but that uses
> domain pages and this uses xenheap pages.
> 
> > +{
> > +    int rc;
> > +    unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> > +    lpae_t pte;
> > +    lpae_t *third = NULL;
> > +
> > +    for(; addr < addr_end; addr += PAGE_SIZE, mfn++)
> > +    {
> > +        if ( !xen_second[second_linear_offset(addr)].pt.valid ||
> > +             !xen_second[second_linear_offset(addr)].pt.table )
> > +        {
> > +            rc = create_xen_table(&xen_second[second_linear_offset(addr)]);
> > +            if ( rc < 0 ) {
> > +                printk("create_xen_entries: L2 failed\n");
> > +                goto out;
> > +            }
> > +        }
> > +
> > +        BUG_ON(!xen_second[second_linear_offset(addr)].pt.valid);
> > +
> > +        third = 
> > __va((paddr_t)xen_second[second_linear_offset(addr)].pt.base
> > +                << PAGE_SHIFT);
> > +        if ( third[third_table_offset(addr)].pt.valid )
> > +            flush_tlb_local();
> 
> Why this flush? (I notice create_p2m_mapping does the same but with
> _all_local())

I dropped the _all_local because we don't want to flush all VMIDs here,
only the hypervisor mappings.
However I am not sure that this flush is actually necessary as we are
going to flush the mapping later on after the pte has been written.


> Isn't it a bug for the third to be already mapped? that suggests
> something is overwriting the mapping, does vmap do that?

I don't know, but I thought that this function should be able to handle
that case.


> > +
> > +        switch ( op ) {
> > +            case INSERT:
> > +                pte = mfn_to_xen_entry(mfn);
> > +                pte.pt.table = 1;
> > +                write_pte(&third[third_table_offset(addr)], pte);
> > +                break;
> > +            case REMOVE:
> > +                memset(&pte, 0x00, sizeof(pte));
> 
>             AKA:  pte.bits = 0;

OK

> > +                write_pte(&third[third_table_offset(addr)], pte);
> > +                break;
> > +            default:
> > +                printk("create_xen_entries: invalid op\n");
> 
> ASSERT? This really can never happen.

Right. A BUG might be better then.

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