| Message ID | 20241104222225.1523751-1-pierrick.bouvier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] hw/riscv: fix build error with clang | expand |
On Tue, Nov 5, 2024 at 8:23 AM Pierrick Bouvier <pierrick.bouvier@linaro.org> 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) > > | ^ > > After a conversation on the mailing list, it was decided to rename and > add a comment for this function. > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/riscv/riscv-iommu.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c > index feb650549ac..12f01a75f5d 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) > { > uint64_t ret = 0; > uint64_t rot = 1; > @@ -528,7 +545,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 = riscv_iommu_pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); > if (intn >= 256) { > /* Interrupt file number out of range */ > res = MEMTX_ACCESS_ERROR; > -- > 2.39.5 > >
On 2024/11/5 06:22, 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) > > | ^ > > After a conversation on the mailing list, it was decided to rename and > add a comment for this function. > > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com> Zhiwei > --- > hw/riscv/riscv-iommu.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c > index feb650549ac..12f01a75f5d 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) > { > uint64_t ret = 0; > uint64_t rot = 1; > @@ -528,7 +545,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 = riscv_iommu_pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); > if (intn >= 256) { > /* Interrupt file number out of range */ > res = MEMTX_ACCESS_ERROR;
Thanks for the review. Feel free to pull the patch in your next PR, so it can be available for release 9.2. Regards, Pierrick On 11/4/24 18:37, Alistair Francis wrote: > On Tue, Nov 5, 2024 at 8:23 AM Pierrick Bouvier > <pierrick.bouvier@linaro.org> 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) >> >> | ^ >> >> After a conversation on the mailing list, it was decided to rename and >> add a comment for this function. >> >> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Alistair > >> --- >> hw/riscv/riscv-iommu.c | 23 ++++++++++++++++++++--- >> 1 file changed, 20 insertions(+), 3 deletions(-) >> >> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c >> index feb650549ac..12f01a75f5d 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) >> { >> uint64_t ret = 0; >> uint64_t rot = 1; >> @@ -528,7 +545,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 = riscv_iommu_pext_u64(PPN_DOWN(gpa), ctx->msi_addr_mask); >> if (intn >= 256) { >> /* Interrupt file number out of range */ >> res = MEMTX_ACCESS_ERROR; >> -- >> 2.39.5 >> >>
On 5/11/24 05:29, Pierrick Bouvier wrote: > Thanks for the review. > Feel free to pull the patch in your next PR, so it can be available for > release 9.2. > > Regards, > Pierrick > > On 11/4/24 18:37, Alistair Francis wrote: >> On Tue, Nov 5, 2024 at 8:23 AM Pierrick Bouvier >> <pierrick.bouvier@linaro.org> 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) >>> >>> | ^ >>> >>> After a conversation on the mailing list, it was decided to rename and >>> add a comment for this function. >>> >>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Fix queued via hw-misc tree.
On 11/5/24 7:55 PM, Philippe Mathieu-Daudé wrote: > On 5/11/24 05:29, Pierrick Bouvier wrote: >> Thanks for the review. >> Feel free to pull the patch in your next PR, so it can be available for release 9.2. >> >> Regards, >> Pierrick >> >> On 11/4/24 18:37, Alistair Francis wrote: >>> On Tue, Nov 5, 2024 at 8:23 AM Pierrick Bouvier >>> <pierrick.bouvier@linaro.org> 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) >>>> >>>> | ^ >>>> >>>> After a conversation on the mailing list, it was decided to rename and >>>> add a comment for this function. >>>> >>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> >>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > > Fix queued via hw-misc tree. Do you fancy taking the riscv-iommu Coverity fixes as well? They're somewhat trivial and it'll spare Alistair from making a PR with just a handful of patches. Thanks, Daniel
diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c index feb650549ac..12f01a75f5d 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) { uint64_t ret = 0; uint64_t rot = 1; @@ -528,7 +545,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 = riscv_iommu_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) | ^ After a conversation on the mailing list, it was decided to rename and add a comment for this function. Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- hw/riscv/riscv-iommu.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)