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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-07-29 at 17:52 +0200, Jan Beulich wrote:
> On 24.07.2024 17:31, Oleksii Kurochko wrote:
> 
> 
> > +/*
> > + * 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.
According to SBI doc start is an virtual address:
https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-rfence.adoc?plain=1#L44

and hart_mask is:
• unsigned long hart_mask is a scalar bit-vector containing hartids


> 
> > + */
> > +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.
Theoretically it is ULONGMAX but we don't looking more then
CONFIG_NR_CPUS.

> 
> > +
> > +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.
Why not? Would it be better to use unsigned long *hmask?

> 
> > +{
> > +    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.
That why it will print INVALID_HARTID which user will identify as
invalid hartid.
Do you mean that there is no any sense to message "invalid hart=%lu" as
it is obviously invalid?

> 
> Nit: Indentation.
> 
> > +            continue;
> 
> 
> Can you really sensibly continue in such a case?
I think yes, the worst thing we will have is that the "bad" CPU won't
be used.
But it might be better to switch to BUG_ON() as if we are inised the
"if CONFIG_NR_CPUS <= cpu" then it could tell us that something went
wrong.


> 
> > +        }
> > +
> > +        hart = cpuid_to_hartid_map(pcpu_info[cpu].processor_id);
> 
> What does "_map" in the function/macro name signify?
It is interconnections/correllation between Xen's CPU id and Hart's id.


~ Oleksii