Re: [PATCH v2 12/25] arm/altp2m: Add HVMOP_altp2m_switch_p2m.

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

Hi Julien,


On 08/04/2016 01:51 PM, Julien Grall wrote:
> Hello Sergej,
>
> On 01/08/16 18:10, Sergej Proskurin wrote:
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/altp2m.c        | 32 ++++++++++++++++++++++++++++++++
>>  xen/arch/arm/hvm.c           |  2 +-
>>  xen/include/asm-arm/altp2m.h |  4 ++++
>>  3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index 80ed553..7404f42 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -33,6 +33,38 @@ struct p2m_domain *altp2m_get_altp2m(struct vcpu *v)
>>      return v->domain->arch.altp2m_p2m[index];
>>  }
>>
>> +int altp2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int
>> idx)
>> +{
>> +    struct vcpu *v;
>> +    int rc = -EINVAL;
>> +
>> +    if ( idx >= MAX_ALTP2M )
>> +        return rc;
>> +
>> +    domain_pause_except_self(d);
>> +
>> +    altp2m_lock(d);
>> +
>> +    if ( d->arch.altp2m_vttbr[idx] != INVALID_VTTBR )
>> +    {
>> +        for_each_vcpu( d, v )
>> +            if ( idx != vcpu_altp2m(v).p2midx )
>> +            {
>> +                atomic_dec(&altp2m_get_altp2m(v)->active_vcpus);
>> +                vcpu_altp2m(v).p2midx = idx;
>> +                atomic_inc(&altp2m_get_altp2m(v)->active_vcpus);
>> +            }
>> +
>> +        rc = 0;
>> +    }
>
> If a domain is calling the function on itself, the current vCPU will
> not switch to the new altp2m because the VTTBR is only restore during
> context switch.

Is this really the case that a switch from a guest to Xen could result
in no context switch at all? I just saw the asserts in context_switch:

[...]
ASSERT(prev != next);
[...]

However, I thought that every trap/hypercall to Xen would eventually
restore the VCPU's state. In this case, I will need to directly write to
VTTBR_EL2. Thank you.

>
> However, I am not sure why you store a p2midx rather than directly a
> pointer to the p2m.
>

Again, this idea has been also adopted from the x86 implementation. And
I really do not see any reason to change that. Do you?

>> +
>> +    altp2m_unlock(d);
>> +
>> +    domain_unpause_except_self(d);
>> +
>> +    return rc;
>> +}
>> +
>>  static void altp2m_vcpu_reset(struct vcpu *v)
>>  {
>>      struct altp2mvcpu *av = &vcpu_altp2m(v);
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index df29cdc..3b508df 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -129,7 +129,7 @@ static int
>> do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>>          break;
>>
>>      case HVMOP_altp2m_switch_p2m:
>> -        rc = -EOPNOTSUPP;
>> +        rc = altp2m_switch_domain_altp2m_by_id(d, a.u.view.view);
>>          break;
>>
>>      case HVMOP_altp2m_set_mem_access:
>> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
>> index afa1580..790bb33 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -49,6 +49,10 @@ void altp2m_teardown(struct domain *d);
>>  void altp2m_vcpu_initialise(struct vcpu *v);
>>  void altp2m_vcpu_destroy(struct vcpu *v);
>>
>> +/* Switch alternate p2m for entire domain */
>> +int altp2m_switch_domain_altp2m_by_id(struct domain *d,
>> +                                      unsigned int idx);
>> +
>>  /* Make a specific alternate p2m valid. */
>>  int altp2m_init_by_id(struct domain *d,
>>                        unsigned int idx);
>>
>

Best regards,
~Sergej


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