On Fri, Nov 1, 2024 at 11:49 AM Peter Maydell <[email protected]> wrote: > > On Fri, 1 Nov 2024 at 18:13, Daniel Henrique Barboza > <[email protected]> wrote: > > > > (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; > > } > > 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
