Re: [PATCH v3 7/9] xen/riscv: introduce and init SBI RFENCE extension

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2024 17:31, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/sbi.h
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -14,6 +14,38 @@
>  
>  #define SBI_EXT_0_1_CONSOLE_PUTCHAR          0x1
>  
> +#define SBI_EXT_BASE                    0x10
> +#define SBI_EXT_RFENCE                  0x52464E43
> +
> +/* SBI function IDs for BASE extension */
> +#define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
> +#define SBI_EXT_BASE_GET_IMP_ID         0x1
> +#define SBI_EXT_BASE_GET_IMP_VERSION    0x2
> +#define SBI_EXT_BASE_PROBE_EXT          0x3
> +
> +/* SBI function IDs for RFENCE extension */
> +#define SBI_EXT_RFENCE_REMOTE_FENCE_I           0x0
> +#define SBI_EXT_RFENCE_REMOTE_SFENCE_VMA        0x1
> +#define SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID   0x2
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA       0x3
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID  0x4
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA       0x5
> +#define SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID  0x6
> +
> +#define SBI_SPEC_VERSION_MAJOR_SHIFT    24
> +#define SBI_SPEC_VERSION_MAJOR_MASK     0x7f
> +#define SBI_SPEC_VERSION_MINOR_MASK     0xffffff
> +
> +/* SBI return error codes */
> +#define SBI_SUCCESS             0
> +#define SBI_ERR_FAILURE         -1
> +#define SBI_ERR_NOT_SUPPORTED   -2
> +#define SBI_ERR_INVALID_PARAM   -3
> +#define SBI_ERR_DENIED          -4
> +#define SBI_ERR_INVALID_ADDRESS -5

Parentheses around all the negative numbers please.

