Re: [Xen-devel] [PATCH v2 09/25] arm/altp2m: Add altp2m table flushing routine.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/03/2016 08:44 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> The current implementation differentiates between flushing and
>> destroying altp2m views. This commit adds the function altp2m_flush,
>> which allows 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.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> v2: Pages in p2m->pages are not cleared in p2m_flush_table anymore.
>>     VMID is freed in p2m_free_one.
>>     Cosmetic fixes.
>> ---
>>  xen/arch/arm/altp2m.c        | 38
>> ++++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/altp2m.h |  5 +----
>>  xen/include/asm-arm/p2m.h    |  3 +++
>>  3 files changed, 42 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index 767f233..e73424c 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -151,6 +151,44 @@ int altp2m_init(struct domain *d)
>>      return 0;
>>  }
>>
>> +void altp2m_flush(struct domain *d)
>> +{
>> +    unsigned int i;
>> +    struct p2m_domain *p2m;
>> +
>> +    /*
>> +     * If altp2m is active, we are not allowed to flush altp2m[0].
>> This special
>> +     * view is considered as the hostp2m as long as altp2m is active.
>> +     */
>> +    ASSERT(!altp2m_active(d));
>
> Because of the race condition I mentioned in the previous patch (#8),
> this ASSERT may be hit randomly.
>

I will handle it by removing the race condition as discussed in patch #1
and #8.

>> +
>> +    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];
>> +
>> +        read_lock(&p2m->lock);
>
> You should use p2m_lock rather than re-inventing your own. If
> p2m_*lock helpers need to be exposed, then expose them.
>
> Also, this should be a write_lock otherwise someone else could access
> at the same time.

Ok, thank you.

>
>> +
>> +        p2m_flush_table(p2m);
>> +
>> +        /*
>> +         * Reset VTTBR.
>> +         *
>> +         * Note that VMID is not freed so that it can be reused later.
>> +         */
>> +        p2m->vttbr.vttbr = INVALID_VTTBR;
>> +        d->arch.altp2m_vttbr[i] = INVALID_VTTBR;
>> +
>> +        read_unlock(&p2m->lock);
>
> I would much prefer if the p2m is fully destroyed rather than
> re-initialized.
>

We have discussed this in patch #07. In case of altp2m_reset, I will
still need to simply flush the p2m (without destroying it entirely) but
I will do it in a rather different way as it is implemented right now.
To avoid any inconveniences, I will probably make some suggestions, when
I am at that point of implementation.

>> +    }
>> +
>> +    altp2m_unlock(d);
>> +}
>> +
>>  void altp2m_teardown(struct domain *d)
>>  {
>>      unsigned int i;
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index a33c740..3ba82a8 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -54,9 +54,6 @@ int altp2m_init_by_id(struct domain *d,
>>                        unsigned int idx);
>>
>>  /* Flush all the alternate p2m's for a domain */
>> -static inline void altp2m_flush(struct domain *d)
>> -{
>> -    /* Not yet implemented. */
>> -}
>> +void altp2m_flush(struct domain *d);
>>
>>  #endif /* __ASM_ARM_ALTP2M_H */
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index f13f285..32326cb 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -222,6 +222,9 @@ mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>>  /* Allocates page table for a p2m. */
>>  int p2m_alloc_table(struct p2m_domain *p2m);
>>
>> +/* 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);
>>
>>
>

Best regards,
~Sergej


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