Re: [Xen-devel] [PATCH 06/18] arm/altp2m: Add a(p2m) table flushing routines.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hello Julien,

On 07/04/2016 05:55 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 04/07/16 12:45, Sergej Proskurin wrote:
>> The current implementation differentiates between flushing and
>> destroying altp2m views. This commit adds the functions
>> p2m_flush_altp2m, and p2m_flush_table, which allow to flush all or
>> individual altp2m views without destroying the entire table. In this
>> way, altp2m views can be reused at a later point in time.
>>
>> In addition, the implementation clears all altp2m entries during the
>> process of flushing. The same applies to hostp2m entries, when it is
>> destroyed. In this way, further domain and p2m allocations will not
>> unintentionally reuse old p2m mappings.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>>   xen/arch/arm/p2m.c        | 67
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/p2m.h | 15 ++++++++---
>>   2 files changed, 78 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 4a745fd..ae789e6 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -2110,6 +2110,73 @@ int p2m_init_altp2m_by_id(struct domain *d,
>> unsigned int idx)
>>       return rc;
>>   }
>>
>> +/* Reset this p2m table to be empty */
>> +static void p2m_flush_table(struct p2m_domain *p2m)
>> +{
>> +    struct page_info *top, *pg;
>> +    mfn_t mfn;
>> +    unsigned int i;
>> +
>> +    /* Check whether the p2m table has already been flushed before. */
>> +    if ( p2m->root == NULL)
>
> This check looks invalid. p2m->root is never reset to NULL by
> p2m_flush_table, so you will always flush.
>

All right. Here, I just wanted to be sure that we don't flush an
invalidpage. I will remove this check.

>> +        return;
>> +
>> +    spin_lock(&p2m->lock);
>> +
>> +    /*
>> +     * "Host" p2m tables can have shared entries &c that need a bit
>> more care
>> +     * when discarding them
>
> I don't understand this comment. Can you explain it?
>

This is a lost comment. However, there are slight differences in freeing
a hostp2m as opposed to an altp2m (see the additional freeing of VMIDs
in hostp2ms). Since, we agreed to using unique VMIDs for altp2m views as
well, I will try to merge the different flush/free functions.Thank you.

>> +     */
>> +    ASSERT(!p2m_is_hostp2m(p2m));
>> +
>> +    /* Zap the top level of the trie */
>> +    top = p2m->root;
>> +
>> +    /* Clear all concatenated first level pages */
>> +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
>> +    {
>> +        mfn = _mfn(page_to_mfn(top + i));
>> +        clear_domain_page(mfn);
>> +    }
>> +
>> +    /* Free the rest of the trie pages back to the paging pool */
>> +    while ( (pg = page_list_remove_head(&p2m->pages)) )
>> +        if ( pg != top  )
>> +        {
>> +            /*
>> +             * Before freeing the individual pages, we clear them to
>> prevent
>> +             * reusing old table entries in future p2m allocations.
>> +             */
>> +            mfn = _mfn(page_to_mfn(pg));
>> +            clear_domain_page(mfn);
>> +            free_domheap_page(pg);
>> +        }
>> +
>> +    page_list_add(top, &p2m->pages);
>
> This code is very similar to p2m_free_one. Can we share some code?
>

Yes, I will merge both functions and extract parts that differ in a
separate function. The same applies to p2m_teardown_hostp2m. Thank you.

>> +
>> +    /* Invalidate VTTBR */
>> +    p2m->vttbr.vttbr = 0;
>> +    p2m->vttbr.vttbr_baddr = INVALID_MFN;
>> +
>> +    spin_unlock(&p2m->lock);
>> +}
>
> Regards,
>

Thank you.

Cheers,
Sergej


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