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

[Tomasz Jeznach]

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

Hope this helps,
- Tomasz

> thanks
> -- PMM