Re: [Xen-devel] [PATCH v3 18/38] arm/p2m: Add HVMOP_altp2m_destroy_p2m

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 09/12/2016 10:41 AM, Julien Grall wrote:
> Hello Sergej,
>
> On 16/08/2016 23:16, Sergej Proskurin wrote:
>> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> v2: Substituted the call to tlb_flush for p2m_flush_table.
>>     Added comments.
>>     Cosmetic fixes.
>>
>> v3: Changed the locking mechanism to "p2m_write_lock" inside the
>>     function "altp2m_destroy_by_id".
>>
>>     Do not flush but rather teardown the altp2m in the function
>>     "altp2m_destroy_by_id".
>>
>>     Exchanged the check "altp2m_vttbr[idx] == INVALID_VTTBR" for
>>     "altp2m_p2m[idx] == NULL" in "altp2m_destroy_by_id".
>> ---
>>  xen/arch/arm/altp2m.c        | 43
>> +++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/hvm.c           |  2 +-
>>  xen/include/asm-arm/altp2m.h |  4 ++++
>>  3 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index b5d1951..c14ab0b 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -190,6 +190,49 @@ void altp2m_flush(struct domain *d)
>>      altp2m_unlock(d);
>>  }
>>
>> +int altp2m_destroy_by_id(struct domain *d, unsigned int idx)
>> +{
>> +    struct p2m_domain *p2m;
>> +    int rc = -EBUSY;
>> +
>> +    /*
>> +     * The altp2m[0] is considered as the hostp2m and is used as a
>> safe harbor
>> +     * to which you can switch as long as altp2m is active. After
>> deactivating
>> +     * altp2m, the system switches back to the original hostp2m
>> view. That is,
>> +     * altp2m[0] should only be destroyed/flushed/freed, when altp2m is
>> +     * deactivated.
>> +     */
>> +    if ( !idx || idx >= MAX_ALTP2M )
>> +        return rc;
>> +
>> +    domain_pause_except_self(d);
>> +
>> +    altp2m_lock(d);
>> +
>> +    if ( d->arch.altp2m_p2m[idx] != NULL )
>> +    {
>> +        p2m = d->arch.altp2m_p2m[idx];
>> +
>> +        if ( !_atomic_read(p2m->active_vcpus) )
>> +        {
>> +            p2m_write_lock(p2m);
>
> I am not sure why you take the lock here. It will not protect you from
> anything as the p2m will get freed just after. So if someone else is
> using it, it will be in big trouble.
>

I will remove the lock in the next patch, thank you.

>> +            p2m_teardown_one(p2m);
>> +            p2m_write_unlock(p2m);
>> +
>> +            xfree(p2m);
>> +            d->arch.altp2m_p2m[idx] = NULL;
>> +
>> +            rc = 0;
>> +        }
>> +    }
>> +
>> +    altp2m_unlock(d);
>> +
>> +    domain_unpause_except_self(d);
>> +
>> +    return rc;
>> +}
>> +
>>  void altp2m_teardown(struct domain *d)
>>  {
>>      unsigned int i;
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index a504dfd..df973ef 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -128,7 +128,7 @@ static int
>> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>
>>      case HVMOP_altp2m_destroy_p2m:
>> -        rc = -EOPNOTSUPP;
>> +        rc = altp2m_destroy_by_id(d, a.u.view.view);
>>          break;
>>
>>      case HVMOP_altp2m_switch_p2m:
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index 5701012..6074079 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -63,4 +63,8 @@ int altp2m_init_next_available(struct domain *d,
>>  /* Flush all the alternate p2m's for a domain. */
>>  void altp2m_flush(struct domain *d);
>>
>> +/* Make a specific alternate p2m invalid */
>> +int altp2m_destroy_by_id(struct domain *d,
>> +                         unsigned int idx);
>> +
>>  #endif /* __ASM_ARM_ALTP2M_H */
>>

Cheers,
~Sergej

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