Re: [PATCH 04/36] xen/arm: add parsing function for cache coloring configuration

[Julien Grall <julien@xxxxxxx>]

On 13/05/2022 15:22, Carlo Nonato wrote:
> Hi Julien,
Hi Carlo,

> I'm Carlo, the new developer that will work on this patch set and on the 
> review.
> Thanks for all the comments. I'll try to answer to all the open points 
> and also
> ask for feedback.
>
> On 09/03/22 20:09, Julien Grall wrote:
> > > - way_size: The size of a LLC way in bytes. This value is mainly used
> > >    to calculate the maximum available colors on the platform.
> > We should only add command line option when they are a strong use 
> > case. In documentation, you wrote that someone may want to overwrite 
> > the way size for "specific needs".
> > Can you explain what would be those needs?
> This parameter is here mainly to support QEMU on which the automatic 
> probing
> of the LLC size doesn't work properly.
I am not in favor of adding command line option just for QEMU. But...

> Also, since from this value we compute the maximum number of colors
> the architecture supports, you may want to fix the way size so as to 
> simulate
> a different use case for debugging purposes.
... this reason is more compelling to me.

> Should I add those notes somewhere (doc, commit messages, etc.)?
So I would mention it in the commit message and also the doc description 
the options.
> > > A cache coloring configuration consists of a selection of colors to be
> > > assigned to a VM or to the hypervisor. It is represented by a set of
> > > ranges. Add a common function that parses a string with a
> > > comma-separated set of hyphen-separated ranges like "0-7,15-16" and
> > > returns both: the number of chosen colors, and an array containing their
> > > ids.
> > > Currently we support platforms with up to 128 colors.
> > Is there any reason this value is hardcoded in Xen rather than part of 
> > the Kconfig?
> Having another parameter to configure can complicate things from
> the user perspective. 
I don't think it would be more complicated. The default would still be 
128 and would help the user to easily modify the value if...
> Also 128 is more than enough for the current ARM
> processors we tested.
... they are using a processor you didn't tested on.

> > > Signed-off-by: Luca Miccio <lucmiccio@xxxxxxxxx>
> > > Signed-off-by: Marco Solieri <marco.solieri@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > > ---
> > >   xen/arch/arm/Kconfig                |   5 ++
> > >   xen/arch/arm/Makefile               |   2 +-
> > >   xen/arch/arm/coloring.c             | 131 ++++++++++++++++++++++++++++
> > >   xen/arch/arm/include/asm/coloring.h |  28 ++++++
> > >   4 files changed, 165 insertions(+), 1 deletion(-)
> > >   create mode 100644 xen/arch/arm/coloring.c
> > >   create mode 100644 xen/arch/arm/include/asm/coloring.h
> > >
> > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > index ecfa6822e4..f0f999d172 100644
> > > --- a/xen/arch/arm/Kconfig
> > > +++ b/xen/arch/arm/Kconfig
> > > @@ -97,6 +97,11 @@ config HARDEN_BRANCH_PREDICTOR
> > >           If unsure, say Y.
> > >   +config COLORING
> > > +    bool "L2 cache coloring"
> > > +    default n
> > This wants to be gated with EXPERT for time-being. SUPPORT.MD woudl
> > Furthermore, I think this wants to be gated with EXPERT for the 
> > time-being.
> > > +    depends on ARM_64
> > Why is this limited to arm64?
> Because arm32 isn't an "interesting" architecture where to have coloring
> since there are locking primitives that provides sufficient isolation 
> and so
> the problem is not common.
I am afraid I don't understand this rationale. What sort of locking are 
you talking about?
That said,I am not asking to implement the 32-bit side. I am more 
interested to know what's the effort required here. IOW, is it disabled 
because you haven't tested?
Cheers,

--
Julien Grall