Re: [Xen-devel] [PATCH v3 19/38] arm/p2m: Add HVMOP_altp2m_switch_p2m

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 09/12/2016 10:47 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>
>> ---
>> v3: Extended the function "altp2m_switch_domain_altp2m_by_id" so that if
>>     the guest domain indirectly calles this function, the current
>> vcpu also
>>     changes the altp2m view without performing an explicit context
>> switch.
>>
>>     Exchanged the check "altp2m_vttbr[idx] == INVALID_VTTBR" for
>>     "altp2m_p2m[idx] == NULL" in "altp2m_switch_domain_altp2m_by_id".
>> ---
>>  xen/arch/arm/altp2m.c        | 48
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/hvm.c           |  2 +-
>>  xen/include/asm-arm/altp2m.h |  4 ++++
>>  3 files changed, 53 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
>> index c14ab0b..ba345b9 100644
>> --- a/xen/arch/arm/altp2m.c
>> +++ b/xen/arch/arm/altp2m.c
>> @@ -32,6 +32,54 @@ 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_p2m[idx] != NULL )
>> +    {
>> +        for_each_vcpu( d, v )
>> +            if ( idx != altp2m_vcpu(v).p2midx )
>
> Could you invert the condition to avoid another layer of nested if?
>

Did you mean checking for (idx == altp2m_vcpu(v).p2midx) and continue
the loop if the condition should be satisfied? If so, then sure thing :)

>> +            {
>> +                atomic_dec(&altp2m_get_altp2m(v)->active_vcpus);
>> +                altp2m_vcpu(v).p2midx = idx;
>> +                atomic_inc(&altp2m_get_altp2m(v)->active_vcpus);
>> +
>> +                /*
>> +                 * In case it is the guest domain, which indirectly
>> called this
>> +                 * function, the current vcpu will not switch its
>> context
>> +                 * within the function "p2m_restore_state". That is,
>> changing
>> +                 * the altp2m_vcpu(v).p2midx will not lead to the
>> requested
>> +                 * altp2m switch on that specific vcpu. To achieve
>> the desired
>> +                 * behavior, we write to VTTBR_EL2 directly.
>> +                 */
>> +                if ( v->domain == d && v == current )
>
> v == current is enough.
>

Ok.

>> +                {
>> +                    struct p2m_domain *ap2m = d->arch.altp2m_p2m[idx];
>> +
>> +                    WRITE_SYSREG64(ap2m->vttbr, VTTBR_EL2);
>> +                    isb();
>
> I don't like the open-coding of VTTBR_EL2. I would much prefer a
> separate helper to update it.
>

Sure, I can introduce an UPDATE_VTTBR macro (including the isb call) in
p2m.h and adjust the remaining writes to VTTBR_EL2 in p2m.c as well. I
hope, I understood you correctly.

>> +                }
>> +            }
>> +
>> +        rc = 0;
>> +    }
>> +
>> +    altp2m_unlock(d);
>> +
>> +    domain_unpause_except_self(d);
>> +
>> +    return rc;
>> +}
>> +
>>  static void altp2m_vcpu_reset(struct vcpu *v)
>>  {
>>      struct altp2mvcpu *av = &altp2m_vcpu(v);
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index df973ef..9ac3422 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -132,7 +132,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 6074079..c2e44ab 100644
>> --- a/xen/include/asm-arm/altp2m.h
>> +++ b/xen/include/asm-arm/altp2m.h
>> @@ -52,6 +52,10 @@ void altp2m_vcpu_destroy(struct vcpu *v);
>>  /* Get current alternate p2m table. */
>>  struct p2m_domain *altp2m_get_altp2m(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);
>>

Cheers,
~Sergej


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