Re: [Xen-devel] [PATCH 15/22] xen/arm: Don't call p2m_alloc_table from arch_domain_create

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

On 07/22/2016 12:38 PM, Julien Grall wrote:
> 
> 
> On 22/07/16 11:39, Sergej Proskurin wrote:
>>
>>
>> On 07/22/2016 12:26 PM, Julien Grall wrote:
>>>
>>>
>>> On 22/07/16 11:16, Sergej Proskurin wrote:
>>>> Hi Julien,
>>>
>>> Hello,
>>>
>>>> On 07/22/2016 11:18 AM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 22/07/16 09:32, Sergej Proskurin wrote:
>>>>>> Hi Julien,
>>>>>
>>>>> Hello Sergej,
>>>>>
>>>>>>> -int p2m_alloc_table(struct domain *d)
>>>>>>> +static int p2m_alloc_table(struct domain *d)
>>>>>>
>>>>>> While moving parts of the altp2m code out of ./xen/arch/arm/p2m.c,
>>>>>> the
>>>>>> function p2m_alloc_table needs to be called from
>>>>>> ./xen/arch/arm/altp2m.c
>>>>>> to allocate the individual altp2m views. Hence it should not be
>>>>>> static.
>>>>>
>>>>> No, this function should not be called outside p2m.c as it will not
>>>>> fully initialize the p2m. You need to need to provide a function to
>>>>> initialize a p2m (such as p2m_init).
>>>>>
>>>>
>>>> The last time we have discussed reusing existing code, among others,
>>>> for
>>>> individual struct p2m_domain initialization routines. Also, we have
>>>> agreed to move altp2m-related parts out of p2m.c into altp2m.c, which
>>>> makes it hard not to access parts required for initialization/teardown
>>>> (that are equal for both p2m and altp2m).
>>>
>>> I remember this discussion. However, the p2m initialize/teardown should
>>> exactly be the same for the hostp2m and altp2m (except for the type of
>>> the p2m). So, a function should be provided to initialize a full p2m to
>>> avoid code duplication.
>>>
>>
>> This is exactly what has been done. Nevertheless, altp2m views are
>> somewhat more dynamic than hostp2m and hence require frequent
>> initialization/teardown of individual views. That is, by moving altp2m
>> parts out of p2m.c we simply need to access this shared code multiple
>> times at runtime from different altp2m-related functions. This applies
>> to more functions that just to p2m_alloc_table.
> 
> I am not convinced that you need to reallocate the root page every time
> rather than clearing them. Anyway, I will need to see the code to
> understand what is done.
> 
> Regards,
> 

In the following, you will find an excerpt of both the p2m.c and
altp2m.c concerning the (alt)p2m initialization. Please note that
altp2m_init_helper is called from different initialization routines in
altp2m.c. Also, please consider that this code has not yet been ported
to your recent patches. Please let me know if you need more information.

---
./xen/arch/p2m.c:

...

int p2m_alloc_table(struct p2m_domain *p2m)
{
    unsigned int i;
    struct page_info *page;
    struct vttbr *vttbr = &p2m->vttbr;

    page = alloc_domheap_pages(NULL, P2M_ROOT_ORDER, 0);
    if ( page == NULL )
        return -ENOMEM;

    /* Clear all first level pages */
    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
        clear_and_clean_page(page + i);

    p2m->root = page;

    /* Initialize the VTTBR associated with the allocated p2m table. */
    vttbr->vttbr = 0;
    vttbr->vmid = p2m->vmid & 0xff;
    vttbr->baddr = page_to_maddr(p2m->root);

    return 0;
}

int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
{
    int rc = 0;

    spin_lock_init(&p2m->lock);
    INIT_PAGE_LIST_HEAD(&p2m->pages);

    spin_lock(&p2m->lock);

    p2m->vmid = INVALID_VMID;
    rc = p2m_alloc_vmid(d, p2m);
    if ( rc != 0 )
        goto err;

    p2m->domain = d;
    p2m->access_required = false;
    p2m->mem_access_enabled = false;
    p2m->default_access = p2m_access_rwx;
    p2m->root = NULL;

    p2m->vttbr.vttbr = INVALID_VTTBR;

    p2m->max_mapped_gfn = 0;
    p2m->lowest_mapped_gfn = INVALID_GFN;
    radix_tree_init(&p2m->mem_access_settings);

err:
    spin_unlock(&p2m->lock);

    return rc;
}

static int p2m_init_hostp2m(struct domain *d)
{
    struct p2m_domain *p2m = p2m_get_hostp2m(d);

    d->arch.vttbr = INVALID_VTTBR;
    p2m->p2m_class = p2m_host;

    return p2m_init_one(d, p2m);
}

int p2m_init(struct domain *d)
{
    int rc;

    rc = p2m_init_hostp2m(d);
    if ( rc )
        return rc;

    return altp2m_init(d);
}

...

---
./xen/arch/altp2m.c:

...

static int altp2m_init_helper(struct domain *d, unsigned int idx)
{
    int rc;
    struct p2m_domain *p2m = d->arch.altp2m_p2m[idx];

    if ( p2m == NULL )
    {
        /* Allocate a new altp2m view. */
        p2m = xzalloc(struct p2m_domain);
        if ( p2m == NULL)
        {
            rc = -ENOMEM;
            goto err;
        }
        memset(p2m, 0, sizeof(struct p2m_domain));
    }

    /* Initialize the new altp2m view. */
    rc = p2m_init_one(d, p2m);
    if ( rc )
        goto err;

    /* Allocate a root table for the altp2m view. */
    rc = p2m_alloc_table(p2m);
    if ( rc )
        goto err;

    p2m->p2m_class = p2m_alternate;
    p2m->access_required = 1;
    _atomic_set(&p2m->active_vcpus, 0);

    d->arch.altp2m_p2m[idx] = p2m;
    d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr;

    /*
     * Make sure that all TLBs corresponding to the new VMID are flushed
     * before using it.
     */
    flush_tlb_p2m(d, p2m);

    return rc;

err:
    if ( p2m )
        xfree(p2m);

    d->arch.altp2m_p2m[idx] = NULL;

    return rc;
}

int altp2m_init_by_id(struct domain *d, unsigned int idx)
{
    int rc = -EINVAL;

    if ( idx >= MAX_ALTP2M )
        return rc;

    altp2m_lock(d);

    if ( d->arch.altp2m_vttbr[idx] == INVALID_VTTBR )
        rc = altp2m_init_helper(d, idx);

    altp2m_unlock(d);

    return rc;
}

int altp2m_init(struct domain *d)
{
    unsigned int i;

    spin_lock_init(&d->arch.altp2m_lock);

    for ( i = 0; i < MAX_ALTP2M; i++ )
    {
        d->arch.altp2m_p2m[i] = NULL;
        d->arch.altp2m_vttbr[i] = INVALID_VTTBR;
    }

    return 0;
}

...

Regards,
~Sergej

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