Re: [Xen-devel] [PATCH v3 10/38] arm/p2m: Move hostp2m init/teardown to individual functions

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,

On 09/01/2016 07:36 PM, Julien Grall wrote:
> Hello Sergej,
> 
> On 16/08/16 23:16, Sergej Proskurin wrote:
>> ---
>>  xen/arch/arm/p2m.c        | 71
>> +++++++++++++++++++++++++++++++++++++++++------
>>  xen/include/asm-arm/p2m.h | 11 ++++++++
>>  2 files changed, 73 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index e859fca..9ef19d4 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1245,27 +1245,53 @@ static void p2m_free_vmid(struct domain *d)
>>      spin_unlock(&vmid_alloc_lock);
>>  }
>>
>> -void p2m_teardown(struct domain *d)
>> +/* Reset this p2m table to be empty. */
>> +void p2m_flush_table(struct p2m_domain *p2m)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> -    struct page_info *pg;
>> +    struct page_info *page, *pg;
>> +    unsigned int i;
>> +
>> +    if ( p2m->root )
>> +    {
>> +        page = p2m->root;
>> +
>> +        /* Clear all concatenated first level pages. */
> 
> s/first/root/
> 

Ok.

>> +        for ( i = 0; i < P2M_ROOT_PAGES; i++ )
>> +            clear_and_clean_page(page + i);
> 
> Clearing live page table like that is not safe. Each entry (64-bit)
> should be written atomically to avoid breaking coherency (e.g if the MMU
> is walking the page table at the same time). However you don't know the
> implementation of clear_and_clean_page.

The function p2m_flush_table gets called by the altp2m subsystem
indrectly through the function p2m_teardown_one when the associated view
gets destroyed. In this case we should not worry about crashing the
domain, as the altp2m views are not active anyway.

The function altp2m_reset calls the function p2m_flush_table directly
(with active altp2m views), however, locks the p2m before flushing the
table. I did not find any locks on page-granularity, so please provide
me with further information about the solution you had in mind.

> 
> Also, this adds a small overhead when tearing down a p2m because the
> clear is not necessary.
> 

The p2m views are cleared very rarely so the overhead is really minimal
as it affects clearing the root tables. Besides the function
altp2m_reset calls the function p2m_flush_table and assumes that the
root tables are cleared as well. If the root tables would not be cleared
at this point, stalled entries in the altp2m views that got wiped out in
the hostp2m would make the system unstable.

>> +    }
>> +
>> +    /*
>> +     * Flush TLBs before releasing remaining intermediate p2m page
>> tables to
>> +     * prevent illegal access to stalled TLB entries.
>> +     */
>> +    p2m_flush_tlb(p2m);
> 
> p2m_flush_table is called in 2 places:
>     - p2m_teardown_one
>     - altp2m_reset
> 
> For p2m_teardown_one, the p2m may not have been allocated because the
> initialization failed. So try flush the TLBs may lead to a panic in Xen
> (the vttbr is invalid).
> 
> For altp2m_reset, this is called while updating the page tables (see
> altp2m_propagate_change). vCPU may still use the page tables at that time.
> 
> I am a bit worry that clear_and_clean_page
> 
>>
>> +    /* Free the rest of the trie pages back to the paging pool. */
>>      while ( (pg = page_list_remove_head(&p2m->pages)) )
>>          free_domheap_page(pg);
>>
>> +    p2m->lowest_mapped_gfn = INVALID_GFN;
>> +    p2m->max_mapped_gfn = _gfn(0);
>> +}
>> +
>> +void p2m_teardown_one(struct p2m_domain *p2m)
>> +{
>> +    p2m_flush_table(p2m);
>> +
>>      if ( p2m->root )
>>          free_domheap_pages(p2m->root, P2M_ROOT_ORDER);
>>
>>      p2m->root = NULL;
>>
>> -    p2m_free_vmid(d);
>> +    p2m_free_vmid(p2m->domain);
>> +
>> +    p2m->vttbr = INVALID_VTTBR;
> 
> Why did you add this line?
> 

Ok. On every p2m_teardown_one invocation the p2m gets destroyed. That
is, I will remove this additional resetting of the VTTBR in the next
patch. Thank you.

>>
>>      radix_tree_destroy(&p2m->mem_access_settings, NULL);
>>  }
>>
>> -int p2m_init(struct domain *d)
>> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>      int rc = 0;
>>
>>      rwlock_init(&p2m->lock);
>> @@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d)
>>          return rc;
>>
>>      p2m->max_mapped_gfn = _gfn(0);
>> -    p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
>> +    p2m->lowest_mapped_gfn = INVALID_GFN;
> 
> Why this change?
> 

Since we compare the gfn's with INVALID_GFN throughout the code it makes
sense to use the macro instead of a hardcoded value.

>>
>>      p2m->domain = d;
>> +    p2m->access_required = false;
> 
> This is not necessary as the p2m is zeroed during the allocation.
> 

Since the p2m is zeroed out at this moment, I can savely remove this
initialization.

>>      p2m->default_access = p2m_access_rwx;
>>      p2m->mem_access_enabled = false;
>> +    p2m->root = NULL;
>> +    p2m->vttbr = INVALID_VTTBR;
> 
> Why do you add those 2 lines?
> 

Same here.

>>      radix_tree_init(&p2m->mem_access_settings);
>>
>>      /*
>> @@ -1293,9 +1322,33 @@ int p2m_init(struct domain *d)
>>      p2m->clean_pte = iommu_enabled &&
>>          !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
>>
>> -    rc = p2m_alloc_table(d);
>> +    return p2m_alloc_table(d);
>> +}
>>
>> -    return rc;
>> +static void p2m_teardown_hostp2m(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    p2m_teardown_one(p2m);
>> +}
>> +
>> +void p2m_teardown(struct domain *d)
>> +{
>> +    p2m_teardown_hostp2m(d);
>> +}
>> +
>> +static int p2m_init_hostp2m(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>> +
>> +    p2m->p2m_class = p2m_host;
>> +
>> +    return p2m_init_one(d, p2m);
>> +}
>> +
>> +int p2m_init(struct domain *d)
>> +{
>> +    return p2m_init_hostp2m(d);
>>  }
>>
>>  /*
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index fa07e19..1a004ed 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -11,6 +11,8 @@
>>
>>  #define paddr_bits PADDR_BITS
>>
>> +#define INVALID_VTTBR (0UL)
> 
> Looking at the current use, you only use INVALID_VTTBR to set but not
> tested. However, the 2 places where it is use are not necessary.
> 

I will remove the macro including the both cases, where it is used for
initialization.

>> +
>>  /* Holds the bit size of IPAs in p2m tables.  */
>>  extern unsigned int p2m_ipa_bits;
>>
>> @@ -226,6 +228,15 @@ void guest_physmap_remove_page(struct domain *d,
>>
>>  mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>>
>> +/* Flushes the page table held by the p2m. */
>> +void p2m_flush_table(struct p2m_domain *p2m);
>> +
>> +/* Initialize the p2m structure. */
>> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m);
>> +
>> +/* Release resources held by the p2m structure. */
>> +void p2m_teardown_one(struct p2m_domain *p2m);
>> +
>>  /*
>>   * P2M rwlock helpers.
>>   */
>>
> 
> Regards,
> 

Best regards,
~Sergej

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