Re: [Xen-devel] [PATCH v2 5/6] xen/arm: read cacheline size when needed

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 20/02/2018 21:03, Stefano Stabellini wrote:
> On Tue, 20 Feb 2018, Julien Grall wrote:
> > On 19/02/18 21:58, Stefano Stabellini wrote:
> > > +        mrc   CP32(r6, CSSELR_EL1)
> > The size of the cache is read using CSSIDR_EL1. But it looks like the way we
> > get the cache line size in Xen is fragile.
> >
> > We are retrieving the cache line size of Level 1 and assume this will be valid
> > for all the other caches. Indeed cache maintenance ops may propagate to other
> > caches depending the target (Point of Coherency vs Point of Unification).
> >
> > Looking at the ARM ARM "Cache hierarchy abstraction for address-based
> > operations" (D3-2061 DDI 0487C.a), CTR_EL0/CTR will holds the minimum line
> > lenght values for the data caches. The value will be the most efficient
> > address stride to use to apply a sequence of VA-based maintenance instructions
> > to a range of VAs.
> >
> > So it would be best and safer for Xen to use CTR/CTLR_EL0.DminLine.
> This is insightful, thank you. Given that this patch is a backport
> candidate, I would prefer to retain the same behavior we had before in
> setup_cache. I can write a separate patch on top of this to make the
> change to use CTR/CTLR_EL0.DminLine. That way, we can make a separate
> decision on each of them on whether we want to backport them (and
> potentially revert them) or not. In other words: this patch as-is is
> suboptimal but is of very little risk. Making changes to the way we
> determine the cacheline size improves the patch but significantly
> increases the risk factor associated with it.
>
> Does it make sense?
By this patch you mean big.LITTLE? If so, then I don't consider it as a 
potential backport. big.LITTLE has never been supported on Xen and hence 
should be considered as a new feature. What is backportable is the patch 
#1 that forbid big.LITTLE.
Regarding the cache line size, I didn't suggest the change because it is 
more efficient. I suggested the patch because the current code to find 
the cache line size is wrong. Imagine there is a cache in the hierarchy 
that has a smaller cache line than your L1 cache. Then you would not 
clean/invalidate correctly that cache.
> > > +        and   r6, r6, #0x7
> > > +        add   r6, r6, #4
> > > +        mov   r7, #1
> > > +        lsl   r6, r7, r6
> > >            mov   r7, r3
> > >      1:      mcr   CP32(r7, DCCMVAC)
> > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> > > index fa0ef70..edea300 100644
> > > --- a/xen/arch/arm/arm64/head.S
> > > +++ b/xen/arch/arm/arm64/head.S
> > > @@ -631,8 +631,14 @@ ENTRY(relocate_xen)
> > >            dsb   sy        /* So the CPU issues all writes to the range */
> > >              mov   x9, x3
> > > -        ldr   x10, =cacheline_bytes /* x10 := step */
> > > -        ldr   x10, [x10]
> > > +
> > > +        mov   x10, #0
> > > +        msr   CSSELR_EL1, x10
> > > +        mrs   x10, CSSELR_EL1
> > > +        and   x10, x10, #0x7
> > > +        add   x10, x10, #4
> > > +        mov   x11, #1
> > > +        lsl   x10, x11, x10
> > Please use dcache_line_size macro (see cache.S).
> Similarly, I would prefer to retain the same old behavior here, and
> fix it/improve it in a separate patch.
See above, you got the wrong end of the stick about the cache line size.

Cheers,

--
Julien Grall

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