Re: [PATCH v6 7/9] xen/riscv: introduce and initialize SBI RFENCE extension

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2024 19:01, Oleksii Kurochko wrote:
> Introduce functions to work with the SBI RFENCE extension for issuing
> various fence operations to remote CPUs.
> 
> Add the sbi_init() function along with auxiliary functions and macro
> definitions for proper initialization and checking the availability of
> SBI extensions. Currently, this is implemented only for RFENCE.
> 
> Introduce sbi_remote_sfence_vma() to send SFENCE_VMA instructions to
> a set of target HARTs. This will support the implementation of
> flush_xen_tlb_range_va().
> 
> Integrate __sbi_rfence_v02 from Linux kernel 6.6.0-rc4 with minimal
> modifications:
>  - Adapt to Xen code style.
>  - Use cpuid_to_hartid() instead of cpuid_to_hartid_map[].
>  - Update BIT(...) to BIT(..., UL).
>  - Rename __sbi_rfence_v02_call to sbi_rfence_v02_real and
>    remove the unused arg5.
>  - Handle NULL cpu_mask to execute rfence on all CPUs by calling
>    sbi_rfence_v02_real(..., 0UL, -1UL,...) instead of creating hmask.
>  - change type for start_addr and size to vaddr_t and size_t.
>  - Add an explanatory comment about when batching can and cannot occur,
>    and why batching happens in the first place.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
with three more cosmetic things taken care of:

> +static long sbi_ext_base_func(long fid)
> +{
> +    struct sbiret ret;
> +
> +    ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> +
> +    if ( !ret.error )
> +    {
> +       /*
> +        * I wasn't able to find a case in the SBI spec where sbiret.value
> +        * could be negative.
> +        *
> +        * Unfortunately, the spec does not specify the possible values of
> +        * sbiret.value, but based on the description of the SBI function,
> +        * ret.value >= 0 when sbiret.error = 0. SPI spec specify only
> +        * possible value for sbiret.error (<= 0 whwere 0 is SBI_SUCCESS ).
> +        *
> +        * Just to be sure that SBI base extension functions one day won't
> +        * start to return a negative value for sbiret.value when
> +        * sbiret.error < 0 BUG_ON() is added.
> +        */

The entire comment's indentation is off by one.

> +static int cf_check sbi_rfence_v02(unsigned long fid,
> +                                   const cpumask_t *cpu_mask,
> +                                   vaddr_t start, size_t size,
> +                                   unsigned long arg4, unsigned long arg5)
> +{
> +    unsigned long hartid, cpuid, hmask = 0, hbase = 0, htop = 0;
> +    int result = -EINVAL;
> +
> +    /*
> +     * hart_mask_base can be set to -1 to indicate that hart_mask can be
> +     * ignored and all available harts must be considered.
> +     */
> +    if ( !cpu_mask )
> +        return sbi_rfence_v02_real(fid, 0UL, -1UL, start, size, arg4);
> +
> +    for_each_cpu ( cpuid, cpu_mask )
> +    {
> +        /*
> +        * Hart IDs might not necessarily be numbered contiguously in
> +        * a multiprocessor system.
> +        *
> +        * This means that it is possible for the hart ID mapping to look 
> like:
> +        *  0, 1, 3, 65, 66, 69
> +        * In such cases, more than one call to sbi_rfence_v02_real() will be
> +        * needed, as a single hmask can only cover sizeof(unsigned long) 
> CPUs:
> +        *  1. sbi_rfence_v02_real(hmask=0b1011, hbase=0)
> +        *  2. sbi_rfence_v02_real(hmask=0b1011, hbase=65)
> +        *
> +        * The algorithm below tries to batch as many harts as possible before
> +        * making an SBI call. However, batching may not always be possible.
> +        * For example, consider the hart ID mapping:
> +        *   0, 64, 1, 65, 2, 66 (1)
> +        *
> +        * Generally, batching is also possible for (1):
> +        *    First (0,1,2), then (64,65,66).
> +        * It just requires a different approach and updates to the current
> +        * algorithm.
> +        */

Except for the initial line, the entire comment's indentation is off by
one.

> +int sbi_remote_sfence_vma(const cpumask_t *cpu_mask,
> +                          vaddr_t start,
> +                          size_t size)

Elsewhere you put multiple parameters on a line when they fit.

Jan