[PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Natanael Copa]

strerrorname_np is non-portable and breaks building with musl libc.

Use strerror(errno) instead, like we do other places.

Cc: qemu-stable@nongnu.org
Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
 target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 45b6cf1cfa..117e33cf90 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
                 multi_ext_cfg->supported = false;
                 val = false;
             } else {
-                error_report("Unable to read ISA_EXT KVM register %s, "
-                             "error code: %s", multi_ext_cfg->name,
-                             strerrorname_np(errno));
+                error_report("Unable to read ISA_EXT KVM register %s: %s",
+                             multi_ext_cfg->name, strerror(errno));
                 exit(EXIT_FAILURE);
             }
         } else {
@@ -895,8 +894,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
          *
          * Error out if we get any other errno.
          */
-        error_report("Error when accessing get-reg-list, code: %s",
-                     strerrorname_np(errno));
+        error_report("Error when accessing get-reg-list: %s",
+                     strerror(errno));
         exit(EXIT_FAILURE);
     }
 
@@ -905,8 +904,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
     reglist->n = rl_struct.n;
     ret = ioctl(kvmcpu->cpufd, KVM_GET_REG_LIST, reglist);
     if (ret) {
-        error_report("Error when reading KVM_GET_REG_LIST, code %s ",
-                     strerrorname_np(errno));
+        error_report("Error when reading KVM_GET_REG_LIST: %s",
+                     strerror(errno));
         exit(EXIT_FAILURE);
     }
 
@@ -927,9 +926,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
         if (ret != 0) {
-            error_report("Unable to read ISA_EXT KVM register %s, "
-                         "error code: %s", multi_ext_cfg->name,
-                         strerrorname_np(errno));
+            error_report("Unable to read ISA_EXT KVM register %s: %s",
+                         multi_ext_cfg->name, strerror(errno));
             exit(EXIT_FAILURE);
         }
 
-- 
2.43.0

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Daniel Henrique Barboza]

On 12/18/23 13:22, Natanael Copa wrote:
> strerrorname_np is non-portable and breaks building with musl libc.
> 
> Use strerror(errno) instead, like we do other places.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
> Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---
>   target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 45b6cf1cfa..117e33cf90 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>                   multi_ext_cfg->supported = false;
>                   val = false;
>               } else {
> -                error_report("Unable to read ISA_EXT KVM register %s, "
> -                             "error code: %s", multi_ext_cfg->name,
> -                             strerrorname_np(errno));
> +                error_report("Unable to read ISA_EXT KVM register %s: %s",
> +                             multi_ext_cfg->name, strerror(errno));


The reason I did this change, as described in 082e9e4a58ba mentioned in the commit
message, was precisely to avoid things like this:

qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error: no such file or directory
  
The generic description of the error works well with file descriptors and so on but it's
weird in the KVM context. This patch is re-introducing it.

If strerrorname_np() is non-portable I believe we're better off dealing with the numeric
errno than with its generic description. I.e:


> +                error_report("Unable to read ISA_EXT KVM register %s, error %d",
> +                             multi_ext_cfg->name, errno);


Same with the other 3 instances you changed in the patch. Thanks,


Daniel



>                   exit(EXIT_FAILURE);
>               }
>           } else {
> @@ -895,8 +894,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>            *
>            * Error out if we get any other errno.
>            */
> -        error_report("Error when accessing get-reg-list, code: %s",
> -                     strerrorname_np(errno));
> +        error_report("Error when accessing get-reg-list: %s",
> +                     strerror(errno));
>           exit(EXIT_FAILURE);
>       }
>   
> @@ -905,8 +904,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>       reglist->n = rl_struct.n;
>       ret = ioctl(kvmcpu->cpufd, KVM_GET_REG_LIST, reglist);
>       if (ret) {
> -        error_report("Error when reading KVM_GET_REG_LIST, code %s ",
> -                     strerrorname_np(errno));
> +        error_report("Error when reading KVM_GET_REG_LIST: %s",
> +                     strerror(errno));
>           exit(EXIT_FAILURE);
>       }
>   
> @@ -927,9 +926,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>           reg.addr = (uint64_t)&val;
>           ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
>           if (ret != 0) {
> -            error_report("Unable to read ISA_EXT KVM register %s, "
> -                         "error code: %s", multi_ext_cfg->name,
> -                         strerrorname_np(errno));
> +            error_report("Unable to read ISA_EXT KVM register %s: %s",
> +                         multi_ext_cfg->name, strerror(errno));
>               exit(EXIT_FAILURE);
>           }
>   

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Peter Maydell]

