On 14.03.2024 17:48, Oleksii wrote:
> On Thu, 2024-03-14 at 15:30 +0100, Jan Beulich wrote:
>> On 13.03.2024 18:27, Andrew Cooper wrote:
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -641,7 +641,7 @@ struct page_info *iommu_alloc_pgtable(struct
>>> domain_iommu *hd,
>>> if ( contig_mask )
>>> {
>>> /* See pt-contig-markers.h for a description of the marker
>>> scheme. */
>>> - unsigned int i, shift = find_first_set_bit(contig_mask);
>>> + unsigned int i, shift = ffsl(contig_mask) - 1;
>>
>> The need for subtracting 1 is why personally I dislike ffs() / ffsl()
>> (and
>> why I think find_first_set_bit() and __ffs() (but no __ffsl()) were
>> introduced).
>>
>> But what I first of all would like to have clarification on is what
>> your
>> (perhaps just abstract at this point) plans are wrt ffz() / ffzl().
>> Potential side-by-side uses would be odd now, and would continue to
>> be odd
>> if the difference in bit labeling was retained. Since we're switching
>> to
>> a consolidated set of basic helpers, such an anomaly would better not
>> survive imo.
> Right now, ffz() is defined as __ffs(~(x)), so and it seems to me
> __ffs()/ffz() exist only as a Linux compatible, so I wanted as a part
> of RISC-V patch series put into xen/linux-compat.h and just include
> this header where it will be necessary:
>
> #define __ffs(x) (ffs(~(x)) - 1)
> #define ffz(x) __ffs(~(x))
Well, right now ffz() is used in just a single file. One option therefore
would be to not have it available generally, and - as you say - if need
be supply it in linux-compat.h. Another option would be to have something
along its lines generally available, if we deem it useful.
> Why should we care about ffzl()? It is not used in Xen, is it?
I find it odd to have ffs() and ffsl(), but then ffz() without ffzl().
That's not my understanding of a generally useful (and largely free of
surprises) set of library routines.
Jan
