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

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Julien,


On 09/14/2016 12:57 PM, Julien Grall wrote:
>
>
> On 13/09/16 14:00, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hello Sergej,
>
>>
>> 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 :)
>
> Correct.
>

Ok, done.

>>
>>>> +            {
>>>> +                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.
>
> Actually, I was concerned about having the following sequence spread:
>    VTTBR_EL2
>    isb
>
> However, the isb is not necessary here because VTTBR_EL2 will not be
> used until we return to the guest.
>
> So I would be fine to keep WRITE_SYSREG64(ap2m->vttbr, VTTBR_EL2);

I will change that, thank you.

Cheers,
~Sergej


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