Re: [Minios-devel] [UNIKRAFT PATCHv6 07/37] plat/common: Add cache maintenance support for arm64

["Wei Chen (Arm Technology China)" <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年9月16日 1:58
> To: Wei Chen (Arm Technology China) <Wei.Chen@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv6 07/37] plat/common: Add cache
> maintenance support for arm64
> 
> Hi Wei,
> 
> On 09/14/2018 08:56 AM, Wei Chen wrote:
> > When MMU is disabled, when we modify some data, for example, write
> > page-table, we will write with Device nGnRnE attributes. So the
> > cache will be bypassed. But the cache may still contain stall data
> > that we will hit when enabling MMU and cache. To prevent such issue,
> > we need to clean the cache potentially before and after updating
> > the page-table area.
> >
> > So we introduce the cache maintenance functions in this patch.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   plat/common/arm/cache64.S                | 85 ++++++++++++++++++++++++
> >   plat/common/include/arm/arm64/cpu_defs.h | 46 +++++++++++++
> >   plat/common/include/arm/cpu_defs.h       | 44 ++++++++++++
> >   plat/kvm/Makefile.uk                     |  1 +
> >   4 files changed, 176 insertions(+)
> >   create mode 100644 plat/common/arm/cache64.S
> >   create mode 100644 plat/common/include/arm/arm64/cpu_defs.h
> >   create mode 100644 plat/common/include/arm/cpu_defs.h
> >
> > diff --git a/plat/common/arm/cache64.S b/plat/common/arm/cache64.S
> > new file mode 100644
> > index 0000000..a725557
> > --- /dev/null
> > +++ b/plat/common/arm/cache64.S
> > @@ -0,0 +1,85 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause */
> > +/*
> > + * Authors: Wei Chen <wei.chen@xxxxxxx>
> > + *
> > + * Copyright (c) 2014 Robin Randhawa
> > + * Copyright (c) 2015 The FreeBSD Foundation
> > + * All rights reserved.
> > + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> 
>  From the copyright, I suspect the code is from FreeBSD. However, I
> can't find anything close to it in the source. Did I miss anything?
> 
> In general, it is a good idea to mention where you code comes from and
> the commit used in the commit message. This helps to check whether code
> has been updated afterwards.

Oh, sorry, I missed this file. Simon had asked me to put some source code
Link to the copyright header, but I still missed this file : (
This source code is based on:
https://github.com/freebsd/freebsd/blob/master/sys/arm64/arm64/cpufunc_asm.S

> 
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + * 3. Neither the name of the copyright holder nor the names of its
> > + *    contributors may be used to endorse or promote products derived from
> > + *    this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> > + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> PURPOSE
> > + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS
> BE
> > + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> > + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> THE
> > + * POSSIBILITY OF SUCH DAMAGE.
> > + *
> > + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > + */
> > +#include <uk/asm.h>
> > +#include <arm/cpu_defs.h>
> > +
> > +/*
> > + * Function to invalidate I/D cache. This takes the start address in x0,
> > + * length in x1. It will corrupt x0 ~ x5.
> > + */
> > +ENTRY(invalidate_idcache_range)
> 
> IHMO, the name is misleading. You don't only invalidate the cache, you
> clean it as well.
> 
> But I am a bit confused of the purpose of the function in the context
> you are using it. Why do you need to invalidate the I-Cache? The boot
> code will not rewrite itself, so the I-Cache will not need an invalidation.
> 

Yes, you're right, we don’t have any relocation operation for code. Invalidate
D-Cache is enough. I will send a patch to fix it.

> > +   /* Get information about the caches from CTR_EL0 */
> > +   mrs     x3, ctr_el0
> > +   mov     x2, #CTR_BYTES_PER_WORD
> > +
> > +   /* Get minimum D cache line size */
> > +   ubfx    x4, x3, #CTR_DMINLINE_SHIFT, #CTR_DMINLINE_WIDTH
> > +   lsl     x4, x2, x4
> > +
> > +   /* Get minimum I cache line size */
> > +   and     x5, x3, #CTR_IMINLINE_MASK
> > +   lsl     x5, x2, x5
> > +
> > +   /* Select the smaller one as I/D cache line size */
> > +   cmp     x5, x4
> > +   csel    x3, x5, x4, le
> > +
> > +   /* Align the start address to line size */
> > +   sub     x4, x3, #1
> > +   and     x2, x0, x4
> > +   add     x1, x1, x2
> > +   bic     x0, x0, x4
> > +1:
> > +   /* clean and invalidate D cache by I/D cache line size */
> > +   dc      civac, x0
> > +   dsb     ish
> 
> This looks incorrect given you use it before the MMU is on. I think you
> want to use nsh here (if not sy).
> 

Emm, Yes. I will fix.

> > +
> > +   /* clean and invalidate I cache by I/D cache line size */
> > +   ic      ivau, x0
> > +   dsb     ish
> 
> Ditto.
> 

Ok,

> > +
> > +   /* Move to next line and reduce the size */
> > +   add     x0, x0, x3
> > +   subs    x1, x1, x3
> > +
> > +   /* Check if all range has been invalidated */
> > +   b.hi    1b
> > +
> > +   isb
> > +
> > +   ret
> > +END(invalidate_idcache_range)
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel