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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2024 10:44, oleksii.kurochko@xxxxxxxxx wrote:
> 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

Oh, so these are describing the VA range to be flushed. Okay.

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

Biased by hart_mask_base in the actual SBI call.

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

See my comment elsewhere about possibly sparse numbering of hart IDs.

>>> +
>>> +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?

It's not only better, but imo a requirement. Unless there's a guarantee
by the spec that hart IDs for any subset of harts are sequential and
starting from 0, you just can't assume they fall in the [0,NR_CPUS) or
really [0,nr_cpu_ids) range. Yet without that you simply can't (ab)use
struct cpumask (and btw it wants to be cpumask_t everywhere).

You may want to take a look at struct physid_mask that we have on x86
for the equivalent purpose.

>>> +{
>>> +    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?

Yes. Plus the problem in this case isn't that there's no hart ID, but
that the CPU ID is out of range. That's what the message wants to say.
If such a check and message are necessary in the first place. I don't
think we have anything like that on x86. And for_each_cpu() also can't
possibly produce any ID at or beyond nr_cpu_ids, where nr_cpu_ids <=
NR_CPUS.

>>> +            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.

Again - your problem here isn't the range of "cpu". What you instead
want to check is ...

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

the hart ID that you get back. If that's INVALID_HARTID, you're in
fact in trouble.

>> What does "_map" in the function/macro name signify?
> It is interconnections/correllation between Xen's CPU id and Hart's id.

Well. What the function does internally is consult the map. Yet that's
not relevant to any caller? They only care about the translation from
one ID space to the other? No matter whether a map, radix tree, rbtree,
or what not is used behind the scenes?

Jan