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;
-- 
2.39.5

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.

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

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

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