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

[Julien Grall <julien.grall@xxxxxxx>]

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.

> > > +            {
> > > +                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);

Regards,

--
Julien Grall

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