On Mon, 18 Dec 2023 at 17:22, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 12/18/23 13:22, Natanael Copa wrote:
> > strerrorname_np is non-portable and breaks building with musl libc.
> >
> > Use strerror(errno) instead, like we do other places.
> >
> > Cc: qemu-stable@nongnu.org
> > Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
> > Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
> > Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> > ---
> >   target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
> >   1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index 45b6cf1cfa..117e33cf90 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
> >                   multi_ext_cfg->supported = false;
> >                   val = false;
> >               } else {
> > -                error_report("Unable to read ISA_EXT KVM register %s, "
> > -                             "error code: %s", multi_ext_cfg->name,
> > -                             strerrorname_np(errno));
> > +                error_report("Unable to read ISA_EXT KVM register %s: %s",
> > +                             multi_ext_cfg->name, strerror(errno));
>
>
> The reason I did this change, as described in 082e9e4a58ba mentioned in the commit
> message, was precisely to avoid things like this:
>
> qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error: no such file or directory
>
> The generic description of the error works well with file descriptors and so on but it's
> weird in the KVM context. This patch is re-introducing it.

We don't seem to worry about that in any of the other
KVM code -- accel/kvm/ has lots of places that
use strerror() or error_setg_errno().

thanks
-- PMM

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Daniel Henrique Barboza]

On 12/18/23 14:53, Peter Maydell wrote:
> On Mon, 18 Dec 2023 at 17:22, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 12/18/23 13:22, Natanael Copa wrote:
>>> strerrorname_np is non-portable and breaks building with musl libc.
>>>
>>> Use strerror(errno) instead, like we do other places.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
>>> Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
>>> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
>>> ---
>>>    target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
>>>    1 file changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>>> index 45b6cf1cfa..117e33cf90 100644
>>> --- a/target/riscv/kvm/kvm-cpu.c
>>> +++ b/target/riscv/kvm/kvm-cpu.c
>>> @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>>>                    multi_ext_cfg->supported = false;
>>>                    val = false;
>>>                } else {
>>> -                error_report("Unable to read ISA_EXT KVM register %s, "
>>> -                             "error code: %s", multi_ext_cfg->name,
>>> -                             strerrorname_np(errno));
>>> +                error_report("Unable to read ISA_EXT KVM register %s: %s",
>>> +                             multi_ext_cfg->name, strerror(errno));
>>
>>
>> The reason I did this change, as described in 082e9e4a58ba mentioned in the commit
>> message, was precisely to avoid things like this:
>>
>> qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error: no such file or directory
>>
>> The generic description of the error works well with file descriptors and so on but it's
>> weird in the KVM context. This patch is re-introducing it.
> 
> We don't seem to worry about that in any of the other
> KVM code -- accel/kvm/ has lots of places that
> use strerror() or error_setg_errno().

I don't know how this is being used in other parts of accel/kvm, but in this particular
instance we're handling the errors from get_one_reg. The kernel docs describes the errors
the API may return as:

--------
Errors include:

ENOENT - no such register
EINVAL - invalid register ID, or no such register (...)
EPERM - (arm64) register access not allowed before vcpu finalization
---------


The API interprets ENOENT as "no such register", but strerror(errno) in this case will output
"no such file or directory". The generic description is forcing me to think "this error
makes no sense ... oh, this might be the description of ENOENT". At this point having an
"error code 2" instead is clearer to me.


Thanks,

Daniel

> 
> thanks
> -- PMM

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Peter Maydell]

On Mon, 18 Dec 2023 at 18:34, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
> On 12/18/23 14:53, Peter Maydell wrote:
> > On Mon, 18 Dec 2023 at 17:22, Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> > We don't seem to worry about that in any of the other
> > KVM code -- accel/kvm/ has lots of places that
> > use strerror() or error_setg_errno().
>
> I don't know how this is being used in other parts of accel/kvm, but in this particular
> instance we're handling the errors from get_one_reg.

Yep; compare accel/kvm/kvm-all.c:kvm_get_one_reg(), which does:

    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
    if (r) {
        trace_kvm_failed_reg_get(id, strerror(-r));
    }

-- PMM

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Michael Tokarev]

18.12.2023 20:20, Daniel Henrique Barboza wrote:
> 
> 
> On 12/18/23 13:22, Natanael Copa wrote:
>> strerrorname_np is non-portable and breaks building with musl libc.
>>
>> Use strerror(errno) instead, like we do other places.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
>> Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
>> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
>> ---
>>   target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>> index 45b6cf1cfa..117e33cf90 100644
>> --- a/target/riscv/kvm/kvm-cpu.c
>> +++ b/target/riscv/kvm/kvm-cpu.c
>> @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>>                   multi_ext_cfg->supported = false;
>>                   val = false;
>>               } else {
>> -                error_report("Unable to read ISA_EXT KVM register %s, "
>> -                             "error code: %s", multi_ext_cfg->name,
>> -                             strerrorname_np(errno));
>> +                error_report("Unable to read ISA_EXT KVM register %s: %s",
>> +                             multi_ext_cfg->name, strerror(errno));
> 
> 
> The reason I did this change, as described in 082e9e4a58ba mentioned in the commit
> message, was precisely to avoid things like this:
> 
> qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error: no such file or directory

If KVM context puts its own unique meaning for ENOENT, maybe something like

  "unable to read KVM register: %s\n", errno == ENOENT ? "no such register" : strerror(errno)

would do it better?

To me, "No such file or directory" already tells everything and does not look
weird, but that's because I've seen this error message for all sorts of contexts
and got used to this. It is definitely understandable.

/mjt

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Daniel Henrique Barboza]

