qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pierrick Bouvier
Subject: Re: [PATCH] hw/riscv: fix build error with clang
Date: Fri, 1 Nov 2024 13:52:20 -0700
User-agent: Mozilla Thunderbird

On 11/1/24 13:46, Daniel Henrique Barboza wrote:


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.


Sure!

Peter, is this compromise satisfying for you?


Thanks,

Daniel



Hope this helps,
- Tomasz

thanks
-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]