[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] xen: arm: force guest memory accesses to cacheable when MMU is disabled



On Tue, 2014-01-07 at 16:59 +0000, Julien Grall wrote:
> On 01/07/2014 04:02 PM, Ian Campbell wrote:
> .>
> > One thing I'm not sure about is reverting the previous fix in a0035ecc0d82.
> > It's reasonably recent so reverting it takes us back to a pretty well
> > understood state in the libraries. The functionality is harmless if
> > incomplete. I think given the first argument I would lean towards reverting.
> 
> I would also prefer reverting the previous patch.
> 
> > 
> > Obviously if you think I'm being to easy on myself please say so.
> 
> Without this patch, we will likely crash a guest in production, that
> it's not acceptable for a release.
> 
> > 
> > Actually, if you think my judgement is right I'd appreciate being told so 
> > too.
> > ---
> >  xen/arch/arm/domain.c           |    5 ++
> >  xen/arch/arm/domain_build.c     |    1 +
> >  xen/arch/arm/traps.c            |  153 
> > ++++++++++++++++++++++++++++++++++++++-
> >  xen/arch/arm/vtimer.c           |    6 +-
> >  xen/include/asm-arm/cpregs.h    |    4 +
> >  xen/include/asm-arm/processor.h |    2 +-
> >  xen/include/asm-arm/sysregs.h   |   19 ++++-
> >  7 files changed, 182 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 124cccf..104d228 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -219,6 +219,11 @@ static void ctxt_switch_to(struct vcpu *n)
> >      else
> >          hcr |= HCR_RW;
> >  
> > +    if ( n->arch.sctlr & SCTLR_M )
> > +        hcr &= ~(HCR_TVM|HCR_DC);
> > +    else
> > +        hcr |= (HCR_TVM|HCR_DC);
> > +
> >      WRITE_SYSREG(hcr, HCR_EL2);
> >      isb();
> >  
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 47b781b..bb31db8 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1026,6 +1026,7 @@ int construct_dom0(struct domain *d)
> >      else
> >          WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_RW, HCR_EL2);
> >  #endif
> > +    WRITE_SYSREG(READ_SYSREG(HCR_EL2) | HCR_DC | HCR_TVM, HCR_EL2);
> 
> Is it useful? As I understand, we will at least context switch one time
> before booting dom0.

For some reason I was thinking that the initial context became d0v0 when
actually it becomes the first idle vcpu. Unlike HCR_RW etc I don't think
anything in the dom0 build code relies on this bit being correct, so I
think you are right that it can be removed.

> If we need it, perhaps the better place to setup it is init_traps?

This may not be quite true today, but: I think it would be good for
init_traps to setup the global/constant HCR settings and leave the
per-VM ones to more per-VM locations.

> 
> >  
> >      /*
> >       * kernel_load will determine the placement of the initrd & fdt in
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 7c5ab19..d00bba3 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1279,6 +1279,23 @@ static void advance_pc(struct cpu_user_regs *regs, 
> > union hsr hsr)
> >      regs->pc += hsr.len ? 4 : 2;
> >  }
> >  
> > +static void update_sctlr(uint32_t v)
> > +{
> > +    /*
> > +     * If MMU (SCTLR_M) is now enabled then we must disable HCR.DC
> > +     * because they are incompatible.
> > +     *
> > +     * Once HCR.DC is disabled then we do not need HCR_TVM either,
> > +     * since it's only purpose was to catch the MMU being enabled.
> > +     *
> > +     * Both are set appropriately on context switch but we need to
> > +     * clear them now since we may not context switch on return to
> > +     * guest.
> > +     */
> > +    if ( v & SCTLR_M )
> > +        WRITE_SYSREG(READ_SYSREG(HCR_EL2) & ~(HCR_DC|HCR_TVM), HCR_EL2);
> 
> Even if it's unlikely, can we handle the case where the guest disable
> the case?

No, we disable the traps so we would never find out that the guest had
done so (at least, not without waiting for a context switch). We don't
really mind this shortcoming though because we expect guests to enable
caches early on and keep them on (in some sense this is part of our ABI)
but in any case if the guest were to disable its MMU it would have to
have taken care of making the caches consistent itself already (e.g. it
is required on native).

This does make me think that the code in ctxt switch is wrong though,
since if the guest does set SCTLR.M then we will enable HCR.DC at that
point, without necessarily doing everything which would be needed. I
think I'll add a per-vcpu flag which indicates whether DC should be set
and clear it in this function.


> Also from ARM ARM B3.2.1, a TLB flush by VMID is required if HCR_DC is
> disabled and the VMID is not changed.

Oh yes, well spotted, will add that.

Ian.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.