On 12/18/23 18:35, Michael Tokarev wrote:
> 18.12.2023 20:20, Daniel Henrique Barboza wrote:
>>
>>
>> On 12/18/23 13:22, Natanael Copa wrote:
>>> strerrorname_np is non-portable and breaks building with musl libc.
>>>
>>> Use strerror(errno) instead, like we do other places.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
>>> Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
>>> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
>>> ---
>>>   target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
>>>   1 file changed, 8 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
>>> index 45b6cf1cfa..117e33cf90 100644
>>> --- a/target/riscv/kvm/kvm-cpu.c
>>> +++ b/target/riscv/kvm/kvm-cpu.c
>>> @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>>>                   multi_ext_cfg->supported = false;
>>>                   val = false;
>>>               } else {
>>> -                error_report("Unable to read ISA_EXT KVM register %s, "
>>> -                             "error code: %s", multi_ext_cfg->name,
>>> -                             strerrorname_np(errno));
>>> +                error_report("Unable to read ISA_EXT KVM register %s: %s",
>>> +                             multi_ext_cfg->name, strerror(errno));
>>
>>
>> The reason I did this change, as described in 082e9e4a58ba mentioned in the commit
>> message, was precisely to avoid things like this:
>>
>> qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error: no such file or directory
> 
> If KVM context puts its own unique meaning for ENOENT, maybe something like
> 
>   "unable to read KVM register: %s\n", errno == ENOENT ? "no such register" : strerror(errno)
> 
> would do it better?


A solution like this is something I can go after if I'm bothered enough with how strerror()
is working in the RISC-V KVM driver.

For now I think we can live with this fix as is since fixing the build is more important
that aesthetics.


Thanks,

Daniel

> 
> To me, "No such file or directory" already tells everything and does not look
> weird, but that's because I've seen this error message for all sorts of contexts
> and got used to this. It is definitely understandable.
> 
> /mjt

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Daniel Henrique Barboza]

On 12/18/23 13:22, Natanael Copa wrote:
> strerrorname_np is non-portable and breaks building with musl libc.
> 
> Use strerror(errno) instead, like we do other places.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: commit 082e9e4a58ba (target/riscv/kvm: improve 'init_multiext_cfg' error msg)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2041
> Buglink: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15541
> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---

Apart from my 'aesthetic preference' of using "error code %d" instead of
strerror(errno), which I stand by, this patch is fixing a build break
and it's an improvement from what we have now. Aesthetics can be dealt
with later.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>




>   target/riscv/kvm/kvm-cpu.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 45b6cf1cfa..117e33cf90 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -832,9 +832,8 @@ static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
>                   multi_ext_cfg->supported = false;
>                   val = false;
>               } else {
> -                error_report("Unable to read ISA_EXT KVM register %s, "
> -                             "error code: %s", multi_ext_cfg->name,
> -                             strerrorname_np(errno));
> +                error_report("Unable to read ISA_EXT KVM register %s: %s",
> +                             multi_ext_cfg->name, strerror(errno));
>                   exit(EXIT_FAILURE);
>               }
>           } else {
> @@ -895,8 +894,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>            *
>            * Error out if we get any other errno.
>            */
> -        error_report("Error when accessing get-reg-list, code: %s",
> -                     strerrorname_np(errno));
> +        error_report("Error when accessing get-reg-list: %s",
> +                     strerror(errno));
>           exit(EXIT_FAILURE);
>       }
>   
> @@ -905,8 +904,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>       reglist->n = rl_struct.n;
>       ret = ioctl(kvmcpu->cpufd, KVM_GET_REG_LIST, reglist);
>       if (ret) {
> -        error_report("Error when reading KVM_GET_REG_LIST, code %s ",
> -                     strerrorname_np(errno));
> +        error_report("Error when reading KVM_GET_REG_LIST: %s",
> +                     strerror(errno));
>           exit(EXIT_FAILURE);
>       }
>   
> @@ -927,9 +926,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
>           reg.addr = (uint64_t)&val;
>           ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, &reg);
>           if (ret != 0) {
> -            error_report("Unable to read ISA_EXT KVM register %s, "
> -                         "error code: %s", multi_ext_cfg->name,
> -                         strerrorname_np(errno));
> +            error_report("Unable to read ISA_EXT KVM register %s: %s",
> +                         multi_ext_cfg->name, strerror(errno));
>               exit(EXIT_FAILURE);
>           }
>   

Re: [PATCH] target/riscv/kvm: do not use non-portable strerrorname_np()

[Michael Tokarev]

18.12.2023 19:22, Natanael Copa:
> strerrorname_np is non-portable and breaks building with musl libc.
> 
> Use strerror(errno) instead, like we do other places.

Applied to the trivial-patches tree, finally..

/mjt