Re: [PATCH v3 5/7] xen/riscv: implement data and instruction cache operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2024 17:32, Oleksii Kurochko wrote:
> Implement following cache operations:
> - clean_and_invalidate_dcache_va_range()
> - clean_dcache_va_range()
> - invalidate_icache()
> 
> The first two functions may require support for the CMO (Cache Management
> Operations) extension and/or hardware-specific instructions.
> Currently, only QEMU is supported, which does not model cache behavior.
> Therefore, clean_and_invalidate_dcache_va_range() and clean_dcache_va_range()
> are implemented to simply return 0. For other cases, generate compilation 
> error
> so a user won't miss to update this function if necessery.
> If hardware supports CMO or hardware-specific instructions, these functions
> should be updated accordingly. To support current implementation of these
> function CONFIG_QEMU_PLATFORM is introduced.
> 
> invalidate_icache() is implemented using fence.i instruction as
> mentioned in the unpriv spec:
>   The FENCE.I instruction was designed to support a wide variety of
>   implementations. A simple implementation can flush the local instruction
>   cache and the instruction pipeline when the FENCE.I is executed.
>   A more complex implementation might snoop the instruction (data) cache
>   on every data (instruction) cache miss, or use an inclusive unified
>   private L2 cache to invalidate lines from the primary instruction cache
>   when they are being written by a local store instruction.
>   If instruction and data caches are kept coherent in this way, or if the
>   memory system consists of only uncached RAMs, then just the fetch pipeline
>   needs to be flushed at a FENCE.I.
> The FENCE.I instruction requires the presence of the Zifencei extension,
> which might not always be available. However, Xen uses the RV64G ISA, which
> guarantees the presence of the Zifencei extension. According to the
> unprivileged ISA specification (version 20240411):
>   One goal of the RISC-V project is that it be used as a stable software
>   development target. For this purpose, we define a combination of a base ISA
>   (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr, Zifencei)
>   as a "general-purpose" ISA, and we use the abbreviation G for the
>   IMAFDZicsr_Zifencei combination of instruction-set extensions.
> 
> Set CONFIG_QEMU_PLATFORM=y in tiny64_defconfig to have proper implemtation of
> clean_and_invalidate_dcache_va_range() and clean_dcache_va_range() for CI.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

I'm not entirely happy with this (and where it appears to be moving us), but
for the time being it's perhaps good enough, so
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
Albeit with ...

> @@ -148,9 +149,28 @@ static inline bool pte_is_mapping(pte_t p)
>      return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK);
>  }
>  
> +static inline int clean_and_invalidate_dcache_va_range(const void *p,
> +                                                       unsigned long size)
> +{
> +#ifndef CONFIG_QEMU_PLATFORM
> +    #error "should clean_and_invalidate_dcache_va_range() be updated?"
> +#endif
> +
> +    return 0;
> +}
> +
> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
> +{
> +#ifndef CONFIG_QEMU_PLATFORM
> +    #error "should clean_dcache_va_range() be updated?"
> +#endif
> +
> +    return 0;
> +}

... the #-es moved back to the 1st column, which I'll take the liberty of
doing while committing. Personally I wonder anyway why these aren't simply
BUILD_BUG_ON("unimplemented").

Jan