Re: [Xen-devel] [PATCH v2 11/25] arm/altp2m: Add HVMOP_altp2m_destroy_p2m.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 08/04/2016 01:46 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, 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.
>> ---
>>  xen/arch/arm/altp2m.c        | 50
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/hvm.c           |  2 +-
>>  xen/include/asm-arm/altp2m.h |  4 ++++
>>  3 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index c22d2e4..80ed553 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -212,6 +212,56 @@ 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_vttbr[idx] != INVALID_VTTBR )
>> +    {
>> +        p2m = d->arch.altp2m_p2m[idx];
>> +
>> +        if ( !_atomic_read(p2m->active_vcpus) )
>> +        {
>> +            read_lock(&p2m->lock);
>
> Please avoid to use open read_lock and use the p2m_*lock helpers. Also
> read lock does not prevent multiple thread to access the p2m at the
> same time.
>

Thank you very much.

>> +
>> +            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[idx] = INVALID_VTTBR;
>> +
>> +            read_unlock(&p2m->lock);
>
> Why did you decide to reset the p2m rather than free it? The code
> would be simpler with the latter.
>

* First, to be as similar to the x86 implementation as possible.
* Second, simply to reuse already allocated p2m's without additional
overhead.

If you should not agree on this choice, I could adapt the implementation
accordingly.

>> +
>> +            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 063a06b..df29cdc 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -125,7 +125,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 3ecae27..afa1580 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -60,4 +60,8 @@ int altp2m_init_next(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 */
>>
>

Best regards,
~Sergej


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