Re: [Xen-devel] [RFC 15/16] xen/arm: Implement Set/Way operations

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 5 Nov 2018, Julien Grall wrote:
> > > +void p2m_set_way_flush(struct vcpu *v)
> > > +{
> > > +    /* This function can only work with the current vCPU. */
> > > +    ASSERT(v == current);
> > 
> > NIT: if it can only operate on current, it makes sense to remove the
> > struct vcpu* parameter
> 
> I prefer to keep struct vcpu *v here. This makes more straightforward to know
> on what the function is working on.
> 
> Furthermore, current is actually quite expensive to use in some circumstance.
> 
> For instance, in nested case, TPIDR_EL2 will get trapped to the host
> hypervisor.
> 
> So it would be best if we start avoid current whenever it is possible.

That's OK


> > 
> > 
> > > +    if ( !(v->arch.hcr_el2 & HCR_TVM) )
> > > +    {
> > > +        p2m_flush_vm(v);
> > > +        vcpu_hcr_set_flags(v, HCR_TVM);
> > > +    }
> > > +}
> > > +
> > > +void p2m_toggle_cache(struct vcpu *v, bool was_enabled)
> > > +{
> > > +    bool now_enabled = vcpu_has_cache_enabled(v);
> > > +
> > > +    /* This function can only work with the current vCPU. */
> > > +    ASSERT(v == current);
> > 
> > NIT: same about struct vcpu* as parameter when only current can be used
> > 
> > 
> > > +    /*
> > > +     * If switching the MMU+caches on, need to invalidate the caches.
> > > +     * If switching it off, need to clean the caches.
> > > +     * Clean + invalidate does the trick always.
> > > +     */
> > > +    if ( was_enabled != now_enabled )
> > > +        p2m_flush_vm(v);
> > > +
> > > +    /* Caches are now on, stop trapping VM ops (until a S/W op) */
> > > +    if ( now_enabled )
> > > +        vcpu_hcr_clear_flags(v, HCR_TVM);
> > > +}
> > > +
> > >   mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
> > >   {
> > >       return p2m_lookup(d, gfn, NULL);
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index 169b57cb6b..cdc10eee5a 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -98,7 +98,7 @@ register_t get_default_hcr_flags(void)
> > >   {
> > >       return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
> > >                (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
> > > -             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
> > > +             HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB|HCR_TSW);
> > >   }
> > >     static enum {
> > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> > > index 49529b97cd..dc46d9d0d7 100644
> > > --- a/xen/arch/arm/vcpreg.c
> > > +++ b/xen/arch/arm/vcpreg.c
> > > @@ -45,9 +45,14 @@
> > >   #define TVM_REG(sz, func, reg...)
> > > \
> > >   static bool func(struct cpu_user_regs *regs, uint##sz##_t *r, bool read)
> > > \
> > >   {
> > > \
> > > +    struct vcpu *v = current;
> > > \
> > > +    bool cache_enabled = vcpu_has_cache_enabled(v);
> > > \
> > > +
> > > \
> > >       GUEST_BUG_ON(read);
> > > \
> > >       WRITE_SYSREG##sz(*r, reg);
> > > \
> > >                                                                           
> > >     \
> > > +    p2m_toggle_cache(v, cache_enabled);
> > > \
> > 
> > This will affect all the registers trapped with TVM. Shouldn't we only
> > call p2m_toggle_cache when relevant? i.e. when changing SCTLR?
> > I think it would be better to only modify the SCTLR emulation handler.
> 
> This is made on purpose, you increase the chance to disable TVM as soon as
> possible. If you only rely on SCTLR, you may end up to trap a lot of registers
> for a long time.
> 
> FWIW, as I already wrote in the commit message, this is based on what KVM
> does.

I missed that. As you explain it, it makes sense. Maybe worth adding an
explicit statement about it: "On ARM64, we call p2m_toggle_cache from on
the TVM-trapped register handlers to increase the chances of disabling
TVM as soon as possible."

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel