| Message ID | 20241101170833.1074954-1-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/riscv: fix build error with clang | expand |
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. Thanks, Daniel > { > uint64_t ret = 0; > uint64_t rot = 1; > @@ -528,7 +528,7 @@ static MemTxResult riscv_iommu_msi_write(RISCVIOMMUState *s, > int cause; > > /* Interrupt File Number */ > - intn = _pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); > + intn = pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); > if (intn >= 256) { > /* Interrupt file number out of range */ > res = MEMTX_ACCESS_ERROR;
On 11/1/24 2:35 PM, Daniel Henrique Barboza 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> I think you can also add a Fixes: 0c54acb824 ("hw/riscv: add RISC-V IOMMU base emulation") >> --- >> 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. > > > Thanks, > > Daniel > >> { >> uint64_t ret = 0; >> uint64_t rot = 1; >> @@ -528,7 +528,7 @@ static MemTxResult riscv_iommu_msi_write(RISCVIOMMUState *s, >> int cause; >> /* Interrupt File Number */ >> - intn = _pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); >> + intn = pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); >> if (intn >= 256) { >> /* Interrupt file number out of range */ >> res = MEMTX_ACCESS_ERROR;
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 ? thanks -- PMM
On 11/1/24 10:48, 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 ? > Adding Tomasz (author of this change) and Alistair, who might have an opinion on this. > thanks > -- PMM
On 11/1/24 10:35, Daniel Henrique Barboza 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. > Looks good to me. I'll wait to see if Tomasz or Alistair want to revisit this before pushing a v2. > > Thanks, > > Daniel > >> { >> uint64_t ret = 0; >> uint64_t rot = 1; >> @@ -528,7 +528,7 @@ static MemTxResult riscv_iommu_msi_write(RISCVIOMMUState *s, >> int cause; >> >> /* Interrupt File Number */ >> - intn = _pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); >> + intn = pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); >> if (intn >= 256) { >> /* Interrupt file number out of range */ >> res = MEMTX_ACCESS_ERROR;
(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
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? thanks -- PMM
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
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. Thanks, Daniel > > Hope this helps, > - Tomasz > >> thanks >> -- PMM
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
On Fri, 1 Nov 2024 at 20:46, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote: > 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) That looks great, thanks. -- PMM
On 11/1/24 10:08, 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) > { > uint64_t ret = 0; > uint64_t rot = 1; > @@ -528,7 +528,7 @@ static MemTxResult riscv_iommu_msi_write(RISCVIOMMUState *s, > int cause; > > /* Interrupt File Number */ > - intn = _pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); > + intn = pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); > if (intn >= 256) { > /* Interrupt file number out of range */ > res = MEMTX_ACCESS_ERROR; Sent v2 with changes asked in this thread.
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) { uint64_t ret = 0; uint64_t rot = 1; @@ -528,7 +528,7 @@ static MemTxResult riscv_iommu_msi_write(RISCVIOMMUState *s, int cause; /* Interrupt File Number */ - intn = _pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); + intn = pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); if (intn >= 256) { /* Interrupt file number out of range */ res = MEMTX_ACCESS_ERROR;
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(-)