> +#define SBI_SPEC_VERSION_DEFAULT        0x1
> +
>  struct sbiret {
>      long error;
>      long value;
> @@ -31,4 +63,29 @@ struct sbiret sbi_ecall(unsigned long ext, unsigned long 
> fid,
>   */
>  void sbi_console_putchar(int ch);
>  
> +/*
> + * Check underlying SBI implementation has RFENCE
> + *
> + * @return 1 for supported AND 0 for not-supported

In which case ...

> + */
> +int sbi_has_rfence(void);

... bool please.

> +/*
> + * Send SFENCE_VMA to a set of target HARTs.
> + *
> + * @param hart_mask mask representing set of target HARTs
> + * @param start virtual address start
> + * @param size virtual address size

Are these really virtual addresses, not somehow a bias and a number
of bits (CPUs) or elements? From the rest of the patch I can't deduce
what these two parameters express.

> + */
> +void sbi_remote_sfence_vma(const unsigned long *hart_mask,

Maybe better hart_mask[]? It's not clear to me though what the upper
bound of the array is.

> @@ -38,7 +48,253 @@ struct sbiret sbi_ecall(unsigned long ext, unsigned long 
> fid,
>      return ret;
>  }
>  
> +static int sbi_err_map_xen_errno(int err)
> +{
> +    switch ( err )
> +    {
> +    case SBI_SUCCESS:
> +        return 0;
> +    case SBI_ERR_DENIED:
> +        return -EACCES;
> +    case SBI_ERR_INVALID_PARAM:
> +        return -EINVAL;
> +    case SBI_ERR_INVALID_ADDRESS:
> +        return -EFAULT;
> +    case SBI_ERR_NOT_SUPPORTED:
> +    case SBI_ERR_FAILURE:
> +    default:
> +        return -EOPNOTSUPP;

Mapping FAILURE to -EOPNOTSUPP may end up confusing.

> +    };
> +}
> +
>  void sbi_console_putchar(int ch)
>  {
>      sbi_ecall(SBI_EXT_0_1_CONSOLE_PUTCHAR, 0, ch, 0, 0, 0, 0, 0);
>  }
> +
> +static unsigned long sbi_major_version(void)
> +{
> +    return (sbi_spec_version >> SBI_SPEC_VERSION_MAJOR_SHIFT) &
> +        SBI_SPEC_VERSION_MAJOR_MASK;
> +}
> +
> +static unsigned long sbi_minor_version(void)
> +{
> +    return sbi_spec_version & SBI_SPEC_VERSION_MINOR_MASK;
> +}
> +
> +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)

Nit: Blanks missing.

> +        return ret.value;
> +    else
> +        return ret.error;
> +}
> +
> +static void sbi_cpumask_to_hartmask(const struct cpumask *cmask,
> +                 struct cpumask *hmask)

I doubt it is valud to re-use struct cpumask for hart maps.

> +{
> +    u32 cpu;

uint32_t or yet better unsigned int please.

> +    unsigned long hart = INVALID_HARTID;
> +
> +    if (!cmask || !hmask)
> +        return;
> +
> +    cpumask_clear(hmask);
> +    for_each_cpu(cpu, cmask)
> +    {
> +        if ( CONFIG_NR_CPUS <= cpu )
> +        {
> +            printk(XENLOG_ERR "SBI: invalid hart=%lu for cpu=%d\n",

%u for the CPU please.

> +                     hart, cpu);

"hart" wasn't set yet and hence is invalid or at least misleading to
log.

Nit: Indentation.

> +            continue;

Can you really sensibly continue in such a case?

> +        }
> +
> +        hart = cpuid_to_hartid_map(pcpu_info[cpu].processor_id);

What does "_map" in the function/macro name signify?

> +        cpumask_set_cpu(hart, hmask);
> +    }
> +}
> +
> +static int __sbi_rfence_v02_real(unsigned long fid,
> +                 unsigned long hmask, unsigned long hbase,
> +                 unsigned long start, unsigned long size,
> +                 unsigned long arg4)

Nit: Indentation (more similar issues elsewhere).

> +{
> +    struct sbiret ret = {0};
> +    int result = 0;
> +
> +    switch ( fid )
> +    {
> +    case SBI_EXT_RFENCE_REMOTE_FENCE_I:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                0, 0, 0, 0);
> +        break;
> +    case SBI_EXT_RFENCE_REMOTE_SFENCE_VMA:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                start, size, 0, 0);
> +        break;
> +    case SBI_EXT_RFENCE_REMOTE_SFENCE_VMA_ASID:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                start, size, arg4, 0);
> +        break;
> +    case SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                start, size, 0, 0);
> +        break;
> +    case SBI_EXT_RFENCE_REMOTE_HFENCE_GVMA_VMID:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                start, size, arg4, 0);
> +        break;
> +    case SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                start, size, 0, 0);
> +        break;
> +    case SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID:
> +        ret = sbi_ecall(SBI_EXT_RFENCE, fid, hmask, hbase,
> +                start, size, arg4, 0);
> +        break;
> +
> +    default:
> +        printk("%s: unknown function ID [%lu]\n",
> +               __func__, fid);
> +        result = -EINVAL;
> +        break;
> +    };
> +
> +    if ( ret.error )
> +    {
> +        result = sbi_err_map_xen_errno(ret.error);
> +        printk("%s: hbase=%lu hmask=0x%lx failed (error %d)\n",

%#lx (also elsewhere)

> +               __func__, hbase, hmask, result);
> +    }
> +
> +    return result;
> +}
> +
> +static int __sbi_rfence_v02(unsigned long fid,
> +                const unsigned long *hart_mask,
> +                unsigned long start, unsigned long size,
> +                unsigned long arg4, unsigned long arg5)
> +{
> +    struct cpumask tmask;
> +    unsigned long hart, hmask, hbase;
> +    int result;
> +
> +    if (!hart_mask)

Nit: Style (and more below). And I'm afraid I'm tired of making such
remarks. Please check style yourself before submitting.

> +static int sbi_spec_is_0_1(void)
> +{
> +    return (sbi_spec_version == SBI_SPEC_VERSION_DEFAULT) ? 1 : 0;

bool and no conditional operator

Jan