On Tue, 2024-07-30 at 11:17 +0200, Jan Beulich wrote: > On 30.07.2024 10:44, oleksii.kuroc...@gmail.com 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
