Re: [PATCH v3 15/38] arm/p2m: Add altp2m table flushing routine

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 09/09/2016 07:02 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 16/08/16 23:16, Sergej Proskurin wrote:
>> The current implementation differentiates between flushing and
>> destroying altp2m views. This commit adds the function altp2m_flush,
>> which allows to release all of the alternate p2m views.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>> v2: Pages in p2m->pages are not cleared in p2m_flush_table anymore.
>>     VMID is freed in p2m_free_one.
>>     Cosmetic fixes.
>>
>> v3: Changed the locking mechanism to "p2m_write_lock" inside the
>>     function "altp2m_flush".
>>
>>     Do not flush but rather teardown the altp2m in the function
>>     "altp2m_flush".
>>
>>     Exchanged the check "altp2m_vttbr[idx] == INVALID_VTTBR" for
>>     "altp2m_p2m[idx] == NULL" in "altp2m_flush".
>> ---
>>  xen/arch/arm/altp2m.c        | 31 +++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/altp2m.h |  3 +++
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index 66a373a..02cffd7 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -34,6 +34,37 @@ 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));
>> +
>> +    altp2m_lock(d);
>> +
>> +    for ( i = 0; i < MAX_ALTP2M; i++ )
>> +    {
>> +        if ( d->arch.altp2m_p2m[i] == NULL )
>> +            continue;
>> +
>> +        p2m = d->arch.altp2m_p2m[i];
>> +
>> +        p2m_write_lock(p2m);
>
> Why do you take the lock here? The p2m should not be used by anyone at
> that time.

Good point. I will not acquire the lock at this point. Thank you.

>
>> +        p2m_teardown_one(p2m);
>
> You may want to add an ASSERT(!atomic_read(p2m->active_vcpus)) somewhere.

Ok.

>
>> +        p2m_write_unlock(p2m);
>> +
>> +        xfree(p2m);
>> +        d->arch.altp2m_p2m[i] = NULL;
>> +    }
>> +
>> +    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 a156109..4c15b75 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -42,4 +42,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct
>> vcpu *v)
>>  int altp2m_init(struct domain *d);
>>  void altp2m_teardown(struct domain *d);
>>
>> +/* Flush all the alternate p2m's for a domain. */
>> +void altp2m_flush(struct domain *d);
>> +
>>  #endif /* __ASM_ARM_ALTP2M_H */
>>

Best regards,
~Sergej

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