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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-07-30 at 11:17 +0200, Jan Beulich wrote:
> 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.
What word "biased" mean here?

> 
> > > > +
> > > > +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).
It is guarantee that hart ID 0 will be present but not that they are
numbered contiguously:
```
Hart IDs might
not necessarily be numbered contiguously in a multiprocessor system,
but at least one hart must
have a hart ID of zero. Hart IDs must be unique within the execution
environment.
```
I have to rework then this things around sbi_cpumask_to_hartmask().

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

> > > > +            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?
Agree, the "_map" in cpuid_to_hartid_map() is useless. Thanks.

~ Oleksii