Re: [PATCH] hw/riscv: fix build error with clang

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 11/1/24 4:23 PM, Tomasz Jeznach wrote:
> On Fri, Nov 1, 2024 at 11:49 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 1 Nov 2024 at 18:13, Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> (Ccing Tomasz)
>>>
>>> On 11/1/24 2:48 PM, Peter Maydell wrote:
>>>> On Fri, 1 Nov 2024 at 17:36, Daniel Henrique Barboza
>>>> <dbarboza@ventanamicro.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 11/1/24 2:08 PM, Pierrick Bouvier wrote:
>>>>>> Introduced in 0c54ac, "hw/riscv: add RISC-V IOMMU base emulation"
>>>>>>
>>>>>> ../hw/riscv/riscv-iommu.c:187:17: error: redefinition of '_pext_u64'
>>>>>>
>>>>>>      187 | static uint64_t _pext_u64(uint64_t val, uint64_t ext)
>>>>>>
>>>>>>          |                 ^
>>>>>>
>>>>>> D:/a/_temp/msys64/clang64/lib/clang/18/include/bmi2intrin.h:217:1: note: previous definition is here
>>>>>>
>>>>>>      217 | _pext_u64(unsigned long long __X, unsigned long long __Y)
>>>>>>
>>>>>>          | ^
>>>>>>
>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>> ---
>>>>>>     hw/riscv/riscv-iommu.c | 4 ++--
>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>>>>>> index feb650549ac..f738570bac2 100644
>>>>>> --- a/hw/riscv/riscv-iommu.c
>>>>>> +++ b/hw/riscv/riscv-iommu.c
>>>>>> @@ -184,7 +184,7 @@ static void riscv_iommu_pri(RISCVIOMMUState *s,
>>>>>>     }
>>>>>>
>>>>>>     /* Portable implementation of pext_u64, bit-mask extraction. */
>>>>>> -static uint64_t _pext_u64(uint64_t val, uint64_t ext)
>>>>>> +static uint64_t pext_u64(uint64_t val, uint64_t ext)
>>>>>
>>>>> I suggest name it 'riscv_iommu_pext_u64' to be clear that this is a local scope function,
>>>>> not to be mistaken with anything available in clang or any other compiler.
>>>>
>>>> More generally, we should avoid using leading '_' in QEMU function
>>>> names; those are reserved for the system.
>>>>
>>>> Also, what does this function do? The comment assumes that
>>>> the reader knows what a "pext_u64" function does, but if you
>>>> don't then it's fairly inscrutable bit-twiddling.
>>>> "bit-mask extraction" suggests maybe we should be using
>>>> the bitops.h extract functions instead ?
>>>
>>> This is the function:
>>>
>>>
>>> /* Portable implementation of pext_u64, bit-mask extraction. */
>>> static uint64_t _pext_u64(uint64_t val, uint64_t ext)
>>> {
>>>       uint64_t ret = 0;
>>>       uint64_t rot = 1;
>>>
>>>       while (ext) {
>>>           if (ext & 1) {
>>>               if (val & 1) {
>>>                   ret |= rot;
>>>               }
>>>               rot <<= 1;
>>>           }
>>>           val >>= 1;
>>>           ext >>= 1;
>>>       }
>>>
>>>       return ret;
>>> }
>>
>> Yes, but what does it actually *do* ? :-)  Presumably
>> it extracts some subpart of 'val' based on 'ext', but
>> what is the format it expects 'ext' to be in, and what
>> kinds of input are valid?
>>
>> For comparison, our extract64 function says:
>>
>>   * extract64:
>>   * @value: the value to extract the bit field from
>>   * @start: the lowest bit in the bit field (numbered from 0)
>>   * @length: the length of the bit field
>>   *
>>   * Extract from the 64 bit input @value the bit field specified by the
>>   * @start and @length parameters, and return it. The bit field must
>>   * lie entirely within the 64 bit word. It is valid to request that
>>   * all 64 bits are returned (ie @length 64 and @start 0).
>>
>> so even if you haven't come across it before you can see
>> what the function is intended to do, what inputs are valid
>> and what are not, and so on, and you don't need to try to
>> reverse-engineer those from the bit operations.
>>
>> I'm not necessarily opposed to having separate implementations
>> of these things if it means the code follows the architectural
>> specifications more closely, but if we do have them can
>> we have documentation comments that describe the behaviour?
>>
> 
> Hey,
> 
> Thank you for the fix. Using a common name and underscore was not a good idea ;)
> The function is an implementation of the bit extraction function as
> documented in RISC-V IOMMU Spec [1], section '2.3.3. Process to
> translate addresses of MSIs'.
> 
> It is also known as PEXT instruction in x86/AVX2 architecture, for
> non-contiguous bits extraction, that's why I've used this name, as it
> might be more familiar to the readers, and to avoid confusion with
> existing extract64() function in bitops.h.
> 
> 
> [1] link: https://github.com/riscv-non-isa/riscv-iommu/releases/tag/v1.0.0

Thanks for clarifying!


Pierrick, I copied the description of this function from the riscv-isa spec
and put in a comment. This is how it would look like:

$ git diff
diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
index feb650549a..969eb56c53 100644
--- a/hw/riscv/riscv-iommu.c
+++ b/hw/riscv/riscv-iommu.c
@@ -183,8 +183,25 @@ static void riscv_iommu_pri(RISCVIOMMUState *s,
      }
  }
  
-/* Portable implementation of pext_u64, bit-mask extraction. */
-static uint64_t _pext_u64(uint64_t val, uint64_t ext)
+/*
+ * Discards all bits from 'val' whose matching bits in the same
+ * positions in the mask 'ext' are zeros, and packs the remaining
+ * bits from 'val' contiguously at the least-significant end of the
+ * result, keeping the same bit order as 'val' and filling any
+ * other bits at the most-significant end of the result with zeros.
+ *
+ * For example, for the following 'val' and 'ext', the return 'ret'
+ * will be:
+ *
+ * val = a b c d e f g h
+ * ext = 1 0 1 0 0 1 1 0
+ * ret = 0 0 0 0 a c f g
+ *
+ * This function, taken from the riscv-iommu 1.0 spec, section 2.3.3
+ * "Process to translate addresses of MSIs", is similar to bit manip
+ * function PEXT (Parallel bits extract) from x86.
+ */
+static uint64_t riscv_iommu_pext_u64(uint64_t val, uint64_t ext)



Feel free to take this diff and squash it in your v2. This way we'll fix the clang
issue and put some docs in the function, a 2 for 1.


Thanks,

Daniel


> 
> Hope this helps,
> - Tomasz
> 
>> thanks
>> -- PMM