|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 19/25] arm/altp2m: Add altp2m_propagate_change.
Hi Julien,
On 08/04/2016 04:50 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> This commit introduces the function "altp2m_propagate_change" that is
>> responsible to propagate changes applied to the host's p2m to a specific
>> or even all altp2m views. In this way, Xen can in-/decrease the guest's
>> physmem at run-time without leaving the altp2m views with
>> stalled/invalid entries.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> xen/arch/arm/altp2m.c | 75
>> ++++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/p2m.c | 14 +++++++++
>> xen/include/asm-arm/altp2m.h | 9 ++++++
>> xen/include/asm-arm/p2m.h | 5 +++
>> 4 files changed, 103 insertions(+)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index f98fd73..f3c1cff 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -133,6 +133,81 @@ out:
>> return rc;
>> }
>>
>> +static inline void altp2m_reset(struct p2m_domain *p2m)
>> +{
>> + read_lock(&p2m->lock);
>
> Again read_lock does not protect you against concurrent access. Only
> against someone else update the page table.
>
> This should be p2m_write_lock.
>
Thank you.
>> +
>> + p2m_flush_table(p2m);
>> + p2m_flush_tlb(p2m);
>
> altp2m_reset may be called on a p2m used by a running vCPU. What this
> code does is:
> 1) clearing root page table
> 2) free intermediate page table
> 3) invalidate the TLB
>
> Until step 3, the other TLBs may contain entries pointing the
> intermediate page table. But they were freed and could therefore be
> re-used for another purpose. So step 2 and 3 should be inverted.
This is an excellent tip. Thank you!
>
> I will re-iterate same message as in the previous series. Please have
> a think about the locking and memory ordering of all this series. I
> found a lot of race condition and I may have miss someone. If you have
> a doubt don't hesitate to ask.
>
I definitely will, thank you for your offer :)
>> +
>> + p2m->lowest_mapped_gfn = INVALID_GFN;
>> + p2m->max_mapped_gfn = _gfn(0);
>> +
>> + read_unlock(&p2m->lock);
>> +}
>> +
>> +void altp2m_propagate_change(struct domain *d,
>> + gfn_t sgfn,
>> + unsigned long nr,
>> + mfn_t smfn,
>> + uint32_t mask,
>> + p2m_type_t p2mt,
>> + p2m_access_t p2ma)
>> +{
>> + struct p2m_domain *p2m;
>> + mfn_t m;
>> + unsigned int i;
>> + unsigned int reset_count = 0;
>> + unsigned int last_reset_idx = ~0;
>> +
>> + if ( !altp2m_active(d) )
>
> This is not safe. d->arch.altp2m_active maybe be turn off just after
> you read. Maybe you want to protect it with altp2m_lock.
>
This is true. Thanks!
>> + return;
>> +
>> + altp2m_lock(d);
>> +
>> + for ( i = 0; i < MAX_ALTP2M; i++ )
>> + {
>> + if ( d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
>> + continue;
>> +
>> + p2m = d->arch.altp2m_p2m[i];
>> +
>> + m = p2m_lookup_attr(p2m, sgfn, NULL, NULL, NULL, NULL);
>
> What is the benefits to check if it already exists in the altp2m?
>
The current implementation directly changes the entry in the particular
altp2m view, without having to flush it if an already existing entry has
been changed.
>> +
>> + /* Check for a dropped page that may impact this altp2m. */
>> + if ( (mfn_eq(smfn, INVALID_MFN) || p2mt == p2m_invalid) &&
>
> Why the check to p2mt against p2m_invalid?
>
I have encountered p2m entries or rather calls to p2m_apply_change with
valid MFN's, however, invalid types. That is, a page drop would not be
recognized if checked only against an invalid MFN.
>> + gfn_x(sgfn) >= gfn_x(p2m->lowest_mapped_gfn) &&
>> + gfn_x(sgfn) <= gfn_x(p2m->max_mapped_gfn) )
>> + {
>> + if ( !reset_count++ )
>> + {
>> + altp2m_reset(p2m);
>> + last_reset_idx = i;
>> + }
>> + else
>> + {
>> + /* At least 2 altp2m's impacted, so reset
>> everything. */
>
> So if you remove a 4KB page in more than 2 altp2m, you will flush all
> the p2m. This sounds really more time consuming (you have to free all
> the intermediate page table) than removing a single 4KB page.
I agree: The solution has been directly adopted from the x86
implementation. It needs further design reconsideration. However, at
this point we would like to finish the overall architecture for ARM
before we go into further reconstructions.
>
>> + for ( i = 0; i < MAX_ALTP2M; i++ )
>> + {
>> + if ( i == last_reset_idx ||
>> + d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
>> + continue;
>> +
>> + p2m = d->arch.altp2m_p2m[i];
>> + altp2m_reset(p2m);
>> + }
>> + goto out;
>> + }
>> + }
>> + else if ( !mfn_eq(m, INVALID_MFN) )
>> + modify_altp2m_range(d, p2m, sgfn, nr, smfn,
>> + mask, p2mt, p2ma);
>
> I am a bit concerned about this function. We decided to limit the size
> of the mapping to avoid long running memory operations (see XSA-158).
>
> With this function you multiply up to 10 times the duration of the
> operation.
>
I see your point. However, on an active system, adaptions of the hostp2m
(while altp2m is active) should be very limited. Another solution would
be to simply flush the altp2m views on every hostp2m modification. IMO
this, however, would not really be a huge performance gain due to
de-allocation and freeing of the altp2m tables. Do you have another idea?
> Also, what is modify_altp2m_range has failed?
>
This is true: not considered at this point. I will return an error to
p2m_apply_changes.
>
>> + }
>> +
>> +out:
>> + altp2m_unlock(d);
>> +}
>> +
>> static void altp2m_vcpu_reset(struct vcpu *v)
>> {
>> struct altp2mvcpu *av = &vcpu_altp2m(v);
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index e0a7f38..31810e6 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -992,6 +992,7 @@ static int apply_p2m_changes(struct domain *d,
>> const bool_t preempt = !is_idle_vcpu(current);
>> bool_t flush = false;
>> bool_t flush_pt;
>> + bool_t entry_written = false;
>> PAGE_LIST_HEAD(free_pages);
>> struct page_info *pg;
>>
>> @@ -1112,6 +1113,7 @@ static int apply_p2m_changes(struct domain *d,
>> &addr, &maddr, &flush,
>> t, a);
>> if ( ret < 0 ) { rc = ret ; goto out; }
>> + if ( ret ) entry_written = 1;
>
> Please don't mix false and 1. This should be true here.
>
I will set the variable to true.
>> count += ret;
>>
>> if ( ret != P2M_ONE_PROGRESS_NOP )
>> @@ -1208,6 +1210,9 @@ out:
>>
>> p2m_write_unlock(p2m);
>>
>> + if ( rc >= 0 && entry_written && p2m_is_hostp2m(p2m) )
>> + altp2m_propagate_change(d, sgfn, nr, smfn, mask, t, a);
>
> There are operation which does not require propagation (for instance
> RELINQUISH and CACHEFLUSH).
>
True. Thank you.
>> +
>> if ( rc < 0 && ( op == INSERT ) &&
>> addr != start_gpaddr )
>> {
>> @@ -1331,6 +1336,15 @@ int modify_altp2m_entry(struct domain *d,
>> struct p2m_domain *ap2m,
>> return apply_p2m_changes(d, ap2m, INSERT, gfn, nr, mfn, 0, t, a);
>> }
>>
>> +int modify_altp2m_range(struct domain *d, struct p2m_domain *ap2m,
>> + gfn_t sgfn, unsigned long nr, mfn_t smfn,
>> + uint32_t m, p2m_type_t t, p2m_access_t a)
>> +{
>> + ASSERT(p2m_is_altp2m(ap2m));
>> +
>> + return apply_p2m_changes(d, ap2m, INSERT, sgfn, nr, smfn, m, t, a);
>> +}
>> +
>> int p2m_alloc_table(struct p2m_domain *p2m)
>> {
>> unsigned int i;
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index dc41f93..9aeb7d6 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -81,4 +81,13 @@ int altp2m_set_mem_access(struct domain *d,
>> p2m_access_t a,
>> gfn_t gfn);
>>
>> +/* Propagates changes made to hostp2m to affected altp2m views. */
>> +void altp2m_propagate_change(struct domain *d,
>> + gfn_t sgfn,
>> + unsigned long nr,
>> + mfn_t smfn,
>> + uint32_t mask,
>> + p2m_type_t p2mt,
>> + p2m_access_t p2ma);
>> +
>> #endif /* __ASM_ARM_ALTP2M_H */
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index 9859ad1..59186c9 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -191,6 +191,11 @@ int modify_altp2m_entry(struct domain *d, struct
>> p2m_domain *p2m,
>> paddr_t gpa, paddr_t maddr, unsigned int level,
>> p2m_type_t t, p2m_access_t a);
>>
>> +/* Modify an altp2m view's range of entries or their attributes. */
>> +int modify_altp2m_range(struct domain *d, struct p2m_domain *p2m,
>> + gfn_t sgfn, unsigned long nr, mfn_t smfn,
>> + uint32_t mask, p2m_type_t t, p2m_access_t a);
>> +
>> /* Clean & invalidate caches corresponding to a region of guest
>> address space */
>> int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>>
>>
>
Best regards,
~Sergej
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |