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

[Daniel Henrique Barboza]

(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;
}

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