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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> + *
> + * 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.
> +       /* 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).
> +
> +       /* clean and invalidate I cache by I/D cache line size */
> +       ic      ivau, x0
> +       dsb     ish
Ditto.

> +
> +       /* 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