(Ccing Tomasz)
On 11/1/24 2:48 PM, Peter Maydell wrote:
On Fri, 1 Nov 2024 at 17:36, Daniel Henrique Barboza
<[email protected]> 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 <[email protected]>
---
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;
}
Taking a look at bitops.h I'm not sure if we have a function that does that.
Perhaps
we have a similar function that does that in another helper somewhere. I
wouldn't
oppose using a common helper if available.
In fact I think this is not the first time we're having this talk, i.e. RISC-V
code
using exclusive bitops/mask functions. target/riscv/cpu.c has a lot of these
cases.
There's a whole infrastructure that ARM uses that handles regs operations that
we
could use in that file, for example.
I can take a look at how we can standardize existing RISC-V code to use common
helpers,
creating more common helpers if needed. Would be a 10.0 work since we're too
late
for 9.2.
Thanks,
Daniel
thanks
-- PMM
